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

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak updated this revision to Diff 157398.
ahatanak added a comment.

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

In https://reviews.llvm.org/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.


Repository:
  rC Clang

https://reviews.llvm.org/D49119

Files:
  lib/Sema/SemaDeclObjC.cpp
  test/SemaObjCXX/noescape.mm


Index: test/SemaObjCXX/noescape.mm
===================================================================
--- test/SemaObjCXX/noescape.mm
+++ test/SemaObjCXX/noescape.mm
@@ -88,3 +88,30 @@
 
   S5<&noescapeFunc2> ne1;
 }
+
+ at protocol NoescapeProt
+-(void) m0:(int*)__attribute__((noescape)) p; // expected-note 2 {{parameter of overridden method is annotated with __attribute__((noescape))}}
+ at end
+
+__attribute__((objc_root_class))
+ at interface C3
+-(void) m0:(int*) p;
+ at end
+
+ at interface C3 () <NoescapeProt>
+ at end
+
+ at implementation C3
+-(void) m0:(int*) p { // expected-warning {{parameter of overriding method should be annotated with __attribute__((noescape))}}
+}
+ at end
+
+__attribute__((objc_root_class))
+ at interface C4 <NoescapeProt>
+-(void) m0:(int*) p; // expected-warning {{parameter of overriding method should be annotated with __attribute__((noescape))}}
+ at end
+
+ at implementation C4
+-(void) m0:(int*) p {
+}
+ at end
Index: lib/Sema/SemaDeclObjC.cpp
===================================================================
--- lib/Sema/SemaDeclObjC.cpp
+++ lib/Sema/SemaDeclObjC.cpp
@@ -109,6 +109,16 @@
   return true;
 }
 
+static void diagnoseNoescape(const ParmVarDecl *NewMD, const ParmVarDecl *OldMD,
+                             Sema &S) {
+  // A parameter of the overriding method should be annotated with noescape
+  // if the corresponding parameter of the overridden method is annotated.
+  if (OldMD->hasAttr<NoEscapeAttr>() && !NewMD->hasAttr<NoEscapeAttr>()) {
+    S.Diag(NewMD->getLocation(), diag::warn_overriding_method_missing_noescape);
+    S.Diag(OldMD->getLocation(), diag::note_overridden_marked_noescape);
+  }
+}
+
 void Sema::CheckObjCMethodOverride(ObjCMethodDecl *NewMethod, 
                                    const ObjCMethodDecl *Overridden) {
   if (Overridden->hasRelatedResultType() && 
@@ -192,13 +202,7 @@
       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);
   }
 }
 
@@ -4643,6 +4647,22 @@
             << 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, *this);
+          }
     }
   } else {
     cast<DeclContext>(ClassDecl)->addDecl(ObjCMethod);


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D49119.157398.patch
Type: text/x-patch
Size: 3387 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20180725/d131c245/attachment.bin>