Page MenuHomePhabricator

[clang-tidy] Suppress reports to similarly used parameters in 'bugprone-easily-swappable-parameters'
AcceptedPublic

Authored by whisperity on Apr 22 2020, 9:46 AM.

Details

Summary

There are several types of functions and various reasons why some "swappable parameters" cannot be fixed with changing the parameters' types, etc. The most common example might be int min(int a, int b)... no matter what you do, the two parameters must remain the same type.

The filtering heuristic implemented in this patch deals with trying to find such functions during the modelling and building of the swappable parameter range. If the parameter currently scrutinised matches either of the predicates below, it will be regarded as not swappable even if the type of the parameter matches.

On average, 50% of the results are silenced by this heuristic (over a set of test projects we investigated). This heuristic has the possibility of producing false negatives, and no path-sensitive analysis is made, as usual in Tidy. However, the reduction of the findings greatly allows developers to see the more useful results about the dodgiest interfaces first and not be deafened by the noise.

The modelling "similarly used parameters" is toggleable by the user as a checker option. Due to the massive benefits this option brings for users who run this check for the first time on their project, the default is to have this turned on.

  • Common expression: if both parameters are found in a common expression somewhere in the function, they are silenced. This heuristic is an absolute blast because it deals with arithmetics, comparisons, forwarding function calls, etc. out of the box at the same time.
  • Returns: if both parameters are returned from different statements of the function, they are silenced. This deals with "switch-like" functions.
  • Pass to same function: if both parameters are passed to the same function('s overload) to the same index, they are silenced. This deals with "dispatch-like" functions.
  • Same member access: if the same member field is accessed or method is called on the two parameters, even if distinct expressions, they are "used in a similar fashion" and are silenced.

Diff Detail

Event Timeline

whisperity created this revision.Apr 22 2020, 9:46 AM
whisperity retitled this revision from [clang-tidy] Add "ignore related parameters" heuristics to 'experimental-cppcoreguidelines-avoid-adjacent-arguments-of-the-same-type' to [clang-tidy] Add "ignore related parameters" heuristics to 'experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type'.Sep 24 2020, 6:23 AM
whisperity planned changes to this revision.Jan 21 2021, 8:45 AM

This patch will be replaced with the refactoring of the base check.

whisperity retitled this revision from [clang-tidy] Add "ignore related parameters" heuristics to 'experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type' to [clang-tidy] Suppress reports to similarly used parameters in 'bugprone-easily-swappable-parameters'.
whisperity edited the summary of this revision. (Show Details)
  • Rebased over updated, rewritten, newer base patch version.
  • Added the heuristic for "same member access" to the patch (which was so far only on my development branch).
whisperity removed a reviewer: zporky.
  • Fixed an issue of the "same function" heuristic that blew up if functions are forward declared.
  • [NFC] Optimised "relatedness" is to be checked first (as it's a trivial double hashmap lookup) before the recursive descent of type checking.
  • [NFC] Reformat the documentation so option values are in single backticks.
aaron.ballman added inline comments.Tue, Mar 16, 9:12 AM
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
491

I *think* you could run into this assert with K&R C function where there is a FunctionDecl to get back to but the decl claims it has no params because it has no prototype (while the call expression actually has the arguments). However, there may be other code that protects us from this case.

542–546

Is this a FIXME because there's a matcher issue this works around or should it be an assertion?

clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.h
43
clang-tools-extra/docs/clang-tidy/checks/bugprone-easily-swappable-parameters.rst
90
whisperity added inline comments.Tue, Mar 16, 10:25 AM
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
542–546

I was unable to create a test for this case. This part of the code, while I was twisting my head left and right, is one of the strictest remnant of the old version of the check... There definitely was a project in the test set back in December 2019 which made the check crash or emit massive false positives, and this thing was added to guard against that.

At a hunch, I think maybe a lambda returning one of its captures (which are, AFAIK, ParmVarDecls of the constructor!) could be what caused the crash/false positive. I will check this out in detail soon.

whisperity added inline comments.Tue, Mar 16, 10:59 AM
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
491

At face value, I would say the fact that a K&R function has no params declared means that the matcher above will not be able to do forEachArgumentWithParam, because the argument vector is N long, but the parameter vector is empty.

Nevertheless, it won't hurt to add a test case for this though.

whisperity marked 4 inline comments as done.
  • Added some test cases to ensure edge cases won't crash.
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
491

No assertion, no crash. But unfortunately, we generate a false positive. But we don't crash. I added a test for this, just in case.

542–546

I do not remember why I thought this makes crashes because I tried and no crashing now (which is good), but lambdas defined inside a function's body is definitely a good reproducer for when this is hit.

Rephrased the comment, and added a test about this.

aaron.ballman added inline comments.Wed, Mar 17, 6:37 AM
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
491

Thanks for adding the test case! I think a false positive is reasonable enough given how rarely I expect people using K&R C function declarations will worry about swapped arguments.

530

Question: would it make sense to (someday, not now) consider output parameters similar to return statements? e.g.,

void int_swap(int &foo, int &bar) {
  int temp = foo;
  foo = bar;
  bar = temp;
}

As a question for today: should co_return should be handled similarly as return?

542–546

Thank you for the clarification and test coverage!

564–567

I don't think there's a need to make these optional when we're using the Enabled flag.

whisperity marked 2 inline comments as done.Wed, Mar 17, 7:02 AM
whisperity added inline comments.
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
530
  1. Maybe. Unfortunately, one needs to be careful with that. Originally I did implement a "5th" heuristic that dealt with ignoring parameters that had the same builtin operator used on them. However, while it silenced a few false positives, it started creating massive amounts of false negatives.

(In the concrete example, I think foo = bar will silence them because "appears in the same expression".)

  1. I don't know. It would require a deeper understanding of Coroutines themselves, and how people use them.
564–567

The original idea was that each heuristic could be individually toggled by the user... But I think we can agree that would be a bad idea and just overengineering.

whisperity added inline comments.Wed, Mar 17, 7:10 AM
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
530

If you don't mind me posting images, I can show a direct example. Things where the inability to track data flow really bites us in the backside.

Examples from Apache Xerces:

Here's a false positive that would be silenced by the logic of "using the same operateur on the two params".

And here is a false negative from the exact same logic.

Maybe it could be some solace that we're restricting to non-const-lvalue-references (and pointers-to-non-const ??) and only assignment (only assignment?!))...

aaron.ballman accepted this revision.Wed, Mar 17, 9:49 AM

LGTM once the optional bits are fixed up.

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

Ah, point #1 is an excellent point. As for #2, I think we can handle it in a follow-up, but I believe co_return follows the same usage patterns as return, generally.

564–567

Yeah, I think it's probably better to drop the optionals here -- we can revisit the decision later if there's user feedback requesting finer granularity here.

This revision is now accepted and ready to land.Wed, Mar 17, 9:49 AM
whisperity marked 4 inline comments as done.

NFC Turned some Optional<T> into just T.

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

Added a TODO for co_return, so we don't forget.