Skip to content

[Parser] Emit diagnostics for @ AttributeName and @\nAttributeName #1658

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

Closed
wants to merge 1 commit into from
Closed

[Parser] Emit diagnostics for @ AttributeName and @\nAttributeName #1658

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
32 changes: 30 additions & 2 deletions Sources/SwiftParser/Attributes.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,8 +166,34 @@ extension Parser {
}

mutating func parseAttribute(argumentMode: AttributeArgumentMode, parseArguments: (inout Parser) -> RawAttributeSyntax.Argument) -> RawAttributeListSyntax.Element {
let (unexpectedBeforeAtSign, atSign) = self.expect(.atSign)
let attributeName = self.parseType()
var (unexpectedBeforeAtSign, atSign) = self.expect(.atSign)
var attributeName = self.parseType()
var unexpectedBetweenAtSignAndAttributeName: RawUnexpectedNodesSyntax?

if atSign.trailingTriviaByteLength != 0 {
unexpectedBeforeAtSign = RawUnexpectedNodesSyntax(
combining: unexpectedBeforeAtSign,
atSign,
arena: self.arena
)
atSign = RawTokenSyntax(
kind: .atSign,
text: atSign.tokenText,
leadingTriviaPieces: atSign.leadingTriviaPieces,
presence: .missing,
arena: self.arena
)
} else if attributeName.raw.leadingTriviaByteLength != 0 {
unexpectedBetweenAtSignAndAttributeName = RawUnexpectedNodesSyntax(attributeName)
// `withLeadingTrivia` only returns `nil` if there is no token in `attributeName`.
// But since `attributeName` has leadingTriviaLength != 0 there must be trivia and thus a token.
// So we can safely force-unwrap here.
attributeName = attributeName
.raw
.withLeadingTrivia([], arena: self.arena)!
.as(RawTypeSyntax.self)!
Comment on lines +191 to +194
Copy link
Member

Choose a reason for hiding this comment

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

I suspect this is where the round-trip failures is coming from. You have already stored attributeName in unexpectedBetweenAtSignAndAttributeName and now you’re adding it again with SourcePresence.present. Although it doesn’t quite match the failure message, we have the node twice in the tree now, once with trivia and once without trivia.

The best solution here is a little bit ugly but it’s the best I can come up with: We want to shove the first token of the attributeName (which contains the leading trivia) into unexpectedBetweenAtSignAndAttributeName and instead synthesize a new missing token that doesn’t have leading trivia for the first token in attributeName.

Getting the first token to put into unexpectedBetweenAtSignAndAttributeName should be as easy as doing attributeName.firstToken(viewMode: .sourceAccurate).

To replace the first token in attributeName you need to write a SyntaxRewriter. On the first token that it encounters, it should generate a corresponding missing token without leading trivia. After that, it doesn’t rewrite any further tokens. SyntaxRewriter operates on the Syntax, not RawSyntax level, so you will need to convert attributeName to a Syntax and extract the RawSyntax from the rewritten node. Also, you need to make sure that the rewritten first token is allocated in the same SyntaxArena as all the other tokens. #1383 contains a change to SyntaxRewriter that allows that.

Copy link
Contributor Author

@TiagoMaiaL TiagoMaiaL May 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Getting the first token to put into unexpectedBetweenAtSignAndAttributeName should be as easy as doing attributeName.firstToken(viewMode: .sourceAccurate).

The only way I managed to do this was to first declare RawSyntax.firstToken as @_spi(RawSyntax) public and then do the following:

    } else if attributeName.raw.leadingTriviaByteLength != 0,
      let firstAttributeNameToken = attributeName.raw.firstToken(viewMode: .sourceAccurate)
    {
      unexpectedBetweenAtSignAndAttributeName = RawUnexpectedNodesSyntax(
        firstAttributeNameToken.withPresence(.present, 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.

Can you push your changes and then I’ll take a look at them.

}

let shouldParseArgument: Bool
switch argumentMode {
case .required:
Expand All @@ -185,6 +211,7 @@ extension Parser {
RawAttributeSyntax(
unexpectedBeforeAtSign,
atSignToken: atSign,
unexpectedBetweenAtSignAndAttributeName,
attributeName: attributeName,
unexpectedBeforeLeftParen,
leftParen: leftParen,
Expand All @@ -199,6 +226,7 @@ extension Parser {
RawAttributeSyntax(
unexpectedBeforeAtSign,
atSignToken: atSign,
unexpectedBetweenAtSignAndAttributeName,
attributeName: attributeName,
leftParen: nil,
argument: nil,
Expand Down
46 changes: 45 additions & 1 deletion Sources/SwiftParserDiagnostics/ParseDiagnosticsGenerator.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -457,7 +457,51 @@ public class ParseDiagnosticsGenerator: SyntaxAnyVisitor {
],
handledNodes: [argument.id]
)
return .visitChildren
}
if let unexpectedAtSign = node.unexpectedBeforeAtSignToken?.onlyToken(where: { $0.tokenKind == .atSign && !$0.trailingTrivia.isEmpty }),
node.atSignToken.presence == .missing
{
addDiagnostic(
unexpectedAtSign,
position: unexpectedAtSign.endPositionBeforeTrailingTrivia,
StaticParserError.invalidWhitespaceBetweenAttributeAtSignAndIdentifier,
fixIts: [
FixIt(
message: StaticParserFixIt.removeExtraneousWhitespace,
changes: [
.makeMissing(unexpectedAtSign, transferTrivia: false),
.makePresent(node.atSignToken),
]
)
],
handledNodes: [
node.id,
unexpectedAtSign.id,
node.atSignToken.id,
]
)
} else if node.attributeName.isMissingAllTokens,
let unexpectedBetweenAtSignTokenAndAttributeName = node.unexpectedBetweenAtSignTokenAndAttributeName,
unexpectedBetweenAtSignTokenAndAttributeName.trailingTriviaLength.utf8Length != 0
{
addDiagnostic(
unexpectedBetweenAtSignTokenAndAttributeName,
StaticParserError.invalidWhitespaceBetweenAttributeAtSignAndIdentifier,
fixIts: [
FixIt(
message: StaticParserFixIt.removeExtraneousWhitespace,
changes: [
.makeMissing(unexpectedBetweenAtSignTokenAndAttributeName, transferTrivia: false),
.makePresent(node.attributeName),
]
)
],
handledNodes: [
node.id,
unexpectedBetweenAtSignTokenAndAttributeName.id,
node.attributeName.id,
]
)
}
return .visitChildren
}
Expand Down
3 changes: 3 additions & 0 deletions Sources/SwiftParserDiagnostics/ParserDiagnosticMessages.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,9 @@ extension DiagnosticMessage where Self == StaticParserError {
public static var invalidWhitespaceAfterPeriod: Self {
.init("extraneous whitespace after '.' is not permitted")
}
public static var invalidWhitespaceBetweenAttributeAtSignAndIdentifier: Self {
.init("extraneous whitespace after '@' is not permitted")
}
public static var joinConditionsUsingComma: Self {
.init("expected ',' joining parts of a multi-clause condition")
}
Expand Down
6 changes: 4 additions & 2 deletions Sources/SwiftSyntax/Raw/RawSyntax.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -293,7 +293,8 @@ extension RawSyntax {
/// - Parameters:
/// - leadingTrivia: The trivia to attach.
/// - arena: SyntaxArena to the result node data resides.
func withLeadingTrivia(_ leadingTrivia: Trivia, arena: SyntaxArena) -> RawSyntax? {
@_spi(RawSyntax)
public func withLeadingTrivia(_ leadingTrivia: Trivia, arena: SyntaxArena) -> RawSyntax? {
switch view {
case .token(let tokenView):
return .makeMaterializedToken(
Expand All @@ -319,7 +320,8 @@ extension RawSyntax {
/// - Parameters:
/// - trailingTrivia: The trivia to attach.
/// - arena: SyntaxArena to the result node data resides.
func withTrailingTrivia(_ trailingTrivia: Trivia, arena: SyntaxArena) -> RawSyntax? {
@_spi(RawSyntax)
public func withTrailingTrivia(_ trailingTrivia: Trivia, arena: SyntaxArena) -> RawSyntax? {
switch view {
case .token(let tokenView):
return .makeMaterializedToken(
Expand Down
39 changes: 39 additions & 0 deletions Tests/SwiftParserTest/AttributeTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -631,4 +631,43 @@ final class AttributeTests: XCTestCase {
"""
)
}

func testInvalidWhitespaceBetweenAtSignAndIdenfifierIsDiagnosed() {
assertParse(
"""
@1️⃣ MyAttribute
func foo() {}
""",
diagnostics: [
DiagnosticSpec(
message: "extraneous whitespace after '@' is not permitted",
fixIts: ["remove whitespace"]
)
],
fixedSource: """
@MyAttribute
func foo() {}
"""
)
}

func testInvalidNewlineBetweenAtSignAndIdenfifierIsDiagnosed() {
assertParse(
"""
@1️⃣
MyAttribute
func foo() {}
""",
diagnostics: [
DiagnosticSpec(
message: "extraneous whitespace after '@' is not permitted",
fixIts: ["remove whitespace"]
)
],
fixedSource: """
@MyAttribute
func foo() {}
"""
)
}
}