Improve diagnostics for effect specifiers #1260
Conversation
|
@swift-ci Please test |
Sources/SwiftParser/Specifiers.swift
Outdated
| } | ||
|
|
||
| while let misplacedSpecifier = self.consume(ifAnyIn: EffectSpecifier.self) { | ||
| if asyncKeyword == nil, S.correctAsyncTokenKinds.contains(misplacedSpecifier.tokenKind) || S.misspelledAsyncTokenKinds.contains(misplacedSpecifier.tokenKind) { |
There was a problem hiding this comment.
This is just AsyncEffectSpecifier right? I think it'd be better to use subsets for all of this, it would be quite a few 😓 . But it doesn't seem great to eg. do the two array concatenations above. Plus if we want to remove the array ifAny anyway...
There was a problem hiding this comment.
I knew this would come up. I updated it to use RawTokenKindSubset
| unexpectedAfterThrows.append(RawSyntax(misplacedSpecifier)) | ||
| } | ||
|
|
||
| if unexpectedBeforeAsync.isEmpty && asyncKeyword == nil && unexpectedBeforeThrows.isEmpty && throwsKeyword == nil && unexpectedAfterThrows.isEmpty { |
There was a problem hiding this comment.
Checking all is fine, but since we always set async/throws we could just check those instead.
There was a problem hiding this comment.
I’d rather be conservative here to make sure we are not dropping any tokens.
| while let (_, handle) = self.at(anyIn: EffectsSpecifier.self), loopProgress.evaluate(currentToken) { | ||
| while let (_, handle) = self.at(anyIn: EffectSpecifier.self), loopProgress.evaluate(currentToken) { |
There was a problem hiding this comment.
We also handle unexpected up to the throws from canRecoverTo. We don't beyond that point though (see other comment).
| let _: () 1️⃣throws | ||
| } | ||
| """, | ||
| diagnostics: [ | ||
| // TODO: Old parser expected error on line 3: consecutive statements on a line must be separated by ';' | ||
| // TODO: Old parser expected error on line 3: expected expression | ||
| DiagnosticSpec(message: "unexpected 'throws' keyword in function") |
There was a problem hiding this comment.
I assume this is parsed as let _: () // throws ie. a variable with void type?
There was a problem hiding this comment.
Correct. Added a substructure test
| @@ -760,7 +760,7 @@ final class DeclarationTests: XCTestCase { | |||
| } | |||
| """, | |||
| diagnostics: [ | |||
| DiagnosticSpec(message: "unexpected code 'bogus rethrows set' in variable") | |||
| DiagnosticSpec(message: "unexpected code 'bogus' before effect specifiers") | |||
| ] | |||
| ) | |||
| } | |||
There was a problem hiding this comment.
func foo() throws bogus {}
func foo() bogus -> Int {}
() bogus reasync -> Int
If I'm reading the code correctly I'm fairly sure they aren't handled well. It'd be good to add + fix these, but also happy to have that in a separate PR if you want though, since it's no worse than what's there at the moment.
There was a problem hiding this comment.
The first one is handled well. For the other two, I’ll look into them in a follow-up PR.
ahoppen
force-pushed
the
ahoppen/effect-specifiers
branch
3 times, most recently
from
a733687 to
967cbc6
Compare
|
@swift-ci Please test |
Instead of modelling the effect specifiers as keywords in the declaration/signature, create dedicated nodes for them. This greatly simplifies how we can recover from incorrect effect specifiers.
ahoppen
force-pushed
the
ahoppen/effect-specifiers
branch
from
967cbc6 to
798c660
Compare
Instead of modelling the effect specifiers as keywords in the declaration/signature, create dedicated nodes for them. This greatly simplifies how we can recover from incorrect effect specifiers.