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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:402-403
+
+    // Unfortunately, undersquiggly highlights are for some reason chewed up
+    // and not respected by diagnostics from Clang-Tidy. :(
+    diag(First->getLocation(), "the first parameter in the range is '%0'",
----------------
whisperity wrote:
> whisperity wrote:
> > aaron.ballman wrote:
> > > whisperity wrote:
> > > > aaron.ballman wrote:
> > > > > Can you post a screenshot of what you mean?
> > > > Given
> > > > 
> > > > ```
> > > > Diag << SourceRange{First->getSourceRange().getBegin(), Last->getSourceRange().getEnd()};
> > > > ```
> > > > 
> > > > The diagnostic is still produced with only a `^` at the original `diag(Location)`, ignoring the fed `SourceRange`:
> > > > 
> > > > ```
> > > > /home/whisperity/LLVM/Build/../llvm-project/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp:21:18: warning: 3 adjacent parameters of 'redeclChain' of similar type ('int') are easily swapped by mistake [bugprone-easily-swappable-parameters]
> > > > void redeclChain(int I, int J, int K) {}
> > > >                  ^
> > > > ```
> > > > 
> > > > Instead of putting all the squigglies in as the range-location highlight, like how `Sema` can diagnose:
> > > > 
> > > > ```
> > > > /home/whisperity/LLVM/Build/../llvm-project/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp:21:18: warning: 3 adjacent parameters of 'redeclChain' of similar type ('int') are easily swapped by mistake [bugprone-easily-swappable-parameters]
> > > > void redeclChain(int I, int J, int K) {}
> > > >                  ^~~~~~~~~~~~~~~~~~~
> > > > ```
> > > > 
> > > > I would not want to put additional note tags in an otherwise already verbose output.
> > > > 
> > > > Back in 2019 I was investigating this issue specifically for another checker I was working on, but hit the wall... Somewhere deep inside where Tidy diagnostic stuff is translated and consumed to Clang stuff, it goes away. It shouldn't, because there are statements that seemingly copy the `Diag.Ranges` array over, but it still does not come out.
> > > > 
> > > > ... EDIT: I found [[ http://lists.llvm.org/pipermail/cfe-dev/2019-October/063676.html | the relevant mailing list post ]].
> > > > ... EDIT: I found the relevant mailing list post.
> > > 
> > > Oh wow, I had no idea! That's a rather unfortunate bug. :-(
> > Speaking of bugs, shall I put it up on Bugzilla, or see if anyone reported already? Would require me digging into that "where are the data structures copied" kind of thing to figure it out again.
> Nevermind, this is worth [[ http://bugzilla.llvm.org/show_bug.cgi?id=49000 | a bug report ]] just so we can track it in the proper location.
If you don't mind doing that effort, it'd be appreciated! Even if you don't root cause the issue, having the report (or pinging an existing one) is still useful.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp:10
+
+void declaration(int Param, int Other); // NO-WARN: No chance to change this function.
+
----------------
whisperity wrote:
> aaron.ballman wrote:
> > whisperity wrote:
> > > aaron.ballman wrote:
> > > > I think this is a case where we could warn when the declaration is outside of a system header (perhaps as an option).
> > > > 
> > > > Thinking about it a bit more, declarations and definitions provide a novel way to get another kind of swap:
> > > > ```
> > > > void func(int x, int y);
> > > > void func(int y, int x) {} // Oops, the swap was probably not intentional
> > > > ```
> > > > which may or may not be interesting for a check (or its heuristics).
> > > I gave this some thought. It is a very good idea, but I believe not for //this// check, but D20689. What do you think of that? Maybe simply saying "call site v. function node that was called" could be extended with a completely analogous, string distance function based "function definition node v. redecl chain" case.
> > Hmmm, my impression is that this check is fundamentally about *parameter* ordering whereas D20689 is about *argument* ordering. Given that the example is about the ordering of parameters and doesn't have anything to do with call sites, I kind of think the functionality would be better in a parameter-oriented check.
> > 
> > That said, it doesn't feel entirely natural to add it to this check because this check is about parameters that are easily swappable *at call sites*, and this situation is not exactly that. However, it could probably be argued that it is appropriate to add it to this check given that swapped parameters between the declaration and the definition are likely to result in swapped arguments at call sites.
> > 
> > Don't feel obligated to add this functionality to this check (or invent a new check), though.
> It just seems incredibly easier to put it into the other check, as that deals with names. But yes, then it would be a bit weird for the "suspicious call argument" check to say something about the definition... This check (and the relevant Core Guideline) was originally meant to consider **only** types (this is something I'll have to emphasise more in the research paper too!). Everything that considers //names// is only for making the check less noisy and make the actually emitted results more useful. However, I feel we should //specifically// not rely on the names and the logic too much.
> 
> But(!) your suggestion is otherwise a good idea. I am not sure if the previous research (with regards to names) consider the whole "forward declaration" situation, even where they did analysis for C projects, not just Java.
> However, I feel we should specifically not rely on the names and the logic too much.

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

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69560/new/

https://reviews.llvm.org/D69560