[SwiftParser] Diagnose missing 'in' after closure signature #3162
Conversation
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 ?
There was a problem hiding this comment.
I think the generic unexpected diagnostic is fine for now.
In general, we avoid removing "unknown" code with fix-it, unless it's really obvious, because they might contain just-misplaced important user code.
There was a problem hiding this comment.
I think the generic unexpected diagnostic is fine for now. In general, we avoid removing "unknown" code with fix-it, unless it's really obvious, because they might contain just-misplaced important user code.
The new commit doesn't have it anymore and is back to previously. I however had to add the missing diagnostic in the recovery test to pass given it now expects the new diagnostic, and the top-level arrow detection during lookahead in canParseClosureSignature is still there
| 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
kwikysw
force-pushed
the
parse-closure-missing-in-syntax
branch
from
8545bf9 to
14140ef
Compare
|
@swift-ci Please test |
| ), | ||
| DiagnosticSpec( | ||
| locationMarker: "4️⃣", | ||
| message: "unexpected code ') -> Int {}' in closure" |
There was a problem hiding this comment.
Interesting, IIUC this means canParseClosureSignature recognized -> Int as the part of the signature, but the actual signature parsing gave up parsing it at 4️⃣, right? That mismatch is not great, could you file an issue? Even before merging this PR is fine, please just state "After #3162 ... "
|
For fixing the format check, just do |
Tighten closure lookahead to accept signatures without 'in' only when a top-level '->' is present. Add a dedicated ClosureMissingInTests suite for missing-'in' and resync scenarios, and update Recovery test 141 to expect the additional diagnostic and adjusted fixed source.
kwikysw
force-pushed
the
parse-closure-missing-in-syntax
branch
from
14140ef to
494d492
Compare
|
@swift-ci Please test |
2 participants
Tighten closure lookahead to accept signatures without 'in' only when a top-level '->' is present. Add a dedicated ClosureMissingInTests suite for missing-'in' and resync scenarios, and update Recovery test 141 to expect the additional diagnostic and adjusted fixed source.
Corresponding compiler PR: swiftlang/swift#84278