[Parse] Create SwitchStmt nodes for switch statements with errors
#36930
Conversation
At the moment, if there is an error in the `switch` statement expression or if the `{` is missing, we return `nullptr` from `parseStmtSwitch`, but we consume tokens while trying to parse the `switch` statement. This causes the AST to not contain any nodes for the tokens that were consumed while trying to parse the `switch` statement.
While this doesn’t cause any issues during compilation (compiling fails anyway so not having the `switch` statement in the AST is not a problem) this causes issues when trying to complete inside an expression that was consumed while trying to parse the `switch` statement but doesn’t have a representation in the AST. The solver-based completion approach can’t find the expression that contains the completion token (because it’s not part of the AST) and thus return empty results.
To fix this, make sure we are always creating a `SwitchStmt` when consuming tokens for it.
Previously, one could always assume that a `SwitchStmt` had a valid `LBraceLoc` and `RBraceLoc`. This is no longer the case because of the recovery. In order to form the `SwitchStmt`’s `SourceRange`, I needed to add a `EndLoc` property to `SwitchStmt` that keeps track of the last token in the `SwitchStmt`. Theoretically we should be able to compute this location by traversing the right brace, case stmts, subject expression, … in reverse order until we find something that’s not missing. But if the `SubjectExpr` is an `ErrorExpr`, representing a missing expression, it might have a source range that points to one after the last token in the statement (this is due to the way the `ErrorExpr` is being constructed), therefore returning an invalid range. So overall I thought it was easier and safer to add another property.
Fixes rdar://76688441 [SR-14490]
|
@swift-ci Please test |
| auto switchStmt = SwitchStmt::create(LabeledStmtInfo(), SourceLoc(), enumRef, | ||
| SourceLoc(), cases, SourceLoc(), C); | ||
| SourceLoc(), cases, SourceLoc(), | ||
| SourceLoc(), C); |
There was a problem hiding this comment.
11 instances of this in lib/Sema/Derived*... We definitely should make SwitchStmt::createImplicit().
| /// The location of the last token in the 'switch' statement. For valid | ||
| /// 'switch' statements this is the same as \c RBraceLoc. If the '}' is | ||
| /// missing this points to the last token before the '}' was expected. | ||
| SourceLoc EndLoc; |
There was a problem hiding this comment.
Instead of introducing EndLoc, can we pass PreviousLoc to LBraceLoc and RBraceLoc in Parser?
This way, we can know "invalid" switch statements by checking LBraceLoc == RBraceLoc.
There was a problem hiding this comment.
We could… I also thought about just abusing RBraceLoc to store the end location like we do for BraceStmt. I think after the parser, no-one really cares about the LBraceLoc and RBraceLoc. But I didn’t really like that approach for BraceStmt already and I don’t want to make the situation worse.
I don’t know if there will ever be a use case for it but we wouldn’t be able to check whether there was a { but no } with your approach. And we would have LBraceLoc be after the end loc of the last case if just the } is missing, which isn’t nice either.
As an alternative, what do you think about renaming RBraceLoc to EndLoc and adding a boolean flag HasRBrace. If it is true, EndLoc represents the RBraceLoc and if it is false there was no }.
There was a problem hiding this comment.
I agree abusing RBraceLoc is not nice.
It's OK. As long as you intentionally do this, I support it :)
At the moment, if there is an error in the
switchstatement expression or if the{is missing, we returnnullptrfromparseStmtSwitch, but we consume tokens while trying to parse theswitchstatement. This causes the AST to not contain any nodes for the tokens that were consumed while trying to parse theswitchstatement.While this doesn’t cause any issues during compilation (compiling fails anyway so not having the
switchstatement in the AST is not a problem) this causes issues when trying to complete inside an expression that was consumed while trying to parse theswitchstatement but doesn’t have a representation in the AST. The solver-based completion approach can’t find the expression that contains the completion token (because it’s not part of the AST) and thus return empty results.To fix this, make sure we are always creating a
SwitchStmtwhen consuming tokens for it.Previously, one could always assume that a
SwitchStmthad a validLBraceLocandRBraceLoc. This is no longer the case because of the recovery. In order to form theSwitchStmt’sSourceRange, I needed to add aEndLocproperty toSwitchStmtthat keeps track of the last token in theSwitchStmt. Theoretically we should be able to compute this location by traversing the right brace, case stmts, subject expression, … in reverse order until we find something that’s not missing. But if theSubjectExpris anErrorExpr, representing a missing expression, it might have a source range that points to one after the last token in the statement (this is due to the way theErrorExpris being constructed), therefore returning an invalid range. So overall I thought it was easier and safer to add another property.Fixes rdar://76688441 [SR-14490]