[modules] Fix tracking ObjCInterfaceType decl when there are multiple definitions.

With the old approach we were updating `ObjCInterfaceType.Decl` to the
last encountered definition. But during loading modules
`ASTDeclReader::VisitObjCInterfaceDecl` keeps the *first* encountered
definition. So with multiple definitions imported there would be a
disagreement between expected definition in `ObjCInterfaceType.Decl` and
actual definition `ObjCInterfaceDecl::getDefinition` which can lead to
incorrect diagnostic.

Fix by not tracking definition in `ObjCInterfaceType` explicitly but by
getting it from redeclaration chain.

Partially reverted 919fc50 keeping the
modified test case as the correct behavior is achieved in a different
way.

Differential Revision: https://reviews.llvm.org/D110452

Author: vsapsai

clang/include/clang/AST/Type.h

@@ -6016,18 +6016,17 @@ inline ObjCProtocolDecl **ObjCTypeParamType::getProtocolStorageImpl() {
 class ObjCInterfaceType : public ObjCObjectType {
   friend class ASTContext; // ASTContext creates these.
   friend class ASTReader;
-  friend class ObjCInterfaceDecl;
   template <class T> friend class serialization::AbstractTypeReader;
 
-  mutable ObjCInterfaceDecl *Decl;
+  ObjCInterfaceDecl *Decl;
 
   ObjCInterfaceType(const ObjCInterfaceDecl *D)
       : ObjCObjectType(Nonce_ObjCInterface),
         Decl(const_cast<ObjCInterfaceDecl*>(D)) {}
 
 public:
   /// Get the declaration of this interface.
-  ObjCInterfaceDecl *getDecl() const { return Decl; }
+  ObjCInterfaceDecl *getDecl() const;
 
   bool isSugared() const { return false; }
   QualType desugar() const { return QualType(this, 0); }

clang/lib/AST/DeclObjC.cpp

@@ -603,10 +603,6 @@ void ObjCInterfaceDecl::allocateDefinitionData() {
   assert(!hasDefinition() && "ObjC class already has a definition");
   Data.setPointer(new (getASTContext()) DefinitionData());
   Data.getPointer()->Definition = this;
-
-  // Make the type point at the definition, now that we have one.
-  if (TypeForDecl)
-    cast<ObjCInterfaceType>(TypeForDecl)->Decl = this;
 }
 
 void ObjCInterfaceDecl::startDefinition() {

clang/lib/AST/Type.cpp

@@ -821,6 +821,13 @@ QualType ObjCObjectType::stripObjCKindOfTypeAndQuals(
                                /*isKindOf=*/false);
 }
 
+ObjCInterfaceDecl *ObjCInterfaceType::getDecl() const {
+  ObjCInterfaceDecl *Canon = Decl->getCanonicalDecl();
+  if (ObjCInterfaceDecl *Def = Canon->getDefinition())
+    return Def;
+  return Canon;
+}
+
 const ObjCObjectPointerType *ObjCObjectPointerType::stripObjCKindOfTypeAndQuals(
                                const ASTContext &ctx) const {
   if (!isKindOfType() && qual_empty())

clang/lib/Sema/SemaLookup.cpp

@@ -5323,11 +5323,8 @@ static NamedDecl *getDefinitionToImport(NamedDecl *D) {
     return FD->getDefinition();
   if (TagDecl *TD = dyn_cast<TagDecl>(D))
     return TD->getDefinition();
-  // The first definition for this ObjCInterfaceDecl might be in the TU
-  // and not associated with any module. Use the one we know to be complete
-  // and have just seen in a module.
   if (ObjCInterfaceDecl *ID = dyn_cast<ObjCInterfaceDecl>(D))
-    return ID;
+    return ID->getDefinition();
   if (ObjCProtocolDecl *PD = dyn_cast<ObjCProtocolDecl>(D))
     return PD->getDefinition();
   if (TemplateDecl *TD = dyn_cast<TemplateDecl>(D))

clang/test/Modules/decldef.mm

@@ -1,10 +1,12 @@
 // RUN: rm -rf %t
-// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fobjc-arc -I %S/Inputs -fmodules-cache-path=%t %s -verify -DUSE_1 -DUSE_2 -DUSE_3 -DUSE_4
-// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fobjc-arc -I %S/Inputs -fmodules-cache-path=%t %s -verify -DUSE_2 -DUSE_3 -DUSE_4
-// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fobjc-arc -I %S/Inputs -fmodules-cache-path=%t %s -verify -DUSE_3 -DUSE_4
-// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fobjc-arc -I %S/Inputs -fmodules-cache-path=%t %s -verify -DUSE_4
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fobjc-arc -I %S/Inputs -fmodules-cache-path=%t %s -verify -DUSE_1 -DUSE_2 -DUSE_3 -DUSE_4 -DUSE_5
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fobjc-arc -I %S/Inputs -fmodules-cache-path=%t %s -verify -DUSE_2 -DUSE_3 -DUSE_4 -DUSE_5
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fobjc-arc -I %S/Inputs -fmodules-cache-path=%t %s -verify -DUSE_3 -DUSE_4 -DUSE_5
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fobjc-arc -I %S/Inputs -fmodules-cache-path=%t %s -verify -DUSE_4 -DUSE_5
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fobjc-arc -I %S/Inputs -fmodules-cache-path=%t %s -verify -DUSE_5
 
 // expected-note@Inputs/def.h:5 0-1{{here}}
+// expected-note@Inputs/def.h:11 0-1{{here}}
 // expected-note@Inputs/def.h:16 0-1{{here}}
 // expected-note@Inputs/def-include.h:11 0-1{{here}}
 
@@ -51,11 +53,17 @@ void testB() {
 }
 
 void testDef() {
+#ifdef USE_4
   [def defMethod];
+  #ifndef USED
+  // expected-error@-2{{definition of 'Def' must be imported from module 'decldef.Def' before it is required}}
+  #define USED
+  #endif
+#endif
 }
 
 void testDef2() {
-#ifdef USE_4
+#ifdef USE_5
   def2->func();
   def3->func();
   #ifndef USED

clang/test/Modules/interface-diagnose-missing-import.m

@@ -1,11 +1,11 @@
 // RUN: rm -rf %t
 // RUN: %clang_cc1 %s -fsyntax-only -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -F%S/Inputs/interface-diagnose-missing-import -verify
+// expected-no-diagnostics
 @interface Buggy
 @end
 
 @import Foo.Bar;
 
-@interface Buggy (MyExt) // expected-error {{definition of 'Buggy' must be imported from module 'Foo' before it is required}}
+// No diagnostic for inaccessible 'Buggy' definition because we have another definition right in this file.
+@interface Buggy (MyExt)
 @end
-
-// expected-note@Foo/RandoPriv.h:3{{definition here is not reachable}}