Consider normalising match reference style

[chris-morgan]

Consider these two examples:

match x {
    &A => ...,
    ...
}

match *x {
    A => ...,
    ...
}

The two are not precisely equivalent, because the first only works if x is an immutable reference while the second dereferences and is thus more general. But any instance of the former can be converted to the latter form with no change in functionality.

I personally would like to see rustfmt normalising the former to the latter.

[sinhpham]

I disagree, a formatting tool should not rewrite the code in any way, this should be the job of a linter.

[nrc]

trouble is a linter doesn't usually fix it just gives an error/warning. So we have to bend the rules for one tool or another. My current thinking is that rustfmt should rewrite code beyond simple formatting, but it should always be opt-in via config. We'll discuss this more in the formatting RFC process.

[regexident]

Wouldn't it be a somewhat better fit to add "fixit"s to a linter, such as clippy? For a linter it would be a logical progression from "hey, this is bad!" to "hey, this is bad, lemme fix it for ya!", while still staying in the domain of linting (after all for many things that clippy emits a fix could be added "easily", like -1 * foo -> -foo), unlike a formatter, which I, too, would expect to not apply any semantic transformations. I'd suppose that it would also be easier for a linter to add modular and maintainable rewriting rules than to integrate such rules into rustfmt's already quite complex (albeit totally justified) logic.

/cc @llogiq, @Manishearth

[Manishearth]

I do want both clippy and Rust to be able to rewrite code to fix warnings (e.g. unused imports, etc). However, this needs a smart rewrite engine which we don't have right now. If someone is interested on writing this I'd be love to give further ideas on how to do it

[llogiq]

We already do have some suggestions (as in "replace this span with this code") that would fit this idea, but before we activate them we want to be sure that they don't introduce errors. I for one would not want to activate them for general consumption before we have some safety net in place (i.e. try and compile the modified code).

[gnzlbg]

trouble is a linter doesn't usually fix it just gives an error/warning.

Clang provides two tools that are analogous to rustfmt (clang-format) and clippy (clang-tidy),
where clang-format only deals with formatting (not even renaming), and clang-tidy does everything else.

Both tools can apply the suggested fixes and both tools promise that your code will still compile. The difference is that while after applying clang-format your code compiles and your tests do pass, after applying clang-tidy your code rarely compiles and you are typically left with a mess of a code base (some lints are actually cyclic..).

IMO the two main reasons clang-tidy automatic fixing does not work are that most of lints have false positives, and even those that do not, the fixes suggested are still sometimes not the best possible fix. Detecting an issue is hard, proposing a fix is, in general, way way harder. For some reason clang-tidy tries to fix all your issues and well... it does a very poor job at that.

Talking about the case above, @nrc, what if the match has only one arm and the best fix is to remove the match altogether? should rustfmt deal with this as well? where does it end?

IMO:

• rustfmt should deal only with formatting without performing any semantic transformation of any kind (this is already hard enough, but it is something that can be done reliably: users should be able to use rustfmt blindly)
• clippy could, in the presence of a rewrite engine, fix issues automatically, but:
  • it should only do so for trivial things that have zero false positives (like the match case above),
  • after applying the fixes it should check that the code still compiles and passes the tests (and revert the fix automatically otherwise).
  • differentiate between suggesting the user about how to fix an issue and automatic ways of fixing issues (e.g. just because a lint has a suggestion about how to fix something does not mean that the suggestion is general enough to be applied automatically in all cases).

[nrc]

@gnzlbg I've actually already suggested one-arm match arms as something Rustfmt could fix: #340.

It may be that we want a third tool, Rust-tidy or something to handle semantic changes. I see two drawbacks with this - rust-tidy would need to be able to format code (at least the code it generated) so would need to include rustfmt, and from a user's perspective why should it be two tools?

I actually see some clearer lines between some of the 'tidy' changes and some of the 'clippy' lints - specifically, 'can it be mechanically fixed with minimal user input'. One might also draw a line about whether the change requires semantic or purely syntactic information. That is subtly different from whether the change itself is purely syntactic.

FWIW, my vision here is probably around a single tool which can do formattinf, refactoring, and 'tidying' based on lints, plus accept mini-scripts for doing these things. Basically anything that requires rewriting code of any kind. The user would choose a mode to operate in, rather than choosing between tools. That is somewhat long term though.

[llogiq]

I'm in talks with the core team to extend the suggestions API to allow us to tell rustc whether a specific suggestion is directly applicable (perhaps also change the API to make applying suggestions easier / more flexible). With that + JSON output, IDEs could then offer applicable suggestions as quick fixes, and it will work for every lint, not just clippy's.

[Manishearth]

Oh, perfect. I have toyed with an idea of a substitution API for rust that uses rustfmt to do things sanely, but it sounds like a lot of work and I'm glad someone else is doing it 😄

[nrc]

SOmewhat out of date since Clippy can fix this and match shorthand means it's not usually necessary