Make restriction lint's use `span_lint_and_then` (a -> e) #13136

xFrednet · 2024-07-21T10:20:30Z

This migrates a few restriction lints to use span_lint_and_then. This change is motivated by #7797.

I'm also interested if it will have an impact on performance. With some of these lints, like clippy::implicit_return I expect an impact, as it was previously creating a suggestion for every implicit return which is just wild.

I've also cleaned up some lint message. Mostly minor stuff. For example: suggestions with a longer message than "try" now use SuggestionStyle::ShowAlways

@blyxyas Could you benchmark this PR? I want to get all the numbers :3

This also crashed our new lintcheck CI with the following message:

Error: $GITHUB_STEP_SUMMARY upload aborted, supports content up to a size of 1024k, got 46731k. For more information see: https://docs.github.com/actions/using-workflows/workflow-commands-for-github-actions#adding-a-markdown-summary

Which is just wild. Like, I've tested the first 20 lints and got like four changes and then this. 50 MB of changed lint messages o.O. Looks like I'll create a separate PR to fix that step ^^

cc: #7797

changelog: none

r? @blyxyas

blyxyas · 2024-07-21T10:35:35Z

I should probably continue working on the bot for automatically setting these up. The perf team said that implementing Clippy would be more hassle than it's worth, so I'm working on a mini-project that would catch a webhook and just run two commands in my RPi or maybe the server that Cloudboxes gave us. It should not be hard, but I can't get around to do it.

Anyways, I just set up the benchmarks!

xFrednet · 2024-07-21T11:39:11Z

Here is a hacked together¹ fixed run for the lintcheck CI: https://github.com/xFrednet/rust-clippy/actions/runs/10027917725

0 added, 0 removed, 61808 changed

As expected, most of them seem to be implicit_return but if_then_some_else_none is apparently also quite popular. :D

I'll create a PR for this and a cleaner fix, but I first want https://github.com/rust-lang/rust-clippy/pull/13133 to be merged ↩

xFrednet · 2024-07-21T13:38:51Z

Do we have any results yet? :3

blyxyas · 2024-07-21T14:24:12Z

Just got the results in:

Instructions

Summary

Range Mean Count

Regressions 0.32%, 0.50% 0.40% 3

Improvements -1.45%, -1.08% -1.27% 2

All -1.45%, 0.50% -0.26% 5

Primary benchmarks

Benchmark Profile Scenario % Change Significance Factor

Secondary benchmarks

Benchmark Profile Scenario % Change Significance Factor

coercions clippy incr-patched: println -1.45% 7.24x

coercions clippy full -1.08% 5.42x

coercions clippy incr-patched: add static arr item 0.50% 2.48x

deep-vector clippy incr-full 0.39% 1.97x

deep-vector clippy full 0.32% 1.59x

Cycles, less accurate

Summary

Range Mean Count

Regressions 0.41%, 16.33% 4.11% 81

Improvements -15.83%, -0.43% -4.35% 72

All -15.83%, 16.33% 0.13% 153

Primary benchmarks

Benchmark Profile Scenario % Change Significance Factor

libc-0.2.124 clippy incr-full 11.21% 56.04x

unicode-normalization-0.1.19 clippy incr-patched: println -10.84% 54.18x

regex-1.5.5 clippy incr-unchanged 9.85% 49.26x

unicode-normalization-0.1.19 clippy incr-unchanged -9.49% 47.46x

helloworld clippy incr-full 9.21% 46.04x

bitmaps-3.1.0 clippy incr-unchanged -7.53% 37.67x

exa-0.10.1 clippy incr-patched: printlns 7.40% 37.00x

bitmaps-3.1.0 clippy incr-patched: println -7.32% 36.59x

clap-3.1.6 clippy incr-unchanged -6.94% 34.71x

typenum-1.17.0 clippy incr-unchanged -6.72% 33.62x

regex-1.5.5 clippy incr-patched: println -6.71% 33.57x

clap-3.1.6 clippy incr-patched: println -6.67% 33.35x

regex-1.5.5 clippy incr-patched: byte frequencies 6.10% 30.49x

webrender-2022 clippy incr-patched: println 5.94% 29.70x

unicode-normalization-0.1.19 clippy full -5.60% 27.98x

cranelift-codegen-0.82.1 clippy incr-patched: println -5.57% 27.83x

stm32f4-0.14.0 clippy incr-unchanged 5.54% 27.71x

regex-1.5.5 clippy incr-patched: reverse 5.38% 26.89x

helloworld clippy incr-patched: println -5.36% 26.79x

ripgrep-13.0.0 clippy incr-unchanged -5.25% 26.24x

regex-1.5.5 clippy incr-patched: Compiler new -5.24% 26.18x

syn-1.0.89 clippy incr-patched: println -5.23% 26.13x

image-0.24.1 clippy incr-patched: println -5.06% 25.31x

typenum-1.17.0 clippy full -5.05% 25.25x

libc-0.2.124 clippy incr-unchanged -4.83% 24.13x

serde-1.0.136 clippy incr-unchanged 4.48% 22.41x

libc-0.2.124 clippy full 4.42% 22.09x

helloworld clippy incr-unchanged -4.41% 22.05x

diesel-1.4.8 clippy incr-unchanged 4.40% 22.01x

exa-0.10.1 clippy incr-unchanged 4.27% 21.33x

regex-1.5.5 clippy incr-patched: is valid cap letter 4.12% 20.62x

ripgrep-13.0.0 clippy full -4.06% 20.29x

libc-0.2.124 clippy incr-patched: clone -4.05% 20.25x

regex-1.5.5 clippy full 3.91% 19.56x

serde_derive-1.0.136 clippy incr-full -3.83% 19.13x

cranelift-codegen-0.82.1 clippy full -3.81% 19.06x

cargo-0.60.0 clippy incr-patched: println -3.67% 18.37x

regex-1.5.5 clippy incr-patched: Job 3.61% 18.03x

diesel-1.4.8 clippy incr-full 3.52% 17.58x

ripgrep-13.0.0 clippy incr-patched: println -3.48% 17.38x

typenum-1.17.0 clippy incr-patched: add fn -3.21% 16.03x

webrender-2022 clippy incr-full -2.95% 14.77x

bitmaps-3.1.0 clippy full 2.80% 13.99x

html5ever-0.26.0 clippy incr-unchanged -2.57% 12.85x

serde_derive-1.0.136 clippy full -2.55% 12.76x

image-0.24.1 clippy full 2.46% 12.30x

serde-1.0.136 clippy incr-patched: println 2.45% 12.27x

serde_derive-1.0.136 clippy incr-unchanged 2.30% 11.50x

exa-0.10.1 clippy full -2.27% 11.35x

image-0.24.1 clippy incr-unchanged 2.26% 11.30x

stm32f4-0.14.0 clippy incr-patched: negate -2.16% 10.79x

regex-1.5.5 clippy incr-patched: compile one -2.14% 10.70x

cranelift-codegen-0.82.1 clippy incr-full 2.14% 10.69x

webrender-2022 clippy full 2.11% 10.56x

clap-3.1.6 clippy incr-full 1.91% 9.57x

syn-1.0.89 clippy incr-unchanged -1.67% 8.35x

typenum-1.17.0 clippy incr-full -1.60% 8.00x

hyper-0.14.18 clippy incr-unchanged 1.51% 7.53x

syn-1.0.89 clippy full 1.48% 7.38x

cranelift-codegen-0.82.1 clippy incr-unchanged -1.40% 7.01x

cargo-0.60.0 clippy incr-full 1.37% 6.86x

serde_derive-1.0.136 clippy incr-patched: println 1.36% 6.78x

unicode-normalization-0.1.19 clippy incr-full -1.29% 6.45x

exa-0.10.1 clippy incr-full -1.19% 5.95x

clap-3.1.6 clippy full -1.12% 5.61x

bitmaps-3.1.0 clippy incr-full -1.12% 5.59x

diesel-1.4.8 clippy incr-patched: println 1.01% 5.05x

serde-1.0.136 clippy incr-full 1.01% 5.03x

diesel-1.4.8 clippy full -0.97% 4.83x

stm32f4-0.14.0 clippy incr-full -0.89% 4.47x

html5ever-0.26.0 clippy incr-patched: println -0.81% 4.05x

html5ever-0.26.0 clippy incr-full 0.80% 4.01x

hyper-0.14.18 clippy full 0.77% 3.85x

image-0.24.1 clippy incr-full -0.73% 3.67x

ripgrep-13.0.0 clippy incr-full 0.69% 3.46x

webrender-2022 clippy incr-unchanged -0.64% 3.21x

regex-1.5.5 clippy incr-full 0.49% 2.45x

Secondary benchmarks

Benchmark Profile Scenario % Change Significance Factor

issue-58319 clippy incr-full 16.33% 81.67x

externs clippy incr-unchanged -15.83% 79.14x

token-stream-stress clippy incr-full 15.51% 77.57x

match-stress clippy incr-unchanged -14.87% 74.33x

externs clippy full -14.39% 71.97x

coercions clippy incr-patched: add static arr item -12.80% 64.02x

issue-88862 clippy full 12.71% 63.55x

issue-46449 clippy full 11.28% 56.39x

unify-linearly clippy full 11.21% 56.07x

regression-31157 clippy incr-unchanged -10.14% 50.72x

unused-warnings clippy incr-full -9.30% 46.50x

coercions clippy full -9.15% 45.76x

many-assoc-items clippy incr-unchanged -8.36% 41.80x

deep-vector clippy incr-patched: println 7.81% 39.05x

derive clippy incr-unchanged 7.73% 38.63x

issue-46449 clippy incr-patched: u8 3072 7.36% 36.80x

issue-46449 clippy incr-patched: io error 6144 6.92% 34.61x

externs clippy incr-full 6.88% 34.41x

issue-88862 clippy incr-full 6.85% 34.23x

unify-linearly clippy incr-full 6.77% 33.85x

projection-caching clippy incr-full 6.64% 33.18x

deep-vector clippy incr-patched: add vec item 5.71% 28.56x

wf-projection-stress-65510 clippy incr-unchanged 5.34% 26.68x

ucd clippy incr-unchanged -5.16% 25.82x

deeply-nested-multi clippy incr-full -5.08% 25.41x

await-call-tree clippy incr-full 4.94% 24.72x

coercions clippy incr-patched: println -4.90% 24.49x

wf-projection-stress-65510 clippy full 4.37% 21.85x

coercions clippy incr-unchanged 4.31% 21.56x

projection-caching clippy incr-unchanged 4.20% 21.00x

deep-vector clippy incr-unchanged 4.19% 20.95x

unify-linearly clippy incr-unchanged -4.13% 20.64x

ctfe-stress-5 clippy incr-full 3.92% 19.58x

many-assoc-items clippy full 3.75% 18.73x

wf-projection-stress-65510 clippy incr-full 3.61% 18.06x

unused-warnings clippy incr-unchanged 3.34% 16.68x

regression-31157 clippy incr-full 3.21% 16.04x

coercions clippy incr-full -3.21% 16.03x

unused-warnings clippy incr-patched: dummy fn -3.16% 15.79x

issue-88862 clippy incr-unchanged 3.04% 15.22x

unused-warnings clippy full -2.92% 14.61x

tt-muncher clippy incr-unchanged -2.91% 14.53x

tt-muncher clippy incr-full 2.71% 13.56x

projection-caching clippy full 2.68% 13.38x

issue-58319 clippy incr-unchanged -2.63% 13.16x

deeply-nested-multi clippy full 2.59% 12.95x

tuple-stress clippy incr-patched: new row 2.54% 12.71x

issue-58319 clippy full 2.51% 12.56x

derive clippy full -2.50% 12.50x

token-stream-stress clippy incr-unchanged -2.41% 12.06x

ucd clippy incr-full 2.31% 11.54x

derive clippy incr-full 2.30% 11.49x

issue-46449 clippy incr-full -1.80% 8.98x

many-assoc-items clippy incr-full 1.75% 8.77x

match-stress clippy full 1.62% 8.10x

regression-31157 clippy incr-patched: println 1.59% 7.96x

helloworld-tiny clippy full -1.58% 7.89x

ucd clippy full 1.53% 7.65x

await-call-tree clippy full 1.52% 7.59x

regression-31157 clippy full -1.34% 6.72x

deeply-nested-multi clippy incr-unchanged -1.25% 6.24x

tt-muncher clippy full -1.21% 6.06x

wg-grammar clippy full -1.19% 5.94x

wg-grammar clippy incr-unchanged -1.12% 5.61x

issue-46449 clippy incr-unchanged -1.11% 5.56x

tuple-stress clippy incr-full 1.03% 5.13x

wg-grammar clippy incr-full 0.94% 4.68x

deep-vector clippy incr-full 0.88% 4.39x

issue-46449 clippy incr-patched: static str 6144 0.86% 4.30x

tuple-stress clippy incr-unchanged 0.84% 4.22x

unify-linearly clippy incr-patched: dummy fn 0.80% 3.98x

ctfe-stress-5 clippy full 0.75% 3.74x

tuple-stress clippy full -0.53% 2.66x

ctfe-stress-5 clippy incr-unchanged 0.44% 2.19x

token-stream-stress clippy full -0.43% 2.15x

ripgrep-13.0.0-tiny clippy full 0.41% 2.07x

Inaccurate

xFrednet · 2024-07-21T15:30:55Z

Huh, I was honestly hoping for some more, but that was wishful thinking. Especially after only touching ~0.6% of our lints with a relatively simple refactoring. It's interesting that we have some regressions. But the overall picture seems to be good.

I plan to refactor the remaining lints as well, but not in this PR. This one is already large enough.

bors · 2024-07-22T21:10:12Z

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

blyxyas · 2024-07-24T12:24:25Z

I'll make a new benchmark, I think that the benchmarks done until this point may have been irrelevant (which is good in terms of that maybe we haven't noticed that optimizations are more important that they are, but maybe we've also missed regressions.

I'll re-verify.

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

I'll make a new benchmark, I think that the benchmarks done until this point may have been irrelevant

I'm interested, what has changed?

blyxyas · 2024-07-24T22:50:24Z

The changes wasn't applying properly, so the benchmarks were benchmarking a mix between the natural variance between runs, the change between rust-lang/rust:master and Clippy's slightly outdated toolchain and well, not much more.

I'm not sure how exactly the measurements were convincing enough that we didn't notice (like nearly zero statistic change for small changes, and still a big change for big changes) but the source code that was being measured was not the one from the PR!

I'll have to create a new wrapper, possibly written in Rust and using libraries like git2 instead of written in bash using commands.

…xendoo Lintcheck: Rework and limit diff output for GH's CI ### Background While working on #13136 I found an amazing limitation of GH's CI. The summary can at most have be 1MB of text. Here is the warning message: > $GITHUB_STEP_SUMMARY upload aborted, supports content up to a size of 1024k, got 46731k. For more information see: https://docs.github.com/actions/using-workflows/workflow-commands-for-github-actions#adding-a-markdown-summary [The PR](#13136) produced a *casual* 61808 changes. Guess that's why those lints are not *warn-by-default* :P. ### Changes: This PR limits the lintcheck diff output in two ways. 1. The diff is limited to 200 messages per lint per section. Hidden messages are indicated by a message at the end of the section. 2. The output is first written to a file and only the first 1MB is written to ` >> $GITHUB_STEP_SUMMARY`. The entire file is also written to the normal CI log. This helps for cases where several lints change and the total size exceeds the 1MB limit. An example of these changes can be seen here: https://github.com/xFrednet/rust-clippy/actions/runs/10028799118?pr=4 --- changelog: none r? `@Alexendoo` Sorry for bombarding you with so many PR's lately 😅 Feel free to pass some of you reviews to me.

blyxyas · 2024-07-25T13:48:25Z

How urgent is this PR? I'm currently working on becnhv2, the successor to becnh, written in Rust, it shouldn't take too long to finish and better benchmark this PR.

If you think that it's more important the review than the tool, I'll just do the review, but we won't have benchmark numbers.

xFrednet · 2024-07-25T17:01:53Z

How urgent is this PR?

I mean, this is not critical at all. It would be nice to not have it open for too long, as there is a high potential of conflicts, with how many files it touched, but that's about it. I'd guess the question is what "shouldn't take too long" means 😅

Couldn't we just benchmark this PR afterwards? It's unlikely, that it will degrade performance. There are also #13145 #13144. While I haven't seen an obvious performance thing like the implicit_return lint in this PR, they also have potential to improve some things.

All of this is not a clear answer xD. I'd suggest a compromise. You'll work on the tool and see if you can get it working in the next week. If it turns out to be more complicated, we move forward with this PR and don't block it. Does that sound reasonable?

Make restriction lint's use `span_lint_and_then` (i -> p) This migrates a few restriction lints to use `span_lint_and_then`. This change is motivated by #7797. I've also cleaned up some lint message. Mostly minor stuff. For example: suggestions with a longer message than `"try"` now use `SuggestionStyle::ShowAlways` --- cc: #7797 brother PR of: #13136 changelog: none

bors · 2024-07-27T03:29:35Z

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

bors · 2024-08-03T07:44:44Z

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

blyxyas · 2024-08-05T00:22:59Z

I'm having problems getting the version for open PRs. The benchmarking tool is having no problems handling PRs a few weeks old (for example, #13089), but PRs that are built on current versions (like, something in between stable and master, but using beta is not yielding success).

I'll just review this and try to benchmark it once some time has happened. And in the mean time I'll try to get it to work on these new versions.

blyxyas

LGTM, thanks! ❤️

blyxyas · 2024-08-05T13:32:09Z

@bors r+

We'll benchmark this in the (hopefully near) future :)

bors · 2024-08-05T13:32:11Z

📌 Commit 0532104 has been approved by blyxyas

It is now in the queue for this repository.

bors · 2024-08-05T13:33:19Z

⌛ Testing commit 0532104 with merge c082bc2...

bors · 2024-08-05T13:41:15Z

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

bors · 2024-08-05T13:41:15Z

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

bors · 2024-08-05T13:42:14Z

👀 Test was successful, but fast-forwarding failed: 422 Changes must be made through a pull request.

Make restriction lint's use `span_lint_and_then` (q -> w) This migrates a few restriction lints to use `span_lint_and_then`. This change is motivated by #7797. I've also cleaned up some lint message. Mostly minor stuff. For example: suggestions with a longer message than `"try"` now use `SuggestionStyle::ShowAlways` --- cc: #7797 sister PR of: #13136 changelog: none

rustbot assigned blyxyas Jul 21, 2024

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jul 21, 2024

xFrednet mentioned this pull request Jul 21, 2024

Migrate to span_clippy_lint #7797

Open

3 tasks

xFrednet force-pushed the 07797-restriction-and-then branch from 68bb0e8 to dbc6914 Compare July 21, 2024 10:23

xFrednet mentioned this pull request Jul 21, 2024

Lintcheck: Rework and limit diff output for GH's CI #13139

Merged

xFrednet changed the title ~~Make restriction lint's use span_lint_and_then (40/116)~~ Make restriction lint's use span_lint_and_then (a -> e) Jul 22, 2024

This was referenced Jul 22, 2024

Make restriction lint's use span_lint_and_then (i -> p) #13144

Merged

Make restriction lint's use span_lint_and_then (q -> w) #13145

Merged

xFrednet force-pushed the 07797-restriction-and-then branch from dbc6914 to c85e305 Compare July 22, 2024 22:11

xFrednet force-pushed the 07797-restriction-and-then branch from c85e305 to a95abb4 Compare July 27, 2024 11:43

blyxyas mentioned this pull request Jul 29, 2024

Optimizing Clippy & linting rust-lang/rust-project-goals#114

Open

2 tasks

xFrednet added 2 commits August 3, 2024 10:18

Migrating restriction lints to span_lint_and_then (a -> d)

a9d72c7

Migrating restriction lints to span_lint_and_then (e -> i)

0532104

xFrednet force-pushed the 07797-restriction-and-then branch from a95abb4 to 0532104 Compare August 3, 2024 08:18

blyxyas approved these changes Aug 5, 2024

View reviewed changes

bors merged commit c082bc2 into rust-lang:master Aug 5, 2024
8 checks passed

xFrednet deleted the 07797-restriction-and-then branch August 5, 2024 14:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make restriction lint's use `span_lint_and_then` (a -> e) #13136

Make restriction lint's use `span_lint_and_then` (a -> e) #13136

xFrednet commented Jul 21, 2024 •

edited

Loading

blyxyas commented Jul 21, 2024

xFrednet commented Jul 21, 2024 •

edited

Loading

xFrednet commented Jul 21, 2024

blyxyas commented Jul 21, 2024 •

edited

Loading

Summary

Primary benchmarks

Secondary benchmarks

Cycles, less accurate

Summary

Primary benchmarks

Secondary benchmarks

xFrednet commented Jul 21, 2024

bors commented Jul 22, 2024

blyxyas commented Jul 24, 2024

xFrednet commented Jul 24, 2024

blyxyas commented Jul 24, 2024 •

edited

Loading

blyxyas commented Jul 25, 2024

xFrednet commented Jul 25, 2024

bors commented Jul 27, 2024

bors commented Aug 3, 2024

blyxyas commented Aug 5, 2024 •

edited

Loading

blyxyas left a comment

blyxyas commented Aug 5, 2024

bors commented Aug 5, 2024

bors commented Aug 5, 2024

bors commented Aug 5, 2024

bors commented Aug 5, 2024

bors commented Aug 5, 2024

Make restriction lint's use span_lint_and_then (a -> e) #13136

Make restriction lint's use span_lint_and_then (a -> e) #13136

Conversation

xFrednet commented Jul 21, 2024 • edited Loading

blyxyas commented Jul 21, 2024

xFrednet commented Jul 21, 2024 • edited Loading

Footnotes

xFrednet commented Jul 21, 2024

blyxyas commented Jul 21, 2024 • edited Loading

Summary

Primary benchmarks

Secondary benchmarks

Cycles, less accurate

Summary

Primary benchmarks

Secondary benchmarks

xFrednet commented Jul 21, 2024

bors commented Jul 22, 2024

blyxyas commented Jul 24, 2024

xFrednet commented Jul 24, 2024

blyxyas commented Jul 24, 2024 • edited Loading

blyxyas commented Jul 25, 2024

xFrednet commented Jul 25, 2024

bors commented Jul 27, 2024

bors commented Aug 3, 2024

blyxyas commented Aug 5, 2024 • edited Loading

blyxyas left a comment

Choose a reason for hiding this comment

blyxyas commented Aug 5, 2024

bors commented Aug 5, 2024

bors commented Aug 5, 2024

bors commented Aug 5, 2024

bors commented Aug 5, 2024

bors commented Aug 5, 2024

Make restriction lint's use `span_lint_and_then` (a -> e) #13136

Make restriction lint's use `span_lint_and_then` (a -> e) #13136

xFrednet commented Jul 21, 2024 •

edited

Loading

xFrednet commented Jul 21, 2024 •

edited

Loading

blyxyas commented Jul 21, 2024 •

edited

Loading

blyxyas commented Jul 24, 2024 •

edited

Loading

blyxyas commented Aug 5, 2024 •

edited

Loading