[clang][DR2621] using enum NAME lookup fix

[urnathan]

Summary

Although using-enum's grammar is 'using elaborated-enum-specifier',
the lookup for the enum is ordinary lookup (and not the tagged-type
lookup that normally occurs wth an tagged-type specifier). Thus (a)
we can find typedefs and (b) do not find enum tags hidden by a non-tag
name (the struct stat thing).

This reimplements that part of using-enum handling, to address DR2621,
where clang's behaviour does not match std intent (and other
compilers).

[aaron.ballman]

Thank you for working on this! The precommit CI failures on Windows look to be unrelated. Btw, when you generate the patch, if you could add some more context to the patch (with -U9999) that would be appreciated -- it makes it easier to review when we can see the surrounding code as well.

You should add a release note for the changes as well. It looks like we've not updated the C++ DR status page to have the latest round of DRs, so no need to regenerate that page. But you should still add a test case to clang/test/CXX/drs/dr26xx.cpp with the appropriate markup (see other examples in that file/test directory) so that we get the correct status when we do go to regenerate that page.

clang/include/clang/Basic/DiagnosticSemaKinds.td

607–608

I don't think we need this one; we can use err_unknown_typename.

clang/lib/Parse/ParseDeclCXX.cpp

722–723

Spell out the types because they're not spelled out in the initialization.

clang/lib/Sema/SemaDeclCXX.cpp

11852

I think you should consider using ClassifyName() instead, as that will not only get you the parsed type information, but it handles things like typo correction, etc. Then you shouldn't need to worry about the diagnostics immediately below.

11862

These days we use dyn_cast_if_present instead.

[urnathan]

I forgot I had a git abbriev for the -U9999 thing, sorry about that.

I tried ClassifyName, but although it claimed to have a type, its getType method returned a ParsedType object that seemed broken -- its 'get' methof produced a thing not recognized as valid QualType and whose dump method seg faulted when called.

[urnathan]

clang/lib/Sema/SemaDeclCXX.cpp

11852

See comments, tried & failed.

[aaron.ballman]

In D134283#3808446, @urnathan wrote:

I forgot I had a git abbriev for the -U9999 thing, sorry about that.

No worries!

I tried ClassifyName, but although it claimed to have a type, its getType method returned a ParsedType object that seemed broken -- its 'get' methof produced a thing not recognized as valid QualType and whose dump method seg faulted when called.

Wha? That seems rather concerning. What does getKind() return? (We assert that the kind is correct when trying to get the data out of the object, but perhaps you weren't using an asserts build?)

[urnathan]

Wha? That seems rather concerning. What does getKind() return? (We assert that the kind is correct when trying to get the data out of the object, but perhaps you weren't using an asserts build?)

getKind() says it's a type and getType() returns a thing that could be a (nonnull) ParsedType. That objects get call returns a QualType I am unable to inspect, and that got printed out in diagnostics as no characters (when it failed the getAsTaggedType call). I find the wrapped pointer objects clang seems to love hard to inspect. I tried tracing through ClassifyName to see how it was constructing the type, but got somewhat lost in a set of lambdas. FWIW ClassifyName seems to be mostly employed in figuring out tentative parse disambiguation.

I am using unoptimized assert builds :)

[aaron.ballman]

In D134283#3811198, @urnathan wrote:

Wha? That seems rather concerning. What does getKind() return? (We assert that the kind is correct when trying to get the data out of the object, but perhaps you weren't using an asserts build?)

getKind() says it's a type and getType() returns a thing that could be a (nonnull) ParsedType. That objects get call returns a QualType I am unable to inspect, and that got printed out in diagnostics as no characters (when it failed the getAsTaggedType call). I find the wrapped pointer objects clang seems to love hard to inspect. I tried tracing through ClassifyName to see how it was constructing the type, but got somewhat lost in a set of lambdas. FWIW ClassifyName seems to be mostly employed in figuring out tentative parse disambiguation.

I am using unoptimized assert builds :)

Well huh, TIL! :-D

I'm happy with the state of things, so LGTM, but I did spot the need for a comment to be added to a test case that you should fix when landing, so the DR scraping script works properly.

clang/test/CXX/drs/dr26xx.cpp

3 ↗

(On Diff #462160)

[aaron.ballman]

LGTM!

[junaire]

clang/lib/Parse/ParseDeclCXX.cpp

710

minor suggestion: will it be easier to read if you add comments like /*ParsedType=*/nullptr, /*ObjectHasErrors=*/false and etc?

[shafik]

clang/lib/Parse/ParseDeclCXX.cpp

710

We should be doing this it is part of the style guide.

This can also be verified via clang-tidy bugprone-argument-comment

[h-vetinari]

Seems DR2621 is not yet listed in https://github.com/llvm/llvm-project/blob/main/clang/www/cxx_dr_status.html (but if it were, it should be updated...)

[urnathan]

Here's an update, just to check I've got all the comments addressed. It doesn't appear this DR has made it to the ready status in cwg-index, so ./make_cxx_dr_status barfed.

(For avoidance of doubt, I was at the CWG mtg last week when we discussed this and moved it to Ready).

ETA: I'll commit in a few days, if there are not further comments.

[aaron.ballman]

Reaffirming my LG but there was a minor formatting thing for the release notes that you can fix when landing. Thank you! (Btw, I was also at that CWG and can confirm this matches the resolution of the DR as discussed.)

clang/docs/ReleaseNotes.rst

359 ↗

(On Diff #462951)

clang/include/clang/Basic/DiagnosticParseKinds.td

 def ext_using_enum_declaration : ExtWarn<
   "using enum declaration is a C++20 extension">,
   InGroup<CXX20>;
+def err_using_enum_expect_identifier : Error<
+  "using enum %select{requires an enum or typedef name|"
+  "does not permit an elaborated enum specifier}0">;
 def err_constructor_bad_name : Error<
   "missing return type for function %0; did you mean the constructor name %1?">;
 def err_destructor_tilde_identifier : Error<

clang/include/clang/Basic/DiagnosticSemaKinds.td

   "with C++ standards before C++20">, InGroup<CXXPre20Compat>, DefaultIgnore;
 def err_using_enum_is_dependent : Error<
   "using-enum cannot name a dependent type">;
+def err_using_enum_not_enum : Error<
+  "%0 is not an enumerated type">;
 def err_ambiguous_inherited_constructor : Error<
   "constructor of %0 inherited from multiple base class subobjects">;
 def note_ambiguous_inherited_constructor_using : Note<

   "using declaration annotated with 'using_if_exists' here">;
 def err_using_if_exists_on_ctor : Error<
   "'using_if_exists' attribute cannot be applied to an inheriting constructor">;
+def err_using_enum_not_found : Error<
+  "no type '%0' found">;
 def err_using_enum_decl_redeclaration : Error<
   "redeclaration of using-enum declaration">;
 def note_using_enum_decl : Note<"%select{|previous }0using-enum declaration">;

clang/include/clang/Sema/Sema.h

                               const ParsedAttributesView &AttrList);
   Decl *ActOnUsingEnumDeclaration(Scope *CurScope, AccessSpecifier AS,
                                   SourceLocation UsingLoc,
-                                  SourceLocation EnumLoc, const DeclSpec &);
+                                  SourceLocation EnumLoc,
+                                  SourceLocation IdentLoc, IdentifierInfo &II,
+                                  CXXScopeSpec *SS = nullptr);
   Decl *ActOnAliasDeclaration(Scope *CurScope, AccessSpecifier AS,
                               MultiTemplateParamsArg TemplateParams,
                               SourceLocation UsingLoc, UnqualifiedId &Name,

clang/lib/Parse/ParseDeclCXX.cpp

 ///
 ///     using-enum-declaration: [C++20, dcl.enum]
 ///       'using' elaborated-enum-specifier ;
+///       The terminal name of the elaborated-enum-specifier undergoes
+///       ordinary lookup
 ///
 ///     elaborated-enum-specifier:
 ///       'enum' nested-name-specifier[opt] identifier

 
     DiagnoseCXX11AttributeExtension(PrefixAttrs);
 
-    DeclSpec DS(AttrFactory);
-    ParseEnumSpecifier(UELoc, DS, TemplateInfo, AS,
-                       // DSC_trailing has the semantics we desire
-                       DeclSpecContext::DSC_trailing);
-
     if (TemplateInfo.Kind) {
       SourceRange R = TemplateInfo.getSourceRange();
       Diag(UsingLoc, diag::err_templated_using_directive_declaration)
           << 1 /* declaration */ << R << FixItHint::CreateRemoval(R);
-
+      SkipUntil(tok::semi);
+      return nullptr;
+    }
+    CXXScopeSpec SS;
+    if (ParseOptionalCXXScopeSpecifier(SS, nullptr, false, false, nullptr,
+                                       false, nullptr, false, true)) {
+      SkipUntil(tok::semi);
+      return nullptr;
+    }
+
+    if (!Tok.is(tok::identifier)) {
+      Diag(Tok.getLocation(), diag::err_using_enum_expect_identifier)
+          << Tok.is(tok::kw_enum);
+      SkipUntil(tok::semi);
+      return nullptr;
+    }
+    auto *IdentInfo = Tok.getIdentifierInfo();
+    auto IdentLoc = ConsumeToken();
+    Decl *UED = Actions.ActOnUsingEnumDeclaration(
+        getCurScope(), AS, UsingLoc, UELoc, IdentLoc, *IdentInfo, &SS);
+    if (!UED) {
+      SkipUntil(tok::semi);
       return nullptr;
     }
 
-    Decl *UED = Actions.ActOnUsingEnumDeclaration(getCurScope(), AS, UsingLoc,
-                                                  UELoc, DS);
     DeclEnd = Tok.getLocation();
     if (ExpectAndConsume(tok::semi, diag::err_expected_after,
                          "using-enum declaration"))

clang/lib/Sema/SemaDeclCXX.cpp

 Decl *Sema::ActOnUsingEnumDeclaration(Scope *S, AccessSpecifier AS,
                                       SourceLocation UsingLoc,
                                       SourceLocation EnumLoc,
-                                      const DeclSpec &DS) {
-  switch (DS.getTypeSpecType()) {
-  case DeclSpec::TST_error:
-    // This will already have been diagnosed
+                                      SourceLocation IdentLoc,
+                                      IdentifierInfo &II, CXXScopeSpec *SS) {
+  assert(!SS->isInvalid() && "ScopeSpec is invalid");
+  ParsedType TypeRep = getTypeName(II, IdentLoc, S, SS);
+  if (!TypeRep) {
+    Diag(IdentLoc, SS && isDependentScopeSpecifier(*SS)
+                       ? diag::err_using_enum_is_dependent
+                       : diag::err_using_enum_not_found)
+        << II.getName()
+        << SourceRange(SS ? SS->getBeginLoc() : IdentLoc, IdentLoc);
+    return nullptr;
+  }
+
+  auto *Enum = dyn_cast_or_null<EnumDecl>(TypeRep.get()->getAsTagDecl());
+  if (!Enum) {
+    Diag(IdentLoc, diag::err_using_enum_not_enum) << TypeRep.get();
     return nullptr;
-
-  case DeclSpec::TST_enum:
-    break;
-
-  case DeclSpec::TST_typename:
-    Diag(DS.getTypeSpecTypeLoc(), diag::err_using_enum_is_dependent);
-    return nullptr;
-
-  default:
-    llvm_unreachable("unexpected DeclSpec type");
   }
 
-  // As with enum-decls, we ignore attributes for now.
-  auto *Enum = cast<EnumDecl>(DS.getRepAsDecl());
   if (auto *Def = Enum->getDefinition())
     Enum = Def;
 
-  auto *UD = BuildUsingEnumDeclaration(S, AS, UsingLoc, EnumLoc,
-                                       DS.getTypeSpecTypeNameLoc(), Enum);
+  auto *UD =
+      BuildUsingEnumDeclaration(S, AS, UsingLoc, EnumLoc, IdentLoc, Enum);
   if (UD)
     PushOnScopeChains(UD, S, /*AddToContext*/ false);
 

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

 namespace A {}
 
 void f() {
-  using enum A::+; // expected-error {{expected identifier}}
-  using enum; // expected-error {{expected identifier or '{'}}
-  using enum class; // expected-error {{expected identifier or '{'}}
-  using enum : blah; // expected-error {{unknown type name 'blah'}} expected-error {{unnamed enumeration must be a definition}}
+  using enum A::+; // expected-error {{using enum requires an enum or typedef name}}
+  using enum; // expected-error {{using enum requires an enum or typedef name}}
+  using enum class; // expected-error {{using enum requires an enum or typedef name}}
+  using enum enum q; // expected-error {{using enum does not permit an elaborated enum specifier}}
 }
 }

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

 enum A { a, // expected-note{{declared here}}
          b,
          c };
-class C; // expected-note{{previous use}}
+class C;
 enum class D : int;
 enum class D { d,
                e,

 #if __cplusplus < 202002
 // expected-warning@-2{{is a C++20 extension}}
 #endif
-using enum Bob::B; // expected-error{{no enum named 'B'}}
+using enum Bob::B; // expected-error{{no type 'B' found}}
 #if __cplusplus < 202002
 // expected-warning@-2{{is a C++20 extension}}
 #endif
-using enum Bob::C; // expected-error{{tag type that does not match}}
+using enum Bob::C; // expected-error{{'Bob::C' is not an enumerated type}}
 #if __cplusplus < 202002
 // expected-warning@-2{{is a C++20 extension}}
 #endif

 #if __cplusplus < 202002
 // expected-warning@-2{{is a C++20 extension}}
 #endif
+
+void DR2621() {
+  using A_t = Bob::A;
+  using enum A_t;
+#if __cplusplus < 202002
+// expected-warning@-2{{is a C++20 extension}}
+#endif
+  A_t x = a;
+}
+
 } // namespace One
 
 namespace Two {