[modules] Allow parsing a duplicate Obj-C interface if a previous one comes from a hidden [sub]module.

[vsapsai]

Summary

Instead of emitting a redefinition error, check that definitions are
equivalent and allow such scenario.

A few non-obvious implementation details:

• to avoid multiple definitions in the redeclaration chain we just drop the new definition after checking for equivalence;
• for checking definition equivalence use ODR hash instead of ASTStructuralEquivalence because it avoids excessive recursive deserialization. Though after detecting a mismatch we do deserialize multiple entities to provide a better error message.

rdar://82908223

[vsapsai]

I also have a very similar change with ActOnDuplicateDefinition for ObjCProtocolDecl. If anybody is interested in how it compares with ObjCInterfaceDecl, I can publish that but it's not finished yet.

clang/lib/Parse/ParseObjc.cpp

17

I'd like to get feedback on Parser/Sema layering. I'll check myself what can be done but at the first glance accessing pieces of Sema from Parser looks questionable.

[jansvoboda11]

This looks good IMO. Let's see if others have more suggestions besides the Parser/Sema layering.

clang/lib/Parse/ParseObjc.cpp

17

Yeah, I think introducing new Sema::ActOnEndClassInterface() and moving the diagnostic logic there might be cleaner.

[vsapsai]

Implement detecting and diagnosing duplicates during parsing based on ODR hash done during deserialization.

[bruno]

Nice new testcase snippets, LGTM

[vsapsai]

Thanks for the review, Bruno!

clang/include/clang/AST/DeclObjC.h

public:
   const ObjCInterfaceDecl *getDefinition() const {
     return hasDefinition()? Data.getPointer()->Definition : nullptr;
   }
 
   /// Starts the definition of this Objective-C class, taking it from
   /// a forward declaration (\@class) to a definition (\@interface).
   void startDefinition();
 
+  /// Starts the definition without sharing it with other redeclarations.
+  /// Such definition shouldn't be used for anything but only to compare if
+  /// a duplicate is compatible with previous definition or if it is
+  /// a distinct duplicate.
+  void startDuplicateDefinitionForComparison();
+  void mergeDuplicateDefinitionWithCommon(const ObjCInterfaceDecl *Definition);
+
   /// Retrieve the superclass type.
   const ObjCObjectType *getSuperClassType() const {
     if (TypeSourceInfo *TInfo = getSuperClassTInfo())
       return TInfo->getType()->castAs<ObjCObjectType>();
 
     return nullptr;
   }
 

clang/include/clang/AST/ODRDiagsEmitter.h

public:
 
   /// Diagnose ODR mismatch between 2 ObjCInterfaceDecl.
   ///
   /// Returns true if found a mismatch and diagnosed it.
   bool diagnoseMismatch(
       const ObjCInterfaceDecl *FirstID, const ObjCInterfaceDecl *SecondID,
       const struct ObjCInterfaceDecl::DefinitionData *SecondDD) const;
 
+  /// Diagnose ODR mismatch between ObjCInterfaceDecl with different
+  /// definitions.
+  bool diagnoseMismatch(const ObjCInterfaceDecl *FirstID,
+                        const ObjCInterfaceDecl *SecondID) const {
+    assert(FirstID->data().Definition != SecondID->data().Definition &&
+           "Don't diagnose differences when definitions are merged already");
+    return diagnoseMismatch(FirstID, SecondID, &SecondID->data());
+  }
+
   /// Diagnose ODR mismatch between 2 ObjCProtocolDecl.
   ///
   /// Returns true if found a mismatch and diagnosed it.
   /// To compare 2 declarations with merged and identical definition data
   /// you need to provide pre-merge definition data in \p SecondDD.
   bool diagnoseMismatch(
       const ObjCProtocolDecl *FirstProtocol,
       const ObjCProtocolDecl *SecondProtocol,

clang/include/clang/Basic/DiagnosticASTKinds.td

def err_module_odr_violation_objc_interface : Error <
   "%0 has different definitions in different modules; first difference is "
   "%select{definition in module '%2'|defined here}1 found "
   "%select{"
   "%select{no super class|super class with type %5}4|"
   "instance variable '%4' access control is "
     "%select{|@private|@protected|@public|@package}5"
   "}3">;
 def note_module_odr_violation_objc_interface : Note <
-  "but in '%0' found "
+  "but in %select{'%1'|definition here}0 found "
   "%select{"
-  "%select{no super class|super class with type %3}2|"
-  "instance variable '%2' access control is "
-    "%select{|@private|@protected|@public|@package}3"
-  "}1">;
+  "%select{no super class|super class with type %4}3|"
+  "instance variable '%3' access control is "
+    "%select{|@private|@protected|@public|@package}4"
+  "}2">;
 
 def err_module_odr_violation_template_parameter : Error <
   "%q0 has different definitions in different modules; first difference is "
   "%select{definition in module '%2'|defined here}1 found "
   "%select{"
   "unnamed template parameter|"
   "template parameter %5|"
   "template parameter with %select{no |}4default argument|"
def err_module_odr_violation_field : Error<
   "field %4|"
   "field %4 with type %5|"
   "%select{non-|}5bitfield %4|"
   "bitfield %4 with one width expression|"
   "%select{non-|}5mutable field %4|"
   "field %4 with %select{no|an}5 initalizer|"
   "field %4 with an initializer"
   "}3">;
-def note_module_odr_violation_field : Note<"but in '%0' found "
+def note_module_odr_violation_field : Note<
+  "but in %select{'%1'|definition here}0 found "
   "%select{"
-  "field %2|"
-  "field %2 with type %3|"
-  "%select{non-|}3bitfield %2|"
-  "bitfield %2 with different width expression|"
-  "%select{non-|}3mutable field %2|"
-  "field %2 with %select{no|an}3 initializer|"
-  "field %2 with a different initializer"
-  "}1">;
+  "field %3|"
+  "field %3 with type %4|"
+  "%select{non-|}4bitfield %3|"
+  "bitfield %3 with different width expression|"
+  "%select{non-|}4mutable field %3|"
+  "field %3 with %select{no|an}4 initializer|"
+  "field %3 with a different initializer"
+  "}2">;
 
 def err_module_odr_violation_typedef : Error<
   "%q0 has different definitions in different modules; first difference is "
   "%select{definition in module '%2'|defined here}1 found "
   "%select{"
   "%select{typedef|type alias}4 name %5|"
   "%select{typedef|type alias}4 %5 with underlying type %6"
   "}3">;

clang/include/clang/Sema/Sema.h

 
   ObjCInterfaceDecl *ActOnStartClassInterface(
       Scope *S, SourceLocation AtInterfaceLoc, IdentifierInfo *ClassName,
       SourceLocation ClassLoc, ObjCTypeParamList *typeParamList,
       IdentifierInfo *SuperName, SourceLocation SuperLoc,
       ArrayRef<ParsedType> SuperTypeArgs, SourceRange SuperTypeArgsRange,
       Decl *const *ProtoRefs, unsigned NumProtoRefs,
       const SourceLocation *ProtoLocs, SourceLocation EndProtoLoc,
-      const ParsedAttributesView &AttrList);
+      const ParsedAttributesView &AttrList, SkipBodyInfo *SkipBody);
 
   void ActOnSuperClassOfClassInterface(Scope *S,
                                        SourceLocation AtInterfaceLoc,
                                        ObjCInterfaceDecl *IDecl,
                                        IdentifierInfo *ClassName,
                                        SourceLocation ClassLoc,
                                        IdentifierInfo *SuperName,
                                        SourceLocation SuperLoc,

clang/lib/AST/DeclObjC.cpp

void ObjCInterfaceDecl::startDefinition() {
 
   // Update all of the declarations with a pointer to the definition.
   for (auto *RD : redecls()) {
     if (RD != this)
       RD->Data = Data;
   }
 }
 
+void ObjCInterfaceDecl::startDuplicateDefinitionForComparison() {
+  Data.setPointer(nullptr);
+  allocateDefinitionData();
+  // Don't propagate data to other redeclarations.
+}
+
+void ObjCInterfaceDecl::mergeDuplicateDefinitionWithCommon(
+    const ObjCInterfaceDecl *Definition) {
+  Data = Definition->Data;
+}
+
 ObjCIvarDecl *ObjCInterfaceDecl::lookupInstanceVariable(IdentifierInfo *ID,
                                               ObjCInterfaceDecl *&clsDeclared) {
   // FIXME: Should make sure no callers ever do this.
   if (!hasDefinition())
     return nullptr;
 
   if (data().ExternallyCompleted)
     LoadExternalDefinition();

clang/lib/AST/ODRDiagsEmitter.cpp

auto DiagError = [FirstRecord, FirstField, FirstModule,
     return Diag(FirstField->getLocation(), diag::err_module_odr_violation_field)
            << FirstRecord << FirstModule.empty() << FirstModule
            << FirstField->getSourceRange() << DiffType;
   };
   auto DiagNote = [SecondField, SecondModule,
                    this](ODRFieldDifference DiffType) {
     return Diag(SecondField->getLocation(),
                 diag::note_module_odr_violation_field)
-           << SecondModule << SecondField->getSourceRange() << DiffType;
+           << SecondModule.empty() << SecondModule << SecondField->getSourceRange() << DiffType;
   };
 
   IdentifierInfo *FirstII = FirstField->getIdentifier();
   IdentifierInfo *SecondII = SecondField->getIdentifier();
   if (FirstII->getName() != SecondII->getName()) {
     DiagError(FieldName) << FirstII;
     DiagNote(FieldName) << SecondII;
     return true;
   }
 
-  assert(Context.hasSameType(FirstField->getType(), SecondField->getType()));
-  (void)Context;
-
   QualType FirstType = FirstField->getType();
   QualType SecondType = SecondField->getType();
   if (computeODRHash(FirstType) != computeODRHash(SecondType)) {
     DiagError(FieldTypeName) << FirstII << FirstType;
     DiagNote(FieldTypeName) << SecondII << SecondType;
     return true;
   }
 
+  assert(Context.hasSameType(FirstField->getType(), SecondField->getType()));
+  (void)Context;
+
   const bool IsFirstBitField = FirstField->isBitField();
   const bool IsSecondBitField = SecondField->isBitField();
   if (IsFirstBitField != IsSecondBitField) {
     DiagError(FieldSingleBitField) << FirstII << IsFirstBitField;
     DiagNote(FieldSingleBitField) << SecondII << IsSecondBitField;
     return true;
   }
 
auto DiagError = [FirstID, &FirstModule,
                           ODRInterfaceDifference DiffType) {
     return Diag(Loc, diag::err_module_odr_violation_objc_interface)
            << FirstID << FirstModule.empty() << FirstModule << Range
            << DiffType;
   };
   auto DiagNote = [&SecondModule, this](SourceLocation Loc, SourceRange Range,
                                         ODRInterfaceDifference DiffType) {
     return Diag(Loc, diag::note_module_odr_violation_objc_interface)
-           << SecondModule << Range << DiffType;
+           << SecondModule.empty() << SecondModule << Range << DiffType;
   };
 
   const struct ObjCInterfaceDecl::DefinitionData *FirstDD = &FirstID->data();
   assert(FirstDD && SecondDD && "Definitions without DefinitionData");
   if (FirstDD != SecondDD) {
     // Check for matching super class.
     auto GetSuperClassSourceRange = [](const TypeSourceInfo *SuperInfo,
                                        const ObjCInterfaceDecl *ID) {
for (auto *D : ID->decls()) {
       if (!ODRHash::isSubDeclToBeProcessed(D, DC))
         continue;
       Hashes.emplace_back(D, computeODRHash(D));
     }
   };
 
   DeclHashes FirstHashes;
   DeclHashes SecondHashes;
-  PopulateHashes(FirstHashes, FirstID, FirstID);
-  PopulateHashes(SecondHashes, SecondID, FirstID);
+  // Use definition as DeclContext because definitions are merged when
+  // DeclContexts are merged and separate when DeclContexts are separate.
+  PopulateHashes(FirstHashes, FirstID, FirstID->getDefinition());
+  PopulateHashes(SecondHashes, SecondID, SecondID->getDefinition());
 
   DiffResult DR = FindTypeDiffs(FirstHashes, SecondHashes);
   ODRMismatchDecl FirstDiffType = DR.FirstDiffType;
   ODRMismatchDecl SecondDiffType = DR.SecondDiffType;
   const Decl *FirstDecl = DR.FirstDecl;
   const Decl *SecondDecl = DR.SecondDecl;
 
   if (FirstDiffType == Other || SecondDiffType == Other) {

clang/lib/Parse/ParseObjc.cpp

 //===--- ParseObjC.cpp - Objective C Parsing ------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 //  This file implements the Objective-C portions of the Parser interface.
 //
 //===----------------------------------------------------------------------===//
 
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/ODRDiagsEmitter.h"
 #include "clang/AST/PrettyDeclStackTrace.h"
 #include "clang/Basic/CharInfo.h"
 #include "clang/Basic/TargetInfo.h"
 #include "clang/Parse/ParseDiagnostic.h"
 #include "clang/Parse/Parser.h"
 #include "clang/Parse/RAIIObjectsForParser.h"
 #include "clang/Sema/DeclSpec.h"
 #include "clang/Sema/Scope.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringExtras.h"
 
if (!ProtocolIdents.empty()) {
                                          /*consumeLastToken=*/true)) {
     return nullptr;
   }
 
   if (Tok.isNot(tok::less))
     Actions.ActOnTypedefedProtocols(protocols, protocolLocs,
                                     superClassId, superClassLoc);
 
+  Sema::SkipBodyInfo SkipBody;
   ObjCInterfaceDecl *ClsType = Actions.ActOnStartClassInterface(
       getCurScope(), AtLoc, nameId, nameLoc, typeParameterList, superClassId,
       superClassLoc, typeArgs,
       SourceRange(typeArgsLAngleLoc, typeArgsRAngleLoc), protocols.data(),
-      protocols.size(), protocolLocs.data(), EndProtoLoc, attrs);
+      protocols.size(), protocolLocs.data(), EndProtoLoc, attrs, &SkipBody);
 
   if (Tok.is(tok::l_brace))
     ParseObjCClassInstanceVariables(ClsType, tok::objc_protected, AtLoc);
 
   ParseObjCInterfaceDeclList(tok::objc_interface, ClsType);
 
+  if (SkipBody.CheckSameAsPrevious) {
+    auto *PreviousDef = cast<ObjCInterfaceDecl>(SkipBody.Previous);
+    if (Actions.ActOnDuplicateODRHashDefinition(ClsType, PreviousDef)) {
+      ClsType->mergeDuplicateDefinitionWithCommon(PreviousDef->getDefinition());
+    } else {
+      ODRDiagsEmitter DiagsEmitter(Diags, Actions.getASTContext(),
+                                   getPreprocessor().getLangOpts());
+      DiagsEmitter.diagnoseMismatch(PreviousDef, ClsType);
+      ClsType->setInvalidDecl();
+    }
+  }
+
   return ClsType;
 }
 
 /// Add an attribute for a context-sensitive type nullability to the given
 /// declarator.
 static void addContextSensitiveTypeNullability(Parser &P,
                                                Declarator &D,
                                                NullabilityKind nullability,

clang/lib/Sema/SemaDeclObjC.cpp

 
 ObjCInterfaceDecl *Sema::ActOnStartClassInterface(
     Scope *S, SourceLocation AtInterfaceLoc, IdentifierInfo *ClassName,
     SourceLocation ClassLoc, ObjCTypeParamList *typeParamList,
     IdentifierInfo *SuperName, SourceLocation SuperLoc,
     ArrayRef<ParsedType> SuperTypeArgs, SourceRange SuperTypeArgsRange,
     Decl *const *ProtoRefs, unsigned NumProtoRefs,
     const SourceLocation *ProtoLocs, SourceLocation EndProtoLoc,
-    const ParsedAttributesView &AttrList) {
+    const ParsedAttributesView &AttrList, SkipBodyInfo *SkipBody) {
   assert(ClassName && "Missing class identifier");
 
   // Check for another declaration kind with the same name.
   NamedDecl *PrevDecl =
       LookupSingleName(TUScope, ClassName, ClassLoc, LookupOrdinaryName,
                        forRedeclarationInCurContext());
 
   if (PrevDecl && !isa<ObjCInterfaceDecl>(PrevDecl)) {
ObjCInterfaceDecl *Sema::ActOnStartClassInterface(
   }
 
   ObjCInterfaceDecl *IDecl
     = ObjCInterfaceDecl::Create(Context, CurContext, AtInterfaceLoc, ClassName,
                                 typeParamList, PrevIDecl, ClassLoc);
   if (PrevIDecl) {
     // Class already seen. Was it a definition?
     if (ObjCInterfaceDecl *Def = PrevIDecl->getDefinition()) {
+      if (SkipBody && !hasVisibleDefinition(Def)) {
+        SkipBody->CheckSameAsPrevious = true;
+        SkipBody->New = IDecl;
+        SkipBody->Previous = Def;
+      } else {
         Diag(AtInterfaceLoc, diag::err_duplicate_class_def)
             << PrevIDecl->getDeclName();
         Diag(Def->getLocation(), diag::note_previous_definition);
         IDecl->setInvalidDecl();
       }
     }
+  }
 
   ProcessDeclAttributeList(TUScope, IDecl, AttrList);
   AddPragmaAttributes(TUScope, IDecl);
 
   // Merge attributes from previous declarations.
   if (PrevIDecl)
     mergeDeclAttributes(IDecl, PrevIDecl);
 
   PushOnScopeChains(IDecl, TUScope);
 
   // Start the definition of this class. If we're in a redefinition case, there
   // may already be a definition, so we'll end up adding to it.
-  if (!IDecl->hasDefinition())
+  if (SkipBody && SkipBody->CheckSameAsPrevious)
+    IDecl->startDuplicateDefinitionForComparison();
+  else if (!IDecl->hasDefinition())
     IDecl->startDefinition();
 
   if (SuperName) {
     // Diagnose availability in the context of the @interface.
     ContextRAII SavedContext(*this, IDecl);
 
     ActOnSuperClassOfClassInterface(S, AtInterfaceLoc, IDecl,
                                     ClassName, ClassLoc,

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

 // Test that each header can compile
 // RUN: %clang_cc1 -fsyntax-only -x objective-c %t/include/first.h -fblocks -fobjc-arc
 // RUN: %clang_cc1 -fsyntax-only -x objective-c %t/include/second.h -fblocks -fobjc-arc
 
 // Run test
 // RUN: %clang_cc1 -I%t/include -verify %t/test.m -fblocks -fobjc-arc \
 // RUN:            -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/modules.cache
 
+// Run the same test with second.h being modular
+// RUN: cat %t/include/second.modulemap >> %t/include/module.modulemap
+// RUN: %clang_cc1 -I%t/include -verify %t/test.m -fblocks -fobjc-arc -DTEST_MODULAR=1 \
+// RUN:            -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/modules.cache
+
 // Test that we don't accept different class definitions with the same name
 // from multiple modules but detect mismatches and provide actionable
 // diagnostic.
 
 //--- include/common.h
 #ifndef COMMON_H
 #define COMMON_H
 @interface NSObject @end
 @protocol CommonProtocol @end
 @protocol ExtraProtocol @end
 #endif
 
 //--- include/first-empty.h
 //--- include/module.modulemap
 module First {
   module Empty {
     header "first-empty.h"
   }
   module Hidden {
     header "first.h"
     export *
   }
 }
+
+//--- include/second.modulemap
 module Second {
   header "second.h"
   export *
 }
 
 //--- test.m
 #if defined(FIRST) || defined(SECOND)
 # include "common.h"

 @interface CompareSuperclassPresence2: NSObject @end
 
 @interface DifferentSuperclass: NSObject @end
 @interface CompareDifferentSuperclass: DifferentSuperclass @end
 #else
 CompareMatchingSuperclass *compareMatchingSuperclass;
 CompareSuperclassPresence1 *compareSuperclassPresence1;
 // expected-error@first.h:* {{'CompareSuperclassPresence1' has different definitions in different modules; first difference is definition in module 'First.Hidden' found super class with type 'NSObject'}}
-// expected-note@second.h:* {{but in 'Second' found no super class}}
+// expected-note-re@second.h:* {{but in {{'Second'|definition here}} found no super class}}
 CompareSuperclassPresence2 *compareSuperclassPresence2;
 // expected-error@first.h:* {{'CompareSuperclassPresence2' has different definitions in different modules; first difference is definition in module 'First.Hidden' found no super class}}
-// expected-note@second.h:* {{but in 'Second' found super class with type 'NSObject'}}
+// expected-note-re@second.h:* {{but in {{'Second'|definition here}} found super class with type 'NSObject'}}
 CompareDifferentSuperclass *compareDifferentSuperclass;
 // expected-error@first.h:* {{'CompareDifferentSuperclass' has different definitions in different modules; first difference is definition in module 'First.Hidden' found super class with type 'NSObject'}}
-// expected-note@second.h:* {{but in 'Second' found super class with type 'DifferentSuperclass'}}
+// expected-note-re@second.h:* {{but in {{'Second'|definition here}} found super class with type 'DifferentSuperclass'}}
 #endif
 
 #if defined(FIRST)
 @interface CompareMatchingConformingProtocols: NSObject<CommonProtocol> @end
 @protocol ForwardProtocol;
 @interface CompareMatchingConformingForwardProtocols: NSObject<ForwardProtocol> @end
 
 @interface CompareProtocolPresence1: NSObject<CommonProtocol> @end

 @interface CompareDifferentProtocols: NSObject<ExtraProtocol> @end
 @interface CompareProtocolOrder: NSObject<ExtraProtocol, CommonProtocol> @end
 #else
 CompareMatchingConformingProtocols *compareMatchingConformingProtocols;
 CompareMatchingConformingForwardProtocols *compareMatchingConformingForwardProtocols;
 
 CompareProtocolPresence1 *compareProtocolPresence1;
 // expected-error@first.h:* {{'CompareProtocolPresence1' has different definitions in different modules; first difference is definition in module 'First.Hidden' found 1 referenced protocol}}
-// expected-note@second.h:* {{but in 'Second' found 0 referenced protocols}}
+// expected-note-re@second.h:* {{but in {{'Second'|definition here}} found 0 referenced protocols}}
 CompareProtocolPresence2 *compareProtocolPresence2;
 // expected-error@first.h:* {{'CompareProtocolPresence2' has different definitions in different modules; first difference is definition in module 'First.Hidden' found 0 referenced protocols}}
-// expected-note@second.h:* {{but in 'Second' found 1 referenced protocol}}
+// expected-note-re@second.h:* {{but in {{'Second'|definition here}} found 1 referenced protocol}}
 
 CompareDifferentProtocols *compareDifferentProtocols;
 // expected-error@first.h:* {{'CompareDifferentProtocols' has different definitions in different modules; first difference is definition in module 'First.Hidden' found 1st referenced protocol with name 'CommonProtocol'}}
-// expected-note@second.h:* {{but in 'Second' found 1st referenced protocol with different name 'ExtraProtocol'}}
+// expected-note-re@second.h:* {{but in {{'Second'|definition here}} found 1st referenced protocol with different name 'ExtraProtocol'}}
 CompareProtocolOrder *compareProtocolOrder;
 // expected-error@first.h:* {{'CompareProtocolOrder' has different definitions in different modules; first difference is definition in module 'First.Hidden' found 1st referenced protocol with name 'CommonProtocol'}}
-// expected-note@second.h:* {{but in 'Second' found 1st referenced protocol with different name 'ExtraProtocol'}}
+// expected-note-re@second.h:* {{but in {{'Second'|definition here}} found 1st referenced protocol with different name 'ExtraProtocol'}}
 #endif
 
 #if defined(FIRST)
 @interface CompareMatchingIVars: NSObject { int ivarName; } @end
 
 @interface CompareIVarPresence1: NSObject @end
 @interface CompareIVarPresence2: NSObject { int ivarPresence2; } @end
 

 @public
   int ivarVisibilityDefault;
 }
 @end
 #else
 CompareMatchingIVars *compareMatchingIVars;
 
 CompareIVarPresence1 *compareIVarPresence1;
+#ifdef TEST_MODULAR
 // expected-error@second.h:* {{'CompareIVarPresence1::ivarPresence1' from module 'Second' is not present in definition of 'CompareIVarPresence1' in module 'First.Hidden'}}
 // expected-note@first.h:* {{definition has no member 'ivarPresence1'}}
+#else
+// expected-error@first.h:* {{'CompareIVarPresence1' has different definitions in different modules; first difference is definition in module 'First.Hidden' found end of class}}
+// expected-note@second.h:* {{but in definition here found instance variable}}
+#endif
 CompareIVarPresence2 *compareIVarPresence2;
 // expected-error@first.h:* {{'CompareIVarPresence2' has different definitions in different modules; first difference is definition in module 'First.Hidden' found instance variable}}
-// expected-note@second.h:* {{but in 'Second' found end of class}}
+// expected-note-re@second.h:* {{but in {{'Second'|definition here}} found end of class}}
 
 CompareIVarName *compareIVarName;
+#ifdef TEST_MODULAR
 // expected-error@second.h:* {{'CompareIVarName::differentIvarName' from module 'Second' is not present in definition of 'CompareIVarName' in module 'First.Hidden'}}
 // expected-note@first.h:* {{definition has no member 'differentIvarName'}}
+#else
+// expected-error@first.h:* {{'CompareIVarName' has different definitions in different modules; first difference is definition in module 'First.Hidden' found field 'ivarName'}}
+// expected-note@second.h:* {{but in definition here found field 'differentIvarName'}}
+#endif
 CompareIVarType *compareIVarType;
+#ifdef TEST_MODULAR
 // expected-error@second.h:* {{'CompareIVarType::ivarType' from module 'Second' is not present in definition of 'CompareIVarType' in module 'First.Hidden'}}
 // expected-note@first.h:* {{declaration of 'ivarType' does not match}}
+#else
+// expected-error@first.h:* {{'CompareIVarType' has different definitions in different modules; first difference is definition in module 'First.Hidden' found field 'ivarType' with type 'int'}}
+// expected-note@second.h:* {{but in definition here found field 'ivarType' with type 'float'}}
+#endif
 CompareIVarOrder *compareIVarOrder;
 // expected-error@first.h:* {{'CompareIVarOrder' has different definitions in different modules; first difference is definition in module 'First.Hidden' found field 'ivarNameInt'}}
-// expected-note@second.h:* {{but in 'Second' found field 'ivarNameFloat'}}
+// expected-note-re@second.h:* {{but in {{'Second'|definition here}} found field 'ivarNameFloat'}}
 
 CompareIVarVisibilityExplicit *compareIVarVisibilityExplicit;
 // expected-error@first.h:* {{'CompareIVarVisibilityExplicit' has different definitions in different modules; first difference is definition in module 'First.Hidden' found instance variable 'ivarVisibility' access control is @public}}
-// expected-note@second.h:* {{but in 'Second' found instance variable 'ivarVisibility' access control is @private}}
+// expected-note-re@second.h:* {{but in {{'Second'|definition here}} found instance variable 'ivarVisibility' access control is @private}}
 CompareIVarVisibilityDefault *compareIVarVisibilityDefault;
 // expected-error@first.h:* {{'CompareIVarVisibilityDefault' has different definitions in different modules; first difference is definition in module 'First.Hidden' found instance variable 'ivarVisibilityDefault' access control is @protected}}
-// expected-note@second.h:* {{but in 'Second' found instance variable 'ivarVisibilityDefault' access control is @public}}
+// expected-note-re@second.h:* {{but in {{'Second'|definition here}} found instance variable 'ivarVisibilityDefault' access control is @public}}
 #endif
 
 #if defined(FIRST)
 @interface CompareMatchingMethods: NSObject
 - (float)matchingMethod:(int)arg;
 @end
 
 @interface CompareMethodPresence1: NSObject

 
 @interface CompareMethodClassInstance: NSObject
 - (void)methodClassInstance;
 @end
 #else
 CompareMatchingMethods *compareMatchingMethods;
 CompareMethodPresence1 *compareMethodPresence1;
 // expected-error@first.h:* {{'CompareMethodPresence1' has different definitions in different modules; first difference is definition in module 'First.Hidden' found method}}
-// expected-note@second.h:* {{but in 'Second' found end of class}}
+// expected-note-re@second.h:* {{but in {{'Second'|definition here}} found end of class}}
 CompareMethodPresence2 *compareMethodPresence2;
 // expected-error@first.h:* {{'CompareMethodPresence2' 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 method}}
+// expected-note-re@second.h:* {{but in {{'Second'|definition here}} found method}}
 CompareMethodName *compareMethodName;
 // expected-error@first.h:* {{'CompareMethodName' has different definitions in different modules; first difference is definition in module 'First.Hidden' found method 'methodNameA'}}
-// expected-note@second.h:* {{but in 'Second' found different method 'methodNameB'}}
+// expected-note-re@second.h:* {{but in {{'Second'|definition here}} found different method 'methodNameB'}}
 
 CompareMethodArgCount *compareMethodArgCount;
 // expected-error@first.h:* {{'CompareMethodArgCount' has different definitions in different modules; first difference is definition in module 'First.Hidden' found method 'methodArgCount::' that has 2 parameters}}
-// expected-note@second.h:* {{but in 'Second' found method 'methodArgCount:' that has 1 parameter}}
+// expected-note-re@second.h:* {{but in {{'Second'|definition here}} found method 'methodArgCount:' that has 1 parameter}}
 CompareMethodArgName *compareMethodArgName;
 // expected-error@first.h:* {{'CompareMethodArgName' has different definitions in different modules; first difference is definition in module 'First.Hidden' found method 'methodArgName:' with 1st parameter named 'argNameA'}}
-// expected-note@second.h:* {{but in 'Second' found method 'methodArgName:' with 1st parameter named 'argNameB'}}
+// expected-note-re@second.h:* {{but in {{'Second'|definition here}} found method 'methodArgName:' with 1st parameter named 'argNameB'}}
 CompareMethodArgType *compareMethodArgType;
 // expected-error@first.h:* {{'CompareMethodArgType' has different definitions in different modules; first difference is definition in module 'First.Hidden' found method 'methodArgType:' with 1st parameter of type 'int'}}
-// expected-note@second.h:* {{but in 'Second' found method 'methodArgType:' with 1st parameter of type 'float'}}
+// expected-note-re@second.h:* {{but in {{'Second'|definition here}} found method 'methodArgType:' with 1st parameter of type 'float'}}
 
 CompareMethodReturnType *compareMethodReturnType;
 // expected-error@first.h:* {{'CompareMethodReturnType' has different definitions in different modules; first difference is definition in module 'First.Hidden' found method 'methodReturnType' with return type 'int'}}
-// expected-note@second.h:* {{but in 'Second' found method 'methodReturnType' with different return type 'float'}}
+// expected-note-re@second.h:* {{but in {{'Second'|definition here}} found method 'methodReturnType' with different return type 'float'}}
 
 CompareMethodOrder *compareMethodOrder;
 // expected-error@first.h:* {{'CompareMethodOrder' has different definitions in different modules; first difference is definition in module 'First.Hidden' found method 'methodOrderFirst'}}
-// expected-note@second.h:* {{but in 'Second' found different method 'methodOrderSecond'}}
+// expected-note-re@second.h:* {{but in {{'Second'|definition here}} found different method 'methodOrderSecond'}}
 CompareMethodClassInstance *compareMethodClassInstance;
 // expected-error@first.h:* {{'CompareMethodClassInstance' has different definitions in different modules; first difference is definition in module 'First.Hidden' found class method 'methodClassInstance'}}
-// expected-note@second.h:* {{but in 'Second' found method 'methodClassInstance' as instance method}}
+// expected-note-re@second.h:* {{but in {{'Second'|definition here}} found method 'methodClassInstance' as instance method}}
 #endif
 
 #if defined(FIRST)
 @interface CompareMatchingProperties: NSObject
 @property int matchingPropName;
 @end
 
 @interface ComparePropertyPresence1: NSObject

 @end
 @interface CompareLastImplAttribute: NSObject
 @property int lastImplAttribute;
 @end
 #else
 CompareMatchingProperties *compareMatchingProperties;
 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}}
+// expected-note-re@second.h:* {{but in {{'Second'|definition here}} found end of class}}
 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}}
+// expected-note-re@second.h:* {{but in {{'Second'|definition here}} found property}}
 
 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'}}
+// expected-note-re@second.h:* {{but in {{'Second'|definition here}} found property 'propNameB'}}
 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'}}
+// expected-note-re@second.h:* {{but in {{'Second'|definition here}} found property 'propType' with type 'float'}}
 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'}}
+// expected-note-re@second.h:* {{but in {{'Second'|definition here}} found property 'propOrderY'}}
 
 CompareMatchingPropertyAttributes *compareMatchingPropertyAttributes;
 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 'readonly' attribute}}
-// expected-note@second.h:* {{but in 'Second' found property 'propAttributes' with different 'readonly' attribute}}
+// expected-note-re@second.h:* {{but in {{'Second'|definition here}} found property 'propAttributes' with different 'readonly' attribute}}
 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}}
+// expected-note-re@second.h:* {{but in {{'Second'|definition here}} found property 'firstImplAttribute' with different 'readonly' attribute}}
 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 'direct' attribute}}
-// expected-note@second.h:* {{but in 'Second' found property 'lastImplAttribute' with different 'direct' attribute}}
+// expected-note-re@second.h:* {{but in {{'Second'|definition here}} found property 'lastImplAttribute' with different 'direct' attribute}}
 #endif

clang/test/Modules/hidden-duplicates.m

 @end
 
 @protocol ForwardDeclaredProtocolWithoutDefinition;
 
 id<TestProtocol> protocolDefinition(id<TestProtocol> t);
 id<ForwardDeclaredProtocolWithoutDefinition> forwardDeclaredProtocol(
     id<ForwardDeclaredProtocolWithoutDefinition> t);
 
+@interface NSObject @end
+@class ForwardDeclaredInterfaceWithoutDefinition;
+
+NSObject *interfaceDefinition(NSObject *o);
+NSObject *forwardDeclaredInterface(NSObject *o);
+
 #endif
 
 //--- include/empty.h
 //--- include/initially_hidden.h
 #include "textual.h"
 
 //--- include/module.modulemap
 module Piecewise {