[Sema] Fix the lookup for a declaration conflicting with an enumerator

[riccibruno]

Summary

Currently the lookup for a declaration conflicting with an enumerator has two issues, both originating from the use of LookupResult::getAsSingle<NameDecl>():

1. getAsSingle returns nothing if an overload set or an UnresolvedUsingValueDecl is found.
2. getAsSingle calls internally NamedDecl::getUnderlyingDecl on the result. This cause problems with using-declarations and namespace aliases.

While we are at it, also do the check for a conflicting declaration before calling CheckShadow (which implements Wshadow). This avoids redundant Wshadow warnings where we already have an error.

Note that this is not the only place where getAsSingle is mistakenly used, but this is better addressed in a later patch.

[aaron.ballman]

lib/Sema/SemaDecl.cpp

16645

Is the change to PrevDecl->getUnderlyingDecl() intended here?

[riccibruno]

lib/Sema/SemaDecl.cpp

16645

Yes it is. Previously it was done inside LookupResult::getAsSingle. However with this patch PrevDecl at this point can be a UsingShadowDecl for a given using-declaration. We need to look for the underlying declaration since this is what CheckShadow expects.

[aaron.ballman]

lib/Sema/SemaDecl.cpp

16645

But when it's not a UsingShadowDecl, will the behavior now be incorrect? e.g., if it was a NamespaceAliasDecl, won't this check whether you are shadowing the aliased namespace as opposed to the alias name itself? Might be worth some tests.

[riccibruno]

• Added a test (see N_shadow) for the behavior of Wshadow. This test showed that I forgot to change CheckShadow(New, PrevDecl, R); to CheckShadow(New, PrevDecl->getUnderlyingDecl(), R); to match change in the condition of the if statement.

• Added a test which exposes a new problem that this patch incidentally solves (see N_conflicting_namespace_alias). Because of the using directive using namespace M;, the namespace alias i for the namespace Q is found in the redeclaration lookup. Before this patch, since getAsSingle used internally getUnderlyingDecl(), PrevDecl was the NamespaceDecl for Q, and not the NamespaceAliasDecl for M::i. Then the declaration for the enumerator i was mistakenly rejected since Q is in the same scope.

• Clarified some comments.

[riccibruno]

lib/Sema/SemaDecl.cpp

16645

Do you mean in an example like the following ?

namespace Q {}
namespace M { namespace i = Q; }
using namespace M;

enum { i };

This is example is mistakenly rejected (I think!) because of the fact that currently getAsSingle will look through the NamespaceAliasDecl for i, and find the NamespaceDecl for Q. There are currently no shadow warning for such an example.

[riccibruno]

Added a test which exposes a new problem that this patch incidentally solves (see N_conflicting_namespace_alias). Because of the using directive using namespace M;, the namespace alias i for the namespace Q is found in the redeclaration lookup. Before this patch, since getAsSingle used internally getUnderlyingDecl(), PrevDecl was for the NamespaceDecl for Q, and not for the VarDecl for Q::i. Then the declaration for the enumerator i was mistakenly rejected since Q is in the same scope.

I meant in the above "[...] PrevDecl was the NamespaceDecl for Q, and not the NamespaceAliasDecl for M::i"

[riccibruno]

test/CXX/dcl.dcl/dcl.enum/p11-enumerator_scope.cpp

115

Note also that this is just my interpretation, and I am not entirely confident that this is correct.

As a data point GCC rejects enum { t } in the template and accepts the other two enumerators, and then in the instantiation rejects enum { t } and enum { typedef_type } but accepts enum { type } (presumably because it delays the check for the hiding rules to the instantiation).

[riccibruno]

test/CXX/dcl.dcl/dcl.enum/p11-enumerator_scope.cpp

115

I have checked and I believe that this is exactly what CWG 11 says.

[aaron.ballman]

I found a few nits, but generally think this LG. However, I think @rsmith should sign off on it just in case I've misinterpreted something along the way.

lib/Sema/SemaDecl.cpp

16537

behaves as no -> behaves as if no

16543

Missing a default label in front of this?

16597

Spurious newline

16600

Spurious newline

[riccibruno]

Address Aaron's comments.

[riccibruno]

Ping :)

lib/Sema/SemaDecl.cpp

Decl *Sema::ActOnEnumConstant(Scope *S, Decl *theEnumDecl, Decl *lastEnumConst,
   EnumDecl *TheEnumDecl = cast<EnumDecl>(theEnumDecl);
   EnumConstantDecl *LastEnumConst =
     cast_or_null<EnumConstantDecl>(lastEnumConst);
 
   // The scope passed in may not be a decl scope.  Zip up the scope tree until
   // we find one that is.
   S = getNonFieldDeclScope(S);
 
-  // Verify that there isn't already something declared with this name in this
-  // scope.
+  // Verify that there isn't already a conflicting declaration with this name
+  // in this scope.
   LookupResult R(*this, Id, IdLoc, LookupOrdinaryName, ForVisibleRedeclaration);
   LookupName(R, S);
-  NamedDecl *PrevDecl = R.getAsSingle<NamedDecl>();
+  NamedDecl *PrevDecl = nullptr;
+  switch (R.getResultKind()) {
+  case LookupResult::Found:
+  case LookupResult::FoundUnresolvedValue:
+  case LookupResult::FoundOverloaded:
+    // LookupResult::getAsSingle should not be used here for two reasons:
+    // - it looks through UsingShadowDecls (among other things). Using it
+    //   instead would cause us to miss using-declarations.
+    // - it behaves as if no declaration was found when the lookup result
+    //   is not LookupResult::Found. This would cause us to miss
+    //   UnresolvedUsingValueDecls and sets of overloaded functions.
+    PrevDecl = *R.begin();
+    break;
+
+  case LookupResult::NotFound:
+  case LookupResult::NotFoundInCurrentInstantiation:
+    break;
+  case LookupResult::Ambiguous:
+    // We don't have to care about an ambiguous lookup result since
+    // none of the cases in LookupResult::AmbiguityKind are relevant here.
+    break;
+  }
 
+  // Check for shadowed template parameters.
   if (PrevDecl && PrevDecl->isTemplateParameter()) {
-    // Maybe we will complain about the shadowed template parameter.
     DiagnoseTemplateParameterShadow(IdLoc, PrevDecl);
     // Just pretend that we didn't see the previous declaration.
     PrevDecl = nullptr;
   }
 
-  // C++ [class.mem]p15:
-  // If T is the name of a class, then each of the following shall have a name
-  // different from T:
-  // - every enumerator of every member of class T that is an unscoped
-  // enumerated type
-  if (getLangOpts().CPlusPlus && !TheEnumDecl->isScoped())
-    DiagnoseClassNameShadow(TheEnumDecl->getDeclContext(),
-                            DeclarationNameInfo(Id, IdLoc));
-
-  EnumConstantDecl *New =
-    CheckEnumConstant(TheEnumDecl, LastEnumConst, IdLoc, Id, Val);
-  if (!New)
-    return nullptr;
+  // Look through using-declarations and namespace aliases.
+  NamedDecl *UnderlyingPrevDecl =
+      PrevDecl ? PrevDecl->getUnderlyingDecl() : nullptr;
+  assert((!PrevDecl || (PrevDecl && UnderlyingPrevDecl)) &&
+         "A previous declaration was found but no underlying declaration ?");
 
+  // Check for a previous declaration which conflicts with the enumerator.
   if (PrevDecl) {
-    if (!TheEnumDecl->isScoped() && isa<ValueDecl>(PrevDecl)) {
-      // Check for other kinds of shadowing not already handled.
-      CheckShadow(New, PrevDecl, R);
-    }
-
     // When in C++, we may get a TagDecl with the same name; in this case the
     // enum constant will 'hide' the tag.
-    assert((getLangOpts().CPlusPlus || !isa<TagDecl>(PrevDecl)) &&
+    assert((getLangOpts().CPlusPlus || !isa<TagDecl>(UnderlyingPrevDecl)) &&
            "Received TagDecl when not in C++!");
-    if (!isa<TagDecl>(PrevDecl) && isDeclInScope(PrevDecl, CurContext, S)) {
+    // An enumerator can only hide the name of a class or enumeration that is
+    // not a typedef name ([basic.scope.declarative]p4). Additionally:
+    //
+    //  1. C++17 [namespace.udecl]p20:
+    //      If a using-declarator uses the keyword typename and specifies a
+    //      dependent name (17.6.2), the name introduced by the
+    //      using-declaration is treated as a typedef-name
+    //
+    //     Therefore we can just reject UnresolvedUsingTypenameDecls, even
+    //     though they might resolve to a class or enumeration type during
+    //     instantiation.
+    //
+    //  2. C++17 [namespace.udecl]p15 means that we need to look through
+    //     using-declarations to determine if a declaration with the same
+    //     name is allowed.
+    if (!isa<TagDecl>(UnderlyingPrevDecl) &&
+        isDeclInScope(PrevDecl, CurContext, S)) {
+      // If the enumerator conflicts with a (non-dependent) using-declaration,
+      // give more info about the conflict instead of a generic error.
+      if (auto *PrevDeclShadow = dyn_cast<UsingShadowDecl>(PrevDecl)) {
+        Diag(IdLoc, diag::err_using_decl_conflict_reverse);
+        Diag(UnderlyingPrevDecl->getLocation(), diag::note_using_decl_target);
+        Diag(PrevDeclShadow->getUsingDecl()->getLocation(),
+             diag::note_using_decl)
+            << 0;
+      } else {
         if (isa<EnumConstantDecl>(PrevDecl))
           Diag(IdLoc, diag::err_redefinition_of_enumerator) << Id;
         else
           Diag(IdLoc, diag::err_redefinition) << Id;
+
+        // FIXME: Don't use the term "definition" for a declaration
+        // which is not actually a definition (here and in general).
         notePreviousDefinition(PrevDecl, IdLoc);
+      }
+
       return nullptr;
     }
   }
 
+  // C++ [class.mem]p15:
+  //   If T is the name of a class, then each of the following shall have a
+  //   name different from T:
+  //   - every enumerator of every member of class T that is an unscoped
+  //     enumerated type
+  if (getLangOpts().CPlusPlus && !TheEnumDecl->isScoped())
+    DiagnoseClassNameShadow(TheEnumDecl->getDeclContext(),
+                            DeclarationNameInfo(Id, IdLoc));
+
+  EnumConstantDecl *New =
+      CheckEnumConstant(TheEnumDecl, LastEnumConst, IdLoc, Id, Val);
+  if (!New)
+    return nullptr;
+
+  // Check for other kinds of shadowing not already handled.
+  if (PrevDecl && isa<ValueDecl>(UnderlyingPrevDecl) &&
+      !TheEnumDecl->isScoped())
+    CheckShadow(New, UnderlyingPrevDecl, R);
+
   // Process attributes.
   ProcessDeclAttributeList(S, New, Attrs);
   AddPragmaAttributes(S, New);
 
   // Register this decl in the current scope stack.
   New->setAccess(TheEnumDecl->getAccess());
   PushOnScopeChains(New, S);
 
   ActOnDocumentableDecl(New);
 
   return New;
 }
 
 // Returns true when the enum initial expression does not trigger the
 // duplicate enum warning.  A few common cases are exempted as follows:
 // Element2 = Element1
 // Element2 = Element1 + 1
 // Element2 = Element1 - 1
 // Where Element2 and Element1 are from the same enum.
 static bool ValidDuplicateEnum(EnumConstantDecl *ECD, EnumDecl *Enum) {
   Expr *InitExpr = ECD->getInitExpr();
   if (!InitExpr)
     return true;
   InitExpr = InitExpr->IgnoreImpCasts();

test/CXX/dcl.dcl/dcl.enum/p11-enumerator_scope.cpp

 // RUN: %clang_cc1 -std=c++17 -Wno-missing-declarations -verify %s
 // RUN: %clang_cc1 -std=c++17 -Wno-missing-declarations -Wshadow -verify -verify=shadow %s
 
 // [dcl.enum]p11 Each enum-name and each unscoped enumerator is declared
 // in the scope that immediately contains the enum-specifier. Each scoped
 // enumerator is declared in the scope of the enumeration. These names obey
 // the scope rules defined for all names in 6.3 and 6.4.
 
-// FIXME: Actually implement the redeclaration lookup properly.
 // FIXME: Improve the wording of the error messages (definition vs declaration).
-// FIXME: Only emit the Wshadow warning when the declaration of the
-//        enumerator does not conflict with another declaration.
 
-// We also test for -Wshadow since the functionality is closely related,
-// and we don't want to mess up Wshadow by fixing the redeclaration lookup.
+// We also test for -Wshadow since the functionality is closely related.
 
 struct S_function {
   void f();             // expected-note 2{{previous definition}}
-                        // FIXME: Don't emit shadow-note@-1 2{{previous declaration}}
   enum { f };           // expected-error {{redefinition of 'f'}}
-                        // FIXME: Don't emit shadow-warning@-1 {{declaration shadows}}
   enum E1 { f };        // expected-error {{redefinition of 'f'}}
-                        // FIXME: Don't emit shadow-warning@-1 {{declaration shadows}}
   enum class E2 { f };  // ok
 };
 
 struct S_overloaded_function {
   void f();
-  void f(int);
-  enum { f };           // FIXME: Reject.
-  enum E1 { f };        // FIXME: Reject.
+  void f(int);          // expected-note 2{{previous definition}}
+  enum { f };           // expected-error {{redefinition of 'f'}}
+  enum E1 { f };        // expected-error {{redefinition of 'f'}}
   enum class E2 { f };  // ok
 };
 
 struct S_function_template {
-  template <typename> void f();
-  enum { f };          // FIXME: Reject.
-  enum E1 { f };       // FIXME: Reject.
+  template <typename> void f(); // expected-note 2{{previous definition}}
+  enum { f };          // expected-error {{redefinition of 'f'}}
+  enum E1 { f };       // expected-error {{redefinition of 'f'}}
   enum class E2 { f }; // ok
 };
 
 struct S_class_enum {
   struct f;
   enum { f }; // ok
   enum e : int;
   enum { e }; // ok
   template <typename T> struct h;   // expected-note {{previous definition}}
   enum { h };                       // expected-error {{redefinition of 'h'}}
 };
 
 struct S_member {
   struct f;
   int f;          // expected-note {{previous definition}}
-                  // FIXME: Don't emit shadow-note@-1 {{previous declaration}}
   static int g;   // expected-note {{previous definition}}
-                  // FIXME: Don't emit shadow-note@-1 {{previous declaration}}
   enum { f, g };  // expected-error {{redefinition of 'f'}}
                   // expected-error@-1 {{redefinition of 'g'}}
-                  // FIXME: Don't emit shadow-warning@-2 2{{declaration shadows}}
 };
 
 namespace S_out_of_line_definition {
   struct S {
     int x;
     void y();
     enum E : int;
     enum F : int;
     enum G : int;
   };
 
   enum S::E : int { x }; // It is unclear whether this is valid. See CWG 1485
   enum S::F : int { y }; // (Out-of-class definition of member unscoped opaque
   int i = x;             // enumeration).
   int x;                 //
 }
 
 namespace S_using_decl {
 
 struct Base {
-  int f; // FIXME: Don't emit shadow-note {{previous declaration}}
+  int f;  // expected-note {{target of using declaration}}
   int g;
 
   struct s;
-  typedef struct s t;
+  typedef struct s t; // expected-note {{target of using declaration}}
 };
 struct OtherBase {};
 struct Der : Base, OtherBase {
-  using Base::f;
-  enum { f }; // FIXME: Reject.
-              // FIXME: Don't emit shadow-warning@-1 {{declaration shadows}}
+  using Base::f; // expected-note {{using declaration}}
+  enum { f }; // expected-error {{declaration conflicts with target of using declaration already in scope}}
   enum { g }; // ok
 
   using OtherBase::OtherBase;
   enum { OtherBase }; // ok
 
   using Base::s;
   enum { s }; // ok
 
-  using Base::t;
-  enum { t }; // FIXME: Reject.
+  using Base::t;  // expected-note {{using declaration}}
+  enum { t };     // expected-error {{declaration conflicts with target of using declaration already in scope}}
 };
 
 } // namespace S_using_decl
 
 namespace S_unresolved_using_decl {
 
 template <typename T> struct Base {
   T t;
   struct type {};
   typedef type typedef_type;
 };
 
 template <typename T> struct Der : Base<T> {
-  using Base<T>::t;
-  enum { t }; // FIXME: Reject.
+  using Base<T>::t; // expected-note {{previous definition}}
+  enum { t };       // expected-error {{redefinition of 't'}}
   // [namespace.udecl]p20:
   //   If a using-declarator uses the keyword typename and specifies a
   //   dependent name (17.6.2), the name introduced by the using-declaration
   //   is treated as a typedef-name.
   // This rule in addition to the hiding rules from [basic.scope.declarative]
   // implies that the following is ill-formed.
   using typename Base<T>::type; // expected-note {{previous definition}}
   enum { type };                // expected-error {{redefinition of 'type'}}
   using typename Base<T>::typedef_type; // expected-note {{previous definition}}
   enum { typedef_type };                // expected-error {{redefinition of 'typedef_type'}}
 };
 
 Der<int> d;
 
 } // namespace S_unresolved_using_decl
 
 namespace N_function {
   void f();             // expected-note 2{{previous definition}}
-                        // FIXME: Don't emit shadow-note@-1 2{{previous declaration}}
   enum { f };           // expected-error {{redefinition of 'f'}}
-                        // FIXME: Don't emit shadow-warning@-1 {{declaration shadows}}
   enum E1 { f };        // expected-error {{redefinition of 'f'}}
-                        // FIXME: Don't emit shadow-warning@-1 {{declaration shadows}}
   enum class E2 { f };  // ok
 }
 
 namespace N_overloaded_function {
   void f();
-  void f(int);
-  enum { f };             // FIXME: Reject.
-  enum E1 { f };          // FIXME: Reject.
+  void f(int);            // expected-note 2{{previous definition}}
+  enum { f };             // expected-error {{redefinition of 'f'}}
+  enum E1 { f };          // expected-error {{redefinition of 'f'}}
   enum class E2 { f };    // ok
 }
 
 namespace N_function_template {
-  template <typename> void f();
-  enum { f };          // FIXME: Reject.
-  enum E1 { f };       // FIXME: Reject.
+  template <typename> void f(); // expected-note 2{{previous definition}}
+  enum { f };          // expected-error {{redefinition of 'f'}}
+  enum E1 { f };       // expected-error {{redefinition of 'f'}}
   enum class E2 { f }; // ok
 }
 
 namespace N_class_enum {
   struct f;
   enum { f }; // ok
   enum e : int;
   enum { e }; // ok
   template <typename T> struct h;   // expected-note {{previous definition}}
   enum { h };                       // expected-error {{redefinition of 'h'}}
   typedef struct f k;               // expected-note {{previous definition}}
   enum { k };                       // expected-error {{redefinition of 'k'}}
 }
 
 namespace N_variable {
   struct f;
   int f;          // expected-note {{previous definition}}
-                  // FIXME: Don't emit shadow-note@-1 {{previous declaration}}
   static int g;   // expected-note {{previous definition}}
-                  // FIXME: Don't emit shadow-note@-1 {{previous declaration}}
   enum { f, g };  // expected-error {{redefinition of 'f'}}
                   // expected-error@-1 {{redefinition of 'g'}}
-                  // FIXME: Don't emit shadow-warning@-2 2{{declaration shadows}}
 }
 
 namespace N_using_decl_aux {
-  int f;  // shadow-note {{previous declaration}}
+  int f;              // expected-note {{target of using declaration}}
   struct s;
-  typedef struct s t;
+  typedef struct s t; // expected-note {{target of using declaration}}
 }
 namespace N_using_decl {
-  using N_using_decl_aux::f;
-  enum { f }; // FIXME: Reject.
-              // shadow-warning@-1 {{declaration shadows a variable in namespace 'N_using_decl_aux'}}
+  using N_using_decl_aux::f; // expected-note {{using declaration}}
+  enum { f };                // expected-error {{declaration conflicts with target of using declaration already in scope}}
 
   using N_using_decl_aux::s;
   enum { s }; // ok
 
-  using N_using_decl_aux::t;
-  enum { t }; // FIXME: Reject.
+  using N_using_decl_aux::t; // expected-note {{using declaration}}
+  enum { t };                // expected-error {{declaration conflicts with target of using declaration already in scope}}
 }
 
 namespace N_conflicting_namespace_alias {
-  namespace Q { int i; }            // FIXME: expected-note 2{{previous definition}}
+  namespace Q { int i; }
   namespace M { namespace i = Q; }
 
   using namespace M;
-  enum { i };   // FIXME: This is fine. expected-error {{redefinition of 'i'}}
+  enum { i };   // ok
   int j = i::i; // ok
 
-  namespace k = M::i;
+  namespace k = M::i; // expected-note {{previous definition}}
   enum { k };         // expected-error {{redefinition of 'k'}}
 }
 
 namespace N_shadow {
   namespace Q { int i, j; } // shadow-note {{previous declaration}}
   namespace M { using Q::i, Q::j; }
 
   using namespace M;
   enum { i }; // shadow-warning {{declaration shadows a variable in namespace 'N_shadow::Q'}}
   enum class E { j }; // no shadow warning
 }
 
 namespace N_enumerator_redefinition {
-  namespace N { enum { e }; } // FIXME: shadow-note {{previous declaration}}
-  using N::e;
-  enum { e }; // FIXME: Reject.
-              // FIXME: Don't emit shadow-warning@-1 {{declaration shadows}}
+  namespace N { enum { e }; } // expected-note {{target of using declaration}}
+  using N::e; // expected-note {{using declaration}}
+  enum { e }; // expected-error {{declaration conflicts with target of using declaration already in scope}}
 
   enum { f }; // expected-note {{previous definition}}
-              // FIXME: shadow-note@-1 {{previous declaration}}
   enum { f }; // expected-error {{redefinition of enumerator 'f'}}
-              // FIXME: Don't emit shadow-warning@-1 {{declaration shadows}}
 }