[modules] Merge ObjC interface ivars with anonymous types.

[vsapsai]

Summary

Without the fix ivars with anonymous types can trigger errors like

error: 'TestClass::structIvar' from module 'Target' is not present in definition of 'TestClass' provided earlier
[...]
note: declaration of 'structIvar' does not match

It happens because types of ivars from different modules are considered
to be different. And it is caused by not merging anonymous TagDecl
from different modules.

To fix that I've changed serialization::needsAnonymousDeclarationNumber
to handle anonymous TagDecl inside ObjCInterfaceDecl. But that's not
sufficient as C code inside ObjCInterfaceDecl doesn't use interface
decl as a decl context but switches to its parent (TranslationUnit in
most cases). I'm changing that to make ObjCContainerDecl the lexical
decl context but keeping the semantic decl context intact.

Test "check-dup-decls-inside-objc.m" doesn't reflect a change in
functionality but captures the existing behavior to prevent regressions.

rdar://85563013

[vsapsai]

@akyrtzi I believe you were working on the code responsible for decl contexts at some point. I'm not sure you'll be able to review the change. Can you recommend someone else knowledgeable with this code?

[vsapsai]

Rebase and address clang-format comment.

[vsapsai]

Ping.

[rjmccall]

This is one of those patches that's difficult to review because it's so hard to foresee the consequences of changing the concepts. That said, I think the basic idea here seems reasonable.

clang/test/Modules/merge-anon-record-definition-in-objc.m

6

Do you want to test this in Objective-C++ mode as well?

[vsapsai]

clang/test/Modules/merge-anon-record-definition-in-objc.m

6

That's a good idea! Let me add that.

[rsmith]

The modules side of this looks good to me, and logically changing the lexical decl context in an interface to be that interface makes a lot of sense, but I agree with @rjmccall that it's hard to predict what the consequences of that change might be. Can you also test this against some body of real-world code?

[vsapsai]

clang/test/Modules/merge-anon-record-definition-in-objc.m

6

It turned out to be a great idea! Looks like Objective-C++ mode has exposed an actual issue. After skipping a bunch of "cannot be defined in a parameter type" I'm hitting "error: use of undeclared identifier 'kX'". And in non-modular case

// clang -fsyntax-only test.mm
@interface NSObject
@end
@interface TestSubject: NSObject {
@public
    enum { kEnumConstant = 0, } enumIvar;
}
@end

int test() {
    return kEnumConstant;
}

does not trigger any errors. So I believe there should be no errors in my test case too.

[vsapsai]

Fix bug with anonymous enum constant lookup in C++.

Turns out the lookup was broken by using ObjCInterfaceDecl as a semantic decl
context for anonymous enums. Fixing that fixed the lookup.

Also added in tests checking scoped enums in C++. Scoped enums cannot be
anonymous, so test only named enum definitions inside Objective-C code.

[vsapsai]

I've run clang with this change on a bunch of code (especially Objective-C code) and there were no regressions.

Also adding @benlangmuir to reviewers as people mentioned he was doing some work in this area earlier.

[vsapsai]

Thanks everyone for the feedback! Especially valuable is opinion on making ObjCContainerDecl a lexical decl context. As discussed earlier, not many people can provide feedback on Objective-C-related code and I'm still responsible for any problems caused by this change. So I'll commit the change and please let me know if it breaks anything, I'll be glad to address it.

clang/lib/Parse/ParseObjc.cpp

 
 void Parser::HelperActionsForIvarDeclarations(Decl *interfaceDecl, SourceLocation atLoc,
                                  BalancedDelimiterTracker &T,
                                  SmallVectorImpl<Decl *> &AllIvarDecls,
                                  bool RBraceMissing) {
   if (!RBraceMissing)
     T.consumeClose();
 
-  Actions.ActOnObjCContainerStartDefinition(interfaceDecl);
+  assert(getObjCDeclContext() == interfaceDecl &&
+         "Ivars should have interfaceDecl as their decl context");
   Actions.ActOnLastBitfield(T.getCloseLocation(), AllIvarDecls);
-  Actions.ActOnObjCContainerFinishDefinition();
   // Call ActOnFields() even if we don't have any decls. This is useful
   // for code rewriting tools that need to be aware of the empty list.
   Actions.ActOnFields(getCurScope(), atLoc, interfaceDecl, AllIvarDecls,
                       T.getOpenLocation(), T.getCloseLocation(),
                       ParsedAttributesView());
 }
 
 ///   objc-class-instance-variables:

 ///     struct-declaration
 ///
 void Parser::ParseObjCClassInstanceVariables(Decl *interfaceDecl,
                                              tok::ObjCKeywordKind visibility,
                                              SourceLocation atLoc) {
   assert(Tok.is(tok::l_brace) && "expected {");
   SmallVector<Decl *, 32> AllIvarDecls;
 
   ParseScope ClassScope(this, Scope::DeclScope|Scope::ClassScope);
-  ObjCDeclContextSwitch ObjCDC(*this);
 
   BalancedDelimiterTracker T(*this, tok::l_brace);
   T.consumeOpen();
   // While we still have something to read, read the instance variables.
   while (Tok.isNot(tok::r_brace) && !isEofOrEom()) {
     // Each iteration of this loop reads one objc-instance-variable-decl.
 
     // Check for extraneous top-level semicolon.
while (Tok.isNot(tok::r_brace) && !isEofOrEom()) {
     // C struct and in Objective-C class instance variables.
     if (Tok.isOneOf(tok::kw_static_assert, tok::kw__Static_assert)) {
       SourceLocation DeclEnd;
       ParseStaticAssertDeclaration(DeclEnd);
       continue;
     }
 
     auto ObjCIvarCallback = [&](ParsingFieldDeclarator &FD) {
-      Actions.ActOnObjCContainerStartDefinition(interfaceDecl);
+      assert(getObjCDeclContext() == interfaceDecl &&
+             "Ivar should have interfaceDecl as its decl context");
       // Install the declarator into the interface decl.
       FD.D.setObjCIvar(true);
       Decl *Field = Actions.ActOnIvar(
           getCurScope(), FD.D.getDeclSpec().getSourceRange().getBegin(), FD.D,
           FD.BitfieldSize, visibility);
-      Actions.ActOnObjCContainerFinishDefinition();
       if (Field)
         AllIvarDecls.push_back(Field);
       FD.complete(Field);
     };
 
     // Parse all the comma separated declarators.
     ParsingDeclSpec DS(*this);
     ParseStructDeclaration(DS, ObjCIvarCallback);

clang/lib/Sema/SemaDecl.cpp

if (Name && SS.isNotEmpty()) {
     if (Previous.isAmbiguous())
       return nullptr;
 
     if (!getLangOpts().CPlusPlus && TUK != TUK_Reference) {
       // FIXME: This makes sure that we ignore the contexts associated
       // with C structs, unions, and enums when looking for a matching
       // tag declaration or definition. See the similar lookup tweak
       // in Sema::LookupName; is there a better way to deal with this?
-      while (isa<RecordDecl>(SearchDC) || isa<EnumDecl>(SearchDC))
+      while (isa<RecordDecl, EnumDecl, ObjCContainerDecl>(SearchDC))
+        SearchDC = SearchDC->getParent();
+    } else if (getLangOpts().CPlusPlus) {
+      // Inside ObjCContainer want to keep it as a lexical decl context but go
+      // past it (most often to TranslationUnit) to find the semantic decl
+      // context.
+      while (isa<ObjCContainerDecl>(SearchDC))
         SearchDC = SearchDC->getParent();
     }
   }
 
   if (Previous.isSingleResult() &&
       Previous.getFoundDecl()->isTemplateParameter()) {
     // Maybe we will complain about the shadowed template parameter.
     DiagnoseTemplateParameterShadow(NameLoc, Previous.getFoundDecl());

clang/lib/Serialization/ASTCommon.cpp

if (auto *VD = dyn_cast<VarDecl>(D))
       return VD->isStaticLocal();
     // FIXME: What about CapturedDecls (and declarations nested within them)?
     return isa<TagDecl>(D) || isa<BlockDecl>(D);
   }
 
   // Otherwise, we only care about anonymous class members / block-scope decls.
   // FIXME: We need to handle lambdas and blocks within inline / templated
   // variables too.
-  if (D->getDeclName() || !isa<RecordDecl>(D->getLexicalDeclContext()))
+  if (D->getDeclName())
+    return false;
+  if (!isa<RecordDecl, ObjCInterfaceDecl>(D->getLexicalDeclContext()))
     return false;
   return isa<TagDecl>(D) || isa<FieldDecl>(D);
 }

clang/lib/Serialization/ASTReaderDecl.cpp

 /// for the given lexical DeclContext. We only care about finding local
 /// definitions of the context; we'll merge imported ones as we go.
 DeclContext *
 ASTDeclReader::getPrimaryDCForAnonymousDecl(DeclContext *LexicalDC) {
   // For classes, we track the definition as we merge.
   if (auto *RD = dyn_cast<CXXRecordDecl>(LexicalDC)) {
     auto *DD = RD->getCanonicalDecl()->DefinitionData;
     return DD ? DD->Definition : nullptr;
+  } else if (auto *OID = dyn_cast<ObjCInterfaceDecl>(LexicalDC)) {
+    return OID->getCanonicalDecl()->getDefinition();
   }
 
   // For anything else, walk its merged redeclarations looking for a definition.
   // Note that we can't just call getDefinition here because the redeclaration
   // chain isn't wired up.
   for (auto *D : merged_redecls(cast<Decl>(LexicalDC))) {
     if (auto *FD = dyn_cast<FunctionDecl>(D))
       if (FD->isThisDeclarationADefinition())

clang/test/AST/ast-dump-decl.mm

 - (void) foo {
 }
 @end
 // CHECK:      ObjCImplementationDecl{{.*}} TestObjCImplementation
 // CHECK-NEXT:   super ObjCInterface{{.*}} 'A'
 // CHECK-NEXT:   ObjCInterface{{.*}} 'TestObjCImplementation'
 // CHECK-NEXT:   CXXCtorInitializer{{.*}} 'X'
 // CHECK-NEXT:     CXXConstructExpr
+// CHECK-NEXT:   CXXRecordDecl{{.*}} struct X definition
+// CHECK-NEXT:     DefinitionData
+// CHECK-NEXT:       DefaultConstructor
+// CHECK-NEXT:       CopyConstructor
+// CHECK-NEXT:       MoveConstructor
+// CHECK-NEXT:       CopyAssignment
+// CHECK-NEXT:       MoveAssignment
+// CHECK-NEXT:       Destructor
+// CHECK-NEXT:     CXXRecordDecl{{.*}} struct X
+// CHECK-NEXT:     FieldDecl{{.*}} i 'int'
+// CHECK-NEXT:     CXXConstructorDecl{{.*}} 'void ()
+// CHECK-NEXT:       CompoundStmt
+// CHECK-NEXT:     CXXConstructorDecl{{.*}} 'void (const X &)
+// CHECK-NEXT:       ParmVarDecl{{.*}} 'const X &'
+// CHECK-NEXT:     CXXConstructorDecl{{.*}} 'void (X &&)
+// CHECK-NEXT:       ParmVarDecl{{.*}} 'X &&'
+// CHECK-NEXT:     CXXDestructorDecl
 // CHECK-NEXT:   ObjCIvarDecl{{.*}} X
 // CHECK-NEXT:   ObjCMethodDecl{{.*}} foo
 
 // @() boxing expressions.
 template <typename T>
 struct BoxingTest {
   static id box(T value) {
     return @(value);

clang/test/Modules/merge-anon-record-definition-in-objc.m

+// UNSUPPORTED: -zos, -aix
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: %clang_cc1 -fsyntax-only -F%t/Frameworks %t/test.m -Wno-objc-property-implementation -Wno-incomplete-implementation \
+// RUN:            -fmodules -fmodule-name=Target -fimplicit-module-maps -fmodules-cache-path=%t/modules.cache
+
+// Test anonymous TagDecl inside Objective-C interfaces are merged and ivars with these anonymous types are merged too.
+
+//--- Frameworks/Foundation.framework/Headers/Foundation.h
+@interface NSObject
+@end
+
+//--- Frameworks/Foundation.framework/Modules/module.modulemap
+framework module Foundation {
+  header "Foundation.h"
+  export *
+}
+
+//--- Frameworks/Target.framework/Headers/Target.h
+#import <Foundation/Foundation.h>
+@interface TestClass : NSObject {
+@public
+  struct {
+    struct { int x; int y; } left;
+    struct { int w; int z; } right;
+  } structIvar;
+  union { int x; float y; } *unionIvar;
+  enum { kX = 0, } enumIvar;
+}
+@property struct { int u; } prop;
+- (struct { int v; })method;
+@end
+
+@interface TestClass() {
+@public
+  struct { int y; } extensionIvar;
+}
+@end
+
+@implementation TestClass {
+@public
+  struct { int z; } implementationIvar;
+}
+@end
+
+//--- Frameworks/Target.framework/Modules/module.modulemap
+framework module Target {
+  header "Target.h"
+  export *
+}
+
+//--- Frameworks/Redirect.framework/Headers/Redirect.h
+#import <Target/Target.h>
+
+//--- Frameworks/Redirect.framework/Modules/module.modulemap
+framework module Redirect {
+  header "Redirect.h"
+  export *
+}
+
+//--- test.m
+// At first import everything as non-modular.
+#import <Target/Target.h>
+// And now as modular to merge same entities obtained through different sources.
+#import <Redirect/Redirect.h>
+// Non-modular import is achieved through using the same name (-fmodule-name) as the imported framework module.
+
+void test(TestClass *obj) {
+  obj->structIvar.left.x = 0;
+  obj->unionIvar->y = 1.0f;
+  obj->enumIvar = kX;
+  int tmp = obj.prop.u;
+  tmp += [obj method].v;
+
+  obj->extensionIvar.y = 0;
+  obj->implementationIvar.z = 0;
+}

clang/test/SemaObjC/check-dup-decls-inside-objc.m

+// RUN: %clang_cc1 -fsyntax-only -verify -Wno-objc-root-class %s
+// RUN: %clang_cc1 -fsyntax-only -verify -Wno-objc-root-class -x objective-c++ %s
+
+// Test decls inside Objective-C entities are considered to be duplicates of same-name decls outside of these entities.
+
+@protocol SomeProtocol
+struct InProtocol {}; // expected-note {{previous definition is here}}
+- (union MethodReturnType { int x; float y; })returningMethod; // expected-note {{previous definition is here}}
+#ifdef __cplusplus
+// expected-error@-2 {{'MethodReturnType' cannot be defined in a parameter type}}
+#endif
+@end
+
+@interface Container {
+  struct InInterfaceCurliesWithField {} field; // expected-note {{previous definition is here}}
+  union InInterfaceCurlies { int x; float y; }; // expected-note {{previous definition is here}}
+}
+enum InInterface { kX = 0, }; // expected-note {{previous definition is here}}
+@end
+
+@interface Container(Category)
+union InCategory { int x; float y; }; // expected-note {{previous definition is here}}
+@end
+
+@interface Container() {
+  enum InExtensionCurliesWithField: int { kY = 1, } extensionField; // expected-note {{previous definition is here}}
+  struct InExtensionCurlies {}; // expected-note {{previous definition is here}}
+}
+union InExtension { int x; float y; }; // expected-note {{previous definition is here}}
+@end
+
+@implementation Container {
+  union InImplementationCurliesWithField { int x; float y; } implField; // expected-note {{previous definition is here}}
+  enum InImplementationCurlies { kZ = 2, }; // expected-note {{previous definition is here}}
+}
+struct InImplementation {}; // expected-note {{previous definition is here}}
+@end
+
+@implementation Container(Category)
+enum InCategoryImplementation { kW = 3, }; // expected-note {{previous definition is here}}
+@end
+
+
+struct InProtocol { int a; }; // expected-error {{redefinition of 'InProtocol'}}
+union MethodReturnType { int a; long b; }; // expected-error {{redefinition of 'MethodReturnType'}}
+
+struct InInterfaceCurliesWithField { int a; }; // expected-error {{redefinition of 'InInterfaceCurliesWithField'}}
+union InInterfaceCurlies { int a; long b; }; // expected-error {{redefinition of 'InInterfaceCurlies'}}
+enum InInterface { kA = 10, }; // expected-error {{redefinition of 'InInterface'}}
+
+union InCategory { int a; long b; }; // expected-error {{redefinition of 'InCategory'}}
+
+enum InExtensionCurliesWithField: int { kB = 11, }; // expected-error {{redefinition of 'InExtensionCurliesWithField'}}
+struct InExtensionCurlies { int a; }; // expected-error {{redefinition of 'InExtensionCurlies'}}
+union InExtension { int a; long b; }; // expected-error {{redefinition of 'InExtension'}}
+
+union InImplementationCurliesWithField { int a; long b; }; // expected-error {{redefinition of 'InImplementationCurliesWithField'}}
+enum InImplementationCurlies { kC = 12, }; // expected-error {{redefinition of 'InImplementationCurlies'}}
+struct InImplementation { int a; }; // expected-error {{redefinition of 'InImplementation'}}
+
+enum InCategoryImplementation { kD = 13, }; // expected-error {{redefinition of 'InCategoryImplementation'}}