unguarded availability: add a fixit for the "annotate '...' with an availability attribute to silence" note

[arphaman]

Summary

The fixit is given only if the API_AVAILABLE macro is defined.

[erik.pilkington]

Thanks for working on this! This looks like it would be very useful.

lib/Sema/SemaDeclAttr.cpp

7230 ↗

(On Diff #107674)

Its unfortunate to loop over every macro. Can we use Preprocessor::getMacroDefinition()?

7231 ↗

(On Diff #107674)

It would be nice if we could recommend using this macro even if it isn't defined, as users might not have included the <os/availability.h> header. Maybe we can do that on apple platforms, noting that the the macro is declared in os/availability.h if it isn't already defined?

7238 ↗

(On Diff #107674)

OffendingDecl, right? Thats the one with the offending availability attribute

7240 ↗

(On Diff #107674)

I was somewhat uncertain about adding a fixit for this because its difficult to determine where exactly the availability attribute should go, it looks like this doesn't emit an attribute at the right place for this, for example. (It should go after the meth)

@interface X
-(new_int)meth;
@end

Maybe we can do better if we look at what Enclosing actually declares? It's not worth it to emit an incorrect fixit.

[arphaman]

lib/Sema/SemaDeclAttr.cpp

7230 ↗

(On Diff #107674)

Good call.

7231 ↗

(On Diff #107674)

I think we either have to go the full way (i.e. have an #include <os/availability.h> fixit inserted as well) or just avoid any fixits. Note that these fixits will be used pretty much only in Xcode which doesn't show any notes that don't have fixits, so an additional note wouldn't make sense. We could potentially change the message of note_partial_availability_silence but I doubt it's that useful.

7240 ↗

(On Diff #107674)

Yeah, Good point.

[arphaman]

Addressed Erik's comments

[erik.pilkington]

LGTM, thanks for working on this!

lib/Sema/SemaDeclAttr.cpp

7063 ↗

(On Diff #107903)

TagDecls also have a special rule: struct __attribute__(...) X;.

7230 ↗

(On Diff #107674)

Looks like there is a convenience function: isMacroDefined(StringRef)

7231 ↗

(On Diff #107674)

Ok, I think this is fine.

cfe/trunk/lib/Sema/SemaDeclAttr.cpp

if (auto *CD = dyn_cast<ObjCContainerDecl>(Ctx)) {
         return Imp->getClassInterface();
       return CD;
     }
   }
 
   return dyn_cast<NamedDecl>(OrigCtx);
 }
 
+namespace {
+
+struct AttributeInsertion {
+  StringRef Prefix;
+  SourceLocation Loc;
+  StringRef Suffix;
+
+  static AttributeInsertion createInsertionAfter(const NamedDecl *D) {
+    return {" ", D->getLocEnd(), ""};
+  }
+  static AttributeInsertion createInsertionAfter(SourceLocation Loc) {
+    return {" ", Loc, ""};
+  }
+  static AttributeInsertion createInsertionBefore(const NamedDecl *D) {
+    return {"", D->getLocStart(), "\n"};
+  }
+};
+
+} // end anonymous namespace
+
+/// Returns a source location in which it's appropriate to insert a new
+/// attribute for the given declaration \D.
+static Optional<AttributeInsertion>
+createAttributeInsertion(const NamedDecl *D, const SourceManager &SM,
+                         const LangOptions &LangOpts) {
+  if (isa<ObjCPropertyDecl>(D))
+    return AttributeInsertion::createInsertionAfter(D);
+  if (const auto *MD = dyn_cast<ObjCMethodDecl>(D)) {
+    if (MD->hasBody())
+      return None;
+    return AttributeInsertion::createInsertionAfter(D);
+  }
+  if (const auto *TD = dyn_cast<TagDecl>(D)) {
+    SourceLocation Loc =
+        Lexer::getLocForEndOfToken(TD->getInnerLocStart(), 0, SM, LangOpts);
+    if (Loc.isInvalid())
+      return None;
+    // Insert after the 'struct'/whatever keyword.
+    return AttributeInsertion::createInsertionAfter(Loc);
+  }
+  return AttributeInsertion::createInsertionBefore(D);
+}
+
 /// Actually emit an availability diagnostic for a reference to an unavailable
 /// decl.
 ///
 /// \param Ctx The context that the reference occurred in
 /// \param ReferringDecl The exact declaration that was referenced.
 /// \param OffendingDecl A related decl to \c ReferringDecl that has an
 /// availability attribute corrisponding to \c K attached to it. Note that this
 /// may not be the same as ReferringDecl, i.e. if an EnumDecl is annotated and
static void DoEmitAvailabilityWarning(Sema &S, AvailabilityResult K,
   if (K == AR_NotYetIntroduced)
     if (const auto *Enclosing = findEnclosingDeclToAnnotate(Ctx)) {
       if (auto *TD = dyn_cast<TagDecl>(Enclosing))
         if (TD->getDeclName().isEmpty()) {
           S.Diag(TD->getLocation(), diag::note_partial_availability_silence)
               << /*Anonymous*/1 << TD->getKindName();
           return;
         }
-      S.Diag(Enclosing->getLocation(), diag::note_partial_availability_silence)
-          << /*Named*/0 << Enclosing;
+      auto FixitNoteDiag = S.Diag(Enclosing->getLocation(),
+                                  diag::note_partial_availability_silence)
+                           << /*Named*/ 0 << Enclosing;
+      // Don't offer a fixit for declarations with availability attributes.
+      if (Enclosing->hasAttr<AvailabilityAttr>())
+        return;
+      if (!S.getPreprocessor().isMacroDefined("API_AVAILABLE"))
+        return;
+      Optional<AttributeInsertion> Insertion = createAttributeInsertion(
+          Enclosing, S.getSourceManager(), S.getLangOpts());
+      if (!Insertion)
+        return;
+      std::string PlatformName =
+          AvailabilityAttr::getPlatformNameSourceSpelling(
+              S.getASTContext().getTargetInfo().getPlatformName())
+              .lower();
+      std::string Introduced =
+          OffendingDecl->getVersionIntroduced().getAsString();
+      FixitNoteDiag << FixItHint::CreateInsertion(
+          Insertion->Loc,
+          (llvm::Twine(Insertion->Prefix) + "API_AVAILABLE(" + PlatformName +
+           "(" + Introduced + "))" + Insertion->Suffix)
+              .str());
     }
 }
 
 static void handleDelayedAvailabilityCheck(Sema &S, DelayedDiagnostic &DD,
                                            Decl *Ctx) {
   assert(DD.Kind == DelayedDiagnostic::Availability &&
          "Expected an availability diagnostic here");
 

cfe/trunk/test/FixIt/fixit-availability.c

 // RUN: %clang_cc1 -fsyntax-only -Wunguarded-availability -fdiagnostics-parseable-fixits -triple x86_64-apple-darwin9 %s 2>&1 | FileCheck %s
 
 __attribute__((availability(macos, introduced=10.12)))
 int function(void);
 
 void use() {
   function();
 // CHECK: fix-it:{{.*}}:{[[@LINE-1]]:3-[[@LINE-1]]:3}:"if (__builtin_available(macOS 10.12, *)) {\n      "
 // CHECK-NEXT: fix-it:{{.*}}:{[[@LINE-2]]:14-[[@LINE-2]]:14}:"\n  } else {\n      // Fallback on earlier versions\n  }"
 }
+
+__attribute__((availability(macos, introduced=10.12)))
+struct New { };
+
+struct NoFixit {
+  struct New field;
+};
+// CHECK-NOT: API_AVAILABLE

cfe/trunk/test/FixIt/fixit-availability.mm

// CHECK-NEXT: fix-it:{{.*}}:{[[@LINE-2]]:6-[[@LINE-2]]:6}:"\n    } else {\n        // Fallback on earlier versions\n    }"
   }
   if (y)
     int z = function();
 // CHECK: fix-it:{{.*}}:{[[@LINE-1]]:5-[[@LINE-1]]:5}:"if (@available(macOS 10.12, *)) {\n        "
 // CHECK-NEXT: fix-it:{{.*}}:{[[@LINE-2]]:24-[[@LINE-2]]:24}:"\n    } else {\n        // Fallback on earlier versions\n    }"
   anotherFunction(y);
   anotherFunction(x);
 }
+
+#define API_AVAILABLE(X) __attribute__((availability(macos, introduced=10.12))) // dummy macro
+
+API_AVAILABLE(macos(10.12))
+@interface NewClass
+@end
+
+@interface OldButOfferFixit
+@property(copy) NewClass *prop;
+// CHECK: fix-it:{{.*}}:{[[@LINE-2]]:1-[[@LINE-2]]:1}:"API_AVAILABLE(macos(10.12))\n"
+
+- (NewClass *)fixitMe;
+// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:22-[[@LINE-1]]:22}:" API_AVAILABLE(macos(10.12))"
+@end
+
+void oldButOfferFixitFn(NewClass *) {
+// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:1-[[@LINE-1]]:1}:"API_AVAILABLE(macos(10.12))\n"
+}
+
+template<typename T>
+struct OldButOfferFixitTag {
+// CHECK: fix-it:{{.*}}:{[[@LINE-1]]:7-[[@LINE-1]]:7}:" API_AVAILABLE(macos(10.12))"
+  NewClass *x;
+};
+
+// Avoid a fixit for declarations that already have an attribute:
+__attribute__((availability(macos, introduced=10.11)))
+@interface WithoutFixit
+@property(copy) NewClass *prop;
+// CHECK-NOT: API_AVAILABLE
+@end