[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.