Bob Clary [:bc] (inactive) Reporter
	Description • 7 years ago

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.

	Manish Goregaokar [:manishearth]
	Comment 1 • 7 years ago

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.

	Bob Clary [:bc] (inactive) Reporter
	Comment 2 • 7 years ago

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

	Manish Goregaokar [:manishearth]
	Comment 3 • 7 years ago

(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)

	Bobby Holley (:bholley)
	Comment 4 • 7 years ago

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

FWIW, Servo uses tidy.

	Manish Goregaokar [:manishearth]
	Comment 5 • 7 years ago

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)

	Andrew Halberstadt [:ahal]
	Comment 6 • 7 years ago

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.

	Bobby Holley (:bholley)
	Comment 7 • 7 years ago

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.

	Andrew Halberstadt [:ahal]
	Comment 8 • 7 years ago

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.

	Manish Goregaokar [:manishearth]
	Comment 9 • 7 years ago

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

	Emilio Cobos Álvarez (:emilio)
	Comment 10 • 7 years ago

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.

	Manish Goregaokar [:manishearth]
	Comment 11 • 7 years ago

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.

	Emilio Cobos Álvarez (:emilio)
	Comment 12 • 7 years ago

(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.

	Manish Goregaokar [:manishearth]
	Comment 13 • 7 years ago

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?

	Andrew Halberstadt [:ahal]
	Comment 14 • 7 years ago

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.

	Manish Goregaokar [:manishearth]
	Comment 15 • 7 years ago

> 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.

	Andrew Halberstadt [:ahal]
	Comment 16 • 7 years ago

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.

	Andrew Halberstadt [:ahal]
	Comment 17 • 7 years ago

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.

	Jack Moffitt [:jack]
	Comment 18 • 7 years ago

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).

	Manish Goregaokar [:manishearth]
	Comment 19 • 7 years ago

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.

	Ralph Giles (:rillian)
	Comment 20 • 7 years ago

(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.

	Andrew Halberstadt [:ahal]
	Comment 21 • 7 years ago

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.

	Ralph Giles (:rillian)
	Comment 22 • 7 years ago

(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.

	Manish Goregaokar [:manishearth]
	Comment 23 • 7 years ago

> 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.