[clang-tidy] Add 'experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type' check

[whisperity]

Summary

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

The following construct might be the result of insufficient design, as capabilities of the type system are not used to their full extent to ensure that at a potential call site no possible argument swap happens.

file open(string_view directory, string_view filename, string_view extension)

The relevant I.24 C++ Core Guidelines rule to which conformity is checked can be found at http://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#i24-avoid-adjacent-unrelated-parameters-of-the-same-type

[whisperity]

A few interesting "true positive" findings:

• AnalysisDeclContext.cpp_9aaed563ddd9ebd73fdd228c2883b8e7.plist.html226 KBDownload
  (Such cases with many bools are being discussed on enhancing type strength)
• bitcoingui.cpp_27769deaa63c894e4fc430d7122c368d.plist.html257 KBDownload
  (Personally, I found this case a rather big find.)
• CheckerRegistry.cpp_d0d988840a154a07670cc410e23a7c8e.plist.html226 KBDownload
• text_format.cc_f34502df2725b30018c086b6767cbd26.plist.html373 KBDownload

Of course, an "interface conformity check" will find "false positive" cases which cannot be proved or done away by a heuristic easily:

FP_substitute.cc_7bdfe1e94aaced3286a6be8172a92884.plist.html209 KBDownload

I attach a render of the check's documentation so it's easy to read if you don't want to manually build the docs:

clang-tidy - cppcoreguidelines-avoid-adjacent-arguments-of-same-type — Extra Clang Tools 10 documentation (2019-10-29 13_59_17).html18 KBDownload

[Eugene.Zelenko]

clang-tools-extra/clang-tidy/cppcoreguidelines/AdjacentArgumentsOfSameTypeCheck.cpp

1 ↗

(On Diff #226883)

Please adjust length to 80 characters.

9 ↗

(On Diff #226883)

Please C++ headers aster LLVM ones.

35 ↗

(On Diff #226883)

Functions should be static. Anonymous namespace only for types definitions.

488 ↗

(On Diff #226883)

Check seems to be useful for C and probably for Objective-C.

clang-tools-extra/docs/ReleaseNotes.rst

147

Please remove This check.

clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-avoid-adjacent-arguments-of-same-type.rst

6 ↗

(On Diff #226883)

Please remove This check.

56 ↗

(On Diff #226883)

Please use single back-tick for option values.

65 ↗

(On Diff #226883)

Please use single back-tick for option values.

125 ↗

(On Diff #226883)

Please add new line.

[whisperity]

@Szelethus and @baloghadamsoftware are colleagues to me whom are far more knowledgeable about check development and I want them to see that I want a review from them. I specifically didn't do an "internal with colleagues" downstream review with regards to this code.

clang-tools-extra/clang-tidy/cppcoreguidelines/AdjacentArgumentsOfSameTypeCheck.cpp

488 ↗

(On Diff #226883)

I'm not knowledgeable about Objective-C at all to make a decision on how the "adjacent argument ranges" could be calculated and what mixups are possible. As for C, should a cppcoreguidelines- check be enabled for C? Or you mean we should allow it to work, and the user will toggle how they see fit.

[whisperity]

clang-tools-extra/clang-tidy/cppcoreguidelines/AdjacentArgumentsOfSameTypeCheck.cpp

488 ↗

(On Diff #226883)

I've added a FIXME for ObjC as I'm really not qualified in that language. C support has been added.

[whisperity]

• Fix some comments, formatting and documentation
• Organised test files to be in the same directory as others on the Monorepo structure
• Helper functions moved from namespace (anonymous) to static.
• Added test for C and enabled check for C code.

[whisperity]

• Removed This check from the documentation comment of the check's class too.

[NoQ]

clang-tools-extra/clang-tidy/cppcoreguidelines/AdjacentArgumentsOfSameTypeCheck.cpp

488 ↗

(On Diff #226883)

The check should keep working on Objective-C to check C-style calls, and it should also keep working on Objective-C++ to check C++-style method calls. You should be able to test this by renaming any of your .c/.cpp test files to .m/.mm (of course you don't *have* to duplicate your tests; just keep a couple of functions to see that the warning is still there).

However, the check shouldn't try to check Objective-C message expressions / method declarations, because Objective-C message syntax has so-called "parameter labels": the caller is forced to spell out which parameter it's passing, which effectively mitigates the problem.

Note that ObjCMethodDecl is not a sub-class of FunctionDecl, so your checker is already working correctly (as long as you remove the explicit suppression).

[whisperity]

There are a few really minor bug fixes, test additions, documentation update, etc. coming along soon, but I've some more pressing matters. However, please feel free to review the patch as-is!

[vingeldal]

Nice! Do you have any numbers on the amount of false positives this check produces, or the ratio of false and true positives?
I recently started work on this check myself but didn't get very far before I discovered this patch and abandoned mine.
Before abandoning I got some feedback though; there were concerns that there would be way to many false positives.

There were requests both to add heuristics to minimize false positives, as I see you have done nicely,
and to provide some data as to how the check performs with regard to false positives.

clang-tools-extra/clang-tidy/cppcoreguidelines/AdjacentArgumentsOfSameTypeCheck.cpp

474 ↗

(On Diff #227254)

Am I getting this right, is this the number of consecutive arguments of the same type which is required for this check to print a diagnostic? If so: why not set the default value to 2? -I think that's what a user who just read the C++ Core Guidelines would expect.

[whisperity]

I'd rather not call them false positive because the definition of false positive is ugly and mushy with regards to this check. This check attempts to enforce an interface rule, whether you(r project) wants to adhere the rule is a concise decision. A type equivalence (or convertibility in case of the follow-up patch D75041 that considers implicit conversions) should be a "true positive" in every case, as an indicator of potentially bad design.

However, there's the question whether or not the similar argument-ness is intentional, i.e. a (from one PoV flawed) but deliberate design decision by the project. We've tested a few projects. The ugliest I've found is OpenCV where every variable of a function argument is either an _InputArray or an _OutputArray...

Another question is, how fragmented you want your type system to be...
I hope my research into this topic will be at least something fruitful. It's all about compromises, one end if using any for all your variables, the other extreme is having a specific type for every single occurrence (stuff like fabs_return_t, create_schema_name_t and stuff).

There's a similar patch D20689 but that uses names (and thus requires that arguments and parameters to be "named" in some way).

clang-tools-extra/clang-tidy/cppcoreguidelines/AdjacentArgumentsOfSameTypeCheck.cpp

474 ↗

(On Diff #227254)

Well, it is to reduce the number of nagging errors. Setting it to 2 would result in every "min" and "pow" and similar functions to match. It's a compromise.

[whisperity]

In D74463#1888270, @aaron.ballman wrote:

I am also concerned about the false positives from this check because I don't think there's going to be an easy heuristic for determining whether two identifiers are "related" to one another.

I agree, and this is an unfortunate thing, but I do not think there could be a sensible heuristic here. Unless we somehow flesh out D20689 which uses name checks. (The two checks could actually complement each other - this one is for users who want to design their APIs defensively and want to see the changes during their code's evolution.)

In D74463#1888270, @aaron.ballman wrote:

There is no obvious way to silence any of the diagnostics generated as false positives short of requiring a naming convention for users to follow, which is not a particularly clean solution.

There is, you decide not to enable the checker?

[aaron.ballman]

In D69560#1889483, @whisperity wrote:

I'd rather not call them false positive because the definition of false positive is ugly and mushy with regards to this check. This check attempts to enforce an interface rule, whether you(r project) wants to adhere the rule is a concise decision. A type equivalence (or convertibility in case of the follow-up patch D75041 that considers implicit conversions) should be a "true positive" in every case, as an indicator of potentially bad design.

The problem is that there *are* false positives, logically. The rule is about "unrelated" parameters, so the false positives come from not having any way to distinguish between related and unrelated arguments. My intuition is that the simple enforcement suggested by the rule is unlikely to be acceptable, but that's why I'm asking for data over large, real-world projects.

However, there's the question whether or not the similar argument-ness is intentional, i.e. a (from one PoV flawed) but deliberate design decision by the project. We've tested a few projects. The ugliest I've found is OpenCV where every variable of a function argument is either an _InputArray or an _OutputArray...

Another question is, how fragmented you want your type system to be...
I hope my research into this topic will be at least something fruitful. It's all about compromises, one end if using any for all your variables, the other extreme is having a specific type for every single occurrence (stuff like fabs_return_t, create_schema_name_t and stuff).

Agreed -- if we can find a heuristic that makes this work, the check has a lot of value.

In D74463#1888270, @aaron.ballman wrote:
There is no obvious way to silence any of the diagnostics generated as false positives short of requiring a naming convention for users to follow, which is not a particularly clean solution.

There is, you decide not to enable the checker?

That is not a useful level of granularity given that this check is likely to be incredibly noisy as part of our other C++ Core Guideline enforcement. We typically have NOLINT comments that we can fall back on for silencing individual instances of a false positive, but that's not likely to be a workable solution by itself for this check in particular because of system and third-party headers which are outside of the user's control (and unlikely to be willing or able to modify).

[vingeldal]

In D69560#1889719, @aaron.ballman wrote:

In D69560#1889483, @whisperity wrote:

I'd rather not call them false positive because the definition of false positive is ugly and mushy with regards to this check. This check attempts to enforce an interface rule, whether you(r project) wants to adhere the rule is a concise decision. A type equivalence (or convertibility in case of the follow-up patch D75041 that considers implicit conversions) should be a "true positive" in every case, as an indicator of potentially bad design.

The problem is that there *are* false positives, logically. The rule is about "unrelated" parameters, so the false positives come from not having any way to distinguish between related and unrelated arguments. My intuition is that the simple enforcement suggested by the rule is unlikely to be acceptable, but that's why I'm asking for data over large, real-world projects.

I agree with this, the way I see it there are false positives and there is nothing "mushy" about the definitions of those.
Though I would prefer that, even if the implementation suggested in the guideline would be to crude, any deviations from what the C++ Core Guidelines specifies must be explicitly chosen by the user and not an option default.

However, there's the question whether or not the similar argument-ness is intentional, i.e. a (from one PoV flawed) but deliberate design decision by the project. We've tested a few projects. The ugliest I've found is OpenCV where every variable of a function argument is either an _InputArray or an _OutputArray...

Another question is, how fragmented you want your type system to be...
I hope my research into this topic will be at least something fruitful. It's all about compromises, one end if using any for all your variables, the other extreme is having a specific type for every single occurrence (stuff like fabs_return_t, create_schema_name_t and stuff).

Agreed -- if we can find a heuristic that makes this work, the check has a lot of value.

In D74463#1888270, @aaron.ballman wrote:
There is no obvious way to silence any of the diagnostics generated as false positives short of requiring a naming convention for users to follow, which is not a particularly clean solution.

There is, you decide not to enable the checker?

That is not a useful level of granularity given that this check is likely to be incredibly noisy as part of our other C++ Core Guideline enforcement. We typically have NOLINT comments that we can fall back on for silencing individual instances of a false positive, but that's not likely to be a workable solution by itself for this check in particular because of system and third-party headers which are outside of the user's control (and unlikely to be willing or able to modify).

Doesn't clang-tidy exclude warnings from system headers by default though?
And there is always the possibility of using multiple .clang-tidy files in the code base to use this check for only selected portions of the code base.
Also I'm convinced this check would already be of use in many code bases, as it is today, (but sure, as you said, supporting data for that would be good) and therefor it would be a clear improvement to clang-tidy.
It would be better to use this check until we can do even better, rather than to hold of the check completely until we can implement it perfectly for every code base.

Would it not be a reasonable approach to accept this implementation and improve NOLINT to work on entire files or lists of files rather than squeezing more heuristics (prone to false negatives) to the check?

clang-tools-extra/clang-tidy/cppcoreguidelines/AdjacentArgumentsOfSameTypeCheck.cpp

474 ↗

(On Diff #227254)

I think this value very well may strike a good balance in that compromise but I still think it is a clear deviation from the guideline as specified.
IMO all deviations from the guideline should be explicitly requested by the user, not set by default, no matter how useful that deviation is.

[aaron.ballman]

In D69560#1889925, @vingeldal wrote:

Doesn't clang-tidy exclude warnings from system headers by default though?

Third-party SDKs are not always brought in as system headers, unfortunately. Even ignoring system and third-party headers, having two parameters of the same type is a natural occurrence for some data types (like bool) where it is going to be extremely hard to tell whether they're related or unrelated parameters.

And there is always the possibility of using multiple .clang-tidy files in the code base to use this check for only selected portions of the code base.

That is an option, but it would be better for the check to be usable without requiring a lot of custom configuration.

Also I'm convinced this check would already be of use in many code bases, as it is today, (but sure, as you said, supporting data for that would be good) and therefor it would be a clear improvement to clang-tidy.

I'm not as convinced, tbh.

It would be better to use this check until we can do even better, rather than to hold of the check completely until we can implement it perfectly for every code base.

Incremental improvement is always a good thing, but it has to start from a stable base. I'm not yet convinced this check meets the bar for inclusion in clang-tidy. There is no proposed way to tell how two parameters relate to one another, and the simple enforcement seems dangerously simple.

Would it not be a reasonable approach to accept this implementation and improve NOLINT to work on entire files or lists of files rather than squeezing more heuristics (prone to false negatives) to the check?

AFAICT, that would be a novel new use of NOLINT, so I'm wary of it. I think the first thing is to gather some data as to how this check performs as-is. If the behavior is unreasonable, we can see what ways in which it is unreasonable with the dataset and see if we can tease out either heuristics or configuration options that would make it more palatable. We may also want to rope some of the C++ Core Guidelines authors in on the discussion at that point because that would be data suggesting their suggested simple enforcement is untenable.

[vingeldal]

In D69560#1890026, @aaron.ballman wrote:

In D69560#1889925, @vingeldal wrote:

Doesn't clang-tidy exclude warnings from system headers by default though?

Third-party SDKs are not always brought in as system headers, unfortunately. Even ignoring system and third-party headers, having two parameters of the same type is a natural occurrence for some data types (like bool) where it is going to be extremely hard to tell whether they're related or unrelated parameters.

And there is always the possibility of using multiple .clang-tidy files in the code base to use this check for only selected portions of the code base.

That is an option, but it would be better for the check to be usable without requiring a lot of custom configuration.

One can't really expect a ones own .clang-tidy file to be applicable to all third party code used in ones project. It is a fact, with or without this check, that if you use third party libraries you can't expect the third party libraries to comply with your own .clang-tidy file. You should expect to set up a separate .clang-tidy file for all third party libraries used by your project with or without this check -and that's ok because it is arguably not a big effort to do so.

Incremental improvement is always a good thing, but it has to start from a stable base. I'm not yet convinced this check meets the bar for inclusion in clang-tidy. There is no proposed way to tell how two parameters relate to one another, and the simple enforcement seems dangerously simple.

Would it not be a reasonable approach to accept this implementation and improve NOLINT to work on entire files or lists of files rather than squeezing more heuristics (prone to false negatives) to the check?

AFAICT, that would be a novel new use of NOLINT, so I'm wary of it. I think the first thing is to gather some data as to how this check performs as-is. If the behavior is unreasonable, we can see what ways in which it is unreasonable with the dataset and see if we can tease out either heuristics or configuration options that would make it more palatable. We may also want to rope some of the C++ Core Guidelines authors in on the discussion at that point because that would be data suggesting their suggested simple enforcement is untenable.

There is also the --header-filter command line option to configure the tool to skip some headers.

To sum up my position:
I'm all for investigating the performance of this check and basing any decisions on data. Until we have that data I just want to challenge the claim that there are no suitable ways to turn the check of for select portions of the code base,
there are definitely ways to do that which both work satisfyingly and are in no way novel or unconventional.

[whisperity]

In D69560#1890026, @aaron.ballman wrote:

In D69560#1889925, @vingeldal wrote:

Doesn't clang-tidy exclude warnings from system headers by default though?

Third-party SDKs are not always brought in as system headers, unfortunately. Even ignoring system and third-party headers, having two parameters of the same type is a natural occurrence for some data types (like bool) where it is going to be extremely hard to tell whether they're related or unrelated parameters.

I have specifically applied the matcher in this patch in a way that it only matches for the definition of the function symbol. If you never create a build database out of the third party "package" and run the check on the third-party library's source code, you will not get a warning for these.

While it's somewhat wonky that an "interface" rule is matching the "implementation" nodes, it is a natural restriction to only warn the user about things they have a reliable way of fixing. Hopefully, they will not forget to align their headers after potentially fixing the implementation code.

[whisperity]

In D69560#1889925, @vingeldal wrote:

I think this value very well may strike a good balance in that compromise but I still think it is a clear deviation from the guideline as specified.
IMO all deviations from the guideline should be explicitly requested by the user, not set by default, no matter how useful that deviation is.

While I am still planning to compile a measurement with potentially tagging false and true positives -- albeit classifying this on random projects I have no domain knowledge about is hard at first glance...

@aaron.ballman Could it be a useful compromise to not say we are trying to solve the cppcoreguidelines- rule, but rather introduce this checker in another "namespace" and maybe mention in the docs that it could, although with caveats, apply "parts" of the Core Guidelines rule?

[aaron.ballman]

In D69560#1890767, @vingeldal wrote:

In D69560#1890026, @aaron.ballman wrote:

That is an option, but it would be better for the check to be usable without requiring a lot of custom configuration.

One can't really expect a ones own .clang-tidy file to be applicable to all third party code used in ones project. It is a fact, with or without this check, that if you use third party libraries you can't expect the third party libraries to comply with your own .clang-tidy file. You should expect to set up a separate .clang-tidy file for all third party libraries used by your project with or without this check -and that's ok because it is arguably not a big effort to do so.

We typically aim for checks to require as little configuration as possible by tailoring the default check behavior to be as reasonable as we can make it.

Incremental improvement is always a good thing, but it has to start from a stable base. I'm not yet convinced this check meets the bar for inclusion in clang-tidy. There is no proposed way to tell how two parameters relate to one another, and the simple enforcement seems dangerously simple.

Would it not be a reasonable approach to accept this implementation and improve NOLINT to work on entire files or lists of files rather than squeezing more heuristics (prone to false negatives) to the check?

AFAICT, that would be a novel new use of NOLINT, so I'm wary of it. I think the first thing is to gather some data as to how this check performs as-is. If the behavior is unreasonable, we can see what ways in which it is unreasonable with the dataset and see if we can tease out either heuristics or configuration options that would make it more palatable. We may also want to rope some of the C++ Core Guidelines authors in on the discussion at that point because that would be data suggesting their suggested simple enforcement is untenable.

There is also the --header-filter command line option to configure the tool to skip some headers.

That is true.

To sum up my position:
I'm all for investigating the performance of this check and basing any decisions on data. Until we have that data I just want to challenge the claim that there are no suitable ways to turn the check of for select portions of the code base,
there are definitely ways to do that which both work satisfyingly and are in no way novel or unconventional.

Using the header filters would be a reasonable approach, if a bit heavy-handed. Ideally, the check would warn about what the rule wants it to warn about. I think the unsatisfying bit is the lack of clarity in the rule itself. Trying to write a checker for a rule with that much subjectivity typically leads to a lot of configuration needs.

In D69560#1890855, @whisperity wrote:

In D69560#1889925, @vingeldal wrote:

I think this value very well may strike a good balance in that compromise but I still think it is a clear deviation from the guideline as specified.
IMO all deviations from the guideline should be explicitly requested by the user, not set by default, no matter how useful that deviation is.

While I am still planning to compile a measurement with potentially tagging false and true positives -- albeit classifying this on random projects I have no domain knowledge about is hard at first glance...

LLVM itself might be a good starting point since we've all got some passing familiarity with the project.

@aaron.ballman Could it be a useful compromise to not say we are trying to solve the cppcoreguidelines- rule, but rather introduce this checker in another "namespace" and maybe mention in the docs that it could, although with caveats, apply "parts" of the Core Guidelines rule?

I think that could be a useful compromise, or name it cppcoreguidelines-experimental-avoid-adjacent-parameters-of-same-type to make it clear that we're trying things out to see what works in practice and the behavior may not match the guideline. I think the most important thing we do will be to eventually coordinate with the C++ Core Guidelines folks though. The eventual goal is for the clang-tidy check to cover what the guideline wants it to cover as its default behavior, but in a way that's going to give acceptable behavior for our users. This may mean we have a slightly chattier diagnostic than I'd like, and it may also mean that some updates to the core guideline are needed to help clarify what constitutes a "related" parameter in a way that's acceptable for analysis tools. What I want to avoid is having users say "this check doesn't match the guideline" or "we want to enable all the guidelines but we always have to disable this one because of the false positives".

Btw, we should update the terminology in the patch to use parameter instead of argument (parameters are what the function declares, arguments are what are passed in at the call site and they bind to parameters -- the issue this check is trying to solve is on the declaration side, not the caller side).

[whisperity]

@aaron.ballman @vingeldal I'll go over the findings (as I've run this on LLVM and various other projects, a few examples are uploaded in my first comment as HTML renders of the bug report!) soon, but I want to wait until a research paper I have based on this topic gets a final decision. (It should happen soon.) I also plan to refurbish D20689 and have it upstreamed as a "co-checker" of this (one for the interface rule, one for the call sites, they kinda want to solve different aspects of the same issue, let it be up for the project how much they wish to enforce).

[aaron.ballman]

In D69560#1891185, @whisperity wrote:

@aaron.ballman @vingeldal I'll go over the findings (as I've run this on LLVM and various other projects, a few examples are uploaded in my first comment as HTML renders of the bug report!) soon, but I want to wait until a research paper I have based on this topic gets a final decision. (It should happen soon.) I also plan to refurbish D20689 and have it upstreamed as a "co-checker" of this (one for the interface rule, one for the call sites, they kinda want to solve different aspects of the same issue, let it be up for the project how much they wish to enforce).

Sounds great, thank you!

[whisperity]

In D69560#1891167, @aaron.ballman wrote:

Btw, we should update the terminology in the patch to use parameter instead of argument (parameters are what the function declares, arguments are what are passed in at the call site and they bind to parameters -- the issue this check is trying to solve is on the declaration side, not the caller side).

Indeed, I think I got confused because the rule heading says "parameter" but immediately the line following says "argument". I'll rename the check, also potentially rebase and all that, it should be a trivial thing to do.
My bigger concerns are about the subsequent patch which makes this rule matching order of magnitude more powerful is implicit conversions. See D75041 for that.

(Originally, the entire idea we came up with was more of a local brainstorming, and it was only during development that I realised that there is a (somewhat?) matching guideline rule about this.)

[zporky]

Agree that we could make an attempt to detect "related" parameters.
A possible heuristic to detect "related arguments" would be to look for interactions between these arguments, e.g. check whether we use them in specific operations, e.g.

// tipical for related iterators
par1 == par2
par1 != par2
etc

// tipical for pointers
par1 - par2
*par1 = *par2

and similar.

[whisperity]

@aaron.ballman I've gone over LLVM (and a few other projects). Some general observations:

• Length of 2 is vile. I understand that the C++CG rule says even lengths of 2 should be matched, but that is industrially infeasible unless one introduces such a rule incrementally to their project. Findings of length 2 are , in general, an order of magnitude more than... basically the rest of the findings.
  • On big projects, even the current "default" of "length 3" seems to be too low. In reality, one should consider (this is likely to be out of scope for Tidy) how often these functions are called, and various other metrics on how serious an offender is.
  • LLVM: 7206 len-2 findings, 767 len-3, 194 len-4, 59 len-5, etc.)
  • OpenCV: 4391 len-2, 987 len-3, 507, 115, ...
    • I did not manually evaluate OpenCV, and I think OpenCV is a project on which - due to their seeming design principle of type erasure - this check is useless.
    • (Similar could be said about LLVM, we are cutting a lot of corners on "niceness" for absolute bleeding-edge performance!)
• Most of the findings come from low-level types, such as bool or int or unsigned or some project or library-specific (such as a few cases of SDVal in LLVM) types.
• Strings and string-like types (such as StringRef, char *, etc.) are also huge offenders.
  • Some of these offenders can be side-stepped with semantic typedefs (D66148)
• Further heuristics are definitely needed, but this patch is huge enough as is, I'd introduce further heuristics in future patches:
  • Allow the user to configure the set of ignored types and variable names from the command-line (I think this already exists as a FIXME in the code as of now)
  • Relatedness check for parameter names (more or less like in D20689!) if we can fetch parameter names -- I am thinking about trying to parse the variable names to find if they have a common prefix and number.
  • If the parameters have a sufficient (question is, what is sufficient) common ancestor in the AST (like as @zporky mentioned, assignment, comparison, etc., but also should consider passing them to the same function -- this would ignore forwarding and factory methods nicely on the first run --), they should be silenced.
    • Forwarding and trampoline functions I've put into the False-positive category, as they will be ignored once such a heuristic is complete.
    • This contributed a huge chunk of the false-positive ratio. Due to the trampolines and factories, and "forwarding overloads", it's around 40% on Xerces and LLVM. Without it, around 15%.
      • This more or less follows naturally, one offending function is one true positive, but every trampoline, pimple, overload, etc. on it is basically an additional false positive.
• Making decisions on code you're not familiar with is hard ;)

This is most likely not possible purely from Tidy as this requires the wrangler tooling support, but I believe people who wish to use this check should be encouraged to **only** take the introduced differences into consideration on their project.

[whisperity]

• Renamed check to experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type.
• s/argument/parameter/g in the code and output where appropriate.

[vingeldal]

In D69560#1935071, @whisperity wrote:

@aaron.ballman I've gone over LLVM (and a few other projects). Some general observations:

• Length of 2 is vile. I understand that the C++CG rule says even lengths of 2 should be matched, but that is industrially infeasible unless one introduces such a rule incrementally to their project. Findings of length 2 are , in general, an order of magnitude more than... basically the rest of the findings.
  • On big projects, even the current "default" of "length 3" seems to be too low. In reality, one should consider (this is likely to be out of scope for Tidy) how often these functions are called, and various other metrics on how serious an offender is.

Not a problem since existing projects can just use the option and change to whatever level suits them best. My opinion is still that the default shouldn't be set to whatever we think is most useful for the majority of existing projects but to what reflects the actual guideline.

Consider how this situation is similar to the guidelines regarding pointers. The guidelines assume that a project isn't using old standards of C++. Where one has a huge legacy code base it will be impractical at best to try to apply the C++ Core Guidelines for pointer handling on the old code.
The expectation is that new projects will use the new libraries and language features available to do things differently, in a way that makes it possible and practical to follow the guidelines; while legacy code remains unchecked or incrementally improved.
For any new code base this guideline shouldn't be a problem and existing projects can just adapt the usage of this rule to fit their situation by applying it selectively in the code base and/or changing the options.

[aaron.ballman]

In D69560#1936153, @vingeldal wrote:

In D69560#1935071, @whisperity wrote:

@aaron.ballman I've gone over LLVM (and a few other projects). Some general observations:

• Length of 2 is vile. I understand that the C++CG rule says even lengths of 2 should be matched, but that is industrially infeasible unless one introduces such a rule incrementally to their project. Findings of length 2 are , in general, an order of magnitude more than... basically the rest of the findings.
  • On big projects, even the current "default" of "length 3" seems to be too low. In reality, one should consider (this is likely to be out of scope for Tidy) how often these functions are called, and various other metrics on how serious an offender is.

Not a problem since existing projects can just use the option and change to whatever level suits them best. My opinion is still that the default shouldn't be set to whatever we think is most useful for the majority of existing projects but to what reflects the actual guideline.

That's what we try to aim for, but ultimately it means we have to decide whether that means we're willing to abandon a check over it or not (and we traditionally have not abandoned checks, but instead gone with more reasonable defaults). This is a case where I would probably rather abandon the check than go with that poor of a default behavior.

I think a more productive way forward is to collaborate with the rule authors until they have a rule that can be checked and reasonably complied with (which I imagine would require some heuristic choices in the rule that we could then apply in the check).

Consider how this situation is similar to the guidelines regarding pointers. The guidelines assume that a project isn't using old standards of C++. Where one has a huge legacy code base it will be impractical at best to try to apply the C++ Core Guidelines for pointer handling on the old code.

The expectation is that new projects will use the new libraries and language features available to do things differently, in a way that makes it possible and practical to follow the guidelines; while legacy code remains unchecked or incrementally improved.
For any new code base this guideline shouldn't be a problem and existing projects can just adapt the usage of this rule to fit their situation by applying it selectively in the code base and/or changing the options.

This is a very different situation, IMO. This is a check that is almost impossible to comply with when developing new code because it is insufficiently clear on what constitutes "related" parameters. Two adjacent types in a function parameter list is an incredibly common situation that arises naturally when writing code (especially for constructors, where your parameter order has to match the declaration order within the class) and designing around that from scratch is not always possible. Retroactively applying this rule to an existing code base would be nearly impossible for a project of any size.

[whisperity]

• Better organisation of code, cleanup of debug prints, fix nomenclature of functions
• Explicitly mention the first and last param of the range for clarity
• Downlift the logic of joining and breaking ranges to main patch (this isn't terribly useful at this point, but both subsequent improvements to the check depend on this change)
  • Basically no longer assume that if param N can be swapped with param 1, then it can also be swapped with 2, 3, etc. without checking.
• Made the list of ignored parameter and type names configurable as a checker option
• Changed the default value of MinimumLength to 2, as prescribed in the guideline's text.

[whisperity]

clang-tools-extra/docs/clang-tidy/checks/experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type.rst

136–139

Why are these typos still here, typedef??? instead of typename.

[whisperity]

I'd like to resurrect the discussion about this. Unfortunately, the concept of experimental- group (in D76545) has fallen silent, too. We will present the results of this analysis soon at IEEE SCAM (Source Code Analysis and Manipulation) 2020. While a previous submission about this topic was rejected on the grounds of not being in line with the conference's usual, hyper-formalised nature, with one reviewer especially praising the paper for its nice touch with the empirical world and the fact that neither argument swaps (another checker worked on by a colleague) nor the interactions of this issue with the type system (this checker) was really investigated in the literature for C++ before, we've tailored both the paper and the implementation based on reviewer comments from the scientific world, and the comments here.
The reviews were mostly favourable, excl. comments about the lack of formalisation and the unfortunate lack of modelling for template instantiations. Such a cold cut, however, only introduces false negatives into the result set. Which is fine, as the users will never see those non-existent reports!

In D69560#1936574, @aaron.ballman wrote:

This is a check that is almost impossible to comply with when developing new code because it is insufficiently clear on what constitutes "related" parameters.

I agree the CppCG is rather vague about this. From an angle, it seems intentional... "relatedness" is hard to pin down formally, it might vary from one "module" to the next even in the same project. In a subsequent patch to this, I've given a few good examples as to what can be reasonably culled by relatedness heuristics. They filter a good chunk of the results, letting go of most of the trivially "valid (but maybe nonsense) if swapped" functions (min, max, find, etc.)

In D69560#1936574, @aaron.ballman wrote:

Two adjacent types in a function parameter list is an incredibly common situation that arises naturally when writing code [...] and designing around that from scratch is not always possible.

I see the reasoning behind this. However, this feels more like a motivation against the rule itself, and not the checker. We can debate the justification for the rule's existence, but (and correct me if I'm wrong) so far I have not seen any tool (that's publicly available, and is for C(++), etc...) that attempts to check your code satisfying this particular rule.

In D69560#1936574, @aaron.ballman wrote:

(especially for constructors, where your parameter order has to match the declaration order within the class)

CMIIW, but I believe this statement is not true. Aggregate initialisation does not care about your constructors, and yes, takes arguments in the order of fields. However, once you have a user-defined constructor, you should be able to define your parameters in any order you like. The only thing you should match is that the member-init-exprs of the constructor are evaluated in order of field declarations, not in the order you wrote them. But trespassing that rule already emits a compiler warning, and in reality, the trespass only causes an issue if there is a data dependency between your fields. You should ensure your member-init-exprs are in the right order (to guard that a later change introducing a data dependency won't blow you up), but this has zero effect on the order of parameters of the constructor.

In D69560#1936574, @aaron.ballman wrote:

Retroactively applying this rule to an existing code base would be nearly impossible for a project of any size.

It is hard in the general case (although there are some approaches and foundations that we could build upon with refactoring, I've taken up the mantle of spending some time with this research), but there are cases where "modernisation" efforts, especially tool-aided ones are already possible. I have been recently suggested reading this paper: H. K. Wright: Incremental Type Migration Using Type Algebra While I agree that the case study in the paper is rather domain-specific if we consider the example there: once a tool has identified a variable/expression to not be a double but instead abseil::Duration, from a function call where such a refactored argument is passed, you can (or, hopefully, need to) replace the type of the parameter. And one such "adjacent parameters of the same type" has been refactored.

My position with the paper, and this implementation, is that it is more useful for new projects than existing ones. It might definitely not be useful for LLVM (a humongous project!), and it might not be possible to fix every report in an afternoon. But thanks to the vast amount of IDEs integrating Clang-Tidy, if someone wishes to adhere to this rule, they can have a (reasonably) immediate report the moment they trespassed the rule. You can also consider it in an incremental mode (see my comment above with incremental report gating via CodeChecker, but surely it's equally trivial to implement in other pipelines) or only in new or new-ish TUs/modules inside the project.

In addition, there are plenty of guidelines other than CppCG, which have at least some of their rules mapped to Tidy checks (and surely other analysers' routines). If a project (be it of any size and any domain) does not wish to adhere to a guideline (or a rule of a guideline), they can configure their invocation to not run those checks. I agree the fact that Tidy not supporting "opt-in" checkers by default is a bit of a drawback here, however, consider the "execution paths" one project might take, w.r.t. to this analysis:

The checker is enabled, and it produces between one and a bazillion reports.

1. They realise their project does not conform to CppCG (in which case why is the checker group enabled in the first place?) or specifically CppCG#I.24 and decide to disable the check. All is well in the world, and nothing happened excluding one new line being added to a configuration file.
2. Multiple projects big and famous enough see that their software are "non-compliant" (at least from the PoV of conformance to CppCG#I.24). They together start to question the necessity of such rule... maybe it shouldn't be a rule in the first place, because it's woefully stupid and impossible to adhere to. This is actionable data one can talk about at conferences and go to the guideline maintainers(?) with, and, if so, the rule gets deprecated/abolished/move to an "intellectual nitpicking" section. (In this case, implementing this check was a nice learning opportunity, and eventually, it will also be removed from Tidy.)
3. A new projects start developing, and they start seeing this rule. They can also decide whether or not conformance to the rule is something they want to live with. If not, previous cases apply. If so, we start to grow projects that are somewhat more type-safe, or less error-prone.

So far, we do not know how painful actual conformance to this rule will be. And, as with most static analysis, we might never know if there ever was "one life saved" by someone deciding to heed this warning. This won't be a panacea overnight, but I still believe it, first, does not have to be, and second, is a step in the right direction.

In D69560#1890767, @vingeldal wrote:

I'm all for investigating the performance of this check and basing any decisions on data. Until we have that data I just want to challenge the claim that there are no suitable ways to turn the check of for select portions of the code base,

Computational performance is in line with Tidy checks in general. LLVM took 33 minutes to run, generally smaller projects are less than a minute. (-j24)
As for the performance in terms of the results... I have a virtual image (a previous conference we submitted the paper to wanted us to submit reproduction images, but because the paper was rejected, I believe they have not archived the image). I've found a CodeChecker database with 15 projects analysed (incl. LLVM) but it's ~150 MiB in size... 😦

[aaron.ballman]

In D69560#2292505, @whisperity wrote:

I'd like to resurrect the discussion about this. Unfortunately, the concept of experimental- group (in D76545) has fallen silent, too. We will present the results of this analysis soon at IEEE SCAM (Source Code Analysis and Manipulation) 2020. While a previous submission about this topic was rejected on the grounds of not being in line with the conference's usual, hyper-formalised nature, with one reviewer especially praising the paper for its nice touch with the empirical world and the fact that neither argument swaps (another checker worked on by a colleague) nor the interactions of this issue with the type system (this checker) was really investigated in the literature for C++ before, we've tailored both the paper and the implementation based on reviewer comments from the scientific world, and the comments here.
The reviews were mostly favourable, excl. comments about the lack of formalisation and the unfortunate lack of modelling for template instantiations. Such a cold cut, however, only introduces false negatives into the result set. Which is fine, as the users will never see those non-existent reports!

Congrats on the SCAM acceptance, I hope the presentation goes well!

In D69560#1936574, @aaron.ballman wrote:

This is a check that is almost impossible to comply with when developing new code because it is insufficiently clear on what constitutes "related" parameters.

I agree the CppCG is rather vague about this. From an angle, it seems intentional... "relatedness" is hard to pin down formally, it might vary from one "module" to the next even in the same project. In a subsequent patch to this, I've given a few good examples as to what can be reasonably culled by relatedness heuristics. They filter a good chunk of the results, letting go of most of the trivially "valid (but maybe nonsense) if swapped" functions (min, max, find, etc.)

I agree that the vagueness seems intentional. That's usually reasonable when you want coding guidelines for a human to check, but doesn't help for production-quality automated checking. Do you happen to know how many false positives the check currently issues without that heuristic?

In D69560#1936574, @aaron.ballman wrote:

Two adjacent types in a function parameter list is an incredibly common situation that arises naturally when writing code [...] and designing around that from scratch is not always possible.

I see the reasoning behind this. However, this feels more like a motivation against the rule itself, and not the checker.

Correct, it's definitely a motivation against the rule itself.

We can debate the justification for the rule's existence, but (and correct me if I'm wrong) so far I have not seen any tool (that's publicly available, and is for C(++), etc...) that attempts to check your code satisfying this particular rule.

I'm not aware of any either. However, an indictment of the rule being too ambiguous is also an indictment of any checkers implementing that rule because they're going to have a higher failure rate than if the rule was not ambiguous.

In D69560#1936574, @aaron.ballman wrote:

(especially for constructors, where your parameter order has to match the declaration order within the class)

CMIIW, but I believe this statement is not true. Aggregate initialisation does not care about your constructors, and yes, takes arguments in the order of fields. However, once you have a user-defined constructor, you should be able to define your parameters in any order you like. The only thing you should match is that the member-init-exprs of the constructor are evaluated in order of field declarations, not in the order you wrote them.

My point is that it's far more common to match all three orders even though you don't have to. e.g.,

// Common, correct
class C {
  int a, b;
  float c;

public:
  C(int a_, int b_, float c_) : a(a_), b(b_), c(c_) {}
};

// Usually held as a code smell, but correct and not commonly diagnosed
class C {
  int a, b;
  float c;

public:
  C(int a_, float c_, int b_) : a(a_), b(b_), c(c_) {}
};

// Incorrect, commonly diagnosed
class C {
  int a, b;
  float c;

public:
  C(int a_, float c_, int b_) : a(a_), c(c_), b(b_) {}
};

But trespassing that rule already emits a compiler warning, and in reality, the trespass only causes an issue if there is a data dependency between your fields. You should ensure your member-init-exprs are in the right order (to guard that a later change introducing a data dependency won't blow you up), but this has zero effect on the order of parameters of the constructor.

I don't fully agree that it has zero effect on the parameter order of the constructors, but more importantly, what do you do about this sort of thing as a user?

class Point {
  int x, y;

public:
  Point(int x, int y);

  friend bool operator==(const Point &LHS, const Point &RHS);
}

class Rect {
  int x, y, width, height;

public:
  Rect(int x, int y, int width, int height);
};

These situations are not ones we should ever be issuing a diagnostic for IMO, but I would argue that at least in the case of the constructors, they violate the spirit of the C++ guideline rule (where swaps are plausible and problematic). This is why I think the *rule* is bogus and I'm not certain we should have a checker for it -- the rule itself is too broad and doesn't account for realistic code.

In D69560#1936574, @aaron.ballman wrote:

Retroactively applying this rule to an existing code base would be nearly impossible for a project of any size.

It is hard in the general case (although there are some approaches and foundations that we could build upon with refactoring, I've taken up the mantle of spending some time with this research), but there are cases where "modernisation" efforts, especially tool-aided ones are already possible. I have been recently suggested reading this paper: H. K. Wright: Incremental Type Migration Using Type Algebra While I agree that the case study in the paper is rather domain-specific if we consider the example there: once a tool has identified a variable/expression to not be a double but instead abseil::Duration, from a function call where such a refactored argument is passed, you can (or, hopefully, need to) replace the type of the parameter. And one such "adjacent parameters of the same type" has been refactored.

My position with the paper, and this implementation, is that it is more useful for new projects than existing ones.

Agreed.

It might definitely not be useful for LLVM (a humongous project!), and it might not be possible to fix every report in an afternoon. But thanks to the vast amount of IDEs integrating Clang-Tidy, if someone wishes to adhere to this rule, they can have a (reasonably) immediate report the moment they trespassed the rule. You can also consider it in an incremental mode (see my comment above with incremental report gating via CodeChecker, but surely it's equally trivial to implement in other pipelines) or only in new or new-ish TUs/modules inside the project.

I understand this point, but it doesn't address the fact that users writing real code to solve problems will *often* run into situations where this rule diagnoses code that cannot be modified. Overloaded operators are a great example of this -- how do you avoid adjacent parameters of the same type in an overloaded comparison operator? How does someone writing a Point or Rect class avoid adjacent parameters of the same type in constructors? How does someone avoid writing adjacent parameters in a callback function whose signature is defined by a standard (like bsearch() or qsort() from the C standard)?

I think the answer to some of these questions is that we can introduce heuristics -- exempt overloaded operators from the check, exempt functions named equal, compare, comp, etc, exempt constructors (perhaps only when all data members have compatible types), etc. But without those sort of heuristics, I'm not certain the check has a lot of value for real code.

In addition, there are plenty of guidelines other than CppCG, which have at least some of their rules mapped to Tidy checks (and surely other analysers' routines). If a project (be it of any size and any domain) does not wish to adhere to a guideline (or a rule of a guideline), they can configure their invocation to not run those checks.

We do have other modules for other coding guidelines like the CERT guidelines, etc. And sometimes those guidelines have similar questionable behavior (for instance, CERT bans any use of calling the system() function -- so users are left without recourse there). That may be a reason to adopt this check, but we often ask that when check authors find an onerous behavior in a rule, they go back to the rule authors to alert them of the problems and see if we can improve the rule at the same time as the check. If the rule authors come back and say "no, we really do want it to diagnose in those cases", then so be it.

I agree the fact that Tidy not supporting "opt-in" checkers by default is a bit of a drawback here, however, consider the "execution paths" one project might take, w.r.t. to this analysis:

The checker is enabled, and it produces between one and a bazillion reports.

1. They realise their project does not conform to CppCG (in which case why is the checker group enabled in the first place?) or specifically CppCG#I.24 and decide to disable the check. All is well in the world, and nothing happened excluding one new line being added to a configuration file.
2. Multiple projects big and famous enough see that their software are "non-compliant" (at least from the PoV of conformance to CppCG#I.24). They together start to question the necessity of such rule... maybe it shouldn't be a rule in the first place, because it's woefully stupid and impossible to adhere to. This is actionable data one can talk about at conferences and go to the guideline maintainers(?) with, and, if so, the rule gets deprecated/abolished/move to an "intellectual nitpicking" section. (In this case, implementing this check was a nice learning opportunity, and eventually, it will also be removed from Tidy.)
3. A new projects start developing, and they start seeing this rule. They can also decide whether or not conformance to the rule is something they want to live with. If not, previous cases apply. If so, we start to grow projects that are somewhat more type-safe, or less error-prone.

So far, we do not know how painful actual conformance to this rule will be. And, as with most static analysis, we might never know if there ever was "one life saved" by someone deciding to heed this warning. This won't be a panacea overnight, but I still believe it, first, does not have to be, and second, is a step in the right direction.

I agree the check doesn't have to be perfect, but given the triviality of finding a large number of false positives, it's worth our effort to see if we can find a solution that is more practical.

I think the concrete steps I'd like to see before moving forward with this check is:

• Pick 1-3 projects with whatever qualifications make sense (size, adherence to the core guidelines, etc) and measure how many diagnostics this check produces as-is and see if we can get an idea of what % of diagnostics are ones the user would struggle to do something with (where struggle is: can't change the order because of <reasons>, changing the order would make the code less readable, etc).
• See if we can come up with some heuristics to reduce the number of poor-quality diagnostics and measure the effectiveness against those projects.
• Once we have some ideas of ways to reduce the "noise" that we're happy with, circle back with the C++ Core Guideline authors to see if they'd be willing to tighten the rule up for those situations. If they *don't* want the changes, that's fine, we can hide them behind a configuration flag (and we can decide whether that flag should be on or off by default at that point).

WDYT?

[whisperity]

In D69560#2319340, @aaron.ballman wrote:

Congrats on the SCAM acceptance, I hope the presentation goes well!

The entire event was amazing, even if short. There was another paper which seemed to target the issue of swaps at call sites (D20689) in the section where I was presenting and due to the SCAM being small and having no parallel session, I can say with confidence, that it was this particular section where the longest discussion was spawned from the topics we discussed. (In every other section people left the call a bit too early. The worst problem of online conferencing... 😦)

---

I've gone over the reports on some projects one by one. Due to the vast number of reports on existing projects, I've only selected 3 of them: Bitcoin (C++), Postgres (C), and Apache Xerces (C++). I've specifically wanted to add a C project, however, Postgres produced so many reports across the configurations (1460, 3033, 2415 and 4253, respectively) that it would take multiple inhumane months to go over them one by one... So, let's focus on Bitcoin and Xerces. Reasonably sized projects both being roughly a single library, they are both written to the OOP paradigm and are of reasonably modern C++.

I've run 4 analyses of each project:

• normal mode (named simply len2 in the uploaded database, only strict type equality is reported)
• normal mode, with relatedness heuristics filtering (len2-rel, D78652, this should be the strictest mode and produce the least number of results. This mode is most conformant in matching the Guideline rule!)
• CVR+Imp mode (len2-cvr-imp, type equality is relatex to type convertibility, which generally means an adjacency of int, const int (CVR) and int, double (Imp) is reported too. CVR is implemented in this patch, Imp is added in a subsequent patch, D75041. This should be the broadest configuration.)
• CVR+Imp mode, with relatedness filtering (len2-cvr-imp-rel)

The number of reports were, in this order of configurations:

• Bitcoin: 183, 82, 516, 259
• Postgres: 3033, 1460, 4253, 2415
• Xerces: 254, 79, 412, 165

In CodeChecker, I have marked cases where simple refactorings (strong typedef or a range-constrained type) could help remove the issue as Confirmed bug. False positives are cases that cannot be removed by heuristics easily, while Intentional bugs (this is a classification category in CodeChecker you can select for your bug, the naming is wonky here but let's just consider it a third bucket) were the ones where sensible heuristics can be devised to suppress these reports. We'll get back to this later. I've checked the results both with "report uniqueing" (1) on and off.

---

BitCoin

Project "component"	Confirmed cases (unique)	False positive (unique)	Suppressable (unique)
Qt MOC generated code			8
Third-party library hosted in repository	13	28 (27)	11
Test code	34	20	12
Project core	136	137	55 (53)
Total	183	185 (184)	86 (84)

These were the numbers for the "normal mode". Now, let's see how it changes when relatedness filtering (as in D78652 Diff 259231) is introduced.

Project "component"	Confirmed cases (Unique)	False positive (Unique)	Suppressable (unique)
Qt MOC generated code
Third-party library hosted in repository	5	7	4
Test code	15	11	2
Project core	62	37	14
Total	82	55	20

(Numbers did not change with enabling "unique reports".)

Modelling implicit conversions blows up the result set. Note that for this analysis, the definition behind confirmed bug is changed a bit: in case of reports where implicit conversions are reported, if the swap through the implicit conversion is dangerous it is reported as a confirmed bug. (In many cases, the implicit conversions stem from f(int, double) functions... where the introduction of a semantic typedef or constrained integer type with no implicit conversions would fix the issue).

The results for cvr-imp are:

Project "component"	Confirmed cases (Unique)	False positive (Unique)	Suppressable (unique)
Qt MOC generated code	136		11
Third-party library hosted in repository	26	29 (28)	11
Test code	56	11	14
Project core	298	138	63 (61)
Total	516	178 (177)	99 (97)

Now, enabling relatedness filtering over these results, gives us:

Project "component"	Confirmed cases (Unique)	False positive (Unique)	Suppressable (unique)
Qt MOC generated code	68
Third-party library hosted in repository	11	7	4
Test code	31	10	3
Project core	149	42	19
Total	259	59	26

(Numbers did not change with enabling "unique reports".)

---

Xerces

I'll use the same order in the reports. In Xerces, building the tests are not part of building the project itself. This is why the test files were not logged and thus were not analysed. In addition, Xerces contains no well-identifiable third-party libraries living in-tree, so the tables below are shorter.

In normal mode:

Bug kind	Confirmed cases (Unique)	False positive (Unique)	Suppressable (unique)
Total	254 (246)	149 (144)	101 (95)

With relatedness filtering:

Bug kind	Confirmed cases (Unique)	False positive (Unique)	Suppressable (unique)
Total	79	61 (59)	53

In broad matching (CVR-Imp) mode:

Bug kind	Confirmed cases (Unique)	False positive (Unique)	Suppressable (unique)
Total	412 (380)	158 (154)	135 (125)

WIth relatedness filtering:

Bug kind	Confirmed cases (Unique)	False positive (Unique)	Suppressable (unique)
Total	165 (159)	55	87 (82)

---

I've attached the result database:

AdjacentParams.sqlite25 MBDownload

So what heuristics can we still put in? While the check is about parameter types and not names, I think we can still (perhaps hidden behind an option) rely on some properites of the parameter name to make decisions. One such is that reports involving otherwise unnamed parameters should be squelched. The other is the ignoring of parameters which have a "patterned name" (i.e. text1, text2, text3, text4 (live example from Xerces!)), as mentioned in [Rice2017].

In addition, I think it would be useful to make certain type names ignored and this list of names configured by the user. bool is the most striking candidate that comes to mind... you can't really do anything with an f(bool a, bool b), at least nothing that would improve the safety and readability of the code. Strong typedefs over bool could exist (this was proposed before for LLVM itself, see this llvm-dev post and D66148), but having a -- to paraphrase my own paper -- f(ShouldFlip flip, ShouldStretch stretch) is absolutely bonkers. By throwing out some types ahead of time, we can lessen the number of crappy reports and noise.

In addition, I plan to investigate whether reporting one-way implicit conversions is useful in the first place. I believe it would be beneficial to put the reporting of that behind a distinct option (i.e. "ReportOneWayImplicit" and "ReportTwoWayImplicit" toggling separate things!) and leaving it off by default. Far too many silly cases of only one way implicitness (such as * or & upcasts, where you can't really call an f(B* b, D* d) with f(d, b) anyways...) producing results.

Perhaps it would also be good to ignore all binary operators, and call operators of functors that are equality predicates, comparators, etc. Some of these cases are yanked by the relatedness heuristics already, but not all...

---

In addition to all this, and tracking back to the talk about default configuration... @aaron.ballman, I have some questions. I've seen that Tidy now supports "check aliases". Do you think moving forward in a way that we rename this check and put it into another group (maybe calling it bugprone-adjacent-parameters-of-same-type) with more sensible defaults and offering a version of the check under cppcoreguidelines- with the defaults more matching the guideline rule would be a good way forward, eventually? Implicit conversions seem to be a real edge of the analysis (hence why I pinned my paper about that problem, also, this was the novelty, the rest of the mixability analysis was done decades ago), and seem to hurt more than simple type equality. However, the guideline is cautious about only warning for same type, and considers const T* and T* already distinct.

---

(1): I'm not exactly sure as to what "report uniqueing" in CodeChecker precisely does these days, but basically it uses the context of the bug and the checker message and whatnot to create a hash for the bug - "unique reports" mode shows each report belonging to the same hash only once.

[Rice2017]: Andrew Rice, et al.: Detecting argument selection defects. In: Proceedings of the ACM on Programming Languages, 1, pp. 104:1-104:23, 2017.

[whisperity]

I was just about to write an issue to the CoreGuidelines project, but I was @vingeldal has posted about "relatedness" before me: https://github.com/isocpp/CppCoreGuidelines/issues/1579. It was in the list of closed issues.

Herb Sutter has changed the rule's title to "Avoid adjacent parameters of the same type when changing the argument order would change meaning". Now the phrase related has been removed from the title, and the body of the rule does not contain any mention to this phrase either.

change of meaning is still hard to prove from a Tidy standpoint (or could be incomputable to prove in the first place...), but a concept that can be grasped with much less mental effort.

The guideline itself still says that the Enforcement is to simply warn if two parameters have the same type. We are doing much more here already with making the Tidy output much more user friendly and less bloatful.

[aaron.ballman]

In D69560#2486806, @whisperity wrote:

I was just about to write an issue to the CoreGuidelines project, but I was @vingeldal has posted about "relatedness" before me: https://github.com/isocpp/CppCoreGuidelines/issues/1579. It was in the list of closed issues.

Herb Sutter has changed the rule's title to "Avoid adjacent parameters of the same type when changing the argument order would change meaning". Now the phrase related has been removed from the title, and the body of the rule does not contain any mention to this phrase either.

change of meaning is still hard to prove from a Tidy standpoint (or could be incomputable to prove in the first place...), but a concept that can be grasped with much less mental effort.

The guideline itself still says that the Enforcement is to simply warn if two parameters have the same type. We are doing much more here already with making the Tidy output much more user friendly and less bloatful.

Thank you for continuing to work on this and working with the rule authors. I agree that "changes meaning" would be really hard to gauge within clang-tidy (but is more precise for a human auditing the code). In theory, we could take the command line options and use them to compile a function to LLVM IR, swap two adjacent parameter identifiers and recompile, then see if the IR is the same or not. But my gut feeling is that this would make the diagnostic behavior very mysterious to users (you change command line flags from -O2 to -O0 and suddenly you get different diagnostics because the optimizations are different) and is not something we'd want to do.

In addition to all this, and tracking back to the talk about default configuration... @aaron.ballman, I have some questions. I've seen that Tidy now supports "check aliases". Do you think moving forward in a way that we rename this check > and put it into another group (maybe calling it bugprone-adjacent-parameters-of-same-type) with more sensible defaults and offering a version of the check under cppcoreguidelines- with the defaults more matching the guideline rule > would be a good way forward, eventually? Implicit conversions seem to be a real edge of the analysis (hence why I pinned my paper about that problem, also, this was the novelty, the rest of the mixability analysis was done decades
ago), and seem to hurt more than simple type equality. However, the guideline is cautious about only warning for same type, and considers const T* and T* already distinct.

Sorry, I missed your question from earlier. I think using a check alias with different option defaults is a workable approach. We may have to live with the fact that C++ Core Guideline check is really chatty and only useful for programs in very specific situations (and that's fine, we do that for other check guidelines sometimes). Having a check with better default options for our users is a good compromise.

[whisperity]

I have posted two questions to GitHub, mostly related to the guideline rule and how free the implementation could be: #1732 and #1733, I think I tagged you on both.

However, if we are going down the alias route, I think it should be the guideline version which is the alias, and this check should live under a different name. Where do you think should we put it? I think bugprone- is the best fit, and readability- is the second-best... The "avoid adjacent parameter of similar types" name could stay... or something like ...-easily-swappable-function-parameters, that involves all the features here, with things like "default length 3" and such, "relatedness" heuristics turned on. Not sure what CVR-modelling's default should be... it finds less when "off", but too easily silences crucial issues (such as memcpy(T*, const T*)).

[aaron.ballman]

In D69560#2487093, @whisperity wrote:

I have posted two questions to GitHub, mostly related to the guideline rule and how free the implementation could be: #1732 and #1733, I think I tagged you on both.

However, if we are going down the alias route, I think it should be the guideline version which is the alias, and this check should live under a different name. Where do you think should we put it? I think bugprone- is the best fit, and readability- is the second-best...

I think bugprone is where it should ideally live, but if we still think we have "too high" of a false positive rate (for however we decide we want to measure that), then readability is a good fallback. (I'm assuming more people enable bugprone-* than enable readability-*.)

The "avoid adjacent parameter of similar types" name could stay... or something like ...-easily-swappable-function-parameters, that involves all the features here, with things like "default length 3" and such, "relatedness" heuristics turned on.

Hmm, how about bugprone-easily-swappable-parameters? We can drop the function from it because we probably also care about function-like things (like lambdas and blocks) and parameters should be sufficiently clear as to what's getting checked.

Not sure what CVR-modelling's default should be... it finds less when "off", but too easily silences crucial issues (such as memcpy(T*, const T*)).

My instinct is that if you accidentally swap a qualified parameter you'll get some other diagnostic about dropped qualifiers and so perhaps the default should be off, but perhaps there's something I'm not thinking of there.

[whisperity]

In D69560#2487117, @aaron.ballman wrote:

In D69560#2487093, @whisperity wrote:

Not sure what CVR-modelling's default should be... it finds less when "off", but too easily silences crucial issues (such as memcpy(T*, const T*)).

My instinct is that if you accidentally swap a qualified parameter you'll get some other diagnostic about dropped qualifiers and so perhaps the default should be off, but perhaps there's something I'm not thinking of there.

That only happens if at a call site the argument is qualified.

void foo(const int& ir1, int& ir2);

void test() {
  int i1 = 8;
  int i2 = 10;

  foo(i2, i1); // No warning here.

  const int ci = 12;
  foo(i1, ci); // Error here.
}

So while const Something and Something are distinct types conceptually (and as of now, according to I.24), an unqualified Something can still be passed in a swapped order at any given call site.
Will think about this.

I'll continue implementing some of the heuristics, re-run the measurements, and then I'll update the patch with the rename.