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

CrazyFanFan · 2024-03-22T20:28:10Z

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 · 2024-03-28T20:46:18Z

@swift-ci please test

beccadax

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

beccadax · 2024-03-28T20:52:05Z

test/Concurrency/sendable_checking.swift

@@ -51,6 +51,7 @@ func testCV(

  acceptCV(ns3) // expected-warning {{conformance of 'NS3' to 'Sendable' is only available in macOS 11.0 or newer}}
  // expected-note @-1 {{add 'if #available' version check}}
+  // expected-note @-2 {{change the @available attribute of the global function on macOS from 10.15 to 11.0}}


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.)

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!)

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?

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() { // ... }

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.

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 · 2024-03-28T20:59:05Z

lib/Sema/TypeCheckAvailability.cpp

+    SourceManager &SM = Context.SourceMgr;
+    auto Version = SM.extractText(Lexer::getCharSourceRangeFromSourceRange(
+                                      SM, Attr->IntroducedRange))
+                       .str();


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.

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; }

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 · 2024-03-28T21:13:21Z

lib/Sema/TypeCheckAvailability.cpp

+    Context.Diags
+        .diagnose(ReferenceRange.Start, diag::update_availability_attribute,
+                  KindForDiagnostic, Attr->platformString(), Version, Required)
+        .fixItReplace(Attr->IntroducedRange, Required);


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.)

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.

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.

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

beccadax · 2024-03-28T21:20:29Z

include/swift/AST/DiagnosticsSema.def

@@ -6649,6 +6649,9 @@ ERROR(availability_variadic_type_only_version_newer, none,
 NOTE(availability_guard_with_version_check, none,
     "add 'if #available' version check", ())

+NOTE(update_availability_attribute, none,


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

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

CrazyFanFan · 2024-04-01T12:40:56Z

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

Renamed the function update_availability_attribute to availability_update_attribute.
Read the version string using Attr->Introduced->getAsString().
Skip update, if Attr->getRange() does not overlap with Attr->IntroducedRange.
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 · 2024-04-27T21:21:49Z

include/swift/AST/DiagnosticsSema.def

@@ -6655,6 +6655,9 @@ ERROR(availability_variadic_type_only_version_newer, none,
 NOTE(availability_guard_with_version_check, none,
     "add 'if #available' version check", ())

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


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))

LucianoPAlmeida · 2024-04-27T21:25:29Z

lib/Sema/TypeCheckAvailability.cpp

+    // `IntroducedRange`, which can be from a macro or the minimum version
+    // supported by the compiler, etc. If there's no explicit introduction range
+    // marked in the code,  skip the update logic and return immediately.
+    if (!Attr->getRange().overlaps(Attr->IntroducedRange)) {


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 requested review from hborla, slavapestov, xedin and tshortli as code owners March 22, 2024 20:28

CrazyFanFan force-pushed the feature/updating_existing_available_attribute branch 2 times, most recently from 1a224c9 to 765b2ec Compare March 23, 2024 07:52

CrazyFanFan requested a review from ktoso as a code owner March 23, 2024 07:52

CrazyFanFan changed the title ~~Modify diagnostics to suggest updating existing available attribute~~ [Diagnostics] Add support for updating the existing 'available' attribute. Mar 25, 2024

CrazyFanFan mentioned this pull request Mar 28, 2024

[SR-15051] Swift should suggest updating existing @available attribute when you use an insufficiently available declaration #57378

Open

beccadax self-requested a review March 28, 2024 20:46

beccadax requested changes Mar 28, 2024

View reviewed changes

CrazyFanFan force-pushed the feature/updating_existing_available_attribute branch from 765b2ec to 438729b Compare April 1, 2024 12:19

CrazyFanFan requested a review from beccadax April 15, 2024 06:32

CrazyFanFan requested a review from LucianoPAlmeida April 26, 2024 06:54

LucianoPAlmeida reviewed Apr 27, 2024

View reviewed changes

CrazyFanFan force-pushed the feature/updating_existing_available_attribute branch from 438729b to 6893a0f Compare April 28, 2024 09:05

Modify diagnostics to suggest updating existing available attribute

cc6e0be

CrazyFanFan force-pushed the feature/updating_existing_available_attribute branch from 6893a0f to cc6e0be Compare April 28, 2024 13:06

CrazyFanFan requested a review from LucianoPAlmeida May 2, 2024 01:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

CrazyFanFan commented Mar 22, 2024

beccadax commented Mar 28, 2024

beccadax left a comment

beccadax Mar 28, 2024

beccadax Mar 28, 2024

CrazyFanFan Mar 29, 2024

CrazyFanFan Mar 29, 2024

beccadax Mar 30, 2024

LucianoPAlmeida Mar 30, 2024

beccadax Mar 28, 2024

CrazyFanFan Mar 29, 2024 •

edited

beccadax Mar 30, 2024

beccadax Mar 28, 2024

CrazyFanFan Mar 29, 2024

beccadax Mar 30, 2024

LucianoPAlmeida Mar 30, 2024

beccadax Mar 28, 2024

CrazyFanFan Mar 29, 2024

CrazyFanFan commented Apr 1, 2024

LucianoPAlmeida Apr 27, 2024

CrazyFanFan Apr 28, 2024

LucianoPAlmeida Apr 27, 2024

CrazyFanFan Apr 28, 2024

	"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))

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

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

Are you sure you want to change the base?

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

Conversation

CrazyFanFan commented Mar 22, 2024

beccadax commented Mar 28, 2024

beccadax left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CrazyFanFan Mar 29, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CrazyFanFan commented Apr 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CrazyFanFan Mar 29, 2024 •

edited