Page MenuHomePhabricator

[clang-tidy] Extend 'bugprone-easily-swappable-parameters' with `typedef` and `const &` diagnostics
ClosedPublic

Authored by whisperity on Jan 30 2021, 6:51 AM.

Details

Summary

The base patch only deals with strict (canonical) type equality, which is merely a subset of all the dangerous function interfaces that we intend to find. In addition, in the base patch, canonical type equivalence is not diagnosed in a way that is immediately apparent to the user.

This patch extends the check with two features:

  1. Proper typedef diagnostics and explanations to the user.
  2. "Reference bind power" matching.

Case 1 will tell the user the common type of "SomeIntegerType" and "SomeOtherIntegerType" is "int" or "my_int" and "int" are the same. This makes the (otherwise, through CanonicalType desugaring already handled and diagnosed, but not explained) typedef case obvious to the user without having to invoke code comprehension tools.

Case 2 is a necessary addition because in every case someone encounters a function f(T t, const T& tr), any expression that might be passed to either can be passed to both. Thus, such adjacent parameter sequences should be matched. This is specifically modelled for const& only, as any other pair combination from "value, ref, qualified ref, rref" have some broad value category in which some actual swap of the arguments at a call would be a compiler error. (E.g. f(int, int&) and f(2, Val) breaks if swapped, similarly how rrefs cannot bind lvalues, etc.) This case broadens the result set, but only with the most dangerous and "Please think about this and refactor..." case.

(Note, that Case 2 does not model things like f(int, std::reference_wrapper<const int>).)

Due to the cases above always being a problem (and there isn't a good enough reason to say typedefs should not be diagnosed, everyone hates the weak typedef...), I decided not to make matching these configurable.

This patch also serves as a proof-of-concept on how I wish the check to evolve over some more subsequent patches, which all aim to introduce user-toggleable modelling.

Originally, the implementation of this feature was part of the very first patch related to this check, D69560 (diff 259320). It has been factored out for clarity and to allow more on-point discussion.

Diff Detail

Event Timeline

whisperity created this revision.Jan 30 2021, 6:51 AM
aaron.ballman added inline comments.Feb 2 2021, 8:32 AM
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
153–177
281–293

"Dissolve certain type sugars" has to be one of my new favorite comments. :-D

284
289
307–308

I think this is reasonable but it does raise a question about implicit conversions, which I suppose is more of a generic question about the check. Should the check consider these to be easily swappable parameter types?

struct S {
  S(int);
  operator int() const;
};

void func(S s, int i);

given that you can call func() like:

S s;
int i;

func(i, s);
func(s, i);

(Also, does the answer change if S has more types it can convert to/from? Or does the answer change if S can only convert from a type (or convert to a type) rather than convert both ways?)

324

It's not really unqualified since it could still have volatile (etc).

522–523
528–529

Might as well hoist this to be above the public access specifier (and can drop the private access specifier entirely at that point).

whisperity added inline comments.Feb 2 2021, 8:57 AM
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
153–177

How in the... Okay, I think I need to up my rebase game because I know for a fact that I fixed BrE to AmE in the parent patch and that patch was merged into the branch this patch was created from and this patch is created against the parent, obviously.

284

Is const respected properly in this case, will it not try to cast it away? Wow... I did not know this!

307–308

Yes, yes, yes and yes! That is the whole idea, that is one of the things in which we are "better" than the existing works... and one of the reasons behind this whole "argument swapping" deal is much worse in C++, than let's say Java. (A lot of existing literature deals with Java, or deals with C++ but only in terms of type equivalence, or not considering types -- only arg and param names -- at all.) That one shall be diagnosed, but the whole implicit conversion "mess" is coming in a later patch!
In fact, the patch for implicit conversions is already up, but in a will be reworked state.

Unfortunately, I can't seem to find a way to link a particular line of a particular diff, so let's do it manually. The ...-implicits.cpp test file in D75041 diff 315384 starting from line 95 shows this being dealt with.

Originally, implicit conversions were warned about when they were only unidirectional (i.e. int -> S possible, S -> int not possible, fn(int{}, S{}) warned). However, this created a lot of false positives even in the two or three projects I went over manually, so... for the time being, I reworked the patch (only changing like 2 lines at most!) to only deal with bidirectional implicit conversions. Because in case you say fn(5, 5) it still accepts, but if you have a valid s, fn(s, 5) is a compile error.
(You can check the earlier diff at the same location for this case.)

This becomes a deep rabbit hole way too fast, because one layer of the analysis is "warn about f(T1, T2) if f(T2, T1) is also possible". That's the idea I'm trying to sell right now. Saying "warn about f(T1, T2) if there exists some type Tx with which f(Tx, Tx) is possible" is a whooole lotta harder to handle. Maybe not impossible, but definitely harder, and with most likely introducing a plethora of weird false positives.

307–308

And once implicit conversions are in, a function definition like fn(int, const S&) will say that it can be mixed up because S::S(int) is a thing and then an S{} can be passed badly to a const S&.

There is also another, currently not yet uploaded part of the patch, which deals with whether fn(int, const int) (note no &!) should be deemed mixable. That one will most likely be behind a user configurable option... Is memcpy(void* dest, const void* source) mixable? Not if you have const void* as a local variable... Which, given the fact that you told me in D69560 that I should drop the const from local variables... means that I'll make the mistake of going memcpy(src, dst) more easily, than if I kept my consts on my locals. But this is some sort of a more philosophical question. C++CG says const T, T is not to be warned about. I say it's still an issue as every non-const implicitly converts to const. But that's why it's best to leave it up for the user to decide.

376

@aaron.ballman Should I say "invalid" here, or "sentinel" here? (Cf. the assert in sanitize().) It's more of a sentinel value... which is also invalid when you want to consider it a valid enum constant to be used for the intended purpose. Full 0...0 bit pattern means someone somewhere messed up great deal. 0...01 (little endian) bit pattern means we concluded there isn't a possible mix. Etc. with the other bits.

whisperity marked 6 inline comments as done.
  • NFC Code style, formatting, wording, etc. changes as per review comments.
  • NFC Removed a stale FIXME
  • [NFC] Reformat the documentation so option values are in single backticks.
aaron.ballman added inline comments.Mar 16 2021, 9:31 AM
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
735–736

I think "with the same force" is going to be hard for users to make sense of. I'm at a bit of a loss for how to word it though. The issue is that T and const T& parameters *might* be easily swapped, so maybe it's best to call it out that way?

whisperity added inline comments.Mar 16 2021, 10:29 AM
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
734

This is a stale FIXME, the uniqueing of the emission is in the line right above...

735–736

This is a note tag to explain the reason behind the mix. The warning itself has been emitted before. How about

a T and const T& parameter accepts all values of T

?

aaron.ballman added inline comments.Mar 16 2021, 10:54 AM
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
734

Heh, I was sort of wondering about that. Please remove the FIXME before committing.

735–736

I think that'd be significantly more clear, yes!

whisperity marked 4 inline comments as done.
  • Made the diagnostic message about "binding power" easier to understand.
whisperity marked an inline comment as done.Mar 18 2021, 7:41 AM
This revision is now accepted and ready to land.Mar 18 2021, 8:00 AM

[NFC] Rebase.

NFC: Rebase & update.

NFC Fix lint.