[clang-tidy] Add 'bugprone-easily-swappable-parameters' check

[whisperity]

Summary

Finds function definitions where parameters of convertible types follow each other directly, making call sites prone to calling the function with swapped (or badly ordered) arguments.

Such constructs are usually the result of insufficient design and lack the exploitation of strong type capabilities that are possible in the language.
This check finds and flags function definitions, and not call sites. The reasoning is that while there are several ways to heuristically detect a potential swapped call, in the long term, ensuring a safe interface is more beneficial.

As an interface rule, it is understandable that it goes nuts over large projects that have yet to make their interfaces stricter and stronger.

Some of the most trivial flagged examples might be:

FILE *open(std::string_view Directory, std::string_view Filename, std::string_view Extension) { /* ... */ }

Originally, the check was meant to implement C++ Core Guideline rule I.24 "Avoid adjacent parameters of the same type when changing the argument order would change meaning" here, however, discussion about the rule's applicability is ongoing. For now, introducing the check outside the cppcoreguidelines- namespace so that people can still benefit from it.

[aaron.ballman]

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

403–404

If you don't mind doing that effort, it'd be appreciated! Even if you don't root cause the issue, having the report (or pinging an existing one) is still useful.

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

11

However, I feel we should specifically not rely on the names and the logic too much.

I agree, to a point. I don't think the basic check logic should be name-sensitive, but I do think we need to rely on names to tweak the true/false positive/negative ratios. I think most of the time when we're relying on names should wind up being configuration options so that users can tune the algorithm to their needs.

We could put the logic into the suspicious call argument check with roughly the same logic -- the call site looks like it has potentially swapped arguments because the function redeclaration chain (which includes the function definition, as that's also a declaration) has inconsistent parameter names. So long as the diagnostic appears on the call site, and then refers back to the inconsistent declarations, it could probably work. However, I think it'd be a bit strange to diagnose the call site because the user of the API didn't really do anything wrong (and they may not even be able to change the declaration or the definition where the problem really lives).

[whisperity]

In D69560#2536570, @steveire wrote:

I haven't read through all the comments, but the word 'easily' implies 'desirable'. This check seems to be for finding params which are undesirably swappable, right?

The easily was to mean that the swap can be done with little effort (i.e. "in an easy fashion"?) and by accident. We want to find function signatures where that is the case, yes. TL;DR: "This function will be misused by a careless client, please try designing a better interface!"

[whisperity]

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

403–404

And I managed to post a broken link originally... Here's the right one: https://bugs.llvm.org/show_bug.cgi?id=49000

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

11

But wait... suppose there is a project A, which sees the header for f(int x, int y) and has a call f(y, x). That will be diagnosed. But if project A is only a client of f, it should be highly unlikely that project A has the definition of f in their tree, right? So people of A will not get the warning for that. Only what is part of the compilation process will be part of the analysis process.

Similarly to how this check matches only function definitions, and never a prototype. The latter one would have bazillion of warnings from every header ever, with the user not having a chance to fix it anyways.

[aaron.ballman]

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

11

But wait... suppose there is a project A

That's the point I was trying to make about it being strange to diagnose on the call site -- the user of the API didn't use it wrong, the designer of the API did something bad.

It's sounding more like this functionality is a good idea for a new check rather than an existing check.

[whisperity]

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

11

Okay, now I understand! But I did not intend to diagnose it on the call site. The other check itself would share the same configuration parameters and decision-making for string distance, and the "fwd decl vs. definition" diagnostic would come emitted to the definition's location! And the "call site vs. called function" would be diagnosed at the call site. And if there isn't a definition in the analysed TU for some function, the former logic is simply skipped.

[aaron.ballman]

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

11

But I did not intend to diagnose it on the call site.

I'd find that to be a bit strange given that the other check is called readability-suspicious-call-argument, because there would be no call in that situation. Or am I misunderstanding? (Sorry if I'm being dense!)

[whisperity]

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

11

No, it is strange! And I think we will come up with a better idea for this because the check logic itself sounds very plausible, and needed. Either factor out the string distance logic somewhere and have two checks with separate matchers, but the same underlying logic then, or rename the other check... The two diagnostics would have a different message either way. The latter option (finding a new name for the check) sounds like the path of least resistance.

[aaron.ballman]

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

11

Ah, yes! I think we're on the same page now. :-)

[whisperity]

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

178

K&R C functions

Call me too young, but I had to look up what a "K&R C function" is, and I am absolutely baffled how this unholy construct is still supported! But: thanks to Clang supporting it properly in the AST, the checker works out of the box!

Given

int foo(a, b)
  int a;
  int b;
{
  return a + b;
}

We get the following output:

/tmp/knr.c:2:3: warning: 2 adjacent parameters of 'foo' of similar type ('int') are easily swapped by mistake [bugprone-easily-swappable-parameters]
  int a;
  ^
/tmp/knr.c:2:7: note: the first parameter in the range is 'a'
  int a;
      ^
/tmp/knr.c:3:7: note: the last parameter in the range is 'b'
  int b;
      ^

(even the locations are consistent!)

Should I add a test case for this? We could use a specifically C test case either way eventually...

---

varargs functions

This is a bit of terminology, but something tells me you meant the variadic function here, right? As opposed to type parameter packs.

Given

int sum(int ints...) {
  return 0;
}

the AST looks something like this:

`-FunctionDecl 0x56372e29e258 <variadic.cpp:1:1, line:3:1> line:1:5 sum 'int (int, ...)'
  |-ParmVarDecl 0x56372e29e188 <col:9, col:13> col:13 ints 'int'

Should we diagnose this? And if so, how? The variadic part is not represented (at least not at first glance?) in the AST. Understanding the insides of such a function would require either overapproximatic stuff and doing a looot of extra handling, or becoming flow sensitive... and we'd still need to understand all the va_ standard functions' semantics either way.

[riccibruno]

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

338

I think that this will become unreachable after D84658 and D85033. I may be missing an argument, but in general I would argue that getNameForDiagnostic should never return an empty name for unnamed entities since this is not useful in diagnostics.

[aaron.ballman]

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

178

Call me too young, but I had to look up what a "K&R C function" is, and I am absolutely baffled how this unholy construct is still supported!

Ah, to be innocent again, how I miss those days. :-D

Should I add a test case for this? We could use a specifically C test case either way eventually...

I think it'd be a useful case, but the one I was specifically more concerned with is:

// N.B.: this is C-specific and does not apply to C++.
void f();

int main(void) {
  f(1, 2, 3.4, "this is why we can't have nice things");
}

where the function has no prototype and so is treated as a varargs call.

This is a bit of terminology, but something tells me you meant the variadic function here, right? As opposed to type parameter packs.

Yes, sorry for being unclear, I am talking about variadic functions.

Should we diagnose this? And if so, how? The variadic part is not represented (at least not at first glance?) in the AST. Understanding the insides of such a function would require either overapproximatic stuff and doing a looot of extra handling, or becoming flow sensitive... and we'd still need to understand all the va_ standard functions' semantics either way.

Well, that's what I'm wondering, really. The arguments are certainly easy to swap because the type system can't help the user to identify swaps without further information (like format specifier strings). However, the checking code would be... highly unpleasant, I suspect. My intuition is to say that we don't support functions without prototypes at all (we just silently ignore them) and that we only check the typed parameters in a variadic function declaration (e.g., we'll diagnose void foo(int i, int j, ...); because of the sequential int parameters, but we won't diagnose void foo(int i, ...); even if call sites look like foo(1, 2);). WDYT?

[whisperity]

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

178

It is definitely highly unpleasant, at one point I remember just glancing into the logic behind printf() related warnings in Sema and it was... odd, to say the least.

That is how the checker works right now. It will not diagnose int f() {} because from the looks of the function definition, it is a 0-parameter function. Same with the variadic ..., it is a single parameter function (which has a rather weird parameter type).

But yes, I think I'll make this explicit in the documentation (and FWIW, the research paper, too).

However, the checking code would be... highly unpleasant, I suspect.

Incredibly, because this check purposefully targets the function definition nodes only, like a cruise missile. For that logic, we would need to gather call sites for variadic functions and start doing some more magic with it.

FYI, templates are also handled in a way that only what is clear from definitions (and not instantiations... I think we can kind of call a particular "overload" call to a variadic like a template instantiation, just for the sake of the argument here) only:

template <typename T, typename U>
void f(T t1, T t2, U u) {} // [t1, t2] same type, WARN.

void g() { f(1, 2, 3); }} // NO-WARN: Even though "U" in this call is also "int", this is not fixed "easily enough" in the definition, so to prevent more false positives, we shut up.

template <>
void f(int i, int j, int k) {} // [i, k] WARN.

Anything that is only known through template instantiations is context-sensitive (the subsequent patch about typedef extends the tests and diagnostics about this), and thus, we forego trying to find the way.

template <typename T> struct vector { using value_type = T; };

template <typename T>
void f(T t, typename vector<T>::element_type u) {} // NO-WARN, dependent.

template <>
void f(int i, vector<int>::element_type i2) {} // WARN, can explicitly unfold the typedef and see "int == int".

template <typename T>
void g(typename vector<T>::element_type e1, typename vector<T>::element_type e2) {} // WARN, not dependent.

178

I definitely shall create and add a C file with test cases like this, marking them // NO-WARN, and explaining the reasoning. If for nothing else, maybe to know in the far future what can be improved upon.

[aaron.ballman]

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

178

I definitely shall create and add a C file with test cases like this, marking them // NO-WARN, and explaining the reasoning. If for nothing else, maybe to know in the far future what can be improved upon.

Excellent, that's the solution I was hoping we'd come to! I find it's useful to add the C test for a function without a prototype (void f()) because some code that expects all function declarations to have a function prototype will assert or crash, so it helps shake out some bugs.

Also, I agree with your interpretation of the template case -- it's another intractable problem at this stage, so best to document how we handle it and move on.

[steveire]

In D69560#2536597, @whisperity wrote:

In D69560#2536570, @steveire wrote:

I haven't read through all the comments, but the word 'easily' implies 'desirable'. This check seems to be for finding params which are undesirably swappable, right?

The easily was to mean that the swap can be done with little effort (i.e. "in an easy fashion"?) and by accident.

I understand that. The problem is the name. The signal-to-kill-thread check is bad. The erase check is inaccurate. The roundings check is incorrect. The operator-in-strlen-in-alloc check is misplaced. Those are all words that indicate the negative. Your name indicates the positive ("easily"). The name indicates that the swappable params are desirable.

Why "easily" instead of "suspicious", "spuriously" or any of the other words that are already used?

[whisperity]

In D69560#2549959, @steveire wrote:

Why "easily" instead of "suspicious", "spuriously" or any of the other words that are already used?

The name was @aaron.ballman's idea after we postponed the experimental-cppcoreguidelines-yaddayaddayadda....
The best I could come up with is potentially. The suspicious is a problem because this check is an interface check, not a call-site check. (The sister check is currently called suspiciously-swapped-argument or something like that.)

The original check idea from C++ Core Guidelines also uses the phrase:

Reason: Adjacent arguments of the same type are easily swapped by mistake.

However, potentially-swappable-parameters just sounds... wonky. It does not carry the weight the rule otherwise meant to.
weakly-typed-parameters?

I am not good with names, sadly.

---

I am conflicted about saying that "easily" always refers to some "positive aspect", however. Use a [[unnamed trademarked safety device]] because you can end up easily cutting off your finger during work. Do not touch a stove/hot water fountain, because you can easily get burnt. Leaving Christmas candles unattended could easily lead to a house fire. Does anyone want to get hurt, burnt, their house destroyed, etc.? There is nothing "positive" in these suggestions or regulations either. And we are doing a tool that is practically the same thing, trying to prevent a (different kind of) fire.

[whisperity]

• Added a C test file with some C-specific language constructs that we may or may not match.
• Explained limitations with regards to variadics, templates, etc. better in the docs, and added test cases for these too.
• When comparing the type names against the ignored type name list, do not consider PrintingPolicy, but take exactly what's in the source code. This mostly concerns bool and _Bool at face value, so this is tested in the C file too.

[whisperity]

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

256

Right, @aaron.ballman I think I got this tested. Please check the C test file. bool becomes _Bool in C code by the preprocessor, but the printing policy still says both should be printed as bool. I added some test cases about this. It is weird, because without doing some extra magic specifically against macros (which might break detecting type names in other locations), I can't really test ignoring _Bool but not ignoring bool. Now, the diagnostics respect the printing policy, but the type names are taken with an empty printing policy, which should give us the result as-is in the code. Unfortunately, "as-is" still means _Bool for bool's case, so even if you ignore bool, you will still get results where bool is written, because bool is a macro (Tidy automatically puts a note about this!) and the real type is _Bool, as per the macro expansion.

Wow... okay, that explanation got convoluted.
Anyways, it's really weird. But: we no longer take the actual printing policy anymore, but the default, which should eliminate all these cases.
The bool/_Bool case doesn't fail because of the printing policy, it fails because bool is a macro in <stdbool.h>.

Also, the type name ignore list not a full match, but a substring match ignore list. So if you have a type named struct Foo, if you ignore Foo, both struct Foo, Foo, MyFoo, etc. will be also ignored. And it's case sensitive.

[whisperity]

Made detection of macros as "parameter type names" more robust.

[whisperity]

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

256

Nevermind, I think I figured this out... or at least managed to figure it out partially... This "SpellingLoc" and "ExpansionLoc" and all this stuff is really convoluted, and there doesn't seem to be any good documentation for them.

[whisperity]

NFC (With all the restructuring and trying to figure out how to implement the latest change, I managed to upload the wrong, unclean diff...)

[whisperity]

Make sure that overloadable binary operators are ignored properly.

[whisperity]

• [NFC] Reformat the documentation so option values are in single backticks.

[whisperity]

@aaron.ballman I've reworked and uploaded the final addition to this line, the crown jewel, the very reason this checker was developed (D75041). So, AFAIK, the "let's rewrite the whole thing with better code quality" work can be considered done, and we may move on with the review.

[aaron.ballman]

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

55

const_reverse_iterator seems plausible here for the same reason as const_iterator.

57

Are you planning to remove the debugging code now that the check is approaching its final form?

364

Is this still needed in light of the comments from @riccibruno?

[whisperity]

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

57

Actually, I would prefer not to. I removed every debug thing possible. However, and this is speaking from experience because I wrote this check two times already from basically scratch... the rest of the debug code that is part of the patch has to be there. If anything goes nuts, especially if there would be false positives later... it would be impossible to figure out what is going on during the modelling without seeing all the steps being taken.

[whisperity]

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

364

It doesn't look like to me as if those patches they commented were ever merged, but I'll check it out.

[aaron.ballman]

In general, I'm happy with this check (and the follow-ups), so I'm giving it my LG. However, it's complex enough that I think having some extra review from @alexfh, @hokein, or one of the other reviewers would be a good idea, so please hold off on landing it for a bit in case any of the other reviewers want to weigh in.

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

57

We don't often leave debugging statements in because they tend to cause a maintenance burden that doesn't justify their benefit. However, I agree with your observation that this code is quite difficult to reason through without debugging aids.

I don't insist on removing the code yet, but would say we should remain open to the idea if it causes a burden in practice. (Either in terms of keeping the debugging code up to date as the check evolves, but also in terms of compile time performance for the check if the debugging code paths turn out to be expensive on build times.)

[whisperity]

In D69560#2629555, @aaron.ballman wrote:

[...] so please hold off on landing it for a bit in case any of the other reviewers want to weigh in.

Due to how the patch itself only being useful in practice if all the pieces fall into place, I will definitely keep circling about until the entire patch tree is ✔. (Including the filtering heuristics, etc.)

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

57

All debugging code is wrapped into the LLVM_DEBUG macro specifically (that's why I even put this little "print bits" function behind NDEBUG) so they are not part of the builds.

I think in general if someone puts the effort into reading the code and has an interactive debugger they could figure it out (especially if they keep track of the recursion constantly), I tried to make everything nicely padded and all the variable names and control flow to make sense. (Of course the wealth of complexity comes in the follow-up patches.)

[aaron.ballman]

In D69560#2629616, @whisperity wrote:

In D69560#2629555, @aaron.ballman wrote:

[...] so please hold off on landing it for a bit in case any of the other reviewers want to weigh in.

Due to how the patch itself only being useful in practice if all the pieces fall into place, I will definitely keep circling about until the entire patch tree is ✔. (Including the filtering heuristics, etc.)

Good idea. :-)

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

57

All debugging code is wrapped into the LLVM_DEBUG macro specifically (that's why I even put this little "print bits" function behind NDEBUG) so they are not part of the builds.

They're part of debug builds still (which is the build configuration I tend to use most often). That said, I think it's fine to leave the debugging code in for now, especially as the check is being actively worked on.

[whisperity]

• Add const reverse iterator to the default-ignored typename list.

[whisperity]

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

364

I checked at they haven't been merged yet. Nonetheless, we can remove this logic later once that logic is in.

I added a test case for unnamed parameters with the current logic printing <unnamed>, so if the underlying call will return something like <unnamed parameter @ 24:48> we will know because that test will blow up.

[aaron.ballman]

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

364

Thank you for checking and adding some test coverage for it!

[whisperity]

Rebased over D98635, allowing us to properly highlight the found "mixable adjacent parameter range" (assuming it's on the same line, of course):

[whisperity]

Made sure that the parameter's name is highlighted promptly in the "first|last parameter of the range is..." note.

[alexfh]

Thanks for the great work! This is mostly looking fine, but I've added a few comments. I could only make a quite superficial review so far, but I'll try to take a deeper look into this soon.

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

70

I think, this function is not needed (see the comment below), but if it is, a single std::string and in-place std::reverse() would be better, imo.

84–87

I suppose that textual representation of the flags would be easier to understand in debug logs than just bits. Maybe single characters or other abbreviations, if this needs to be compact.

113–120

I'd switch to scoped enums (enum class), remove the macro, and use simpler code like None = 0x1, Trivial = 0x2, Canonical = 0x4, etc.

133

What should this method do and how it should be used? Same for Mix::sanitize()

295–297

Would llvm::find(Check.IgnoredParameterNames, NodeName) != Check.IgnoredParameterNames.end() work?

302

No need for parentheses here.

313–320

Is there a chance that you're trying to reimplement a part of Lexer::makeFileCharRange functionality here?

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

129

Does the top-level const change anything with regard to the "mixability"? It's a purely implementation-side difference, callers could still swap parameters.

[alexfh]

BTW, any ideas why "patch application failed"? (See the Build status in the Diff Detail section)

It would be really nice to get pre-merge checks to work for the patch.

[whisperity]

In D69560#2747092, @alexfh wrote:

BTW, any ideas why "patch application failed"? (See the Build status in the Diff Detail section)

It would be really nice to get pre-merge checks to work for the patch.

Put simply, I think I haven't rebased the patch since a few changes ago. The past few changes were NFC/lint stuff only.

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

113–120

enum class is a good idea!

Unfortunately, I'm not really sold on the hexa literals... It makes the code confusing the read, because you have to keep in mind which range the hexa literals mean, and when there is a shift in position, e.g. if you read 0x20 you have to count out that okay, that's 2nd position, so at least 16, so at least the 4th bit, but it is two, so you shift it again to the right, so in the bit pattern it turns on the 5th bit actually.
It makes it very easy to misread or misinterpret things, especially when trying to work on the source code.
(In the last extension patch, the number of elements in the bit mask go up to the 8th or the 9th bit, I don't remember exactly at the top of my head.)

Compared to this, the current text when read immediately tells you which "decimal" position you should expect the appropriate bit to be toggled on, if the flag is there.

Binary literals, on the other hand, would be amazing to use here, sadly we're only in C++14.

133

Once there are more flags in the bitmask (not just "None" and "Trivial"), this method deals with sanitising the flags that are toggled on and off by the recursive descent on the type tree.

For example, if there is a typedef involved (see D95736), it will toggle on the typedef flag, and start investigating the aliased type. This subsequent investigation might figure out that the aliased type is trivial, or it is distinct. In case it is distinct, that level of the recursion will turn on the "Not mixable" flag. In this case, when the investigation returns completely, we will have "Typedef" and "None" both turned on. This method will throw away everything but the "None" bit, so when the data is returned to the diagnostic-generating part of the check, it will know what to do.

313–320

Does that expand the token length properly? I took this idiom of getLocForEndOfToken from various other Tidy checks... basically, anything that deals with replacing some sort of textual range in the code calls to this function. I need to have the exact ending location so I can obtain the full text, as appears in the source file. In this case, to compare against the user's input.

E.g. here is one example from utils/UsingInserter.cpp:

  32   │ Optional<FixItHint> UsingInserter::createUsingDeclaration(
  33   │     ASTContext &Context, const Stmt &Statement, StringRef QualifiedName) {
/* ... */
  42   │   SourceLocation InsertLoc = Lexer::getLocForEndOfToken(
  43   │       Function->getBody()->getBeginLoc(), 0, SourceMgr, Context.getLangOpts());

A quick grep for makeCharRange in the repository didn't turn up any significant result, apart from one unit test.

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

129

Unfortunately, coding styles differ whether people mark local variables const or not, across projects. Even with pointers: in some cases, a swapped call to memcpy might be caught if the user made one of the pointers passed already const void* instead of just void*.

D95736 deals with implementing the logic for this, it is exposed as a user-configurable option whether they want to consider const stuff the same as non-const stuff.

[whisperity]

There was a bit of talk on Twitter recently about strong typing which reminded me of this checker! 😁 I'll fix all issues in one go once we're through with all the comments, I'd like to see the full picture of what needs to be done.