[Sema] Error out early for tags defined inside an enumeration.

[vsapsai]

Summary

This fixes PR28903 by avoiding access check for inner enum constant. We
are performing access check because one enum constant references another
and because enum is defined in CXXRecordDecl. But access check doesn't
work because FindDeclaringClass doesn't expect more than one EnumDecl
and because inner enum has access AS_none due to not being an immediate
child of a record.

The change detects an enum is defined in wrong place and allows to skip
parsing its body. Access check is skipped together with body parsing.
There was no crash in C, added test case to cover the new error.

rdar://problem/28530809

[vsapsai]

I have also considered adding tests for structs and unions defined inside an enumeration but decided it doesn't add much value, given the code is working for all TagDecl.

Another option I had in mind is having a note pointing to outer enum. But the nesting is done in the same file so the outer enum shouldn't be hard to find. So I decided that complexity outweighs the added value.

If anybody thinks the aforementioned changes are valuable, please tell so.

[vsapsai]

It occurred to me that another approach for fixing the crash can be setting IsTypeSpecifier to true while we ParseCastExpression. According to

cast-expression:
  unary-expression
  ( type-id ) cast-expression
type-id:
  type-specifier-seq abstract-declarator[opt]

it looks reasonable to require IsTypeSpecifier during parsing part of cast expression inside parentheses.

What do you think? Is this approach worth pursuing? Does it sound better than the one offered initially?

[vsapsai]

Ping. If you don't have time to review the change in depth, please say if the approach is wrong on high level.

[rnk]

I think this looks good, even without fixing the access control crash, this seems like a diagnostic improvement.

clang/test/Sema/enum.c

128

This is a simpler test case that fixes the extraneous diagnostics:

struct PR28903 {
  enum {
    E1 = (enum {  // expected-error-re {{...}}
            E2, E3 = E2
          })0
  } e;
};

[doug.gregor]

Looks good to me; sorry for the delay.

[vsapsai]

• Remove extraneous diagnostics in test case per Reid Kleckner feedback.

[vsapsai]

clang/test/Sema/enum.c

128

Removed extraneous diagnostics in slightly different way. Keep enum constants prefixed with PR28903 because in C struct doesn't create a scope for enum constants and I want to avoid polluting test namespace with common names like E1, E2, E3.

[alberto_magni]

Hi all,
this change is causing busybox to fail to compile.
The faulty enum is here:
https://git.busybox.net/busybox/tree/include/libbb.h#n639
The nested union is inside a sizeof statement.

Something like this:

enum {
  A = sizeof(union {
    int x;
    int y;
  })
};

I this behavior expected ? Do you suggest to patch busybox ?

Many Thanks

[vsapsai]

Thanks for following up, Alberto. I haven't expected such a use case. It is possible to achieve the same with LSA_SIZEOF_SA = sizeof(((len_and_sockaddr *)0)->u) but I don't like it and don't want to force developers using such approach.

For solving this problem I think to restrict error only to C++ and to allow tags inside enums for C. Alberto, what do you think, will it work for you? And from implementation perspective allowing tags in enums for C should be safe because things go haywire for C++ while checking access rules and C doesn't have access rules.

[alberto_magni]

Yes, restricting the error to C++ would work. Many thanks.

[vsapsai]

Submitted for review https://reviews.llvm.org/D38109 - [fixup][Sema] Allow in C to define tags inside enumerations.

clang/include/clang/Basic/DiagnosticSemaKinds.td

 def err_type_defined_in_result_type : Error<
   "%0 cannot be defined in the result type of a function">;
 def err_type_defined_in_param_type : Error<
   "%0 cannot be defined in a parameter type">;
 def err_type_defined_in_alias_template : Error<
   "%0 cannot be defined in a type alias template">;
 def err_type_defined_in_condition : Error<
   "%0 cannot be defined in a condition">;
+def err_type_defined_in_enum : Error<
+  "%0 cannot be defined in an enumeration">;
 
 def note_pure_virtual_function : Note<
   "unimplemented pure virtual method %0 in %1">;
 
 def note_pure_qualified_call_kext : Note<
   "qualified call to %0::%1 is treated as a virtual call to %1 due to -fapple-kext">;
 
 def err_deleted_decl_not_first : Error<

clang/lib/Sema/SemaDecl.cpp

CreateNewDecl:
   //   A type-specifier-seq shall not define a class or enumeration [...].
   if (getLangOpts().CPlusPlus && (IsTypeSpecifier || IsTemplateParamOrArg) &&
       TUK == TUK_Definition) {
     Diag(New->getLocation(), diag::err_type_defined_in_type_specifier)
       << Context.getTagDeclType(New);
     Invalid = true;
   }
 
+  if (!Invalid && TUK == TUK_Definition && DC->getDeclKind() == Decl::Enum) {
+    Diag(New->getLocation(), diag::err_type_defined_in_enum)
+      << Context.getTagDeclType(New);
+    Invalid = true;
+  }
+
   // Maybe add qualifier info.
   if (SS.isNotEmpty()) {
     if (SS.isSet()) {
       // If this is either a declaration or a definition, check the
       // nested-name-specifier against the current context. We don't do this
       // for explicit specializations, because they have similar checking
       // (with more specific diagnostics) in the call to
       // CheckMemberSpecialization, below.

clang/test/Sema/enum.c

 }
 
 typedef enum { NegativeShort = (short)-1 } NegativeShortEnum;
 int NegativeShortTest[NegativeShort == -1 ? 1 : -1];
 
 // PR24610
 enum Color { Red, Green, Blue }; // expected-note{{previous use is here}}
 typedef struct Color NewColor; // expected-error {{use of 'Color' with tag type that does not match previous declaration}}
+
+// PR28903
+struct PR28903 {
+  enum {
+    PR28903_A = (enum { // expected-error-re {{'enum PR28903::(anonymous at {{.*}})' cannot be defined in an enumeration}}
+      PR28903_B,
+      PR28903_C = PR28903_B
+    })0
+  };
+  int makeStructNonEmpty;
+};

clang/test/SemaCXX/enum.cpp

 
 // This is accepted as a GNU extension. In C++98, there was no provision for
 // expressions with UB to be non-constant.
 enum { overflow = 123456 * 234567 };
 #if __cplusplus >= 201103L
 // expected-warning@-2 {{not an integral constant expression}}
 // expected-note@-3 {{value 28958703552 is outside the range of representable values}}
 #endif
+
+// PR28903
+struct PR28903 {
+  enum {
+    PR28903_A = (enum { // expected-error-re {{'PR28903::(anonymous enum at {{.*}})' cannot be defined in an enumeration}}
+      PR28903_B,
+      PR28903_C = PR28903_B
+    })
+  };
+};