Skip to content

Conversation

Muscraft
Copy link
Member

@Muscraft Muscraft commented Sep 2, 2025

While testing my changes to make rustc use annotate-snippets, I encountered a new clippy test failure stemming from two suggestion output changes in #145273. The new output in these two cases feels like a regression as it is not as clear as the old output, and adds unnecessary information.

Before #145273 (Diff style)
before

After #145273 ("multi-line" style)
after

The reason for the change was that a new suggestion part (which matches existing code) was added on a different line than the existing parts, causing the suggestion style to change from Diff to "multi-line". Since this new part matches existing code, no code changes show up in the output for it, but it still makes the suggestion style "multi-line" when it doesn't need to be.

To get the old output back, I made it so that suggestion parts that perfectly match existing code get filtered out.

try-job: aarch64-apple

@rustbot
Copy link
Collaborator

rustbot commented Sep 2, 2025

r? @petrochenkov

rustbot has assigned @petrochenkov.
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

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-clippy Relevant to the Clippy team. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 2, 2025
@rustbot
Copy link
Collaborator

rustbot commented Sep 2, 2025

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Collaborator

bors commented Sep 2, 2025

📌 Commit f696cd8 has been approved by petrochenkov

It is now in the queue for this repository.

@bors 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 Sep 2, 2025
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Sep 2, 2025
…r=petrochenkov

fix: Filter suggestion parts that match existing code

While testing my changes to make `rustc` use `annotate-snippets`, I encountered a new `clippy` test failure stemming from [two](https://github.com/rust-lang/rust/pull/145273/files#diff-6e8403e31463539666afbc00479cb416dc767a518f562b6e2960630953ee7da2R275-R278) [suggestion](https://github.com/rust-lang/rust/pull/145273/files#diff-6e8403e31463539666afbc00479cb416dc767a518f562b6e2960630953ee7da2R289-R292) output changes in rust-lang#145273. The new output in these two cases feels like a regression as it is not as clear as the old output, and adds unnecessary information.

Before rust-lang#145273 (`Diff` style)
![before](https://github.com/user-attachments/assets/36f33635-cbce-45f1-823d-0cbe6f0cfe46)

After rust-lang#145273 ("multi-line" style)
![after](https://github.com/user-attachments/assets/d4cb00b8-5a42-436e-9329-db84347138f0)

The reason for the change was that a new suggestion part (which matches existing code) was added on a different line than the existing parts, causing the suggestion style to change from `Diff` to "multi-line". Since this new part matches existing code, no code changes show up in the output for it, but it still makes the suggestion style "multi-line" when it doesn't need to be.

To get the old output back, I made it so that suggestion parts that perfectly match existing code get filtered out.
bors added a commit that referenced this pull request Sep 2, 2025
Rollup of 8 pull requests

Successful merges:

 - #139113 (unstable book: in a sanitizer example, check the code)
 - #145823 (editorconfig: don't use nonexistent syntax)
 - #145962 (Ensure we emit an allocator shim when only some crate types need one)
 - #146032 (Explicity disable LSX feature for `loongarch64-unknown-none` target)
 - #146090 (Derive `PartialEq` for `InvisibleOrigin`)
 - #146120 (Correct typo in `rustc_errors` comment)
 - #146121 (fix: Filter suggestion parts that match existing code)
 - #146134 (llvm: nvptx: Layout update to match LLVM)

r? `@ghost`
`@rustbot` modify labels: rollup
tgross35 added a commit to tgross35/rust that referenced this pull request Sep 3, 2025
…r=petrochenkov

fix: Filter suggestion parts that match existing code

While testing my changes to make `rustc` use `annotate-snippets`, I encountered a new `clippy` test failure stemming from [two](https://github.com/rust-lang/rust/pull/145273/files#diff-6e8403e31463539666afbc00479cb416dc767a518f562b6e2960630953ee7da2R275-R278) [suggestion](https://github.com/rust-lang/rust/pull/145273/files#diff-6e8403e31463539666afbc00479cb416dc767a518f562b6e2960630953ee7da2R289-R292) output changes in rust-lang#145273. The new output in these two cases feels like a regression as it is not as clear as the old output, and adds unnecessary information.

Before rust-lang#145273 (`Diff` style)
![before](https://github.com/user-attachments/assets/36f33635-cbce-45f1-823d-0cbe6f0cfe46)

After rust-lang#145273 ("multi-line" style)
![after](https://github.com/user-attachments/assets/d4cb00b8-5a42-436e-9329-db84347138f0)

The reason for the change was that a new suggestion part (which matches existing code) was added on a different line than the existing parts, causing the suggestion style to change from `Diff` to "multi-line". Since this new part matches existing code, no code changes show up in the output for it, but it still makes the suggestion style "multi-line" when it doesn't need to be.

To get the old output back, I made it so that suggestion parts that perfectly match existing code get filtered out.
tgross35 added a commit to tgross35/rust that referenced this pull request Sep 3, 2025
…r=petrochenkov

fix: Filter suggestion parts that match existing code

While testing my changes to make `rustc` use `annotate-snippets`, I encountered a new `clippy` test failure stemming from [two](https://github.com/rust-lang/rust/pull/145273/files#diff-6e8403e31463539666afbc00479cb416dc767a518f562b6e2960630953ee7da2R275-R278) [suggestion](https://github.com/rust-lang/rust/pull/145273/files#diff-6e8403e31463539666afbc00479cb416dc767a518f562b6e2960630953ee7da2R289-R292) output changes in rust-lang#145273. The new output in these two cases feels like a regression as it is not as clear as the old output, and adds unnecessary information.

Before rust-lang#145273 (`Diff` style)
![before](https://github.com/user-attachments/assets/36f33635-cbce-45f1-823d-0cbe6f0cfe46)

After rust-lang#145273 ("multi-line" style)
![after](https://github.com/user-attachments/assets/d4cb00b8-5a42-436e-9329-db84347138f0)

The reason for the change was that a new suggestion part (which matches existing code) was added on a different line than the existing parts, causing the suggestion style to change from `Diff` to "multi-line". Since this new part matches existing code, no code changes show up in the output for it, but it still makes the suggestion style "multi-line" when it doesn't need to be.

To get the old output back, I made it so that suggestion parts that perfectly match existing code get filtered out.
@jieyouxu
Copy link
Member

jieyouxu commented Sep 3, 2025

This probably also fixes (or at least suppresses tests/crashes/131762.rs): #146139 (comment)

Could you convert it into a ui test or remove if redundant?

@bors r-

@bors 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 Sep 3, 2025
@jieyouxu
Copy link
Member

jieyouxu commented Sep 3, 2025

(Let me double-check)

@bors try jobs=aarch64-apple

rust-bors bot added a commit that referenced this pull request Sep 3, 2025
fix: Filter suggestion parts that match existing code

try-job: aarch64-apple
@rust-bors

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-bors

This comment was marked as resolved.

@Muscraft Muscraft force-pushed the filter-suggestion-parts branch from f696cd8 to 3f529cf Compare September 3, 2025 14:28
@rustbot

This comment has been minimized.

@Muscraft
Copy link
Member Author

Muscraft commented Sep 3, 2025

I have been unable to reproduce the test failure on my Mac locally, so I might have to use try builds to figure it out.

@bors try jobs=aarch64-apple

@rust-bors

This comment has been minimized.

rust-bors bot added a commit that referenced this pull request Sep 3, 2025
fix: Filter suggestion parts that match existing code

try-job: aarch64-apple
@rust-log-analyzer

This comment has been minimized.

@rust-bors

This comment was marked as resolved.

@Muscraft Muscraft force-pushed the filter-suggestion-parts branch from 3f529cf to e7fac67 Compare September 4, 2025 23:19
@rustbot
Copy link
Collaborator

rustbot commented Sep 4, 2025

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

@rust-log-analyzer

This comment has been minimized.

@Muscraft Muscraft force-pushed the filter-suggestion-parts branch from e7fac67 to b307a11 Compare September 4, 2025 23:42
@Muscraft
Copy link
Member Author

Muscraft commented Sep 5, 2025

I spent a ton of time looking into why the ICE was no longer happening on CI, but would still happen locally.

Before my change, something causes one of the spans in tests/crashes/131762.rs to be malformed, which causes a suggestion to have overlapping parts, one of which is empty (span is empty and snippet is empty). When running in CI (or debug-assertions weren't enabled), the overlapping Parts would fail this assert!, causing the expected ICE. When running with debug-assertions enabled, the empty Part would fail this debug_assert_eq!, causing an ICE before the other assert! could be reached. In both cases, tests/crashes/131762.rs would ICE, so the test would pass. With my original changes, when debug-assertions were enabled, it would ICE on the debug_assert_eq!, so the test would pass. However, when debug-assertions were disabled, my changes would filter out the empty Part before the assert!, making it so there were no overlapping Parts, so no ICE happened.

After thinking about it for a bit, I realized that my changes were only masking the ICE, not fixing the cause of it. To make it so, I was no longer "masking" the ICE; I moved the assert! checking for disjoint spans before the filter for suggestion parts that match existing code, making it so an ICE would still happen if a suggestion's spans weren't disjoint.

@rustbot review

@rustbot 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 Sep 5, 2025
@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Collaborator

bors commented Sep 5, 2025

📌 Commit b307a11 has been approved by petrochenkov

It is now in the queue for this repository.

@bors 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 Sep 5, 2025
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Sep 5, 2025
…r=petrochenkov

fix: Filter suggestion parts that match existing code

While testing my changes to make `rustc` use `annotate-snippets`, I encountered a new `clippy` test failure stemming from [two](https://github.com/rust-lang/rust/pull/145273/files#diff-6e8403e31463539666afbc00479cb416dc767a518f562b6e2960630953ee7da2R275-R278) [suggestion](https://github.com/rust-lang/rust/pull/145273/files#diff-6e8403e31463539666afbc00479cb416dc767a518f562b6e2960630953ee7da2R289-R292) output changes in rust-lang#145273. The new output in these two cases feels like a regression as it is not as clear as the old output, and adds unnecessary information.

Before rust-lang#145273 (`Diff` style)
![before](https://github.com/user-attachments/assets/36f33635-cbce-45f1-823d-0cbe6f0cfe46)

After rust-lang#145273 ("multi-line" style)
![after](https://github.com/user-attachments/assets/d4cb00b8-5a42-436e-9329-db84347138f0)

The reason for the change was that a new suggestion part (which matches existing code) was added on a different line than the existing parts, causing the suggestion style to change from `Diff` to "multi-line". Since this new part matches existing code, no code changes show up in the output for it, but it still makes the suggestion style "multi-line" when it doesn't need to be.

To get the old output back, I made it so that suggestion parts that perfectly match existing code get filtered out.

try-job: aarch64-apple
bors added a commit that referenced this pull request Sep 5, 2025
Rollup of 4 pull requests

Successful merges:

 - #138944 (Add `__isPlatformVersionAtLeast` and `__isOSVersionAtLeast` symbols)
 - #146041 (tidy: --bless now makes escheck run with --fix)
 - #146121 (fix: Filter suggestion parts that match existing code)
 - #146241 (rustc_infer: change top-level doc comment to inner)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors
Copy link
Collaborator

bors commented Sep 5, 2025

⌛ Testing commit b307a11 with merge 99317ef...

@bors
Copy link
Collaborator

bors commented Sep 5, 2025

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing 99317ef to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 5, 2025
@bors bors merged commit 99317ef into rust-lang:master Sep 5, 2025
11 checks passed
@rustbot rustbot added this to the 1.91.0 milestone Sep 5, 2025
bors added a commit that referenced this pull request Sep 5, 2025
Rollup of 4 pull requests

Successful merges:

 - #138944 (Add `__isPlatformVersionAtLeast` and `__isOSVersionAtLeast` symbols)
 - #146041 (tidy: --bless now makes escheck run with --fix)
 - #146121 (fix: Filter suggestion parts that match existing code)
 - #146241 (rustc_infer: change top-level doc comment to inner)

r? `@ghost`
`@rustbot` modify labels: rollup
Copy link
Contributor

github-actions bot commented Sep 5, 2025

What is this? This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.

Comparing 9cd272d (parent) -> 99317ef (this PR)

Test differences

Show 4 test diffs

4 doctest diffs were found. These are ignored, as they are noisy.

Test dashboard

Run

cargo run --manifest-path src/ci/citool/Cargo.toml -- \
    test-dashboard 99317ef14d0be42fa4039eea7c5ce50cb4e9aee7 --output-dir test-dashboard

And then open test-dashboard/index.html in your browser to see an overview of all executed tests.

Job duration changes

  1. dist-aarch64-linux: 8950.7s -> 6408.0s (-28.4%)
  2. dist-various-1: 3687.6s -> 4222.4s (14.5%)
  3. x86_64-rust-for-linux: 2552.0s -> 2864.5s (12.2%)
  4. x86_64-gnu-stable: 7912.3s -> 7049.0s (-10.9%)
  5. i686-gnu-nopt-1: 7251.2s -> 7998.3s (10.3%)
  6. x86_64-gnu-llvm-19-3: 6371.6s -> 6969.0s (9.4%)
  7. dist-apple-various: 4092.4s -> 4461.9s (9.0%)
  8. aarch64-gnu-llvm-19-1: 3342.3s -> 3634.6s (8.7%)
  9. armhf-gnu: 5128.8s -> 4718.9s (-8.0%)
  10. i686-gnu-2: 5430.2s -> 5859.7s (7.9%)
How to interpret the job duration changes?

Job durations can vary a lot, based on the actual runner instance
that executed the job, system noise, invalidated caches, etc. The table above is provided
mostly for t-infra members, for simpler debugging of potential CI slow-downs.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (99317ef): 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)

Results (primary 1.6%, secondary -2.1%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)
3.9% [3.9%, 3.9%] 1
Regressions ❌
(secondary)
2.4% [2.4%, 2.4%] 1
Improvements ✅
(primary)
-0.7% [-0.7%, -0.7%] 1
Improvements ✅
(secondary)
-3.2% [-5.5%, -2.1%] 4
All ❌✅ (primary) 1.6% [-0.7%, 3.9%] 2

Cycles

Results (primary 2.1%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

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

Binary size

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

Bootstrap: 466.363s -> 468.222s (0.40%)
Artifact size: 390.44 MiB -> 390.57 MiB (0.03%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-clippy Relevant to the Clippy team. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants