Commit 0fba450b authored by Whisperity's avatar Whisperity
Browse files

[clang-tidy] Suppress reports to patternedly named parameters in... 

[clang-tidy] Suppress reports to patternedly named parameters in 'bugprone-easily-swappable-parameters'

While the original check's purpose is to identify potentially dangerous
functions based on the parameter types (as identifier names do not mean
anything when it comes to the language rules), unfortunately, such a plain
interface check rule can be incredibly noisy. While the previous
"filtering heuristic" is able to find many similar usages, there is an entire
class of parameters that should not be warned about very easily mixed by that
check: parameters that have a name and their name follows a pattern,
e.g. `text1, text2, text3, ...`.`

This patch implements a simple, but powerful rule, that allows us to detect
such cases and ensure that no warnings are emitted for parameter sequences that
follow a pattern, even if their types allow for them to be potentially mixed at a call site.

Given a threshold `k`, warnings about two parameters are filtered from the
result set if the names of the parameters are either prefixes or suffixes of
each other, with at most k letters difference on the non-common end.
(Assuming that the names themselves are at least `k` long.)

 - The above `text1, text2` is an example of this. (Live finding from Xerces.)
 - `LHS` and `RHS` are also fitting the bill here. (Live finding from... virtually any project.)
 - So does `Qmat, Tmat, Rmat`. (Live finding from I think OpenCV.)

Reviewed By: aaron.ballman

Differential Revision: http://reviews.llvm.org/D97297
parent b9ece034

clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp

+91 −3
Original line number Diff line number Diff line
@@ -70,6 +70,11 @@ static constexpr bool DefaultModelImplicitConversions = true; 
/// used together.

static constexpr bool DefaultSuppressParametersUsedTogether = true;



/// The default value for the NamePrefixSuffixSilenceDissimilarityTreshold

/// check option.

static constexpr std::size_t

    DefaultNamePrefixSuffixSilenceDissimilarityTreshold = 1;



using namespace clang::ast_matchers;



namespace clang {
@@ -85,6 +90,8 @@ static bool isIgnoredParameter(const TheCheck &Check, const ParmVarDecl *Node); 
static inline bool

isSimilarlyUsedParameter(const SimilarlyUsedParameterPairSuppressor &Suppressor,

                         const ParmVarDecl *Param1, const ParmVarDecl *Param2);

static bool prefixSuffixCoverUnderThreshold(std::size_t Threshold,

                                            StringRef Str1, StringRef Str2);

} // namespace filter



namespace model {
@@ -1292,13 +1299,25 @@ static MixableParameterRange modelMixingRange( 


  for (std::size_t I = StartIndex + 1; I < NumParams; ++I) {

    const ParmVarDecl *Ith = FD->getParamDecl(I);

    LLVM_DEBUG(llvm::dbgs() << "Check param #" << I << "...\n");



    StringRef ParamName = Ith->getName();

    LLVM_DEBUG(llvm::dbgs()

               << "Check param #" << I << " '" << ParamName << "'...\n");

    if (filter::isIgnoredParameter(Check, Ith)) {

      LLVM_DEBUG(llvm::dbgs() << "Param #" << I << " is ignored. Break!\n");

      break;

    }



    StringRef PrevParamName = FD->getParamDecl(I - 1)->getName();

    if (!ParamName.empty() && !PrevParamName.empty() &&

        filter::prefixSuffixCoverUnderThreshold(

            Check.NamePrefixSuffixSilenceDissimilarityTreshold, PrevParamName,

            ParamName)) {

      LLVM_DEBUG(llvm::dbgs() << "Parameter '" << ParamName

                              << "' follows a pattern with previous parameter '"

                              << PrevParamName << "'. Break!\n");

      break;

    }



    // Now try to go forward and build the range of [Start, ..., I, I + 1, ...]

    // parameters that can be messed up at a call site.

    MixableParameterRange::MixVector MixesOfIth;
@@ -1675,6 +1694,70 @@ isSimilarlyUsedParameter(const SimilarlyUsedParameterPairSuppressor &Suppressor, 
  return Suppressor(Param1, Param2);

}



static void padStringAtEnd(SmallVectorImpl<char> &Str, std::size_t ToLen) {

  while (Str.size() < ToLen)

    Str.emplace_back('\0');

}



static void padStringAtBegin(SmallVectorImpl<char> &Str, std::size_t ToLen) {

  while (Str.size() < ToLen)

    Str.insert(Str.begin(), '\0');

}



static bool isCommonPrefixWithoutSomeCharacters(std::size_t N, StringRef S1,

                                                StringRef S2) {

  assert(S1.size() >= N && S2.size() >= N);

  StringRef S1Prefix = S1.take_front(S1.size() - N),

            S2Prefix = S2.take_front(S2.size() - N);

  return S1Prefix == S2Prefix && !S1Prefix.empty();

}



static bool isCommonSuffixWithoutSomeCharacters(std::size_t N, StringRef S1,

                                                StringRef S2) {

  assert(S1.size() >= N && S2.size() >= N);

  StringRef S1Suffix = S1.take_back(S1.size() - N),

            S2Suffix = S2.take_back(S2.size() - N);

  return S1Suffix == S2Suffix && !S1Suffix.empty();

}



/// Returns whether the two strings are prefixes or suffixes of each other with

/// at most Threshold characters differing on the non-common end.

static bool prefixSuffixCoverUnderThreshold(std::size_t Threshold,

                                            StringRef Str1, StringRef Str2) {

  if (Threshold == 0)

    return false;



  // Pad the two strings to the longer length.

  std::size_t BiggerLength = std::max(Str1.size(), Str2.size());



  if (BiggerLength <= Threshold)

    // If the length of the strings is still smaller than the threshold, they

    // would be covered by an empty prefix/suffix with the rest differing.

    // (E.g. "A" and "X" with Threshold = 1 would mean we think they are

    // similar and do not warn about them, which is a too eager assumption.)

    return false;



  SmallString<32> S1PadE{Str1}, S2PadE{Str2};

  padStringAtEnd(S1PadE, BiggerLength);

  padStringAtEnd(S2PadE, BiggerLength);



  if (isCommonPrefixWithoutSomeCharacters(

          Threshold, StringRef{S1PadE.begin(), BiggerLength},

          StringRef{S2PadE.begin(), BiggerLength}))

    return true;



  SmallString<32> S1PadB{Str1}, S2PadB{Str2};

  padStringAtBegin(S1PadB, BiggerLength);

  padStringAtBegin(S2PadB, BiggerLength);



  if (isCommonSuffixWithoutSomeCharacters(

          Threshold, StringRef{S1PadB.begin(), BiggerLength},

          StringRef{S2PadB.begin(), BiggerLength}))

    return true;



  return false;

}



} // namespace filter



/// Matches functions that have at least the specified amount of parameters.
@@ -1891,7 +1974,10 @@ EasilySwappableParametersCheck::EasilySwappableParametersCheck( 
                                           DefaultModelImplicitConversions)),

      SuppressParametersUsedTogether(

          Options.get("SuppressParametersUsedTogether",

                      DefaultSuppressParametersUsedTogether)) {}

                      DefaultSuppressParametersUsedTogether)),

      NamePrefixSuffixSilenceDissimilarityTreshold(

          Options.get("NamePrefixSuffixSilenceDissimilarityTreshold",

                      DefaultNamePrefixSuffixSilenceDissimilarityTreshold)) {}



void EasilySwappableParametersCheck::storeOptions(

    ClangTidyOptions::OptionMap &Opts) {
@@ -1904,6 +1990,8 @@ void EasilySwappableParametersCheck::storeOptions( 
  Options.store(Opts, "ModelImplicitConversions", ModelImplicitConversions);

  Options.store(Opts, "SuppressParametersUsedTogether",

                SuppressParametersUsedTogether);

  Options.store(Opts, "NamePrefixSuffixSilenceDissimilarityTreshold",

                NamePrefixSuffixSilenceDissimilarityTreshold);

}



void EasilySwappableParametersCheck::registerMatchers(MatchFinder *Finder) {

clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.h

+6 −0
Original line number Diff line number Diff line
@@ -51,6 +51,12 @@ public: 
  /// If enabled, diagnostics for parameters that are used together in a

  /// similar way are not emitted.

  const bool SuppressParametersUsedTogether;



  /// The number of characters two parameter names might be dissimilar at

  /// either end for the report about the parameters to be silenced.

  /// E.g. the names "LHS" and "RHS" are 1-dissimilar suffixes of each other,

  /// while "Text1" and "Text2" are 1-dissimilar prefixes of each other.

  const std::size_t NamePrefixSuffixSilenceDissimilarityTreshold;

};



} // namespace bugprone

clang-tools-extra/docs/clang-tidy/checks/bugprone-easily-swappable-parameters.rst

+21 −0
Original line number Diff line number Diff line
@@ -169,6 +169,27 @@ noisiness. 
    * Separate ``return`` statements return either of the parameters on

      different code paths.



.. option:: NamePrefixSuffixSilenceDissimilarityTreshold



    The number of characters two parameter names might be different on *either*

    the head or the tail end with the rest of the name the same so that the

    warning about the two parameters are silenced.

    Defaults to `1`.

    Might be any positive integer.

    If `0`, the filtering heuristic based on the parameters' names is turned

    off.



    This option can be used to silence warnings about parameters where the

    naming scheme indicates that the order of those parameters do not matter.



    For example, the parameters ``LHS`` and ``RHS`` are 1-dissimilar suffixes

    of each other: ``L`` and ``R`` is the different character, while ``HS``

    is the common suffix.

    Similarly, parameters ``text1, text2, text3`` are 1-dissimilar prefixes

    of each other, with the numbers at the end being the dissimilar part.

    If the value is at least `1`, such cases will not be reported.





Limitations

-----------

clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-ignore.cpp

+2 −1
Original line number Diff line number Diff line
@@ -5,7 +5,8 @@ 
// RUN:     {key: bugprone-easily-swappable-parameters.IgnoredParameterTypeSuffixes, value: "T"}, \

// RUN:     {key: bugprone-easily-swappable-parameters.QualifiersMix, value: 0}, \

// RUN:     {key: bugprone-easily-swappable-parameters.ModelImplicitConversions, value: 0}, \

// RUN:     {key: bugprone-easily-swappable-parameters.SuppressParametersUsedTogether, value: 0} \

// RUN:     {key: bugprone-easily-swappable-parameters.SuppressParametersUsedTogether, value: 0}, \

// RUN:     {key: bugprone-easily-swappable-parameters.NamePrefixSuffixSilenceDissimilarityTreshold, value: 0} \

// RUN:  ]}' --



void ignoredUnnamed(int I, int, int) {} // NO-WARN: No >= 2 length of non-unnamed.

clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-implicit-qualifiers.cpp

+2 −1
Original line number Diff line number Diff line
@@ -5,7 +5,8 @@ 
// RUN:     {key: bugprone-easily-swappable-parameters.IgnoredParameterTypeSuffixes, value: ""}, \

// RUN:     {key: bugprone-easily-swappable-parameters.QualifiersMix, value: 1}, \

// RUN:     {key: bugprone-easily-swappable-parameters.ModelImplicitConversions, value: 1}, \

// RUN:     {key: bugprone-easily-swappable-parameters.SuppressParametersUsedTogether, value: 0} \

// RUN:     {key: bugprone-easily-swappable-parameters.SuppressParametersUsedTogether, value: 0}, \

// RUN:     {key: bugprone-easily-swappable-parameters.NamePrefixSuffixSilenceDissimilarityTreshold, value: 0} \

// RUN:  ]}' --



void numericAndQualifierConversion(int I, const double CD) { numericAndQualifierConversion(CD, I); }