From 211761332e4381c37edd91be7c59fc048014ff4e Mon Sep 17 00:00:00 2001 From: Stephen Concannon Date: Wed, 12 May 2021 20:25:22 +0200 Subject: [PATCH] [clang-tidy] Allow opt-in or out of some commonly occuring patterns in NarrowingConversionsCheck. 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). Patch by Stephen Concannon! Differential Revision: https://reviews.llvm.org/D99543 --- .../NarrowingConversionsCheck.cpp | 61 ++++++++++++++++- .../cppcoreguidelines/NarrowingConversionsCheck.h | 3 + .../cppcoreguidelines-narrowing-conversions.rst | 20 ++++++ ...owing-conversions-equivalentbitwidth-option.cpp | 26 ++++++++ ...onversions-ignoreconversionfromtypes-option.cpp | 76 ++++++++++++++++++++++ ...es-narrowing-conversions-intemplates-option.cpp | 35 ++++++++++ 6 files changed, 219 insertions(+), 2 deletions(-) create mode 100644 clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-narrowing-conversions-equivalentbitwidth-option.cpp create mode 100644 clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-narrowing-conversions-ignoreconversionfromtypes-option.cpp create mode 100644 clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-narrowing-conversions-intemplates-option.cpp diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp index 86371fd..8ce9afc8 100644 --- a/clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp +++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp @@ -7,9 +7,12 @@ //===----------------------------------------------------------------------===// #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" @@ -21,18 +24,33 @@ using namespace clang::ast_matchers; namespace clang { namespace tidy { namespace cppcoreguidelines { +namespace { +auto hasAnyListedName(const std::string &Names) { + const std::vector NameList = + utils::options::parseStringList(Names); + return hasAnyName(std::vector(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); } @@ -42,6 +60,24 @@ void NarrowingConversionsCheck::registerMatchers(MatchFinder *Finder) { 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); @@ -53,7 +89,13 @@ void NarrowingConversionsCheck::registerMatchers(MatchFinder *Finder) { 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); @@ -65,7 +107,12 @@ void NarrowingConversionsCheck::registerMatchers(MatchFinder *Finder) { 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("="))) @@ -256,6 +303,16 @@ void NarrowingConversionsCheck::handleIntegralCast(const ASTContext &Context, 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)) diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.h b/clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.h index 177cdab..12a6bb8 100644 --- a/clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.h +++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.h @@ -94,6 +94,9 @@ private: const BinaryOperator &Op); const bool WarnOnFloatingPointNarrowingConversion; + const bool WarnWithinTemplateInstantiation; + const bool WarnOnEquivalentBitWidth; + const std::string IgnoreConversionFromTypes; const bool PedanticMode; }; diff --git a/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-narrowing-conversions.rst b/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-narrowing-conversions.rst index 79eb582..bfc7669 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-narrowing-conversions.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-narrowing-conversions.rst @@ -35,6 +35,26 @@ Options 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().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 diff --git a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-narrowing-conversions-equivalentbitwidth-option.cpp b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-narrowing-conversions-equivalentbitwidth-option.cpp new file mode 100644 index 0000000..947f169 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-narrowing-conversions-equivalentbitwidth-option.cpp @@ -0,0 +1,26 @@ +// 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] +} diff --git a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-narrowing-conversions-ignoreconversionfromtypes-option.cpp b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-narrowing-conversions-ignoreconversionfromtypes-option.cpp new file mode 100644 index 0000000..2fc5621 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-narrowing-conversions-ignoreconversionfromtypes-option.cpp @@ -0,0 +1,76 @@ +// 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] +} diff --git a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-narrowing-conversions-intemplates-option.cpp b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-narrowing-conversions-intemplates-option.cpp new file mode 100644 index 0000000..863df40 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-narrowing-conversions-intemplates-option.cpp @@ -0,0 +1,35 @@ +// 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 +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); +} -- 2.7.4