Improve -Wshadow warnings with enumerators

[aaron.ballman]

Summary

Currently, we do not check for enumerators that shadow other enumerators as part of -Wshadow, but gcc does provide such a diagnostic for this case. This is intended to catch shadowing issues like:

enum E1{e1};
void f(void) {
  enum E2{e1};
}

This patch addresses PR24718.

[aaron.ballman]

Ping?

[erik.pilkington]

lib/Sema/SemaDecl.cpp

16320

I don't think we should do this for scoped enums in C++, i.e. this should be fine:

int Foo;
enum class X { Foo };

Can you enclose this in if (!TheEnumDecl->isScoped()) and add a test? Other than that, LGTM!

[aaron.ballman]

Updating based on review feedback.

lib/Sema/SemaDecl.cpp

16320

Thanks, that's a great point! I've corrected in an updated patch.

[aaron.ballman]

Committed in r344259 on @erik.pilkington 's LGTM. Self-accepting so I can close the review.

[sberg]

doesnt this make -Wshadow more aggressive for enumerators than for other entities?

~ cat test17.cc
struct S1;
struct S2;
struct S3 {
  void S1();
  enum { S2 };
};

~ llvm/inst/bin/clang++ -fsyntax-only -Wshadow test17.cc 
test17.cc:5:10: warning: declaration shadows a variable in the global namespace
      [-Wshadow]
  enum { S2 };
         ^
test17.cc:2:8: note: previous declaration is here
struct S2;
       ^
1 warning generated.

warns about enumerator S2 but not about similar function S1

(ran into such a new -Wshadow while compiling LibreOffice)

[aaron.ballman]

In D52400#1266307, @sberg wrote:

doesnt this make -Wshadow more aggressive for enumerators than for other entities?

It does, but whether that's an issue with the enumerator shadow diagnosing or with the other entities not diagnosing, I'm less clear. Consider this slight modification to your code:

struct S1;
struct S2;
struct S3 {
  void S1();
  enum { S2 };

  void f(S2 s);
};

On the one hand, the warning is telling you about a problem before you hit it. However, this code will err on the declaration of S::f() anyway because S2 is not a type, so the warning doesn't get us all *that* much benefit.

Then again, this is a case where you don't get any error but you do get a silent behavioral ambiguity without the current enumerator shadow diagnostic:

struct S1;
struct S2;
struct S3 {
  void S1();
  enum { S2 };

  void f(decltype(S2) s);
};

So there are cases where this behavior can be somewhat useful.

(ran into such a new -Wshadow while compiling LibreOffice)

Was it a frequent/annoying occurrence?

[sberg]

In D52400#1266341, @aaron.ballman wrote:

In D52400#1266307, @sberg wrote:

[...]

Then again, this is a case where you don't get any error but you do get a silent behavioral ambiguity without the current enumerator shadow diagnostic:

struct S1;
struct S2;
struct S3 {
  void S1();
  enum { S2 };

  void f(decltype(S2) s);
};

So there are cases where this behavior can be somewhat useful.

but decltype(S2) is a syntax error when S2 names a struct type, no?

(ran into such a new -Wshadow while compiling LibreOffice)

Was it a frequent/annoying occurrence?

there was less than 20 cases overall. about half were "good" warnings about clashing enumerators from different non-scoped enums. the others were "unhelpful" ones about clashes with class names, two of them in stable interface code that cant be changed (so would need ugly #pragma clang warning decorations), one of them even about entities in unrelated include files, so whether a warning is emitted depends on the order in which the files happen to get included in a TU

(and in any case, "declaration shadows a variable" sounds wrong when the shadowed entity is a class type. thats why I thought something is not quite right with this new code yet)

[aaron.ballman]

In D52400#1267499, @sberg wrote:

In D52400#1266341, @aaron.ballman wrote:

In D52400#1266307, @sberg wrote:

[...]

Then again, this is a case where you don't get any error but you do get a silent behavioral ambiguity without the current enumerator shadow diagnostic:

struct S1;
struct S2;
struct S3 {
  void S1();
  enum { S2 };

  void f(decltype(S2) s);
};

So there are cases where this behavior can be somewhat useful.

but decltype(S2) is a syntax error when S2 names a struct type, no?

Ugh, you're absolutely right.

(ran into such a new -Wshadow while compiling LibreOffice)

Was it a frequent/annoying occurrence?

there was less than 20 cases overall. about half were "good" warnings about clashing enumerators from different non-scoped enums. the others were "unhelpful" ones about clashes with class names, two of them in stable interface code that cant be changed (so would need ugly #pragma clang warning decorations), one of them even about entities in unrelated include files, so whether a warning is emitted depends on the order in which the files happen to get included in a TU

Thanks, that's good information!

(and in any case, "declaration shadows a variable" sounds wrong when the shadowed entity is a class type. thats why I thought something is not quite right with this new code yet)

Definitely agreed. I think I'm convinced that this should be silenced when the conflict is with a type rather than a value. I'll try to look into this next week when I'm back from WG14 meetings.

[aaron.ballman]

In D52400#1267731, @aaron.ballman wrote:

In D52400#1267499, @sberg wrote:

(and in any case, "declaration shadows a variable" sounds wrong when the shadowed entity is a class type. thats why I thought something is not quite right with this new code yet)

Definitely agreed. I think I'm convinced that this should be silenced when the conflict is with a type rather than a value. I'll try to look into this next week when I'm back from WG14 meetings.

I've silenced this scenario in r344898, thank you for raising the issue!

[sberg]

I've silenced this scenario in r344898, thank you for raising the issue!

thanks! works fine for me

lib/Sema/SemaDecl.cpp

     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.
-  NamedDecl *PrevDecl = LookupSingleName(S, Id, IdLoc, LookupOrdinaryName,
-                                         ForVisibleRedeclaration);
+  LookupResult R(*this, Id, IdLoc, LookupOrdinaryName, ForVisibleRedeclaration);
+  LookupName(R, S);
+  NamedDecl *PrevDecl = R.getAsSingle<NamedDecl>();
+
   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;
 
   if (PrevDecl) {
+    if (!TheEnumDecl->isScoped()) {
+      // 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++!");
     if (!isa<TagDecl>(PrevDecl) && isDeclInScope(PrevDecl, CurContext, S)) {
       if (isa<EnumConstantDecl>(PrevDecl))
         Diag(IdLoc, diag::err_redefinition_of_enumerator) << Id;
       else

test/Sema/warn-shadow.c

 // rdar://8883302
 void rdar8883302() {
   extern int bob; // don't warn for shadowing.
 }
 
 void test8() {
   int bob; // expected-warning {{declaration shadows a variable in the global scope}}
 }
+
+enum PR24718_1{pr24718}; // expected-note {{previous declaration is here}}
+void PR24718(void) {
+  enum PR24718_2{pr24718}; // expected-warning {{declaration shadows a variable in the global scope}}
+}

test/SemaCXX/warn-shadow.cpp

 namespace PR33947 {
 void f(int a) {
   struct A {
     void g(int a) {}
     A() { int a; }
   };
 }
 }
+
+int PR24718;
+enum class X { PR24718 }; // Ok, not shadowing