[Sema][ObjC] Issue a warning when a method declared in a protocol is non-escaping but the corresponding method in the implementation is escaping.

[ahatanak]

Summary

This patch makes Sema issue a warning when there is an extension that conforms to a protocol that declares a non-escaping method and the corresponding method in the implementation isn't non-escaping.

rdar://problem/39548196

[vsapsai]

Overall looks good to me. Maybe add a test when a protocol is declared for an interface, not for a category. Something like

__attribute__((objc_root_class))
@interface C4 <NoescapeProt>
-(void) m0:(int*) p; // expected-warning {{parameter of overriding method should be annotated with __attribute__((noescape))}}
@end

@implementation C4
-(void) m0:(int*) p {
}
@end

Looks like there is no such test already and it seems different enough to be worth adding. The idea for this test was triggered by code for (auto *C : IDecl->visible_categories()) and the goal is to cover a case when protocol is not for a category.

Also I had a few ideas for tests when the warning isn't required and it is absent. But I'm not sure they are actually valuable. If you are interested, we can discuss it in more details.

Another feedback is an idea for potential improvement: have a note pointing to the place where protocol conformity is declared. Currently the warning looks like

code_review.m:16:19: warning: parameter of overriding method should be annotated with __attribute__((noescape))
      [-Wmissing-noescape]
-(void) m0:(int*) p { // expected-warning {{parameter of overriding method should be annotated with __attri...
                  ^
code_review.m:2:44: note: parameter of overridden method is annotated with __attribute__((noescape))
-(void) m0:(int*)__attribute__((noescape)) p; // expected-note {{parameter of overridden method is annotate...
                                           ^

and we can see the method both in implementation and in protocol. But in some cases it might be unclear where exactly that protocol was added to your class. I'm not sure this change is sufficiently useful, it's more for discussion.

[ahatanak]

Add a test case for an interface conforming to a non-escaping protocol.

In D49119#1164285, @vsapsai wrote:

Also I had a few ideas for tests when the warning isn't required and it is absent. But I'm not sure they are actually valuable. If you are interested, we can discuss it in more details.

Could you elaborate on what kind of tests you have in mind?

Another feedback is an idea for potential improvement: have a note pointing to the place where protocol conformity is declared. Currently the warning looks like

code_review.m:16:19: warning: parameter of overriding method should be annotated with __attribute__((noescape))
      [-Wmissing-noescape]
-(void) m0:(int*) p { // expected-warning {{parameter of overriding method should be annotated with __attri...
                  ^
code_review.m:2:44: note: parameter of overridden method is annotated with __attribute__((noescape))
-(void) m0:(int*)__attribute__((noescape)) p; // expected-note {{parameter of overridden method is annotate...
                                           ^

and we can see the method both in implementation and in protocol. But in some cases it might be unclear where exactly that protocol was added to your class. I'm not sure this change is sufficiently useful, it's more for discussion.

I think it's possible to add a note that helps the user find where the category conforming to the non-escaping protocol is declared.

[vsapsai]

In D49119#1176047, @ahatanak wrote:

In D49119#1164285, @vsapsai wrote:

Also I had a few ideas for tests when the warning isn't required and it is absent. But I'm not sure they are actually valuable. If you are interested, we can discuss it in more details.

Could you elaborate on what kind of tests you have in mind?

• declaring noescape on implementation when nothing like that was mentioned in interface or protocols;
• have class method and instance method with the same name, only one of them is noescape, test that we don't show spurious warning in this case;
• try selector names that look similar but are different, like -foo, -foo:, -foo:: This test suggestion isn't really related to noescape, I just got carried away.

[vsapsai]

Looks good to me. Any further improvements are up to you.

[ahatanak]

Thanks, I'll update the patch and commit it today.

In D49119#1176139, @vsapsai wrote:

In D49119#1176047, @ahatanak wrote:

In D49119#1164285, @vsapsai wrote:

Also I had a few ideas for tests when the warning isn't required and it is absent. But I'm not sure they are actually valuable. If you are interested, we can discuss it in more details.

Could you elaborate on what kind of tests you have in mind?

• declaring noescape on implementation when nothing like that was mentioned in interface or protocols;

Maybe we should consider this, but I don't think this is incorrect functionally. If you pass an object to such methods, clang's IRGen will just emit the unoptimized code that is emitted for calls to functions/methods taking escaping parameters (e.g., no 'nocapture' on parameters, no stack block optimization implemented in r337580). The opposite case (non-escaping interface and escaping implementation) is incorrect and can cause problems, so we need some diagnostics.

• have class method and instance method with the same name, only one of them is noescape, test that we don't show spurious warning in this case;

I can add a test case for this.

• try selector names that look similar but are different, like -foo, -foo:, -foo:: This test suggestion isn't really related to noescape, I just got carried away.

[ahatanak]

• Produce a note that tells users where the class extension conforming to the protocol containing the non-escaping method is declared.
• Add a class method that has the same name as the instance method and check that no spurious warnings are issued.

include/clang/Basic/DiagnosticSemaKinds.td

def err_conflicting_overriding_cc_attributes : Error<
   "virtual function %0 has different calling convention attributes "
   "%diff{($) than the function it overrides (which has calling convention $)|"
   "than the function it overrides}1,2">;
 def warn_overriding_method_missing_noescape : Warning<
   "parameter of overriding method should be annotated with "
   "__attribute__((noescape))">, InGroup<MissingNoEscape>;
 def note_overridden_marked_noescape : Note<
   "parameter of overridden method is annotated with __attribute__((noescape))">;
+def note_cat_conform_to_noescape_prot : Note<
+  "%select{category|class extension}0 conforms to protocol %1 which defines method %2">;
 
 def err_covariant_return_inaccessible_base : Error<
   "invalid covariant return for virtual function: %1 is a "
   "%select{private|protected}2 base class of %0">, AccessControl;
 def err_covariant_return_ambiguous_derived_to_base_conv : Error<
   "return type of virtual function %3 is not covariant with the return type of "
   "the function it overrides (ambiguous conversion from derived class "
   "%0 to base class %1:%2)">;

lib/Sema/SemaDeclObjC.cpp

bool Sema::checkInitMethod(ObjCMethodDecl *method,
   }
 
   // Otherwise, it's an error.
   Diag(loc, diag::err_arc_init_method_unrelated_result_type);
   method->setInvalidDecl();
   return true;
 }
 
+/// Issue a warning if the parameter of the overridden method is non-escaping
+/// but the parameter of the overriding method is not.
+static bool diagnoseNoescape(const ParmVarDecl *NewD, const ParmVarDecl *OldD,
+                             Sema &S) {
+  if (OldD->hasAttr<NoEscapeAttr>() && !NewD->hasAttr<NoEscapeAttr>()) {
+    S.Diag(NewD->getLocation(), diag::warn_overriding_method_missing_noescape);
+    S.Diag(OldD->getLocation(), diag::note_overridden_marked_noescape);
+    return false;
+  }
+
+  return true;
+}
+
+/// Produce additional diagnostics if a category conforms to a protocol that
+/// defines a method taking a non-escaping parameter.
+static void diagnoseNoescape(const ParmVarDecl *NewD, const ParmVarDecl *OldD,
+                             const ObjCCategoryDecl *CD,
+                             const ObjCProtocolDecl *PD, Sema &S) {
+  if (!diagnoseNoescape(NewD, OldD, S))
+    S.Diag(CD->getLocation(), diag::note_cat_conform_to_noescape_prot)
+        << CD->IsClassExtension() << PD
+        << cast<ObjCMethodDecl>(NewD->getDeclContext());
+}
+
 void Sema::CheckObjCMethodOverride(ObjCMethodDecl *NewMethod, 
                                    const ObjCMethodDecl *Overridden) {
   if (Overridden->hasRelatedResultType() && 
       !NewMethod->hasRelatedResultType()) {
     // This can only happen when the method follows a naming convention that
     // implies a related result type, and the original (overridden) method has
     // a suitable return type, but the new (overriding) method does not have
     // a suitable return type.
if (newDecl->hasAttr<NSConsumedAttr>() !=
         oldDecl->hasAttr<NSConsumedAttr>()) {
       Diag(newDecl->getLocation(),
            getLangOpts().ObjCAutoRefCount
                ? diag::err_nsconsumed_attribute_mismatch
                : diag::warn_nsconsumed_attribute_mismatch);
       Diag(oldDecl->getLocation(), diag::note_previous_decl) << "parameter";
     }
 
-    // A parameter of the overriding method should be annotated with noescape
-    // if the corresponding parameter of the overridden method is annotated.
-    if (oldDecl->hasAttr<NoEscapeAttr>() && !newDecl->hasAttr<NoEscapeAttr>()) {
-      Diag(newDecl->getLocation(),
-           diag::warn_overriding_method_missing_noescape);
-      Diag(oldDecl->getLocation(), diag::note_overridden_marked_noescape);
-    }
+    diagnoseNoescape(newDecl, oldDecl, *this);
   }
 }
 
 /// Check a method declaration for compatibility with the Objective-C
 /// ARC conventions.
 bool Sema::CheckARCMethodDecl(ObjCMethodDecl *method) {
   ObjCMethodFamily family = method->getMethodFamily();
   switch (family) {
if (ObjCInterfaceDecl *IDecl = ImpDecl->getClassInterface()) {
 
         // Warn about defining -dealloc in a category.
         if (isa<ObjCCategoryImplDecl>(ImpDecl) && IMD->isOverriding() &&
             ObjCMethod->getSelector().getMethodFamily() == OMF_dealloc) {
           Diag(ObjCMethod->getLocation(), diag::warn_dealloc_in_category)
             << ObjCMethod->getDeclName();
         }
       }
+
+      // Warn if a method declared in a protocol to which a category or
+      // extension conforms is non-escaping and the implementation's method is
+      // escaping.
+      for (auto *C : IDecl->visible_categories())
+        for (auto &P : C->protocols())
+          if (auto *IMD = P->lookupMethod(ObjCMethod->getSelector(),
+                                          ObjCMethod->isInstanceMethod())) {
+            assert(ObjCMethod->parameters().size() ==
+                       IMD->parameters().size() &&
+                   "Methods have different number of parameters");
+            auto OI = IMD->param_begin(), OE = IMD->param_end();
+            auto NI = ObjCMethod->param_begin();
+            for (; OI != OE; ++OI, ++NI)
+              diagnoseNoescape(*NI, *OI, C, P, *this);
+          }
     }
   } else {
     cast<DeclContext>(ClassDecl)->addDecl(ObjCMethod);
   }
 
   if (PrevMethod) {
     // You can never have two method definitions with the same name.
     Diag(ObjCMethod->getLocation(), diag::err_duplicate_method_decl)

test/SemaObjCXX/noescape.mm

#if __cplusplus < 201406
   // expected-error@-4 {{non-type template argument of type 'void (*)(__attribute__((noescape)) int *)' cannot be converted to a value of type 'void (*)(int *)'}}
   // expected-error@-4 {{non-type template argument of type 'void (*)(int *)' cannot be converted to a value of type 'void (*)(__attribute__((noescape)) int *)'}}
 #else
   // expected-error@-6 {{value of type 'void (*)(int *)' is not implicitly convertible to 'void (*)(__attribute__((noescape)) int *)'}}
 #endif
 
   S5<&noescapeFunc2> ne1;
 }
+
+@protocol NoescapeProt
+-(void) m0:(int*)__attribute__((noescape)) p; // expected-note 2 {{parameter of overridden method is annotated with __attribute__((noescape))}}
++(void) m1:(int*)__attribute__((noescape)) p;
+-(void) m1:(int*) p;
+@end
+
+__attribute__((objc_root_class))
+@interface C3
+-(void) m0:(int*) p;
++(void) m1:(int*)__attribute__((noescape)) p;
+-(void) m1:(int*) p;
+@end
+
+@interface C3 () <NoescapeProt> // expected-note {{class extension conforms to protocol 'NoescapeProt' which defines method 'm0:'}}
+@end
+
+@implementation C3
+-(void) m0:(int*) p { // expected-warning {{parameter of overriding method should be annotated with __attribute__((noescape))}}
+}
++(void) m1:(int*)__attribute__((noescape)) p {
+}
+-(void) m1:(int*) p {
+}
+@end
+
+__attribute__((objc_root_class))
+@interface C4 <NoescapeProt>
+-(void) m0:(int*) p; // expected-warning {{parameter of overriding method should be annotated with __attribute__((noescape))}}
+@end
+
+@implementation C4
+-(void) m0:(int*) p {
+}
++(void) m1:(int*)__attribute__((noescape)) p {
+}
+-(void) m1:(int*) p {
+}
+@end