Page MenuHomePhabricator

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

Authored by riccibruno on Apr 21 2019, 2:42 PM.

Details

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.

Diff Detail

Repository
rC Clang

Event Timeline

riccibruno created this revision.Apr 21 2019, 2:42 PM
aaron.ballman added inline comments.Apr 22 2019, 4:53 AM
lib/Sema/SemaDecl.cpp
16645

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

riccibruno marked an inline comment as done.Apr 22 2019, 5:06 AM
riccibruno added inline comments.
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 added inline comments.Apr 22 2019, 5:18 AM
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 updated this revision to Diff 196264.EditedApr 23 2019, 9:01 AM
  • 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 marked an inline comment as done.Apr 23 2019, 9:10 AM
riccibruno added inline comments.
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.

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 marked an inline comment as done.Apr 23 2019, 10:50 AM
riccibruno added inline comments.
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 planned changes to this revision.Apr 25 2019, 6:04 AM
riccibruno retitled this revision from [Sema] Fix the lookup for a declaration conflicting with an enumerator (bogus use of LookupResult::getAsSingle) to [Sema] Fix the lookup for a declaration conflicting with an enumerator.
riccibruno edited the summary of this revision. (Show Details)
riccibruno added a reviewer: rjmccall.
riccibruno marked 3 inline comments as done.Apr 29 2019, 5:15 PM
riccibruno added inline comments.
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.

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 updated this revision to Diff 198629.May 8 2019, 5:24 AM
riccibruno marked 4 inline comments as done.

Address Aaron's comments.