Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generate lint categories and explanations with declare_clippy_lint #9541

Merged
merged 2 commits into from
Oct 23, 2022

Conversation

Alexendoo
Copy link
Member

This means contributors will no longer have to run cargo dev update_lints after changing a lints documentation or its category, which may also mean fewer merge conflicts in general

It works by swapping declare_clippy_lint out for a proc_macro of the same name. The proc macro emits a LintInfo alongside the generated Lint which are gathered into declared_lint::LINTS. The categories/explanations are then read from declared_lint::LINTS at runtime

The removal of src/docs is split into a separate commit to be more easily ignored

It is slightly slower though, adding a bit under a second to build time. Less noticeable in full builds or with a slower linker (benchmark uses mold)

hyperfine --warmup 2 \
    --parameter-list commit "declare-proc-macro,master" \
    --command-name "{commit}" \
    --setup "git checkout {commit}" \
    --prepare "touch clippy_lints/src/lib.rs" \
    "cargo build"
Benchmark 1: declare-proc-macro
  Time (mean ± σ):     10.731 s ±  0.154 s    [User: 7.739 s, System: 1.791 s]
  Range (min … max):   10.598 s … 11.125 s    10 runs

Benchmark 2: master
  Time (mean ± σ):      9.422 s ±  0.094 s    [User: 7.183 s, System: 1.732 s]
  Range (min … max):    9.287 s …  9.624 s    10 runs

Summary
  'master' ran
    1.14 ± 0.02 times faster than 'declare-proc-macro'

r? @flip1995
cc @llogiq for --explain

changelog: none

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Sep 26, 2022
@bors
Copy link
Contributor

bors commented Sep 27, 2022

☔ The latest upstream changes (presumably #9543) made this pull request unmergeable. Please resolve the merge conflicts.

@Alexendoo
Copy link
Member Author

Added back the code block # removal functionality that I missed, I'll also leave this as conflicting when it happens next as it'll be a pain to keep on top of

@bors
Copy link
Contributor

bors commented Oct 1, 2022

☔ The latest upstream changes (presumably #9567) made this pull request unmergeable. Please resolve the merge conflicts.

@llogiq
Copy link
Contributor

llogiq commented Oct 1, 2022

I think reducing the code churn may be worth that second of build time. Well done, @Alexendoo!

Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great! Thanks for doing this. I'm also not that concerned about the compile time of Clippy here. Not having so many files that can conflict between PRs is definitely worth it IMO.

clippy_lints/src/lib.rs Outdated Show resolved Hide resolved
declare_clippy_lint/Cargo.toml Outdated Show resolved Hide resolved
declare_clippy_lint/src/lib.rs Show resolved Hide resolved
@Alexendoo
Copy link
Member Author

Thanks! If any more conflicts crop up I'll keep on top of them

@flip1995
Copy link
Member

@bors r+ p=100

Thanks! (Going to bed. If this should fail for some reason, feel free to r=me again)

@bors
Copy link
Contributor

bors commented Oct 23, 2022

📌 Commit 8134566 has been approved by flip1995

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Oct 23, 2022

⌛ Testing commit 8134566 with merge 5b09d4e...

@bors
Copy link
Contributor

bors commented Oct 23, 2022

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: flip1995
Pushing 5b09d4e to master...

@bors bors merged commit 5b09d4e into rust-lang:master Oct 23, 2022
@Alexendoo Alexendoo deleted the declare-proc-macro branch October 24, 2022 17:50
koka831 added a commit to koka831/rust-clippy that referenced this pull request Oct 25, 2022
removes document text files that are no longer needed by rust-lang#9541.
bors added a commit that referenced this pull request Oct 25, 2022
chore: remove unnecessary files

removes document text files that are no longer needed by #9541.

changelog: none

r? `@Alexendoo`
bors added a commit that referenced this pull request Oct 25, 2022
Remove `lib.register_*` and `src/docs*` in `cargo dev update_lints`

Follow up to #9709 / #9541

There's a good number of PRs with some leftover `src/docs` files for example, and as a reviewer it's something we're used to ignoring so it can easily slip through

r? `@flip1995`

changelog: none
kraktus pushed a commit to kraktus/rust-clippy that referenced this pull request Oct 26, 2022
removes document text files that are no longer needed by rust-lang#9541.
winterqt added a commit to winterqt/nixpkgs that referenced this pull request Feb 19, 2023
As of Rust 1.67.0, the cargo-clippy binary now relies on the rustc_private
libraries [0], so let's do the RPATH fixup to it too.

I've also added a comment to explain the RPATH situation, as it took me
a bit to figure out.

[0]: rust-lang/rust-clippy#9541
winterqt added a commit to NixOS/nixpkgs that referenced this pull request Feb 20, 2023
As of Rust 1.67.0, the cargo-clippy binary now relies on the rustc_private
libraries [0], so let's do the RPATH fixup to it too.

I've also added a comment to explain the RPATH situation, as it took me
a bit to figure out.

[0]: rust-lang/rust-clippy#9541
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants