Page MenuHomePhabricator

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

Authored by whisperity on Oct 29 2019, 6:13 AM.

Details

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.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
whisperity updated this revision to Diff 322363.Feb 9 2021, 5:58 AM

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

Make sure that overloadable binary operators are ignored properly.

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

@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 added inline comments.Mar 16 2021, 8:38 AM
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 added inline comments.Mar 16 2021, 10:21 AM
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 added inline comments.Mar 16 2021, 10:25 AM
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 accepted this revision.Mar 16 2021, 10:34 AM

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.)

This revision is now accepted and ready to land.Mar 16 2021, 10:34 AM

[...] 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.)

[...] 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 edited reviewers, added: njames93; removed: Szelethus.Mar 16 2021, 11:50 AM
whisperity marked 6 inline comments as done.
whisperity added a subscriber: Szelethus.
  • Add const reverse iterator to the default-ignored typename list.
whisperity added inline comments.Mar 17 2021, 5:31 AM
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 added inline comments.Mar 17 2021, 5:42 AM
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
364

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

whisperity marked an inline comment as done.Mar 18 2021, 7:37 AM
whisperity set the repository for this revision to rG LLVM Github Monorepo.

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

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

alexfh added a comment.May 9 2021, 8:34 PM

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
71

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.

85–88

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.

114–121

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

134

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

296–298

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

303

No need for parentheses here.

314–321

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
130

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 added a comment.May 9 2021, 8:38 PM

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.

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
114–121

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.

134

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.

314–321

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
130

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.

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.

whisperity marked 8 inline comments as done.

All changes are NFC and styling only.

  • Remove the FB macro in favour of explicitly specifying the bit flag value
  • Change debug printout from bit pattern to individual significant letters

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

I think I figured out why the pre-build CI isn't working... it seems that the fact that this patch depends on an abandoned revision D76545 misleads the CI logic as it sees the dependency, and even though it is abandoned, tries to download and apply the contents of D76545, which makes it fail...

INFO    D76545#251840 commit 1e9b6b89a7b5c49612018b120c2c142106056f82 has a later commit date then7cdbf1ed4b94259a3aad2a7575e928fa61b0e57e
[...]
ERROR   error: patch failed: clang-tools-extra/docs/clang-tidy/index.rst:62
error: clang-tools-extra/docs/clang-tidy/index.rst: patch does not apply
error: patch failed: clang-tools-extra/clang-tidy/ClangTidyForceLinker.h:15
error: clang-tools-extra/clang-tidy/ClangTidyForceLinker.h: patch does not apply

(Try nudging the CI to only apply this commit after removal of parent links.)

Attempt to fix build failure on Windows buildbot (operator new was taking unsigned long instead of size_t in a test which resulted in hard error).

aaron.ballman accepted this revision.Jun 25 2021, 5:18 AM

This is a very involved check that I think is going to produce some interesting results for users, thank you for working so diligently on it! I think all of the patches in the series have now been accepted and this is ready to land. @whisperity, are you aware of any changes that are left to be made?

This is a very involved check that I think is going to produce some interesting results for users, thank you for working so diligently on it! I think all of the patches in the series have now been accepted and this is ready to land. @whisperity, are you aware of any changes that are left to be made?

I'll ping @martong privately if they have additional comments about D75041, but otherwise, no, I think no more things left to do. Landing checklist complete.

This is a very involved check that I think is going to produce some interesting results for users, thank you for working so diligently on it! I think all of the patches in the series have now been accepted and this is ready to land. @whisperity, are you aware of any changes that are left to be made?

I'll ping @martong privately if they have additional comments about D75041, but otherwise, no, I think no more things left to do. Landing checklist complete.

Great! Unless you hear from @alexfh, @martong, or someone else with additional concerns, I think you're set to land this!

I thought I sent this batch of comments loong ago =\

Better late than never.

clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
114–121

First of all, do we ever need to know the position of the set bit in these flag enumerators? Isn't it enough that each enumerator has a distinct bit set? If so, there's not much complexity in reading and expanding sequences like 0x0001, 0x0002, 0x0004, 0x0008, 0x0010, 0x0020, 0x0040, 0x0080, 0x0100, 0x0200, ....
However, if that's not to your taste, there's another frequently used style of defining flag enumerations in LLVM: 1 << 0, 1 << 1, 1 << 2, 1 << 3, ....

Either of these is fine, but using macros for this purpose is not necessary and doesn't make the code any easier to read or to modify.

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.

Do I understand correctly that you need this exclusively for the purpose of reading debug logs? If so, it seems to be better to decode the bits before logging them. See the comment on line 87 above.

134

Thanks for the explanation! But it would be very helpful to have this information in the code, not only in a code review comment.

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

The const in const void * is not top-level, it applies to the pointed at type. A top-level const here would be const void * const (a constant pointer to a constant void) or void * const - a const pointer to a non-const void. Now getting back to the original question. Consider this function:

void qualified1(int I, const int CI) {}

The top-level const for the second parameter isn't a part of the function signature and doesn't help with prevention of the unintentionally swapped parameters. I guess, the same goes for top-level volatile.

Whatever arguments x and y are passed to qualified1 they can be swapped without being noticed.

whisperity marked 2 inline comments as done.Jun 29 2021, 1:59 AM
whisperity added inline comments.
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
114–121

Yes, there individual positions themselves don't matter! The only thing that matters is that some flags are < than None and the rest are >=, but that's only needed because of the ugly workaround.

I turned the debug printouts to use a manual formatting with "reasonable" characters being set (e.g. T for Trivial or & for reference). It's much more readable and straight-forward, because you actually do not need to know the position of the bits themselves, those don't matter.

134

The full patch stack landed, and this function has a much more elaborated implementation (with more comments), I believe. In this patch, there was indeed nothing to add here.

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

True. We can think about this later. The modelling logic would be made even more complicated by handling this, however, and the reports themselves would become asymmetric, because to understand the reports, users would need to actively think about this particular example (whether something is a "top-level const" or a "non-top-level const").

I know the check has almost been made into a CppCG checker (which in the strictest case says to only warn if something has the same type) and then we backtracked from it, some of these design decisions are hereditary from the previous reviews and development process.

I think the fact that there is a useful way from the user's perspective to toggle this is beneficial for two reasons:

  • By default, the number of warnings are smaller. To an already verbose checker as it is, this is a clear upside.
  • Even if mathematically there is no difference between I and CI from a language standpoint, if the const is there, it could be indicative of the user meaning something (similar to how I tried indicating a few things by making local variables const, and then during the review of the patch, I was told to not do that. It seems Tidy uses this coding style (something I do not personally agree with, local const variables prevent me from mistakes too!), where there are other project that use a different style). So in essence, this still allows us to respect coding styles in some way. By making one of the variables const, you wanted to indicate something - something that the checker can pick up on, and give you the choice to not start yelling immediately. At least when you're using the default settings and not tuning the individual options.
RKSimon added inline comments.
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
57

@whisperity There is a missing comma at the end of the "const_reverse_iterator" line causing implicit concatenation with the following "Constreverseiterator" line.

whisperity marked 3 inline comments as done.Aug 24 2021, 7:13 AM
whisperity added inline comments.
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
57

Thanks... I'll get around to hotfixing this ASAP, and hope it can be backported to 13.0...

(I wish there was a checker for this? 🙄)

whisperity marked an inline comment as done.Aug 28 2021, 7:03 AM