WIP: [clang][modules] Do not report declarations without linkage as ambiguous

[jansvoboda11]

Summary

When finalizing the result of name lookup that encountered ambiguity, we check equivalence for declarations with internal linkage. If the declarations are equivalent, we just produce a warning instead of ambiguous result.

In Sema::isEquivalentInternalLinkageDeclaration that implements the check, we have a special case for constants of anonymous enums. However, it only kicks in for C++ anonymous enums (since they have NoLinkage, meaning they are not externally visible).

In (Objective)C, constants of anonymous enums are VisibleNoLinkage, meaning they _are_ externally visible, so the special case doesn't apply for them.

This patch renames the function to isEquivalentNonExternalLinkageDeclaration (its documentation already says it handles declarations with "internal/no linkage") and makes it so that even VisibleNoLinkage declarations are not marked as ambiguous. This is achieved by using hasExternalFormalLinkage instead of isExternallyVisible in one of the equivalence checks. Later on, we don't even emit the warning for such declarations, since they are considered the same entity.

[vsapsai]

My concern with this approach is I'm not sure if isEquivalentInternalLinkageDeclaration is a desired long-term solution for anonymous C++ enums. If it is, we can piggy-back on it, otherwise hitching our wagon to this train might be undesirable.

Also I need to check the patch again because NonExternal != Internal is not easy for my comprehension.

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

10

For enums it is nice to test having a constant that references another constant in the same enum. E.g., kAnonymousEnumAnotherValue = kAnonymousEnumValue + 1.

39

Should we test also non-equivalent enums or is it tested somewhere else?

[jansvoboda11]

In D121465#3384825, @vsapsai wrote:

My concern with this approach is I'm not sure if isEquivalentInternalLinkageDeclaration is a desired long-term solution for anonymous C++ enums.

Is there anything suggesting it's not?

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

39

I believe that's tested in redefinition-c-tagtypes.m.

[jansvoboda11]

Refer to enum constant while defining another

clang/include/clang/Sema/Sema.h

public:
   bool hasVisibleExplicitSpecialization(
       const NamedDecl *D, llvm::SmallVectorImpl<Module *> *Modules = nullptr);
 
   /// Determine if there is a visible declaration of \p D that is a member
   /// specialization declaration (as opposed to an instantiated declaration).
   bool hasVisibleMemberSpecialization(
       const NamedDecl *D, llvm::SmallVectorImpl<Module *> *Modules = nullptr);
 
-  /// Determine if \p A and \p B are equivalent internal linkage declarations
+  /// Determine if \p A and \p B are equivalent no/internal linkage declarations
   /// from different modules, and thus an ambiguity error can be downgraded to
-  /// an extension warning.
-  bool isEquivalentInternalLinkageDeclaration(const NamedDecl *A,
+  /// an extension warning or omitted.
+  bool isEquivalentNonExternalLinkageDeclaration(const NamedDecl *A,
                                                  const NamedDecl *B);
   void diagnoseEquivalentInternalLinkageDeclarations(
       SourceLocation Loc, const NamedDecl *D,
       ArrayRef<const NamedDecl *> Equiv);
 
   bool isUsualDeallocationFunction(const CXXMethodDecl *FD);
 
   bool isCompleteType(SourceLocation Loc, QualType T,
                       CompleteTypeKind Kind = CompleteTypeKind::Default) {

clang/lib/Sema/SemaDeclCXX.cpp

if (auto *RD = dyn_cast<CXXRecordDecl>(D)) {
               DeclarationNameInfo(BUD->getDeclName(), BUD->getLocation())))
         return true;
     }
 
     if (IsEquivalentForUsingDecl(Context, D, Target)) {
       if (UsingShadowDecl *Shadow = dyn_cast<UsingShadowDecl>(*I))
         PrevShadow = Shadow;
       FoundEquivalentDecl = true;
-    } else if (isEquivalentInternalLinkageDeclaration(D, Target)) {
+    } else if (isEquivalentNonExternalLinkageDeclaration(D, Target)) {
       // We don't conflict with an existing using shadow decl of an equivalent
       // declaration, but we're not a redeclaration of it.
       FoundEquivalentDecl = true;
     }
 
     if (isVisible(D))
       (isa<TagDecl>(D) ? Tag : NonTag) = D;
   }

clang/lib/Sema/SemaLookup.cpp

void LookupResult::resolveKind() {
   llvm::SmallDenseMap<QualType, unsigned, 16> UniqueTypes;
 
   bool Ambiguous = false;
   bool HasTag = false, HasFunction = false;
   bool HasFunctionTemplate = false, HasUnresolved = false;
   NamedDecl *HasNonFunction = nullptr;
 
   llvm::SmallVector<NamedDecl*, 4> EquivalentNonFunctions;
+  bool ComplainOnEquivalent = false;
 
   unsigned UniqueTagIndex = 0;
 
   unsigned I = 0;
   while (I < N) {
     NamedDecl *D = Decls[I]->getUnderlyingDecl();
     D = cast<NamedDecl>(D->getCanonicalDecl());
 
while (I < N) {
     } else if (isa<FunctionTemplateDecl>(D)) {
       HasFunction = true;
       HasFunctionTemplate = true;
     } else if (isa<FunctionDecl>(D)) {
       HasFunction = true;
     } else {
       if (HasNonFunction) {
         // If we're about to create an ambiguity between two declarations that
-        // are equivalent, but one is an internal linkage declaration from one
-        // module and the other is an internal linkage declaration from another
-        // module, just skip it.
-        if (getSema().isEquivalentInternalLinkageDeclaration(HasNonFunction,
+        // are equivalent, but one is an internal/no linkage declaration from
+        // one module and the other is an internal/no linkage declaration from
+        // another module, just skip it.
+        if (getSema().isEquivalentNonExternalLinkageDeclaration(HasNonFunction,
                                                                 D)) {
+          // Don't complain about declarations that are externally visible.
+          ComplainOnEquivalent |= !HasNonFunction->isExternallyVisible();
+          ComplainOnEquivalent |= !D->isExternallyVisible();
+
           EquivalentNonFunctions.push_back(D);
           Decls[I] = Decls[--N];
           continue;
         }
 
         Ambiguous = true;
       }
       HasNonFunction = D;
if (isa<TagDecl>(Decls[UniqueTagIndex]->getUnderlyingDecl()) &&
         canHideTag(OtherDecl))
       Decls[UniqueTagIndex] = Decls[--N];
     else
       Ambiguous = true;
   }
 
   // FIXME: This diagnostic should really be delayed until we're done with
   // the lookup result, in case the ambiguity is resolved by the caller.
-  if (!EquivalentNonFunctions.empty() && !Ambiguous)
+  if (!EquivalentNonFunctions.empty() && !Ambiguous && ComplainOnEquivalent)
     getSema().diagnoseEquivalentInternalLinkageDeclarations(
         getNameLoc(), HasNonFunction, EquivalentNonFunctions);
 
   Decls.truncate(N);
 
   if (HasNonFunction && (HasFunction || HasUnresolved))
     Ambiguous = true;
 

clang/lib/Sema/SemaOverload.cpp

 
 /// Determine whether two declarations are "equivalent" for the purposes of
 /// name lookup and overload resolution. This applies when the same internal/no
 /// linkage entity is defined by two modules (probably by textually including
 /// the same header). In such a case, we don't consider the declarations to
 /// declare the same entity, but we also don't want lookups with both
 /// declarations visible to be ambiguous in some cases (this happens when using
 /// a modularized libstdc++).
-bool Sema::isEquivalentInternalLinkageDeclaration(const NamedDecl *A,
+bool Sema::isEquivalentNonExternalLinkageDeclaration(const NamedDecl *A,
                                                      const NamedDecl *B) {
   auto *VA = dyn_cast_or_null<ValueDecl>(A);
   auto *VB = dyn_cast_or_null<ValueDecl>(B);
   if (!VA || !VB)
     return false;
 
-  // The declarations must be declaring the same name as an internal linkage
+  // The declarations must be declaring the same name as an internal/no linkage
   // entity in different modules.
   if (!VA->getDeclContext()->getRedeclContext()->Equals(
           VB->getDeclContext()->getRedeclContext()) ||
       getOwningModule(VA) == getOwningModule(VB) ||
-      VA->isExternallyVisible() || VB->isExternallyVisible())
+      VA->hasExternalFormalLinkage() || VB->hasExternalFormalLinkage())
     return false;
 
   // Check that the declarations appear to be equivalent.
   //
   // FIXME: Checking the type isn't really enough to resolve the ambiguity.
   // For constants and functions, we should check the initializer or body is
   // the same. For non-constant variables, we shouldn't allow it at all.
   if (Context.hasSameType(VA->getType(), VB->getType()))
OverloadCandidateSet::BestViableFunction(Sema &S, SourceLocation Loc,
   while (!PendingBest.empty()) {
     auto *Curr = PendingBest.pop_back_val();
     for (auto *Cand : Candidates) {
       if (Cand->Viable && !Cand->Best &&
           !isBetterOverloadCandidate(S, *Curr, *Cand, Loc, Kind)) {
         PendingBest.push_back(Cand);
         Cand->Best = true;
 
-        if (S.isEquivalentInternalLinkageDeclaration(Cand->Function,
+        if (S.isEquivalentNonExternalLinkageDeclaration(Cand->Function,
                                                         Curr->Function))
           EquivalentCands.push_back(Cand->Function);
         else
           Best = end();
       }
     }
   }
 
   // If we found more than one best candidate, this is ambiguous.

clang/test/Modules/ambiguous-anonymous-enum-lookup.m.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
+// RUN: %clang_cc1 -fsyntax-only -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/modules.cache -I %t/include %t/test.m   -verify
+
+//--- include/textual.h
+#ifndef TEXTUAL_H
+#define TEXTUAL_H
+enum {
+  kAnonymousEnumValue0 = 0,
+  kAnonymousEnumValue1 = kAnonymousEnumValue0 + 1
+};
+#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" }
+}
+
+//--- test.cpp
+#include "empty.h"
+#include "textual.h"
+#include "initially_hidden.h"
+
+int testReferencingAnonymousEnumConstant() {
+  return kAnonymousEnumValue1; // expected-warning {{ambiguous use of internal linkage declaration 'kAnonymousEnumValue1' defined in multiple modules}}
+                               // expected-note@textual.h:5 {{declared here}}
+                               // expected-note@textual.h:5 {{declared here in module 'Piecewise.InitiallyHidden'}}
+}
+
+//--- test.m
+#include "empty.h"
+#include "textual.h"
+#include "initially_hidden.h"
+
+int testReferencingAnonymousEnumConstant() {
+  return kAnonymousEnumValue1; // expected-no-diagnostics
+}