Bug 1339537 - Part 5: Produce better annotation reason diagnostics for implicit annotations, r=ehsan

This allows for the alignas(_) case to be distinguished from the
MOZ_NON_PARAM case through notes.

MozReview-Commit-ID: 4KIbzEKnmNU

build/clang-plugin/CustomTypeAnnotation.cpp

@@ -27,6 +27,8 @@ void CustomTypeAnnotation::dumpAnnotationReason(BaseCheck &Check,
       "%1 is a %0 type because it is an array of %0 type %2";
   const char* Templ =
       "%1 is a %0 type because it has a template argument %0 type %2";
+  const char* Implicit =
+      "%1 is a %0 type because %2";
 
   AnnotationReason Reason = directAnnotationReason(T);
   for (;;) {
@@ -54,6 +56,14 @@ void CustomTypeAnnotation::dumpAnnotationReason(BaseCheck &Check,
         << Pretty << T << Reason.Type;
       break;
     }
+    case RK_Implicit: {
+      const TagDecl *Declaration = T->getAsTagDecl();
+      assert(Declaration && "This type should be a TagDecl");
+
+      Check.diag(Declaration->getLocation(), Implicit, DiagnosticIDs::Note)
+        << Pretty << T << Reason.ImplicitReason;
+      return;
+    }
     default:
       // FIXME (bug 1203263): note the original annotation.
       return;
@@ -64,24 +74,21 @@ void CustomTypeAnnotation::dumpAnnotationReason(BaseCheck &Check,
   }
 }
 
-bool CustomTypeAnnotation::hasLiteralAnnotation(QualType T) const {
-#if CLANG_VERSION_FULL >= 306
-  if (const TagDecl *D = T->getAsTagDecl()) {
-#else
-  if (const CXXRecordDecl *D = T->getAsCXXRecordDecl()) {
-#endif
-    return hasFakeAnnotation(D) || hasCustomAnnotation(D, Spelling);
-  }
-  return false;
-}
-
 CustomTypeAnnotation::AnnotationReason
 CustomTypeAnnotation::directAnnotationReason(QualType T) {
-  if (hasLiteralAnnotation(T)) {
-    AnnotationReason Reason = {T, RK_Direct, nullptr};
+  if (const TagDecl *D = T->getAsTagDecl()) {
+    if (hasCustomAnnotation(D, Spelling)) {
+      AnnotationReason Reason = {T, RK_Direct, nullptr, ""};
       return Reason;
     }
 
+    std::string ImplAnnotReason = getImplicitReason(D);
+    if (!ImplAnnotReason.empty()) {
+      AnnotationReason Reason = {T, RK_Implicit, nullptr, ImplAnnotReason};
+      return Reason;
+    }
+  }
+
   // Check if we have a cached answer
   void *Key = T.getAsOpaquePtr();
   ReasonCache::iterator Cached = Cache.find(T.getAsOpaquePtr());
@@ -93,7 +100,7 @@ CustomTypeAnnotation::directAnnotationReason(QualType T) {
   if (const clang::ArrayType *Array = T->getAsArrayTypeUnsafe()) {
     if (hasEffectiveAnnotation(Array->getElementType())) {
       AnnotationReason Reason = {Array->getElementType(), RK_ArrayElement,
-                                 nullptr};
+                                 nullptr, ""};
       Cache[Key] = Reason;
       return Reason;
     }
@@ -106,7 +113,7 @@ CustomTypeAnnotation::directAnnotationReason(QualType T) {
 
       for (const CXXBaseSpecifier &Base : Declaration->bases()) {
         if (hasEffectiveAnnotation(Base.getType())) {
-          AnnotationReason Reason = {Base.getType(), RK_BaseClass, nullptr};
+          AnnotationReason Reason = {Base.getType(), RK_BaseClass, nullptr, ""};
           Cache[Key] = Reason;
           return Reason;
         }
@@ -115,7 +122,7 @@ CustomTypeAnnotation::directAnnotationReason(QualType T) {
       // Recurse into members
       for (const FieldDecl *Field : Declaration->fields()) {
         if (hasEffectiveAnnotation(Field->getType())) {
-          AnnotationReason Reason = {Field->getType(), RK_Field, Field};
+          AnnotationReason Reason = {Field->getType(), RK_Field, Field, ""};
           Cache[Key] = Reason;
           return Reason;
         }
@@ -140,7 +147,7 @@ CustomTypeAnnotation::directAnnotationReason(QualType T) {
     }
   }
 
-  AnnotationReason Reason = {QualType(), RK_None, nullptr};
+  AnnotationReason Reason = {QualType(), RK_None, nullptr, ""};
   Cache[Key] = Reason;
   return Reason;
 }
@@ -151,7 +158,7 @@ CustomTypeAnnotation::tmplArgAnnotationReason(ArrayRef<TemplateArgument> Args) {
     if (Arg.getKind() == TemplateArgument::Type) {
       QualType Type = Arg.getAsType();
       if (hasEffectiveAnnotation(Type)) {
-        AnnotationReason Reason = {Type, RK_TemplateInherited, nullptr};
+        AnnotationReason Reason = {Type, RK_TemplateInherited, nullptr, ""};
         return Reason;
       }
     } else if (Arg.getKind() == TemplateArgument::Pack) {
@@ -162,6 +169,6 @@ CustomTypeAnnotation::tmplArgAnnotationReason(ArrayRef<TemplateArgument> Args) {
     }
   }
 
-  AnnotationReason Reason = {QualType(), RK_None, nullptr};
+  AnnotationReason Reason = {QualType(), RK_None, nullptr, ""};
   return Reason;
 }

build/clang-plugin/CustomTypeAnnotation.h

@@ -15,11 +15,13 @@ class CustomTypeAnnotation {
     RK_BaseClass,
     RK_Field,
     RK_TemplateInherited,
+    RK_Implicit,
   };
   struct AnnotationReason {
     QualType Type;
     ReasonKind Kind;
     const FieldDecl *Field;
+    std::string ImplicitReason;
 
     bool valid() const { return Kind != RK_None; }
   };
@@ -53,13 +55,14 @@ public:
   }
 
 private:
-  bool hasLiteralAnnotation(QualType T) const;
   AnnotationReason directAnnotationReason(QualType T);
   AnnotationReason tmplArgAnnotationReason(ArrayRef<TemplateArgument> Args);
 
 protected:
-  // Allow subclasses to apply annotations to external code:
-  virtual bool hasFakeAnnotation(const TagDecl *D) const { return false; }
+  // Allow subclasses to apply annotations for reasons other than a direct
+  // annotation. A non-empty string return value means that the object D is
+  // annotated, and should contain the reason why.
+  virtual std::string getImplicitReason(const TagDecl *D) const { return ""; }
 };
 
 extern CustomTypeAnnotation StackClass;

build/clang-plugin/MemMoveAnnotation.h

@@ -17,7 +17,7 @@ public:
   virtual ~MemMoveAnnotation() {}
 
 protected:
-  bool hasFakeAnnotation(const TagDecl *D) const override {
+  std::string getImplicitReason(const TagDecl *D) const override {
     // Annotate everything in ::std, with a few exceptions; see bug
     // 1201314 for discussion.
     if (getDeclarationNamespace(D) == "std") {
@@ -48,11 +48,11 @@ protected:
           Name == "_Atomic_llong" ||
           Name == "_Atomic_ullong" ||
           Name == "_Atomic_address") {
-        return false;
+        return "";
       }
-      return true;
+      return "it is an stl-provided type not guaranteed to be memmove-able";
     }
-    return false;
+    return "";
   }
 };
 

build/clang-plugin/NonParamInsideFunctionDeclChecker.cpp

@@ -15,12 +15,11 @@ protected:
   // MSVC limitations which prevent passing explcitly aligned types by value as
   // parameters. This overload of hasFakeAnnotation injects fake MOZ_NON_PARAM
   // annotations onto these types.
-  bool hasFakeAnnotation(const TagDecl *D) const override {
+  std::string getImplicitReason(const TagDecl *D) const override {
     // Check if the decl itself has an AlignedAttr on it.
     for (const Attr *A : D->attrs()) {
       if (isa<AlignedAttr>(A)) {
-        D->dump();
-        return true;
+        return "it has an alignas(_) annotation";
       }
     }
 
@@ -29,9 +28,7 @@ protected:
       for (auto F : RD->fields()) {
         for (auto A : F->attrs()) {
           if (isa<AlignedAttr>(A)) {
-            D->dump();
-
-            return true;
+            return ("member '" + F->getName() + "' has an alignas(_) annotation").str();
           }
         }
       }
@@ -39,7 +36,7 @@ protected:
 
     // We don't need to check the types of fields, as the CustomTypeAnnotation
     // infrastructure will handle that for us.
-    return false;
+    return "";
   }
 };
 NonParamAnnotation NonParam;
@@ -111,6 +108,8 @@ void NonParamInsideFunctionDeclChecker::check(
              DiagnosticIDs::Note)
           << Spec->getSpecializedTemplate();
       }
+
+      NonParam.dumpAnnotationReason(*this, T, p->getLocation());
     }
   }
 }

build/clang-plugin/tests/TestNonMemMovableStd.cpp

@@ -6,10 +6,10 @@ class MOZ_NEEDS_MEMMOVABLE_TYPE Mover { T mForceInst; }; // expected-error-re 4
 namespace std {
 // In theory defining things in std:: like this invokes undefined
 // behavior, but in practice it's good enough for this test case.
-template<class C> class basic_string { };
+template<class C> class basic_string { }; // expected-note 2 {{'std::basic_string<char>' is a non-memmove()able type because it is an stl-provided type not guaranteed to be memmove-able}} expected-note {{'std::string' (aka 'basic_string<char>') is a non-memmove()able type because it is an stl-provided type not guaranteed to be memmove-able}}
 typedef basic_string<char> string;
 template<class T, class U> class pair { T mT; U mU; }; // expected-note {{std::pair<bool, std::basic_string<char> >' is a non-memmove()able type because member 'mU' is a non-memmove()able type 'std::basic_string<char>'}}
-class arbitrary_name { };
+class arbitrary_name { }; // expected-note {{'std::arbitrary_name' is a non-memmove()able type because it is an stl-provided type not guaranteed to be memmove-able}}
 }
 
 class HasString { std::string m; }; // expected-note {{'HasString' is a non-memmove()able type because member 'm' is a non-memmove()able type 'std::string' (aka 'basic_string<char>')}}

build/clang-plugin/tests/TestNonParameterChecker.cpp

@@ -7,9 +7,9 @@ class MOZ_NON_PARAM NonParamClass {};
 enum MOZ_NON_PARAM NonParamEnum { X, Y, Z };
 enum class MOZ_NON_PARAM NonParamEnumClass { X, Y, Z };
 
-struct HasNonParamStruct { NonParam x; int y; };
-union HasNonParamUnion { NonParam x; int y; };
-struct HasNonParamStructUnion { HasNonParamUnion z; };
+struct HasNonParamStruct { NonParam x; int y; }; // expected-note 14 {{'HasNonParamStruct' is a non-param type because member 'x' is a non-param type 'NonParam'}}
+union HasNonParamUnion { NonParam x; int y; }; // expected-note 18 {{'HasNonParamUnion' is a non-param type because member 'x' is a non-param type 'NonParam'}}
+struct HasNonParamStructUnion { HasNonParamUnion z; }; // expected-note 9 {{'HasNonParamStructUnion' is a non-param type because member 'z' is a non-param type 'HasNonParamUnion'}}
 
 #define MAYBE_STATIC
 #include "NonParameterTestCases.h"
@@ -180,10 +180,10 @@ void testLambda()
 
 // Check that alignas() implies the MOZ_NON_PARAM attribute.
 
-struct alignas(8) AlignasStruct { char a; };
+struct alignas(8) AlignasStruct { char a; }; // expected-note {{'AlignasStruct' is a non-param type because it has an alignas(_) annotation}}
 void takesAlignasStruct(AlignasStruct x) { } // expected-error {{Type 'AlignasStruct' must not be used as parameter}} expected-note {{Please consider passing a const reference instead}}
 void takesAlignasStructByRef(const AlignasStruct& x) { }
 
-struct AlignasMember { alignas(8) char a; };
+struct AlignasMember { alignas(8) char a; }; // expected-note {{'AlignasMember' is a non-param type because member 'a' has an alignas(_) annotation}}
 void takesAlignasMember(AlignasMember x) { } // expected-error {{Type 'AlignasMember' must not be used as parameter}} expected-note {{Please consider passing a const reference instead}}
 void takesAlignasMemberByRef(const AlignasMember& x) { }