Page MenuHomePhabricator

[Chronik] Add `rustfmt` linter, to style check Rust files
ClosedPublic

Authored by tobias_ruck on Jun 27 2022, 14:04.

Details

Reviewers
Fabien
Group Reviewers
Restricted Project
Commits
rABCc451a96ffc0b: [Chronik] Add `rustfmt` linter, to style check Rust files
Summary

Check formatting via rustfmt. Configuration is kept rusty, instead of mimicking clang-format.

During development, cargo +nightly fmt should be used to wrap comments, strings, group imports, etc. As these features are unstable and might have false-positives, we don't enforce on them in the CI yet.

Stable rustfmt currently doesn't enforce line length limits strictly, therefore we have to verify length outselves. We skip length limits that are fixed by rustfmt, however, the current approach leaves some false positives:

  • If two lines within one rustfmt diff exceed the limit,
  • one of them is fixed by rustfmt,
  • then for *both* will be issued a warning.

Since this both doesn't occur too often, and also should be fixed by the "recommended" (aka cargo +nightly fmt) approach, the trade-off of having a simple implementation seems worth it.

Also, if cargo fmt wraps more and more code successfully, these false positives should disappear.

Test Plan
  1. arc lint displays four errors, one being a false positive in line 12.
  2. arc lint will have only fit the spaces after struct.
  3. cargo +nightly fmt fixes all errors except the line having way too many x's.

Diff Detail

Repository
rABC Bitcoin ABC
Branch
chronik-rustfmt
Lint
Lint Warnings
SeverityLocationCodeMessage
Warningchronik/bitcoinsuite-core/src/lib.rs:7RUSTFMTLine limit exceeded
Warningchronik/bitcoinsuite-core/src/lib.rs:12RUSTFMTLine limit exceeded
Warningchronik/bitcoinsuite-core/src/lib.rs:13RUSTFMTLine limit exceeded
Auto-Fixchronik/bitcoinsuite-core/src/lib.rs:10RUSTFMTRustfmt violation
Unit
No Test Coverage
Build Status
Buildable 19507
Build 38744: Build Diff
Build 38743: arc lint + arc unit

Event Timeline

De-mote exceeding line limit to warnings

Remove test code that intentionally triggers the linter

tobias_ruck edited the test plan for this revision. (Show Details)
Fabien requested changes to this revision.Jun 27 2022, 21:57
Fabien added inline comments.
arcanist/linter/RustfmtLinter.php
94 ↗(On Diff #34153)

This check should come before the $orig == $stdout early exit. In the event you only have long lines it will be unnoticed otherwise.

This revision now requires changes to proceed.Jun 27 2022, 21:57

Use cargo fmt -- --check to lint files, which outputs a diff, and process that.

tobias_ruck edited the test plan for this revision. (Show Details)
tobias_ruck edited the summary of this revision. (Show Details)
tobias_ruck edited the test plan for this revision. (Show Details)
tobias_ruck edited the summary of this revision. (Show Details)
tobias_ruck edited the test plan for this revision. (Show Details)

Use nightly rustfmt, which simplifies the linter substantially

Fabien requested changes to this revision.Jun 29 2022, 16:19
Fabien added inline comments.
.rustfmt.toml
6 ↗(On Diff #34182)

Outdated comment

arcanist/linter/RustfmtLinter.php
22 ↗(On Diff #34182)

It's actually a requirement

103 ↗(On Diff #34182)

return $messages, otherwise you're losing your line limit error

This revision now requires changes to proceed.Jun 29 2022, 16:19

Fix comments in .rustfmt.toml, don't group rules but sort by alphabet. Use rustfmt +abc-nightly in the linter.

Return $messages instead of array() when rustfmt made no changes to the code

Fabien added inline comments.
arcanist/linter/RustfmtLinter.php
24 ↗(On Diff #34188)
25 ↗(On Diff #34188)
This revision is now accepted and ready to land.Jul 1 2022, 11:58

Use ${} and "" in install instructions