-
Notifications
You must be signed in to change notification settings - Fork 447
Fix a misparser where we would classify a call to 'open' as a DeclModifier #1370
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
@swift-ci Please test |
|
@swift-ci Please test Windows |
|
Windows failure is unrelated. |
| @@ -60,7 +60,9 @@ extension TokenConsumer { | |||
| modifierProgress.evaluate(subparser.currentToken) | |||
| { | |||
| subparser.eat(handle) | |||
| if subparser.at(.leftParen) && modifierKind.canHaveParenthesizedArgument { | |||
| if modifierKind != .open && subparser.at(.leftParen) && modifierKind.canHaveParenthesizedArgument { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why only for open?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because all the other access modifiers are lexer classified keywords and thus can’t be used as function base names.
| @@ -286,7 +286,11 @@ final class DeclarationTests: XCTestCase { | |||
| fileprivate fileprivate(set) var fileprivateProp = 0 | |||
| private private(set) var privateProp = 0 | |||
| internal(set) var defaultProp = 0 | |||
| """ | |||
| """, | |||
| diagnostics: [ | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
open open(set) var openProp = 0 is valid isn't it? Pointless, but valid 😅?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I agree that it should be valid but it’s rejected with the same error in the C++ parser.
Also, related, I would imagine that
func foo() {
var open = 1
open
var myVar = 2
}is valid but it’s rejected in the C++ parser and the safest thing I can think of doing is to also reject it in the new parser. Any new heuristics to disambiguate open as an identifier vs decl modifier might introduce new source breakage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah. It fails top level but doesn't fail when it's a member. Ie.
class Foo {
open open(set) var openProp = 0
}
is fine.
While I was working on this, I also noticed that we forgot to pass
isAtTopLevelin a few places. They didn’t really matter here but I made sure to correctly pass the flag regardless.