optimize tidy check on `src/tools/tidy/src/issues.txt` #123339

onur-ozkan · 2024-04-01T21:02:16Z

This change applies to the following:

Handles is_sorted in the first iteration without needing a second.
Fixes line sorting on --bless.
Reads issues.txt as str rather than making it part of the source code.

rustbot · 2024-04-01T21:02:25Z

rustbot has assigned @Mark-Simulacrum.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

workingjubilee · 2024-04-01T21:11:00Z

src/tools/tidy/src/ui_tests.rs

-    if bless && !remaining_issue_names.is_empty() {
-        let issues_txt_header = r#"
-/*
-============================================================
-    ⚠️⚠️⚠️NOTHING SHOULD EVER BE ADDED TO THIS LIST⚠️⚠️⚠️
-============================================================
-*/
-[
-"#;


I genuinely do not think we should remove this.

I understand the concern but I'm not sure how this will help. If someone adds a line to the end of this file, reviewers won't be able to see this header anyway.

How about hardcoding the current length and checking if it's not equal to the lines in this file? This way we will continuously be aware of it without relying on reviewers mistakes.

It is not for informing a reviewer.

const ISSUES_ENTRY_LIMIT is already checked.

It is not for informing a reviewer.

It has to. When someone accidentally adds a line, reviewer should be aware of this information.

const ISSUES_ENTRY_LIMIT is already checked.

What I mean is this:

diff --git a/src/tools/tidy/src/ui_tests.rs b/src/tools/tidy/src/ui_tests.rs index 5b3b948d4a1..86e72823036 100644 --- a/src/tools/tidy/src/ui_tests.rs +++ b/src/tools/tidy/src/ui_tests.rs @@ -100,6 +100,9 @@ fn check_entries(tests_path: &Path, bad: &mut bool) { } pub fn check(root_path: &Path, bless: bool, bad: &mut bool) { + // This can be decreased but can never be increased. + const EXPECTED_LINE_COUNT: usize = 4372; + let path = &root_path.join("tests"); check_entries(&path, bad); @@ -120,6 +123,8 @@ pub fn check(root_path: &Path, bless: bool, bad: &mut bool) { }) .collect(); + assert_eq!(EXPECTED_LINE_COUNT, allowed_issue_names.len(), "If you have added new lines to issues.txt, please revert them; otherwise, decrease the EXPECTED_LINE_COUNT."); + if !is_sorted && !bless { tidy_error!( bad,

It has to. When someone accidentally adds a line, reviewer should be aware of this information.

The vast majority of the information in the Rust repository is addressed directly at the contributor, and only incidentally useful to a reviewer, so I disagree with this "has to" assertion.

Anyways, the reason there's nothing exactingly doing assert_eq! is because there was a request in the original PR to make this list --blessable, to reduce annoyance. So I implemented that. Making sure this number --blesses only up and not down would require a lot of fiddly logic, I would think? And it would also invite merge conflicts on the value a lot.

bors · 2024-04-03T23:10:56Z

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

Mark-Simulacrum · 2024-04-05T14:14:11Z

It seems like we ought to be able to move to using include_str!() and skip a fixed header before iterating through the lines, without making any other, unrelated changes to the behavior (e.g., adding an assertion about a specific number).

onur-ozkan · 2024-04-05T15:05:16Z

@rustbot ready

Mark-Simulacrum

r=me with nits fixed

Mark-Simulacrum · 2024-04-05T20:11:44Z

src/tools/tidy/src/ui_tests.rs

+    let mut is_sorted = true;
+    let allowed_issue_names: BTreeSet<_> = include_str!("issues.txt")
+        .lines()
+        .skip(issues_txt_header.lines().count()) // skip the header lines


This should probably be a strip_prefix(issues_txt_header), so that we assert that the contents of that block have not changed.

Mark-Simulacrum · 2024-04-05T20:12:55Z

src/tools/tidy/src/ui_tests.rs

+                is_sorted = false;
+            }
+
+            prev_line = line.clone();


FWIW, there's some needless copying going on here AFAICT. BTreeSet<&'static str> should be fine (and should be what you get from iterating lines).

onur-ozkan · 2024-04-05T21:18:18Z

@bors r=Mark-Simulacrum

bors · 2024-04-05T21:18:21Z

📌 Commit 49c10e4 has been approved by Mark-Simulacrum

It is now in the queue for this repository.

…Mark-Simulacrum optimize tidy check on `src/tools/tidy/src/issues.txt` This change applies to the following: - Handles `is_sorted` in the first iteration without needing a second. - Fixes line sorting on `--bless`. - Reads `issues.txt` as str rather than making it part of the source code. Fixes rust-lang#123002

…iaskrgr Rollup of 6 pull requests Successful merges: - rust-lang#114788 (impl get_mut_or_init and get_mut_or_try_init for OnceCell and OnceLock) - rust-lang#122291 (Stabilize `const_caller_location` and `const_location_fields`) - rust-lang#123321 (Bump dependencies) - rust-lang#123339 (optimize tidy check on `src/tools/tidy/src/issues.txt`) - rust-lang#123357 (CI: Redirect stderr to stdout to order GHA logs) - rust-lang#123504 (bootstrap: split cargo-miri test into separate Step) r? `@ghost` `@rustbot` modify labels: rollup

…Mark-Simulacrum optimize tidy check on `src/tools/tidy/src/issues.txt` This change applies to the following: - Handles `is_sorted` in the first iteration without needing a second. - Fixes line sorting on `--bless`. - Reads `issues.txt` as str rather than making it part of the source code. Fixes rust-lang#123002

…iaskrgr Rollup of 5 pull requests Successful merges: - rust-lang#114788 (impl get_mut_or_init and get_mut_or_try_init for OnceCell and OnceLock) - rust-lang#122291 (Stabilize `const_caller_location` and `const_location_fields`) - rust-lang#123339 (optimize tidy check on `src/tools/tidy/src/issues.txt`) - rust-lang#123357 (CI: Redirect stderr to stdout to order GHA logs) - rust-lang#123504 (bootstrap: split cargo-miri test into separate Step) r? `@ghost` `@rustbot` modify labels: rollup

matthiaskrgr · 2024-04-06T10:59:32Z

@bors r-
#123544 (comment)

This change applies to the following: - Handles `is_sorted` in the first iteration without needing a second. - Fixes line sorting on `--bless`. - Reads `issues.txt` as str rather than making it part of the source code. Signed-off-by: onur-ozkan <work@onurozkan.dev>

onur-ozkan · 2024-04-06T11:54:49Z

@bors r=Mark-Simulacrum (corrected the usage of / in Windows paths.)

bors · 2024-04-06T11:54:51Z

📌 Commit b0f2e60 has been approved by Mark-Simulacrum

It is now in the queue for this repository.

bors · 2024-04-06T15:10:26Z

⌛ Testing commit b0f2e60 with merge 773fb88...

bors · 2024-04-06T17:11:05Z

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing 773fb88 to master...

rust-timer · 2024-04-06T18:27:57Z

Finished benchmarking commit (773fb88): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.2%	[2.2%, 2.2%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.2%	[-2.2%, -2.2%]	1
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 666.761s -> 668.041s (0.19%)
Artifact size: 318.27 MiB -> 318.25 MiB (-0.01%)

rustbot assigned Mark-Simulacrum Apr 1, 2024

rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Apr 1, 2024

onur-ozkan mentioned this pull request Apr 1, 2024

tidy compiles very slowly #123002

Closed

onur-ozkan force-pushed the optimize-tidy-check branch from 0ae5bae to 6e5caac Compare April 1, 2024 21:05

workingjubilee reviewed Apr 1, 2024

View reviewed changes

Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 5, 2024

onur-ozkan force-pushed the optimize-tidy-check branch from 2e202e9 to e9904e7 Compare April 5, 2024 15:04

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 5, 2024

Mark-Simulacrum reviewed Apr 5, 2024

View reviewed changes

onur-ozkan force-pushed the optimize-tidy-check branch from e9904e7 to 49c10e4 Compare April 5, 2024 21:01

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 5, 2024

matthiaskrgr mentioned this pull request Apr 6, 2024

Rollup of 7 pull requests #123539

Closed

matthiaskrgr mentioned this pull request Apr 6, 2024

Rollup of 6 pull requests #123542

Closed

matthiaskrgr mentioned this pull request Apr 6, 2024

Rollup of 5 pull requests #123544

Closed

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 6, 2024

onur-ozkan force-pushed the optimize-tidy-check branch from 49c10e4 to b0f2e60 Compare April 6, 2024 11:49

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 6, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 6, 2024

bors merged commit 773fb88 into rust-lang:master Apr 6, 2024

rustbot added this to the 1.79.0 milestone Apr 6, 2024

onur-ozkan deleted the optimize-tidy-check branch April 6, 2024 17:14

This was referenced Apr 6, 2024

Rewrite native thread-local storage #116123

Merged

Simplify key-based thread locals #122494

Merged

Rework ptr-to-ref conversion suggestion for method calls #123007

Merged

optimize tidy check on src/tools/tidy/src/issues.txt #123339

optimize tidy check on src/tools/tidy/src/issues.txt #123339

Uh oh!

Conversation

onur-ozkan commented Apr 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Apr 1, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

onur-ozkan Apr 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bors commented Apr 3, 2024

Uh oh!

Mark-Simulacrum commented Apr 5, 2024

Uh oh!

onur-ozkan commented Apr 5, 2024

Uh oh!

Mark-Simulacrum left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

onur-ozkan commented Apr 5, 2024

Uh oh!

bors commented Apr 5, 2024

Uh oh!

matthiaskrgr commented Apr 6, 2024

Uh oh!

onur-ozkan commented Apr 6, 2024

Uh oh!

bors commented Apr 6, 2024

Uh oh!

bors commented Apr 6, 2024

Uh oh!

bors commented Apr 6, 2024

Uh oh!

rust-timer commented Apr 6, 2024

Overall result: no relevant changes - no action needed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

optimize tidy check on `src/tools/tidy/src/issues.txt` #123339

optimize tidy check on `src/tools/tidy/src/issues.txt` #123339

onur-ozkan commented Apr 1, 2024 •

edited

Loading

onur-ozkan Apr 1, 2024 •

edited

Loading

Mark-Simulacrum left a comment •

edited

Loading