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

Update availability diagnostic for macOS major version number changes #40307

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

cbjeukendrup
Copy link
Contributor

@cbjeukendrup cbjeukendrup commented Nov 27, 2021

Resolves: #57419

From the description of the Swift Report in question:

In TypeCheckAvailability.cpp, the function fixAvailabilityByNarrowingNearbyVersionCheck() checks if an availability specifier near the site of an error has a version that's very close to the one needed for that API, and if so, offers a fix-it for it.
"Very close" is defined as having the same major version number on most OSes, but for macOS, the minor version also has to match. This made sense a few years ago when macOS releases all had 10.x versions, but now that Apple is incrementing the major macOS version number regularly, it probably doesn't make sense anymore. So this condition should be tweaked to only apply if the major version number is 10.

So that's what I did 🙂

@cbjeukendrup
Copy link
Contributor Author

cbjeukendrup commented Nov 28, 2021

I moved the "narrow nearby version check" fix-it from the error to a separate note.

So the error is something like

'foo' is only available in macOS 10.12.2 or newer

and before, the accompanying fix-it was like

replace '10.12.1' with '10.12.2'

The suggested fix-it was rather cryptic, because there was no information to the user why this was suggested, and it was even unclear what it would do, because the change would almost always be in a different line than the error.

Now, the fix-it will appear as a separate note. This way, it can have its own (hopefully more helpful) description. I have chosen the following:

narrow nearby version check from '10.12.1' with '10.12.2'

This fix-it will now also co-exist with the usual fix-it options, like "add 'if #available' version check" and "add @​available attribute to enclosing global function". Personally, I don't see a very good reason why the "narrowing" option should replace the usual options.


I do see two exceptions though:

  • if the statement causing the version requirements error is the only statement inside the closest version check, then the "narrowing" option should be the only option, even if the existing version check is not close to the required version (example:)
     func globalFunc() { // no fix-it about adding @available attribute 
        if #available(macOS 10.12.1, *) { // fix-it about narrowing should be here
            doThingThatRequires_macOS_10_12_2() // error; no fix-it about adding another if #available
        }
    
        if #available(macOS 10.9, *) { // fix-it about narrowing should be here, even though it's not close
            doThingThatRequires_macOS_12_0() // error; no fix-it about adding another if #available
        }
    }
  • if we are already in a version-checked context so to speak, it would not make sense to give fix-its about adding a version check higher up in the hierarchy, because that would make the inner version check redundant (example:)
    struct Structure { // no fix-it about adding @available(macOS 10.12.2, *), because that would make the if #available redundant
        func function() { // no fix-it about adding @available(macOS 10.12.2, *), because that would make the if #available redundant
            if #available(macOS 10.12.1, *) {
                doThingThatRequiresOnly_macOS_10_12_1()
                doThingThatRequires_macOS_10_12_2() // error; this time also a fix-it about adding another if #available, because this time it's not the only statement inside the existing #available
            }
        }
    

I would be happy to implement this (maybe by adding it to this PR, or in a next PR), but to do so, I will need to find out how to check whether the current statement is the only one inside its "version check context". If somebody could give me a hint about that, I would appreciate it.

@cbjeukendrup cbjeukendrup force-pushed the update_fixAvailabilityByNarrowingNearbyVersionCheck branch from 0c08372 to 0c6ad93 Compare December 2, 2021 22:07
@cbjeukendrup
Copy link
Contributor Author

Rebased to fix conflicts.

@cbjeukendrup
Copy link
Contributor Author

@DougGregor Hi! Excuse me for @mentioning you out of the blue, but I saw you are the code owner for this area. I would like to ask if you or someone else could take a look at this PR and give some feedback. I would regret if it would go stale.

@cbjeukendrup cbjeukendrup force-pushed the update_fixAvailabilityByNarrowingNearbyVersionCheck branch from 0c6ad93 to e1e1ed8 Compare May 23, 2022 08:25
@cbjeukendrup cbjeukendrup force-pushed the update_fixAvailabilityByNarrowingNearbyVersionCheck branch from e1e1ed8 to bfa6423 Compare April 13, 2023 13:13
@cbjeukendrup cbjeukendrup force-pushed the update_fixAvailabilityByNarrowingNearbyVersionCheck branch from bfa6423 to 3ceecd4 Compare April 13, 2023 13:15
@xwu
Copy link
Collaborator

xwu commented May 17, 2023

I would suggest that this PR would be easier to review if you narrowly restricted it to fixing the linked issue.

Changing how the diagnostics are emitted is a rather separate concern and I can imagine there could be objections regarding the approach here of adjusting only this specific diagnostic and emitting an additional note on the basis of UI concerns that aren't unique to this diagnostic.

@cbjeukendrup cbjeukendrup force-pushed the update_fixAvailabilityByNarrowingNearbyVersionCheck branch from 3ceecd4 to afd1e7b Compare May 18, 2023 23:01
@cbjeukendrup
Copy link
Contributor Author

@xwu Done!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[SR-15093] Update availability diagnostic for macOS major version number changes
2 participants