-
Notifications
You must be signed in to change notification settings - Fork 742
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Distinguish between type syntax vs expression syntax in the parser #13671
Conversation
Test this change out locally with the following install scripts (Action run 8458462785) VSCode
Azure CLI
|
@@ -1037,13 +1039,13 @@ protected IEnumerable<Token> NewLines() | |||
} | |||
} | |||
|
|||
private IntegerLiteralSyntax NumericLiteral() | |||
private (Token literal, ulong value) NumericLiteral() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The grammar isn't changing, just the AST. So where previously we would have always used IntegerLiteralSyntax
, we now use IntegerLiteralSyntax
in a value expression and IntegerTypeLiteralSyntax
in a type expression. That way there's no ambiguity later in the compilation process as to whether this syntax node was part of a type or a value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
…13671) Resolves #13618 In order to make type determinations, the `DeclaredTypeManager` needs to know whether it is within type syntax or expression syntax. This distinction is meaningful for property access (e.g., `<array type>[*]` is legal within type syntax but not within expression syntax, whereas `<array value>[?0]` is legal within expression syntax but not within type syntax), variable access (variables within type syntax can refer to other types but not values, and variables within expression syntax can refer to values but not types), nullability control syntax, and literal value syntax. Prior to Bicep 0.26, the type manager was relying on internal state that assumed the AST would be visited in order. This resulted in unstable type assignments if the type of type syntax was requested out of order. In Bicep 0.26.54, this was replaced with a deterministic check that used the model's syntax hierarchy to determine whether a given syntax node was a descendant of the type declaration of an `output` or `parameter` statement or the assigned value of a `type` statement (indicating type syntax) or not (indicating expression syntax). However, querying the syntax hierarchy can throw an exception if a node was not part of the original model (e.g., if it was created by a `SyntaxRewriteVisitor`). This PR moves the responsibility for making this distinction to the parser. The parser is already tracking whether it is within type syntax or expression syntax, as the grammar for each syntax class is distinct. This PR introduces some new descendent classes of `TypeSyntax` that are minimally changed copies of descendent classes of `ExpressionSyntax`: `VariableAccessSyntax` is a descendent of `ExpressionSyntax`, and `TypeVariableAccessSyntax` is a descendent of `TypeSyntax`, but the two classes are otherwise identical. This means that the compiler can determine whether an arbitrary syntax node was encountered in type syntax or expression syntax based solely on the C# type of the syntax node rather than having to rely on the syntax hierarchy or order of operations. About 50% of the line count for this PR comes from baseline changes. These are all to the `main.syntax.bicep` artifacts and consist of the class name for specific nodes being changed (e.g., `VariableAccessSyntax` -> `TypeVariableAccessSyntax`, `StringSyntax` -> `StringTypeLiteralSyntax`, etc.). ###### Microsoft Reviewers: [Open in CodeFlow](https://microsoft.github.io/open-pr/?codeflow=https://github.com/Azure/bicep/pull/13671)
Resolves #13618
In order to make type determinations, the
DeclaredTypeManager
needs to know whether it is within type syntax or expression syntax. This distinction is meaningful for property access (e.g.,<array type>[*]
is legal within type syntax but not within expression syntax, whereas<array value>[?0]
is legal within expression syntax but not within type syntax), variable access (variables within type syntax can refer to other types but not values, and variables within expression syntax can refer to values but not types), nullability control syntax, and literal value syntax.Prior to Bicep 0.26, the type manager was relying on internal state that assumed the AST would be visited in order. This resulted in unstable type assignments if the type of type syntax was requested out of order. In Bicep 0.26.54, this was replaced with a deterministic check that used the model's syntax hierarchy to determine whether a given syntax node was a descendant of the type declaration of an
output
orparameter
statement or the assigned value of atype
statement (indicating type syntax) or not (indicating expression syntax). However, querying the syntax hierarchy can throw an exception if a node was not part of the original model (e.g., if it was created by aSyntaxRewriteVisitor
).This PR moves the responsibility for making this distinction to the parser. The parser is already tracking whether it is within type syntax or expression syntax, as the grammar for each syntax class is distinct. This PR introduces some new descendent classes of
TypeSyntax
that are minimally changed copies of descendent classes ofExpressionSyntax
:VariableAccessSyntax
is a descendent ofExpressionSyntax
, andTypeVariableAccessSyntax
is a descendent ofTypeSyntax
, but the two classes are otherwise identical. This means that the compiler can determine whether an arbitrary syntax node was encountered in type syntax or expression syntax based solely on the C# type of the syntax node rather than having to rely on the syntax hierarchy or order of operations.About 50% of the line count for this PR comes from baseline changes. These are all to the
main.syntax.bicep
artifacts and consist of the class name for specific nodes being changed (e.g.,VariableAccessSyntax
->TypeVariableAccessSyntax
,StringSyntax
->StringTypeLiteralSyntax
, etc.).Microsoft Reviewers: Open in CodeFlow