New [suspicious_arguments] lint for possibly swapped arguments

[5225225]

changelog: [suspicious_arguments]: add lint for probably accidentally swapped arguments

---

[rust-highfive]

r? @llogiq

(rust-highfive has picked a reviewer for you, use r? to override)

[5225225]

let defnames: Map<&str, usize> = ...;
let to_def_idx: Vec<Option<usize>> = vec![None; definition.len()];

for (i, _) in call.iter().enumerate() {
  // set to_def_idx[i] to Some(_) if the name matches a defname
}

let mut cycle = Vec::new();
for callidx in 0..to_def_idx.len() {
  cycle.clear();
  let mut cur = callidx;
  while let Some(defidx) = to_def_idx[cur].take() {
    cycle.push(cur);
    cur = callidx;
  }
  if Some(start) = cycle.iter().position(|e| *e == cur) {
    // cycle detected at `cycle[start..]`, emit lint
  }
}

regarding bigger permutations, I feel it should be possible to do well
and not that complex
record the edges 'callsite name' -> 'param name' in a Vec<Option>
and you can just follow the graph along to find perms
so, to_def_idx defines a (partial) permutation between the args indices
then, the idea of my algorithm is to follow the edges of the permutation until you find a cycle

A thought of a better algorithm that would detect tri_rotate from @moulins

Untested but worth considering

[bors]

☔ The latest upstream changes (presumably #9288) made this pull request unmergeable. Please resolve the merge conflicts.

[moulins]

Here's a better version of the algorithm I sketched out yesterday on Discord, with bugs fixed and some explanatory comments:

// Take care of degenerate cases.
if def_args.len() != call_args.len() || call_args.len() < 2 {
  return;
}

// The mapping of argument names to their 'correct' position.
let def_names: Map<&str, usize> = ...;

// For each call argument, the index of the matching name in the definition.
let to_def_idx: Vec<Option<usize>> = def_args.iter().map(|arg| {
  arg_to_ident_str(arg).and_then(|name| def_names.get(name))
});

let mut chain = Vec::new();
for call_idx in 0..to_def_idx.len() {
  // Follow the 'chain' of indices until we get stuck.
  // We remove the links as we traverse them to avoid infinite loops in cycles.
  chain.clear();
  let mut cur = call_idx;
  while let Some(def_idx) = to_def_idx[cur].take() {
    cycle.push(cur);
    cur = def_idx;
  }

  // If the chain contains `cur`, then we have a cycle. Note that 1-cycles shouldn't be linted,
  // as they correspond to well-positioned arguments, and so we ignore the last element.
  let cycle_start = {
    let mut iter = chain.iter();
    iter.next_back();
    iter.position(|e| *e == cur)
  };

  if Some(start) = cycle_start {
    let cycle = &chain[start..];
    // We have found a cyclic permutation of the arguments, emit the lint.
  }
}

[bors]

☔ The latest upstream changes (presumably #9295) made this pull request unmergeable. Please resolve the merge conflicts.

[Alexendoo]

You can use an // aux-build comment to test the separate crate version without running into these path issues

[llogiq]

Sorry for being absent for so long and thanks to @Alexendoo for taking over the review. I have a few nitpicks and one question, otherwise this looks mergeworthy.

[llogiq]

Is there a specific reason for using the 2015 edition? We usually try to have all tests against the current edition, so if there is something specific to rust-2015, we should probably factor out a suspicious_argument_like_its_2015.rs or something.

[5225225]

Yeah, I was testing anonymous args, which you can only get with a trait in 2015. I'll move that to a different file.

[llogiq]

There is quite some rightwards drift here. We can use either the if_chain macro or the recently reverted-on-stable-but-available-on-nightly let-chains feature (basically if let Some((arg, call_span)) = arg_and_span && let Some(&def_idx) = idxs.get(arg) && call_idx != def_idx && .. etc.).

[llogiq]

Suggested change

	let mut def_args = Vec::new();
	for ident in cx.tcx.fn_arg_names(def_id) {
	def_args.push((ident.to_string(), ident.span));
	}
	let def_args = cx.tcx.fn_arg_names(def_id).map(|ident|{
	(ident.to_string(), ident.span)
	}).collect::<Vec<_>>();

[llogiq]

Suggested change

	let mut call_args = Vec::new();
	for call_arg in args {
	call_args.push(guess_ident(call_arg));
	}
	let call_args = args.map(guess_ident).collect::<Vec<_>>();

[llogiq]

Your call to fn_arg_names probably needs a bit more than just a bare tcx, as it seems to cause an ICE in the test. I'm not exactly sure why right now, might look into it later. Also you'll need to cargo dev fmt on the code.

[afetisov]

It's not uncommon to have dummy parameter names which don't indicate anything, and thus cannot be reasonably used to guess accidental swaps. mem::swap in the tests is one such example, but there are many others. I would be very annoyed if the lint fired in those situations.

One example is arithmetic traits. It's common to implement unordered operation for different types based on a swap of some canonical order. E.g.

impl std::ops::Add<Foo> for Bar { /* snip */ }

impl std::ops::Add<Bar> for Foo {
    type Output = <Bar as Add<Foo>>::Output;
    fn add(&self, other: &Bar) -> Self::Output {
        std::ops::Add::add(other, self)
    }
}

I'm not sure if this specific lint will fire on this specific example (self probably doesn't have an associated identifier, does it?), but the pattern itself is quite common for essentially symmetric functions and traits.

It's hard to detect dummy identifiers in general, but the specific case of one-letter names is easy and common. It's unreasonable to expect that if I name my indexing variables i, j, k, or my closures f, g, h, then those names carry any meaning and must be matched with whatever functions I pass them to. And what should I do if the functions themselves name the arguments inconsistently? What should I do if the arguments with essentially the same semantics (e.g. closure for default value and for mapping, like in Option::map_or_else) are named g, f in one function, and f, g in the other?

Also, the lint ignores argument types (even has an explicit test for that with itertools::all(f, iterable) on line 93). But that would already be an error in the compiler. What's the point of emitting a warning on the same items and for the same reason as the error? That just causes notification overload.

[blyxyas]

@5225225 I'm going to close this for now (365 days without activity), feel free to reopen if you still want to work on it! ❤️

PS: In this blog post, you can use SVG Filters instead of JS for better performance (letting the GPU do the rendering work instead of modifying the image with the CPU) ฅ՞•ﻌ•՞ฅ