[clang-tidy] Allow opt-in or out of some commonly occuring patterns in NarrowingConversionsCheck.

[Stephen]

Summary

Within clang-tidy's NarrowingConversionsCheck.

• Allow opt-out of some common occurring patterns, such as:
  • Implicit casts between types of equivalent bit widths.
  • Implicit casts occurring from the return of a ::size() method.
  • Implicit casts on size_type and difference_type.
• Allow opt-in of errors within template instantiations.

This will help projects adopt these guidelines iteratively.
Developed in conjunction with Yitzhak Mandelbaum (ymandel).

[Stephen]

Updated change description and improved formatting.

[ymandel]

But please wait for additional "Accept" from either hokein or aaron.ballman. Thanks!

[aaron.ballman]

Thanks! I think the changes generally LG, but I did have a question that might generate a small change.

clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp

86

Should this also support size_t and ptrdiff_t given that those are usually the same as size_type and difference_type?

[hokein]

a small suggestion to simplify the tests.

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-narrowing-conversions-equivalentbitwidth-off.cpp

1 ↗

(On Diff #333994)

instead of having multiple individual test files, I think we can group them into a single file, you can use the flag -check-suffix=, like

// RUN: %check_clang_tidy -check-suffix=TESTCASE1 ...
// RUN: %check_clang_tidy -check-suffix=TESTCASE2 ...


// some warning code for test-case1
CHECK-MESSAGES-TESTCASE1: ....

[Stephen]

• Add ability to pass semi-colon separated list of types to ignore, instead of assuming only size_type and difference_type.
• Remove ability to avoid warnings on ::size() method, since it's subsumed by allowing narrowing conversions of size_t.
• Coalesce on/off option tests into per-option test files.

[Stephen]

Thanks for the review! I think I've addressed the comments -- hopefully I kept it inline with what you're thinking. Please have another look at your leisure.

clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp

86

Thanks, yeah, that's a good idea. I was somewhat apprehensive to ignore all conversions involving size_t, but it's probably ok. Now that we are, we don't need the ::size() method stuff, so I removed that.

And it seemed like I might as well make the list of types configurable, so I rolled that in here too. So it's a bigger change that you maybe had intended, though hopefully not too far off the mark.

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-narrowing-conversions-equivalentbitwidth-off.cpp

1 ↗

(On Diff #333994)

Thanks! Besides cutting down the number of files, it really helped clarify the distinction in behavior with the warning on/off.

I coalesced the on/off pairs into a single, but kept the options tested in separate files, as that seemed to be the pattern in place.

Let me know if you'd prefer to squeeze them into fewer.

[Stephen]

(fix patch to include all local changes)

[ymandel]

clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-narrowing-conversions.rst

51

nit: add dash "semicolon-separated"

[Stephen]

Within clang-tidy's NarrowingConversionsCheck.

• Allow opt-out of some common occurring patterns, such as:
  • Implicit casts between types of equivalent bit widths.
  • Implicit casts occurring from the return of a ::size() method.
  • Implicit casts on size_type and difference_type.
• Allow opt-in of errors within template instantiations.

This will help projects adopt these guidelines iteratively.
Developed in conjunction with Yitzhak Mandelbaum (ymandel).

[Stephen]

Thanks, ymandel!

Anything else, aaron.ballman or hokein?

[njames93]

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-narrowing-conversions-ignoreconversionfromtypes-option.cpp

13

This is breaking tests on windows, It seems like its pre-defined to be unsigned long long on windows.
Simplest fix is to rename size_t to something else.
I guess changing the typedef to unsigned long long will change behaviour of tests.
Failing that an // UNSUPPORTED: system-windows directive at the top of the file would also work.

[Stephen]

1. Updating D99543: [clang-tidy] Allow opt-in or out of some commonly occuring patterns in NarrowingConversionsCheck. #
2. Enter a brief description of the changes included in this update.
3. The first line is used as subject, next lines as comment. #
4. If you intended to create a new revision, use:
5. $ arc diff --create

Within clang-tidy's NarrowingConversionsCheck.

• Allow opt-out of some common occurring patterns, such as:
  • Implicit casts between types of equivalent bit widths.
  • Implicit casts occurring from the return of a ::size() method.
  • Implicit casts on size_type and difference_type.
• Allow opt-in of errors within template instantiations.

This will help projects adopt these guidelines iteratively.
Developed in conjunction with Yitzhak Mandelbaum (ymandel).

[Stephen]

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-narrowing-conversions-ignoreconversionfromtypes-option.cpp

13

Thanks, njames93. I've switched to custom type names to avoid these sorts of conflicts. I've also removed the difference_type test, since it didn't test distinct functionality from size_type, once they use custom names.

Sorry for the inconvenience.

[hokein]

thanks, looks good. Sorry for the delay.

clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.h

private:
 
   void handleImplicitCast(const ASTContext &Context,
                           const ImplicitCastExpr &Cast);
 
   void handleBinaryOperator(const ASTContext &Context,
                             const BinaryOperator &Op);
 
   const bool WarnOnFloatingPointNarrowingConversion;
+  const bool WarnWithinTemplateInstantiation;
+  const bool WarnOnEquivalentBitWidth;
+  const std::string IgnoreConversionFromTypes;
   const bool PedanticMode;
 };
 
 } // namespace cppcoreguidelines
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_NARROWING_CONVERSIONS_H

clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp

 //===--- NarrowingConversionsCheck.cpp - clang-tidy------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "NarrowingConversionsCheck.h"
+#include "../utils/OptionsUtils.h"
 #include "clang/AST/ASTContext.h"
+#include "clang/AST/Expr.h"
 #include "clang/AST/Type.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
 #include "llvm/ADT/APSInt.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/SmallVector.h"
 
 #include <cstdint>
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace cppcoreguidelines {
+namespace {
+auto hasAnyListedName(const std::string &Names) {
+  const std::vector<std::string> NameList =
+      utils::options::parseStringList(Names);
+  return hasAnyName(std::vector<StringRef>(NameList.begin(), NameList.end()));
+}
+} // namespace
 
 NarrowingConversionsCheck::NarrowingConversionsCheck(StringRef Name,
                                                      ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
       WarnOnFloatingPointNarrowingConversion(
           Options.get("WarnOnFloatingPointNarrowingConversion", true)),
+      WarnWithinTemplateInstantiation(
+          Options.get("WarnWithinTemplateInstantiation", false)),
+      WarnOnEquivalentBitWidth(Options.get("WarnOnEquivalentBitWidth", true)),
+      IgnoreConversionFromTypes(Options.get("IgnoreConversionFromTypes", "")),
       PedanticMode(Options.get("PedanticMode", false)) {}
 
 void NarrowingConversionsCheck::storeOptions(
     ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "WarnOnFloatingPointNarrowingConversion",
                 WarnOnFloatingPointNarrowingConversion);
+  Options.store(Opts, "WarnWithinTemplateInstantiation",
+                WarnWithinTemplateInstantiation);
+  Options.store(Opts, "WarnOnEquivalentBitWidth", WarnOnEquivalentBitWidth);
+  Options.store(Opts, "IgnoreConversionFromTypes", IgnoreConversionFromTypes);
   Options.store(Opts, "PedanticMode", PedanticMode);
 }
 
 void NarrowingConversionsCheck::registerMatchers(MatchFinder *Finder) {
   // ceil() and floor() are guaranteed to return integers, even though the type
   // is not integral.
   const auto IsCeilFloorCallExpr = expr(callExpr(callee(functionDecl(
       hasAnyName("::ceil", "::std::ceil", "::floor", "::std::floor")))));
 
+  // We may want to exclude other types from the checks, such as `size_type`
+  // and `difference_type`. These are often used to count elements, represented
+  // in 64 bits and assigned to `int`. Rarely are people counting >2B elements.
+  const auto IsConversionFromIgnoredType =
+      hasType(namedDecl(hasAnyListedName(IgnoreConversionFromTypes)));
+
+  // `IsConversionFromIgnoredType` will ignore narrowing calls from those types,
+  // but not expressions that are promoted to `int64` due to a binary expression
+  // with one of those types. For example, it will continue to reject:
+  // `int narrowed = int_value + container.size()`.
+  // We attempt to address common incidents of compound expressions with
+  // `IsIgnoredTypeTwoLevelsDeep`, allowing binary expressions that have one
+  // operand of the ignored types and the other operand of another integer type.
+  const auto IsIgnoredTypeTwoLevelsDeep =
+      anyOf(IsConversionFromIgnoredType,
+            binaryOperator(hasOperands(IsConversionFromIgnoredType,
+                                       hasType(isInteger()))));
+
   // Casts:
   //   i = 0.5;
   //   void f(int); f(0.5);
   Finder->addMatcher(
       traverse(TK_AsIs, implicitCastExpr(
                             hasImplicitDestinationType(
                                 hasUnqualifiedDesugaredType(builtinType())),
                             hasSourceExpression(hasType(
                                 hasUnqualifiedDesugaredType(builtinType()))),
                             unless(hasSourceExpression(IsCeilFloorCallExpr)),
                             unless(hasParent(castExpr())),
-                            unless(isInTemplateInstantiation()))
+                            WarnWithinTemplateInstantiation
+                                ? stmt()
+                                : stmt(unless(isInTemplateInstantiation())),
+                            IgnoreConversionFromTypes.empty()
+                                ? castExpr()
+                                : castExpr(unless(hasSourceExpression(
+                                      IsIgnoredTypeTwoLevelsDeep))))
                             .bind("cast")),
       this);
 
   // Binary operators:
   //   i += 0.5;
   Finder->addMatcher(
       binaryOperator(
           isAssignmentOperator(),
           hasLHS(expr(hasType(hasUnqualifiedDesugaredType(builtinType())))),
           hasRHS(expr(hasType(hasUnqualifiedDesugaredType(builtinType())))),
           unless(hasRHS(IsCeilFloorCallExpr)),
-          unless(isInTemplateInstantiation()),
+          WarnWithinTemplateInstantiation
+              ? binaryOperator()
+              : binaryOperator(unless(isInTemplateInstantiation())),
+          IgnoreConversionFromTypes.empty()
+              ? binaryOperator()
+              : binaryOperator(unless(hasRHS(IsIgnoredTypeTwoLevelsDeep))),
           // The `=` case generates an implicit cast
           // which is covered by the previous matcher.
           unless(hasOperatorName("=")))
           .bind("binary_op"),
       this);
 }
 
 static const BuiltinType *getBuiltinType(const Expr &E) {
void NarrowingConversionsCheck::handleIntegralCast(const ASTContext &Context,
   // From [conv.integral]p7.3.8:
   // Conversions to unsigned integer is well defined so no warning is issued.
   // "The resulting value is the smallest unsigned value equal to the source
   // value modulo 2^n where n is the number of bits used to represent the
   // destination type."
   if (ToType->isUnsignedInteger())
     return;
   const BuiltinType *FromType = getBuiltinType(Rhs);
+
+  // With this option, we don't warn on conversions that have equivalent width
+  // in bits. eg. uint32 <-> int32.
+  if (!WarnOnEquivalentBitWidth) {
+    uint64_t FromTypeSize = Context.getTypeSize(FromType);
+    uint64_t ToTypeSize = Context.getTypeSize(ToType);
+    if (FromTypeSize == ToTypeSize)
+      return;
+  }
+
   llvm::APSInt IntegerConstant;
   if (getIntegerConstantExprValue(Context, Rhs, IntegerConstant)) {
     if (!isWideEnoughToHold(Context, IntegerConstant, *ToType))
       diagNarrowIntegerConstantToSignedInt(SourceLoc, Lhs, Rhs, IntegerConstant,
                                            Context.getTypeSize(FromType));
     return;
   }
   if (!isWideEnoughToHold(Context, *FromType, *ToType))

clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-narrowing-conversions.rst

 Options
 -------
 
 .. option:: WarnOnFloatingPointNarrowingConversion
 
     When `true`, the check will warn on narrowing floating point conversion
     (e.g. ``double`` to ``float``). `true` by default.
 
+.. option:: WarnWithinTemplateInstantiation
+
+    When `true`, the check will warn on narrowing conversions within template
+    instantations. `false` by default.
+
+.. option:: WarnOnEquivalentBitWidth
+
+    When `true`, the check will warn on narrowing conversions that arise from
+    casting between types of equivalent bit width. (e.g.
+    `int n = uint(0);` or `long long n = double(0);`) `true` by default.
+
+.. option:: IgnoreConversionFromTypes
+
+   Narrowing conversions from any type in this semicolon-separated list will be
+   ignored. This may be useful to weed out commonly occurring, but less commonly
+   problematic assignments such as `int n = std::vector<char>().size();` or
+   `int n = std::difference(it1, it2);`. The default list is empty, but one
+   suggested list for a legacy codebase would be
+   `size_t;ptrdiff_t;size_type;difference_type`.
+
 .. option:: PedanticMode
 
     When `true`, the check will warn on assigning a floating point constant
     to an integer value even if the floating point value is exactly
     representable in the destination type (e.g. ``int i = 1.0;``).
     `false` by default.
 
 FAQ

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-narrowing-conversions-equivalentbitwidth-option.cpp

+// RUN: %check_clang_tidy -check-suffix=DEFAULT %s \
+// RUN: cppcoreguidelines-narrowing-conversions %t -- \
+// RUN: -config='{CheckOptions: [ \
+// RUN: ]}'
+
+// RUN: %check_clang_tidy -check-suffix=DISABLED %s \
+// RUN: cppcoreguidelines-narrowing-conversions %t -- \
+// RUN: -config='{CheckOptions: [ \
+// RUN:   {key: cppcoreguidelines-narrowing-conversions.WarnOnEquivalentBitWidth, value: 0} \
+// RUN: ]}'
+
+void narrowing_equivalent_bitwidth() {
+  int i;
+  unsigned int j;
+  i = j;
+  // CHECK-MESSAGES-DEFAULT: :[[@LINE-1]]:7: warning: narrowing conversion from 'unsigned int' to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions]
+  // DISABLED: Warning disabled with WarnOnEquivalentBitWidth=0.
+}
+
+void most_narrowing_is_not_ok() {
+  int i;
+  long long j;
+  i = j;
+  // CHECK-MESSAGES-DEFAULT: :[[@LINE-1]]:7: warning: narrowing conversion from 'long long' to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions]
+  // CHECK-MESSAGES-DISABLED: :[[@LINE-2]]:7: warning: narrowing conversion from 'long long' to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions]
+}

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-narrowing-conversions-ignoreconversionfromtypes-option.cpp

+// RUN: %check_clang_tidy -check-suffix=DEFAULT %s \
+// RUN: cppcoreguidelines-narrowing-conversions %t -- \
+// RUN: -config='{CheckOptions: [ \
+// RUN: ]}'
+
+// RUN: %check_clang_tidy -check-suffix=IGNORED %s \
+// RUN: cppcoreguidelines-narrowing-conversions %t -- \
+// RUN: -config='{CheckOptions: [ \
+// RUN:   {key: cppcoreguidelines-narrowing-conversions.IgnoreConversionFromTypes, value: "global_size_t;nested_size_type"} \
+// RUN: ]}'
+
+// We use global_size_t instead of 'size_t' because windows predefines size_t.
+typedef long long global_size_t;
+
+struct vector {
+  typedef long long nested_size_type;
+
+  global_size_t size() const { return 0; }
+};
+
+void narrowing_global_size_t() {
+  int i;
+  global_size_t j;
+  i = j;
+  // CHECK-MESSAGES-DEFAULT: :[[@LINE-1]]:7: warning: narrowing conversion from 'global_size_t' (aka 'long long') to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions]
+  // IGNORED: Warning is disabled with IgnoreConversionFromTypes=global_size_t.
+}
+
+void narrowing_size_type() {
+  int i;
+  vector::nested_size_type j;
+  i = j;
+  // CHECK-MESSAGES-DEFAULT: :[[@LINE-1]]:7: warning: narrowing conversion from 'vector::nested_size_type' (aka 'long long') to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions]
+  // IGNORED: Warning is disabled with IgnoreConversionFromTypes=nested_size_type.
+}
+
+void narrowing_size_method() {
+  vector v;
+  int i, j;
+  i = v.size();
+  // CHECK-MESSAGES-DEFAULT: :[[@LINE-1]]:7: warning: narrowing conversion from 'global_size_t' (aka 'long long') to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions]
+  // IGNORED: Warning is disabled with IgnoreConversionFromTypes=global_size_t.
+
+  i = j + v.size();
+  // CHECK-MESSAGES-DEFAULT: :[[@LINE-1]]:7: warning: narrowing conversion from 'long long' to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions]
+  // IGNORED: Warning is disabled with IgnoreConversionFromTypes=global_size_t.
+}
+
+void narrowing_size_method_binary_expr() {
+  int i;
+  int j;
+  vector v;
+  i = j + v.size();
+  // CHECK-MESSAGES-DEFAULT: :[[@LINE-1]]:7: warning: narrowing conversion from 'long long' to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions]
+  // IGNORED: Warning is disabled with IgnoreConversionFromTypes=global_size_t.
+}
+
+void narrowing_size_method_binary_op() {
+  int i, j;
+  vector v;
+  i += v.size();
+  // CHECK-MESSAGES-DEFAULT: :[[@LINE-1]]:8: warning: narrowing conversion from 'global_size_t' (aka 'long long') to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions]
+  // IGNORED: Warning is disabled with IgnoreConversionFromTypes=global_size_t.
+
+  i += j + v.size();
+  // CHECK-MESSAGES-DEFAULT: :[[@LINE-1]]:8: warning: narrowing conversion from 'long long' to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions]
+  // IGNORED: Warning is disabled with IgnoreConversionFromTypes=global_size_t.
+}
+
+void most_narrowing_is_not_ok() {
+  int i;
+  long long j;
+  i = j;
+  // CHECK-MESSAGES-DEFAULT: :[[@LINE-1]]:7: warning: narrowing conversion from 'long long' to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions]
+  // CHECK-MESSAGES-IGNORED: :[[@LINE-2]]:7: warning: narrowing conversion from 'long long' to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions]
+}

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-narrowing-conversions-intemplates-option.cpp

+// RUN: %check_clang_tidy -check-suffix=DEFAULT %s \
+// RUN: cppcoreguidelines-narrowing-conversions %t -- \
+// RUN: -config='{CheckOptions: [ \
+// RUN: ]}'
+
+// RUN: %check_clang_tidy -check-suffix=WARN %s \
+// RUN: cppcoreguidelines-narrowing-conversions %t -- \
+// RUN: -config='{CheckOptions: [ \
+// RUN:   {key: cppcoreguidelines-narrowing-conversions.WarnWithinTemplateInstantiation, value: 1} \
+// RUN: ]}'
+
+template <typename OrigType>
+void assign_in_template(OrigType jj) {
+  int ii;
+  ii = jj;
+  // DEFAULT: Warning disabled because WarnWithinTemplateInstantiation=0.
+  // CHECK-MESSAGES-WARN: :[[@LINE-2]]:8: warning: narrowing conversion from 'long long' to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions]
+}
+
+void narrow_inside_template_not_ok() {
+  long long j = 123;
+  assign_in_template(j);
+}
+
+void assign_outside_template(long long jj) {
+  int ii;
+  ii = jj;
+  // CHECK-MESSAGES-DEFAULT: :[[@LINE-1]]:8: warning: narrowing conversion from 'long long' to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions]
+  // CHECK-MESSAGES-WARN: :[[@LINE-2]]:8: warning: narrowing conversion from 'long long' to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions]
+}
+
+void narrow_outside_template_not_ok() {
+  long long j = 123;
+  assign_outside_template(j);
+}