DNM: Fix up effect specifiers in function signatures #889
Conversation
bnbarham
force-pushed
the
handle-random-specifiers
branch
from
2a0c83d to
14c8cd8
Compare
bnbarham
force-pushed
the
handle-random-specifiers
branch
from
14c8cd8 to
ee7691a
Compare
| if self.at(.arrow) { | ||
| result = self.parseFunctionReturnClause().returnClause | ||
| if let handle = self.canRecoverTo(.arrow) { | ||
| result = self.parseReturnClause(handle).returnClause |
There was a problem hiding this comment.
You are dropping the effect specifiers here, right?
There was a problem hiding this comment.
Weird that I can't see this in the review. But no, they're added to the return clause itself. They're only returned to be able to make async/throws missing when necessary. This is one of the many reasons I'm not a huge fan of this patch (though this particular returning of unexpected nodes was already here).
It really seems like we should just recover + place tokens in unexpected + have ASTGen handle these diagnostics. Though one issue there is that they wouldn't be parsed as contextual keywords if we just did regular recovery vs looked for the effects specifically.
@unknown is sort of an example of this, though it currently allows any attribute as the @unknown attribute and parses the rest as unexpected (ie. just recovers to the case/default).
There was a problem hiding this comment.
Ah, never mind. I missed that throws also gets added to the unexpected tokens of the returnClause.
There was a problem hiding this comment.
Yeah, but it's super easy to miss. Hence my problem with doing it this way :)
| var loopProgress = LoopProgressCondition() | ||
| while let (specifier, handle) = self.canRecoverTo(anyIn: EffectsSpecifier.self), loopProgress.evaluate(currentToken) { | ||
| let (unexpected, token) = self.eatKeepUnexpected(handle) | ||
| specifiers.append(contentsOf: unexpected.map { SpecifierWithToken(specifier: nil, token: $0) }) |
There was a problem hiding this comment.
I don’t really like how we are considering unexpected tokens as SpecifierWithToken here and setting specifier to nil.
What do you think about changing SpecifierWithToken to
struct ParsedEffectsSpecifier {
let specifier: EffectsSpecifier?
let unexpected: RawUnexpectedNodesSyntax?
let token: RawTokenSyntax
}That way you also don’t need to modify eat.
There was a problem hiding this comment.
Much better name, but I'm not sure I understand the unexpected suggestion. The issue here is that they all need to get merged into the single RawUnexpectedNodeSyntax.
There was a problem hiding this comment.
We can add RawUnexpectedNodeSyntax as a child of RawUnexpectedNodeSyntax. Or, if you don’t like that, maybe it should be
struct ParsedEffectsSpecifier {
let specifier: EffectsSpecifier?
let unexpected: [RawTokenSyntax]
let token: RawTokenSyntax
}There was a problem hiding this comment.
Sure, makes sense.
In progress, up for feedback/to comment in another PR.