1361342 - (clippy) [meta] Clippy for Firefox

Status: NEW

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

Description

[Bob Clary [:bc] (inactive)]

Clippy <https://github.com/Manishearth/rust-clippy> appears to be one possibility in terms of linting Rust code. It requires a nightly rust installation however.

This bug is to determine what setup steps are required to install Clippy and the required nightly rust toolchain and how to actually lint rust code using it.

Comment 1

[Manish Goregaokar [:manishearth]]

Later this week we're having a meeting about https://github.com/rust-lang/rust/issues/40921 , which is the path for Clippy on stable.

Comment 2

[Bob Clary [:bc] (inactive)]

manishearth: Totally great! Thanks, I'll be there.

Comment 3

[Manish Goregaokar [:manishearth]]

(No need to be there, just letting you know that the nightly requirement should be going away. The bug will be updated after the meeting)

Comment 4

[Bobby Holley (:bholley)]

My understanding is that clippy is somewhat opinionated and controversial, but maybe that's being addressed somehow.

FWIW, Servo uses tidy.

Comment 5

[Manish Goregaokar [:manishearth]]

Most lints are not opinionated. Some are, but you can just disable those. (Clippy's pedantic mode is much more opinionated but you don't need to use it; most folks don't)

rustfmt is more opinionated than clippy is :)

(Tidy is a very small set of checks, and is mostly a mini-rustfmt, not overlapping with the domain of what Clippy does)

Comment 6

[Andrew Halberstadt [:ahal]]

Hey manishearth, any updates from that meeting?

Bobby, ultimately you and the other gecko devs working on rust will be the ones dealing with this tool. Could you expand on your reservations about clippy? Is the ability to disable specific lints good enough? Gecko devs would have full control over the lint configurations (what lints to run and where to run it).

For the record, I have no rust experience and haven't used any of these tools, so want to make sure we actually implement something the gecko devs want. My uneducated opinion is that clippy looks the most powerful, but if there is a general preference for rustfmt or tidy, we could post to a mailing list for further discussion.

Comment 7

[Bobby Holley (:bholley)]

I haven't actually used clippy myself, and so my concerns were a proxy for the ones emilio expressed to me. Needinfoing him.

Also needinfoing jack, since at least for the shared servo code this is really more his call than ours.

Comment 8

[Andrew Halberstadt [:ahal]]

I think anything whose canonical source lives outside of m-c, shouldn't be linted from m-c.. So I imagine we'd be excluding servo from whatever gets built here. But I have very little clue how this is all setup.. so maybe I'm wrong.

Comment 9

[Manish Goregaokar [:manishearth]]

I suspect he means the style code that lives in both m-c and servo.

(Clippy only lints one crate at a time so restricting crates is easy)

Clippy is meant to be used while disabling lints so not liking a particular lint should not be a case for skipping clippy. That said, I'd like to hear which lints are not preferred. Generally clippy follows rust style, so these end up being domain specific (like disabling the float comparison lint on a crate that deals with floats from CSS)



Regarding the stability discussion: the compiler team is pretty busy so it hasn't been discussed much in a meeting yet.

The main discussion is at https://internals.rust-lang.org/t/rust-ci-and-submodule-crates/5232/6

Comment 10

[Emilio Cobos Álvarez (:emilio)]

I'm not fond of some lints (particularly the single_match_else one). But assuming we arrive to a set of lints we all agree with, and disable the ones there isn't agreement about, I'd be fine with clippy.

Comment 11

[Manish Goregaokar [:manishearth]]

Oh, because it sometimes spans more LOC? Meh.



I'm *really* not fond of micromanaging style lints for minor reasons. We delayed rustfmting clippy so long because we couldn't decide on the config, finally landed it with the default config and it was fine. The risk we run here is that other folks will express the exact opposite preference and you reach an impasse due to a minor reason that was actually never a problem before.

That said, I personally don't mind either way for this lint, and hope others don't either, so this may not be a problem.

Comment 12

[Emilio Cobos Álvarez (:emilio)]

(In reply to Manish Goregaokar [:manishearth] from comment #11)
> Oh, because it sometimes spans more LOC? Meh.

Not only that, but also because it suggests to change code like:

let foo = match bar {
    Some(b) => b,
    None => return,
};

to

let foo = if let Some(b) = bar {
    b
} else {
    return;
}

Which isn't nearly as clear.

Comment 13

[Manish Goregaokar [:manishearth]]

That actually sounds like something we could ignore in clippy itself. You're right that it is pretty clearly more readable. File a bug with some example heuristics?

Comment 14

[Andrew Halberstadt [:ahal]]

I did a bit of experimentation, braindump below.

clippy:
- Requires compilation, can only check an entire crate (no file level checking)
- Would not integrate well with the existing |mach lint| infrastructure
- Need to figure out how to run this properly. E.g, running `cd gfx/webrender && rustup run nightly cargo clippy` had a compile error.. not sure if this is because of using rust nightly or if running clippy like this is incompatible with the mozilla-central build system. Might need to use it as a compiler plugin and do a |mach build|?

rustfmt:
- Seemed to work well with --write-mode=checkstyle
- Allows file-level linting, no compilation
- Should be relatively easy to integrate with existing mozlint infra
- Would be nice if errors had an associated code or name to identify them

tidy:
- Couldn't seem to get it working
- Not many docs, doesn't seem to check much
- Willing to write this one off for now

I'm tempted to stand up a rustfmt based linter, if for no other reason than it's clear to me how to do it and shouldn't take terribly long to do. Also, if clippy ends up requiring a |mach build|, would developers still prefer it? There's also the possibility of using rustfmt for lightweight on-the-fly linting, but then providing `ac_add_option --enable-clippy` or something like that as a build variant primarily meant for catching stuff in CI.

Comment 15

[Manish Goregaokar [:manishearth]]

> Might need to use it as a compiler plugin and do a |mach build|?


Compiler plugin behind a feature would work. This is what we used to do in servo. You could have a ./mach clippylint that is a mach build with that feature turned on.

Comment 16

[Andrew Halberstadt [:ahal]]

I suspect gecko devs might be more comfortable using a build flag as opposed to a new mach command, but either way would be do-able. There's also already a clang-tidy build variant.. I wonder if we could just run both clang-tidy and clippy as part of the same build.

Comment 17

[Andrew Halberstadt [:ahal]]

We'll either need to have clippy support in stable, or the ability to build with rust nightly in automation before using clippy is feasible (bug 1354994). Looks like the latter might be closer to landing.

This bug got a bit derailed with discussion on what linters to use, but I'm going to leave this open to continue investigating using clippy as a build variant, and file a new bug to tackle adding a rustfmt linter in the meantime.

Comment 18

[Jack Moffitt [:jack]]

I believe Servo already uses clippy, or something very similar. I'm fine with turning that on in the shared codebase assuming we'll not be changing code style (or rather, not changing except to mollify clippy/rustfmt etc).

Comment 19

[Manish Goregaokar [:manishearth]]

I would wait for it to become stable. This may take a couple months. But then it will be more reliable as well, currently clippy stops working once a week because an internal API changed. Not a great idea for automation.

Comment 20

[Ralph Giles (:rillian)]

(In reply to Andrew Halberstadt [:ahal] from comment #17)

> We'll either need to have clippy support in stable, or the ability to build
> with rust nightly in automation before using clippy is feasible (bug
> 1354994). Looks like the latter might be closer to landing.

Rather than expect to be able to use nightly features in Firefox, a simpler approach is to package a binary of cargo-clippy in tooltool (or a docker container) and use that to run the lint task. You can do this today without waiting for dependencies.

Let me know if you want help figuring out the tooltool stuff.

Comment 21

[Andrew Halberstadt [:ahal]]

Thanks, but we'd still need to compile Firefox with rust nightly in that case, wouldn't we?

But I think it's a moot point, sounds like it's better to wait for clippy to support rust stable before proceeding much further here. Removing the dependency.

Comment 22

[Ralph Giles (:rillian)]

(In reply to Andrew Halberstadt [:ahal] from comment #21)
> Thanks, but we'd still need to compile Firefox with rust nightly in that
> case, wouldn't we?

I thought `cargo clippy` worked with only stable rust installed. When I tried a new build today that failed because it dynamically links to parts of rustc. Maybe that could be worked around by doing a --static build.
 
> But I think it's a moot point, sounds like it's better to wait for clippy to
> support rust stable before proceeding much further here. Removing the
> dependency.

That works too.

Comment 23

[Manish Goregaokar [:manishearth]]

> I thought `cargo clippy` worked with only stable rust installed. 

Au contraire, it needs a nightly build. Clippy directly links into the compiler internals, which are unstable because they're implementation details. You must have a nightly, and you must build clippy with the same nightly that you are using.

The path to stabilization is to basically ship it alongside rustc via rustup, using the same special flag that lets libstd work as a stable library even if it contains unstable internals.

Comment 24

[James Graham [:jgraham]]

So clippy now runs on stable; does that remove some of the roadblocks here? We're in the process of making geckodriver and dependencies pass clippy (in the default configuration) and it would be nice to have CI enforce that going forward.

Comment 25

[Manish Goregaokar [:manishearth]]

Should be easier to integrate now. cargo clippy should just work.

Comment 26

[Sylvestre Ledru [:Sylvestre]]

Thanks for the update.
It is probably time indeed!
We will have a look how to implement it (mozlint is probably the right way - bug 1361341 )

Comment 27

[Sylvestre Ledru [:Sylvestre]]

Renaming this bug as it is now the meta bug for clippy issues