Skip to content

Tweak diagnostic for use suggestion to blank text surrounding span. #90941

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

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions compiler/rustc_errors/src/diagnostic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -350,6 +350,7 @@ impl Diagnostic {
msg: msg.to_owned(),
style,
applicability,
transcription: Default::default(),
tool_metadata: Default::default(),
});
self
Expand Down Expand Up @@ -378,6 +379,7 @@ impl Diagnostic {
msg: msg.to_owned(),
style: SuggestionStyle::CompletelyHidden,
applicability,
transcription: Default::default(),
tool_metadata: Default::default(),
});
self
Expand Down Expand Up @@ -433,6 +435,7 @@ impl Diagnostic {
msg: msg.to_owned(),
style,
applicability,
transcription: Default::default(),
tool_metadata: Default::default(),
});
self
Expand Down Expand Up @@ -476,6 +479,7 @@ impl Diagnostic {
msg: msg.to_owned(),
style: SuggestionStyle::ShowCode,
applicability,
transcription: Default::default(),
tool_metadata: Default::default(),
});
self
Expand All @@ -501,6 +505,7 @@ impl Diagnostic {
msg: msg.to_owned(),
style: SuggestionStyle::ShowCode,
applicability,
transcription: Default::default(),
tool_metadata: Default::default(),
});
self
Expand Down Expand Up @@ -583,6 +588,7 @@ impl Diagnostic {
msg: msg.to_owned(),
style: SuggestionStyle::CompletelyHidden,
applicability,
transcription: Default::default(),
tool_metadata: ToolMetadata::new(tool_metadata),
})
}
Expand Down
45 changes: 40 additions & 5 deletions compiler/rustc_errors/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ pub use rustc_lint_defs::{pluralize, Applicability};
use rustc_serialize::json::Json;
use rustc_serialize::{Decodable, Decoder, Encodable, Encoder};
use rustc_span::source_map::SourceMap;
use rustc_span::{Loc, MultiSpan, Span};
use rustc_span::{Loc, MultiSpan, SourceFile, Span};

use std::borrow::Cow;
use std::hash::{Hash, Hasher};
Expand Down Expand Up @@ -142,6 +142,10 @@ pub struct CodeSuggestion {
pub msg: String,
/// Visual representation of this suggestion.
pub style: SuggestionStyle,
/// Role of context of spans. Usually the context around every
/// span should be included in the output. But sometimes the
/// suggestion stands entirely on its own.
pub transcription: Transcription,
/// Whether or not the suggestion is approximate
///
/// Sometimes we may show suggestions with placeholders,
Expand All @@ -152,6 +156,24 @@ pub struct CodeSuggestion {
pub tool_metadata: ToolMetadata,
}

#[derive(Clone, Debug, PartialEq, Hash, Encodable, Decodable)]
pub enum Transcription {
/// The characters in the line(s) around each span should be
/// included in the output.
Copy,

/// The span is solely providing the indentation and lexical
/// context; the source code contents of the surrounding code is
/// irrelevant and should not be included in the suggestion.
Blank,
}

impl Default for Transcription {
fn default() -> Self {
Transcription::Copy
}
}

#[derive(Clone, Debug, PartialEq, Hash, Encodable, Decodable)]
/// See the docs on `CodeSuggestion::substitutions`
pub struct Substitution {
Expand Down Expand Up @@ -193,6 +215,19 @@ impl SubstitutionPart {
}

impl CodeSuggestion {
fn get_line<'a>(&self, sf: &'a SourceFile, line_number: usize) -> Option<Cow<'a, str>> {
self.transcribe(sf.get_line(line_number))
}

fn transcribe<'a>(&self, text: Option<Cow<'a, str>>) -> Option<Cow<'a, str>> {
match self.transcription {
Transcription::Copy => text,
Transcription::Blank => text.map(|s| {
s.chars().map(|c| if c.is_whitespace() { c } else { ' ' }).collect()
Copy link
Member

Choose a reason for hiding this comment

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

If the goal is to preserve indentation, I think removing any non-whitespace characters may be better? For example, in the async fn case, you probably want use aligned with async, not indented to the fn.

Copy link
Member Author

@pnkfelix pnkfelix Nov 16, 2021

Choose a reason for hiding this comment

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

Well, then trailing ... */ comments (which would likewise get removed under your suggestion) end up messing up the indentation?

And I guess if the item is on the same line as its containing module, we might likewise have a problem:

mod m { fn foo() { Command::new("git"); } }

But I freely admit, both of these cases sound much more rare than #[tokio::main]. :)

Copy link
Member Author

Choose a reason for hiding this comment

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

(I think I'm gradually talking myself into going back and looking at deriving the span from the span of the containing mod/crate again, despite my earlier frustrations with that approach.)

}),
}
}

/// Returns the assembled code suggestions, whether they should be shown with an underline
/// and whether the substitution only differs in capitalization.
pub fn splice_lines(
Expand Down Expand Up @@ -283,7 +318,7 @@ impl CodeSuggestion {
let mut prev_hi = sm.lookup_char_pos(bounding_span.lo());
prev_hi.col = CharPos::from_usize(0);
let mut prev_line =
lines.lines.get(0).and_then(|line0| sf.get_line(line0.line_index));
lines.lines.get(0).and_then(|line0| self.get_line(sf, line0.line_index));
let mut buf = String::new();

let mut line_highlight = vec![];
Expand All @@ -310,13 +345,13 @@ impl CodeSuggestion {
}
// push lines between the previous and current span (if any)
for idx in prev_hi.line..(cur_lo.line - 1) {
if let Some(line) = sf.get_line(idx) {
if let Some(line) = self.get_line(sf, idx) {
buf.push_str(line.as_ref());
buf.push('\n');
highlights.push(std::mem::take(&mut line_highlight));
}
}
if let Some(cur_line) = sf.get_line(cur_lo.line - 1) {
if let Some(cur_line) = self.get_line(sf, cur_lo.line - 1) {
let end = match cur_line.char_indices().nth(cur_lo.col.to_usize()) {
Some((i, _)) => i,
None => cur_line.len(),
Expand Down Expand Up @@ -349,7 +384,7 @@ impl CodeSuggestion {
acc += len as isize - (cur_hi.col.0 - cur_lo.col.0) as isize;
}
prev_hi = cur_hi;
prev_line = sf.get_line(prev_hi.line - 1);
prev_line = self.get_line(sf, prev_hi.line - 1);
for line in part.snippet.split('\n').skip(1) {
acc = 0;
highlights.push(std::mem::take(&mut line_highlight));
Expand Down
7 changes: 6 additions & 1 deletion compiler/rustc_resolve/src/diagnostics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use std::ptr;
use rustc_ast::{self as ast, Path};
use rustc_ast_pretty::pprust;
use rustc_data_structures::fx::FxHashSet;
use rustc_errors::{struct_span_err, Applicability, DiagnosticBuilder};
use rustc_errors::{struct_span_err, Applicability, DiagnosticBuilder, Transcription};
use rustc_feature::BUILTIN_ATTRIBUTES;
use rustc_hir::def::Namespace::{self, *};
use rustc_hir::def::{self, CtorKind, CtorOf, DefKind, NonMacroAttrKind};
Expand Down Expand Up @@ -1843,6 +1843,11 @@ crate fn show_candidates(
accessible_path_strings.into_iter().map(|a| a.0),
Applicability::Unspecified,
);
let last_idx = err.suggestions.len() - 1;
// rust#87613: for `use` suggestions, transcribed source
// code can yield incorrect suggestions. Instead, use
// source span solely to establish line number and indent.
err.suggestions[last_idx].transcription = Transcription::Blank;
} else {
msg.push(':');

Expand Down
63 changes: 63 additions & 0 deletions src/test/ui/proc-macro/amputate-span.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
// aux-build:amputate-span.rs
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we make it run-rustfix? That way we enforce that the emitted suggestion is actually valid.

Copy link
Member Author

@pnkfelix pnkfelix Nov 16, 2021

Choose a reason for hiding this comment

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

Adding // run-rustfix to this test does not work out of the box. It ends up rewriting the code to the bad thing that we are trying to prevent:

#[amputate_span::drop_first_token]
/* what the
hey */ async use std::process::Command;

fn main() {
    Command::new("git"); //~ ERROR [E0433]
}

and

#[amputate_span::drop_first_token]
/* what the
hey */ async use std::process::Command;

fn main() {
    Command::new("git"); //~ ERROR [E0433]
}

I'm guessing that's because rustfix does not know Transcription::Blanks?

Copy link
Member Author

@pnkfelix pnkfelix Nov 16, 2021

Choose a reason for hiding this comment

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

This may be somewhat related to this FIXME from PR #85427 that originally stems from PR #44215. Fixing that might serve as an argument for moving away from the "infer the span from the given item's span" strategy, and using the span of the containing mod/crate instead, as discussed in my other comment

// FIXME: UsePlacementFinder is broken because active attributes are
// removed, and thus the `derive` attribute here is not in the AST.
// An inert attribute should work, though.
// #[derive(Debug)]
use std::path::Path;
#[allow(warnings)]
pub struct Foo;

// edition:2018
// compile-flags: --extern amputate_span

// This test has been crafted to ensure the following things:
//
// 1. There's a resolution error that prompts the compiler to suggest
// adding a `use` item.
//
// 2. There are no `use` or `extern crate` items in the source
// code. In fact, there is only one item, the `fn main`
// declaration.
//
// 3. The single `fn main` declaration has an attribute attached to it
// that just deletes the first token from the given item.
//
// You need all of these conditions to hold in order to replicate the
// scenario that yielded issue 87613, where the compiler's suggestion
// looks like:
//
// ```
// help: consider importing this struct
// |
// 47 | hey */ async use std::process::Command;
// | ++++++++++++++++++++++++++
// ```
//
// The first condition is necessary to force the compiler issue a
// suggestion. The second condition is necessary to force the
// suggestion to be issued at a span associated with the sole
// `fn`-item of this crate. The third condition is necessary in order
// to yield the weird state where the associated span of the `fn`-item
// does not actually cover all of the original source code of the
// `fn`-item (which is why we are calling it an "amputated" span
// here).
//
// Note that satisfying conditions 2 and 3 requires the use of the
// `--extern` compile flag.
//
// You might ask yourself: What code would do such a thing? The
// answer is: the #[tokio::main] attribute does *exactly* this (as
// well as injecting some other code into the `fn main` that it
// constructs).

#[amputate_span::drop_first_token]
/* what the
hey */ async fn main() {
Command::new("git"); //~ ERROR [E0433]
}

// (The /* ... */ comment in the above is not part of the original
// bug. It is just meant to illustrate one particular facet of the
// original non-ideal behavior, where we were transcribing the
// trailing comment as part of the emitted suggestion, for better or
// for worse.)

mod inner {
#[amputate_span::drop_first_token]
/* another interesting
case */ async fn foo() {
Command::new("git"); //~ ERROR [E0433]
}
}
25 changes: 25 additions & 0 deletions src/test/ui/proc-macro/amputate-span.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
error[E0433]: failed to resolve: use of undeclared type `Command`
--> $DIR/amputate-span.rs:48:5
|
LL | Command::new("git");
| ^^^^^^^ not found in this scope
|
help: consider importing this struct
|
LL | use std::process::Command;
|

error[E0433]: failed to resolve: use of undeclared type `Command`
--> $DIR/amputate-span.rs:61:2
|
LL | Command::new("git");
| ^^^^^^^ not found in this scope
|
help: consider importing this struct
|
LL | use std::process::Command;
|

error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0433`.
14 changes: 14 additions & 0 deletions src/test/ui/proc-macro/auxiliary/amputate-span.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
// force-host
// no-prefer-dynamic

#![crate_type = "proc-macro"]

extern crate proc_macro;

use proc_macro::TokenStream;

#[proc_macro_attribute]
pub fn drop_first_token(attr: TokenStream, input: TokenStream) -> TokenStream {
assert!(attr.is_empty());
input.into_iter().skip(1).collect()
}