Implement Interchangeable_params lint

[jcarey9149]

changelog: [interchangable_params]: Add interchangable_params lint

Fixes #16735

This PR implements issue #16735, which suggests a new lint that examines the types of parameters of a function and suggests that any repeated types get newtype values. Doing this will help prevent accidentally changing the order of the parameters by helping the compiler detect the issue.

[rustbot]

r? @samueltardieu

rustbot has assigned @samueltardieu.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: 7 candidates
• 7 candidates expanded to 7 candidates
• Random selection from Jarcho, dswij, llogiq, samueltardieu

[github-actions]

Lintcheck changes for 9d6ccc8

Lint	Added	Removed	Changed
clippy::interchangeable_params	273	0	0

This comment will be updated if you push new changes

[samueltardieu]

The lint is not ready yet, as it has many issues:

• It suggests creating struct in an impl block, which is not possible. Even if the structs were created outside the impl block, the lint would fail on things like

  impl Foo {
      fn f(self, other1: &Self, other2: &Self) {}
  }

  as it would suggest wrapping Self, which is not possible outside the impl. The situation would also be identical with any dependent type, or with generic parameters.
• It will suggest creating new struct from the parameter's capitalized name, which may conflict with the existing types, as in

  fn f(string: String, a: String);

  which would fail on the recursive struct String(String); type definition.
• Generic parameters cannot be used outside the function definition. For example,

  fn do_nothing<T>(a: T, b: T) {}

  would suggest

  struct X(T);
  struct Y(T);
  fn do_nothing<T>(x: X, y: Y) {}

  which is not valid Rust.

Also, in general, this lint does too many snippet manipulation and string creation. It would probably be better to use the clippy_utils::source functions to get references to the source, and bail out if any snippet cannot be obtained.

Parsing for { or ( might also not be robust in presence of inline comments inside the function definiiton.

View changes since this review

[samueltardieu]

Using .split_once() would avoid unnecessary work. Also, this seems to be used to get the indentation only, which should be done only when emitting the lint and doesn't require manipulating the span.

Also, in which case wouldn't the snippet contain {? In this case, wouldn't it be better to bail out from the lint?

[samueltardieu]

Ditto, only used when emitting the lint, should be deferred. And same question about bailing out if snippet_opt() returns None.

[samueltardieu]

Again, split_once() would be better.

[samueltardieu]

Two typos: "interchangeable parameters". The message is not clear enough though.

[samueltardieu]

Why use the textual representation instead of the TyKind? Wouldn't that be possible?

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[jcarey9149]

Thank you for your feedback.

[ada4a]

This currently lints in closures, as seen in:

warning: interchangable paramters
   --> target/lintcheck/sources/ahash-0.8.11/src/random_state.rs:265:19
    |
265 |         let mix = |l: u64, r: u64| {
    |                   ^^^^^^^^^^^^^^^^
    |
help: consider writing
    |
265 ~         let mix = struct L(u64);
266 +         struct R(u64);
267 +         /* .. */
268 +         |l: u64, r: u64| {
269 ~             let mut h = hasher.clone(l: L, r: R) {
    |

Closure are most often just used as local helpers, so I'd argue that adding newtypes for their parameters isn't really worth it.

View changes since the review

[jcarey9149]

Not ready for primetime yet. Changed strategy, detects impl functions and closures. Not making suggestions yet.