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
Comments
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. |
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? |
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. |
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? |
Actually, TIL that the situation is a bit more complicated than this. See SR-15096 for more details.
I don't think there is a technical challenge.
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 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.
I think it should be possible; an 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 |
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 |
Additional Detail from JIRA
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:
Applying the autocorrect and making the function as open instead, the code is changed to this and warning is emitted:
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:
I think we can improve these diagnostics to something like this to make this more convenient:
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?
The text was updated successfully, but these errors were encountered: