[clang] trigger -Wcast-qual on functional casts

[sousajo]

Summary

-Wcast-qual does not trigger on the following code in Clang, but does in GCC.

const auto i = 42;
using T = int*;
auto p = T(&i);

The expected behavior is that a functional cast should trigger
the warning the same as the equivalent C cast because
the meaning is the same, and nothing about the functional cast
makes it easier to recognize that a const_cast is occurring.

Fixes https://github.com/llvm/llvm-project/issues/62083

[shafik]

Thank you for putting this PR, I was actually going to do this myself but I probably might not get to it soon.

clang/test/SemaCXX/warn-cast-qual.cpp

218

I would actually like to see each test above replicated for the functional cast. I realize this is a lot of work but we want to be sure that the diagnostic is really doing the equivalent check for both functional and non-functional cast.

I am not sure if we just do them right below each non-functional case of split out the functional cast cases into their own functions.

@aaron.ballman please let me know if you disagree here.

[sousajo]

Add more tests

[sousajo]

clang/test/SemaCXX/warn-cast-qual.cpp

218

I replicated them on each of them functions

[sousajo]

• get rid of whitespace unrelated changes

[sousajo]

@shafik does it look better now?

[shafik]

LGTM

[sousajo]

can someone land it for me pls? Jorge Pinto Sousa <[e-mail]>

[sousajo]

kind ping ^^

[aaron.ballman]

In D148276#4278138, @sousajo wrote:

can someone land it for me pls? Jorge Pinto Sousa <[e-mail]>

I've landed on your behalf, and I took the liberty to add a release note when landing it.

[thurston]

I think this change is triggering an error on a buildbot (https://lab.llvm.org/buildbot/#/builders/37/builds/21631/steps/20/logs/stdio):

/b/sanitizer-x86_64-linux/build/llvm-project/clang/lib/Analysis/UnsafeBufferUsage.cpp:1348:45: error: cast from 'const clang::ASTContext' to 'clang::ASTContext &' drops const qualifier [-Werror,-Wcast-qual]
          std::remove_const_t<ASTContext &>(Ctx),

even though the code is deliberately trying to remove the const qualifier the right (?) way.

[aaron.ballman]

In D148276#4287900, @thurston wrote:

I think this change is triggering an error on a buildbot (https://lab.llvm.org/buildbot/#/builders/37/builds/21631/steps/20/logs/stdio):

/b/sanitizer-x86_64-linux/build/llvm-project/clang/lib/Analysis/UnsafeBufferUsage.cpp:1348:45: error: cast from 'const clang::ASTContext' to 'clang::ASTContext &' drops const qualifier [-Werror,-Wcast-qual]
          std::remove_const_t<ASTContext &>(Ctx),

even though the code is deliberately trying to remove the const qualifier the right (?) way.

Ouch, that's pretty sad. I'll work around it and land a fix shortly, thank you for pointing this out!

[aaron.ballman]

In D148276#4287931, @aaron.ballman wrote:

In D148276#4287900, @thurston wrote:

I think this change is triggering an error on a buildbot (https://lab.llvm.org/buildbot/#/builders/37/builds/21631/steps/20/logs/stdio):

/b/sanitizer-x86_64-linux/build/llvm-project/clang/lib/Analysis/UnsafeBufferUsage.cpp:1348:45: error: cast from 'const clang::ASTContext' to 'clang::ASTContext &' drops const qualifier [-Werror,-Wcast-qual]
          std::remove_const_t<ASTContext &>(Ctx),

even though the code is deliberately trying to remove the const qualifier the right (?) way.

Ouch, that's pretty sad. I'll work around it and land a fix shortly, thank you for pointing this out!

Actually, that's going to be more involved than a quick fix. I think I'll revert the patch and reopen this review so that @sousajo can investigate the proper fix.

[aaron.ballman]

Reverted in 7f00ecd029213aee9baf297554b0cb25a5ffbd0f

[sousajo]

hey :( I will try to investigate it a bit sometime next week ^^ thanks for pointing it out

[aaron.ballman]

In D148276#4288145, @sousajo wrote:

hey :( I will try to investigate it a bit sometime next week ^^ thanks for pointing it out

Thank you for looking into it! I didn't think about remove_const, remove_cv, etc. when considering test cases, sorry about that!

[sousajo]

In D148276#4288236, @aaron.ballman wrote:

In D148276#4288145, @sousajo wrote:

hey :( I will try to investigate it a bit sometime next week ^^ thanks for pointing it out

Thank you for looking into it! I didn't think about remove_const, remove_cv, etc. when considering test cases, sorry about that!

No worries :) Ill add those and try to craft a fix. If I am really lost I also let you know ^^

[thurston]

Thanks Aaron and Jorge for the quick revert and replies! :-)

[sousajo]

have been sick, and could not advance much except I added the tests to replicate the issue. Any ideas on how to proceed here?

[aaron.ballman]

In D148276#4324314, @sousajo wrote:

have been sick, and could not advance much except I added the tests to replicate the issue. Any ideas on how to proceed here?

Sorry to hear you've been sick, but thank you for your patience while I thought about this further. Two points worth observing:

1. GCC diagnoses the problematic code the same way your patch does: https://godbolt.org/z/44GnM9jzE
2. Clang and GCC both diagnose the notionally equivalent code using a C-style cast: https://godbolt.org/z/bbTPYb5rE

Based on that, it seems defensible for us to diagnose that code by re-landing the patch. However, I wonder why the changes broke anything. We build with GCC all the time. Is -Wcast-qual only enabled for bots building with Clang?

However, I also feel like this behavior is somewhat user-hostile because it seems reasonable to expect the developer is well aware that they're casting away the const qualifier when using something named remove_const_t to specify the cast destination type, so it seems potentially reasonable to silence the diagnostic in those cases (consistently, for both function- and c-style casts). It seems to be a reasonably popular approach to casting away const in the wild: https://sourcegraph.com/search?q=context:global+%5Cb%5Bstd::%5D%3Fremove_const%5B_t%5D%3F%5C%3C.*%5C%3E%5B::type%5D%3F%5C%28.*%5C%29&patternType=regexp&sm=1&groupBy=group so this might allow more folks to opt into -Wcast-qual. But implementing that would be tricky because we'd have to look through the cast expression to see whether it came from a type trait, and we typically do not want the frontend to be inspecting AST nodes by name (like looking for particular identifiers and changing behavior based on that), and we'd be introducing another divergence between the Clang and GCC behaviors.

clang/docs/ReleaseNotes.rst

- Clang now avoids unnecessary diagnostic warnings for obvious expressions in
   the case of binary operators with logical OR operations.
   (`#57906 <https://github.com/llvm/llvm-project/issues/57906>`_)
 - Clang's "static assertion failed" diagnostic now points to the static assertion
   expression instead of pointing to the ``static_assert`` token.
   (`#61951 <https://github.com/llvm/llvm-project/issues/61951>`_)
 - ``-Wformat`` now recognizes ``%lb`` for the ``printf``/``scanf`` family of
   functions.
   (`#62247: <https://github.com/llvm/llvm-project/issues/62247>`_).
+- ``-Wcast-qual`` now triggers on function-style casts.
+  (`#62083 <https://github.com/llvm/llvm-project/issues/62083>`_)
 
 Bug Fixes in This Version
 -------------------------
 
 - Fix crash when diagnosing incorrect usage of ``_Nullable`` involving alias
   templates.
   (`#60344 <https://github.com/llvm/llvm-project/issues/60344>`_)
 - Fix confusing warning message when ``/clang:-x`` is passed in ``clang-cl``

clang/lib/Sema/SemaCast.cpp

if (Op.SrcExpr.isInvalid())
     return ExprError();
 
   auto *SubExpr = Op.SrcExpr.get();
   if (auto *BindExpr = dyn_cast<CXXBindTemporaryExpr>(SubExpr))
     SubExpr = BindExpr->getSubExpr();
   if (auto *ConstructExpr = dyn_cast<CXXConstructExpr>(SubExpr))
     ConstructExpr->setParenOrBraceRange(SourceRange(LPLoc, RPLoc));
 
+  // -Wcast-qual
+  DiagnoseCastQual(Op.Self, Op.SrcExpr, Op.DestType);
+
   return Op.complete(CXXFunctionalCastExpr::Create(
       Context, Op.ResultType, Op.ValueKind, CastTypeInfo, Op.Kind,
       Op.SrcExpr.get(), &Op.BasePath, CurFPFeatureOverrides(), LPLoc, RPLoc));
 }

clang/test/SemaCXX/warn-cast-qual.cpp

void foo_0() {
 
   int &a2 = (int &)a;                      // expected-warning {{cast from 'const int' to 'int &' drops const qualifier}}
   const int &a3 = (int &)a;                // expected-warning {{cast from 'const int' to 'int &' drops const qualifier}}
   int &a4 = (int &)((const int &)a);       // expected-warning {{cast from 'const int' to 'int &' drops const qualifier}}
   int &a5 = (int &)((int &)a);             // expected-warning {{cast from 'const int' to 'int &' drops const qualifier}}
   const int &a6 = (int &)((int &)a);       // expected-warning {{cast from 'const int' to 'int &' drops const qualifier}}
   const int &a7 = (int &)((const int &)a); // expected-warning {{cast from 'const int' to 'int &' drops const qualifier}}
   const int &a8 = (const int &)((int &)a); // expected-warning {{cast from 'const int' to 'int &' drops const qualifier}}
+
+  using T = int&;
+  using T2 = const int&;
+  const int &a11 =T2(a);      // no warning
+  int a22 = T(a);             // expected-warning {{cast from 'const int' to 'int &' drops const qualifier}}
+  const int &a33 = T(a);      // expected-warning {{cast from 'const int' to 'int &' drops const qualifier}}
+  int &a44 = T(T2(a));        // expected-warning {{cast from 'const int' to 'int &' drops const qualifier}}
+  int &a55 = T(T(a));         // expected-warning {{cast from 'const int' to 'int &' drops const qualifier}}
+  const int &a66 = T(T(a));   // expected-warning {{cast from 'const int' to 'int &' drops const qualifier}}
+  int &a77 = T(T2(a));        // expected-warning {{cast from 'const int' to 'int &' drops const qualifier}}
+  const int &a88 = T2(T(a));  // expected-warning {{cast from 'const int' to 'int &' drops const qualifier}}
 }
 
 void foo_1() {
   volatile int a = 0;
 
   volatile int &a0 = a;                 // no warning
   volatile int &a1 = (volatile int &)a; // no warning
 
   int &a2 = (int &)a;                            // expected-warning {{cast from 'volatile int' to 'int &' drops volatile qualifier}}
   volatile int &a3 = (int &)a;                   // expected-warning {{cast from 'volatile int' to 'int &' drops volatile qualifier}}
   int &a4 = (int &)((volatile int &)a);          // expected-warning {{cast from 'volatile int' to 'int &' drops volatile qualifier}}
   int &a5 = (int &)((int &)a);                   // expected-warning {{cast from 'volatile int' to 'int &' drops volatile qualifier}}
   volatile int &a6 = (int &)((int &)a);          // expected-warning {{cast from 'volatile int' to 'int &' drops volatile qualifier}}
   volatile int &a7 = (int &)((volatile int &)a); // expected-warning {{cast from 'volatile int' to 'int &' drops volatile qualifier}}
   volatile int &a8 = (volatile int &)((int &)a); // expected-warning {{cast from 'volatile int' to 'int &' drops volatile qualifier}}
+
+  using T = int&;
+  using T2 = volatile int&;
+  volatile int &a11 =T2(a);     // no warning
+  int a22 = T(a);               // expected-warning {{cast from 'volatile int' to 'int &' drops volatile qualifier}}
+  volatile int &a33 = T(a);     // expected-warning {{cast from 'volatile int' to 'int &' drops volatile qualifier}}
+  int &a44 = T(T2(a));          // expected-warning {{cast from 'volatile int' to 'int &' drops volatile qualifier}}
+  int &a55 = T(T(a));           // expected-warning {{cast from 'volatile int' to 'int &' drops volatile qualifier}}
+  volatile int &a66 = T(T(a));  // expected-warning {{cast from 'volatile int' to 'int &' drops volatile qualifier}}
+  int &a77 = T(T2(a));          // expected-warning {{cast from 'volatile int' to 'int &' drops volatile qualifier}}
+  volatile int &a88 = T2(T(a)); // expected-warning {{cast from 'volatile int' to 'int &' drops volatile qualifier}}
 }
 
 void foo_2() {
   const volatile int a = 0;
 
   const volatile int &a0 = a;                       // no warning
   const volatile int &a1 = (const volatile int &)a; // no warning
 
   int &a2 = (int &)a;                                        // expected-warning {{cast from 'const volatile int' to 'int &' drops const and volatile qualifiers}}
   const volatile int &a3 = (int &)a;                         // expected-warning {{cast from 'const volatile int' to 'int &' drops const and volatile qualifiers}}
   int &a4 = (int &)((const volatile int &)a);                // expected-warning {{cast from 'const volatile int' to 'int &' drops const and volatile qualifiers}}
   int &a5 = (int &)((int &)a);                               // expected-warning {{cast from 'const volatile int' to 'int &' drops const and volatile qualifiers}}
   const volatile int &a6 = (int &)((int &)a);                // expected-warning {{cast from 'const volatile int' to 'int &' drops const and volatile qualifiers}}
   const volatile int &a7 = (int &)((const volatile int &)a); // expected-warning {{cast from 'const volatile int' to 'int &' drops const and volatile qualifiers}}
   const volatile int &a8 = (const volatile int &)((int &)a); // expected-warning {{cast from 'const volatile int' to 'int &' drops const and volatile qualifiers}}
+
+  using T = int&;
+  using T2 = const volatile int&;
+  const volatile int &a11 =T2(a);           // no warning
+  int a22 = T(a);                     // expected-warning {{cast from 'const volatile int' to 'int &' drops const and volatile qualifiers}}
+  const volatile int &a33 = T(a);     // expected-warning {{cast from 'const volatile int' to 'int &' drops const and volatile qualifiers}}
+  int &a44 = T(T2(a));                // expected-warning {{cast from 'const volatile int' to 'int &' drops const and volatile qualifiers}}
+  int &a55 = T(T(a));                 // expected-warning {{cast from 'const volatile int' to 'int &' drops const and volatile qualifiers}}
+  const volatile int &a66 = T(T(a));  // expected-warning {{cast from 'const volatile int' to 'int &' drops const and volatile qualifiers}}
+  const volatile int &a77 = T(T2(a)); // expected-warning {{cast from 'const volatile int' to 'int &' drops const and volatile qualifiers}}
+  const volatile int &a88 = T2(T(a)); // expected-warning {{cast from 'const volatile int' to 'int &' drops const and volatile qualifiers}}
 }
 
 void bar_0() {
   const int *_a = 0;
   const int **a = &_a;
 
   int **a0 = (int **)((const int **)a); // expected-warning {{cast from 'const int *' to 'int *' drops const qualifier}}
   int **a1 = (int **)((int **)a);       // expected-warning {{cast from 'const int *' to 'int *' drops const qualifier}}
 
   // const int **a2 = (int **)((int **)a);
   // const int **a3 = (int **)((const int **)a);
 
   const int **a4 = (const int **)((int **)a);        // expected-warning {{cast from 'const int *' to 'int *' drops const qualifier}} expected-warning {{cast from 'int **' to 'const int **' must have all intermediate pointers const qualified to be safe}}
   const int **a5 = (const int **)((const int **)a); // no warning
+
+  using T = int**;
+  using T2 = const int**;
+
+  int **a00 = T(T2(a)) ;          // expected-warning {{cast from 'const int *' to 'int *' drops const qualifier}}
+  int **a11 = T(T(a));            // expected-warning {{cast from 'const int *' to 'int *' drops const qualifier}}
+
+  const int **a44 = T2(T(a));     // expected-warning {{cast from 'const int *' to 'int *' drops const qualifier}} expected-warning {{cast from 'T' (aka 'int **') to 'T2' (aka 'const int **') must have all intermediate pointers const qualified to be safe}}
+  const int **a55 = T2(T2(a));    // no warning
+
 }
 
 void bar_1() {
   const int *_a = 0;
   const int *&a = _a;
 
   int *&a0 = (int *&)((const int *&)a); // expected-warning {{cast from 'const int *' to 'int *' drops const qualifier}}
   int *&a1 = (int *&)((int *&)a);       // expected-warning {{cast from 'const int *' to 'int *' drops const qualifier}}
 
   // const int *&a2 = (int *&)((int *&)a);
   // const int *&a3 = (int *&)((const int *&)a);
 
   const int *&a4 = (const int *&)((int *&)a);        // expected-warning {{cast from 'const int *' to 'int *' drops const qualifier}} expected-warning {{cast from 'int *' to 'const int *&' must have all intermediate pointers const qualified to be safe}}
   const int *&a5 = (const int *&)((const int *&)a); // no warning
+
+  using T = int*&;
+  using T2 = const int*&;
+
+  int *&a00 = T(T2(a));           // expected-warning {{cast from 'const int *' to 'int *' drops const qualifier}}
+  int *&a11 = T(T(a));            // expected-warning {{cast from 'const int *' to 'int *' drops const qualifier}}
+
+  const int *&a44 = T2(T(a));     // expected-warning {{cast from 'const int *' to 'int *' drops const qualifier}} expected-warning {{cast from 'int *' to 'T2' (aka 'const int *&') must have all intermediate pointers const qualified to be safe}}
+  const int *&a55 = T2(T2(a));    // no warning
 }
 
 void baz_0() {
   struct C {
     void A() {}
     void B() const {}
   };
 
   const C S;
   S.B();
 
   ((C &)S).B(); // expected-warning {{cast from 'const C' to 'C &' drops const qualifier}}
   ((C &)S).A(); // expected-warning {{cast from 'const C' to 'C &' drops const qualifier}}
 
   ((C *)&S)->B(); // expected-warning {{cast from 'const C *' to 'C *' drops const qualifier}}
   ((C *)&S)->A(); // expected-warning {{cast from 'const C *' to 'C *' drops const qualifier}}
+
+  using T = C&;
+  using T2 = C*;
+  T(S).B(); // expected-warning {{cast from 'const C' to 'C &' drops const qualifier}}
+  T(S).A(); // expected-warning {{cast from 'const C' to 'C &' drops const qualifier}}
+
+  T2(&S)->B(); // expected-warning {{cast from 'const C *' to 'C *' drops const qualifier}}
+  T2(&S)->A(); // expected-warning {{cast from 'const C *' to 'C *' drops const qualifier}}
 }
 
 void baz_1() {
   struct C {
     const int a;
     int b;
 
     C() : a(0) {}
   };
 
   {
     C S;
     S.b = 0;
 
     (int &)(S.a) = 0; // expected-warning {{cast from 'const int' to 'int &' drops const qualifier}}
     (int &)(S.b) = 0; // no warning
 
     *(int *)(&S.a) = 0; // expected-warning {{cast from 'const int *' to 'int *' drops const qualifier}}
     *(int *)(&S.b) = 0; // no warning
+
+    using T = int&;
+    using T2 = int*;
+    T(S.a) = 0; // expected-warning {{cast from 'const int' to 'int &' drops const qualifier}}
+    T(S.b) = 0; // no warning
+
+    *T2(&S.a) = 0; // expected-warning {{cast from 'const int *' to 'int *' drops const qualifier}}
+    *T2(&S.b) = 0; // no warning
   }
   {
     const C S;
 
     (int &)(S.a) = 0; // expected-warning {{cast from 'const int' to 'int &' drops const qualifier}}
     (int &)(S.b) = 0; // expected-warning {{cast from 'const int' to 'int &' drops const qualifier}}
 
     *(int *)(&S.a) = 0; // expected-warning {{cast from 'const int *' to 'int *' drops const qualifier}}
     *(int *)(&S.b) = 0; // expected-warning {{cast from 'const int *' to 'int *' drops const qualifier}}
-  }
+
+    using T = int&;
+    using T2 = int*;
+    T(S.a) = 0; // expected-warning {{cast from 'const int' to 'int &' drops const qualifier}}
+    T(S.b) = 0; // expected-warning {{cast from 'const int' to 'int &' drops const qualifier}}
+
+    *T2(&S.a) = 0; // expected-warning {{cast from 'const int *' to 'int *' drops const qualifier}}
+    *T2(&S.b) = 0; // expected-warning {{cast from 'const int *' to 'int *' drops const qualifier}}
+  }
+
+void qux_0() {
+  const auto i = 42;
+  using T = int*;
+  auto p = T(&i); // expected-warning {{cast from 'const int *' to 'int *' drops const qualifier}}
+
+  const auto i2 = 42;
+  using T2 = const int*;
+  auto p2 = T2(&i2);
+
+  volatile auto i3 = 42;
+  using T3= int*;
+  auto p3 = T3(&i3); // expected-warning {{cast from 'volatile int *' to 'int *' drops volatile qualifier}}
+
+  volatile auto i4 = 42;
+  using T4 = volatile int*;
+  auto p4 = T4(&i4);
+
+  const volatile auto i5 = 42;
+  using T5= int*;
+  auto p5 = T5(&i5);  // expected-warning {{cast from 'const volatile int *' to 'int *' drops const and volatile qualifiers}}
+
+  const volatile auto i6 = 42;
+  using T6= const volatile int*;
+  auto p6 = T6(&i6);
 }