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

 namespace tidy {
 namespace misc {
 
 /// Find casts of calculation results to bigger type. Typically from int to
 /// long. If the intention of the cast is to avoid loss of precision then
 /// the cast is misplaced, and there can be loss of precision. Otherwise
 /// such cast is ineffective.
 ///
-/// There is one option:
-///
-///   - `CheckImplicitCasts`: Whether to check implicit casts as well which may
-//      be the most common case. Enabled by default.
-///
 /// For the user-facing documentation see:
 /// http://clang.llvm.org/extra/clang-tidy/checks/misc-misplaced-widening-cast.html
 class MisplacedWideningCastCheck : public ClangTidyCheck {
 public:
   MisplacedWideningCastCheck(StringRef Name, ClangTidyContext *Context);
-  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
-
-private:
-  const bool CheckImplicitCasts;
 };
 
 } // namespace misc
 } // namespace tidy
 } // namespace clang
 
 #endif

clang-tidy/misc/MisplacedWideningCastCheck.cpp

 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace misc {
 
 MisplacedWideningCastCheck::MisplacedWideningCastCheck(
     StringRef Name, ClangTidyContext *Context)
-    : ClangTidyCheck(Name, Context),
-      CheckImplicitCasts(Options.get("CheckImplicitCasts", true)) {}
-
-void MisplacedWideningCastCheck::storeOptions(
-    ClangTidyOptions::OptionMap &Opts) {
-  Options.store(Opts, "CheckImplicitCasts", CheckImplicitCasts);
-}
+    : ClangTidyCheck(Name, Context) {}
 
 void MisplacedWideningCastCheck::registerMatchers(MatchFinder *Finder) {
   const auto Calc =
       expr(anyOf(binaryOperator(
                      anyOf(hasOperatorName("+"), hasOperatorName("-"),
                            hasOperatorName("*"), hasOperatorName("<<"))),
                  unaryOperator(hasOperatorName("~"))),
            hasType(isInteger()))
           .bind("Calc");
 
-  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");
+  const auto Cast = explicitCastExpr(hasDestinationType(isInteger()),
+                                     has(ignoringParenImpCasts(Calc)))
+                        .bind("Cast");
 
   Finder->addMatcher(varDecl(hasInitializer(Cast)), this);
   Finder->addMatcher(returnStmt(hasReturnValue(Cast)), this);
   Finder->addMatcher(callExpr(hasAnyArgument(Cast)), this);
   Finder->addMatcher(binaryOperator(hasOperatorName("="), hasRHS(Cast)), this);
   Finder->addMatcher(
       binaryOperator(matchers::isComparisonOperator(), hasEitherOperand(Cast)),
       this);
static bool isFirstWider(BuiltinType::Kind First, BuiltinType::Kind Second) {
   if ((FirstSize = relativeCharSizesW(First)) != 0 &&
       (SecondSize = relativeCharSizesW(Second)) != 0)
     return FirstSize > SecondSize;
   return false;
 }
 
 void MisplacedWideningCastCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *Cast = Result.Nodes.getNodeAs<CastExpr>("Cast");
-  if (!CheckImplicitCasts && isa<ImplicitCastExpr>(Cast))
-    return;
   if (Cast->getLocStart().isMacroID())
     return;
 
   const auto *Calc = Result.Nodes.getNodeAs<Expr>("Calc");
   if (Calc->getLocStart().isMacroID())
     return;
 
   ASTContext &Context = *Result.Context;

docs/clang-tidy/checks/misc-misplaced-widening-cast.rst

 for instance:
 
 .. code-block:: c++
 
     long f(int x) {
         return (long)x * 1000;
     }
 
-Implicit casts
---------------
-
-Forgetting to place the cast at all is at least as dangerous and at least as
-common as misplacing it. If :option:`CheckImplicitCasts` is enabled the check
-also detects these cases, for instance:
 
-.. code-block:: c++
-
-    long f(int x) {
-        return x * 1000;
-    }
 
 Floating point
 --------------
 
 Currently warnings are only written for integer conversion. No warning is
 written for this code:
 
 .. code-block:: c++
 
     double f(float x) {
         return (double)(x * 10.0f);
     }
-
-Options
--------
-
-.. option:: CheckImplicitCasts
-
-   If non-zero, enables detection of implicit casts. Default is non-zero.

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

-// RUN: %check_clang_tidy %s misc-misplaced-widening-cast %t -- -config="{CheckOptions: [{key: misc-misplaced-widening-cast.CheckImplicitCasts, value: 0}]}" --
-
-void func(long arg) {}
-
-void assign(int a, int b) {
-  long l;
-
-  l = a * b;
-  l = (long)(a * b);
-  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: either cast from 'int' to 'long' is ineffective, or there is loss of precision before the conversion [misc-misplaced-widening-cast]
-  l = (long)a * b;
-
-  l = a << 8;
-  l = (long)(a << 8);
-  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: either cast from 'int' to 'long'
-  l = (long)b << 8;
-
-  l = static_cast<long>(a * b);
-  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: either cast from 'int' to 'long'
-}
-
-void compare(int a, int b, long c) {
-  bool l;
-
-  l = a * b == c;
-  l = c == a * b;
-  l = (long)(a * b) == c;
-  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: either cast from 'int' to 'long'
-  l = c == (long)(a * b);
-  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: either cast from 'int' to 'long'
-  l = (long)a * b == c;
-  l = c == (long)a * b;
-}
-
-void init(unsigned int n) {
-  long l1 = n << 8;
-  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) {
-  func(n << 8);
-  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;
-  } else if (a > 0) {
-    return (long)(a * 1000);
-    // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: either cast from 'int' to 'long'
-  } else {
-    return (long)a * 1000;
-  }
-}

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

 // RUN: %check_clang_tidy %s misc-misplaced-widening-cast %t -- -config="{CheckOptions: [{key: misc-misplaced-widening-cast.CheckImplicitCasts, value: 1}]}" --
 
 void func(long arg) {}
 
 void assign(int a, int b) {
   long l;
 
   l = a * b;
-  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: either cast from 'int' to 'long' is ineffective, or there is loss of precision before the conversion [misc-misplaced-widening-cast]
   l = (long)(a * b);
   // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: either cast from 'int' to 'long'
   l = (long)a * b;
 
   l = a << 8;
-  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: either cast from 'int' to 'long'
   l = (long)(a << 8);
   // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: either cast from 'int' to 'long'
   l = (long)b << 8;
 
   l = static_cast<long>(a * b);
   // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: either cast from 'int' to 'long'
 }
 
 void compare(int a, int b, long c) {
   bool l;
 
   l = a * b == c;
-  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: either cast from 'int' to 'long'
   l = c == a * b;
-  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: either cast from 'int' to 'long'
   l = (long)(a * b) == c;
   // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: either cast from 'int' to 'long'
   l = c == (long)(a * b);
   // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: either cast from 'int' to 'long'
   l = (long)a * b == c;
   l = c == (long)a * b;
 }
 
 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) {
   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;
-    // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: either cast from 'int' to 'long'
   } else if (a > 0) {
     return (long)(a * 1000);
     // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: either cast from 'int' to 'long'
   } else {
     return (long)a * 1000;
   }
 }