[Clang][Sema] Fix type of enumerators in incomplete enumerations #84068

Kupa-Martin · 2024-03-05T20:57:56Z

Enumerators dont have the type of their enumeration before the closing brace. In these cases Expr::getEnumCoercedType() incorrectly returned the enumeration type.

Introduced in PR #81418
Fixes #84712

llvmbot · 2024-03-05T20:58:28Z

@llvm/pr-subscribers-clang

Author: None (Kupa-Martin)

Changes

Enumerators dont have the type of their enumeration before the closing brace. In these cases Expr::getEnumCoercedType() incorrectly returned the enumeration type.

Introduced in PR #81418

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

2 Files Affected:

(modified) clang/lib/AST/Expr.cpp (+6-3)
(modified) clang/test/Sema/warn-compare-enum-types-mismatch.c (+3-1)

diff --git a/clang/lib/AST/Expr.cpp b/clang/lib/AST/Expr.cpp
index b4de2155adcebd..9a28e6f750069b 100644
--- a/clang/lib/AST/Expr.cpp
+++ b/clang/lib/AST/Expr.cpp
@@ -264,10 +264,13 @@ namespace {
 }
 
 QualType Expr::getEnumCoercedType(const ASTContext &Ctx) const {
-  if (isa<EnumType>(this->getType()))
+  if (isa<EnumType>(this->getType())) {
     return this->getType();
-  else if (const auto *ECD = this->getEnumConstantDecl())
-    return Ctx.getTypeDeclType(cast<EnumDecl>(ECD->getDeclContext()));
+  } else if (const auto *ECD = this->getEnumConstantDecl()) {
+    const auto *ED = cast<EnumDecl>(ECD->getDeclContext());
+    if (ED->isCompleteDefinition())
+      return Ctx.getTypeDeclType(ED);
+  }
   return this->getType();
 }
 
diff --git a/clang/test/Sema/warn-compare-enum-types-mismatch.c b/clang/test/Sema/warn-compare-enum-types-mismatch.c
index 2b72aae16b977a..1094a6972e778d 100644
--- a/clang/test/Sema/warn-compare-enum-types-mismatch.c
+++ b/clang/test/Sema/warn-compare-enum-types-mismatch.c
@@ -6,7 +6,9 @@ typedef enum EnumA {
 } EnumA;
 
 enum EnumB {
-  B
+  B,
+  B1 = 1,
+  B2 = A == B1
 };
 
 enum {

Kupa-Martin · 2024-03-05T21:05:15Z

@erichkeane I cant add the previous reviewers myself, could you do it for me please?

clang/lib/AST/Expr.cpp

Sirraide · 2024-03-05T21:13:17Z

Also, this probably needs a release note.

Kupa-Martin · 2024-03-05T22:38:45Z

Also, this probably needs a release note.

If you want I'll add one but this bug has been on main no longer than a week, so I didnt think it would be necessary.

Sirraide · 2024-03-05T22:50:49Z

Also, this probably needs a release note.

If you want I'll add one but this bug has been on main no longer than a week, so I didnt think it would be necessary.

I see. Yeah, I don’t think we really need one if the bug was introduced and fixed in the same version.

erichkeane · 2024-03-06T14:19:55Z

Also, this probably needs a release note.

If you want I'll add one but this bug has been on main no longer than a week, so I didnt think it would be necessary.

I see. Yeah, I don’t think we really need one if the bug was introduced and fixed in the same version.

Thats correct, a release note is only necessary in cases where the bug existed in a release.

shafik

Should we also have a C++ test for this fix?

Kupa-Martin · 2024-03-08T03:09:12Z

Should we also have a C++ test for this fix?

clang/test/Sema/warn-compare-enum-types-mismatch.c should cover both C and C++. Or do you mean some other kind of test?

AaronBallman

Hmm, the rules are different between C and C++, but I think we may be okay. In C++:

https://eel.is/c++draft/enum#dcl.enum-5.sentence-6
Following the closing brace of an enum-specifier, each enumerator has the type of its enumeration.

In C23 6.7.3.3p12:
During the processing of each enumeration constant in the enumerator list, the type of the enumeration constant shall be:

p15:
The enumeration member type for an enumerated type without fixed underlying type upon completion is:
— int if all the values of the enumeration are representable as an int; or,
— the enumerated type.140)

Footnote 140) The integer type selected during processing of the enumerator list (before completion) of the enumeration may not be the same as the compatible implementation-defined integer type selected for the completed enumeration.

So I believe:

typedef enum EnumA {
  A
} EnumA;

enum EnumB {
  B,
  B1 = 1,
  B2 = A == B1
};

is not an enum compare warning in C++ because B1 doesn't have an enumeration type due to the enumeration not being fully-defined, and is not an enum compare warning in C because A has type int (due to p15) and B1 has type int (due to p12).

I think we play a bit fast-and-loose with the way we treat enum constants in C in Clang, but that's outside of the scope of this patch. So LGTM aside from some tiny nits!

AaronBallman · 2024-03-08T12:28:51Z

clang/lib/AST/Expr.cpp

+  if (isa<EnumType>(getType())) {
+    return getType();
+  } else if (const auto *ECD = getEnumConstantDecl()) {
+    const auto *ED = cast<EnumDecl>(ECD->getDeclContext());
+    if (ED->isCompleteDefinition())
+      return Ctx.getTypeDeclType(ED);
+  }


Suggested change

if (isa<EnumType>(getType())) {

return getType();

} else if (const auto *ECD = getEnumConstantDecl()) {

const auto *ED = cast<EnumDecl>(ECD->getDeclContext());

if (ED->isCompleteDefinition())

return Ctx.getTypeDeclType(ED);

}

if (isa<EnumType>(getType()))

return getType();

if (const auto *ECD = getEnumConstantDecl()) {

const auto *ED = cast<EnumDecl>(ECD->getDeclContext());

if (ED->isCompleteDefinition())

return Ctx.getTypeDeclType(ED);

}

No else after return via our coding standard, NFC

AaronBallman · 2024-03-08T12:47:19Z

clang/test/Sema/warn-compare-enum-types-mismatch.c

@@ -6,7 +6,9 @@ typedef enum EnumA {
 } EnumA;

 enum EnumB {
-  B
+  B,


It might be good to capture some of what I wrote in the review summary as a comment here so it's clear why this is correct in both C and C++.

I think what I was asking, was do we have an equivalent C++ test that verifies in a .cpp file that we also do not obtain a diagnostic for this.

Kupa-Martin · 2024-03-08T22:37:50Z

not an enum compare warning in C++ because B1 doesn't have an enumeration type due to the enumeration not being fully-defined, and is not an enum compare warning in C because A has type int (due to p15) and B1 has type int (due to p12).

Yes, you are correct.

I think we play a bit fast-and-loose with the way we treat enum constants in C in Clang, but that's outside of the scope of this patch.

There are enough differences between the C23 and C++ standards to produce different diagnostics. I've made a godbolt snippet that tries to keep track of the type of different enumerators in C23 and C++; we warn when we shouldnt and we dont warn when we should. In any case, this patch didnt intend to cover C23.

shafik · 2024-03-09T00:58:01Z

So I believe:
typedef enum EnumA {
  A
} EnumA;

enum EnumB {
  B,
  B1 = 1,
  B2 = A == B1
};
is not an enum compare warning in C++ because B1 doesn't have an enumeration type due to the enumeration not being fully-defined, and is not an enum compare warning in C because A has type int (due to p15) and B1 has type int (due to p12).

Right, so do we have a test for C++ that verifies that.

Kupa-Martin · 2024-03-09T01:11:00Z

@shafik We dont have a dedicated cpp test for this. I can add one if you want, but clang/test/Sema/warn-compare-enum-types-mismatch.c runs clang both on C and C++ mode, so I didnt think it necessary.

shafik · 2024-03-11T15:19:40Z

@shafik We dont have a dedicated cpp test for this. I can add one if you want, but clang/test/Sema/warn-compare-enum-types-mismatch.c runs clang both on C and C++ mode, so I didnt think it necessary.

I think we just a bug that demonstrates this issue: #84712

So if this fixes that as well then we should add tests for those cases too.

Sirraide · 2024-03-11T15:59:56Z

Regarding #84712: From what I can tell, the behaviour of cases such as

enum e1 { a };
enum e2 { b = a };

has changed between C++11 and C++14 (at least the wording is different starting with C++14; there may be some other section that states the same for C++11, but I couldn’t find one); specifically, C++11’s [dcl.enum]p5 states that:

If the underlying type is not fixed, the type of each enumerator is the type
of its initializing value:

If an initializer is specified for an enumerator, the initializing value has the same type as the expression
and the constant-expression shall be an integral constant expression (5.19)

Whereas in C++14 (and later), that same section reads:

If the underlying type is not fixed, the type of each enumerator prior
to the closing brace is determined as follows:

If an initializer is specified for an enumerator, the constant-expression shall be an integral constant
expression (5.20). If the expression has unscoped enumeration type, the enumerator has the underlying
type of that enumeration type, otherwise it has the same type as the expression.

That is, not only is the type of b above not e2 before the closing brace, but it would seem that its type is supposed to be e1 before C++14, and whatever the underlying type of e1 is starting with C++14.

Kupa-Martin · 2024-03-11T16:40:05Z

@shafik We dont have a dedicated cpp test for this. I can add one if you want, but clang/test/Sema/warn-compare-enum-types-mismatch.c runs clang both on C and C++ mode, so I didnt think it necessary.

I think we just a bug that demonstrates this issue: #84712

So if this fixes that as well then we should add tests for those cases too.

I've included a separate test for C++. If no further changes are needed, could someone please merge this for me? I dont have commit access.

AaronBallman

Aside from some formatting issues, this LGTM. Once you push up a fix for the formatting, I'll land. Thank you!

AaronBallman · 2024-03-11T17:48:13Z

clang/test/Sema/warn-compare-enum-types-mismatch.c

@@ -1,12 +1,19 @@
 // RUN: %clang_cc1 -x c -fsyntax-only -verify -Wenum-compare -Wno-unused-comparison %s
 // RUN: %clang_cc1 -x c++ -fsyntax-only -verify -Wenum-compare -Wno-unused-comparison %s

+// In C enumerators (i.e enumeration constants) have type int (until C23). In order to support diagnostics such as 


There's trailing whitespace here and it goes over the 80-col limit; can you reformat?

AaronBallman · 2024-03-11T17:48:25Z

clang/test/Sema/warn-compare-enum-types-mismatch.c

-  B
+  B,
+  B1 = 1,
+  // In C++ this comparison doesnt warn as enumerators dont have the type of their enumeration before the closing 


Same issue here as above with trailing whitespace and 80 col.

Enumerators dont have the type of their enumeration before the closing brace. In these cases Expr::getEnumCoercedType() incorrectly returned the enumeration type. Introduced in PR llvm#81418 Fixes llvm#84712

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Mar 5, 2024

erichkeane requested review from Xazax-hun, cor3ntin, AaronBallman and shafik March 5, 2024 21:12

Sirraide reviewed Mar 5, 2024

View reviewed changes

clang/lib/AST/Expr.cpp Outdated Show resolved Hide resolved

Kupa-Martin force-pushed the enumcomparefix branch from bc9f937 to 92c0ebc Compare March 5, 2024 22:48

shafik reviewed Mar 8, 2024

View reviewed changes

AaronBallman approved these changes Mar 8, 2024

View reviewed changes

Kupa-Martin force-pushed the enumcomparefix branch from 92c0ebc to 4bbd5cf Compare March 8, 2024 22:40

shafik mentioned this pull request Mar 11, 2024

[clang] Invalid rejection of enum arithmetic #84712

Closed

Kupa-Martin force-pushed the enumcomparefix branch from 4bbd5cf to 57538a2 Compare March 11, 2024 16:36

AaronBallman approved these changes Mar 11, 2024

View reviewed changes

[Clang][Sema] Fix type of enumerators in incomplete enumerations

cc6a132

Enumerators dont have the type of their enumeration before the closing brace. In these cases Expr::getEnumCoercedType() incorrectly returned the enumeration type. Introduced in PR llvm#81418 Fixes llvm#84712

Kupa-Martin force-pushed the enumcomparefix branch from 57538a2 to cc6a132 Compare March 11, 2024 18:13

AaronBallman merged commit f8fab21 into llvm:main Mar 12, 2024
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Clang][Sema] Fix type of enumerators in incomplete enumerations #84068

[Clang][Sema] Fix type of enumerators in incomplete enumerations #84068

Kupa-Martin commented Mar 5, 2024 •

edited

llvmbot commented Mar 5, 2024

Kupa-Martin commented Mar 5, 2024

Sirraide commented Mar 5, 2024

Kupa-Martin commented Mar 5, 2024

Sirraide commented Mar 5, 2024

erichkeane commented Mar 6, 2024

shafik left a comment

Kupa-Martin commented Mar 8, 2024

AaronBallman left a comment

AaronBallman Mar 8, 2024

Kupa-Martin Mar 8, 2024

AaronBallman Mar 8, 2024

Kupa-Martin Mar 8, 2024

shafik Mar 9, 2024

Kupa-Martin commented Mar 8, 2024

shafik commented Mar 9, 2024

Kupa-Martin commented Mar 9, 2024

shafik commented Mar 11, 2024

Sirraide commented Mar 11, 2024 •

edited

Kupa-Martin commented Mar 11, 2024 •

edited

AaronBallman left a comment

AaronBallman Mar 11, 2024

Kupa-Martin Mar 11, 2024

AaronBallman Mar 11, 2024

Kupa-Martin Mar 11, 2024

[Clang][Sema] Fix type of enumerators in incomplete enumerations #84068

[Clang][Sema] Fix type of enumerators in incomplete enumerations #84068

Conversation

Kupa-Martin commented Mar 5, 2024 • edited

llvmbot commented Mar 5, 2024

Kupa-Martin commented Mar 5, 2024

Sirraide commented Mar 5, 2024

Kupa-Martin commented Mar 5, 2024

Sirraide commented Mar 5, 2024

erichkeane commented Mar 6, 2024

shafik left a comment

Choose a reason for hiding this comment

Kupa-Martin commented Mar 8, 2024

AaronBallman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kupa-Martin commented Mar 8, 2024

shafik commented Mar 9, 2024

Kupa-Martin commented Mar 9, 2024

shafik commented Mar 11, 2024

Sirraide commented Mar 11, 2024 • edited

Kupa-Martin commented Mar 11, 2024 • edited

AaronBallman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kupa-Martin commented Mar 5, 2024 •

edited

Sirraide commented Mar 11, 2024 •

edited

Kupa-Martin commented Mar 11, 2024 •

edited