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

Mon Feb 8 12:34:39 PST 2021

aaron.ballman added inline comments.

================
Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:177
+                                       const std::size_t StartIndex) {
+  const std::size_t NumParams = FD->getNumParams();
+  assert(StartIndex < NumParams && "out of bounds for start");
----------------
whisperity wrote:
> whisperity wrote:
> > aaron.ballman wrote:
> > > whisperity wrote:
> > > > aaron.ballman wrote:
> > > > > Some interesting test cases to consider: varargs functions and K&R C functions
> > > > > K&R C functions
> > > > 
> > > > Call me too young, but I had to look up what a "K&R C function" is, and I am absolutely baffled how this unholy construct is still supported! **But:** thanks to Clang supporting it properly in the AST, the checker works out of the box!
> > > > 
> > > > Given
> > > > 
> > > > ```
> > > > int foo(a, b)
> > > >   int a;
> > > >   int b;
> > > > {
> > > >   return a + b;
> > > > }
> > > > ```
> > > > 
> > > > We get the following output:
> > > > 
> > > > ```
> > > > /tmp/knr.c:2:3: warning: 2 adjacent parameters of 'foo' of similar type ('int') are easily swapped by mistake [bugprone-easily-swappable-parameters]
> > > >   int a;
> > > >   ^
> > > > /tmp/knr.c:2:7: note: the first parameter in the range is 'a'
> > > >   int a;
> > > >       ^
> > > > /tmp/knr.c:3:7: note: the last parameter in the range is 'b'
> > > >   int b;
> > > >       ^
> > > > ```
> > > > 
> > > > (even the locations are consistent!)
> > > > 
> > > > Should I add a test case for this? We could use a specifically C test case either way eventually...
> > > > 
> > > > -----
> > > > 
> > > > > varargs functions
> > > > 
> > > > This is a bit of terminology, but something tells me you meant the //variadic function// here, right? As opposed to type parameter packs.
> > > > 
> > > > Given
> > > > 
> > > > ```
> > > > int sum(int ints...) {
> > > >   return 0;
> > > > }
> > > > ```
> > > > 
> > > > the AST looks something like this:
> > > > 
> > > > ```
> > > > `-FunctionDecl 0x56372e29e258 <variadic.cpp:1:1, line:3:1> line:1:5 sum 'int (int, ...)'
> > > >   |-ParmVarDecl 0x56372e29e188 <col:9, col:13> col:13 ints 'int'
> > > > ```
> > > > 
> > > > Should we diagnose this? And if so, how? The variadic part is not represented (at least not at first glance?) in the AST. Understanding the insides of such a function would require either overapproximatic stuff and doing a looot of extra handling, or becoming flow sensitive... and we'd still need to understand all the `va_` standard functions' semantics either way.
> > > > Call me too young, but I had to look up what a "K&R C function" is, and I am absolutely baffled how this unholy construct is still supported!
> > > 
> > > Ah, to be innocent again, how I miss those days. :-D
> > > 
> > > > Should I add a test case for this? We could use a specifically C test case either way eventually...
> > > 
> > > I think it'd be a useful case, but the one I was specifically more concerned with is:
> > > ```
> > > // N.B.: this is C-specific and does not apply to C++.
> > > void f();
> > > 
> > > int main(void) {
> > >   f(1, 2, 3.4, "this is why we can't have nice things");
> > > }
> > > ```
> > > where the function has no prototype and so is treated as a varargs call.
> > > 
> > > > This is a bit of terminology, but something tells me you meant the variadic function here, right? As opposed to type parameter packs.
> > > 
> > > Yes, sorry for being unclear, I am talking about variadic functions.
> > > 
> > > > Should we diagnose this? And if so, how? The variadic part is not represented (at least not at first glance?) in the AST. Understanding the insides of such a function would require either overapproximatic stuff and doing a looot of extra handling, or becoming flow sensitive... and we'd still need to understand all the va_ standard functions' semantics either way.
> > > 
> > > Well, that's what I'm wondering, really. The arguments are certainly easy to swap because the type system can't help the user to identify swaps without further information (like format specifier strings). However, the checking code would be... highly unpleasant, I suspect. My intuition is to say that we don't support functions without prototypes at all (we just silently ignore them) and that we only check the typed parameters in a variadic function declaration (e.g., we'll diagnose `void foo(int i, int j, ...);` because of the sequential `int` parameters, but we won't diagnose `void foo(int i, ...);` even if call sites look like `foo(1, 2);`). WDYT?
> > It is definitely highly unpleasant, at one point I remember just glancing into the logic behind `printf()` related warnings in Sema and it was... odd, to say the least.
> > 
> > That is how the checker works right now. It will not diagnose `int f() {}` because from the looks of the function definition, it is a 0-parameter function. Same with the variadic `...`, it is a single parameter function (which has a rather //weird// parameter type).
> > 
> > But yes, I think I'll make this explicit in the documentation (and FWIW, the research paper, too). 
> > 
> > > However, the checking code would be... highly unpleasant, I suspect.
> > 
> > Incredibly, because this check purposefully targets the function definition nodes only, like a cruise missile. For that logic, we would need to gather call sites for variadic functions and start doing some more magic with it.
> > 
> > FYI, templates are also handled in a way that only what is clear from definitions (and not instantiations... I think we can kind of call a particular "overload" call to a variadic like a template instantiation, just for the sake of the argument here) only:
> > 
> > ```
> > template <typename T, typename U>
> > void f(T t1, T t2, U u) {} // [t1, t2] same type, WARN.
> > 
> > void g() { f(1, 2, 3); }} // NO-WARN: Even though "U" in this call is also "int", this is not fixed "easily enough" in the definition, so to prevent more false positives, we shut up.
> > 
> > template <>
> > void f(int i, int j, int k) {} // [i, k] WARN.
> > ```
> > 
> > Anything that is only known through template instantiations is context-sensitive (the subsequent patch about typedef extends the tests and diagnostics about this), and thus, we forego trying to find the way.
> > 
> > ```
> > template <typename T> struct vector { using value_type = T; };
> > 
> > template <typename T>
> > void f(T t, typename vector<T>::element_type u) {} // NO-WARN, dependent.
> > 
> > template <>
> > void f(int i, vector<int>::element_type i2) {} // WARN, can explicitly unfold the typedef and see "int == int".
> > 
> > template <typename T>
> > void g(typename vector<T>::element_type e1, typename vector<T>::element_type e2) {} // WARN, not dependent.
> > ```
> I definitely shall create and add a C file with test cases like this, marking them `// NO-WARN`, and explaining the reasoning. If for nothing else, maybe to know in the far future what can be improved upon.
> I definitely shall create and add a C file with test cases like this, marking them // NO-WARN, and explaining the reasoning. If for nothing else, maybe to know in the far future what can be improved upon.

Excellent, that's the solution I was hoping we'd come to! I find it's useful to add the C test for a function without a prototype (`void f()`) because some code that expects all function declarations to have a function prototype will assert or crash, so it helps shake out some bugs.

Also, I agree with your interpretation of the template case -- it's another intractable problem at this stage, so best to document how we handle it and move on.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69560