[clang-tidy] don't warn about implicit widening casts in function calls

[danielmarjamaki]

Summary

This patch fixes https://bugs.llvm.org/show_bug.cgi?id=32246

To avoid spurious warnings, clang-tidy should not warn about misplaced widening casts for implicit casts in function calls.

[danielmarjamaki]

In my opinion, we should stop warning about all implicit casts.

Take for instance:

long l1;
if (condition)
  l1 = n << 8;  // <- implicit cast
else
  l1 = ~0L;

That is fine. Nothing suspicious. Just because the destination variable is long doesn't have to mean the result is long. If we want to warn I would say that valueflow analysis should be used to see if there is truncation.

The original idea was that we would warn if the user tried to cast the result but did that wrong. I don't feel that this is the idea of this checker anymore.

[xazax.hun]

I wonder whether warning on implicit casts still makes sense for example in mission critical code. So maybe it is worth to have a configuration option with the default setting being less strict and chatty. What do you think?

[alexfh]

In D31097#704621, @xazax.hun wrote:

I wonder whether warning on implicit casts still makes sense for example in mission critical code. So maybe it is worth to have a configuration option with the default setting being less strict and chatty. What do you think?

But it's not about "misplaced casts", it's about implicit conversions and -Wconversion diagnostic can take care of this.

[xazax.hun]

In D31097#704626, @alexfh wrote:

In D31097#704621, @xazax.hun wrote:

I wonder whether warning on implicit casts still makes sense for example in mission critical code. So maybe it is worth to have a configuration option with the default setting being less strict and chatty. What do you think?

But it's not about "misplaced casts", it's about implicit conversions and -Wconversion diagnostic can take care of this.

Good point, I'm convinced now.

[alexfh]

In D31097#704626, @alexfh wrote:

In D31097#704621, @xazax.hun wrote:

I wonder whether warning on implicit casts still makes sense for example in mission critical code. So maybe it is worth to have a configuration option with the default setting being less strict and chatty. What do you think?

But it's not about "misplaced casts", it's about implicit conversions and -Wconversion diagnostic can take care of this.

Actually, the diagnostics about implicit casts here might be useful (but maybe in a separate check). I have to look again at https://reviews.llvm.org/D17987.

[danielmarjamaki]

In D31097#704628, @alexfh wrote:

In D31097#704626, @alexfh wrote:

In D31097#704621, @xazax.hun wrote:

I wonder whether warning on implicit casts still makes sense for example in mission critical code. So maybe it is worth to have a configuration option with the default setting being less strict and chatty. What do you think?

But it's not about "misplaced casts", it's about implicit conversions and -Wconversion diagnostic can take care of this.

Actually, the diagnostics about implicit casts here might be useful (but maybe in a separate check). I have to look again at https://reviews.llvm.org/D17987.

there can definitely be bugs when there are such implicit casts.

but this checking has no precision at all. I am against that we don't care about precision.

adding casts to silence such warnings are dangerous too. I have seen for instance in Clang repo when there is "loss of sign" warning and the developer fix that by casting for instance a "size_t" to "int" and then there is logically loss of precision.

In D31097#704626, @alexfh wrote:

In D31097#704621, @xazax.hun wrote:

I wonder whether warning on implicit casts still makes sense for example in mission critical code. So maybe it is worth to have a configuration option with the default setting being less strict and chatty. What do you think?

But it's not about "misplaced casts", it's about implicit conversions and -Wconversion diagnostic can take care of this.

I agree..

Just want to advertise the analyzer ConversionChecker also in case you didn't know about it. That is supposed to be a precise checker for loss of precision and loss of sign. It does not detect this loss of precision in implicit casts but I would like that is taken care of.

[danielmarjamaki]

Remove warnings for implicit casts.

[danielmarjamaki]

I believe there is pointless code in relativeIntSizes etc. If there is for instance "a+b" then the result can't be a char type.

static int relativeIntSizes(BuiltinType::Kind Kind) {
  switch (Kind) {
  case BuiltinType::UChar:
    return 1;
  case BuiltinType::SChar:
    return 1;
  case BuiltinType::Char_U:
    return 1;
  case BuiltinType::Char_S:
    return 1;
  case BuiltinType::UShort:
    return 2;
  case BuiltinType::Short:
    return 2;
  case BuiltinType::UInt:
    return 3;
  case BuiltinType::Int:
    return 3;
  case BuiltinType::ULong:
    return 4;
  case BuiltinType::Long:
    return 4;
  case BuiltinType::ULongLong:
    return 5;
  case BuiltinType::LongLong:
    return 5;
  case BuiltinType::UInt128:
    return 6;
  case BuiltinType::Int128:
    return 6;
  default:
    return 0;
  }
}

[baloghadamsoftware]

Hi!

There is an option to disable the checking of widening casts. It is enabled by default. You can disable it any time. Or, if you find too much false positives, we can discuss about setting this option to disabled as default.

I am convinced that checking implicit widening casts are also necessary. We should probably change the error message in the implicit case from "misplaced" to "missing", and maybe also rename the checker itself. Separating it to two different checkers, which are almost copy of each other is huge code duplication.

[danielmarjamaki]

In D31097#707385, @baloghadamsoftware wrote:

Hi!

There is an option to disable the checking of widening casts. It is enabled by default. You can disable it any time. Or, if you find too much false positives, we can discuss about setting this option to disabled as default.

I am convinced that checking implicit widening casts are also necessary. We should probably change the error message in the implicit case from "misplaced" to "missing", and maybe also rename the checker itself. Separating it to two different checkers, which are almost copy of each other is huge code duplication.

It would help to disable it by default and changing the message. But also I believe it's philosophically different to the original checker.

I would say that your logic is more philosophically similar to Wconversion. Could it be added there instead?

Did you try this check on real code? Do you think there is a problem that should be fixed here?

void foo(unsigned int N) {
  long L;
  if (N<10)
    L = N << 8;
  ...

I am assuming that such code is not uncommon. If you don't think there is a problem in that code then I would personally suggest that you update the ConversionChecker in the analyzer instead.

I do believe that a warning about that code is a false positive.

The idea with the misc-misplaced-widening-cast is that if the developer writes such code:

void foo(unsigned int N) {
  long L;
  if (N<10)
    L = (long)(N << 8);
  ...

Then there is a message "either cast is misplaced or there is truncation". In this case the cast is misplaced and it can be removed.

[danielmarjamaki]

Well.. feel free to provide an alternative fix. If the message is more specific and it must be enabled explicitly by an option then maybe it's good enough for me.

[danielmarjamaki]

I would recommend that this is either fixed soon or that we commit my changes so it can be implemented more properly later. Right now users will see false positives.

[baloghadamsoftware]

OK, please create a patch where implicit casts are disabled by default. You can also find a new name for the checker.

[baloghadamsoftware]

Or I can do it for you if you wish.

[danielmarjamaki]

Or I can do it for you if you wish.

yes please

[danielmarjamaki]

I believe https://reviews.llvm.org/D32164 is better

clang-tidy/misc/MisplacedWideningCastCheck.cpp

const auto ExplicitCast = explicitCastExpr(hasDestinationType(isInteger()),
                                              has(ignoringParenImpCasts(Calc)));
   const auto ImplicitCast =
       implicitCastExpr(hasImplicitDestinationType(isInteger()),
                        has(ignoringParenImpCasts(Calc)));
   const auto Cast = expr(anyOf(ExplicitCast, ImplicitCast)).bind("Cast");
 
   Finder->addMatcher(varDecl(hasInitializer(Cast)), this);
   Finder->addMatcher(returnStmt(hasReturnValue(Cast)), this);
-  Finder->addMatcher(callExpr(hasAnyArgument(Cast)), this);
+  Finder->addMatcher(callExpr(hasAnyArgument(ExplicitCast.bind("Cast"))), this);
   Finder->addMatcher(binaryOperator(hasOperatorName("="), hasRHS(Cast)), this);
   Finder->addMatcher(
       binaryOperator(matchers::isComparisonOperator(), hasEitherOperand(Cast)),
       this);
 }
 
 static unsigned getMaxCalculationWidth(const ASTContext &Context,
                                        const Expr *E) {

test/clang-tidy/misc-misplaced-widening-cast.cpp

void init(unsigned int n) {
   long l1 = n << 8;
   // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: either cast from 'unsigned int' to 'long'
   long l2 = (long)(n << 8);
   // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: either cast from 'unsigned int' to 'long'
   long l3 = (long)n << 8;
 }
 
 void call(unsigned int n) {
+  // Don't warn about implicit casts. See https://bugs.llvm.org/show_bug.cgi?id=32246
   func(n << 8);
-  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: either cast from 'unsigned int' to 'long'
   func((long)(n << 8));
   // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: either cast from 'unsigned int' to 'long'
   func((long)n << 8);
 }
 
 long ret(int a) {
   if (a < 0) {
     return a * 1000;