Skip to content

[SwiftParser] Parse @abi attribute arguments even without feature flag #3062

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
Apr 23, 2025

Conversation

rintaro
Copy link
Member

@rintaro rintaro commented Apr 21, 2025

When the parser see @abi attribute without the language feature enabled, not parsing the interior causes catastrophic breakage in the tree. To avoid that, parse the argument decl is parsed, but instead of generating ABIAttributeArgumentsSyntax, store it as a UnexpectedNodesSyntax after the left-paren.

@rintaro
Copy link
Member Author

rintaro commented Apr 21, 2025

@swift-ci Please test

@rintaro rintaro force-pushed the parser-abi-interior branch from 69087d0 to eedcc5f Compare April 21, 2025 18:05
@rintaro
Copy link
Member Author

rintaro commented Apr 21, 2025

@swift-ci Please test

@rintaro
Copy link
Member Author

rintaro commented Apr 21, 2025

@swift-ci Please test Windows

@xedin
Copy link
Contributor

xedin commented Apr 21, 2025

@rintaro I think if the feature is not enabled the whole attribute is not supported, is it possible to move UnexpectedNodesSyntax up to cover not just argument but the attribute node itself?

@xedin
Copy link
Contributor

xedin commented Apr 21, 2025

I think the new diagnostic should be about @abi attribute itself instead of its argument...

@rintaro
Copy link
Member Author

rintaro commented Apr 21, 2025

Any @<identifier> is a valid (custom) attribute in syntax trees. I don't feel it's a good idea to treat the whole attribute as "unexpected". I admit the diagnostics are not great, but the recovery is better than the current main.

@rintaro
Copy link
Member Author

rintaro commented Apr 21, 2025

Is there a specific reason that made you suggest that?

@xedin
Copy link
Contributor

xedin commented Apr 21, 2025

I was just looking at the diagnostics you added and they don't seem super useful, it feels like we should either accept the presence of the attribute and validate it fully or skip it all together if the feature is not enabled. That's something we do in TypeCheckAttr by diagnosing the attribute and marking it as invalid when the feature is not there...

@beccadax
Copy link
Contributor

beccadax commented Apr 21, 2025

I'll have to tweak a #if in one of the macro tests in swiftlang/swift#79937, but once I do that, this fix should work fine.

Copy link
Member

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

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

I want to note that this is technically source breaking in case somebody has a custom attribute defined as @abi, eg. the following was correctly parsable using the new parser before this PR and now no longer is. But we broke this kind of code in the C++ parser already, so I don’t think it’s a big deal.

@globalActor
actor abi {
    static let shared: abi = abi()
}

@abi struct Foo {}

AttributeSyntax(
attributeName: TypeSyntax("abi"),
leftParen: .leftParenToken(),
[Syntax(try! FunctionDeclSyntax("func fn() -> Int"))],
Copy link
Member

Choose a reason for hiding this comment

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

Could make the test function throws and then avoid the try! here.

Suggested change
[Syntax(try! FunctionDeclSyntax("func fn() -> Int"))],
[Syntax(try FunctionDeclSyntax("func fn() -> Int"))],

When the parser see `@abi` attribute without the language feature
enabled, not parsing the interior causes catastrophic breakage in the
tree. To avoid that, parse the argument decl is parsed, but instead of
generating `ABIAttributeArgumentsSyntax`, store it as a
`UnexpectedNodesSyntax` after the left-paren.
@rintaro rintaro force-pushed the parser-abi-interior branch from eedcc5f to 7e8f21c Compare April 22, 2025 18:47
@rintaro
Copy link
Member Author

rintaro commented Apr 22, 2025

@swift-ci Please test

@rintaro
Copy link
Member Author

rintaro commented Apr 22, 2025

@swift-ci Please test Windows

@rintaro
Copy link
Member Author

rintaro commented Apr 22, 2025

@swift-ci Please test

@rintaro
Copy link
Member Author

rintaro commented Apr 22, 2025

@swift-ci Please test Windows

@rintaro
Copy link
Member Author

rintaro commented Apr 22, 2025

@swift-ci Please test Linux

@rintaro rintaro merged commit 3a45155 into swiftlang:main Apr 23, 2025
26 checks passed
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.

4 participants