Skip to content

Handle DeinitializerDeclSyntax errors in a single diagnostic #2185

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

Jump to bottom
Merged
merged 1 commit into from
Sep 19, 2023
Merged

Handle DeinitializerDeclSyntax errors in a single diagnostic #2185

merged 1 commit into from
Sep 19, 2023

Conversation

kimdv
Copy link
Contributor

@kimdv kimdv commented Sep 13, 2023

Part of #2021

I wanted to open this PR so we can discuss it isolated

@kimdv kimdv requested a review from ahoppen as a code owner September 13, 2023 18:52
ahoppen added a commit to ahoppen/swift-syntax that referenced this pull request Sep 13, 2023
@ahoppen
Applying PR swiftlang#2185
84a7564
@kimdv kimdv force-pushed the kimdv/handle-DeinitializerDeclSyntax-in-a-single-diagnostic branch from 378a886 to 41a1737 Compare September 14, 2023 18:43
@kimdv kimdv force-pushed the kimdv/handle-DeinitializerDeclSyntax-in-a-single-diagnostic branch from 41a1737 to 57625ba Compare September 16, 2023 18:56
@kimdv kimdv requested a review from ahoppen September 16, 2023 18:56
@kimdv kimdv force-pushed the kimdv/handle-DeinitializerDeclSyntax-in-a-single-diagnostic branch 2 times, most recently from e1e1df7 to 575d17e Compare September 17, 2023 14:57
@kimdv
Copy link
Contributor Author

kimdv commented Sep 17, 2023

@swift-ci please test

@kimdv kimdv requested a review from Matejkob September 17, 2023 14:57
Matejkob
Matejkob reviewed Sep 17, 2023
View reviewed changes
Copy link
Contributor

@Matejkob Matejkob left a comment

Choose a reason for hiding this comment

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

First off, apologies for not diving deep into the changes earlier. I initially responded to one of your comments and didn't go through the other changes. Now that you've requested a re-review, I've looked at all of them. So, my comments might be coming in a tad late. Feel free to take them or leave them; they're mostly minor suggestions. Overall, the changes look solid to me. That said, I don't feel fully equipped to weigh in on the underlying business logic just yet.

@kimdv kimdv force-pushed the kimdv/handle-DeinitializerDeclSyntax-in-a-single-diagnostic branch from 575d17e to dd980f7 Compare September 18, 2023 06:17
@kimdv
Copy link
Contributor Author

kimdv commented Sep 18, 2023

First off, apologies for not diving deep into the changes earlier. I initially responded to one of your comments and didn't go through the other changes. Now that you've requested a re-review, I've looked at all of them. So, my comments might be coming in a tad late. Feel free to take them or leave them; they're mostly minor suggestions. Overall, the changes look solid to me. That said, I don't feel fully equipped to weigh in on the underlying business logic just yet.

No need to apologies. No worries!
Thanks for the reviews, I've applied them

@kimdv
Copy link
Contributor Author

kimdv commented Sep 18, 2023

@swift-ci please test

ahoppen
ahoppen reviewed Sep 18, 2023
View reviewed changes
Tests/SwiftParserTest/translated/InitDeinitTests.swift Outdated
DiagnosticSpec(message: "deinitializers cannot have a name", fixIts: ["remove 'x'"])
DiagnosticSpec(message: "deinitializers cannot have name", fixIts: ["remove 'x'"])
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, this isn’t grammatically correct anymore. I think we need the a in case the first element in the list is name or return clause.

Sources/SwiftParserDiagnostics/ParseDiagnosticsGenerator.swift Outdated
addDiagnostic(
returnType,
.deinitCannotHaveReturnType,
node,
Copy link
Member

Choose a reason for hiding this comment

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

Could we anchor this at nodes.first!? That way the diagnostic actually shows up at the error unexpected nodes instead of the deinit keyword. And also, I would add a highlight for all the unexpected nodes (would be worth checking in at least one test case as well).

@kimdv kimdv force-pushed the kimdv/handle-DeinitializerDeclSyntax-in-a-single-diagnostic branch from dd980f7 to 1c934d6 Compare September 18, 2023 19:28
@kimdv kimdv force-pushed the kimdv/handle-DeinitializerDeclSyntax-in-a-single-diagnostic branch from 1c934d6 to e63f6b7 Compare September 19, 2023 06:18
@kimdv kimdv force-pushed the kimdv/handle-DeinitializerDeclSyntax-in-a-single-diagnostic branch from e63f6b7 to 2b229e8 Compare September 19, 2023 06:23
@kimdv
Copy link
Contributor Author

kimdv commented Sep 19, 2023

@swift-ci please test

@kimdv
Copy link
Contributor Author

kimdv commented Sep 19, 2023

@swift-ci please test windows

@kimdv kimdv enabled auto-merge September 19, 2023 11:56
@kimdv kimdv merged commit aeda331 into swiftlang:main Sep 19, 2023
@kimdv kimdv deleted the kimdv/handle-DeinitializerDeclSyntax-in-a-single-diagnostic branch September 19, 2023 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Matejkob Matejkob Matejkob left review comments

@ahoppen ahoppen ahoppen approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kimdv @ahoppen @Matejkob