update grammar and tests to differentiate reserved tokens#437
Conversation
timostamm
left a comment
There was a problem hiding this comment.
I ran the added tests against cel-go, and can confirm that the expressions parse successfully - although I'm not sure that the conformance tests are suitable to cover them, see comments below.
| name: "struct_names" | ||
| description: "Check that reserved identifiers are permitted as struct names as long as they are not language keywords" | ||
| test { | ||
| name: "as" | ||
| description: "Check that `as` can be used as a struct name." | ||
| expr: "as{} || true" |
There was a problem hiding this comment.
Running these tests in cel-go at d8351df yields the error "unknown type: as" when planning the program (conformance_test.go:237).
So the expressions parse sucessfully, confirming that the identifiers are permitted in cel-go, but the planner expects a type as to be declared.
| name: "struct_fields" | ||
| description: "Check that reserved identifiers are permitted as struct fields as long as they are not language keywords" | ||
| test { | ||
| name: "as" | ||
| description: "Check that `as` can be used as a struct name." | ||
| expr: "a{ as: 1 } || true" |
There was a problem hiding this comment.
Same as the "struct_names" tests, these tests don't pass with cel-go because the planner expects a to be declared. They do parse successfully.
|
/gcbrun |
|
@hudlow I'm fine with the updates to the grammar in the spec file. Maybe separate out the conformance test changes into another PR? |
| } | ||
| } | ||
| section { | ||
| name: "struct_names" |
There was a problem hiding this comment.
We're going to restrict the use of identifiers in type names to declared types, which will render most of the struct_names tests moot. Perhaps remove this even though it's technically correct as it's not feasible to actually execute?
| test { | ||
| name: "as" | ||
| description: "Check that `as` can be used as a struct name." | ||
| expr: "a{ as: 1 } || true" |
There was a problem hiding this comment.
The recommendation would be to update the TestAllTypes proto with fields that use the reserved identifiers. We may need to enable backtick syntax in the CEL environment to support this case, but I believe it will work if we do.
|
Per suggestion, updated to separate out test changes |
|
/gcbrun |
fixes #433