[ODRHash] Hash `ObjCPropertyDecl` and diagnose discovered mismatches.

[vsapsai]

Rebase.

[vsapsai]

Rebase.

[vsapsai]

Ping.

[bruno]

Very nice how this has improved since the version I worked on in the past, thanks!

[vsapsai]

Thanks for the review!

clang/include/clang/AST/ODRDiagsEmitter.h

enum ODRMismatchDecl {
     Field,
     CXXMethod,
     TypeAlias,
     TypeDef,
     Var,
     Friend,
     FunctionTemplate,
     ObjCMethod,
+    ObjCProperty,
     Other
   };
 
   struct DiffResult {
     const Decl *FirstDecl = nullptr, *SecondDecl = nullptr;
     ODRMismatchDecl FirstDiffType = Other, SecondDiffType = Other;
   };
 
private:
   ///
   /// Returns true if found a mismatch and diagnosed it.
   bool diagnoseSubMismatchObjCMethod(const NamedDecl *FirstObjCContainer,
                                      StringRef FirstModule,
                                      StringRef SecondModule,
                                      const ObjCMethodDecl *FirstMethod,
                                      const ObjCMethodDecl *SecondMethod) const;
 
+  /// Check if Objective-C properties are the same and diagnose if different.
+  ///
+  /// Returns true if found a mismatch and diagnosed it.
+  bool
+  diagnoseSubMismatchObjCProperty(const NamedDecl *FirstObjCContainer,
+                                  StringRef FirstModule, StringRef SecondModule,
+                                  const ObjCPropertyDecl *FirstProp,
+                                  const ObjCPropertyDecl *SecondProp) const;
+
 private:
   DiagnosticsEngine &Diags;
   const ASTContext &Context;
   const LangOptions &LangOpts;
 };
 
 } // namespace clang
 
 #endif

clang/include/clang/Basic/DiagnosticASTKinds.td

def note_module_odr_violation_template_parameter : Note <
   "template parameter with %select{no |}2default argument|"
   "template parameter with different default argument}1">;
 
 def err_module_odr_violation_mismatch_decl : Error<
   "%q0 has different definitions in different modules; first difference is "
   "%select{definition in module '%2'|defined here}1 found "
   "%select{end of class|public access specifier|private access specifier|"
   "protected access specifier|static assert|field|method|type alias|typedef|"
-  "data member|friend declaration|function template|method}3">;
+  "data member|friend declaration|function template|method|property}3">;
 def note_module_odr_violation_mismatch_decl : Note<"but in '%0' found "
   "%select{end of class|public access specifier|private access specifier|"
   "protected access specifier|static assert|field|method|type alias|typedef|"
-  "data member|friend declaration|function template|method}1">;
+  "data member|friend declaration|function template|method|property}1">;
 
 def err_module_odr_violation_record : Error<
   "%q0 has different definitions in different modules; first difference is "
   "%select{definition in module '%2'|defined here}1 found "
   "%select{"
   "static assert with condition|"
   "static assert with message|"
   "static assert with %select{|no }4message|"
def note_module_odr_violation_method_params : Note<"but in '%0' found "
   "%select{method %3|constructor|destructor}2 "
     "that has %4 parameter%s4|"
   "%select{method %3|constructor|destructor}2 "
     "with %ordinal4 parameter of type %5%select{| decayed from %7}6|"
   "%select{method %3|constructor|destructor}2 "
     "with %ordinal4 parameter named %5"
   "}1">;
 
+def err_module_odr_violation_objc_property : Error<
+  "%q0 has different definitions in different modules; first difference is "
+  "%select{definition in module '%2'|defined here}1 found "
+  "%select{"
+  "property %4|"
+  "property %4 with type %5|"
+  "%select{no|'required'|'optional'}4 property control|"
+  "property %4 with %select{default |}6'%select{none|readonly|getter|assign|"
+    "readwrite|retain|copy|nonatomic|setter|atomic|weak|strong|"
+    "unsafe_unretained|nullability|null_resettable|class|direct}5' attribute"
+  "}3">;
+def note_module_odr_violation_objc_property : Note<
+  "but in '%0' found "
+  "%select{"
+  "property %2|"
+  "property %2 with type %3|"
+  "%select{no|'required'|'optional'}2 property control|"
+  "property %2 with different '%select{none|readonly|getter|assign|"
+    "readwrite|retain|copy|nonatomic|setter|atomic|weak|strong|"
+    "unsafe_unretained|nullability|null_resettable|class|direct}3' attribute"
+  "}1">;
+
 def err_module_odr_violation_mismatch_decl_unknown : Error<
   "%q0 %select{with definition in module '%2'|defined here}1 has different "
   "definitions in different modules; first difference is this "
   "%select{||||static assert|field|method|type alias|typedef|data member|"
   "friend declaration|function template|method|"
-  "unexpected decl}3">;
+  "property|unexpected decl}3">;
 def note_module_odr_violation_mismatch_decl_unknown : Note<
   "but in '%0' found "
   "%select{||||different static assert|different field|different method|"
   "different type alias|different typedef|different data member|"
   "different friend declaration|different function template|different method|"
-  "another unexpected decl}1">;
+  "different property|another unexpected decl}1">;
 
 
 def remark_sanitize_address_insert_extra_padding_accepted : Remark<
     "-fsanitize-address-field-padding applied to %0">, ShowInSystemHeader,
     InGroup<SanitizeAddressRemarks>;
 def remark_sanitize_address_insert_extra_padding_rejected : Remark<
     "-fsanitize-address-field-padding ignored for %0 because it "
     "%select{is not C++|is packed|is a union|is trivially copyable|"

clang/lib/AST/ODRDiagsEmitter.cpp

if (FirstName != SecondName) {
     DiagError(Name) << FirstName;
     DiagNote(Name) << SecondName;
     return true;
   }
 
   return false;
 }
 
+bool ODRDiagsEmitter::diagnoseSubMismatchObjCProperty(
+    const NamedDecl *FirstObjCContainer, StringRef FirstModule,
+    StringRef SecondModule, const ObjCPropertyDecl *FirstProp,
+    const ObjCPropertyDecl *SecondProp) const {
+  enum ODRPropertyDifference {
+    Name,
+    Type,
+    ControlLevel, // optional/required
+    Attribute,
+  };
+
+  auto DiagError = [FirstObjCContainer, FirstModule, FirstProp,
+                    this](SourceLocation Loc, ODRPropertyDifference DiffType) {
+    return Diag(Loc, diag::err_module_odr_violation_objc_property)
+           << FirstObjCContainer << FirstModule.empty() << FirstModule
+           << FirstProp->getSourceRange() << DiffType;
+  };
+  auto DiagNote = [SecondModule, SecondProp,
+                   this](SourceLocation Loc, ODRPropertyDifference DiffType) {
+    return Diag(Loc, diag::note_module_odr_violation_objc_property)
+           << SecondModule << SecondProp->getSourceRange() << DiffType;
+  };
+
+  IdentifierInfo *FirstII = FirstProp->getIdentifier();
+  IdentifierInfo *SecondII = SecondProp->getIdentifier();
+  if (FirstII->getName() != SecondII->getName()) {
+    DiagError(FirstProp->getLocation(), Name) << FirstII;
+    DiagNote(SecondProp->getLocation(), Name) << SecondII;
+    return true;
+  }
+  if (computeODRHash(FirstProp->getType()) !=
+      computeODRHash(SecondProp->getType())) {
+    DiagError(FirstProp->getLocation(), Type)
+        << FirstII << FirstProp->getType();
+    DiagNote(SecondProp->getLocation(), Type)
+        << SecondII << SecondProp->getType();
+    return true;
+  }
+  if (FirstProp->getPropertyImplementation() !=
+      SecondProp->getPropertyImplementation()) {
+    DiagError(FirstProp->getLocation(), ControlLevel)
+        << FirstProp->getPropertyImplementation();
+    DiagNote(SecondProp->getLocation(), ControlLevel)
+        << SecondProp->getPropertyImplementation();
+    return true;
+  }
+
+  // Go over the property attributes and stop at the first mismatch.
+  unsigned FirstAttrs = (unsigned)FirstProp->getPropertyAttributes();
+  unsigned SecondAttrs = (unsigned)SecondProp->getPropertyAttributes();
+  if (FirstAttrs != SecondAttrs) {
+    for (unsigned I = 0; I < NumObjCPropertyAttrsBits; ++I) {
+      unsigned CheckedAttr = (1 << I);
+      if ((FirstAttrs & CheckedAttr) == (SecondAttrs & CheckedAttr))
+        continue;
+
+      bool IsFirstWritten =
+          (unsigned)FirstProp->getPropertyAttributesAsWritten() & CheckedAttr;
+      bool IsSecondWritten =
+          (unsigned)SecondProp->getPropertyAttributesAsWritten() & CheckedAttr;
+      DiagError(IsFirstWritten ? FirstProp->getLParenLoc()
+                               : FirstProp->getLocation(),
+                Attribute)
+          << FirstII << (I + 1) << IsFirstWritten;
+      DiagNote(IsSecondWritten ? SecondProp->getLParenLoc()
+                               : SecondProp->getLocation(),
+               Attribute)
+          << SecondII << (I + 1);
+      return true;
+    }
+  }
+
+  return false;
+}
+
 ODRDiagsEmitter::DiffResult
 ODRDiagsEmitter::FindTypeDiffs(DeclHashes &FirstHashes,
                                DeclHashes &SecondHashes) {
   auto DifferenceSelector = [](const Decl *D) {
     assert(D && "valid Decl required");
     switch (D->getKind()) {
     default:
       return Other;
auto DifferenceSelector = [](const Decl *D) {
     case Decl::Var:
       return Var;
     case Decl::Friend:
       return Friend;
     case Decl::FunctionTemplate:
       return FunctionTemplate;
     case Decl::ObjCMethod:
       return ObjCMethod;
+    case Decl::ObjCProperty:
+      return ObjCProperty;
     }
   };
 
   DiffResult DR;
   auto FirstIt = FirstHashes.begin();
   auto SecondIt = SecondHashes.begin();
   while (FirstIt != FirstHashes.end() || SecondIt != SecondHashes.end()) {
     if (FirstIt != FirstHashes.end() && SecondIt != SecondHashes.end() &&
bool ODRDiagsEmitter::diagnoseMismatch(
   assert(FirstDiffType == SecondDiffType);
   switch (FirstDiffType) {
   case Other:
   case EndOfClass:
   case PublicSpecifer:
   case PrivateSpecifer:
   case ProtectedSpecifer:
   case ObjCMethod:
+  case ObjCProperty:
     llvm_unreachable("Invalid diff type");
 
   case StaticAssert: {
     const StaticAssertDecl *FirstSA = cast<StaticAssertDecl>(FirstDecl);
     const StaticAssertDecl *SecondSA = cast<StaticAssertDecl>(SecondDecl);
 
     const Expr *FirstExpr = FirstSA->getAssertExpr();
     const Expr *SecondExpr = SecondSA->getAssertExpr();
case FunctionTemplate:
     llvm_unreachable("Invalid diff type");
   case ObjCMethod: {
     if (diagnoseSubMismatchObjCMethod(FirstProtocol, FirstModule, SecondModule,
                                       cast<ObjCMethodDecl>(FirstDecl),
                                       cast<ObjCMethodDecl>(SecondDecl)))
       return true;
     break;
   }
+  case ObjCProperty: {
+    if (diagnoseSubMismatchObjCProperty(FirstProtocol, FirstModule,
+                                        SecondModule,
+                                        cast<ObjCPropertyDecl>(FirstDecl),
+                                        cast<ObjCPropertyDecl>(SecondDecl)))
+      return true;
+    break;
+  }
   }
 
   Diag(FirstDecl->getLocation(),
        diag::err_module_odr_violation_mismatch_decl_unknown)
       << FirstProtocol << FirstModule.empty() << FirstModule << FirstDiffType
       << FirstDecl->getSourceRange();
   Diag(SecondDecl->getLocation(),
        diag::note_module_odr_violation_mismatch_decl_unknown)
       << SecondModule << FirstDiffType << SecondDecl->getSourceRange();
   return true;
 }

clang/lib/AST/ODRHash.cpp

void VisitFieldDecl(const FieldDecl *D) {
     }
 
     Hash.AddBoolean(D->isMutable());
     AddStmt(D->getInClassInitializer());
 
     Inherited::VisitFieldDecl(D);
   }
 
+  void VisitObjCPropertyDecl(const ObjCPropertyDecl *D) {
+    ID.AddInteger(D->getPropertyAttributes());
+    ID.AddInteger(D->getPropertyImplementation());
+    AddQualType(D->getType());
+    AddDecl(D);
+
+    Inherited::VisitObjCPropertyDecl(D);
+  }
+
   void VisitFunctionDecl(const FunctionDecl *D) {
     // Handled by the ODRHash for FunctionDecl
     ID.AddInteger(D->getODRHash());
 
     Inherited::VisitFunctionDecl(D);
   }
 
   void VisitCXXMethodDecl(const CXXMethodDecl *D) {
switch (D->getKind()) {
     case Decl::Field:
     case Decl::Friend:
     case Decl::FunctionTemplate:
     case Decl::StaticAssert:
     case Decl::TypeAlias:
     case Decl::Typedef:
     case Decl::Var:
     case Decl::ObjCMethod:
+    case Decl::ObjCProperty:
       return true;
   }
 }
 
 void ODRHash::AddSubDecl(const Decl *D) {
   assert(D && "Expecting non-null pointer.");
 
   ODRDeclVisitor(ID, *this).Visit(D);

clang/test/Modules/compare-objc-protocol.m

 // expected-error@first.h:* {{'CompareMethodClassInstance' has different definitions in different modules; first difference is definition in module 'First.Hidden' found instance method 'methodClassInstance'}}
 // expected-note@second.h:* {{but in 'Second' found method 'methodClassInstance' as class method}}
 
 id<CompareMethodRequirednessExplicit> compareMethodRequirednessExplicit;
 // expected-error@first.h:* {{'CompareMethodRequirednessExplicit' has different definitions in different modules; first difference is definition in module 'First.Hidden' found 'optional' method control}}
 // expected-note@second.h:* {{but in 'Second' found 'required' method control}}
 id<CompareMethodRequirednessDefault> compareMethodRequirednessDefault; // no error
 #endif
+
+#if defined(FIRST)
+@protocol CompareMatchingProperties
+@property int matchingPropName;
+@end
+
+@protocol ComparePropertyPresence1
+@property int propPresence1;
+@end
+@protocol ComparePropertyPresence2
+@end
+
+@protocol ComparePropertyName
+@property int propNameA;
+@end
+
+@protocol ComparePropertyType
+@property int propType;
+@end
+
+@protocol ComparePropertyOrder
+@property int propOrderX;
+@property int propOrderY;
+@end
+
+@protocol CompareMatchingPropertyAttributes
+@property (nonatomic, assign) int matchingProp;
+@end
+@protocol ComparePropertyAttributes
+@property (nonatomic) int propAttributes;
+@end
+// Edge cases.
+@protocol CompareFirstImplAttribute
+@property int firstImplAttribute;
+@end
+@protocol CompareLastImplAttribute
+// Cannot test with protocols 'direct' attribute because it's not allowed.
+@property (class) int lastImplAttribute;
+@end
+#elif defined(SECOND)
+@protocol CompareMatchingProperties
+@property int matchingPropName;
+@end
+
+@protocol ComparePropertyPresence1
+@end
+@protocol ComparePropertyPresence2
+@property int propPresence2;
+@end
+
+@protocol ComparePropertyName
+@property int propNameB;
+@end
+
+@protocol ComparePropertyType
+@property float propType;
+@end
+
+@protocol ComparePropertyOrder
+@property int propOrderY;
+@property int propOrderX;
+@end
+
+@protocol CompareMatchingPropertyAttributes
+@property (assign, nonatomic) int matchingProp;
+@end
+@protocol ComparePropertyAttributes
+@property (atomic) int propAttributes;
+@end
+// Edge cases.
+@protocol CompareFirstImplAttribute
+@property (readonly) int firstImplAttribute;
+@end
+@protocol CompareLastImplAttribute
+@property int lastImplAttribute;
+@end
+#else
+id<CompareMatchingProperties> compareMatchingProperties;
+id<ComparePropertyPresence1> comparePropertyPresence1;
+// expected-error@first.h:* {{'ComparePropertyPresence1' has different definitions in different modules; first difference is definition in module 'First.Hidden' found property}}
+// expected-note@second.h:* {{but in 'Second' found end of class}}
+id<ComparePropertyPresence2> comparePropertyPresence2;
+// expected-error@first.h:* {{'ComparePropertyPresence2' has different definitions in different modules; first difference is definition in module 'First.Hidden' found end of class}}
+// expected-note@second.h:* {{but in 'Second' found property}}
+id<ComparePropertyName> comparePropertyName;
+// expected-error@first.h:* {{'ComparePropertyName' has different definitions in different modules; first difference is definition in module 'First.Hidden' found property 'propNameA'}}
+// expected-note@second.h:* {{but in 'Second' found property 'propNameB'}}
+id<ComparePropertyType> comparePropertyType;
+// expected-error@first.h:* {{'ComparePropertyType' has different definitions in different modules; first difference is definition in module 'First.Hidden' found property 'propType' with type 'int'}}
+// expected-note@second.h:* {{but in 'Second' found property 'propType' with type 'float'}}
+id<ComparePropertyOrder> comparePropertyOrder;
+// expected-error@first.h:* {{'ComparePropertyOrder' has different definitions in different modules; first difference is definition in module 'First.Hidden' found property 'propOrderX'}}
+// expected-note@second.h:* {{but in 'Second' found property 'propOrderY'}}
+
+id<CompareMatchingPropertyAttributes> compareMatchingPropertyAttributes;
+id<ComparePropertyAttributes> comparePropertyAttributes;
+// expected-error@first.h:* {{'ComparePropertyAttributes' has different definitions in different modules; first difference is definition in module 'First.Hidden' found property 'propAttributes' with 'nonatomic' attribute}}
+// expected-note@second.h:* {{but in 'Second' found property 'propAttributes' with different 'nonatomic' attribute}}
+id<CompareFirstImplAttribute> compareFirstImplAttribute;
+// expected-error@first.h:* {{'CompareFirstImplAttribute' has different definitions in different modules; first difference is definition in module 'First.Hidden' found property 'firstImplAttribute' with default 'readonly' attribute}}
+// expected-note@second.h:* {{but in 'Second' found property 'firstImplAttribute' with different 'readonly' attribute}}
+id<CompareLastImplAttribute> compareLastImplAttribute;
+// expected-error@first.h:* {{'CompareLastImplAttribute' has different definitions in different modules; first difference is definition in module 'First.Hidden' found property 'lastImplAttribute' with 'class' attribute}}
+// expected-note@second.h:* {{but in 'Second' found property 'lastImplAttribute' with different 'class' attribute}}
+#endif