Limit conditions for inferencing a possible FuncDecl #2149
Conversation
There was a problem hiding this comment.
Thanks for the PR @mininny. The changes look good to me and I quite like the unexpected 'word1' diagnostic because at this point you can just view word1 as garbage text inside the member decl list.
And you can definitely change the existing tests. If the diagnostics improve, it’s totally fine to change their expected behavior.
| @@ -281,9 +281,11 @@ extension Parser { | |||
| ) | |||
| } | |||
|
|
|||
| let isProbablyFuncDecl = self.at(.identifier, .wildcard) || self.at(anyIn: Operator.self) != nil | |||
| let isPossibleFuncIdentifier = self.at(.identifier, .wildcard) | |||
| let isPossibleFuncParen = self.peek(isAt: .identifier, .leftParen) || self.peek(isAt: .identifier, .leftAngle) | |||
There was a problem hiding this comment.
You should be able to shorten this to
| let isPossibleFuncParen = self.peek(isAt: .identifier, .leftParen) || self.peek(isAt: .identifier, .leftAngle) | |
| let isPossibleFuncParen = self.peek(isAt: .identifier, .leftParen, .leftAngle) |
There was a problem hiding this comment.
Thanks! I shortened it. However, while testing it out, I found that in
class MyClass {
foo<Int>
}
< is parsed as a binaryOperator rather than a left angle. I think this might need some follow-up item that should be looked at.
…ssing decl syntax inside a member decl list
mininny
force-pushed
the
limit-parser-func-decl-inference
branch
from
057218f to
e6b8bd9
Compare
Thanks for the review and comment! I've added some tests and marked the PR as ready for review. I have another question; when adding tests, is it okay to add tests to |
|
@swift-ci Please test |
|
It looks like your changes aren’t formatted. Could you run |
mininny
changed the title
Yep, I ran the the format.py 👍 |
|
@swift-ci Please test |
|
@swift-ci Please test Windows |
Follow-up from #2109
Currently, the parser consumes the
word1token as an incomplete function declaration since most declarations within a MemberDeclList are functions.However, without more information around the token like
funcor()which would indicate that it is a possible FuncDecl, it could also be other declarations, likelet,var, or even something liketypealias.This PR peeks the tokens after the identifier and checks if it has
(or<which would indicate that it's a function.Any freestanding identifiers as shown in the example above would be parsed as
MissingDeclSyntax, which spits outunexpected 'word1' in MyClass.This would affect existing test cases that rely on the previous behavior and change the resulting diagnostics.
I've validated that this change is compatible with most test cases, but I'm not sure how(if I'm allowed to) to fix existing test cases, and add test cases(because many of them are translations from swift repo)
Also, I think a better diagnostic in the example above is something like:
expected keyword before `word1`. did you mean to add `func` or `var`?, but I'm not yet sure how I can feed the parsed information from Parser to Diagnostics.