AvailabilityAttrs: Refactor context checking when diagnosing an availability violation

[erik.pilkington]

Summary

This patch removes some redundant functions that implement checking availability against context, and implements a new, more correct one: ShouldDiagnoseAvailabilityInContext. This is done to more easily allow delaying AD_NotYetIntroduced diagnostics, which is a patch I'll submit right after this!

As it happens, this fixes a bug:

int unavailable_global __attribute__((unavailable));

__attribute__((unavailable))
@interface Foo
- meth;
@end

@implementation Foo
- meth {
  (void) unavailable_global; // no error
  (void) ^{
    (void) unavailable_global; // incorrect-error: 'unavailable_global' is not available
  };
}
@end

Here, there is a reference to an unavailable declaration, unavailable, in the context of a block. We shouldn't emit an error here because 'meth' is implicitly unavailable, meaning that we should be able to reference other unavailable declarations inside it

The problem is that, though both isDeclUnavailable() and getCurContextAvailability() check the reference to unavailable_global, isDeclUnavailable doesn't infer availability attributes from @interface to @implementation (But does consider nested contexts), and getCurContextAvailability() doesn't consider non-immediate contexts (But does infer from @interface -> @implementation). Since they both don't catch this case, this error is emitted when it really shouldn't be!

Thanks for taking a look!

[aaron.ballman]

include/clang/Sema/Sema.h

9747

Since you're altering the doxygen comments, it would be nice to add a comment for Message.

lib/Sema/SemaDeclAttr.cpp

6346

Please add an explanatory string literal to the condition.

6350

Please do not use auto here; the type is not spelled out in the initialization.

6368

Same comment here as well.

6372

You can use auto here though. :-)

6398

Please do not use auto here; the type is not spelled out in the initialization.

6699

s/then/than

[manmanren]

This is a nice cleanup!

Manman

lib/Sema/SemaDeclAttr.cpp

6348

Can you add a comment for the lambda?

lib/Sema/SemaExpr.cpp

179

I don't quite get why we can remove the above logic (i.e why we can avoid passing ContextVersion when diagnosing the availability). Is it because we move the logic to DoEmitAvailabilityWarning?

[erik.pilkington]

This new patch fixes the style issues @aaron.ballman and @manmanren brought up.
Thanks,
Erik

lib/Sema/SemaExpr.cpp

179

Yep. It doesn't make sense to check context here because we may delay the diagnostic, in which case we have to check again in the new context (this what isDecl(Deprecated|Unavailable) does). I think it makes more sense to do all the checking in DoEmitAvailabilityWarning, because at that point the paths for delayed and normal diagnostics meet again, and we always have the correct context.

[aaron.ballman]

LGTM

include/clang/Sema/Sema.h

   IdentifierInfo *getFloat128Identifier() const;
 
   Decl *getObjCDeclContext() const;
 
   DeclContext *getCurLexicalContext() const {
     return OriginalLexicalContext ? OriginalLexicalContext : CurContext;
   }
 
-  AvailabilityResult getCurContextAvailability() const;
-
-  /// \brief Get the verison that this context implies.
-  /// For instance, a method in an interface that is annotated with an
-  /// availability attribuite effectively has the availability of the interface.
-  VersionTuple getVersionForDecl(const Decl *Ctx) const;
-
   /// \brief The diagnostic we should emit for \c D, or \c AR_Available.
   ///
   /// \param D The declaration to check. Note that this may be altered to point
   /// to another declaration that \c D gets it's availability from. i.e., we
   /// walk the list of typedefs to find an availability attribute.
-  ///
-  /// \param ContextVersion The version to compare availability against.
-  AvailabilityResult
-  ShouldDiagnoseAvailabilityOfDecl(NamedDecl *&D, VersionTuple ContextVersion,
-                                   std::string *Message);
+  AvailabilityResult ShouldDiagnoseAvailabilityOfDecl(NamedDecl *&D,
+                                                      std::string *Message);
 
   const DeclContext *getCurObjCLexicalContext() const {
     const DeclContext *DC = getCurLexicalContext();
     // A category implicitly has the attribute of the interface.
     if (const ObjCCategoryDecl *CatD = dyn_cast<ObjCCategoryDecl>(DC))
       DC = CatD->getClassInterface();
     return DC;
   }

lib/Sema/SemaDecl.cpp

     (void)WeakUndeclaredIdentifiers.insert(
       std::pair<IdentifierInfo*,WeakInfo>(AliasName, W));
   }
 }
 
 Decl *Sema::getObjCDeclContext() const {
   return (dyn_cast_or_null<ObjCContainerDecl>(CurContext));
 }
-
-AvailabilityResult Sema::getCurContextAvailability() const {
-  const Decl *D = cast_or_null<Decl>(getCurObjCLexicalContext());
-  if (!D)
-    return AR_Available;
-
-  // If we are within an Objective-C method, we should consult
-  // both the availability of the method as well as the
-  // enclosing class.  If the class is (say) deprecated,
-  // the entire method is considered deprecated from the
-  // purpose of checking if the current context is deprecated.
-  if (const ObjCMethodDecl *MD = dyn_cast<ObjCMethodDecl>(D)) {
-    AvailabilityResult R = MD->getAvailability();
-    if (R != AR_Available)
-      return R;
-    D = MD->getClassInterface();
-  }
-  // If we are within an Objective-c @implementation, it
-  // gets the same availability context as the @interface.
-  else if (const ObjCImplementationDecl *ID =
-            dyn_cast<ObjCImplementationDecl>(D)) {
-    D = ID->getClassInterface();
-  }
-  // Recover from user error.
-  return D ? D->getAvailability() : AR_Available;
-}

lib/Sema/SemaDeclAttr.cpp

       }
     }
 
   S.Diag(diag.Loc, diag.getForbiddenTypeDiagnostic())
     << diag.getForbiddenTypeOperand() << diag.getForbiddenTypeArgument();
   diag.Triggered = true;
 }
 
-static bool isDeclDeprecated(Decl *D) {
-  do {
-    if (D->isDeprecated())
-      return true;
-    // A category implicitly has the availability of the interface.
-    if (const ObjCCategoryDecl *CatD = dyn_cast<ObjCCategoryDecl>(D))
-      if (const ObjCInterfaceDecl *Interface = CatD->getClassInterface())
-        return Interface->isDeprecated();
-  } while ((D = cast_or_null<Decl>(D->getDeclContext())));
-  return false;
-}
-
-static bool isDeclUnavailable(Decl *D) {
-  do {
-    if (D->isUnavailable())
-      return true;
-    // A category implicitly has the availability of the interface.
-    if (const ObjCCategoryDecl *CatD = dyn_cast<ObjCCategoryDecl>(D))
-      if (const ObjCInterfaceDecl *Interface = CatD->getClassInterface())
-        return Interface->isUnavailable();
-  } while ((D = cast_or_null<Decl>(D->getDeclContext())));
-  return false;
-}
-
 static const AvailabilityAttr *getAttrForPlatform(ASTContext &Context,
                                                   const Decl *D) {
   // Check each AvailabilityAttr to find the one for this platform.
   for (const auto *A : D->attrs()) {
     if (const auto *Avail = dyn_cast<AvailabilityAttr>(A)) {
       // FIXME: this is copied from CheckAvailability. We should try to
       // de-duplicate.
 

       // Match the platform name.
       if (RealizedPlatform == TargetPlatform)
         return Avail;
     }
   }
   return nullptr;
 }
 
+/// \brief whether we should emit a diagnostic for \c K and \c DeclVersion in
+/// the context of \c Ctx. For example, we should emit an unavailable diagnostic
+/// in a deprecated context, but not the other way around.
+static bool ShouldDiagnoseAvailabilityInContext(Sema &S, AvailabilityResult K,
+                                                VersionTuple DeclVersion,
+                                                Decl *Ctx) {
+  assert(K != AR_Available);
+
+  auto IsContextGreater = [&](const Decl *C) {
+    if (K == AR_NotYetIntroduced) {
+      if (auto *AA = getAttrForPlatform(S.Context, C))
+        if (AA->getIntroduced() >= DeclVersion)
+          return true;
+    } else if (K == AR_Deprecated)
+      if (C->isDeprecated())
+        return true;
+
+    if (C->isUnavailable())
+      return true;
+    return false;
+  };
+
+  do {
+    if (IsContextGreater(Ctx))
+      return false;
+
+    // An implementation implicitly has the availability of the interface.
+    if (const auto *CatOrImpl = dyn_cast<ObjCImplDecl>(Ctx))
+      if (const auto *Interface = CatOrImpl->getClassInterface())
+        if (IsContextGreater(Interface))
+          return false;
+    // A category implicitly has the availability of the interface.
+    if (const ObjCCategoryDecl *CatD = dyn_cast<ObjCCategoryDecl>(Ctx))
+      if (const ObjCInterfaceDecl *Interface = CatD->getClassInterface())
+        if (IsContextGreater(Interface))
+          return false;
+  } while ((Ctx = cast_or_null<Decl>(Ctx->getDeclContext())));
+
+  return true;
+}
+
 static void DoEmitAvailabilityWarning(Sema &S, AvailabilityResult K,
                                       Decl *Ctx, const NamedDecl *D,
                                       StringRef Message, SourceLocation Loc,
                                       const ObjCInterfaceDecl *UnknownObjCClass,
                                       const ObjCPropertyDecl *ObjCProperty,
                                       bool ObjCPropertyAccess) {
   // Diagnostics for deprecated or unavailable.
   unsigned diag, diag_message, diag_fwdclass_message;
   unsigned diag_available_here = diag::note_availability_specified_here;
 
   // Matches 'diag::note_property_attribute' options.
   unsigned property_note_select;
 
   // Matches diag::note_availability_specified_here.
   unsigned available_here_select_kind;
 
-  // Don't warn if our current context is deprecated or unavailable.
+  VersionTuple DeclVersion;
+  if (auto *AA = getAttrForPlatform(S.Context, D))
+    DeclVersion = AA->getIntroduced();
+
+  if (!ShouldDiagnoseAvailabilityInContext(S, K, DeclVersion, Ctx))
+    return;
+
   switch (K) {
   case AR_Deprecated:
-    if (isDeclDeprecated(Ctx) || isDeclUnavailable(Ctx))
-      return;
     diag = !ObjCPropertyAccess ? diag::warn_deprecated
                                : diag::warn_property_method_deprecated;
     diag_message = diag::warn_deprecated_message;
     diag_fwdclass_message = diag::warn_deprecated_fwdclass_message;
     property_note_select = /* deprecated */ 0;
     available_here_select_kind = /* deprecated */ 2;
     break;
 
   case AR_Unavailable:
-    if (isDeclUnavailable(Ctx))
-      return;
     diag = !ObjCPropertyAccess ? diag::err_unavailable
                                : diag::err_property_method_unavailable;
     diag_message = diag::err_unavailable_message;
     diag_fwdclass_message = diag::warn_unavailable_fwdclass_message;
     property_note_select = /* unavailable */ 1;
     available_here_select_kind = /* unavailable */ 0;
 
     if (auto attr = D->getAttr<UnavailableAttr>()) {

     return;
   }
 
   Decl *Ctx = cast<Decl>(getCurLexicalContext());
   DoEmitAvailabilityWarning(*this, AR, Ctx, D, Message, Loc, UnknownObjCClass,
                             ObjCProperty, ObjCPropertyAccess);
 }
 
-VersionTuple Sema::getVersionForDecl(const Decl *D) const {
-  assert(D && "Expected a declaration here!");
-
-  VersionTuple DeclVersion;
-  if (const auto *AA = getAttrForPlatform(getASTContext(), D))
-    DeclVersion = AA->getIntroduced();
-
-  const ObjCInterfaceDecl *Interface = nullptr;
-
-  if (const auto *MD = dyn_cast<ObjCMethodDecl>(D))
-    Interface = MD->getClassInterface();
-  else if (const auto *ID = dyn_cast<ObjCImplementationDecl>(D))
-    Interface = ID->getClassInterface();
-
-  if (Interface) {
-    if (const auto *AA = getAttrForPlatform(getASTContext(), Interface))
-      if (AA->getIntroduced() > DeclVersion)
-        DeclVersion = AA->getIntroduced();
-  }
-
-  return std::max(DeclVersion, Context.getTargetInfo().getPlatformMinVersion());
-}
-
 namespace {
 
 /// \brief This class implements -Wunguarded-availability.
 ///
 /// This is done with a traversal of the AST of a function that makes reference
 /// to a partially available declaration. Whenever we encounter an \c if of the
 /// form: \c if(@available(...)), we use the version from the condition to visit
 /// the then statement.
 class DiagnoseUnguardedAvailability
     : public RecursiveASTVisitor<DiagnoseUnguardedAvailability> {
   typedef RecursiveASTVisitor<DiagnoseUnguardedAvailability> Base;
 
   Sema &SemaRef;
+  Decl *Ctx;
 
   /// Stack of potentially nested 'if (@available(...))'s.
   SmallVector<VersionTuple, 8> AvailabilityStack;
 
   void DiagnoseDeclAvailability(NamedDecl *D, SourceRange Range);
 
 public:
-  DiagnoseUnguardedAvailability(Sema &SemaRef, VersionTuple BaseVersion)
-      : SemaRef(SemaRef) {
-    AvailabilityStack.push_back(BaseVersion);
+  DiagnoseUnguardedAvailability(Sema &SemaRef, Decl *Ctx)
+      : SemaRef(SemaRef), Ctx(Ctx) {
+    AvailabilityStack.push_back(
+        SemaRef.Context.getTargetInfo().getPlatformMinVersion());
   }
 
   void IssueDiagnostics(Stmt *S) { TraverseStmt(S); }
 
   bool TraverseIfStmt(IfStmt *If);
 
   bool VisitObjCMessageExpr(ObjCMessageExpr *Msg) {
     if (ObjCMethodDecl *D = Msg->getMethodDecl())

 
   bool VisitTypeLoc(TypeLoc Ty);
 };
 
 void DiagnoseUnguardedAvailability::DiagnoseDeclAvailability(
     NamedDecl *D, SourceRange Range) {
 
   VersionTuple ContextVersion = AvailabilityStack.back();
-  if (AvailabilityResult Result = SemaRef.ShouldDiagnoseAvailabilityOfDecl(
-          D, ContextVersion, nullptr)) {
+  if (AvailabilityResult Result =
+          SemaRef.ShouldDiagnoseAvailabilityOfDecl(D, nullptr)) {
     // All other diagnostic kinds have already been handled in
     // DiagnoseAvailabilityOfDecl.
     if (Result != AR_NotYetIntroduced)
       return;
 
     const AvailabilityAttr *AA = getAttrForPlatform(SemaRef.getASTContext(), D);
     VersionTuple Introduced = AA->getIntroduced();
 
+    if (ContextVersion >= Introduced)
+      return;
+
+    // If the context of this function is more unavailable then D, we should not
+    // emit a diagnostic.
+    if (!ShouldDiagnoseAvailabilityInContext(SemaRef, Result, Introduced, Ctx))
+      return;
+
     SemaRef.Diag(Range.getBegin(), diag::warn_unguarded_availability)
         << Range << D
         << AvailabilityAttr::getPrettyPlatformName(
                SemaRef.getASTContext().getTargetInfo().getPlatformName())
         << Introduced.getAsString();
 
     SemaRef.Diag(D->getLocation(), diag::note_availability_specified_here)
         << D << /* partial */ 3;

       return;
 
     Body = FD->getBody();
   } else if (auto *MD = dyn_cast<ObjCMethodDecl>(D))
     Body = MD->getBody();
 
   assert(Body && "Need a body here!");
 
-  VersionTuple BaseVersion = getVersionForDecl(D);
-  DiagnoseUnguardedAvailability(*this, BaseVersion).IssueDiagnostics(Body);
+  DiagnoseUnguardedAvailability(*this, D).IssueDiagnostics(Body);
 }

lib/Sema/SemaExpr.cpp

static bool HasRedeclarationWithoutAvailabilityInCategory(const Decl *D) {
   for (const ObjCCategoryDecl *Cat : OID->visible_categories())
     if (ObjCMethodDecl *CatMeth =
             Cat->getMethod(OMD->getSelector(), OMD->isInstanceMethod()))
       if (!CatMeth->hasAttr<AvailabilityAttr>())
         return true;
   return false;
 }
 
-AvailabilityResult Sema::ShouldDiagnoseAvailabilityOfDecl(
-    NamedDecl *&D, VersionTuple ContextVersion, std::string *Message) {
-  AvailabilityResult Result = D->getAvailability(Message, ContextVersion);
+AvailabilityResult
+Sema::ShouldDiagnoseAvailabilityOfDecl(NamedDecl *&D, std::string *Message) {
+  AvailabilityResult Result = D->getAvailability(Message);
 
   // For typedefs, if the typedef declaration appears available look
   // to the underlying type to see if it is more restrictive.
   while (const TypedefNameDecl *TD = dyn_cast<TypedefNameDecl>(D)) {
     if (Result == AR_Available) {
       if (const TagType *TT = TD->getUnderlyingType()->getAs<TagType>()) {
         D = TT->getDecl();
-        Result = D->getAvailability(Message, ContextVersion);
+        Result = D->getAvailability(Message);
         continue;
       }
     }
     break;
   }
 
   // Forward class declarations get their attributes from their definition.
   if (ObjCInterfaceDecl *IDecl = dyn_cast<ObjCInterfaceDecl>(D)) {
     if (IDecl->getDefinition()) {
       D = IDecl->getDefinition();
-      Result = D->getAvailability(Message, ContextVersion);
+      Result = D->getAvailability(Message);
     }
   }
 
   if (const EnumConstantDecl *ECD = dyn_cast<EnumConstantDecl>(D))
     if (Result == AR_Available) {
       const DeclContext *DC = ECD->getDeclContext();
       if (const EnumDecl *TheEnumDecl = dyn_cast<EnumDecl>(DC))
-        Result = TheEnumDecl->getAvailability(Message, ContextVersion);
+        Result = TheEnumDecl->getAvailability(Message);
     }
 
-  switch (Result) {
-  case AR_Available:
-    return Result;
-
-  case AR_Unavailable:
-  case AR_Deprecated:
-    return getCurContextAvailability() != Result ? Result : AR_Available;
-
-  case AR_NotYetIntroduced: {
+  if (Result == AR_NotYetIntroduced) {
     // Don't do this for enums, they can't be redeclared.
     if (isa<EnumConstantDecl>(D) || isa<EnumDecl>(D))
       return AR_Available;
 
     bool Warn = !D->getAttr<AvailabilityAttr>()->isInherited();
     // Objective-C method declarations in categories are not modelled as
     // redeclarations, so manually look for a redeclaration in a category
     // if necessary.
     if (Warn && HasRedeclarationWithoutAvailabilityInCategory(D))
       Warn = false;
     // In general, D will point to the most recent redeclaration. However,
     // for `@class A;` decls, this isn't true -- manually go through the
     // redecl chain in that case.
     if (Warn && isa<ObjCInterfaceDecl>(D))
       for (Decl *Redecl = D->getMostRecentDecl(); Redecl && Warn;
            Redecl = Redecl->getPreviousDecl())
         if (!Redecl->hasAttr<AvailabilityAttr>() ||
             Redecl->getAttr<AvailabilityAttr>()->isInherited())
           Warn = false;
 
     return Warn ? AR_NotYetIntroduced : AR_Available;
   }
-  }
-  llvm_unreachable("Unknown availability result!");
+
+  return Result;
 }
 
 static void
 DiagnoseAvailabilityOfDecl(Sema &S, NamedDecl *D, SourceLocation Loc,
                            const ObjCInterfaceDecl *UnknownObjCClass,
                            bool ObjCPropertyAccess) {
-  VersionTuple ContextVersion;
-  if (const DeclContext *DC = S.getCurObjCLexicalContext())
-    ContextVersion = S.getVersionForDecl(cast<Decl>(DC));
-
   std::string Message;
-  // See if this declaration is unavailable, deprecated, or partial in the
-  // current context.
+  // See if this declaration is unavailable, deprecated, or partial.
   if (AvailabilityResult Result =
-          S.ShouldDiagnoseAvailabilityOfDecl(D, ContextVersion, &Message)) {
+          S.ShouldDiagnoseAvailabilityOfDecl(D, &Message)) {
 
     if (Result == AR_NotYetIntroduced && S.getCurFunctionOrMethodDecl()) {
       S.getEnclosingFunction()->HasPotentialAvailabilityViolations = true;
       return;
     }
 
     const ObjCPropertyDecl *ObjCPDecl = nullptr;
     if (const ObjCMethodDecl *MD = dyn_cast<ObjCMethodDecl>(D)) {
       if (const ObjCPropertyDecl *PD = MD->findPropertyDecl()) {
-        AvailabilityResult PDeclResult =
-            PD->getAvailability(nullptr, ContextVersion);
+        AvailabilityResult PDeclResult = PD->getAvailability(nullptr);
         if (PDeclResult == Result)
           ObjCPDecl = PD;
       }
     }
 
     S.EmitAvailabilityWarning(Result, D, Message, Loc, UnknownObjCClass,
                               ObjCPDecl, ObjCPropertyAccess);
   }

test/SemaObjC/class-unavail-warning.m

-// RUN: %clang_cc1  -fsyntax-only  -triple x86_64-apple-darwin10 -verify %s
+// RUN: %clang_cc1  -fsyntax-only -fblocks -triple x86_64-apple-darwin10 -verify %s
 // rdar://9092208
 
 __attribute__((unavailable("not available")))
 @interface MyClass { // expected-note 8 {{'MyClass' has been explicitly marked unavailable here}}
 @public
     void *_test;
     MyClass *ivar; // no error.
 }

 UNAVAILABLE
 @interface UnavailIP<SomeProto> // no error
 @end
 UNAVAILABLE
 @protocol UnavailProt<SomeProto> // no error
 @end
 @interface UnavailSub(cat)<SomeProto> // no error
 @end
+
+int unavail_global UNAVAILABLE;
+
+UNAVAILABLE __attribute__((objc_root_class))
+@interface TestAttrContext
+-meth;
+@end
+
+@implementation TestAttrContext
+-meth {
+  unavail_global = 2; // no warn
+  (void) ^{
+    unavail_global = 4; // no warn
+  };
+}
+@end