-
Notifications
You must be signed in to change notification settings - Fork 10.6k
[Parse] Split declaration list parsing from parseList() #6005
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
7436699 to
cdd2eec
Compare
include/swift/AST/Stmt.h
Outdated
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.
I'd like @bitjammer to weigh in on removing these, given that he's working towards preserving more source information. Would these just become "trivia", like comments and whitespace?
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.
Kind of a gray area, similar to escaping backticks, but we can keep track of things like this in the syntax tree.
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.
I'd prefer not to remove source information from the current nodes if it's not going to get observed during parse. Also, if you are going to use consumeToken() call, please either comment what you are consuming, or use consumeToken(tok::_____) to indicate what you are expecting to drop. But I would prefer not to drop source information right now.
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.
I'm OK to preserve TrailingSemiLocs. But I think we have a problem in the current implementation.
For example, on let (a, b) = (1, 2), the parser produces 3 decls in this order:
(pattern_binding_decl
(pattern_tuple (pattern_named 'a') (pattern_named 'b'))
(tuple_expr ...)
(var_decl "a")
(var_decl "b")
The current master implementation attaches TrailingSemiLoc only to the last (var_decl "b") which is not relevant, I think. It should be the pattern_binding_decl.
Fixing this is not trivial. If you are OK, I will do that in another PR.
Sounds good? @bitjammer
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.
I'm sad to lose these errors. I think they're for people who have a brain flutter and think } will close #if. It might be worth the improvement, though.
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.
I removed this error because it makes me feel that this #if block was implicitly terminated by the parser.
Probably, we should emit a specialized error for this, something like unexpected '}' in conditional compilation block or conditional compilation blocks must surround self-contained statements.
How do you think?
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.
Okay, that's fair (and can be added separately).
|
@rintaro Do you mind rebasing this? @jrose-apple Does this LGTY? |
Parsing declaration list (e.g. member list of nominal decl) is very
different from comma separated list, because it's elements are separated with
new-line or semi-colon. There's no good reason to consolidate them.
Also, declaration list in 'extension' or inside of decl '#if' didn't
emit diagnostics for consecutive declarations on a line.
class C {
#if true
var value: Int = 42 func foo() {}
#endif
}
extension C {
func bar() {} subscript(i: Int) -> Int {
return 24
}
}
This change consolidates declaration list parsing for
members of nominal decls, extensions, and inside of '#if'.
In addition, removed unnecessary property 'TrailingSemiLoc' from decl.
cdd2eec to
c92dbe5
Compare
|
@swift-ci Please smoke test |
|
Still delegating to @bitjammer (about the whole patch). |
|
Looking. |
bitjammer
left a comment
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.
LGTM.
|
Thank you for reviewing! Can I merge this before merging master to 3.1? |
|
I'm not sure if merging now makes the 3.1 branch or not, but if @bitjammer is comfortable with it for 3.1 as well it can be cherry-picked over. Note that we are in "strict mode" now so please follow the template. |
Parsing declaration list (e.g. member list of nominal decl) is very
different from comma separated list, because their elements are separated with
new-line or semi-colon. There's no good reason to consolidate them.
Also, declaration list in
extensionor inside of decl#ifdidn't properly diagnoseconsecutive declarations on a line. This code used to be successfully compiled.
This PR unifies declaration list parsing for
members of nominal decls, extensions, and inside of
#if.In addition, removed unnecessary property
TrailingSemiLocfromDecl,Stmt, andExpr.