[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

16

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

16

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/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/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/PrettyDeclStackTrace.h"
 #include "clang/Basic/CharInfo.h"
+#include "clang/Basic/DiagnosticSema.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) {
+    if (Actions.ActOnDuplicateDefinition(SkipBody.Previous, SkipBody)) {
+      ClsType->mergeDuplicateDefinitionWithCommon(
+          cast<ObjCInterfaceDecl>(SkipBody.Previous)->getDefinition());
+    } else {
+      Diag(AtLoc, diag::err_duplicate_class_def) << ClsType->getDeclName();
+      Diag(SkipBody.Previous->getLocation(), diag::note_previous_definition);
+      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/hidden-duplicates.m

+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: %clang_cc1 -emit-llvm -o %t/test.bc -I %t/include %t/test.m -verify \
+// RUN:            -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/modules.cache
+// RUN: %clang_cc1 -emit-llvm -o %t/test.bc -I %t/include %t/test.m -verify -DTEST_MAKE_HIDDEN_VISIBLE=1 \
+// RUN:            -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/modules.cache
+// RUN: %clang_cc1 -emit-llvm -o %t/test.bc -I %t/include %t/test.m -verify -x objective-c++ \
+// RUN:            -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/modules.cache
+// RUN: %clang_cc1 -emit-llvm -o %t/test.bc -I %t/include %t/test.m -verify -DTEST_MAKE_HIDDEN_VISIBLE=1 -x objective-c++ \
+// RUN:            -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/modules.cache
+
+// Test parsing duplicate Objective-C entities when a previous entity is defined
+// in a hidden [sub]module and cannot be used.
+//
+// Testing with header guards and includes on purpose as tracking imports in
+// modules is a separate issue.
+
+//--- include/textual.h
+#ifndef TEXTUAL_H
+#define TEXTUAL_H
+
+__attribute__((objc_root_class))
+@interface NSObject {
+@public
+  int ivarName;
+}
+@property (assign, nonatomic) int propName;
+@end
+
+@class ForwardDeclaredClassWithoutDefinition;
+
+#endif
+
+//--- include/empty.h
+//--- include/initially_hidden.h
+#include "textual.h"
+
+//--- include/module.modulemap
+module Piecewise {
+  module Empty {
+    header "empty.h"
+  }
+  module InitiallyHidden {
+    header "initially_hidden.h"
+    export *
+  }
+}
+
+//--- test.m
+// Including empty.h loads the entire module Piecewise but keeps InitiallyHidden hidden.
+#include "empty.h"
+#include "textual.h"
+#ifdef TEST_MAKE_HIDDEN_VISIBLE
+#include "initially_hidden.h"
+#endif
+// expected-no-diagnostics
+
+void testAccessingInterface(NSObject *obj) {
+  obj->ivarName = obj.propName;
+}
+
+void testForwardDecl(ForwardDeclaredClassWithoutDefinition *obj);