Add Rust linting/formatting tools

We need this as another initial step to support Rust development in tor.

Work will involve adding rustfmt, Clippy, and determining rules we want/don't want.

See conversation in legacy/trac#22106 (moved)

changed milestone to %Tor: 0.3.5.x-final in legacy/trac

added best-practice in Legacy / Trac component::core tor/tor in Legacy / Trac milestone::Tor: 0.3.5.x-final in Legacy / Trac owner::isis in Legacy / Trac points::1 in Legacy / Trac priority::medium in Legacy / Trac resolution::implemented in Legacy / Trac reviewer::nickm in Legacy / Trac rust in Legacy / Trac severity::normal in Legacy / Trac status::closed in Legacy / Trac type::enhancement in Legacy / Trac labels

cargo clippy is a great tool, but I don't think that it should be part of a CI run, and I don't think that it makes sense (or is worthwhile) to spend time deciding which rules should be included or not.

The reason is that cargo clippy is meant to be extremely enthusiastic about giving suggestions. Often, these suggestions are helpful, but sometimes, they aren't. And, for a given rule, there's no way to know whether it will always be useful or not (i.e., there's no way to decide in advance whether it should be "required"). For instance, consider needless_range_loop. This is often a good warning, but sometimes it really does make more sense to use an explicit index. There's no way to know, except by using context and judgement.

I wonder whether a focus on requiring linting tools as part of a development/CI process is a legacy from C development. Since rustc already includes quite extensive errors and warnings, is a required linting process beyond "no warnings on compilation" necessary?

To put it another way, what errors are people hoping to catch using a linter that rustc wouldn't already warn about?

Trac:
Username: hdevalence

Hey, thanks for the further info on clippy.

Replying to hdevalence:

cargo clippy is a great tool, but I don't think that it should be part of a CI run, and I don't think that it makes sense (or is worthwhile) to spend time deciding which rules should be included or not.

The reason is that cargo clippy is meant to be extremely enthusiastic about giving suggestions. Often, these suggestions are helpful, but sometimes, they aren't. And, for a given rule, there's no way to know whether it will always be useful or not (i.e., there's no way to decide in advance whether it should be "required"). For instance, consider needless_range_loop. This is often a good warning, but sometimes it really does make more sense to use an explicit index. There's no way to know, except by using context and judgement.

Yes, we'll definitely need to slowly build up which warnings we follow/ignore, mostly on a as-needed basis, as you said. It isn't practical to decide everything in one go.

I wonder whether a focus on requiring linting tools as part of a development/CI process is a legacy from C development. Since rustc already includes quite extensive errors and warnings, is a required linting process beyond "no warnings on compilation" necessary?

Linting is a pretty common practice in languages other than C. For example, Golang has its own linter: https://github.com/golang/lint and JavaScript has JSHint and other tools.

To put it another way, what errors are people hoping to catch using a linter that rustc wouldn't already warn about?

It is true that we'll lean on rustc for compile errors, but using a linter helps ensure a certain coding style.

Because clippy is so opinionated and also in flux, we hope starting simple and iterating when needed will have the best result. Hopefully we can give feedback to clippy developers to help improve the usefulness of it as well.

Trac:
Keywords: rust deleted, rust best-practice added

Trac:
Username: alexcrichton
Cc: Sebastian, teor to Sebastian, teor, acrichton@mozilla.com

Trac:
Points: N/A to 1
Status: new to accepted
Owner: N/A to isis

Patch to add a Makefile target for running clippy in my bug22156 branch, PR here.

Trac:
Milestone: Tor: unspecified to Tor: 0.3.5.x-final
Status: accepted to needs_review

Trac:
Reviewer: N/A to nickm

Looks fine; merged!

Trac:
Resolution: N/A to implemented
Status: needs_review to closed

closed

changed time estimate to 8h

mentioned in issue legacy/trac#23351 (moved)

mentioned in issue legacy/trac#26958 (moved)

moved from legacy/trac#22156 (moved)

added Feature label and removed 1 deleted label

added Rust label and removed 1 deleted label