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

a7medev · 2025-06-06T09:58:32Z

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>

lib/Parse/ParseDecl.cpp

rintaro · 2025-06-06T14:29:39Z

lib/Parse/ParseDecl.cpp

+      auto diag = diagnose(Loc, diag::attr_access_expected_set, AttrName);
+
+      if (Tok.is(tok::l_paren)) {
+        return makeParserSuccess();


Special handling private((. Could you explain what this is for?

The previous code already checked for the current token not being ( before consuming a ) as it wouldn't be closing the ( of the access modifier subject as expected in the recovery.

I thought I'd add the same check in the case of private(( <declaration> but thinking about it again it might not be necessary and we can still just suggest to replace the extra ( with set) (we can also do the same for private((), no?).

What do you think?

It seems like the previous code was assuming if you had private( () then the () was something unrelated to the private(, i.e the closing ) matches the ( in the (), not the private(. That seems somewhat reasonable to me, although I doubt this case happens often in practice. It does more closely match what swift-syntax does though, so I think we should probably move this check to the if (peekToken().is(tok::r_paren)) condition, which would still allow us to correct private(( <declaration>.

lucianopa-msft · 2025-06-06T17:15:53Z

test/attr/accessibility.swift

 var unterminatedSubject = 0

-private(42 // expected-error{{expected 'set' as subject of 'private' modifier}} expected-error{{expected declaration}}
+private(42 // expected-error{{expected 'set' as subject of 'private' modifier}} {{9-11=set)}} expected-error{{expected declaration}}


Should a test with more than one token e.g. private(42 45) be added as well?

There's this one already with multiple random tokens but I haven't added any assertions for the fix-its (as I didn't handle that case). What do you have in mind?

private(a bunch of random tokens)

In that case should fix-it correct this case as well?

I've been thinking about handling it by basically consuming tokens until we find a declaration and replace the whole range till the declaration with set or set) but there are some problems that we need to consider:

In the worst case, we won't have a declaration until EOF so we'll end up scanning the remaining part of the file and checking isStartOfSwiftDecl with each token but that's an extreme case that won't happen with a lot of remaining code anyways I think. We can also limit how far ahead we search.

<access-modifier>( may not be followed by a declaration of its own (just by mistake maybe?) and so the fix-it will try to replace irrelevant but valid tokens until the first declaration with set which doesn't make sense. Here's an example:
private( // Fix-it would suggest replacing the code from here if isEnabled { startServiceA() startServiceB() performTaskC() } // till here with `set` var name: String = "Swift"

I'd love to hear your suggestions here.

I don't think 2. should be necessary, maybe we could leave this case out as is, maybe only handle by consuming multiple tokens in same line e.g. until find \n. I think my question was just to exercise thinking about other odd cases maybe something like private set((())). But at the end maybe those odd cases don't need to be handle as they are more unlikely.

FWIW even if we don't handle these cases, I think it's still worth adding them as tests

a7medev · 2025-06-14T00:57:36Z

@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

Thanks!

hamishknight · 2025-06-16T13:59:20Z

lib/Parse/ParseDecl.cpp

+      int OpenParens = 1;
+      while (!Tok.isAtStartOfLine() && !isStartOfSwiftDecl()) {


I'm not convinced we should have such an aggressive lookahead for this, consider the case where e.g the user has forgotten to write func:

private( foo(_ x: Int) -> Bool let x = 0

I don't think we want to suggest completely wiping out the function signature they wrote. Generally I think we only ought to be suggesting deleting code when it's very obviously bogus.

IMO the logic here should be:

If we have private(), insert set

If we have private(<token>) where <token> isn't set, replace that token

If we have private( with no <token>), then insert set), such that the above case becomes private(set) foo

The swift-syntax parser has a slightly more advanced recovery where it can delete certain non-bracket tokens if they appear before a set, but I don't think it's worth trying to emulate that here since we'll be switching over to that parser at some point in the future.

Makes sense, I think that's similar to point #2 discussed previously.

What you're referring to is similar to the initial PR (commit 903496d), do you think we should revert back to that state and address issues from there?

And thanks for the note on switching to the swift-syntax parser, I'll keep that in mind for future changes.

do you think we should revert back to that state and address issues from there?

Yeah I think that's probably best, we could still add special cases for things like private(foo bar) and private(()), but I'm not convinced those are particularly likely mistakes to make, so I think it's fine not to handle them. Though if we did, we'd also want to handle them in swift-syntax.

I reverted back to the initial implementation.

If we have private( with no <token>), then insert set), such that the above case becomes private(set) foo.

The initial implementation handles this case differently:

private( <declaration> inserts set) to become private(set) <declaration>

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

It depends on whether there's a declaration after ( or (<token> or not, so it won't handle the private( foo(_: Int) case since there's no valid declaration following ( or foo.

We can add this case or replace the 2 cases above with it, or keep the 2 cases described as is.

I'd suggest keeping the 2 cases and suggesting set) even if no declaration is found, this allows us to cover more cases like private(get var name, private( var name, and private( name.
Removing the 2 cases can reduce the overhead of using isStartOfSwiftDecl, but I feel like it's still helpful for providing a better fix-it.

What do you think?

Thanks! I think it's fine to do both the general case where we don't have a declaration, as well as the private(<unexpected> <declaration> case that checks isStartOfSwiftDecl. The private( <declaration> case should naturally fall out of the general case though I think?

@hamishknight After some testing, replacing private( <not-declaration> with private(set) <not-declaration> has some pros and cons.

private( foo(...) turns into private(set) foo(...). ✅

private((())) turns into private(set)(())) which is not ideal. ❌

private(some random tokens) turns into private(set) some random tokens) which is not ideal. ❌

It's not very reliable in its simple form, though some cases can be solved by matching multiple tokens between the parentheses. Other cases may still yield weird results like private( some random token turning into private(set) some random tokens which we can leave as is to handle cases like number 1, or not handle cases without matching parentheses to guarantee reliable suggestions.

What do you think?

private((())) turns into private(set)(())) which is not ideal. ❌

This seems fine to me, we have no way of knowing what the user is trying to do with ((())) and it's very unlikely to come up in practice, I'm happy to leave it be.

private(some random tokens) turns into private(set) some random tokens) which is not ideal. ❌

This does seem a bit more unfortunate, but again I think it's pretty unlikely to come up in practice, so I'm okay not handling it for now. If we did want to handle it, we would also need to teach swift-syntax to handle it too.

hamishknight · 2025-06-17T14:52:20Z

lib/Parse/ParseDecl.cpp

      // Minimal recovery: if there's a single token and then an r_paren,
      // consume them both. If there's just an r_paren, consume that.


Nit: This comment is now outdated

The recovery still hasn't changed, do you think we should update the comment to mention the fix-its as well?

I think it's worth mentioning the isAtStartOfSwiftDecl logic, but otherwise I think it's fine

hamishknight · 2025-06-17T15:01:14Z

lib/Parse/ParseDecl.cpp

+      int OpenParens = 1;
+      while (!Tok.isAtStartOfLine() && !isStartOfSwiftDecl()) {


Thanks! I think it's fine to do both the general case where we don't have a declaration, as well as the private(<unexpected> <declaration> case that checks isStartOfSwiftDecl. The private( <declaration> case should naturally fall out of the general case though I think?

hamishknight · 2025-06-17T15:02:31Z

test/attr/accessibility.swift

 var unterminatedSubject = 0

-private(42 // expected-error{{expected 'set' as subject of 'private' modifier}} expected-error{{expected declaration}}
+private(42 // expected-error{{expected 'set' as subject of 'private' modifier}} {{9-11=set)}} expected-error{{expected declaration}}


FWIW even if we don't handle these cases, I think it's still worth adding them as tests

hamishknight · 2025-06-17T15:08:25Z

lib/Parse/ParseDecl.cpp

+      auto diag = diagnose(Loc, diag::attr_access_expected_set, AttrName);
+
+      if (Tok.is(tok::l_paren)) {
+        return makeParserSuccess();


It seems like the previous code was assuming if you had private( () then the () was something unrelated to the private(, i.e the closing ) matches the ( in the (), not the private(. That seems somewhat reasonable to me, although I doubt this case happens often in practice. It does more closely match what swift-syntax does though, so I think we should probably move this check to the if (peekToken().is(tok::r_paren)) condition, which would still allow us to correct private(( <declaration>.

a7medev added 2 commits June 6, 2025 12:47

[Diagnostics] Add tests for accessibility expected set fix-its

58f6799

[Diagnostics] Add fix-its for missing set and ) of access modifier

903496d

a7medev requested review from bnbarham, CodaFi, DougGregor, hamishknight and rintaro as code owners June 6, 2025 09:58

a7medev commented Jun 6, 2025

View reviewed changes

lib/Parse/ParseDecl.cpp Show resolved Hide resolved

rintaro reviewed Jun 6, 2025

View reviewed changes

lucianopa-msft reviewed Jun 6, 2025

View reviewed changes

a7medev requested review from rintaro, lucianopa-msft and LucianoPAlmeida June 14, 2025 00:39

hamishknight reviewed Jun 16, 2025

View reviewed changes

a7medev force-pushed the feat/access-control-set-fix-it branch from 2aa75e6 to 903496d Compare June 16, 2025 17:17

a7medev requested a review from hamishknight June 16, 2025 17:34

hamishknight reviewed Jun 17, 2025

View reviewed changes

		int OpenParens = 1;
		while (!Tok.isAtStartOfLine() && !isStartOfSwiftDecl()) {

		// Minimal recovery: if there's a single token and then an r_paren,
		// consume them both. If there's just an r_paren, consume that.

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

Are you sure you want to change the base?

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

Uh oh!

Conversation

a7medev commented Jun 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hamishknight Jun 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

a7medev commented Jun 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hamishknight left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hamishknight Jun 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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

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

a7medev commented Jun 6, 2025 •

edited

Loading

hamishknight Jun 17, 2025 •

edited

Loading

a7medev commented Jun 14, 2025 •

edited

Loading

hamishknight Jun 17, 2025 •

edited

Loading