Skip to content

Add configuration options to --explain #10751

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

Merged
merged 2 commits into from
May 9, 2023

Conversation

blyxyas
Copy link
Member

@blyxyas blyxyas commented May 5, 2023

This PR rearranges some modules, taking metadata_collector out of internal_lints and making public just the necessary functions for explain() to use.

The output looks something like this:

$ cargo run --bin cargo-clippy --manifest-path ../rust-clippy/Cargo.toml -- --explain cognitive_complexity
### What it does
Checks for methods with high cognitive complexity.

### Why is this bad?
Methods of high cognitive complexity tend to be hard to
both read and maintain. Also LLVM will tend to optimize small methods better.

### Known problems
Sometimes it's hard to find a way to reduce the
complexity.

### Example
You'll see it when you get the warning.

========================================
Configuration for clippy::cognitive_complexity:
- cognitive-complexity-threshold: The maximum cognitive complexity a function can have (default: 25)

Fixes #9990
r? @xFrednet


changelog: Docs: cargo clippy --explain LINT now shows possible configuration options for the explained lint
#10751

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label May 5, 2023
@xFrednet
Copy link
Member

xFrednet commented May 5, 2023

I've only skimmed over the code, but we might be able, to only move ClippyConfiguration outside of the internal feature. That would make those dependencies still optional. Would that be an option @blyxyas

Btw, thank you for giving this a shot, the idea already looks good ❤️

@blyxyas blyxyas force-pushed the explain_with_config branch from dddb664 to 2a4571d Compare May 6, 2023 06:38
@blyxyas
Copy link
Member Author

blyxyas commented May 6, 2023

Ok, the new commit minimizes changes and build times (53.15s from 58s). Keeping the same dependencies.
It also changes the configuration section to a more Markdown-like style:

### Configuration for clippy::cognitive_complexity:
    - cognitive-complexity-threshold: The maximum cognitive complexity a function can have (default: 25)

@flip1995
Copy link
Member

flip1995 commented May 9, 2023

I also really like the idea! Thanks for keeping those deps optional and measuring the compile time and sizes. I'll now leave this PR to you and the friendly penguin to review :)

///
/// Would yield:
/// ```rust, ignore
/// Some(["lint_name_1", "lint_name_2"], "Papa penguin, papa penguin")
Copy link
Member

Choose a reason for hiding this comment

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

I'm happy that you kept the doc comments ❤️

println!("### Configuration for {}:", info.lint.name_lower());
for position in config_vec_positions {
let conf = &mdconf[position];
println!(" - {}: {} (default: {})", conf.name, conf.doc, conf.default);
Copy link
Member

Choose a reason for hiding this comment

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

I would suggest moving this into format into a method on the configuration struct, to have it more uniform with the other formatting code. However, that is just a style question, and I can see reasons for having it here, so this is an optional suggestion :)


I feel like the formatting is a bit hard to read, for lints with multiple configs, like in this example:

> cargo run --bin cargo-clippy --manifest-path ../rust-clippy/Cargo.toml -- --explain manual_let_else


### Configuration for clippy::manual_let_else:
    - msrv: The minimum rust version that the project supports (default: None)
    - matches-for-let-else: Whether the matches should be considered by the lint, and whether there should
    be filtering for common types. (default: WellKnownTypes)

But I also don't know how to improve it much. It should be fine, since we have very few lints with multiple config values :)

Copy link
Member Author

@blyxyas blyxyas May 9, 2023

Choose a reason for hiding this comment

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

About the first issue: I'll fix it.
About the second: I have 0 ideas about how to fix this. Maybe adding a newline between ### Configuration for ... and the options?

Copy link
Member Author

Choose a reason for hiding this comment

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

After taking a look at the code again, I don't think I can even fix the first problem.
Each configuration option depends on mdconf and config_vec_positions. While mdconf is independent (taken from get_configuration_metadata()), config_vec_positions depends on info.

I think extrapolating the whole fragment (from line 480 to 491) would abstract so much the process of getting the configuration, that it would difficult future contributors and would make the function more readable, but less understandable overall.

I'll still fix the formatting.

Copy link
Member

Choose a reason for hiding this comment

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

Alright, then just keep it as is. Fixing the indention was a nice catch! Thank you, I'll leave you then honer of ordering @bors to merge it :D

Copy link
Member Author

@blyxyas blyxyas May 9, 2023

Choose a reason for hiding this comment

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

omg thx ❤️

@xFrednet
Copy link
Member

xFrednet commented May 9, 2023

Looks good to me, I have one small optional NIT, and then it's ready to be merged. You can r=me with or without the suggestion. (r=me meaning commenting bors r=xFrednet with an @ in front)

@bors delegate+

@xFrednet
Copy link
Member

xFrednet commented May 9, 2023

@bors delegate+

@bors
Copy link
Contributor

bors commented May 9, 2023

✌️ @blyxyas can now approve this pull request

@blyxyas
Copy link
Member Author

blyxyas commented May 9, 2023

@bors r=xFrednet

@bors
Copy link
Contributor

bors commented May 9, 2023

📌 Commit 3e8fea6 has been approved by xFrednet

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented May 9, 2023

⌛ Testing commit 3e8fea6 with merge 1407c76...

@bors
Copy link
Contributor

bors commented May 9, 2023

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: xFrednet
Pushing 1407c76 to master...

@bors bors merged commit 1407c76 into rust-lang:master May 9, 2023
@blyxyas blyxyas deleted the explain_with_config branch October 5, 2023 09:03
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.

We should extend --explain to include config options
5 participants