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

Improve "unknown field" error messages #8823

Merged
merged 1 commit into from
May 18, 2022
Merged

Conversation

smoelius
Copy link
Contributor

@smoelius smoelius commented May 12, 2022

Fixes #8806

Sample output:

error: error reading Clippy's configuration file `/home/smoelius/github/smoelius/rust-clippy/clippy.toml`: unknown field `foobar`, expected one of
           allow-expect-in-tests              enable-raw-pointer-heuristic-for-send    standard-macro-braces
           allow-unwrap-in-tests              enforced-import-renames                  third-party
           allowed-scripts                    enum-variant-name-threshold              too-large-for-stack
           array-size-threshold               enum-variant-size-threshold              too-many-arguments-threshold
           avoid-breaking-exported-api        literal-representation-threshold         too-many-lines-threshold
           await-holding-invalid-types        max-fn-params-bools                      trivial-copy-size-limit
           blacklisted-names                  max-include-file-size                    type-complexity-threshold
           cargo-ignore-publish               max-struct-bools                         unreadable-literal-lint-fractions
           cognitive-complexity-threshold     max-suggested-slice-pattern-length       upper-case-acronyms-aggressive
           cyclomatic-complexity-threshold    max-trait-bounds                         vec-box-size-threshold
           disallowed-methods                 msrv                                     verbose-bit-mask-threshold
           disallowed-types                   pass-by-value-size-limit                 warn-on-all-wildcard-imports
           doc-valid-idents                   single-char-binding-names-threshold      
       at line 1 column 1

You can test this by (say) adding foobar = 42 to Clippy's root clippy.toml file, and running cargo run --bin cargo-clippy.

Note that, to get the terminal width, this PR adds termize as a dependency to cargo-clippy. However, termize is also how rustc_errors gets the terminal width. So, hopefully, this is not a dealbreaker.

r? @xFrednet

changelog: Enhancements: the "unknown field" error messages for config files now wraps the field names.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label May 12, 2022
@xFrednet
Copy link
Member

The output looks very cool ❤️ the dependency should be fine if it's also used by rustc_errors 🙃

This is also worthy of a changelog entry 🙃

Copy link
Member

@xFrednet xFrednet left a comment

Choose a reason for hiding this comment

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

The output already looks good to me 👍

src/main.rs Outdated
@@ -123,8 +123,12 @@ impl ClippyCmd {
.map(|arg| format!("{}__CLIPPY_HACKERY__", arg))
.collect();

// Currently, `CLIPPY_TERMINAL_WIDTH` is used only to format "unknown field" error messages.
let terminal_width = termize::dimensions_stdin().map_or(0, |(w, _)| w);
Copy link
Member

Choose a reason for hiding this comment

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

Is there any advantage in getting the width here and storing it in an environment value? This will not be used 99% of the time. Could we move this to the warning code? Having an environment value is usually intended to enable users to configure or read something, but neither applies here.

If there is a reason for this, I'm fine with it 🙃

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think Cargo is piping rustc's IO, though I haven't been able to figure out precisely where this occurs (in the jobserver crate is my best guess). When I tried to do as you suggested, I got None from termize::dimensions() every time. I even tried at the very start of the driver's main function, but the result was the same.

As I was typing this, I realized that dimensions_stdin should be dimensions in the line you cited, hence the forced push. (dimensions_stdin was a relic from experimenting with this very issue.)

Copy link
Member

Choose a reason for hiding this comment

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

Good to know that might also come in handy for a different project 🙃

Copy link
Member

Choose a reason for hiding this comment

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

As I was typing this, I realized that dimensions_stdin should be dimensions in the line you cited, hence the forced push. (dimensions_stdin was a relic from experimenting with this very issue.)

Thanks for the hint. GitHub has finally improved their force push information so that it's possible to see the specific changes of a force push. 🥳

clippy_lints/src/utils/conf.rs Show resolved Hide resolved
@xFrednet
Copy link
Member

Looks good to me, thank you for the update!

I've changed the changelog slightly, hope that's okay with you 🙃

@bors r+

@bors
Copy link
Collaborator

bors commented May 18, 2022

📌 Commit 5647257 has been approved by xFrednet

@bors
Copy link
Collaborator

bors commented May 18, 2022

⌛ Testing commit 5647257 with merge b6ad6fc...

@bors
Copy link
Collaborator

bors commented May 18, 2022

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

@bors bors merged commit b6ad6fc into rust-lang:master May 18, 2022

let mut msg = String::from(prefix);
for row in 0..rows {
write!(msg, "\n").unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like a typical clippy lint: use writeln instead 😁

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ironically, on lines 422, 426, and 437, I originally had s += .... Then, format_push_string fired, and I changed them to write!.

I don't know why write_with_newline doesn't fire here. But to be honest, I'm kind of glad it doesn't. My aesthetic self kind of prefers the symmetry with lines 426 and 437. 🤷

@smoelius smoelius deleted the unknown-field branch May 18, 2022 22:01
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.

Improve "unknown key" error message?
5 participants