Skip to content

Fix newline parsing at trailing trivia #1661

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.

Sign up for GitHub

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

Merged
merged 7 commits into from
May 15, 2023
Merged

Fix newline parsing at trailing trivia #1661

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
9b2ea0d
Add logic to check if previous token's trailing trivia contains newli…
TTOzzi May 14, 2023
2ab4021
Add test case
TTOzzi May 14, 2023
79c1b1a
Modify parsing logic so that a newline in a multiline string also has…
TTOzzi May 15, 2023
d852fb9
Rename the property that stores the previous parse value
TTOzzi May 15, 2023
1b18fdb
Add logic to check for newline during token text lexing
TTOzzi May 15, 2023
8e6ce86
Formatting
TTOzzi May 15, 2023
fb134e9
Rename newlinePresence property of the Lexer Result
TTOzzi May 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 16 additions & 7 deletions Sources/SwiftParser/Lexer/Cursor.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,6 +242,9 @@ extension Lexer {

/// If we have already lexed a token, the kind of the previously lexed token
var previousTokenKind: RawTokenKind?

/// If we have already lexed a token, stores whether the previous lexeme‘s ending contains a newline.
var previousLexemeTrailingNewlinePresence: NewlinePresence?

/// If the `previousTokenKind` is `.keyword`, the keyword kind. Otherwise
/// `nil`.
Expand Down Expand Up @@ -402,6 +405,15 @@ extension Lexer.Cursor {
} else {
newlineInLeadingTrivia = .absent
}

var flags: Lexer.Lexeme.Flags = []
if newlineInLeadingTrivia == .present {
flags.insert(.isAtStartOfLine)
}
if let previousLexemeTrailingNewlinePresence, previousLexemeTrailingNewlinePresence == .present {
flags.insert(.isAtStartOfLine)
}
self.previousLexemeTrailingNewlinePresence = nil
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@TTOzzi TTOzzi May 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

https://github.com/apple/swift-syntax/pull/1661/files#diff-253b01bc981faa185173c1697784a41bf1b586e91ec7b4de806dfd2646db52c3R1900

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.

Suggested change
self.previousLexemeTrailingNewlinePresence = nil
self.previousLexemeTrailingNewlinePresence = .absent

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that’s just what I was just about to suggest

Copy link
Member Author

@TTOzzi TTOzzi May 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


// Token text.
let textStart = self
Expand Down Expand Up @@ -433,28 +445,24 @@ extension Lexer.Cursor {
if let stateTransition = result.stateTransition {
self.stateStack.perform(stateTransition: stateTransition, stateAllocator: stateAllocator)
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, I missed that!
I'll make sure to run it before every commit.
Thanks for pointing that out 🙇

// Trailing trivia.
let trailingTriviaStart = self
if let trailingTriviaMode = result.trailingTriviaLexingMode ?? currentState.trailingTriviaLexingMode(cursor: self) {
let triviaResult = self.lexTrivia(mode: trailingTriviaMode)
self.previousLexemeTrailingNewlinePresence = triviaResult.newlinePresence
diagnostic = TokenDiagnostic(combining: diagnostic, triviaResult.error?.tokenDiagnostic(tokenStart: cursor))
}

if self.currentState.shouldPopStateWhenReachingNewlineInTrailingTrivia && self.is(at: "\r", "\n") {
self.stateStack.perform(stateTransition: .pop, stateAllocator: stateAllocator)
}

var flags = result.flags
if newlineInLeadingTrivia == .present {
flags.insert(.isAtStartOfLine)
}

diagnostic = TokenDiagnostic(combining: diagnostic, result.error?.tokenDiagnostic(tokenStart: cursor))

let lexeme = Lexer.Lexeme(
tokenKind: result.tokenKind,
flags: flags,
flags: result.flags.union(flags),
diagnostic: diagnostic,
start: leadingTriviaStart.pointer,
leadingTriviaLength: leadingTriviaStart.distance(to: textStart),
Expand Down Expand Up @@ -1889,6 +1897,7 @@ extension Lexer.Cursor {
if character == UInt8(ascii: "\r") {
_ = self.advance(matching: "\n")
}
self.previousLexemeTrailingNewlinePresence = .present
return Lexer.Result(.stringSegment, error: error)
} else {
// Single line literals cannot span multiple lines.
Expand Down
12 changes: 6 additions & 6 deletions Tests/SwiftParserTest/LexerTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1182,9 +1182,9 @@ public class LexerTests: XCTestCase {
"""#,
lexemes: [
LexemeSpec(.multilineStringQuote, leading: " ", text: #"""""#, trailing: "\n"),
LexemeSpec(.stringSegment, text: " line 1\n"),
LexemeSpec(.stringSegment, text: " line 2\n"),
LexemeSpec(.stringSegment, text: " "),
LexemeSpec(.stringSegment, text: " line 1\n", flags: .isAtStartOfLine),
LexemeSpec(.stringSegment, text: " line 2\n", flags: .isAtStartOfLine),
LexemeSpec(.stringSegment, text: " ", flags: .isAtStartOfLine),
LexemeSpec(.multilineStringQuote, text: #"""""#),
]
)
Expand All @@ -1198,9 +1198,9 @@ public class LexerTests: XCTestCase {
"""#,
lexemes: [
LexemeSpec(.multilineStringQuote, leading: " ", text: #"""""#, trailing: "\n"),
LexemeSpec(.stringSegment, text: " line 1 ", trailing: "\\\n"),
LexemeSpec(.stringSegment, text: " line 2\n"),
LexemeSpec(.stringSegment, text: " "),
LexemeSpec(.stringSegment, text: " line 1 ", trailing: "\\\n", flags: .isAtStartOfLine),
LexemeSpec(.stringSegment, text: " line 2\n", flags: .isAtStartOfLine),
LexemeSpec(.stringSegment, text: " ", flags: .isAtStartOfLine),
LexemeSpec(.multilineStringQuote, text: #"""""#),
]
)
Expand Down
19 changes: 19 additions & 0 deletions Tests/SwiftParserTest/StatementTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -763,4 +763,23 @@ final class StatementTests: XCTestCase {
"""
)
}

func testTrailingTriviaIncludesNewline() {
assertParse(
"""
let a = 2/*
*/let b = 3
"""
)

assertParse(
"""
let a = 2/*



*/let b = 3
"""
)
}
}