1369792 - Add a new rustfmt linter

Status: RESOLVED DUPLICATE of bug 1551078

(Developer Infrastructure :: Lint and Formatting, enhancement, P4)

Description

[Andrew Halberstadt [:ahal]]

In bug 1361342 there was some discussion about which rust linter would be most useful. We determined that clippy was likely the most powerful, but it currently requires rust nightly and isn't very stable. So while we wait for clippy to support rust stable (eta several months), we figured it would be worthwhile to stand up a rustfmt based linter.

https://github.com/rust-lang-nursery/rustfmt

Rustfmt is primarily a code formatting tool, but it has a 'checkstyle' mode that makes it behave like a linter. This bug will investigate adding it as a mozlint linter.

To start this linter will be opt-in, meaning module owners will be able to turn this on if they desire. If it turns out to be useful and gets wide buy-in, we can always switch it to opt-out at a later date.

Comment 1

[Andrew Halberstadt [:ahal]]

I started working on this, but quickly hit a wall. It looks like the --write-mode=checkstyle mode is quite broken. I filed the following issues:
https://github.com/rust-lang-nursery/rustfmt/issues/1635
https://github.com/rust-lang-nursery/rustfmt/issues/1636

I think I can still stand this up with --write-mode=diff, but it'll be a bit clunky. I'll try it out and see how it looks.

Comment 2

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1369792 - Add a rustfmt linter

Review commit: https://reviewboard.mozilla.org/r/146296/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/146296/

Comment 3

[Andrew Halberstadt [:ahal]]

The attached patch is a little hacky, but seems to work well! Things left to do here:

1) Bootstrap rustfmt, so the proper version gets installed automatically
2) Add rustfmt to the 'lint' docker image
3) Create a taskcluster task
4) Find a vict.. er, customer to be the first module to enable this

There might be some configuration gotchas to smooth over depending how rustfmt handles its configuration. For example, if cwd looks like:

- rustfmt.toml
- foo
  - rustfmt.toml
  - src
    -lib.rs
- bar
  - rustfmt.toml
  - src
    -lib.rs

Then we run:
rustfmt foo/src/lib.rs bar/src/lib.rs

What configuration gets used for each file?

Comment 4

[Andrew Halberstadt [:ahal]]

So far as I can tell no one has asked for a rustfmt linter. So I'm tempted to wait for clippy support and just go with that unless someone requests this in the meantime.

Comment 6

[Andrew Halberstadt [:ahal]]

Looks like that other bug is about actually formatting mozilla-central (and not about adding an ongoing linter). So this should probably be re-opened. I think rbartlensky might be looking into this.

Comment 7

[Robert Bartlensky [:rbartlensky]]

Attachment: Bug 1369792: Add rustfmt to ./mach lint.

Comment 8

[Robert Bartlensky [:rbartlensky]]

Could you please provide some feedback on this? Should I install rustfmt in a different way? At the moment I assume I can find cargo and rustfmt. The patch is available on Phabricator.

Comment 9

[Nathan Froyd [:froydnj]]

(In reply to Robert Bartlensky [:rbartlensky] from comment #8)
> Could you please provide some feedback on this? Should I install rustfmt in
> a different way? At the moment I assume I can find cargo and rustfmt. The
> patch is available on Phabricator.

Are you aware of https://bugzilla.mozilla.org/show_bug.cgi?id=1454764 ?  I guess this bug would be done in service of that one?

Comment 10

[Andrew Halberstadt [:ahal]]

Right, as far as I'm aware that bug is simply running rustfmt against mozilla-central, but it isn't putting in place any infrastructure to ensure we don't regress the format in the future.

I asked :rbartlensky to needinfo you as the module owner of https://wiki.mozilla.org/Modules/All#C.2B.2B.2FRust_usage.2C_tools.2C_and_style

Basically the main thing we want to know is whether:
1) this should be set up as a linter (so breaking the format either gets backed out or causes a lint warning)
2) this should be set up as part of |mach static-analysis| (which simply aids in running the format from time to time)

Personally I think 1) would be more useful, but I wanted to get your opinion and the opinion of people who work in rust every day.

Comment 11

[Andrew Halberstadt [:ahal]]

Also, applying the attached patch and testing out |mach lint -l rustfmt| would likely be more useful for you than trying to review the patch.

Comment 12

[Nathan Froyd [:froydnj]]

Yes, once things are formatted we want a linter enforcing style, cf https://bugzilla.mozilla.org/show_bug.cgi?id=1454777

Comment 14

[Andrew Halberstadt [:ahal]]

As far as bootstrapping goes, is vendoring rustfmt the way to go? Or should we try to set it up with cargo at runtime?

Comment 15

[Bobby Holley (:bholley)]

(In reply to Andrew Halberstadt [:ahal] from comment #14)
> As far as bootstrapping goes, is vendoring rustfmt the way to go? Or should
> we try to set it up with cargo at runtime?

I'll defer to Nathan if he feels differently, but I think host dependencies are the way to go here. We already do this for cbindgen, and so it's easy to just add another required thing to |cargo install|. The advantage of doing so is that it doesn't need to be rebuilt for each tree, and thus doesn't increase build times.

Note that, at the moment, you can't |cargo install rustfmt| because it's not 1.0 yet (the package is rustfmt-preview for now). That will change after the release.

Comment 16

[Nathan Froyd [:froydnj]]

(In reply to Bobby Holley (:bholley) from comment #15)
> (In reply to Andrew Halberstadt [:ahal] from comment #14)
> > As far as bootstrapping goes, is vendoring rustfmt the way to go? Or should
> > we try to set it up with cargo at runtime?
> 
> I'll defer to Nathan if he feels differently, but I think host dependencies
> are the way to go here.

This way was my inclination as well; doing things this way also transitions nicely to when we have the requirement of rustfmt'd code, and we need to install via bootstrap or whatever.

Comment 17

[Andrew Halberstadt [:ahal]]

Thanks, makes sense.

(In reply to Nathan Froyd [:froydnj] from comment #16)
> doing things this way also transitions
> nicely to when we have the requirement of rustfmt'd code, and we need to
> install via bootstrap or whatever.

Fyi this *is* the patch that is going to start requiring rustfmt'd code (depending on whether we decide to make infractions errors or warnings that is).

Follow-up question. Should we block on rustfmt 1.0? Or is it fine to move ahead with the preview for now. We could also start off logging infractions at the warning level (which is hidden by default and doesn't result in backout) in the interim.

Comment 18

[Bobby Holley (:bholley)]

(In reply to Andrew Halberstadt [:ahal] from comment #17)
> Follow-up question. Should we block on rustfmt 1.0? Or is it fine to move
> ahead with the preview for now. We could also start off logging infractions
> at the warning level (which is hidden by default and doesn't result in
> backout) in the interim.

I think it's better to wait for 1.0 for anything that either (A) requires us to reformat the codebase, or (B) requires developers to install tools on their machine. Any infrastructural progress we can make up until that point is fair game.

Comment 19

[Andrew Halberstadt [:ahal]]

In that case I propose we defer bootstrapping for now and make sure this is only running at the "warning" level.

In practical terms that means rustfmt issues won't be displayed by default via |mach lint| (though the total number of warnings in a file will show up in the little summary at the end) nor cause a backout. To actually see what the issues are, one would have to run with -W/--warnings.

Once rustfmt 1.0 comes out, we could implement bootstrapping and change the level from "warning" -> "error" (fixing any remaining issues at the same time).

Comment 20

[Emilio Cobos Álvarez (:emilio)]

Andrew, is there any chance to get something working with a particular rustfmt version? We already have the rust repack that can download a particular rust toolchain and includes rustfmt (we do this since bug 1432153).

We'd need this at least for the servo/ directory, since Servo is enforcing rustfmt on automation now.

We'd need to download the rust toolchain specified in: https://github.com/servo/servo/blob/master/rust-toolchain (I can keep those in sync in servo and the taskcluster files).

AFAIK the WIP here should be almost ready, and looking at the patch it should be easy to just find the binary in ~/.mozbuild or download it. I can take care of the part of getting the right version of the binaries in automation...

Is there any chance we could coordinate in getting this finished up?

Thanks!

Comment 21

[Andrew Halberstadt [:ahal]]

Yeah, bootstrapping is the hardest part left unimplemented. If you know how to handle that such that we can guarantee the same version of rustfmt is used everywhere (locally and in automation) then I can polish this up a little and get it landed.

Mozlint has a `setup` hook that will get called before the linters run. Here is an example:
https://searchfox.org/mozilla-central/source/tools/lint/eslint.yml#30

This value points to a `module:function`, in ESLint's case it's this one:
https://searchfox.org/mozilla-central/source/tools/lint/eslint/__init__.py#36

This function should a) check that everything is up to date, and b) update it if not. Maybe the best approach here would be for me to rebase and fix up this patch, then set up a stub `setup` function which just bails if rustfmt doesn't exist. Then maybe you could implement the bootstrapping on top of that commit.

Comment 22

[Andrew Halberstadt [:ahal]]

But first, Sylvestre, is anyone working on or planning to work on this? If so I'll leave it to them as I'm not sure how much time I can devote to it.

Comment 23

[Robert Bartlensky [:rbartlensky]]

I used to work on this, but I don't have the time to work on it at the moment. Feel free to assign anyone else to the bug.

Comment 24

[Emilio Cobos Álvarez (:emilio)]

Yeah, comment 21 sounds great, thanks!

Comment 25

[Sylvestre Ledru [:Sylvestre]]

AFAIK, nobody is actually working on it but Robert was trying to integrate that at review phase.
He fixed an issue for this:
https://github.com/rust-lang-nursery/rustfmt/commit/992b179d33360b9e623e287442e754c6a2f8805e

AFAIK, we would have to:
* Build or install it (we could download it like we are doing with clang-{tidy,format})
* Integrate that into mach lint
* Integrate the support in release-service to integrate that at review phase
* Get bug 1454764 landed

I would estimate 2 to 3 weeks of work to get it running in production. I Could find someone to work on it but this will be more in Q1 next year.

> Yeah, bootstrapping is the hardest part left unimplemented. If you know how to handle that such that we can guarantee the 
> same version of rustfmt is used everywhere (locally and in automation) then I can polish this up a little and get it landed.
In mach, we could just enforce the usage of the same version like clang-format.
For coding style tools, there isn't much solutions to avoid useless diff between versions.

Comment 26

[Andrew Halberstadt [:ahal]]

Sorry it took me awhile to look into this. After spending some time trying to get this working nicely with the "linting" paradigm, I'm starting to change my mind and think this would be better off implemented similarly to |mach clang-format| instead.

While :rbartlensky's patch works nicely enough, there are quite a lot of hacky things we need to do to make it "look" like a linter. Some examples:

1. Since `cargo fmt` can't accept paths and just runs on an entire crate (or all of mozilla-central in our case), we can't *just* format the specified files that were passed in. Robert got around this by post-filtering the issues after the fact, so essentially we would always format the entire tree, but then discard any issues that we didn't "care" about. This works because `cargo fmt` is super fast, but it's still kind of awkward.

It's worth noting that the `rustfmt` binary does accept file paths (though not directories). However when I try to use this directly it seems to hang (or is at least *much* slower than `cargo fmt`)

2. This implementation relies on regexes and text processing to work, so will be fragile (compared to most other linters which provide some sort of structured output).

3. Outputting diffs mixed in with the other |mach lint| output looks pretty weird.

Comment 27

[Andrew Halberstadt [:ahal]]

That being said.. I think I have an idea to move forward with this that wouldn't be too much extra work and can re-use a lot of what :rbartlensky and myself worked on in here.

I think it would be prudent to have a single |mach format| command that can run various formatters from various languages all at once (otherwise, we'll end up with |mach clang-format|, |mach rustfmt|, |mach yapf|, etc). My idea would be to keep using mozlint to support this, but to implement it as a separate "instance" from |mach lint|. So entirely different configuration directory (e.g tools/format), entirely different mach command (e.g mach format), but same underlying library to manage files and job dispatching.

Fwiw, the library "mozlint" is horribly named. It is actually a general purpose job dispatch tool that allows you to register "arbitrary tasks" that take file paths as their inputs. This description can apply equally as well to formatters as it does to linters.

To summarize, I propose:

1. Do some light refactoring of mozlint to stop assuming linting
2. Create a tools/format directory that contains formatter implementations as well as a new |mach format| command
3. Build ./mach format -f rustfmt
4. Migrate |mach clang-format| to this new system

I'll file a new bug as this is getting way out of scope here. Also if adding rustfmt is still time sensitive, I'm still ok with forging ahead here, though as long as we remove it once the |mach format| proposal gets landed.

Comment 28

[Andrew Halberstadt [:ahal]]

I filed bug 1511122. In there you'll find a quick and dirty patch I wrote that, while very very rough, does work for the purpose of preventing format infractions in CI.

After applying:
$ ./mach format -l rust --check

I would personally prefer the approach in that other bug to the patch here, as I suspect would others. Though I'd want to do some refactoring of "mozlint" (including a name change) before landing that other patch, and I'm not sure when/if I'll have time for that.