[PATCH] D75041: [clang-tidy] Extend 'bugprone-easily-swappable-parameters' with mixability because of implicit conversions

[Whisperity via Phabricator via cfe-commits]

whisperity marked 2 inline comments as done.
whisperity added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:657-667
+  // Certain kinds unfortunately need to be side-stepped for canonical type
+  // matching.
+  if (LType->getAs<FunctionProtoType>() || RType->getAs<FunctionProtoType>()) {
+    // Unfortunately, the canonical type of a function pointer becomes the
+    // same even if exactly one is "noexcept" and the other isn't, making us
+    // give a false positive report irrespective of implicit conversions.
+    LLVM_DEBUG(llvm::dbgs()
----------------
whisperity wrote:
> whisperity wrote:
> > martong wrote:
> > > aaron.ballman wrote:
> > > > whisperity wrote:
> > > > > @aaron.ballman Getting ahead of the curve here. I understand this is ugly. Unfortunately, no matter how much I read the standard, I don't get anything of "canonical type" mentioned, it seems to me this concept is something inherent to the model of Clang.
> > > > > 
> > > > > Basically why this is here: imagine a `void (*)() noexcept` and a `void (*)()`. One's `noexcept`, the other isn't. Inside the AST, this is a `ParenType` of a `PointerType` to a `FunctionProtoType`. There exists a //one-way// implicit conversion from the `noexcept` to the non-noexcept ("noexceptness can be discarded", similarly to how "constness can be gained")
> > > > > Unfortunately, because this is a one-way implicit conversion, it won't return on the code path earlier for implicit conversions.
> > > > > 
> > > > > Now because of this, the recursive descent in our code will reach the point when the two innermost `FunctionProtoType`s are in our hands. As a fallback case, we simply ask Clang "Hey, do //you// think these two are the same?". And for some weird reason, Clang will say "Yes."... While one of them is a `noexcept` function and the other one isn't.
> > > > > 
> > > > > I do not know the innards of the AST well enough to even have a glimpse of whether or not this is a bug. So that's the reason why I went ahead and implemented this side-stepping logic. Basically, as the comment says, it'lll **force** the information of "No matter what you do, do NOT consider these mixable!" back up the recursion chain, and handle it appropriately later.
> > > > > Unfortunately, no matter how much I read the standard, I don't get anything of "canonical type" mentioned, it seems to me this concept is something inherent to the model of Clang.
> > > > 
> > > > It is more of a Clang-centric concept. Basically, a canonical type is one that's had all of the typedefs stripped off it.
> > > > 
> > > > > Now because of this, the recursive descent in our code will reach the point when the two innermost FunctionProtoTypes are in our hands. As a fallback case, we simply ask Clang "Hey, do you think these two are the same?". And for some weird reason, Clang will say "Yes."... While one of them is a noexcept function and the other one isn't.
> > > > 
> > > > I think a confounding factor in this area is that `noexcept` did not used to be part of the function type until one day it started being a part of the function type. So my guess is that this is fallout from this sort of thing: https://godbolt.org/z/EYfj8z (which may be worth keeping in mind when working on the check).
> > > About `noexcept`: we've faced a similar problem in the `ASTImporter` library. We could not import a noexcept function's definition if we already had one without the noexcept specifier. 
> > > 
> > > Thus, in `ASTStructuralEquivalence.cpp` we do differentiate the function types based on their noexcept specifier (and we even check the noexcept expression).:
> > > ```
> > > TEST_F(StructuralEquivalenceFunctionTest, Noexcept) {
> > >   auto t = makeNamedDecls("void foo();",
> > >                           "void foo() noexcept;", Lang_CXX11);
> > >   EXPECT_FALSE(testStructuralMatch(t));
> > > }
> > > TEST_F(StructuralEquivalenceFunctionTest, NoexceptNonMatch) {
> > >   auto t = makeNamedDecls("void foo() noexcept(false);",
> > >                           "void foo() noexcept(true);", Lang_CXX11);
> > >   EXPECT_FALSE(testStructuralMatch(t));
> > > }
> > > ```
> > > 
> > > May be in these special cases it would worth to reuse the ASTStructuralEquivalenceContext class?
> > Definitely, good catch, @martong, thank you very much! @aaron.ballman, what do you think? If I see this right, `StructuralEquivalenceContext` is part of `libClangAST` so should be readily available.
> > 
> > The only issue I'm seeing is that this class takes non-**`const`** `ASTContext` and `Decl` nodes...
> Well, I started looking into putting up `const` whereever possible into the aforementioned type, but I hit a hurdle. When checking equivalence of records, the algorithm tries to ASTImport-complete the definition of a record if it's not fully defined yet... which is **not** a `const` method... I'll look into it some more, maybe we can do an overload on whether you're Structual equivalence checking in a const context or a non-const context.
Right. Unfortunately, it seems that fixing `StructuralEquivalenceContext` would require a set of nontrivial changes which, at least to my current understanding, would also require severely restructuring how the clients of that class use that class. The concrete example is that there needs to be a separate implementation for const and non-const record type equivalence checks, and the latter would not try importing, while the former continues to use the current logic. That is the part where "blindly" //putting const everywhere// just doesn't work anymore.

I would not dare to fix this for this patch right now, my understanding of the `ASTImporter` is not sufficient for details like this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75041