improve `lintcheck` #9731

kraktus · 2022-10-27T15:04:46Z

Two axes were improved (one per commit):

Logging
Fixing --fix flag

About logging, I'm open to other form and crates to use, my main point was to actually see actual clippy's suggestions, rather than for each crate look for the line

changelog: none

Use `simplelog` to handle logs, as `env_logger` does not handle writing to file for the moment, see rust-cli/env_logger#208 Do not push most verbose logs on stdout, only push them in the log file

rust-highfive · 2022-10-27T15:04:50Z

r? @flip1995

(rust-highfive has picked a reviewer for you, use r? to override)

kraktus · 2022-10-27T15:06:37Z

r? @matthiaskrgr

There were several issues: - `--fix` was ignored as part of rust-lang#9461 - `--filter` in conjunction to `--fix` was broken due to rust-lang#9703 After `lintcheck --fix` is used, crates will be re-extracted since their content has been modified

flip1995 · 2022-10-28T07:45:45Z

I'm really unsure if we need this level of logging in the lintcheck tool. This introduces 2 new dependencies, that I don't think help us much here. I really don't need coloring in my terminal when using the tool.

kraktus · 2022-10-28T12:32:35Z

I'm really unsure if we need this level of logging in the lintcheck tool. This introduces 2 new dependencies, that I don't think help us much here. I really don't need coloring in my terminal when using the tool.

This additional logging already allowed me to fix two issues of use_self that went unnoticed: #9726 and #9704. I think it's a powerful tool to be more proactive about lint FPs, especially machine applicable ones.

About the colouring/formatting, I'd be fine with making it similar to plain (e)println!

For the two dependencies, I'm not sure why it is an issue since we have no restrain in how we run lintcheck. log crate is definitely trustworthy, simplelog also get a decent amount of download but would be ready to change it to whatever minimalistic log handler we'd like. We could even simulate its behaviour with just plain write!, but I feel that would really be reinventing the wheel.

Alexendoo · 2022-10-28T13:14:50Z

I was thinking about adding another output mode alongside normal/markdown that output the rendered diagnostics to the log, would that cover it?

Alexendoo · 2022-10-28T13:16:22Z

lintcheck/src/main.rs

-            vec!["--fix", "--"]
+            // need to pass `clippy` arg even if already feeding `cargo-clippy`
+            // see https://github.com/rust-lang/rust-clippy/pull/9461
+            vec!["clippy", "--fix", "--allow-no-vcs", "--"]
        } else {
            vec!["--", "--message-format=json", "--"]


While you're in the neighbourhood, could change this bit to

Suggested change

vec!["--", "--message-format=json", "--"]

vec!["clippy", "--message-format=json", "--"]

I did find this quite confusing before the conversation in #9461

flip1995 · 2022-10-28T14:11:18Z

There is a list of allowed crates in the Rust compiler: https://github.com/rust-lang/rust/blob/master/src/tools/tidy/src/deps.rs Clippy should also adhere to this list of crates. For more information about this, see https://rustc-dev-guide.rust-lang.org/crates-io.html

I'm ok with the log crate as a dependency. But simplelog is not part of the above list.

I'll leave the decision whether log is actually necessary to Alex/Matthias, since they are the main owners / maintainers of this tool.

kraktus · 2022-10-29T15:53:10Z

There is a list of allowed crates in the Rust compiler: https://github.com/rust-lang/rust/blob/master/src/tools/tidy/src/deps.rs Clippy should also adhere to this list of crates. For more information about this, see https://rustc-dev-guide.rust-lang.org/crates-io.html

I'm ok with the log crate as a dependency. But simplelog is not part of the above list.

I'll leave the decision whether log is actually necessary to Alex/Matthias, since they are the main owners / maintainers of this tool.

This is fair, I can implement a simple log consumer by hand which would avoid that dependency. It looks like lintcheck is already using crates not from the whitelist though (walkdir ureq, toml, tar, clap), only counting direct deps.

I was thinking about adding another output mode alongside normal/markdown that output the rendered diagnostics to the log, would that cover it?

Functionality-wise it would, but I feel this would be less-maintainable than leveraging on the rust ecosystem.

Alexendoo · 2022-11-04T13:38:29Z

It would be an additional branch in ClippyWarning::to_output, after #9764 that could be as simple as OutputFormat::Rendered => self.diag.to_string()

Personally I don't see a compelling reason to add the crates

kraktus · 2022-11-13T15:32:06Z

First just want to thank you Alexendoo for your PR because I believe lintcheck is our cornerstone tool to proactively find new false positives.

Now, I would argue that the difference of complexity between cea4044 (from #9764 which also add 2 new deps and rely on instable features) and 5461e99 favouring the latter is compelling enough to rely on log and env_logger.

With latest version of env_logger fixing their issue with piping logs to a file, I can revert to use it and all new deps would be in the rustc whitelist. Will wait for a final call from matthiaskrgr before doing so though.

Alexendoo · 2022-11-13T16:01:01Z

Comparing those commits seems odd, they do completely different things. Adding log/env_logger would not make diff/strip-ansi-escapes any less necessary

The sigpipe feature is so that lintcheck ... | head/less/etc doesn't panic

bors · 2023-03-04T23:50:33Z

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

xFrednet · 2024-03-21T15:39:16Z

Hey @matthiaskrgr, this is a ping from triage. A previous commit, mentioned that they would like input from you. Would you mind taking a look at it: #9731 (comment)

@kraktus Since this PR was created quite some time ago do you believe this would still be a good addition?

matthiaskrgr · 2024-03-22T21:52:29Z

Oh damn, I have absolute no memory of ever seeing this 😓 but I've also turned off all github mail notifications since getting 30-40 mails a day was getting unbearable.

To be honest I'm not sure if I am still fit as "area expert" for lintcheck, I did write the initial impl with a clear goal of what I wanted it to be, but it has also been rewritten several times (at least thats what it feels like) so that I no longer feel familiar with the code, I have also not run it in probably 3-4 years by myself (if others do, that is great though :D)

I looked over the PR and did not see anything that made me think "omg wtf", so lgtm I guess??

kraktus · 2024-03-23T08:50:51Z

I think fixing —fix would be a good addition yes, eventually running it as CI whenever a new MachineApplicable lint is created to know about cornercases should be a goal.

matthiaskrgr · 2024-03-23T09:55:59Z

Ah yes!
I've integrated that into icemaker around one and a half years ago (take a .rs file, make sure it compiles, run cargo clippy --fix, and note if suggestion applies or not ) which yielded around 70 tickets already guess :D

With whole crates there is always a higher change though that lints do collide in some weird way, sometimes two lints give conflicting suggestions for the same line, or that you'll have to remove major parts of the program to get a somewhat usable mvce.

xFrednet · 2024-03-24T08:30:39Z

To be honest I'm not sure if I am still fit as "area expert" for lintcheck, I did write the initial impl with a clear goal of what I wanted it to be, but it has also been rewritten several times (at least thats what it feels like) so that I no longer feel familiar with the code, I have also not run it in probably 3-4 years by myself (if others do, that is great though :D)

Okay, that's understandable!

r? @Alexendoo Would you mind taking over this review? You already left a few comments earlier and know lintcheck a bit AFAIK. If you don't have the time, you can reassign it to me :)

I think fixing --fix would be a good addition yes, eventually running it as CI whenever a new MachineApplicable lint is created to know about cornercases should be a goal.

Sounds reasonable, thank you for the answer :D

Alexendoo · 2024-03-26T17:53:49Z

Yeah I can review it, may need redoing since it's changed a fair bit since the PR was opened, I don't think anybody has touched --fix so I assume it's still in need of repair

xFrednet · 2024-06-20T20:08:27Z

Hey @Alexendoo this is a ping from triage. Could you take a look at this issue and indicate what needs to be done next?

kraktus · 2024-06-21T08:51:55Z

I could rebase it and work on next week. I'll need to check back the code as I've lost track of the changes since. I'd like to get a green light on it before though. (both fixing --fix, and additionally improve logging, probably using env_logger which has fixed the issues that prevented it from using in the first place.)

flip1995 · 2024-06-21T09:27:48Z

As we already use some deps that are not white listed in the Rust repo and lintcheck is not really part of the Rust repo workspace, simplelog should be fine. (It's MIT/Apache2.0 licensed). So this should resolve my blocker of this PR :)

Alexendoo · 2024-06-21T17:49:34Z

Personally I'd rather have -v print the diagnostics in full in the report than have it go via logging

xFrednet · 2024-07-23T06:38:46Z

Hey @kraktus, could you rebase this PR on master? Recently, we made several changes to lintcheck.

For the changes. I'd like to have --fix working. We now run lintcheck in our CI but it doesn't check the suggestions yet :D

For the logging part. Our CI now displays the full error message. You can take a look at https://github.com/xFrednet/rust-clippy/actions/runs/10044508861 from #13139 as an example. Having a local option would also be cool, but I'd also be in favor of a flag instead of logging.

@Alexendoo I've also assigned myself, as I want to keep track of it. We can both do reviews on this one :D

kraktus · 2024-07-24T17:51:26Z

Hey, I don't have plan to work on this in the foreseeable future, feel free to pick it up

xFrednet · 2024-07-24T18:22:53Z

Alright, then I'll close this PR for now. Thank you :D

Better logging for lintcheck

5461e99

Use `simplelog` to handle logs, as `env_logger` does not handle writing to file for the moment, see rust-cli/env_logger#208 Do not push most verbose logs on stdout, only push them in the log file

rust-highfive assigned flip1995 Oct 27, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Oct 27, 2022

rust-highfive assigned matthiaskrgr and unassigned flip1995 Oct 27, 2022

kraktus force-pushed the lcheck3 branch from 1679a4f to 3a2c1e5 Compare October 27, 2022 15:25

Fix lintcheck --fix

3a2c1e5

There were several issues: - `--fix` was ignored as part of rust-lang#9461 - `--filter` in conjunction to `--fix` was broken due to rust-lang#9703 After `lintcheck --fix` is used, crates will be re-extracted since their content has been modified

Alexendoo reviewed Oct 28, 2022

View reviewed changes

rustbot assigned Alexendoo and unassigned matthiaskrgr Mar 24, 2024

xFrednet self-assigned this Jul 23, 2024

xFrednet closed this Jul 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improve `lintcheck` #9731

improve `lintcheck` #9731

kraktus commented Oct 27, 2022 •

edited by xFrednet

Loading

rust-highfive commented Oct 27, 2022

kraktus commented Oct 27, 2022

flip1995 commented Oct 28, 2022

kraktus commented Oct 28, 2022 •

edited

Loading

Alexendoo commented Oct 28, 2022

Alexendoo Oct 28, 2022

flip1995 commented Oct 28, 2022

kraktus commented Oct 29, 2022

Alexendoo commented Nov 4, 2022

kraktus commented Nov 13, 2022

Alexendoo commented Nov 13, 2022

bors commented Mar 4, 2023

xFrednet commented Mar 21, 2024

matthiaskrgr commented Mar 22, 2024

kraktus commented Mar 23, 2024

matthiaskrgr commented Mar 23, 2024

xFrednet commented Mar 24, 2024

Alexendoo commented Mar 26, 2024

xFrednet commented Jun 20, 2024

kraktus commented Jun 21, 2024 •

edited

Loading

flip1995 commented Jun 21, 2024

Alexendoo commented Jun 21, 2024

xFrednet commented Jul 23, 2024

kraktus commented Jul 24, 2024

xFrednet commented Jul 24, 2024

	vec!["--", "--message-format=json", "--"]
	vec!["clippy", "--message-format=json", "--"]

improve lintcheck #9731

improve lintcheck #9731

Conversation

kraktus commented Oct 27, 2022 • edited by xFrednet Loading

rust-highfive commented Oct 27, 2022

kraktus commented Oct 27, 2022

flip1995 commented Oct 28, 2022

kraktus commented Oct 28, 2022 • edited Loading

Alexendoo commented Oct 28, 2022

Alexendoo Oct 28, 2022

Choose a reason for hiding this comment

flip1995 commented Oct 28, 2022

kraktus commented Oct 29, 2022

Alexendoo commented Nov 4, 2022

kraktus commented Nov 13, 2022

Alexendoo commented Nov 13, 2022

bors commented Mar 4, 2023

xFrednet commented Mar 21, 2024

matthiaskrgr commented Mar 22, 2024

kraktus commented Mar 23, 2024

matthiaskrgr commented Mar 23, 2024

xFrednet commented Mar 24, 2024

Alexendoo commented Mar 26, 2024

xFrednet commented Jun 20, 2024

kraktus commented Jun 21, 2024 • edited Loading

flip1995 commented Jun 21, 2024

Alexendoo commented Jun 21, 2024

xFrednet commented Jul 23, 2024

kraktus commented Jul 24, 2024

xFrednet commented Jul 24, 2024

improve `lintcheck` #9731

improve `lintcheck` #9731

kraktus commented Oct 27, 2022 •

edited by xFrednet

Loading

kraktus commented Oct 28, 2022 •

edited

Loading

kraktus commented Jun 21, 2024 •

edited

Loading