Skip to content

[Diagnostics] Add fix-its for missing set and ) after access modifier #82058

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 5 commits into from
Aug 16, 2025
Merged

[Diagnostics] Add fix-its for missing set and ) after access modifier #82058

merged 5 commits into from
Aug 16, 2025

Conversation

a7medev
Copy link
Contributor

@a7medev a7medev commented Jun 6, 2025 •
edited
Loading

Summary

Added fix-its missing set subject and ) for access control modifiers. The added fix-its handle the following cases:

  • private(set inserts ) to become private(set)
  • private() inserts set to become private(set)
  • private(<unexpected>) replaces <unexpected> with set to become private(set)
  • private( <declaration> inserts set) to become private(set) <declaration>
  • private(<unexpected> <declaration> replaces <unexpected> with set) to become private(set) <declaration>

a7medev
a7medev commented Jun 6, 2025
View reviewed changes
rintaro
rintaro reviewed Jun 6, 2025
View reviewed changes
lucianopa-msft
lucianopa-msft reviewed Jun 6, 2025
View reviewed changes
@a7medev
Copy link
Contributor Author

a7medev commented Jun 14, 2025 •
edited
Loading

@lucianopa-msft @rintaro I've updated the fix-its to include multiple invalid tokens (and invalid parentheses like private((()))), I generalized the code to not have special cases for <access-control>() and <access-control>(<invalid>) and added recovery for all cases with added fix-its.
I'd appreciate your review. 🙏🏼

hamishknight
hamishknight reviewed Jun 16, 2025
View reviewed changes
Copy link
Contributor

@hamishknight hamishknight left a comment

Choose a reason for hiding this comment

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

Thanks!

@a7medev a7medev force-pushed the feat/access-control-set-fix-it branch from 2aa75e6 to 903496d Compare June 16, 2025 17:17
@a7medev a7medev requested a review from hamishknight June 16, 2025 17:34
hamishknight
hamishknight reviewed Jun 17, 2025
View reviewed changes
@a7medev a7medev force-pushed the feat/access-control-set-fix-it branch from 903496d to dd61a21 Compare August 9, 2025 22:41
@a7medev a7medev force-pushed the feat/access-control-set-fix-it branch from dd61a21 to 3cd3c04 Compare August 9, 2025 22:58
@a7medev a7medev requested a review from hamishknight August 9, 2025 23:19
hamishknight
hamishknight reviewed Aug 15, 2025
View reviewed changes
Copy link
Contributor

@hamishknight hamishknight 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! Couple more minor comments, but otherwise this LGTM

@a7medev a7medev requested a review from hamishknight August 15, 2025 12:24
@hamishknight
Copy link
Contributor

@swift-ci please test

@hamishknight
Copy link
Contributor

hamishknight commented Aug 15, 2025 •
edited
Loading

@a7medev Looks like the test needs updating for the consumeToken change:

[2025-08-15T14:03:06.313Z] /home/build-user/swift/test/attr/accessibility.swift:97:95: error: expected error not produced
[2025-08-15T14:03:06.313Z] private(42 // expected-error{{expected 'set' as subject of 'private' modifier}} {{9-11=set)}} expected-error{{expected declaration}}
[2025-08-15T14:03:06.313Z]                                                                                              ~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

@a7medev
Copy link
Contributor Author

a7medev commented Aug 15, 2025

@hamishknight Fixed. Can you please rerun the tests? 🙏🏼

@hamishknight
Copy link
Contributor

@swift-ci please test

@hamishknight
Copy link
Contributor

@swift-ci please test Windows

@hamishknight hamishknight merged commit 114ea29 into swiftlang:main Aug 16, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hamishknight hamishknight hamishknight approved these changes

@bnbarham bnbarham Awaiting requested review from bnbarham bnbarham is a code owner

@CodaFi CodaFi Awaiting requested review from CodaFi CodaFi is a code owner

@DougGregor DougGregor Awaiting requested review from DougGregor DougGregor is a code owner

@rintaro rintaro Awaiting requested review from rintaro rintaro is a code owner

@lucianopa-msft lucianopa-msft Awaiting requested review from lucianopa-msft

@LucianoPAlmeida LucianoPAlmeida Awaiting requested review from LucianoPAlmeida

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

Successfully merging this pull request may close these issues.
5 participants
@a7medev @hamishknight @rintaro @LucianoPAlmeida @lucianopa-msft