[Diagnostics] Add support for updating the existing 'available' attribute.

[CrazyFanFan]

Revise the context to recommend an update to the existing 'available' attribute.

Consider the following code:

@available(macOS 42, *) 
func foo() {}

@available(macOS 12, *) 
func bar() {
    foo()
}

A new fix-it suggests altering the '@available' attribute of the global function on macOS from 12 to 42.

11 │ @available(macOS 12, *) 
12 │ func bar() {
13 │     foo()
   │     ├─ error: 'foo()' is only available in macOS 42 or newer
   │     ├─ note: add 'if #available' version check
   │     ╰─ note: change the @available attribute of the global function on macOS from 12 to 42
14 │ }

Resolves #57378

[beccadax]

@swift-ci please test

[beccadax]

This is a great start, but I have a few suggestions to refine it.

[beccadax]

I think it would be more typical to emit this note at the source location of the @available attribute it will change, not on the call that caused the diagnostic.

(This is true for the other test cases, too.)

[beccadax]

Also, I see you test the fix-it along with the note in some of these files, but you should probably do it in all of them. (If you don't expect a fix-it, you can assert that by writing {{none}}—but I'm thinking you probably shouldn't emit this note without a fix-it, so maybe that's behavior you ought to change!)

[CrazyFanFan]

I think it would be more typical to emit this note at the source location of the @available attribute it will change, not on the call that caused the diagnostic.

(This is true for the other test cases, too.)

At first I also planned to put this note at the source location of the @available attribute, but that would introduce a new problem. If there more than one call caused the diagnostic, it would result in multiple waning and notes in the same location; Do you have any suggestions for that?

[CrazyFanFan]

Also, I see you test the fix-it along with the note in some of these files, but you should probably do it in all of them. (If you don't expect a fix-it, you can assert that by writing {{none}}—but I'm thinking you probably shouldn't emit this note without a fix-it, so maybe that's behavior you ought to change!)

Regarding fix-it, in the file test/Sema/availability_define.swift, the -define-availability is used to declare available, which causes crashes during fix-it testing. Do you have any suggestions?

// RUN: %target-typecheck-verify-swift \
// RUN:   -define-availability "_iOS13Aligned:macOS 10.15, iOS 13.0" \

// ...

@available(_iOS13Aligned, *)
func client() {
// ...
}

[beccadax]

the -define-availability is used to declare available, which causes crashes during fix-it testing

Oh, interesting! I think you'll probably need to debug this issue to figure out why it's happening. Starting points:

• Where in the compiler is the crash? Is it in the DiagnosticEngine class? The DiagnosticVerifier class? Is the function you modified on the call stack?
• What are the source locations you're using from the AvailableAttr? Are any of them invalid? (If so, you might be able to add an if that checks if they're invalid and skips this diagnostic.)

If you get stuck, push the version of your code where you see the crash and I'll try to debug it.

[LucianoPAlmeida]

If there more than one call caused the diagnostic, it would result in multiple waning and notes in the same location; Do you have any suggestions for that?

I don't have any suggestion for this, but not sure it would be a big problem and it would definitely help to have the note located at the declaration such as users will have better understanding of the availability attribute looking into declaration. Multiple notes, users will be able to reason about them as well IMO.

[beccadax]

Instead of doing this intricate source-manager dance, could you use Attr->Introduced->getAsString()? That might not produce exactly the same text if there are leading zeroes or something, but it seems much simpler and ought to give you the same semantic result.

[CrazyFanFan]

Thank you for your review.
Attr->Introduced->getAsString()works well in most cases, but due to canonicalizePlatformVersion transforming the version number, when encountering@available(macOS, introduced: 10.16), Attr->Introduced->getAsString()will return 11.0 instead of 10.16. This may cause more confusion than leading zeros.

llvm::VersionTuple swift::canonicalizePlatformVersion(
    PlatformKind platform, const llvm::VersionTuple &version) {

  // Canonicalize macOS version for macOS Big Sur to treat
  // 10.16 as 11.0.
  if (platform == PlatformKind::macOS ||
      platform == PlatformKind::macOSApplicationExtension) {
    return llvm::Triple::getCanonicalVersionForOS(llvm::Triple::MacOSX,
                                                  version);
  }

  return version;
}

[beccadax]

I think this is okay because technically there was no macOS 10.16. The compiler is rewriting something less correct into something more correct.

[beccadax]

This looks like it'll do the right thing for attributes like:

• @available(*, unavailable) (just skips it)
• @available(macOS, introduced: 10.15)
• @available(macOS 10.15, iOS 13, *)

But what if you have something like these? Are there any tests cases like them?

• @available(macOS, unavailable)
• @available(macOS, deprecated: 12)

(You could just bail out for these like you do for unconditionally unavailable attributes, or you could make your fix-it insert , introduced: 11 after the platform name in these cases.)

Also:

• @available(*, deprecated)

(For this, you probably either have to bail out or you have to act as though there is no active attribute and offer to insert a new one.)

[CrazyFanFan]

For the deprecated, I personally believe it is unnecessary to continue expanding to support new capabilities.
The same for unavailable methods, unless a developer actively removes the unavailable attribute, we should not consider expanding to support new capabilities.
Therefore, I think return is a good idea.

[beccadax]

Okay. If you don't want to handle those cases (except by returning), I would file a follow-up issue suggesting that they could be supported in the future.

[LucianoPAlmeida]

I would also suggest add test cases for those kind of attributes in this PR as extra scenarios if we don't already.

[beccadax]

Nit: Since the other diagnostic is named availability_add_attribute, maybe this one should be named availability_update_attribute.

[CrazyFanFan]

Ok, I will change the name in the next patch, thanks.

[CrazyFanFan]

Thank you for your response, @beccadax and @LucianoPAlmeida. I made some updates
This patch includes the following updates:

1. Renamed the function update_availability_attribute to availability_update_attribute.
2. Read the version string using Attr->Introduced->getAsString().
3. Skip update, if Attr->getRange() does not overlap with Attr->IntroducedRange.
4. Currently, I haven't found a more concise way to fetch ReferenceName, and I would greatly appreciate any suggestions in the comments.

Thank you for your time and assistance.

[LucianoPAlmeida]

Nit: Should we use quotes on versions as well?

Suggested change

	"update @available attribute for %0 from %1 to %2 to meet the requirements of '%3'", (StringRef, StringRef, StringRef, StringRef))
	"update @available attribute for %0 from '%1' to '%2' to meet the requirements of '%3'", (StringRef, StringRef, StringRef, StringRef))

[CrazyFanFan]

Done

[LucianoPAlmeida]

Should we also check if range is valid?

Suggested change

	if (!Attr->getRange().overlaps(Attr->IntroducedRange)) {
	if (!Attr->getRange().isValid() || !Attr->getRange().overlaps(Attr->IntroducedRange)) {

[CrazyFanFan]

Done