You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
The let chain grammar has some issues, and could use some improvement.
The let chain grammar doesn't handle the minimum precedence of chains of associative operators (see prec_let_scrutinee_needs_par). The ExcludedConditions doesn't explain how "deep" the exclusion goes in terms of the AST. For example, naively:
#[cfg(false)]fnf() -> Range<i32>{letmut z = 0;iflet r = 1+z=1{}todo!()}// Contrast this with `if 1+z=1 {}` which is OK since it doesn't have the// minimum precedence restriction.
The Scrutinee here could be parsed as arithmetic plus of 1 and z=1 (assignment expression, whose value is unit). The ExcludedConditions doesn't explain that the minimum precedence applies to the RHS of the add expression. Similar to #1808, the parser keeps track of a minimum precedence for some period of time (while parsing a chain of associative operators I believe), but there are many places where sub-expressions ignore this minimum precedence (I think all sub-expressions except for associative operators?).
I don't know how to specify that. One option is to define a variant of Expression that excludes the relevant expressions, and has a new set of associative operator expressions that have the appropriate exclusions. Like IfCondArithmeticOrLogicalExpression -> IfCondExpression + IfCondExpression | ... and IfCondExpression is the set of restricted expressions. However, I'm concerned this will result in a very large duplication of the Expression sub-grammar.
In https://hackmd.io/@ehuss/S1juwZuyxl I also sketched out a grammar that made LetExpression an actual expression, but only in a ConditionExpression. I don't know if that would help with anything. I think the current Restrictions::ALLOW_LET restriction is correctly captured in the grammar, but it could use more scrutiny.
The range expression exclusion is a little odd since it allows ranges that don't have a LHS. I think it is fine as it is, but it seemed quite unusual and could use some more inspection.
The text was updated successfully, but these errors were encountered:
This adds some more exclusions to the let chain grammar to capture the
minimum precedence while parsing chains. Note that this is incomplete,
see rust-lang#1811. I didn't want to
block things on finalizing this.
I also reworked the `while` grammar to just reuse the grammar from `if`
to avoid the duplication.
The let chain grammar has some issues, and could use some improvement.
The let chain grammar doesn't handle the minimum precedence of chains of associative operators (see
prec_let_scrutinee_needs_par
). The ExcludedConditions doesn't explain how "deep" the exclusion goes in terms of the AST. For example, naively:The Scrutinee here could be parsed as arithmetic plus of
1
andz=1
(assignment expression, whose value is unit). The ExcludedConditions doesn't explain that the minimum precedence applies to the RHS of the add expression. Similar to #1808, the parser keeps track of a minimum precedence for some period of time (while parsing a chain of associative operators I believe), but there are many places where sub-expressions ignore this minimum precedence (I think all sub-expressions except for associative operators?).I don't know how to specify that. One option is to define a variant of
Expression
that excludes the relevant expressions, and has a new set of associative operator expressions that have the appropriate exclusions. LikeIfCondArithmeticOrLogicalExpression -> IfCondExpression
+IfCondExpression | ...
andIfCondExpression
is the set of restricted expressions. However, I'm concerned this will result in a very large duplication of the Expression sub-grammar.In https://hackmd.io/@ehuss/S1juwZuyxl I also sketched out a grammar that made
LetExpression
an actual expression, but only in aConditionExpression
. I don't know if that would help with anything. I think the currentRestrictions::ALLOW_LET
restriction is correctly captured in the grammar, but it could use more scrutiny.The range expression exclusion is a little odd since it allows ranges that don't have a LHS. I think it is fine as it is, but it seemed quite unusual and could use some more inspection.
The text was updated successfully, but these errors were encountered: