[Clang][Sema] Fix missing warning when comparing mismatched enums in … #81418

martinkupa · 2024-02-11T15:33:23Z

…C mode

Factored logic from CheckImplicitConversion into new methods Expr::getEnumConstantDecl and Expr::getEnumCoercedType for use in checkEnumArithmeticConversions.

Fix #29217

github-actions · 2024-02-11T15:33:40Z

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be
notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write
permissions for the repository. In which case you can instead tag reviewers by
name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review
by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate
is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

llvmbot · 2024-02-11T15:34:05Z

@llvm/pr-subscribers-clang

Author: None (44-2-Kupa-Martin)

Changes

…C mode

Factored logic from CheckImplicitConversion into new methods Expr::getEnumConstantDecl and Expr::getEnumCoercedType for use in checkEnumArithmeticConversions.

Fix #29217

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

9 Files Affected:

(modified) clang/docs/ReleaseNotes.rst (+3)
(modified) clang/include/clang/AST/Expr.h (+13)
(modified) clang/lib/AST/Expr.cpp (+16)
(modified) clang/lib/Sema/SemaChecking.cpp (+2-9)
(modified) clang/lib/Sema/SemaExpr.cpp (+2-1)
(modified) clang/test/Sema/builtins-elementwise-math.c (+4)
(added) clang/test/Sema/warn-compare-enum-types-mismatch.c (+42)
(renamed) clang/test/Sema/warn-conditional-enum-types-mismatch.c (+1-1)
(modified) clang/test/Sema/warn-overlap.c (+2-2)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index ece6013f672621..00ddf0b9656a31 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -161,6 +161,9 @@ Improvements to Clang's time-trace
 
 Bug Fixes in This Version
 -------------------------
+- Fixed missing warnings when comparing mismatched enumeration constants
+  in C (`#29217 <https://github.com/llvm/llvm-project/issues/29217>`).
+
 - Clang now accepts elaborated-type-specifiers that explicitly specialize
   a member class template for an implicit instantiation of a class template.
 
diff --git a/clang/include/clang/AST/Expr.h b/clang/include/clang/AST/Expr.h
index 3fc481a62a78a9..bf0622bdeca30e 100644
--- a/clang/include/clang/AST/Expr.h
+++ b/clang/include/clang/AST/Expr.h
@@ -153,6 +153,12 @@ class Expr : public ValueStmt {
     TR = t;
   }
 
+  /// If this expression is an enumeration constant, return the
+  /// enumeration type under which said constant was declared.
+  /// Otherwise return the expression's type.
+  /// Note this effectively circumvents the weak typing of C's enum constants
+  QualType getEnumCoercedType(const ASTContext &Ctx) const;
+
   ExprDependence getDependence() const {
     return static_cast<ExprDependence>(ExprBits.Dependent);
   }
@@ -471,6 +477,13 @@ class Expr : public ValueStmt {
   /// bit-fields, but it will return null for a conditional bit-field.
   FieldDecl *getSourceBitField();
 
+  /// If this expression refers to an enum constant, retrieve its declaration
+  EnumConstantDecl *getEnumConstantDecl();
+
+  const EnumConstantDecl *getEnumConstantDecl() const {
+    return const_cast<Expr *>(this)->getEnumConstantDecl();
+  }
+
   const FieldDecl *getSourceBitField() const {
     return const_cast<Expr*>(this)->getSourceBitField();
   }
diff --git a/clang/lib/AST/Expr.cpp b/clang/lib/AST/Expr.cpp
index 8b10e289583260..78a0aba5abc3f1 100644
--- a/clang/lib/AST/Expr.cpp
+++ b/clang/lib/AST/Expr.cpp
@@ -263,6 +263,14 @@ namespace {
   }
 }
 
+QualType Expr::getEnumCoercedType(const ASTContext &Ctx) const {
+  bool NotEnumType = dyn_cast<EnumType>(this->getType()) == nullptr;
+  if (NotEnumType)
+    if (const EnumConstantDecl *ECD = this->getEnumConstantDecl())
+      return Ctx.getTypeDeclType(cast<EnumDecl>(ECD->getDeclContext()));
+  return this->getType();
+}
+
 SourceLocation Expr::getExprLoc() const {
   switch (getStmtClass()) {
   case Stmt::NoStmtClass: llvm_unreachable("statement without class");
@@ -4097,6 +4105,14 @@ FieldDecl *Expr::getSourceBitField() {
   return nullptr;
 }
 
+EnumConstantDecl *Expr::getEnumConstantDecl() {
+  Expr *E = this->IgnoreParenImpCasts();
+  if (DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(E))
+    if (EnumConstantDecl *ECD = dyn_cast<EnumConstantDecl>(DRE->getDecl()))
+      return ECD;
+  return nullptr;
+}
+
 bool Expr::refersToVectorElement() const {
   // FIXME: Why do we not just look at the ObjectKind here?
   const Expr *E = this->IgnoreParens();
diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp
index 71e6e7230fc455..1a869e16c987f4 100644
--- a/clang/lib/Sema/SemaChecking.cpp
+++ b/clang/lib/Sema/SemaChecking.cpp
@@ -16050,15 +16050,8 @@ static void CheckImplicitConversion(Sema &S, Expr *E, QualType T,
   // Diagnose conversions between different enumeration types.
   // In C, we pretend that the type of an EnumConstantDecl is its enumeration
   // type, to give us better diagnostics.
-  QualType SourceType = E->getType();
-  if (!S.getLangOpts().CPlusPlus) {
-    if (DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(E))
-      if (EnumConstantDecl *ECD = dyn_cast<EnumConstantDecl>(DRE->getDecl())) {
-        EnumDecl *Enum = cast<EnumDecl>(ECD->getDeclContext());
-        SourceType = S.Context.getTypeDeclType(Enum);
-        Source = S.Context.getCanonicalType(SourceType).getTypePtr();
-      }
-  }
+  QualType SourceType = E->getEnumCoercedType(S.Context);
+  Source = S.Context.getCanonicalType(SourceType).getTypePtr();
 
   if (const EnumType *SourceEnum = Source->getAs<EnumType>())
     if (const EnumType *TargetEnum = Target->getAs<EnumType>())
diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index 4049ab3bf6cafb..2b6accfd4a4d50 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -1497,7 +1497,8 @@ static void checkEnumArithmeticConversions(Sema &S, Expr *LHS, Expr *RHS,
   //
   // Warn on this in all language modes. Produce a deprecation warning in C++20.
   // Eventually we will presumably reject these cases (in C++23 onwards?).
-  QualType L = LHS->getType(), R = RHS->getType();
+  QualType L = LHS->getEnumCoercedType(S.Context),
+           R = RHS->getEnumCoercedType(S.Context);
   bool LEnum = L->isUnscopedEnumerationType(),
        REnum = R->isUnscopedEnumerationType();
   bool IsCompAssign = ACK == Sema::ACK_CompAssign;
diff --git a/clang/test/Sema/builtins-elementwise-math.c b/clang/test/Sema/builtins-elementwise-math.c
index e7b36285fa7dcf..2e05337273ee41 100644
--- a/clang/test/Sema/builtins-elementwise-math.c
+++ b/clang/test/Sema/builtins-elementwise-math.c
@@ -76,6 +76,7 @@ void test_builtin_elementwise_add_sat(int i, short s, double d, float4 v, int3 i
 
   enum f { three };
   enum f x = __builtin_elementwise_add_sat(one, three);
+  // expected-warning@-1 {{comparison of different enumeration types ('enum e' and 'enum f')}}
 
   _BitInt(32) ext; // expected-warning {{'_BitInt' in C17 and earlier is a Clang extension}}
   ext = __builtin_elementwise_add_sat(ext, ext);
@@ -134,6 +135,7 @@ void test_builtin_elementwise_sub_sat(int i, short s, double d, float4 v, int3 i
 
   enum f { three };
   enum f x = __builtin_elementwise_sub_sat(one, three);
+  // expected-warning@-1 {{comparison of different enumeration types ('enum e' and 'enum f')}}
 
   _BitInt(32) ext; // expected-warning {{'_BitInt' in C17 and earlier is a Clang extension}}
   ext = __builtin_elementwise_sub_sat(ext, ext);
@@ -189,6 +191,7 @@ void test_builtin_elementwise_max(int i, short s, double d, float4 v, int3 iv, u
 
   enum f { three };
   enum f x = __builtin_elementwise_max(one, three);
+  // expected-warning@-1 {{comparison of different enumeration types ('enum e' and 'enum f')}}
 
   _BitInt(32) ext; // expected-warning {{'_BitInt' in C17 and earlier is a Clang extension}}
   ext = __builtin_elementwise_max(ext, ext);
@@ -244,6 +247,7 @@ void test_builtin_elementwise_min(int i, short s, double d, float4 v, int3 iv, u
 
   enum f { three };
   enum f x = __builtin_elementwise_min(one, three);
+  // expected-warning@-1 {{comparison of different enumeration types ('enum e' and 'enum f')}}
 
   _BitInt(32) ext; // expected-warning {{'_BitInt' in C17 and earlier is a Clang extension}}
   ext = __builtin_elementwise_min(ext, ext);
diff --git a/clang/test/Sema/warn-compare-enum-types-mismatch.c b/clang/test/Sema/warn-compare-enum-types-mismatch.c
new file mode 100644
index 00000000000000..1c50f35516353f
--- /dev/null
+++ b/clang/test/Sema/warn-compare-enum-types-mismatch.c
@@ -0,0 +1,42 @@
+// 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
+
+typedef enum EnumA {
+  A
+} EnumA;
+
+enum EnumB {
+  B
+};
+
+enum {
+  C
+};
+
+void foo(void) {
+  enum EnumA a = A;
+  enum EnumB b = B;
+  A == B;
+  // expected-warning@-1 {{comparison of different enumeration types}}
+  a == (B);
+  // expected-warning@-1 {{comparison of different enumeration types}}
+  a == b;
+  // expected-warning@-1 {{comparison of different enumeration types}}
+  A > B;
+  // expected-warning@-1 {{comparison of different enumeration types}}
+  A >= b;
+  // expected-warning@-1 {{comparison of different enumeration types}}
+  a > b;
+  // expected-warning@-1 {{comparison of different enumeration types}}
+  (A) <= ((B));
+  // expected-warning@-1 {{comparison of different enumeration types}}
+  a < B;
+  // expected-warning@-1 {{comparison of different enumeration types}}
+  a < b;
+  // expected-warning@-1 {{comparison of different enumeration types}}
+
+  // In the following cases we purposefully differ from GCC and dont warn 
+  a == C; 
+  A < C;
+  b >= C; 
+}
\ No newline at end of file
diff --git a/clang/test/Sema/warn-conditional-emum-types-mismatch.c b/clang/test/Sema/warn-conditional-enum-types-mismatch.c
similarity index 88%
rename from clang/test/Sema/warn-conditional-emum-types-mismatch.c
rename to clang/test/Sema/warn-conditional-enum-types-mismatch.c
index c9e2eddc7764bd..f039245b6fabe5 100644
--- a/clang/test/Sema/warn-conditional-emum-types-mismatch.c
+++ b/clang/test/Sema/warn-conditional-enum-types-mismatch.c
@@ -21,7 +21,7 @@ int get_flag(int cond) {
   #ifdef __cplusplus
   // expected-warning@-2 {{conditional expression between different enumeration types ('ro' and 'rw')}}
   #else 
-  // expected-no-diagnostics
+  // expected-warning@-4 {{conditional expression between different enumeration types ('enum ro' and 'enum rw')}}
   #endif
 }
 
diff --git a/clang/test/Sema/warn-overlap.c b/clang/test/Sema/warn-overlap.c
index 1eddfd1077fd92..2db07ebcd17b87 100644
--- a/clang/test/Sema/warn-overlap.c
+++ b/clang/test/Sema/warn-overlap.c
@@ -1,5 +1,5 @@
-// RUN: %clang_cc1 -fsyntax-only -verify -Wtautological-overlap-compare %s
-// RUN: %clang_cc1 -fsyntax-only -verify -Wall -Wno-unused -Wno-loop-analysis %s
+// RUN: %clang_cc1 -fsyntax-only -verify -Wtautological-overlap-compare -Wno-enum-compare %s
+// RUN: %clang_cc1 -fsyntax-only -verify -Wall -Wno-unused -Wno-loop-analysis -Wno-enum-compare %s
 
 #define mydefine 2

martinkupa · 2024-02-20T16:44:54Z

ping

erichkeane

Not the best one to review this, but some drive-bys

erichkeane · 2024-02-22T15:50:47Z

clang/lib/AST/Expr.cpp

@@ -263,6 +263,14 @@ namespace {
  }
 }

+QualType Expr::getEnumCoercedType(const ASTContext &Ctx) const {
+  bool NotEnumType = dyn_cast<EnumType>(this->getType()) == nullptr;
+  if (NotEnumType)


Suggested change

if (NotEnumType)

if (!isa<EnumType>(this->getType()))

Though, would probably suggest just something like:

if (isa<EnumType>(getType())) return getType(); else if (const EnumConstantDecl *ECD = getEnumConstantDecl()) return Ctx.getTypeDeclType... return getType();

erichkeane · 2024-02-22T15:54:09Z

clang/lib/AST/Expr.cpp

+EnumConstantDecl *Expr::getEnumConstantDecl() {
+  Expr *E = this->IgnoreParenImpCasts();
+  if (DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(E))
+    if (EnumConstantDecl *ECD = dyn_cast<EnumConstantDecl>(DRE->getDecl()))


Suggested change

if (EnumConstantDecl *ECD = dyn_cast<EnumConstantDecl>(DRE->getDecl()))

return dyn_cast<EnumConstantDecl>(DRE->getDecl());

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

clang/lib/AST/Expr.cpp

erichkeane

A couple more coding standard edits, else LGTM.

clang/lib/AST/Expr.cpp

erichkeane · 2024-02-23T20:57:57Z

clang/lib/AST/Expr.cpp

@@ -4097,6 +4105,13 @@ FieldDecl *Expr::getSourceBitField() {
  return nullptr;
 }

+EnumConstantDecl *Expr::getEnumConstantDecl() {
+  Expr *E = this->IgnoreParenImpCasts();
+  if (DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(E))


Suggested change

if (DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(E))

if (auto *DRE = dyn_cast<DeclRefExpr>(E))

…C mode Factored logic from `CheckImplicitConversion` into new methods `Expr::getEnumConstantDecl` and `Expr::getEnumCoercedType` for use in `checkEnumArithmeticConversions`. Fix llvm#29217

martinkupa · 2024-02-26T21:34:44Z

If everything looks good could someone please merge this pr? I dont have commit access.

github-actions · 2024-02-27T14:59:19Z

@Kupa-Martin Congratulations on having your first Pull Request (PR) merged into the LLVM Project!

Your changes will be combined with recent changes from other authors, then tested
by our build bots. If there is a problem with a build, you may recieve a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as
the builds can include changes from many authors. It is not uncommon for your
change to be included in a build that fails due to someone else's changes, or
infrastructure issues.

How to do this, and the rest of the post-merge process, is covered in detail here.

If your change does cause a problem, it may be reverted, or you can revert it yourself.
This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!

nathanchance · 2024-02-28T01:33:41Z

For what it's worth, this change adds several instances of -Wenum-enum-conversion for the Linux kernel: ClangBuiltLinux/linux#2002. I assume this is intentional given the nature of the change as a whole but neither the tests nor the release notes seem to really reflect that. In case it is a bug that this change affected that, consider this a report :)

erichkeane · 2024-02-28T14:21:10Z

For what it's worth, this change adds several instances of -Wenum-enum-conversion for the Linux kernel: ClangBuiltLinux/linux#2002. I assume this is intentional given the nature of the change as a whole but neither the tests nor the release notes seem to really reflect that. In case it is a bug that this change affected that, consider this a report :)

You're correct, those are intentional. https://github.com/llvm/llvm-project/pull/81418/files#diff-f240d794093a226da48919a27929148597edd1460a1d38b0decbced3fc153ddc specifically covers 'conversions' but arithmetic operations are also covered by that warning.

stbergmann · 2024-03-03T09:58:49Z

This change started to break the following C++26 code:

$ cat test.cc
enum E1 { E11 };
enum E2 {
    E21 = E11,
    E22 = 1,
    E23 = E21 + E22
};

clang++ -std=c++26 -fsyntax-only test.cc
test.cc:5:15: error: invalid arithmetic between different enumeration types ('E1' and 'E2')
    5 |     E23 = E21 + E22
      |           ~~~ ^ ~~~
1 error generated.

as within the definition of enum E2 with unfixed underlying type, while the type of E21 is indeed E1, the type of E22 there is int rather than E2 (see [dcl.enum]/5.1 "If an initializer is specified for an enumerator, the constant-expression shall be an integral constant expression (7.7). 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.")

martinkupa · 2024-03-03T12:24:03Z

@stbergmann is right. @erichkeane How should I proceed? Should I open a revert PR? Or should I open a separate issue?

erichkeane · 2024-03-04T14:37:31Z

@stbergmann is right. @erichkeane How should I proceed? Should I open a revert PR? Or should I open a separate issue?

For now, unless someone insists, I think we're likely fine to do a follow-up PR to fix that. Make sure to put the same reviewers on, and we'll see if we can review this quickly.

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

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

) 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

The behavior of this warning changed in llvm/llvm-project#81418, specifically for C code. Bug: 328490295 Change-Id: I613768ac057aaa2dbe9fbb9303595fda99bacbf9 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5368669 Reviewed-by: Arthur Eubanks <[email protected]> Commit-Queue: Amy Huang <[email protected]> Cr-Commit-Position: refs/heads/main@{#1271874}

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

shafik requested review from cor3ntin, AaronBallman, Xazax-hun, erichkeane and Endilll February 22, 2024 06:14

erichkeane reviewed Feb 22, 2024

View reviewed changes

shafik reviewed Feb 22, 2024

View reviewed changes

clang/test/Sema/warn-compare-enum-types-mismatch.c Outdated Show resolved Hide resolved

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

martinkupa force-pushed the enumcompare branch 2 times, most recently from ca5879d to f08540e Compare February 23, 2024 20:48

erichkeane reviewed Feb 23, 2024

View reviewed changes

[Clang][Sema] Fix missing warning when comparing mismatched enums in …

706ad23

…C mode Factored logic from `CheckImplicitConversion` into new methods `Expr::getEnumConstantDecl` and `Expr::getEnumCoercedType` for use in `checkEnumArithmeticConversions`. Fix llvm#29217

martinkupa force-pushed the enumcompare branch from f08540e to 706ad23 Compare February 23, 2024 21:10

Endilll removed their request for review February 23, 2024 21:26

erichkeane approved these changes Feb 27, 2024

View reviewed changes

erichkeane merged commit 8c2ae42 into llvm:main Feb 27, 2024

martinkupa deleted the enumcompare branch March 4, 2024 15:52

martinkupa mentioned this pull request Mar 5, 2024

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Clang][Sema] Fix missing warning when comparing mismatched enums in … #81418

[Clang][Sema] Fix missing warning when comparing mismatched enums in … #81418

martinkupa commented Feb 11, 2024

github-actions bot commented Feb 11, 2024

llvmbot commented Feb 11, 2024

martinkupa commented Feb 20, 2024

erichkeane left a comment

erichkeane Feb 22, 2024

martinkupa Feb 23, 2024

erichkeane Feb 22, 2024

erichkeane left a comment

erichkeane Feb 23, 2024

martinkupa commented Feb 26, 2024

github-actions bot commented Feb 27, 2024

nathanchance commented Feb 28, 2024

erichkeane commented Feb 28, 2024

stbergmann commented Mar 3, 2024

martinkupa commented Mar 3, 2024

erichkeane commented Mar 4, 2024

	if (EnumConstantDecl *ECD = dyn_cast<EnumConstantDecl>(DRE->getDecl()))
	return dyn_cast<EnumConstantDecl>(DRE->getDecl());

	if (DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(E))
	if (auto *DRE = dyn_cast<DeclRefExpr>(E))

[Clang][Sema] Fix missing warning when comparing mismatched enums in … #81418

[Clang][Sema] Fix missing warning when comparing mismatched enums in … #81418

Conversation

martinkupa commented Feb 11, 2024

github-actions bot commented Feb 11, 2024

llvmbot commented Feb 11, 2024

martinkupa commented Feb 20, 2024

erichkeane left a comment

Choose a reason for hiding this comment

erichkeane Feb 22, 2024

Choose a reason for hiding this comment

martinkupa Feb 23, 2024

Choose a reason for hiding this comment

erichkeane Feb 22, 2024

Choose a reason for hiding this comment

erichkeane left a comment

Choose a reason for hiding this comment

erichkeane Feb 23, 2024

Choose a reason for hiding this comment

martinkupa commented Feb 26, 2024

github-actions bot commented Feb 27, 2024

nathanchance commented Feb 28, 2024

erichkeane commented Feb 28, 2024

stbergmann commented Mar 3, 2024

martinkupa commented Mar 3, 2024

erichkeane commented Mar 4, 2024