Suggestion for warning for C++ non-static functional cast

Clang Frontend
1

The fact that, in C++, T(x) is exactly equivalent to (T)x (and not to T temporary(x);) is, in my experience, not widely known and even experts find it to be “surprising and unpleasant”. People instead think of T(x) as “constructor syntax”. Which it effectively is for any number of arguments other than 1.

Note that this equivalence is true even when x is a pack expansion with a single element. A variadic template that forwards to a constructor must use if constexpr(sizeof...(x) == 1), or a similar explicit handling mechanism, to substitute in a static_cast for that case if it wants to avoid ever invoking a reinterpret_cast or similar. Or it can constrain on std::is_constructible which uses T x(args...);.

Clang currently has no warnings relating to functional casts whatsoever. -Wold-style-cast is purely syntax-based and never triggers on any functional cast, no matter how dubious. -Wcast-qual also does not trigger on functional casts, which probably warrants a separate bug report. (GCC’s equivalent does warn.) [edit: I have made that bug report.]

I propose a warning -Wnon-static-functional-cast which will trigger whenever T(x) is not resolved to static_cast<T>(x) under the explicit conversion rewrite rules in the standard. Any such casts that are desired should be explicitly spelled out with the relevant reinterpret_cast or const_casts, and should not use a syntax that is widely understood to have a completely different meaning. static_cast is semantically equivalent to the direct-initialization that occurs with T(x...) for sizeof...(x) != 1 syntax, and for T y(x);.

In case you were wondering, T{x} is not a cast expression according to the language rules, so it does not suffer from any variant of this problem. Direct-list-initalization has no special case for one argument (except for deduction of auto).

I know that -Weverything is not for production use. I use it in the above to demonstrate that Clang has literally no warnings that diagnose this at all.

3 Likes
2

Thanks for taking the time to write this up, it’s very much appreciated! If I’m understanding the GCC diagnostics correctly, it looks like -Wcast-qual catches this; I would prefer that this be bundled into the same flag on the Clang side for parity.

3

-Wcast-qual only catches the const_cast part of this, not the reinterpret_cast part. My new proposed warning would trigger on it even if the const were added to T’s definition.

1 Like
4

Thanks for raising this! I think we should match GCC’s behavior for -Wcast-qual here (catching the const_cast part but not the rest).

As for -Wnon-static-functional-cast, I’ve got some questions/concerns:

  1. Is there evidence that most uses of a functional cast expression that doesn’t resolve to a static_cast cause problems? Basically, what’s the false positive rate for the diagnostic, where working code is diagnosed?
  2. Related, how chatty is the diagnostic overall? Does it catch 95+% true positives but there’s hundreds of diagnostics in sampled projects?
    (Both of these questions relate to the fact that we typically expect diagnostics to be enabled by default unless there’s a compelling reason to carry an off-by-default diagnostic.)
  3. Do you know if the GCC folks are planning to add something similar?
  4. Is there a reason this is limited to function-style casts and not also including C-style casts as those have the same issue? If you expand the scope to C-style casts, do any of the answers to preceding questions change significantly?
5

We had a user request this a few months back so one of our team members proposed a patch for -Whidden-reinterpret-cast. Unfortunately it wasn’t accepted upstream but I’m just pointing it out for reference as it is very similar to the suggestion here.

6

-Whidden-reinterpret-cast seems to have slightly different semantics. It does not diagnose casting away const and casting to references to the inaccessible base.

7

Re: 4) the point being made in the post was that 95%, a number I made up, of the users, expect T(x) to be a call to constructors (or giving the rv in T rv(x);, depends on how much learners internalize C++) and (T)x to be a C-style cast. C-style cast has nothing to diagnose here, as the behavior matches ordinary users’ expectations.

2 Likes
8

Regarding 1. and 4., the problem is that T(x) visually appears to be part of a semantic and syntactic family that it is not part of: direct-initialization. T(), T(x), T(x, y) are direct-initialization (forwards to value-initialization), functional cast, and direct-initialization. T(args...) is direct-initialization except specifically when provided with one argument. static_cast<T>(x) is not exactly direct-initialization, but the differences are quite minor. T(x) is common enough that it would probably be untenable to remove entirely from most code, but having this warning could serve to ensure that it doesn’t behave unexpectedly.

Re: just 4., (T)x can never appear to be anything other than a “wildcard” C-style cast. It can never appear in a variadic template instantiation unless it appeared exactly that way in the base template. The reason functional casts are uniquely problematic is this syntax collision. I have nothing against a warning for hidden reinterpret_cast in C casts, but I think this should not be that warning because people actually expect to have (T)x be a reinterpret_cast some of the time, but I’ve never seen anyone use T(x) and mean that.

Re: 1. and 2., I do not have hard numbers (given that I have no easy way to test a diagnostic that does not exist in either Clang or GCC) but I expect that the false positive rate is probably pretty low. Most style guides already advocate for using named casts, and even setting that aside, in every case where the “wildcard” casting behavior is intended, the simple solution is to move the parentheses to the left. By definition this will maintain semantics, and it will be regarded by most developers, at least based on those in my orbit and those who write/talk about style and readability I know of, as much more clear about what it is doing. (For the record, the only power that casts have that no named casts have is to cast to private base class reference/pointer. Doing that is questionable, but at least there is theoretically a case where a named cast can’t be used.) As I already said, most people, including C++ experts, simply don’t even know that this is something that happens. In fact I’ve even heard incorrect advice being given about this; people say “Don’t use C casts, they’re dangerous. T(x) is safer.” when in fact it is not safer at all.

Note that any template constrained on std::is_constructible_v<T, Args...> or equivalent will never trigger this proposed warning, because this case is only when T(x) is not equivalent to direct-initialization, but the type trait tests whether T v(x); is well-formed which is always direct-initialization. new (ptr) T(x) is also direct-initialization, and is used in for example the constraints for std::construct_at, which transitively applies to everything using std::allocator.

Re: 3., I intend to raise this same suggestion with GCC if there is not already an active suggestion for it. I don’t currently have an account on their bug tracker or I’d have done it already.

This is just an aside that probably won’t convince anyone, but I generally don’t like -Werror, but I’d definitely turn on -Werror=non-static-functional-cast immediately. If I trigger it, it will be a bug, 100% of the time.

1 Like
9

Which reminds me of https://reviews.llvm.org/D76572 “Replace T(x) with reinterpret_cast<T>(x) everywhere it means reinterpret_cast. No functional change” and the comment I left there at https://reviews.llvm.org/D76572#1945599: "I’ve meanwhile improved LibreOffice’s cstylecast check now […], to also flag function-style casts that should be const_/reinterpret_cast, and it indeed only found a handful of cases that would better be reinterpret_cast " (But I think that LibreOffice plugin diagnoses neither functional casts to inaccessible bases nor functional casts involving parameter pack expansions in template code.)

10

FWIW, I personally agree that using named casts generally makes for better code. However, function-style casts are valid C++ code and if this diagnostic is going to warn on them simply because they don’t resolve to a named cast, this feels more like a style warning. Style warnings are generally off-by-default because they’re too chatty, and we don’t typically add new off-by-default warnings without significant evidence that it’ll be used (we’ve found most of these diagnostics are not enabled and otherwise aren’t worth carrying). Instead, we usually put this kind of warning in clang-tidy.

All that said, this feels kind of on the edge of style vs correctness, as it is possible the user was confused as to what T(x) means. But at the same time, I’ve seen code bases in the past that use T(x) because they want to turn x into a T and it does not matter whether that’s a cast or a constructor call to them, so I worry about false positives.

I think we need some concrete numbers to help us decide whether this is an opinionated diagnostic or whether this is a reasonable one to have in the frontend. It’d really help to try the proposed functionality out on a large corpus of mixed-age C++ code (like building the packages from a distro). If it turns out this diagnostic is not particularly chatty or opinionated, that would definitely help me feel more comfortable with it being on-by-default.

Another thing that would help is to have some real-world-ish examples where the cast is actively harmful to more than readability of the code.

11

I think that the pack expansion aspect of this warning is clearly more of a correctness issue than a style one. T(args...) is, I think, clearly not expected to resolve to reinterpret_cast<T>(args) in the single-argument case. Any code which intends to check constructibility by constraining on T(args...) is potentially incorrect in a subtle way. The standard library defends against this by constraining on new T(args) but that is not the immediately obvious spelling to most people and requires a separate check for destructibility in most cases.

FWIW, I personally agree that using named casts generally makes for better code. However, function-style casts are valid C++ code and if this diagnostic is going to warn on them simply because they don’t resolve to a named cast, this feels more like a style warning.

What I was trying to say was that the alternative is not solely T(x) vs. a named cast, (T)x is another alternative, with much wider recognition and the exact same semantics if those semantics are intended. However many people know that T(x) can be a reinterpret_cast, I am confident that a much larger number know that about (T)x. And, of course, (T)x allows non-simple type IDs such as pointers and unsigned long as well. However I’m not sure what you mean by “simply because they don’t resolve to a named cast” because by definition they all resolve to named casts. That’s what [expr.type.conv], by way of [expr.cast], does. (Except for the relaxed rules for static_cast to inaccessible bases, but that probably deserves a warning anyway.)

Potentially this could have a ‘strictness’ level like some other warnings, where level 1 diagnoses only dependent and variadic cases, and level 2 diagnoses any non-static_cast-equivalent cast. Level 1 could be enabled by default and level 2 perhaps included in some relevant warning group or in -Wall/-Wextra. These could also be separate warnings entirely, but I think a strictness level would make sense there.

I would love to have hard numbers too, but I can’t provide them because it seems this check is not implemented anywhere. I have just checked and the the clang-tidy check cppcoreguidelines-pro-type-cstyle-cast is implemented as documented (only flagging C-style casts) and not as described by the core guidelines (flagging functional casts in the same situations). This seems to be the result of a merging of certain guidelines without ever updating the check. I do not have familiarity with either codebase so writing a custom fork with the warning I have described is beyond my means at the moment.

12

I think I agree with this, and that suggests to me one way to make this an on-by-default warning is to limit it to just pack expansion scenarios, and not others. The pack expansion situation is murky enough to warrant a “are you really sure you intended this?” kind of diagnostic.

Sure, and I understood that (though I maybe didn’t make that clear in my response). My point was mostly just that we don’t typically issue warnings for “you’re using a language feature” kind of situations. We did it with -Wvla because of the security concerns around memory accesses, but in general we avoid this sort of thing. This diagnostic, at least in its general form, is basically “you’re using a function-style cast, did you mean to do that?” which is a “you’re using a language feature”-style warning.

I was speaking to this from the original message:

I propose a warning -Wnon-static-functional-cast which will trigger whenever T(x) is not resolved to static_cast<T>(x) under the explicit conversion rewrite rules in the standard.

I think there was some confusion on my side – I thought you were proposing to add this diagnostic by doing the implementation work yourself. So my suggestion was basically to do that implementation in your own fork, run some experiments that way, and then come back with those numbers. But if you’re not proposing to add this diagnostic yourself (if it’s basically a feature request), that’s a different thing. In that case, I’d recommend filing an issue in the issue tracker suggesting an enhancement along the lines of what you’d like to see, and be sure to link back to this discussion.

13

I would love to have hard numbers too, but I can’t provide them because it seems this check is not implemented anywhere. […] I do not have familiarity with either codebase so writing a custom fork with the warning I have described is beyond my means at the moment.

I strongly recommend you try doing it, especially when the hidden-reinterpret-cast patch showed you where and how to adjust, including tests. You will find it to be intuitive. After you proposed the patch, I’m sure that we can find someone to run a patched Clang on several large code bases, such as Google’s, and report back here.

1 Like