[Sema] Enum conversion warning when one signed and other unsigned.

[red1bluelost]

Summary

Ensures an -Wenum-conversion warning happens when one of the enums is
signed and the other is unsigned. Also adds a test file to verify these
warnings.

This warning would not happen since the -Wsign-conversion would make a
diagnostic then return, never allowing the -Wenum-conversion checks.

For example:

C
enum PE { P = -1 };
enum NE { N };
enum NE conv(enum PE E) { return E; }

Before this would only create a diagnostic with -Wsign-conversion and never on
-Wenum-conversion. Now it will create a diagnostic for both -Wsign-conversion
and -Wenum-conversion.

I could change it to just warn on -Wenum-conversion as that was what I initially
did. Seeing PR35200 (or GitHub Issue 316268), I let both diagnostics check so that
the sign conversion could generate a warning.

This was tested with check-clang and all tests passed along with the new test
file.

[aaron.ballman]

Thank you for working on this, and for your patience -- this review fell off my radar for a bit, sorry about that!

I think there's an issue here to be solved, but I'm not certain this is the correct way to solve it. Giving two or more diagnostics for the same underlying issue is generally something we try not to do, but happens as a result of your changes. However, this code should diagnose all four times, not just twice as it currently does, so there is a bug to fix: https://godbolt.org/z/avPj5q7hc

clang/lib/Sema/SemaChecking.cpp

13556–13557

I don't think this change is correct -- we *want* to silence the conversion warnings in this case. GCC behaves that way as well: https://godbolt.org/z/oona5Mr5T

clang/test/Sema/enum-enum-conversion.c

11–15

Testing tip: instead of using the preprocessor and duplicating so much, you can use a feature of -verify where you give it a prefix. e.g.,

// RUN: ... -verify=stuff ...
// RUN: ... -verify=other ...

void func(void) {
  thing();  // stuff-warning {{"this is a warning about stuff}} \
               other-error {{"this is an error, it's different than stuff}}
}

Using this sort of style makes the tests much easier to read.

26

Please add a newline to the end of the file.

[red1bluelost]

Updates patch based on comments.

Silences the sign conversions on enum-to-enum so that only enum-to-enum will warn.
Simplifies the test case since now only one warning occurs.

[red1bluelost]

clang/lib/Sema/SemaChecking.cpp

13556–13557

Updated the code to silence the sign conversion and only warn the enum-conversion.

clang/test/Sema/enum-enum-conversion.c

11–15

Thank! That is helpful to know!
I ended up not needing it here since the silencing of sign-conversion means only one warning will occur.

[aaron.ballman]

Thanks for the updates! I think this is getting somewhat close, but I'd like to see some additional test cases to ensure we're not regressing behavior we care about (I think we may be losing warnings about sign conversion).

// Signed enums
enum SE1 { N1 = -1 };
enum SE2 { N2 = -2 };
// Unsigned unums
enum UE1 { P1 };
enum UE2 { P2 };

int f1(enum UE1 E) {
  return E; // warning about sign conversion
}

int f2(enum UE1 E) {
  return E; // warning about sign conversion
}

int f3(enum SE1 E) {
  return E; // shouldn't warn
}

int f4(enum SE1 E) {
  return E; // shouldn't warn
}

[aaron.ballman]

Also, before we land this, you should add a release note to clang/docs/ReleaseNotes.rst about the bugfix.

[red1bluelost]

Adds regression tests for enum to int sign conversion warnings and notes the
changes in the release notes.

[aaron.ballman]

clang/docs/ReleaseNotes.rst

113–114

Minor tweak

clang/test/Sema/enum-sign-conversion.c

6–7

Unrelated formatting change?

12–16

The second variants are unused.

35–41

Shouldn't these not warn as well -- the underlying enum type is unsigned?

[red1bluelost]

Addresses comments to improve wording and remove unnecessary code.

clang/docs/ReleaseNotes.rst

113–114

Thanks!

clang/test/Sema/enum-sign-conversion.c

6–7

Changed back.

12–16

Good catch, removed.

35–41

This is the case with 'win32' for the ABI. It has enum be signed as default. I changed the name of the verify to win32 to that it is easier to see that is the case.

[aaron.ballman]

LGTM aside from some minor corrections. Btw, do you need someone to land this on your behalf? If so, what name and email address would you like used for patch attribution?

clang/test/Sema/enum-sign-conversion.c

6–7

Almost, there's still whitespace changes. :-)

11–14

Then use E1 in place of UE1 -- it was the name UE1 plus the comment that kept tripping me up.

35–41

Ah, thanks, based on that, I have some other naming suggestions.

[red1bluelost]

Addresses the last couple comments.

[red1bluelost]

If that's all good, could you commit on my behalf. Here is name and email: Micah Weston ([e-mail])

Thanks for the great reviews! It was really helpful with learning the frontend stuff.

[aaron.ballman]

Thank you for the fix! I've landed on your behalf in 882915df61e33f3a2b7f58e52f572717e1c11499.

clang/docs/ReleaseNotes.rst

- ``-Wliteral-range`` will warn on floating-point equality comparisons with
   ``(float) f == 0.1`` is always false.
 - ``-Winline-namespace-reopened-noninline`` now takes into account that the
   ``inline`` keyword must appear on the original but not necessarily all
   extension definitions of an inline namespace and therefore points its note
   at the original definition. This fixes `Issue 50794 (PR51452)
   <https://github.com/llvm/llvm-project/issues/50794>`_.
 - ``-Wunused-but-set-variable`` now also warns if the variable is only used
   by unary operators.
+- ``-Wenum-conversion`` now warns on converting a signed enum of one type to an
+  unsigned enum of a different type (or vice versa) rather than
+  ``-Wsign-conversion``.
 
 Non-comprehensive list of changes in this release
 -------------------------------------------------
 - Improve __builtin_dump_struct:
   - Support bitfields in struct and union.
   - Improve the dump format, dump both bitwidth(if its a bitfield) and field value.
   - Remove anonymous tag locations.
   - Beautify dump format, add indent for nested struct and struct members.

clang/lib/Sema/SemaChecking.cpp

if (E->EvaluateAsInt(Result, S.Context, Expr::SE_AllowSideEffects) &&
                 << E->getSourceRange() << SourceRange(CC));
         return;
       }
     }
 
     // Fall through for non-constants to give a sign conversion warning.
   }
 
-  if ((TargetRange.NonNegative && !LikelySourceRange.NonNegative) ||
+  if ((!isa<EnumType>(Target) || !isa<EnumType>(Source)) &&
+      ((TargetRange.NonNegative && !LikelySourceRange.NonNegative) ||
        (!TargetRange.NonNegative && LikelySourceRange.NonNegative &&
-       LikelySourceRange.Width == TargetRange.Width)) {
+        LikelySourceRange.Width == TargetRange.Width))) {
     if (S.SourceMgr.isInSystemMacro(CC))
       return;
 
     unsigned DiagID = diag::warn_impcast_integer_sign;
 
     // Traditionally, gcc has warned about this under -Wsign-compare.
     // We also want to warn about it in -Wconversion.
     // So if -Wconversion is off, use a completely identical diagnostic
     // in the sign-compare group.
     // The conditional-checking code will
     if (ICContext) {
       DiagID = diag::warn_impcast_integer_sign_conditional;
       *ICContext = true;
     }
 
     return DiagnoseImpCast(S, E, T, CC, DiagID);
   }
 
   // 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())) {

clang/test/Sema/enum-enum-conversion.c

+// RUN: %clang_cc1 -triple=x86_64-pc-linux-gnu -fsyntax-only -Wenum-conversion -verify %s
+// RUN: %clang_cc1 -triple=x86_64-pc-linux-gnu -fsyntax-only -Wconversion -verify %s
+
+// Signed enums
+enum SE1 { N1 = -1 };
+enum SE2 { N2 = -2 };
+// Unsigned unums
+enum UE1 { P1 };
+enum UE2 { P2 };
+
+enum UE2 f1(enum UE1 E) {
+  return E; // expected-warning {{implicit conversion from enumeration type 'enum UE1' to different enumeration type 'enum UE2'}}
+}
+
+enum SE1 f2(enum UE1 E) {
+  return E; // expected-warning {{implicit conversion from enumeration type 'enum UE1' to different enumeration type 'enum SE1'}}
+}
+
+enum UE1 f3(enum SE1 E) {
+  return E; // expected-warning {{implicit conversion from enumeration type 'enum SE1' to different enumeration type 'enum UE1'}}
+}
+
+enum SE2 f4(enum SE1 E) {
+  return E; // expected-warning {{implicit conversion from enumeration type 'enum SE1' to different enumeration type 'enum SE2'}}
+}

clang/test/Sema/enum-sign-conversion.c

-// RUN: %clang_cc1 -triple=x86_64-pc-linux-gnu -fsyntax-only -verify -DUNSIGNED -Wsign-conversion %s
-// RUN: %clang_cc1 -triple=x86_64-pc-win32 -fsyntax-only -verify -Wsign-conversion %s
+// RUN: %clang_cc1 -triple=x86_64-pc-linux-gnu -fsyntax-only -Wsign-conversion -verify=unsigned,both %s
+// RUN: %clang_cc1 -triple=x86_64-pc-linux-gnu -fsyntax-only -Wconversion -verify=unsigned,both %s
+// RUN: %clang_cc1 -triple=x86_64-pc-win32 -fsyntax-only -verify -Wsign-conversion -verify=win32,both %s
 
 // PR35200
 enum X { A,B,C};
 int f(enum X x) {
-#ifdef UNSIGNED
-  return x; // expected-warning {{implicit conversion changes signedness: 'enum X' to 'int'}}
-#else
-  // expected-no-diagnostics
-  return x;
-#endif
+  return x; // unsigned-warning {{implicit conversion changes signedness: 'enum X' to 'int'}}
+}
+
+enum SE1 { N1 = -1 }; // Always a signed underlying type.
+enum E1 { P1 };       // Unsigned underlying type except on Windows.
+
+// ensure no regression with enum to sign (related to enum-enum-conversion.c)
+int f1(enum E1 E) {
+  return E; // unsigned-warning {{implicit conversion changes signedness: 'enum E1' to 'int'}}
+}
+
+enum E1 f2(int E) {
+  return E; // unsigned-warning {{implicit conversion changes signedness: 'int' to 'enum E1'}}
+}
+
+int f3(enum SE1 E) {
+  return E; // shouldn't warn
+}
+
+enum SE1 f4(int E) {
+  return E; // shouldn't warn
+}
+
+unsigned f5(enum E1 E) {
+  return E; // win32-warning {{implicit conversion changes signedness: 'enum E1' to 'unsigned int'}}
+}
+
+enum E1 f6(unsigned E) {
+  return E; // win32-warning {{implicit conversion changes signedness: 'unsigned int' to 'enum E1'}}
+}
+
+unsigned f7(enum SE1 E) {
+  return E; // both-warning {{implicit conversion changes signedness: 'enum SE1' to 'unsigned int'}}
+}
+
+enum SE1 f8(unsigned E) {
+  return E; // both-warning {{implicit conversion changes signedness: 'unsigned int' to 'enum SE1'}}
 }