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

[SR-15071] Improve diagnostics on 'open' access modifier on extensions #57397

Closed
swift-ci opened this issue Aug 14, 2021 · 6 comments
Closed

[SR-15071] Improve diagnostics on 'open' access modifier on extensions #57397

swift-ci opened this issue Aug 14, 2021 · 6 comments

Comments

@swift-ci
Copy link
Collaborator

@swift-ci swift-ci commented Aug 14, 2021

Previous ID SR-15071
Radar None
Original Reporter mininny (JIRA User)
Type Improvement
Status Closed
Resolution Done
Additional Detail from JIRA
Votes 0
Component/s
Labels Improvement, DiagnosticsQoI
Assignee mininny (JIRA)
Priority Medium

md5: ca1690fe4f16ba62e256be8707e4d560

Issue Description:

I'm not sure if this is a valid issue, but I found some confusion when creating an 'open' extension.

Using 'open' as default access for an extension results in the following error:

open extension AClass { // Extensions cannot use 'open' as their default access; use 'public'
// Replace 'open' with 'public'
    @objc func functionA()

Applying the autocorrect and making the function as open instead, the code is changed to this and warning is emitted:

public extension AClass {
    @objc open func functionA() // 'open' modifier conflicts with extension's default access of 'public'

where the warning holds no purpose because the 'open' modifier still works. (It seems like this is the case for other types of this conflict, i.e. public function in a private extension, and the function is still publicly visible. Similar thing can be observed in classes, i.e. a private class with a public variable, and no warning is shown there)

To remove the warning, you have to now remove the 'public' modifier like this:

extension PublicClass {
    @objc open func functionC()

I think we can improve these diagnostics to something like this to make this more convenient:

open extension AClass { // Extensions cannot use 'open' as their default access; remove the default access and mark methods as 'open' instead // remove 'open'
    @objc func functionA() // add 'open' ← maybe?

Also, while we're at it, I'm curious about the rationale behind extensions not being able to default to 'open'. I've seen some discussions around it(long time ago here) and supporting it; would it be an appropriate change for swift evolution?

@typesanitizer
Copy link

@typesanitizer typesanitizer commented Aug 19, 2021

I agree, this seems like a good diagnostic improvement idea. I suspect access control changes are not really on the table, unless there is some strong reason in favor of this. I'm afraid I don't know the rationale though. I skimmed the thread you linked but I couldn't quite get it.

@LucianoPAlmeida
Copy link
Collaborator

@LucianoPAlmeida LucianoPAlmeida commented Aug 20, 2021

When I first look at this, I thought this could be a good candidate to have an educational note that could potentially give more insight on the rational. Maybe?

@typesanitizer
Copy link

@typesanitizer typesanitizer commented Aug 20, 2021

My understanding is that educational notes are generally about giving more details about an error, especially when it is difficult to convey the problem in a short error message. The rationale part can be somewhat helpful, but it's not strictly necessary. However, in this case, the error is relatively clear.

Also, in some cases, the rationale is about (objective) soundness related reasons, and that would be a good fit. Whereas, in this case, it seems like the limitation is largely due to subjective reasons, so it seems less useful IMO to add that.

@swift-ci
Copy link
Collaborator Author

@swift-ci swift-ci commented Aug 20, 2021

Comment by Minhyuk Kim (JIRA)

theindigamer (JIRA User) I see. I just think having a restriction on open extensions is a bit consistent with the current behavior of extension for other access control levels. I'm also assuming this wouldn't pose any technical challenge since 'open extension/func' and 'extension/open func' would basically be a syntactic difference, right? Do you think this would be okay for a swift evolution proposal?

Also, I'm trying to see if something like this is possible:

open extension AClass { // expected-note {{Remove 'open'}}
    @objc func functionA() // expected-fixit {{add 'open'}}
    @objc func functionB() // expected-fixit {{add 'open'}}

Is it possible to grab each function and add fixit for it to add 'open' modifier? I'm having difficulty finding an example of this, could you please give me some guidance on where this is being done?

@typesanitizer
Copy link

@typesanitizer typesanitizer commented Aug 20, 2021

Actually, TIL that the situation is a bit more complicated than this. See SR-15096 for more details.

I'm also assuming this wouldn't pose any technical challenge since 'open extension/func' and 'extension/open func' would basically be a syntactic difference, right?

I don't think there is a technical challenge.

Do you think this would be okay for a swift evolution proposal?

I mean... it's okay in that yeah, it's not an unreasonable suggestion to make. That said, my guess is that such a pitch/proposal is likely to get a mixed and/or negative reaction, especially as more of the focus shifts towards value types and actors and less usage of classes. Also, as I wrote in SR-15096 open in extensions only makes sense for @objc classes, not pure Swift classes.

Also, my sense is that the discussion on access control was quite contentious in the past (you can see this in the thread you linked), so new changes would need to meet a high bar for improvements.

Is it possible to grab each function and add fixit for it to add 'open' modifier?

I think it should be possible; an ExtensionDecl is an IterableDeclContext, so you should be able to iterate over the members, filter them (to only take into account which ones will be marked open) and attach fix-its. Note that `fixItInsert` and similar return an `InflightDiagnostic`, so I think you may have to do something like (in pseudocode-ish):

  auto diag = /* create initial diagnostic */
  for (auto &member: extensionDecl->getMembers()) { / * not sure if getMembers() should be used or some other function */
    if (shouldSkip(member)) { continue; }
    diag = diag.fixItInsert(...);
  }

Due to SR-15096, I think this should check if the extended type is not an @objc class, and if so, it should suggest public in the fix-it instead of open.

@swift-ci
Copy link
Collaborator Author

@swift-ci swift-ci commented Aug 21, 2021

Comment by Minhyuk Kim (JIRA)

Understood. I agree, we can just fix the diagnostics since the proposal wouldn't bring any significant improvement over the current syntax. Thanks for the comments!

PR is created here: #38989

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants