Skip to content

chore: Improve how the other suggestions message gets rendered #143661

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Muscraft
Copy link
Member

@Muscraft Muscraft commented Jul 9, 2025

Note: This change is part of my ongoing work to use annotate-snippets as rustc's emitter

This change started as a way to remove some specialty code paths from annotate-snippets, by making the "and {} other candidates" message get rendered like a secondary message with no level, but turned into a fix for the message's Unicode output. Before this change, when using the Unicode output, the other suggestions message would get rendered outside of the main suggestion block, making it feel disconnected from what it was referring to. This change makes it so that the message is on the last line of the block, aligning its rendering with other secondary messages, and making it clear what the message is referring to.

Before:

error[E0433]: failed to resolve: use of undeclared type `IntoIter`
   ╭▸ $DIR/issue-82956.rs:28:24
   │
LL │         let mut iter = IntoIter::new(self);
   │                        ━━━━━━━━ use of undeclared type `IntoIter`
   ╰╴
help: consider importing one of these structs
   ╭╴
LL + use std::array::IntoIter;
   ├╴
LL + use std::collections::binary_heap::IntoIter;
   ├╴
LL + use std::collections::btree_map::IntoIter;
   ├╴
LL + use std::collections::btree_set::IntoIter;
   ╰╴
     and 9 other candidates

After:

error[E0433]: failed to resolve: use of undeclared type `IntoIter`
   ╭▸ $DIR/issue-82956.rs:28:24
   │
LL │         let mut iter = IntoIter::new(self);
   │                        ━━━━━━━━ use of undeclared type `IntoIter`
   ╰╴
help: consider importing one of these structs
   ╭╴
LL + use std::array::IntoIter;
   ├╴
LL + use std::collections::binary_heap::IntoIter;
   ├╴
LL + use std::collections::btree_map::IntoIter;
   ├╴
LL + use std::collections::btree_set::IntoIter;
   │
   ╰ and 9 other candidates

@rustbot
Copy link
Collaborator

rustbot commented Jul 9, 2025

r? @compiler-errors

rustbot has assigned @compiler-errors.
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-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 9, 2025
Comment on lines 1 to 2
//@ revisions: ascii unicode
//@[unicode] compile-flags: -Zunstable-options --error-format=human-unicode
Copy link
Member

@jieyouxu jieyouxu Jul 9, 2025

Choose a reason for hiding this comment

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

Discussion: maybe use e.g. a copy of this test for diagnostics emitter testing? This const-generics test AFAICT is a regression test (for a previous ICE), I feel like it probably shouldn't be dual-serving as diagnostics emitter testing, as there are two different test intentions? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

That is a very good point. I had not considered why a test exists when choosing one. I agree that it shouldn't serve the role of a regression test and a diagnostic emitter test. Looking at the other available tests, tests/ui/suggestions/too-many-field-suggestions.stderr looks to be the best candidate, but I would be happy to create a dedicated test if that is better.

Copy link
Member

@jieyouxu jieyouxu Jul 9, 2025

Choose a reason for hiding this comment

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

I think a dedicated test for this specific purpose is probably best, even if superficially it looks like an exact copy of another test, because even that test looks like it has a different test intention (of suggestions for field suggestions). But a test comment would clarify that so.

In that, for the intentions of this PR, the actual "error" doesn't matter as much as the suggestion format itself :)

@Muscraft Muscraft force-pushed the other-suggestion-message branch from 97c03f1 to 2230273 Compare July 9, 2025 10:09
@Muscraft Muscraft force-pushed the other-suggestion-message branch from 2230273 to 0043854 Compare July 9, 2025 13:14
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. 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.

4 participants