Fix newline parsing at trailing trivia #1661
Conversation
| flags.insert(.isAtStartOfLine) | ||
| } | ||
| if let previousTokenNewlinePresence, previousTokenNewlinePresence == .present, | ||
| !currentState.isParsingMultilineString { |
There was a problem hiding this comment.
Why do you need the isParsingMultilineString check? I just tried removing it and it looks like I just need to update three lexer tests, which seems perfectly reasonable to me because the line 1 and line 2 tokens really are on new lines.
There was a problem hiding this comment.
In an effort to minimize side effects, I focused on maintaining the existing test cases, which led me to misunderstand the behavior of the lexer. As you mentioned, it is correct that the line1, line2, and whitespace tokens, excluding the """ character, are really on new lines.
I will try to make the modifications quickly within this week. Thank you for your prompt feedback 🙂
|
|
||
| /// If we have already lexed a token, the `NewlinePresence` of the previously lexed token | ||
| var previousTokenNewlinePresence: NewlinePresence? |
There was a problem hiding this comment.
Just a naming suggestion: I’d like to make it absolutely clear that this is about whether the previous token’s trailing trivia has a newline and avoid any confusion of whether the previous token had a newline inside its leading trivia. So I’d go with
| /// If we have already lexed a token, the `NewlinePresence` of the previously lexed token | |
| var previousTokenNewlinePresence: NewlinePresence? | |
| /// If we have already lexed a token, stores whether the previous lexeme‘s trailing trivia contains a newline. | |
| var previousLexemeTrailingTrivaNewlinePresence: NewlinePresence? |
There was a problem hiding this comment.
Great suggestion! I will try to make the modifications along with the changes mentioned above. Thank you.
| @@ -433,28 +445,24 @@ extension Lexer.Cursor { | |||
| if let stateTransition = result.stateTransition { | |||
| self.stateStack.perform(stateTransition: stateTransition, stateAllocator: stateAllocator) | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
I think swift-format will complain about this whitespace. Could you run format.py? https://github.com/apple/swift-syntax/blob/main/CONTRIBUTING.md#formatting
There was a problem hiding this comment.
Oops, I missed that!
I'll make sure to run it before every commit.
Thanks for pointing that out 🙇
| if let previousLexemeTrailingNewlinePresence, previousLexemeTrailingNewlinePresence == .present { | ||
| flags.insert(.isAtStartOfLine) | ||
| } | ||
| self.previousLexemeTrailingNewlinePresence = nil |
There was a problem hiding this comment.
Resetting to nil doesn’t really make sense here, right? If we follow the same meaning as for previousTokenKind, nil means that there hasn’t been a previous token but in this case there has been. I think the better implementation would be to have in else branch in the if statement in which you set self.previousLexemeTrailingNewlinePresence below and in it set self.previousLexemeTrailingNewlinePresence = .absent.
If you do that, I think you can also move this entire code block below the Token text lexing section and initialize flags with result.flags and avoid the union call when forming the result.
There was a problem hiding this comment.
As you said, it seems like it should have a non-nil value when lexing the previous token.
However, I'm concerned that it would be redundant to assign .absent to each else in the branch that sets previousLexemeTrailingNewlinePresence.
The code above makes it look like I need to put previousLexemeTrailingNewlinePresence = .absent in the lex logic for each case.
https://github.com/apple/swift-syntax/blob/main/Sources/SwiftParser/Lexer/Cursor.swift#L410-L431
How about assigning .absent as the default value after using previousLexemeTrailingNewlinePresence at that position?
I think we can get some reasonable logic with little modification.
| self.previousLexemeTrailingNewlinePresence = nil | |
| self.previousLexemeTrailingNewlinePresence = .absent |
There was a problem hiding this comment.
Additionally, if we move this block of code below the token text lexing section, the value assigned in the token text lexing logic will affect the flag value of the current token, so it would be difficult to move it 🤔
There was a problem hiding this comment.
Oh, now that I think about it, inside the lexing logic, it makes more sense to include the flag in the lexer's result and return it.
I'll make a quick fix and commit it.
There was a problem hiding this comment.
Yes, that’s just what I was just about to suggest
There was a problem hiding this comment.
Similar to TriviaResult, I also added a trailingNewlinePresence property to Lexer.Result and changed it so that modifications to the previousLexemeTrailingNewlinePresence value can only be made within the nextToken method.
The flag initialize logic was also moved under the token text lexing section.
TTOzzi
force-pushed
the
fix-newline-parsing-at-trailing-trivia
branch
from
7732f6e to
8e6ce86
Compare
|
@swift-ci Please test |
|
I appreciate your kind feedback. Thank you @ahoppen! |
|
@TTOzzi Could you also open a PR with this changes targeting the |
|
@ahoppen Of course, I can do it! When should I create the PR until? |
|
There is no clear deadline at the moment. Earlier is always better than later though 😉 |
|
Ok. It's currently working hours here, so I'll create it after I finish work and leave for the day 😉 |
|
Anytime this week is equally good |
Resolve #1626
Unlike leadingTrivia, trailingTrivia was not using the newlinePresence value of triviaResult.
Modified the logic to store the trailingTrivia newlinePresence value from the previous token and set isAtStartOfLine flag when parsing the next token.