[PATCH] D20689: [clang-tidy] Add 'readability-suspicious-call-argument' check
Whisperity via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Fri Mar 19 07:03:47 PDT 2021
whisperity added inline comments.
================
Comment at: clang-tools-extra/clang-tidy/readability/SuspiciousCallArgumentCheck.cpp:347
+// Checks whether ArgType converts implicitly to ParamType.
+static bool areTypesCompatible(QualType ArgType, QualType ParamType,
+ const ASTContext &Ctx) {
----------------
aaron.ballman wrote:
> 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.
I talked with @aaron.ballman in private about this a little, and unfortunately, the route to call `typesAreCompatible()` is no dice. First things first, in C++ mode, that function just early returns (essentially) `==` on the type. Otherwise, it would call `QualType ASTContext::mergeTypes(QualType, QualType, ...)`. Okay, let's delve into this thing. Now, first things first, somewhere down the line if you happen to give it something C++-specific (e.g. `LValueReferenceType`), it will just assert into your face. (Because C++ things are only `compatible` if the types are equal, most likely. In C++ mode, `mergeType` shouldn't be, and doesn't get, called.)
But there are even more issues with this function. Put simply, it does not perform **any** "implicit conversions". Sic!, I'm putting it in between quotes, but the fact is that if one type is `const` and the other isn't, it will just say //"Nope, these are not compatible."//.
So modelling the whole idea of //"is passing an argument of type `T` acceptable to a parameter of type `U`?"// is not possible with these functions.
So even though `typesAreCompatible()`'s documentation comment says:
> Compatibility predicates used to check assignment expressions.
by far and large, we are not having the right kinds of types in our hands (in terms of this check) to fall back to this logic.
I've done a skim of the codebase to find call sites for these functions and try to explore my way back and forth from there to see //how// this function should be used, and the results are daunting.
{F15911467}
The image is from `lib/Sema/SemaExpr.cpp` somewhere along the lines of checking address spaces (for Obj-C, the vast majority of this function seems to only deal with Obj-C-specific things!), from `checkConditionalPointerCompatibility`. The call to `mergeTypes()` is in the bottom left, the right pane is the continuation below on the result here is used.
It seems to me (and many other call sites to this function tend to behave the same way) that //Sema// does a lot of casting-away-things (like constness) before it can reasonably call this function, and if the result comes back valid, this function (and many others) then just re-apply manually the things that were originally cast off.
Thus, I see that if we would call this function in the check, a lot (if not all, or maybe //even more than now!//) of the logic that is currently here would still have to remain to prefix and suffix the call to `mergeTypes()`.
Just for the sake of trying it all out, I put together a version where I leave the handling of the "C++ stuff" (like references) to the checker, but the rest back to the `ASTContext`, but it didn't fall into place:
```
ParamType = PointerType 0x5627819d0930 'const int *'
`-QualType 0x5627819cfef1 'const int' const
`-BuiltinType 0x5627819cfef0 'int'
ArgType = PointerType 0x562781a49290 'int *'
`-BuiltinType 0x5627819cfef0 'int'
mergeType(Param, Arg) = <<<NULL>>>
mergeType(Arg, Param) = <<<NULL>>>
```
================
Comment at: clang-tools-extra/clang-tidy/readability/SuspiciousCallArgumentCheck.cpp:347
+// Checks whether ArgType converts implicitly to ParamType.
+static bool areTypesCompatible(QualType ArgType, QualType ParamType,
+ const ASTContext &Ctx) {
----------------
whisperity wrote:
> aaron.ballman wrote:
> > 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.
> I talked with @aaron.ballman in private about this a little, and unfortunately, the route to call `typesAreCompatible()` is no dice. First things first, in C++ mode, that function just early returns (essentially) `==` on the type. Otherwise, it would call `QualType ASTContext::mergeTypes(QualType, QualType, ...)`. Okay, let's delve into this thing. Now, first things first, somewhere down the line if you happen to give it something C++-specific (e.g. `LValueReferenceType`), it will just assert into your face. (Because C++ things are only `compatible` if the types are equal, most likely. In C++ mode, `mergeType` shouldn't be, and doesn't get, called.)
>
> But there are even more issues with this function. Put simply, it does not perform **any** "implicit conversions". Sic!, I'm putting it in between quotes, but the fact is that if one type is `const` and the other isn't, it will just say //"Nope, these are not compatible."//.
> So modelling the whole idea of //"is passing an argument of type `T` acceptable to a parameter of type `U`?"// is not possible with these functions.
>
> So even though `typesAreCompatible()`'s documentation comment says:
>
> > Compatibility predicates used to check assignment expressions.
>
> by far and large, we are not having the right kinds of types in our hands (in terms of this check) to fall back to this logic.
>
> I've done a skim of the codebase to find call sites for these functions and try to explore my way back and forth from there to see //how// this function should be used, and the results are daunting.
>
> {F15911467}
>
> The image is from `lib/Sema/SemaExpr.cpp` somewhere along the lines of checking address spaces (for Obj-C, the vast majority of this function seems to only deal with Obj-C-specific things!), from `checkConditionalPointerCompatibility`. The call to `mergeTypes()` is in the bottom left, the right pane is the continuation below on the result here is used.
>
> It seems to me (and many other call sites to this function tend to behave the same way) that //Sema// does a lot of casting-away-things (like constness) before it can reasonably call this function, and if the result comes back valid, this function (and many others) then just re-apply manually the things that were originally cast off.
>
> Thus, I see that if we would call this function in the check, a lot (if not all, or maybe //even more than now!//) of the logic that is currently here would still have to remain to prefix and suffix the call to `mergeTypes()`.
>
> Just for the sake of trying it all out, I put together a version where I leave the handling of the "C++ stuff" (like references) to the checker, but the rest back to the `ASTContext`, but it didn't fall into place:
>
> ```
> ParamType = PointerType 0x5627819d0930 'const int *'
> `-QualType 0x5627819cfef1 'const int' const
> `-BuiltinType 0x5627819cfef0 'int'
> ArgType = PointerType 0x562781a49290 'int *'
> `-BuiltinType 0x5627819cfef0 'int'
> mergeType(Param, Arg) = <<<NULL>>>
> mergeType(Arg, Param) = <<<NULL>>>
> ```
> ```
> ╰─ git diff --cached --stat
> clang/include/clang/AST/ASTContext.h | 67 ++++++++++++++++++++++++++++++++++++-------------------------------
> clang/lib/AST/ASTContext.cpp | 72 +++++++++++++++++++++++++++++++++++++-----------------------------------
> 2 files changed, 73 insertions(+), 66 deletions(-)
> ```
{F15911833}
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D20689/new/
https://reviews.llvm.org/D20689
More information about the cfe-commits
mailing list