Skip to content

Disallow whitespace between @, attribute name and ( #2466

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

Merged
merged 1 commit into from
Feb 24, 2024

Conversation

ahoppen
Copy link
Member

@ahoppen ahoppen commented Feb 3, 2024

SwiftParser equivalent of swiftlang/swift#71237.

Add a Swift version parameter to parser. When parsing in Swift 6 mode, disallow whitespace between @, the attribute’s name and (. In Swift 5 warn.

Use this opportunity to refactor how we diagnose extraneous whitespace.

rdar://121668395

@ahoppen ahoppen requested a review from bnbarham as a code owner February 3, 2024 03:36
@ahoppen ahoppen force-pushed the ahoppen/disallow-attribute-space branch from 8492161 to 4d5d32a Compare February 3, 2024 03:42
@ahoppen ahoppen changed the title Disallow whitespace between @, attribute name and (, multi-line simple string literals and non-decimal line numbers to #sourceLocation Disallow whitespace between @, attribute name and ( Feb 3, 2024
@ahoppen
Copy link
Member Author

ahoppen commented Feb 3, 2024

@swift-ci Please test

@ahoppen
Copy link
Member Author

ahoppen commented Feb 3, 2024

swiftlang/swift#71367

@swift-ci Please test

var (unexpectedBeforeAtSign, atSign) = self.expect(.atSign)
if atSign.trailingTriviaByteLength > 0 || self.currentToken.leadingTriviaByteLength > 0 {
let diagnostic = TokenDiagnostic(
self.swiftVersion < .v6 ? .extraneousTrailingWhitespaceWarning : .extraneousTrailingWhitespaceError,
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should implement .warnUntilSwiftVersion() in SwiftSyntax too?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don’t think that’s realistically possible. We need to have a different case in SwiftSyntax.TokenDiagnostic so that one of them can be marked as an error and the other one as a warning.

Comment on lines -1276 to +1292
unexpectedBeforePound = RawUnexpectedNodesSyntax(combining: unexpectedBeforePound, pound, arena: self.arena)
pound = self.missingToken(.pound)
let diagnostic = TokenDiagnostic(
.extraneousTrailingWhitespaceError,
byteOffset: pound.leadingTriviaByteLength + pound.tokenText.count
)
pound = pound.tokenView.withTokenDiagnostic(tokenDiagnostic: diagnostic, arena: self.arena)
Copy link
Member

Choose a reason for hiding this comment

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

Out of curiosity, why you decided to go with TokenDiagnostic instead of the previous "unexpected" route?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because the unexpected route didn’t have any way of representing warnings, just errors. If there are unexpected or missing tokens in the syntax tree, we always consider the tree as having an error (which makes sense to me) and TokenDiagnostic is the only way we can currently communicate warnings.

self.swiftVersion < .v6 ? .extraneousLeadingWhitespaceWarning : .extraneousLeadingWhitespaceError,
byteOffset: 0
)
leftParen = leftParen.tokenView.withTokenDiagnostic(tokenDiagnostic: diagnostic, arena: self.arena)
Copy link
Member

@rintaro rintaro Feb 5, 2024

Choose a reason for hiding this comment

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

I think I understand why you did this , but the inconsistency of the diagnosing token between #/@ and ( is a little unfortunate. 😢

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, it truly is. But I think everything else would be even more of a pain…

Add a Swift version parameter to parser. When parsing in Swift 6 mode, disallow whitespace between `@`, the attribute’s name and `(`. In Swift 5 warn.

Use this opportunity to refactor how we diagnose extraneous whitespace.

rdar://121668395
@ahoppen ahoppen force-pushed the ahoppen/disallow-attribute-space branch from 3a4e8de to 156b74f Compare February 13, 2024 17:06
@ahoppen
Copy link
Member Author

ahoppen commented Feb 13, 2024

swiftlang/swift#71367

@swift-ci Please test

@ahoppen
Copy link
Member Author

ahoppen commented Feb 13, 2024

swiftlang/swift#71367

@swift-ci Please test Windows

1 similar comment
@ahoppen
Copy link
Member Author

ahoppen commented Feb 23, 2024

swiftlang/swift#71367

@swift-ci Please test Windows

@ahoppen
Copy link
Member Author

ahoppen commented Feb 23, 2024

swiftlang/swift#71367

@swift-ci Please test Windows

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants