Skip to content

Improved diagnostics for misplaced AttributeList in variable declaration #1383

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
Mar 25, 2024

Conversation

flashspys
Copy link
Contributor

@flashspys flashspys commented Mar 2, 2023

Previously the error messages for this code were quite bad. Now they are great!

struct A {
  var @State name: String
}

Part of this PR are some minor changes:

  • The fixit message is shown in the diagnostics formatter
  • The SyntaxRewriter now can use a given arena
  • Do not print colors if run within Xcode (debatable, feel free to remove)

Best regards from Cocoaheads 😉

@flashspys flashspys requested a review from ahoppen as a code owner March 2, 2023 19:27
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, Felix. Just a few minor comments but overall this looks very good to me.

Und viele Grüße aus Cupertino an CocoaHeads Aachen 🎉


DeclSyntax(
"""
public init(arena: SyntaxArena? = nil) {
Copy link
Member

Choose a reason for hiding this comment

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

Actually, I think it would be better if all nodes created by a SyntaxRewriter live in the same SyntaxArena. Creating a SyntaxArena is fairly costly because it creates a new bump pointer allocator and it’s a waste to create a new one for every node. So, what I’m saying is that arena can be non-optional and this could be

Suggested change
public init(arena: SyntaxArena? = nil) {
public init(arena: SyntaxArena = SyntaxArena()) {

Copy link
Contributor Author

@flashspys flashspys Mar 11, 2023

Choose a reason for hiding this comment

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

I tried that, but unfortunately the tests are failing. The MacroApplication SyntaxRewriter crashes internally with Precondition failed: an arena can't have a new child once it's owned by other arenas

@flashspys flashspys requested a review from ahoppen March 11, 2023 13:08
@ahoppen
Copy link
Member

ahoppen commented Mar 11, 2023

@swift-ci Please test

@flashspys flashspys force-pushed the cocoaheads/misplaced-attribute branch 2 times, most recently from ac1f92d to 61a4c7d Compare March 13, 2023 22:37
@ahoppen
Copy link
Member

ahoppen commented Mar 13, 2023

@swift-ci Please test

@flashspys
Copy link
Contributor Author

The SwiftSyntaxBuilder tests were failing, I fixed the part I could. They are still failing on my machine with some really weird error messages, but maybe this is already known?

@flashspys flashspys force-pushed the cocoaheads/misplaced-attribute branch from 0af142b to de6735d Compare March 17, 2023 20:32
@ahoppen
Copy link
Member

ahoppen commented Mar 17, 2023

@swift-ci Please test

@ahoppen
Copy link
Member

ahoppen commented Mar 22, 2024

Picking this up. Rebased on main and removed the changes to not print ASCII colors in Xcode terminal as well as displaying a hammer to indicate the presence of a Fix-It.

@ahoppen
Copy link
Member

ahoppen commented Mar 22, 2024

@swift-ci Please test

@ahoppen ahoppen force-pushed the cocoaheads/misplaced-attribute branch from ee56a3b to b6f45b7 Compare March 25, 2024 08:28
@ahoppen
Copy link
Member

ahoppen commented Mar 25, 2024

@swift-ci Please test

@ahoppen
Copy link
Member

ahoppen commented Mar 25, 2024

@swift-ci Please test Windows

@ahoppen ahoppen merged commit 8160c80 into swiftlang:main Mar 25, 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