[Sema] Always search the full function scope context if a potential availability violation is encountered

[logan-5]

Summary

Always search the full function scope context if a potential availability violation is encountered. This fixes both https://bugs.llvm.org/show_bug.cgi?id=50309 and https://bugs.llvm.org/show_bug.cgi?id=50310.

Previously, lambdas inside functions would mark their own bodies for later analysis when encountering a potentially unavailable decl, without taking into consideration that the entire lambda itself might be correctly guarded inside an @available check. The same applied to inner class member functions. Blocks happened to work as expected already, since Sema::getEnclosingFunction() skips through block scopes.

This patch instead simply and conservatively marks the entire outermost function scope for search, and removes some special-case logic that prevented DiagnoseUnguardedAvailabilityViolations from traversing down into lambdas and nested functions. This correctly accounts for arbitrarily nested lambdas, inner classes, and blocks that may be inside appropriate @available checks at any ancestor level. It also treats all potential availability violations inside functions consistently, without being overly sensitive to the current DeclContext, which previously caused issues where e.g. nested struct members were warned about twice.

DiagnoseUnguardedAvailabilityViolations now has more work to do in some cases, particularly in functions with many (possibly deeply) nested lambdas and classes, but the big-O is the same, and the simplicity of the approach and the fact that it fixes at least two bugs feels like a strong win IMO.

[arphaman]

Thanks for the patch! I think the new checking semantics behavior provided by this patch are fine and match what the Swift compiler allows in the if #available blocks in Swift. I will do some additional testing tomorrow but I'm hoping I can approve your patch in the next few days.

[logan-5]

Thanks very much; I look forward to any feedback!

[arphaman]

I think this is good to go, I haven't observed any issues with this patch so far in my testing. LGTM

clang/include/clang/Sema/Sema.h

public:
   getCurLambda(bool IgnoreNonLambdaCapturingScope = false);
 
   /// Retrieve the current generic lambda info, if any.
   sema::LambdaScopeInfo *getCurGenericLambda();
 
   /// Retrieve the current captured region, if any.
   sema::CapturedRegionScopeInfo *getCurCapturedRegion();
 
+  /// Retrieve the current function, if any, that should be analyzed for
+  /// potential availability violations.
+  sema::FunctionScopeInfo *getCurFunctionAvailabilityContext();
+
   /// WeakTopLevelDeclDecls - access to \#pragma weak-generated Decls
   SmallVectorImpl<Decl *> &WeakTopLevelDecls() { return WeakTopLevelDecl; }
 
   /// Called before parsing a function declarator belonging to a function
   /// declaration.
   void ActOnStartFunctionDeclarationDeclarator(Declarator &D,
                                                unsigned TemplateParameterDepth);
 

clang/lib/Sema/SemaAvailability.cpp

 
 public:
   DiagnoseUnguardedAvailability(Sema &SemaRef, Decl *Ctx)
       : SemaRef(SemaRef), Ctx(Ctx) {
     AvailabilityStack.push_back(
         SemaRef.Context.getTargetInfo().getPlatformMinVersion());
   }
 
-  bool TraverseDecl(Decl *D) {
-    // Avoid visiting nested functions to prevent duplicate warnings.
-    if (!D || isa<FunctionDecl>(D))
-      return true;
-    return Base::TraverseDecl(D);
-  }
-
   bool TraverseStmt(Stmt *S) {
     if (!S)
       return true;
     StmtStack.push_back(S);
     bool Result = Base::TraverseStmt(S);
     StmtStack.pop_back();
     return Result;
   }
 
   void IssueDiagnostics(Stmt *S) { TraverseStmt(S); }
 
   bool TraverseIfStmt(IfStmt *If);
 
-  bool TraverseLambdaExpr(LambdaExpr *E) { return true; }
-
   // for 'case X:' statements, don't bother looking at the 'X'; it can't lead
   // to any useful diagnostics.
   bool TraverseCaseStmt(CaseStmt *CS) { return TraverseStmt(CS->getSubStmt()); }
 
   bool VisitObjCPropertyRefExpr(ObjCPropertyRefExpr *PRE) {
     if (PRE->isClassReceiver())
       DiagnoseDeclAvailability(PRE->getClassReceiver(), PRE->getReceiverLocation());
     return true;
void Sema::DiagnoseUnguardedAvailabilityViolations(Decl *D) {
   else if (auto *BD = dyn_cast<BlockDecl>(D))
     Body = BD->getBody();
 
   assert(Body && "Need a body here!");
 
   DiagnoseUnguardedAvailability(*this, D).IssueDiagnostics(Body);
 }
 
+FunctionScopeInfo *Sema::getCurFunctionAvailabilityContext() {
+  if (FunctionScopes.empty())
+    return nullptr;
+
+  // Conservatively search the entire current function scope context for
+  // availability violations. This ensures we always correctly analyze nested
+  // classes, blocks, lambdas, etc. that may or may not be inside if(@available)
+  // checks themselves.
+  return FunctionScopes.front();
+}
+
 void Sema::DiagnoseAvailabilityOfDecl(NamedDecl *D,
                                       ArrayRef<SourceLocation> Locs,
                                       const ObjCInterfaceDecl *UnknownObjCClass,
                                       bool ObjCPropertyAccess,
                                       bool AvoidPartialAvailabilityChecks,
                                       ObjCInterfaceDecl *ClassReceiver) {
   std::string Message;
   AvailabilityResult Result;
   const NamedDecl* OffendingDecl;
   // See if this declaration is unavailable, deprecated, or partial.
   std::tie(Result, OffendingDecl) =
       ShouldDiagnoseAvailabilityOfDecl(*this, D, &Message, ClassReceiver);
   if (Result == AR_Available)
     return;
 
   if (Result == AR_NotYetIntroduced) {
     if (AvoidPartialAvailabilityChecks)
       return;
 
     // We need to know the @available context in the current function to
     // diagnose this use, let DiagnoseUnguardedAvailabilityViolations do that
     // when we're done parsing the current function.
-    if (getCurFunctionOrMethodDecl()) {
-      getEnclosingFunction()->HasPotentialAvailabilityViolations = true;
-      return;
-    } else if (getCurBlock() || getCurLambda()) {
-      getCurFunction()->HasPotentialAvailabilityViolations = true;
+    if (FunctionScopeInfo *Context = getCurFunctionAvailabilityContext()) {
+      Context->HasPotentialAvailabilityViolations = true;
       return;
     }
   }
 
   const ObjCPropertyDecl *ObjCPDecl = nullptr;
   if (const auto *MD = dyn_cast<ObjCMethodDecl>(D)) {
     if (const ObjCPropertyDecl *PD = MD->findPropertyDecl()) {
       AvailabilityResult PDeclResult = PD->getAvailability(nullptr);
       if (PDeclResult == Result)
         ObjCPDecl = PD;
     }
   }
 
   EmitAvailabilityWarning(*this, Result, D, OffendingDecl, Message, Locs,
                           UnknownObjCClass, ObjCPDecl, ObjCPropertyAccess);
 }

clang/lib/Sema/SemaExpr.cpp

ExprResult Sema::ActOnObjCAvailabilityCheckExpr(
   auto Spec = llvm::find_if(AvailSpecs, [&](const AvailabilitySpec &Spec) {
     return Spec.getPlatform() == Platform;
   });
 
   VersionTuple Version;
   if (Spec != AvailSpecs.end())
     Version = Spec->getVersion();
 
-  // The use of `@available` in the enclosing function should be analyzed to
+  // The use of `@available` in the enclosing context should be analyzed to
   // warn when it's used inappropriately (i.e. not if(@available)).
-  if (getCurFunctionOrMethodDecl())
-    getEnclosingFunction()->HasPotentialAvailabilityViolations = true;
-  else if (getCurBlock() || getCurLambda())
-    getCurFunction()->HasPotentialAvailabilityViolations = true;
+  if (FunctionScopeInfo *Context = getCurFunctionAvailabilityContext())
+    Context->HasPotentialAvailabilityViolations = true;
 
   return new (Context)
       ObjCAvailabilityCheckExpr(Version, AtLoc, RParen, Context.BoolTy);
 }
 
 ExprResult Sema::CreateRecoveryExpr(SourceLocation Begin, SourceLocation End,
                                     ArrayRef<Expr *> SubExprs, QualType T) {
   if (!Context.getLangOpts().RecoveryAST)

clang/test/SemaObjC/unguarded-availability.m

int test_members() {
 
   (void)s.n.nested_member; // expected-warning{{'nested_member' is only available on macOS 10.12 or newer}} expected-note{{@available}}
 }
 
 void test_blocks() {
   (void) ^{
     func_10_12(); // expected-warning{{'func_10_12' is only available on macOS 10.12 or newer}} expected-note{{@available}}
   };
+
+  if (@available(macos 10.12, *))
+    (void) ^{
+      func_10_12();
+      (void) ^{
+        func_10_12();
+      };
+    };
 }
 
 void test_params(int_10_12 x); // expected-warning {{'int_10_12' is only available on macOS 10.12 or newer}} expected-note{{annotate 'test_params' with an availability attribute to silence this warning}}
 
 void test_params2(int_10_12 x) AVAILABLE_10_12; // no warn
 
 void (^topLevelBlockDecl)() = ^ {
   func_10_12(); // expected-warning{{'func_10_12' is only available on macOS 10.12 or newer}} expected-note{{@available}}
struct DontWarnTwice {
     }
   };
   void([] () {
     func_10_12(); // expected-warning{{'func_10_12' is only available on macOS 10.12 or newer}} expected-note{{@available}}
   });
   (void)(^ {
     func_10_12(); // expected-warning{{'func_10_12' is only available on macOS 10.12 or newer}} expected-note{{@available}}
   });
+
+  if (@available(macos 10.12, *)) {
+    void([]() {
+      func_10_12();
+      void([] () {
+        func_10_12();
+      });
+      struct DontWarn {
+        void f() {
+          func_10_12();
+        }
+      };
+    });
+  }
+
+  if (@available(macos 10.12, *)) {
+    struct DontWarn {
+      void f() {
+        func_10_12();
+        void([] () {
+          func_10_12();
+        });
+        struct DontWarn2 {
+          void f() {
+            func_10_12();
+          }
+        };
+      }
+    };
+  }
 }
 
 #endif
 
 struct InStruct { // expected-note{{annotate 'InStruct' with an availability attribute to silence}}
   new_int mem; // expected-warning{{'new_int' is only available on macOS 10.12 or newer}}
 
   struct { new_int mem; } anon; // expected-warning{{'new_int' is only available on macOS 10.12 or newer}} expected-note{{annotate anonymous struct with an availability attribute to silence}}

 -(new_int)meth; // expected-warning{{'new_int' is only available on macOS 10.12 or newer}} expected-note{{annotate 'meth' with an availability attribute}}
 @end
 
 @interface Proper // expected-note{{annotate 'Proper' with an availability attribute}}
 @property (class) new_int x; // expected-warning{{'new_int' is only available}}
 @end
 
 void with_local_struct() {
-  struct local { // expected-note{{annotate 'local' with an availability attribute}}
-    new_int x; // expected-warning{{'new_int' is only available}}
+  struct local { 
+    new_int x; // expected-warning{{'new_int' is only available}} expected-note{{enclose 'new_int' in an @available check}}
+  };
+  if (@available(macos 10.12, *)) {
+    struct DontWarn {
+      new_int x;
     };
   }
+}
 
 // rdar://33156429:
 // Avoid the warning on protocol requirements.
 
 AVAILABLE_10_12
 @protocol NewProtocol // expected-note {{'NewProtocol' has been marked as being introduced in macOS 10.12 here, but the deployment target is macOS 10.9.0}}
 @end