Opened 3 years ago

Closed 18 months ago

#22156 closed enhancement (implemented)

Add Rust linting/formatting tools

Reported by: chelseakomlo Owned by: isis
Priority: Medium Milestone: Tor: 0.3.5.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: rust best-practice
Cc: Sebastian, teor, acrichton@… Actual Points:
Parent ID: Points: 1
Reviewer: nickm Sponsor:

Description

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 #22106

Child Tickets

Change History (8)

comment:1 Changed 3 years ago by 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.

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?

comment:2 in reply to:  1 Changed 3 years ago by chelseakomlo

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.

comment:3 Changed 2 years ago by nickm

Keywords: best-practice added

comment:4 Changed 2 years ago by alexcrichton

Cc: acrichton@… added

comment:5 Changed 18 months ago by isis

Owner: set to isis
Points: 1
Status: newaccepted

comment:6 Changed 18 months ago by isis

Milestone: Tor: unspecifiedTor: 0.3.5.x-final
Status: acceptedneeds_review

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

comment:7 Changed 18 months ago by dgoulet

Reviewer: nickm

comment:8 Changed 18 months ago by nickm

Resolution: implemented
Status: needs_reviewclosed

Looks fine; merged!

Note: See TracTickets for help on using tickets.