Skip to content
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

Partial SwiftSyntax support for experimental @abi attribute #2902

Merged
merged 2 commits into from
Dec 4, 2024

Conversation

beccadax
Copy link
Contributor

Matches (or will match) swiftlang/swift#76878.

This is mostly a subset of the work in #2882 which adds support for @abi on most of the declarations I intend to permit, but not on nominal types. It's intended to unblock progress on the feature while I work through how to refactor the syntax tree for nominal types without making overly disruptive changes.

I've updated a few things here to respond to review comments. In particular, a review comment asking about #if handling prompted me to add a test for that; I found that I didn't like how it was being diagnosed, so I added a facility that can parse #if syntax into unexpected nodes on its contents.

Adds basic support for parsing the `@abi` attribute. This commit doesn’t support everything we eventually want it to—particularly, significant refactoring of the syntax tree will be needed to support nominal types—but it handles enough to match what’s implemented in the compiler so far.
@beccadax
Copy link
Contributor Author

@swift-ci please test

enclosingUnexpected.addUnexpected(before: unexpectedBefore, afterThrough: .nestingLevel(of: self))

// Parse the node we actually want.
return parseResult(&self, enclosingUnexpected)
Copy link
Member

Choose a reason for hiding this comment

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

I’m not a fan of EnclosingUnexpectedHandle because it will cause a memory allocation every time parseDeclaration is called + retain release calls to EnclosingUnexpectedHandle, which I would really like to avoid for performance reasons. Also, when stashing tokens inside EnclosingUnexpectedHandle, we need to remember to eat them later on and put them somewhere in the syntax tree, which is quite easy to forget, which would cause use to loose source accuracy.

Instead, would it be possible to assign the unexpected #if tokens to the node returned by parseResult, similar to what we do here? This will create a copy of the resulting syntax node (and leave the original in the bump allocator) but that should be fine because we expect the error cases to be quite rare.

let existingUnexpected: [RawSyntax]
if let unexpectedNode = layout.children[layout.children.count - 1] {
precondition(unexpectedNode.is(RawUnexpectedNodesSyntax.self))
existingUnexpected = unexpectedNode.as(RawUnexpectedNodesSyntax.self).elements
} else {
existingUnexpected = []
}
let unexpected = RawUnexpectedNodesSyntax(elements: existingUnexpected + remainingTokens, arena: self.arena)
let withUnexpected = layout.replacingChild(at: layout.children.count - 1, with: unexpected.raw, arena: self.arena)
return R.init(transferTrailingTrivaFromEndOfFileIfPresent(raw: withUnexpected))!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ha—I actually did something like that first, but decided it required too many hacks. (Particularly when I added in support for decl headers, which could only be smuggled through the outer IfConfigDeclSyntax by wrapping them in the corresponding decl...)

Fine, though, I'll work it out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed the implementation to something that "un-parses" the RawIfConfigDeclSyntax nodes after they've been parsed.

This combination is not permitted, since `#if` can evaluate to zero or many declarations and cannot contain declaration headers (which will become supported in this position in a future PR).
@beccadax beccadax force-pushed the abi-changed-your-first-name branch from 121e861 to 1739cb3 Compare November 22, 2024 05:30
@beccadax beccadax requested a review from ahoppen November 22, 2024 05:30
@beccadax
Copy link
Contributor Author

@swift-ci please test

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.

Thank you, Becca. I like this, all the non-trivial stuff is nicely localized and thus easy to reason about.

@nkcsgexi
Copy link
Contributor

nkcsgexi commented Dec 3, 2024

@swift-ci please test

@nkcsgexi
Copy link
Contributor

nkcsgexi commented Dec 4, 2024

testSwiftValidationTestsuite seems to be failing on macOS. @beccadax could you take a look?

@ahoppen
Copy link
Member

ahoppen commented Dec 4, 2024

That failure is unrelated. I’m investigating it.

@ahoppen
Copy link
Member

ahoppen commented Dec 4, 2024

Disabled testSwiftValidationTestsuite while I investigate further: #2909

@swift-ci Please test macOS

@nkcsgexi
Copy link
Contributor

nkcsgexi commented Dec 4, 2024

Thank you, Alex!

@beccadax
Copy link
Contributor Author

beccadax commented Dec 4, 2024

@ahoppen The validation test failure may be something I saw when I was working on this PR: the Swift repo's validation-test/compiler_crashers_fixed/26170-swift-parser-skipsingle.swift was causing a stack overflow because the (invalid) braces were too deeply nested. I've worked around it by applying this locally:

--- a/Tests/SwiftParserTest/ParserTests.swift
+++ b/Tests/SwiftParserTest/ParserTests.swift
@@ -134,7 +134,13 @@ class ParserTests: ParserTestCase {
     runParserTests(
       name: "Swift validation tests",
       path: testDir,
-      checkDiagnostics: false
+      checkDiagnostics: false,
+      shouldExclude: { url in
+        [
+          // FIXME: This test is overflowing the stack
+          "26170-swift-parser-skipsingle.swift"
+        ].contains(url.lastPathComponent)
+      }
     )
   }
 }

@beccadax beccadax merged commit 0bf5b6c into swiftlang:main Dec 4, 2024
3 checks passed
@nkcsgexi
Copy link
Contributor

nkcsgexi commented Dec 4, 2024

🎉

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