[Clang] Fix Sema::ClassifyName so that it classifies EnumConstantDecl as NonType when they are brought into scope via using enum

[shafik]

Summary

Currently Sema::ClassifyName(...) in some cases when an enumerator is brought into scope via using enum during lookup it can end up being classified as an OverloadSet. It looks like this was never accounted for when using enum support was implemented and we need to add a check to allow an EnumConstantDecl to be classified as NonType even when it is a class member.

This fixes:
https://github.com/llvm/llvm-project/issues/58057
https://github.com/llvm/llvm-project/issues/59014
https://github.com/llvm/llvm-project/issues/54746

[urnathan]

Thanks, I didn;'t know about ClassifyName, and obviously never hit a need to adjust it. Thanks for fixing.

The comment above says we don't resolve member-access non-overload sets in order to check access. Do we still get that right for, say,

class B {
 enum A {E}; 
 static auto F (B *p) { return p->E;} // ok
};
auto F (B *p) { return p->E; } // not ok

If so, I guess that comment's not right (any more).

[shafik]

In D138091#3930356, @urnathan wrote:

Thanks, I didn;'t know about ClassifyName, and obviously never hit a need to adjust it. Thanks for fixing.

The comment above says we don't resolve member-access non-overload sets in order to check access. Do we still get that right for, say,

class B {
 enum A {E}; 
 static auto F (B *p) { return p->E;} // ok
};
auto F (B *p) { return p->E; } // not ok

If so, I guess that comment's not right (any more).

It says we need to defer certain access checks until we know the context as well.

I checked and with this change it gets this case right. So no behavior change there and I ran check-clang as well and that was clean.

[shafik]

Adding some more reviewers to get more eyes who may be familiar with this corner for the code.

[aaron.ballman]

clang/lib/Sema/SemaDecl.cpp

1250–1252

This change looks correct to me -- enum constants are class members but are certainly not forming an overload set!

Did you audit the other places we call isCXXClassMember() to ensure they're sensible regarding enum constant declarations?

[shafik]

clang/lib/Sema/SemaDecl.cpp

1250–1252

I had not but after looking I don't see any other locations that need modifying.

[aaron.ballman]

LGTM! Please add a release note when landing since this fixes open issues.

[shafik]

Add release note.

clang/docs/ReleaseNotes.rst

- Clang 14 predeclared some builtin POSIX library functions in ``gnu2x`` mode,
   and Clang 15 accidentally stopped predeclaring those functions in that
   language mode. Clang 16 now predeclares those functions again. This fixes
   `Issue 56607 <https://github.com/llvm/llvm-project/issues/56607>`_.
 - GNU attributes being applied prior to standard attributes would be handled
   improperly, which was corrected to match the behaviour exhibited by GCC.
   `Issue 58229 <https://github.com/llvm/llvm-project/issues/58229>`_
 - The builtin type trait ``__is_aggregate`` now returns ``true`` for arrays of incomplete
   types in accordance with the suggested fix for `LWG3823 <https://cplusplus.github.io/LWG/issue3823>`_
+- Fix bug with using enum that could lead to enumerators being treated as if
+  they were part of an overload set. This fixes
+  `Issue 58067 <https://github.com/llvm/llvm-project/issues/58057>`_
+  `Issue 59014 <https://github.com/llvm/llvm-project/issues/59014>`_
+  `Issue 54746 <https://github.com/llvm/llvm-project/issues/54746>`_
 
 Improvements to Clang's diagnostics
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 - Clang will now check compile-time determinable string literals as format strings.
   Fixes `Issue 55805: <https://github.com/llvm/llvm-project/issues/55805>`_.
 - ``-Wformat`` now recognizes ``%b`` for the ``printf``/``scanf`` family of
   functions and ``%B`` for the ``printf`` family of functions. Fixes
   `Issue 56885: <https://github.com/llvm/llvm-project/issues/56885>`_.

clang/lib/Sema/SemaDecl.cpp

if ((NextToken.is(tok::identifier) ||
     return BuildTypeFor(Type, *Result.begin());
   }
 
   // If we already know which single declaration is referenced, just annotate
   // that declaration directly. Defer resolving even non-overloaded class
   // member accesses, as we need to defer certain access checks until we know
   // the context.
   bool ADL = UseArgumentDependentLookup(SS, Result, NextToken.is(tok::l_paren));
-  if (Result.isSingleResult() && !ADL && !FirstDecl->isCXXClassMember())
+  if (Result.isSingleResult() && !ADL &&
+      (!FirstDecl->isCXXClassMember() || isa<EnumConstantDecl>(FirstDecl)))
     return NameClassification::NonType(Result.getRepresentativeDecl());
 
   // Otherwise, this is an overload set that we will need to resolve later.
   Result.suppressDiagnostics();
   return NameClassification::OverloadSet(UnresolvedLookupExpr::Create(
       Context, Result.getNamingClass(), SS.getWithLocInContext(Context),
       Result.getLookupNameInfo(), ADL, Result.isOverloadedResult(),
       Result.begin(), Result.end()));
 }

clang/test/SemaCXX/cxx20-using-enum.cpp

class TPLa {
   using enum Bob; // expected-note{{previous using-enum}}
   using enum Bob; // expected-error{{redeclaration of using-enum}}
 };
 
 TPLa<int> a;
 
 } // namespace Thirteen
 
+namespace GH58057 {
+struct Wrap {
+enum Things {
+  Value1,
+  Value2
+};
+};
+
+using enum Wrap::Things;
+
+int f() {
+  return (Value1 | Value2);
+}
+}
+
+namespace GH59014 {
+struct X {
+  enum Masks {Mask = 1,Shift = 0};
+};
+
+void f(int a) {
+  using enum X::Masks;
+
+  auto u = (Mask);
+  auto v = (Mask << Shift);
+  void (~(Mask));
+}
+}
+
 #endif