[SwiftParser] Diagnose missing 'in' after closure signature #3162
Conversation
Teach SwiftSyntax to mirror the compiler's missing-'in' closure diagnostic: 'canParseClosureSignature' now only succeeds without 'in' when a top-level arrow was seen, 'ParseDiagnosticsGenerator' reports both the "unexpected tokens prior to 'in'" and "expected 'in' after the closure signature" diagnostics with the appropriate fix-it, the diagnostic strings are exposed for clients, the new closure-focused test suite verifies the scenarios the compiler added, and Recovery test 141 is updated to expect the extra diagnostic and rewritten source.
kwikysw requested review from ahoppen, bnbarham, hamishknight and rintaro as code owners 
| @rintaro Hi, here is the matching swift-syntax change for the missing-'in' diagnostic. |
| DiagnosticSpec( | ||
| locationMarker: "2️⃣", | ||
| message: "expected 'in' after the closure signature", | ||
| fixIts: ["insert 'in'"] |
There was a problem hiding this comment.
This doesn't look right. in is duplicated in the fixedSource.
There was a problem hiding this comment.
Yes, it's because of the resync fix it that adds an 'in', but doesn't remove the previous one. I'll update it depending on whether we should keep the fix it block in our new block or rely on the UnexpectedNodes diagnostic.
But thinking about it this would be pretty cumbersome and tricky to implement
| let baseInKeyword: TokenSyntax = | ||
| node.inKeyword.isMissing | ||
| ? node.inKeyword | ||
| : node.inKeyword.with(\.presence, .missing) | ||
| let adjustedInKeyword = | ||
| baseInKeyword | ||
| .with(\.leadingTrivia, tokens.first?.leadingTrivia ?? baseInKeyword.leadingTrivia) | ||
| .with(\.trailingTrivia, []) | ||
| addDiagnostic( | ||
| unexpected, | ||
| .expectedInAfterClosureSignature, | ||
| fixIts: [ | ||
| FixIt( | ||
| message: InsertTokenFixIt(missingNodes: [Syntax(adjustedInKeyword)]), | ||
| changes: [.makePresent(adjustedInKeyword)] | ||
| ) | ||
| ], | ||
| handledNodes: [adjustedInKeyword.id] | ||
| ) |
There was a problem hiding this comment.
What's the intent of this block?
Also, is unexpectedTokensPriorToIn really needed? UnexpectedNodesSyntax should be automatically handled. I think it should diagnose it like unexpected code in closure signature or something.
There was a problem hiding this comment.
It's the block synthesizing the custom diagnostic for the resync case and reporting the fix-it. This is indeed redundant, only difference being phrasing from the custom diagnostic message in ParserDiagnosticMessages ("unexpected ... in closure signature" instead of after closure signature).
However, if we remove it, we'll still have the generic diagnostic from the UnexpectedNodes visitor, but no fix it.
Should we keep it ?
| if node.inKeyword.isMissing { | ||
| addDiagnostic( | ||
| node.inKeyword, | ||
| .expectedInAfterClosureSignature, |
There was a problem hiding this comment.
I thought missing tokens are also automatically handled (ParseDiagnosticsGenerator.handleMissingToken(_:)). What would happen without this change?
There was a problem hiding this comment.
This is correct, this one is actually redundant and still gives the diagnostic, just with a different wording. I'll remove it
Teach SwiftSyntax to mirror the compiler's missing-'in' closure diagnostic: 'canParseClosureSignature' now only succeeds without 'in' when a top-level arrow was seen, 'ParseDiagnosticsGenerator' reports both the "unexpected tokens prior to 'in'" and "expected 'in' after the closure signature" diagnostics with the appropriate fix-it, the diagnostic strings are exposed for clients, the new closure-focused test suite verifies the scenarios the compiler added, and Recovery test 141 is updated to expect the extra diagnostic and rewritten source.
Corresponding compiler PR: swiftlang/swift#84278