[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

16608

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

[riccibruno]

lib/Sema/SemaDecl.cpp

16608

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

16608

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

16608

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

107

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

107

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?

16599

Spurious newline

16602

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 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;
 
+    llvm_unreachable("Unexpected LookupResultKind");
+  }
+
+  // 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;
-
+  // Check for a previous declaration which conflicts with the enumerator.
+  // [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 [basic.scope] and [basic.lookup].
   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)) &&
            "Received TagDecl when not in C++!");
+    // An enumerator can only hide the name of a class or enumeration that is
+    // not a typedef name ([basic.scope.declarative]p4). Additionally:
+    // [namespace.udir]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.
     if (!isa<TagDecl>(PrevDecl) && isDeclInScope(PrevDecl, CurContext, S)) {
       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>(PrevDecl->getUnderlyingDecl()) &&
+      !TheEnumDecl->isScoped())
+    CheckShadow(New, PrevDecl->getUnderlyingDecl(), 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);
 

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.
 
 // Conflict with another declaration at class scope.
 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_data_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;
   int g;
 };
 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 {{previous definition}}
+  enum { f }; // expected-error {{redefinition of 'f'}}
   enum { g }; // ok
 
   using OtherBase::OtherBase;
   enum { OtherBase }; // ok
 };
 
 } // 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'}}
 };
 
 } // namespace S_unresolved_using_decl
 
 // Conflict with another declaration at namespace scope
 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
 
   extern void g();     // expected-note {{previous definition}}
-                       // FIXME: Don't emit shadow-note@-1 {{previous declaration}}
   enum { g };          // expected-error {{redefinition of 'g'}}
-                       // FIXME: Don't emit shadow-warning@-1 {{declaration shadows}}
   enum class E3 { g }; // 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}}
+namespace N_using_decl_aux { int f; }
 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 {{previous definition}}
+  enum { f };                // expected-error {{redefinition of 'f'}}
 }
 
 namespace N_conflicting_namespace_alias {
-  namespace Q { int i; }            // FIXME: expected-note {{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 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
 }