Skip to content
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

annotate-snippet emitter: Include suggestions in output #61809

Open
phansch opened this issue Jun 13, 2019 · 3 comments
Open

annotate-snippet emitter: Include suggestions in output #61809

phansch opened this issue Jun 13, 2019 · 3 comments
Assignees
Labels
A-diagnostics Area: Messages for errors, warnings, and lints C-enhancement Category: An issue proposing an enhancement or a PR with one. E-help-wanted Call for participation: Help is requested to fix this issue. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@phansch
Copy link
Member

phansch commented Jun 13, 2019

Part of #59346

In order for the new AnnotateSnippetEmitterWriter to include suggestions in the output, we essentially have to pass &db.suggestions to emit_messages_default and deal with a couple of edge-cases.

Relevant FIXME:

// FIXME(#59346): Collect suggestions (see emitter.rs)
let suggestions: &[_] = &[];

emitter.rs equivalent:

if let Some((sugg, rest)) = db.suggestions.split_first() {
if rest.is_empty() &&
// don't display multi-suggestions as labels
sugg.substitutions.len() == 1 &&
// don't display multipart suggestions as labels
sugg.substitutions[0].parts.len() == 1 &&
// don't display long messages as labels
sugg.msg.split_whitespace().count() < 10 &&
// don't display multiline suggestions as labels
!sugg.substitutions[0].parts[0].snippet.contains('\n') &&
// when this style is set we want the suggestion to be a message, not inline
sugg.style != SuggestionStyle::HideCodeAlways &&
// trivial suggestion for tooling's sake, never shown
sugg.style != SuggestionStyle::CompletelyHidden
{
let substitution = &sugg.substitutions[0].parts[0].snippet.trim();
let msg = if substitution.len() == 0 || sugg.style.hide_inline() {
// This substitution is only removal or we explicitly don't want to show the
// code inline, don't show it
format!("help: {}", sugg.msg)
} else {
format!("help: {}: `{}`", sugg.msg, substitution)
};
primary_span.push_span_label(sugg.substitutions[0].parts[0].span, msg);
} else {
// if there are multiple suggestions, print them all in full
// to be consistent. We could try to figure out if we can
// make one (or the first one) inline, but that would give
// undue importance to a semi-random suggestion
suggestions = &db.suggestions;
}
}

  • The tricky part is figuring out the first half of the conditional. We probably need it in the new emitter, too. Is it enough to just copy it over? Maybe extract that code so that the code is shared in both emitters?
  • Otherwise it's just passing &db.suggestions through
  • Should take into account the MAX_SUGGESTIONS value somewhere (add a UI test for this)

This issue has been assigned to @phansch via this comment.

@phansch

This comment has been minimized.

@rustbot rustbot added the A-diagnostics Area: Messages for errors, warnings, and lints label Jun 13, 2019
@phansch phansch changed the title annotate_snippet emitter: Include suggestions in output annotate-snippet emitter: Include suggestions in output Jun 13, 2019
@phansch

This comment has been minimized.

@rustbot rustbot added the E-help-wanted Call for participation: Help is requested to fix this issue. label Jun 13, 2019
@phansch
Copy link
Member Author

phansch commented Aug 31, 2019

@rustbot claim

@rustbot rustbot self-assigned this Aug 31, 2019
phansch added a commit to phansch/rust that referenced this issue Sep 1, 2019
An initial refactoring before working on rust-lang#61809.

This moves the whole block into a method so that it can be reused in the
annotate-snippet output. It's already used in the new emitter, but
there's no UI tests with suggestions included in this PR.

A first look at some UI tests with suggestions showed that there's some
more work to do in [annotate-snippet-rs][annotate-snippet-rs] before the
new output is closer to the current one.
Centril added a commit to Centril/rust that referenced this issue Sep 1, 2019
…bank

librustc_errors: Extract sugg/subst handling into method

An initial refactoring before working on rust-lang#61809.

This moves the whole block into a method so that it can be reused in the
annotate-snippet emitter. The method is already used in the new emitter, but
there's no UI tests with suggestions included in this PR.

A first look at some UI tests with suggestions showed that there's some
more work to do in [annotate-snippet-rs][annotate-snippet-rs] before the new output is closer to the
current one, so I opted to do that in a second step.

r? @estebank

[annotate-snippet-rs]: https://github.com/rust-lang/annotate-snippets-rs
Centril added a commit to Centril/rust that referenced this issue Sep 3, 2019
…bank

librustc_errors: Extract sugg/subst handling into method

An initial refactoring before working on rust-lang#61809.

This moves the whole block into a method so that it can be reused in the
annotate-snippet emitter. The method is already used in the new emitter, but
there's no UI tests with suggestions included in this PR.

A first look at some UI tests with suggestions showed that there's some
more work to do in [annotate-snippet-rs][annotate-snippet-rs] before the new output is closer to the
current one, so I opted to do that in a second step.

r? @estebank

[annotate-snippet-rs]: https://github.com/rust-lang/annotate-snippets-rs
Centril added a commit to Centril/rust that referenced this issue Sep 3, 2019
…bank

librustc_errors: Extract sugg/subst handling into method

An initial refactoring before working on rust-lang#61809.

This moves the whole block into a method so that it can be reused in the
annotate-snippet emitter. The method is already used in the new emitter, but
there's no UI tests with suggestions included in this PR.

A first look at some UI tests with suggestions showed that there's some
more work to do in [annotate-snippet-rs][annotate-snippet-rs] before the new output is closer to the
current one, so I opted to do that in a second step.

r? @estebank

[annotate-snippet-rs]: https://github.com/rust-lang/annotate-snippets-rs
Centril added a commit to Centril/rust that referenced this issue Sep 3, 2019
…bank

librustc_errors: Extract sugg/subst handling into method

An initial refactoring before working on rust-lang#61809.

This moves the whole block into a method so that it can be reused in the
annotate-snippet emitter. The method is already used in the new emitter, but
there's no UI tests with suggestions included in this PR.

A first look at some UI tests with suggestions showed that there's some
more work to do in [annotate-snippet-rs][annotate-snippet-rs] before the new output is closer to the
current one, so I opted to do that in a second step.

r? @estebank

[annotate-snippet-rs]: https://github.com/rust-lang/annotate-snippets-rs
@crlf0710 crlf0710 added C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jun 11, 2020
@dtolnay dtolnay assigned phansch and unassigned rustbot Jan 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints C-enhancement Category: An issue proposing an enhancement or a PR with one. E-help-wanted Call for participation: Help is requested to fix this issue. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

3 participants