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

[vsapsai]

Summary

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 919fc50034b44c48aae8b80283f253ec2ee17f45 keeping the
modified test case as the correct behavior is achieved in a different
way.

[vsapsai]

This is a clean-up before https://reviews.llvm.org/D110453

clang/lib/AST/DeclObjC.cpp

608

Tracking a type per decl is complementary to tracking decl per type and it looks potentially bug-prone. But I haven't noticed any problems with it and haven't tried to remove TypeForDecl. If anybody has any extra information on it, we can schedule subsequent fixes.

[vsapsai]

Ping. Failing ORC tests seem to be unrelated, seen the same failed tests in other code reviews.

[vsapsai]

Rebase and trigger a new round of pre-merge checks.

[vsapsai]

Another rebase and pre-merge checks.

clang/include/clang/AST/Type.h

 ///   - There are no protocol qualifiers.  To reinforce this, code which
 ///     tries to invoke the protocol methods via an ObjCInterfaceType will
 ///     fail to compile.
 ///   - It is its own base type.  That is, if T is an ObjCInterfaceType*,
 ///     T->getBaseType() == QualType(T, 0).
 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); }
 
   static bool classof(const Type *T) {
     return T->getTypeClass() == ObjCInterface;
   }
 

clang/lib/AST/DeclObjC.cpp

bool ObjCInterfaceDecl::isDesignatedInitializer(Selector Sel,
   }
   return false;
 }
 
 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() {
   allocateDefinitionData();
 
   // Update all of the declarations with a pointer to the definition.
   for (auto *RD : redecls()) {
     if (RD != this)

clang/lib/AST/Type.cpp

QualType ObjCObjectType::stripObjCKindOfTypeAndQuals(
 
   return ctx.getObjCObjectType(ctx.getQualifiedType(baseType,
                                                     splitBaseType.Quals),
                                getTypeArgsAsWritten(),
                                /*protocols=*/{},
                                /*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())
     return this;
 
   QualType obj = getObjectType()->stripObjCKindOfTypeAndQuals(ctx);
   return ctx.getObjCObjectPointerType(obj)->castAs<ObjCObjectPointerType>();
 }

clang/lib/Sema/SemaLookup.cpp

 /// the given declaration.
 static NamedDecl *getDefinitionToImport(NamedDecl *D) {
   if (VarDecl *VD = dyn_cast<VarDecl>(D))
     return VD->getDefinition();
   if (FunctionDecl *FD = dyn_cast<FunctionDecl>(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))
     if (NamedDecl *TTD = TD->getTemplatedDecl())
       return getDefinitionToImport(TTD);
   return nullptr;
 }
 

clang/test/Modules/decldef.mm

 // 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}}
 
 @class Def;
 Def *def;
 class Def2; // expected-note 0-1{{forward decl}}
 Def2 *def2;
 namespace Def3NS { class Def3; } // expected-note 0-1{{forward decl}}
#ifdef USE_3
   #ifndef USED
   // expected-error@-2{{definition of 'B' must be imported from module 'decldef.Def' before it is required}}
   #define USED
   #endif
 #endif
 }
 
 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
   // expected-error@-3 {{definition of 'Def2' must be imported}}
   #define USED
   #endif
 #endif
 }

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

 // 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}}