[Clang][Sema] Skip checking anonymous enum in using enum declaration

[jcsxky]

Try to fix #86790
getFETokenInfo requires DeclarationName shouldn't be empty and this will produce crash when checking name conflict of an anonymous NamedDecl in Sema::PushOnScopeChains and whether it's a reserved identifier or not. These wouldn't happen when it's a anonymous enum and we can skip the checking and just add the declaration to current scope.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Qizhi Hu (jcsxky)

Changes

Try to fix #86790
Skip checking anonymous enumeration in using enum declaration.

---

Full diff: https://github.com/llvm/llvm-project/pull/87144.diff

3 Files Affected:

• (modified) clang/docs/ReleaseNotes.rst (+1)
• (modified) clang/lib/Sema/SemaDecl.cpp (+4)
• (added) clang/test/SemaCXX/PR86790.cpp (+9)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 232de0d7d8bb73..39f896652d03d5 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -459,6 +459,7 @@ Bug Fixes to C++ Support
   following the first `::` were ignored).
 - Fix an out-of-bounds crash when checking the validity of template partial specializations. (part of #GH86757).
 - Fix an issue caused by not handling invalid cases when substituting into the parameter mapping of a constraint. Fixes (#GH86757).
+- Fix a crash when the using enum declaration uses an anonymous enumeration. Fixes (#GH86790).
 
 Bug Fixes to AST Handling
 ^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index 0bd88ece2aa544..dda0b08d93bc75 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -1537,6 +1537,10 @@ void Sema::PushOnScopeChains(NamedDecl *D, Scope *S, bool AddToContext) {
       cast<FunctionDecl>(D)->isFunctionTemplateSpecialization())
     return;
 
+  if (isa<EnumDecl>(D) && D->getDeclName().isEmpty()) {
+    S->AddDecl(D);
+    return;
+  }
   // If this replaces anything in the current scope,
   IdentifierResolver::iterator I = IdResolver.begin(D->getDeclName()),
                                IEnd = IdResolver.end();
diff --git a/clang/test/SemaCXX/PR86790.cpp b/clang/test/SemaCXX/PR86790.cpp
new file mode 100644
index 00000000000000..65403baea32c01
--- /dev/null
+++ b/clang/test/SemaCXX/PR86790.cpp
@@ -0,0 +1,9 @@
+// RUN: %clang_cc1 -verify -std=c++20 -fsyntax-only %s
+// expected-no-diagnostics
+
+enum {A, S, D, F};
+int main() {
+    using asdf = decltype(A);
+    using enum asdf; // this line causes the crash
+    return 0;
+}
\ No newline at end of file

[AaronBallman]

Thank you! LGTM aside from expanding the testing a bit.

[AaronBallman]

I'd like to see an additional test case demonstrating that we correctly added the enumerators to the correct scope:

enum {A, S, D, F};
constexpr struct T {
  using asdf = decltype(A);
  using enum asdf;
} t;

static_assert(t.D == D);
static_assert(T::S == S);

and

enum {A, S, D, F};
constexpr struct T {
  struct {
    using asdf = decltype(A);
    using enum asdf;
  } inner;
} t;

static_assert(t.inner.D == D);
static_assert(t.D == D); // expected-error

[jcsxky]

Thanks for your review and testcase. This makes the fix complete. Code has been updated with the new added testcase.

[shafik]

LGTM after addressing Aaron's comments.

Can you elaborate more on the details of the bug in the summary. This goes into the git log and we want folks to be able to understand the problem well from the summary w/o having to do additional checks.

Thank you

[jcsxky]

LGTM after addressing Aaron's comments.

Can you elaborate more on the details of the bug in the summary. This goes into the git log and we want folks to be able to understand the problem well from the summary w/o having to do additional checks.

Thank you

Added.

[zygoloid]

Why do we push a UsingEnumDecl into the scope at all? Would it make sense for the caller to just add the declaration to the DeclContext instead of calling PushOnScopeChains?

[jcsxky]

Sema::ActOnUsingEnumDeclaration invoke PushOnScopeChains when D is a UsingEnumDecl (anonymous or not) and do some extra works. Just add the declaration would skip checking even it's not anonymous.

[zygoloid]

Are those useful checks? It seems surprising to me that we'd push a using enum declaration into scope given that it never introduces any name of its own (the enumerators are handled separately). Are we really performing checks here that we need for a named enum but that are wrong pr unnecessary for an anonymous enum?

[jcsxky]

These checks include name conflict and reserved identifier or not. From this perspective I think it's required here. It will take some time to attempt to skip calling PushOnScopeChains and run unittest to find failed cases.

[zygoloid]

For what name? The using enum declaration doesn't introduce the name of the enum.

[zygoloid]

The reserved identifier check appears to be incorrect. For example, for:

enum __A {
    x, y
};
void f() {
    using enum __A;
    enum __A e;
}

Clang produces two warnings: one on the declaration of enum __A and one on the using enum __A. The second warning is a bug -- the -Wreserved-identifier warning is only supposed to fire on the declaration of a reserved name, and using enum __A does not declare the name __A, it only references it. (Note that there's no warning on the use of enum __A on the next line, which again is only a reference, not a declaration.)

[jcsxky]

Ah, I see. I skip those checking when D is using enum(UsingEnumDecl) by adding

if (isa<UsingEnumDecl>(D))
    return;

at

llvm-project/clang/lib/Sema/SemaDecl.cpp

Line 1529 in cebf77f

and it makes sense after runing unit test on check-clang locally. Then I think you are right.