[Sema] Reject list-initialization of enumeration types from a brace-init-list containing a single element of a different scoped enumeration type

[ahatanak]

Summary

It is rejected because it doesn't satisfy the condition that the element has to be implicitly convertible to the underlying type of the enum variable.

http://eel.is/c++draft/dcl.init.list#3.8

[ahatanak]

The assertion assert(FromType->isIntegralOrUnscopedEnumerationType()) in StandardConversionSequence::getNarrowingKind fails when the invalid initialization is performed.

[aaron.ballman]

Please add more details to the summary and remove the rdar link (nobody outside of Apple can access that anyway). Also, this should have a release note for the bug fix.

clang/lib/Sema/SemaInit.cpp

4506

This doesn't match the comments immediately above here and I don't think is the correct fix.

We're handling this case: http://eel.is/c++draft/dcl.init.list#3.8

A scoped enumeration has a fixed underlying type (https://eel.is/c++draft/dcl.enum#5.sentence-5). The initializer list has a single element and that element can be implicitly converted to the underlying type (int in all of the test cases changed in this patch). And this is a direct initialization case, so I think we should be performing the conversion here rather than skipping to the next bullet.

[ahatanak]

clang/lib/Sema/SemaInit.cpp

4506

Can scoped enums be implicitly converted to integer types? Unscoped enums can be converted to an integer type, but I don't see any mention of scoped enums here: https://eel.is/c++draft/conv.integral

It seems that the original paper was trying to change the rules about conversions from the underlying type to a scoped enum. It doesn't look like it's allowing conversion from a scope enum to another scope enum.

https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2016/p0138r2.pdf

[aaron.ballman]

clang/lib/Sema/SemaInit.cpp

4506

Can scoped enums be implicitly converted to integer types? Unscoped enums can be converted to an integer type, but I don't see any mention of scoped enums here: https://eel.is/c++draft/conv.integral

Correct, they cannot be implicitly converted to an integer.

It seems that the original paper was trying to change the rules about conversions from the underlying type to a scoped enum. It doesn't look like it's allowing conversion from a scope enum to another scope enum.

Agreed, however, I think where we want this to fail is below in the attempt at conversion. "v can be implicitly converted to U" is the part that should be failing here, and we're now skipping over the bit of code that's checking whether the implicit conversion is valid.

[ahatanak]

clang/lib/Sema/SemaInit.cpp

4506

Is the code below checking whether the implicit conversion is valid? It looks like it's assuming the implicit conversion is valid and adding an implicit conversion sequence based on that assumption. If the source is an integer, unscoped enum, or floating type, the implicit conversion that is performed later should succeed except when there is narrowing.

Or are you suggesting we should add a check to Sema::PerformImplicitConversion that rejects conversions from scoped enums to other types? It seems to me that it's better to detect the error earlier.

[ahatanak]

clang/lib/Sema/SemaInit.cpp

4506

Alternatively, we can emit a diagnostic in the code below that specifically calls out conversion from scoped enums to integer types.

[aaron.ballman]

clang/lib/Sema/SemaInit.cpp

4506

Is the code below checking whether the implicit conversion is valid?

It's forming the conversion sequence as-if it must be valid, but that causes us to get the right diagnostics. We do the same for narrowing float conversions: https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaInit.cpp#L4521 and I would expect us to then need changes so we get to here: https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaInit.cpp#L8478

[ahatanak]

clang/lib/Sema/SemaInit.cpp

4506

But a conversion from a scoped enum to another scoped enum or its underlying type isn't a narrowing conversion unless the conversion from the underlying type is narrowing. I guess the current code is forming the conversion sequence as if it is valid when the source type is a floating type just to call DiagnoseNarrowingInInitList. @rsmith, any comments?

If we want to detect the invalid conversion while performing conversion, shouldn't the call to PerformImplicitConversion, which is called before reaching the call to DiagnoseNarrowingInInitList, fail? Why should it succeed?

https://github.com/llvm/llvm-project/blob/7689c7fc9e08cc430daca3714bcffdd00fd538bd/clang/lib/Sema/SemaInit.cpp#L8467

But I think the invalid conversion should be detected at the very beginning of the function before conversion is attempted where it checks whether the initialization sequence is invalid (https://github.com/llvm/llvm-project/blob/7689c7fc9e08cc430daca3714bcffdd00fd538bd/clang/lib/Sema/SemaInit.cpp#L8020). That can be done by calling Sequence.SetFailed when the source type is a scoped enum.

[ahatanak]

clang/lib/Sema/SemaInit.cpp

4506

Also, it's not clear to me why the diagnostic this patch emits (cannot initialize a variable of type 'test12::B' with an lvalue of type 'test12::A') isn't right. It's kind of generic, but it doesn't seem incorrect to me. What is the correct diagnostic in this case?

[aaron.ballman]

clang/lib/Sema/SemaInit.cpp

4506

Given your example (but with names less likely to cause confusion):

enum class FirstEnum;
enum class SecondEnum;
FirstEnum FirstValue;
SecondEnum SecondValue{FirstValue};

Starting from recognizing that we're performing list initialization, we get to:

http://eel.is/c++draft/dcl.init.list#3.8

Based on our example, T is SecondEnum, U is int, v is FirstValue. The question then becomes can you implicitly convert FirstValue to int and the answer is no. The diagnostic we form in that case is "cannot initialize a variable of type '<type1>' with an lvalue of type '<type2>'". e.g., https://godbolt.org/z/an38EK3cs

So I think I was wrong; based on the comments on line 4508, it looks like we do *not* want to get into that if block but instead let the general single-element case below handle it. (I had missed that last sentence before and that turned out to be an important one.) Based on the diagnostic given when we do that (as your patch currently does), the diagnostic is what I'd expect us to generate.

I'm very sorry for the back and forth on this, but I *think* your patch is actually correct as-is. CC @erichkeane and @rsmith for a second opinion given that I already messed the logic up once before. :-)

[ahatanak]

clang/lib/Sema/SemaInit.cpp

4506

No problem and thank you for the detailed explanation of the rules in the standard. I've updated the summary based on the discussion we had.

[aaron.ballman]

LGTM! Can you also add a release note about the fix before landing the changes?

[ahatanak]

Fix the comment to match the code.

clang/lib/Sema/SemaInit.cpp

if (S.getLangOpts().CPlusPlus && !DestType->isAggregateType() &&
     //     is direct-list-initialization, the object is initialized with the
     //     value T(v); if a narrowing conversion is required to convert v to
     //     the underlying type of T, the program is ill-formed.
     auto *ET = DestType->getAs<EnumType>();
     if (S.getLangOpts().CPlusPlus17 &&
         Kind.getKind() == InitializationKind::IK_DirectList &&
         ET && ET->getDecl()->isFixed() &&
         !S.Context.hasSameUnqualifiedType(E->getType(), DestType) &&
-        (E->getType()->isIntegralOrEnumerationType() ||
+        (E->getType()->isIntegralOrUnscopedEnumerationType() ||
          E->getType()->isFloatingType())) {
       // There are two ways that T(v) can work when T is an enumeration type.
       // If there is either an implicit conversion sequence from v to T or
       // a conversion function that can convert from v to T, then we use that.
-      // Otherwise, if v is of integral, enumeration, or floating-point type,
-      // it is converted to the enumeration type via its underlying type.
+      // Otherwise, if v is of integral, unscoped enumeration, or floating-point
+      // type, it is converted to the enumeration type via its underlying type.
       // There is no overlap possible between these two cases (except when the
       // source value is already of the destination type), and the first
       // case is handled by the general case for single-element lists below.
       ImplicitConversionSequence ICS;
       ICS.setStandard();
       ICS.Standard.setAsIdentityConversion();
       if (!E->isPRValue())
         ICS.Standard.First = ICK_Lvalue_To_Rvalue;

clang/test/SemaCXX/enum-scoped.cpp

 // RUN: %clang_cc1 -fsyntax-only -pedantic -std=c++11 -verify -triple x86_64-apple-darwin %s
+// RUN: %clang_cc1 -fsyntax-only -pedantic -std=c++17 -verify -triple x86_64-apple-darwin %s
 
 enum class E1 {
   Val1 = 1L
 };
 
 enum struct E2 {
   Val1 = '\0'
 };

 E3 v4 = Val2;
 E1 v5 = Val2; // expected-error{{cannot initialize a variable}}
 
 static_assert(sizeof(E3) == 1, "bad size");
 
 int x2 = Val2;
 
 int a1[Val2];
-int a2[E1::Val1]; // expected-error{{size of array has non-integer type}}
+int a2[E1::Val1];
+
+#if __cplusplus >= 201703L
+// expected-error@-3 {{type 'E1' is not implicitly convertible to 'unsigned long'}}
+#else
+// expected-error@-5 {{size of array has non-integer type}}
+#endif
 
 int* p1 = new int[Val2];
-int* p2 = new int[E1::Val1]; // expected-error{{array size expression must have integral or unscoped enumeration type, not 'E1'}}
+int* p2 = new int[E1::Val1];
+
+#if __cplusplus >= 201703L
+// expected-error@-3 {{converting 'E1' to incompatible type 'unsigned long'}}
+#else
+// expected-error@-5 {{array size expression must have integral or unscoped enumeration type, not 'E1'}}
+#endif
 
 enum class E4 {
   e1 = -2147483648, // ok
   e2 = 2147483647, // ok
   e3 = 2147483648 // expected-error{{enumerator value evaluates to 2147483648, which cannot be narrowed to type 'int'}}
 };
 
 enum class E5 {
namespace test11 {
 
   bool f() { return !f1(); } // expected-error {{invalid argument type 'test11::E2' (aka 'test11::E') to unary expression}}
 }
 
 namespace PR35586 {
   enum C { R, G, B };
   enum B { F = (enum C) -1, T}; // this should compile cleanly, it used to assert.
 };
+
+namespace test12 {
+// Check that clang rejects this code without crashing in c++17.
+enum class A;
+enum class B;
+A a;
+B b{a}; // expected-error {{cannot initialize}}
+}