[PATCH] D20689: [clang-tidy] Add 'readability-suspicious-call-argument' check

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/readability/SuspiciousCallArgumentCheck.cpp:347-349
+static bool areTypesCompatible(QualType ArgType, QualType ParamType,
+                               const ASTContext &Ctx) {
+  if (ArgType.isNull() || ParamType.isNull())
----------------
whisperity wrote:
> aaron.ballman wrote:
> > whisperity wrote:
> > > whisperity wrote:
> > > > aaron.ballman wrote:
> > > > > It seems like we're doing an awful lot of the same work as `ASTContext::typesAreCompatible()` and type compatibility rules are pretty complex, so I worry about this implementation being different than the `ASTContext` implementation. Have you explored whether we can reuse more of the logic from `ASTContext` here, or are they doing fundamentally different kinds of type compatibility checks?
> > > > No, I didn't know that function even existed. This check must be older than that function.
> > > Actually, no, that function is pretty old... However, that function, and all the function it subsequently calls, require a **non-const** `ASTContext`. I have changed `ASTContext`:
> > > 
> > > ```
> > > ╰─ git diff --cached --stat
> > >  clang/include/clang/AST/ASTContext.h                                     | 67 ++++++++++++++++++++++++++++++++++++-------------------------------
> > >  clang/lib/AST/ASTContext.cpp                                             | 72 +++++++++++++++++++++++++++++++++++++-----------------------------------
> > >  2 files changed, 73 insertions(+), 66 deletions(-)
> > > ```
> > > 
> > > making related member functions and internal static functions take `const ASTContext &`/`*`.
> > > 
> > > This, by itself, did not break any of the tests of `check-clang check-clang-unit check-clang-tools check-clang-extra-unit`!
> > Doubtful -- `typesAreCompatible()` is critical for checking the semantics of assignment in C, overloading in C++, etc. It may have simply been overlooked when writing this check. Given the complexities of type checking, my intuition is that we should be leaning on `ASTContext` for as much of this functionality as we can get away with. That will also get us nice "extras" like caring about address spaces, ARC, etc which are what got me worried when I started looking at this implementation.
> @aaron.ballman Changing the function to be 
> 
> ```
> return Ctx.typesAreCompatible(ArgType, ParamType);
> ```
> 
> will make the checker miss the test case about `T`/`const T&` mixup.
> 
> ```
> void value_const_reference(int llllll, const int& kkkkkk);
> void const_ref_value_swapped() {
>   const int& kkkkkk = 42;
>   const int& llllll = 42;
>   value_const_reference(kkkkkk, llllll);
>   // error: CHECK-MESSAGES: expected string not found in input:
>   // warning: 1st argument 'kkkkkk' (passed to 'llllll') looks like it might be swapped with the 2nd, 'llllll' (passed to 'kkkkkk')
> }
> ```
> 
> ----
> 
> Setting `bool CompareUnqualified = true` (3rd argument to `typesAreCompatible`) doesn't help either.
> Which is valid from `typesAreCompatible`'s perspective... that function answer the question, applied to the context of the above test: //"Is `llllll = kkkkkk;` valid?"//, which is obviously **false** as both are `const T&`s.
Yeah, I expect there to be a delta between the work this check is doing and the existing work done by `typesAreCompatible()`. However, given the complexity of type compatibility checking, I'd say it's better for us to try to refactor the ASTContext functionality so that we can share as much of the implementation as plausible rather than duplicate some really difficult logic in the tidy check.


================
Comment at: clang-tools-extra/clang-tidy/readability/SuspiciousCallArgumentCheck.cpp:657-660
+    } else {
+      ParamTypes.push_back(QualType());
+      ParamNames.push_back(StringRef());
+    }
----------------
whisperity wrote:
> aaron.ballman wrote:
> > Can this case happen?
> Oops... It seems I posted the updated patch right where you were writing more comments and we got into a data race. Which case are you referring to? It's now affixed to a `diag(` call for me...
Hehe, it's "fun" when the comments move around like this, isn't it? :-D I meant the `else` clause in:
```
  for (std::size_t I = 0, E = CalleeFuncDecl->getNumParams(); I != E; ++I) {
    if (const ParmVarDecl *Param = CalleeFuncDecl->getParamDecl(I)) {
      ParamTypes.push_back(Param->getType());

      if (IdentifierInfo *II = Param->getIdentifier()) {
        ParamNames.push_back(II->getName());
      } else {
        ParamNames.push_back(StringRef());
      }
    } else { // This seems like it should be impossible, no?
      ParamTypes.push_back(QualType());
      ParamNames.push_back(StringRef());
    }
  }
```


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/readability-suspicious-call-argument.rst:75
+In the case of ``oldValue`` and ``value`` compared, the similarity percentage
+is `8 / 5 = 62.5`\%.
+
----------------
whisperity wrote:
> aaron.ballman wrote:
> > Similar to above, I wonder how numeric digits impact this heuristic -- do the defaults consider this to be a swap?
> > ```
> > void foo(int frobble1, int frobble2);
> > foo(frobble2, frobble1); // Hopefully identified as a swap
> > foo(bar2, bar1); // How about this?
> > ```
> Currently, neither of these are matched. I have to look into why the first isn't... it really should, based on the "equality" heuristic. It's too trivial.
> 
> The second... well... that's trickier. I would say it shouldn't match, because if it did, we would be swamped with false positives. The suffix is only 1 character, and we need 25/30% based on the string's length.
I agree that the first one should be caught and I also agree that the second one is tricky but that matching it would likely increase false positives.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D20689