[modules] Fix ambiguous name lookup for enum constants from hidden submodules.

[vsapsai]

Summary

Fix errors like

clang/test/Modules/redefinition-c-tagtypes.m:36:10: error: reference to 'FST' is ambiguous
  return FST;
         ^
clang/test/Modules/redefinition-c-tagtypes.m:24:3: note: candidate found by name lookup is 'FST'
  FST = 22,
  ^
clang/test/Modules/Inputs/F.framework/PrivateHeaders/NS.h:7:3: note: candidate found by name lookup is 'FST'
  FST = 22,
  ^

The name lookup is ambiguous because we have 2 EnumConstantDecl for the
same identifier - one from hidden submodule, another non-modular from .m
file. One way to avoid ambiguity is for decls to have the same canonical
decl. But in this case the hidden decl is deserialized during lexing,
right before non-modular decl is created. So ASTDeclReader cannot do
anything useful in mergeMergeable(Mergeable<EnumConstantDecl>*) and
cannot make different decls have the same canonical decl.

The fix is roughly a follow-up to D31778. As a duplicate non-modular
EnumDecl is dropped, we are dropping its constants too and remove them
from the global name lookup. Another option was to avoid adding
EnumConstantDecl to IdResolver when we know they are parsed for
comparison only. But we still need that global name lookup for constants
referencing other constants from the same enum, like MinXOther = MinX.
So instead we remove the constants after ParseEnumBody is done.

rdar://82908206

[vsapsai]

clang/lib/Sema/SemaDecl.cpp

16216–16223

Here is the early return the added comment refers to.

[vsapsai]

After more testing and thinking I've realized we are still not handling anonymous enums properly. On one hand anonymous EnumDecl aren't duplicates and we aren't making a hidden EnumDecl + EnumConstantDecl visible and don't have ambiguity problems. On the other hand, when we do make them visible through explicit import, we are hitting ambiguous references again.

Need to check the previous discussions and see if can bring ObjC/C closer to C++ in context of handling these duplicates. In ASTReaderDecl I was trying to keep ObjC/C support similar to C++ (modulo templates) and it turned out to be better for comprehension.

[vsapsai]

Add a test case for referencing an anonymous enum constant in C++.

[vsapsai]

Attempt to restore a previous commit.

[vsapsai]

As I was trying to replicate C++ behavior I've created a test case ambiguous-anonymous-enum-lookup.cpp. And it results in diagnostics

clang/test/Modules/Output/ambiguous-anonymous-enum-lookup.cpp.tmp/test.cpp:6:10: warning: ambiguous use of internal linkage declaration 'kAnonymousEnumValue' defined in multiple modules [-Wmodules-ambiguous-internal-linkage]
  return kAnonymousEnumValue;
         ^
clang/test/Modules/Output/ambiguous-anonymous-enum-lookup.cpp.tmp/include/textual.h:5:3: note: declared here
  kAnonymousEnumValue = 0,
  ^
clang/test/Modules/Output/ambiguous-anonymous-enum-lookup.cpp.tmp/include/textual.h:5:3: note: declared here in module 'Piecewise.InitiallyHidden'
  kAnonymousEnumValue = 0,
  ^

Is this warning correct or is it wrong and we have the same bug both in ObjC and C++? I feel I am biased (and bad at C++ linkage rules), so need a separate opinion on correctness.

[vsapsai]

Adding Michael who is infinitely better than me in C++.

[vsapsai]

In discussions outside of this review the consensus is that "warning: ambiguous use of internal linkage declaration" is correct, so I won't change anything for C++. For Objective-C I still need to find a way to handle anonymous enums.

clang/lib/Sema/SemaDecl.cpp

if (TagDecl *PrevTagDecl = dyn_cast<TagDecl>(PrevDecl)) {
               // C11 6.2.7/1 (or 6.1.2.6/1 in C89).
               NamedDecl *Hidden = nullptr;
               if (SkipBody && !hasVisibleDefinition(Def, &Hidden)) {
                 // There is a definition of this tag, but it is not visible. We
                 // explicitly make use of C++'s one definition rule here, and
                 // assume that this definition is identical to the hidden one
                 // we already have. Make the existing definition visible and
                 // use it in place of this one.
                 if (!getLangOpts().CPlusPlus) {
                   // Postpone making the old definition visible until after we
                   // complete parsing the new one and do the structural
                   // comparison.
                   SkipBody->CheckSameAsPrevious = true;
                   SkipBody->New = createTagFromNewDecl();
                   SkipBody->Previous = Def;
                   return Def;
                 } else {
                   SkipBody->ShouldSkip = true;
                   SkipBody->Previous = Def;
                   makeMergedDefinitionVisible(Hidden);
                   // Carry on and handle it like a normal definition. We'll
                   // skip starting the definitiion later.
                 }
               } else if (!IsExplicitSpecializationAfterInstantiation) {

 
 bool Sema::ActOnDuplicateDefinition(DeclSpec &DS, Decl *Prev,
                                     SkipBodyInfo &SkipBody) {
   if (!hasStructuralCompatLayout(Prev, SkipBody.New))
     return false;
 
   // Make the previous decl visible.
   makeMergedDefinitionVisible(SkipBody.Previous);
+  // For the ignored `EnumDecl` definition remove its `EnumConstantDecl` from
+  // global name lookup to avoid future ambiguous lookups. Doing this for ObjC/C
+  // to mimic early return in `Sema::ActOnTag` that avoids calling
+  // `PushOnScopeChains` for duplicate `TagDecl` definitions. Doing it only for
+  // `EnumDecl` among all `TagDecl` because enum constants have global name
+  // lookup while record field lookup is limited to a record.
+  if (!getLangOpts().CPlusPlus) {
+    if (const auto *ED = dyn_cast<EnumDecl>(SkipBody.New))
+      for (EnumDecl::enumerator_iterator EC = ED->enumerator_begin(),
+                                         End = ED->enumerator_end();
+           EC != End; ++EC)
+        IdResolver.RemoveDecl(*EC);
+  }
   return true;
 }
 
 Decl *Sema::ActOnObjCContainerStartDefinition(Decl *IDecl) {
   assert(isa<ObjCContainerDecl>(IDecl) &&
          "ActOnObjCContainerStartDefinition - Not ObjCContainerDecl");
   DeclContext *OCD = cast<DeclContext>(IDecl);
   assert(OCD->getLexicalParent() == CurContext &&

clang/test/Modules/ambiguous-anonymous-enum-lookup.cpp

+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: %clang_cc1 -fsyntax-only -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/modules.cache -I %t/include %t/test.cpp -verify
+
+//--- include/textual.h
+#ifndef TEXTUAL_H
+#define TEXTUAL_H
+
+enum {
+  kAnonymousEnumValue = 0,
+};
+
+#endif
+
+//--- include/empty.h
+// empty
+
+//--- include/initially_hidden.h
+#include <textual.h>
+
+//--- include/module.modulemap
+module Piecewise {
+  module Empty {
+    header "empty.h"
+    export *
+  }
+  module InitiallyHidden {
+    header "initially_hidden.h"
+    export *
+  }
+}
+
+//--- test.cpp
+#include <empty.h>
+#include <textual.h>
+#include <initially_hidden.h>
+
+// expected-no-diagnostics
+int testReferencingAnonymousEnumConstant() {
+  return kAnonymousEnumValue;
+}

clang/test/Modules/redefinition-c-tagtypes.m

 #else
   SND = 44, // expected-note {{enumerator 'SND' with value 44 here}}
   // expected-error@redefinition-c-tagtypes.m:23 {{type 'enum NSE' has incompatible definitions}}
   // expected-note@Inputs/F.framework/PrivateHeaders/NS.h:8 {{enumerator 'SND' with value 43 here}}
 #endif
   TRD = 55
 };
 
+int testReferencingNSEConstant() {
+  return FST;
+}
+
 #define NS_ENUM(_type, _name) \
   enum _name : _type _name;   \
   enum _name : _type
 
 typedef NS_ENUM(int, NSMyEnum) {
   MinX = 11,
 #ifndef CHANGE_TAGS
   MinXOther = MinX,
 #else
   MinXOther = TRD, // expected-note {{enumerator 'MinXOther' with value 55 here}}
-  // expected-error@redefinition-c-tagtypes.m:39 {{type 'enum NSMyEnum' has incompatible definitions}}
+  // expected-error@redefinition-c-tagtypes.m:43 {{type 'enum NSMyEnum' has incompatible definitions}}
   // expected-note@Inputs/F.framework/PrivateHeaders/NS.h:18 {{enumerator 'MinXOther' with value 11 here}}
 #endif
 };
+
+int testReferencingNSMyEnumConstant() {
+  return MinX;
+}