From 105516220fdf1f5c8e55822ed4e6ada22959342c Mon Sep 17 00:00:00 2001 From: Felix Klock Date: Tue, 16 Nov 2021 04:25:13 +0000 Subject: [PATCH] Tweak diagnostic for `use` suggestion to blank text surrounding span. Our suggestion system is oriented around diff's to code, and in that context, if your spans are perfect, everything works out. But our spans are not always perfect, due to macros. In the specific case of issue 87613, the #[tokio::main] attribute macro works by rewriting an `async fn main` into an `fn main`, by just removing the `async` token. The problem is that the `async` text remains in the source code, and the span of the code rewritten by suggestion system happily transcribes all the text on the line of that `fn main` when generating a `use` suggestion, yielding the absurd looking `async use std::process::Command;` suggestion. This patch works around this whole problem by adding a way for suggestions to indicate that their transcriptions should not include the original source code; just its *whitespace*. This leads to a happy place where the suggested line numbers are correct and the indentation is usually also correct, while avoiding the `async use` output we would see before. --- compiler/rustc_errors/src/diagnostic.rs | 6 ++ compiler/rustc_errors/src/lib.rs | 45 +++++++++++-- compiler/rustc_resolve/src/diagnostics.rs | 7 ++- src/test/ui/proc-macro/amputate-span.rs | 63 +++++++++++++++++++ src/test/ui/proc-macro/amputate-span.stderr | 25 ++++++++ .../ui/proc-macro/auxiliary/amputate-span.rs | 14 +++++ 6 files changed, 154 insertions(+), 6 deletions(-) create mode 100644 src/test/ui/proc-macro/amputate-span.rs create mode 100644 src/test/ui/proc-macro/amputate-span.stderr create mode 100644 src/test/ui/proc-macro/auxiliary/amputate-span.rs diff --git a/compiler/rustc_errors/src/diagnostic.rs b/compiler/rustc_errors/src/diagnostic.rs index f2381d75c565f..2f272730647cd 100644 --- a/compiler/rustc_errors/src/diagnostic.rs +++ b/compiler/rustc_errors/src/diagnostic.rs @@ -350,6 +350,7 @@ impl Diagnostic { msg: msg.to_owned(), style, applicability, + transcription: Default::default(), tool_metadata: Default::default(), }); self @@ -378,6 +379,7 @@ impl Diagnostic { msg: msg.to_owned(), style: SuggestionStyle::CompletelyHidden, applicability, + transcription: Default::default(), tool_metadata: Default::default(), }); self @@ -433,6 +435,7 @@ impl Diagnostic { msg: msg.to_owned(), style, applicability, + transcription: Default::default(), tool_metadata: Default::default(), }); self @@ -476,6 +479,7 @@ impl Diagnostic { msg: msg.to_owned(), style: SuggestionStyle::ShowCode, applicability, + transcription: Default::default(), tool_metadata: Default::default(), }); self @@ -501,6 +505,7 @@ impl Diagnostic { msg: msg.to_owned(), style: SuggestionStyle::ShowCode, applicability, + transcription: Default::default(), tool_metadata: Default::default(), }); self @@ -583,6 +588,7 @@ impl Diagnostic { msg: msg.to_owned(), style: SuggestionStyle::CompletelyHidden, applicability, + transcription: Default::default(), tool_metadata: ToolMetadata::new(tool_metadata), }) } diff --git a/compiler/rustc_errors/src/lib.rs b/compiler/rustc_errors/src/lib.rs index b6cf332f511ec..c56a1ad91d225 100644 --- a/compiler/rustc_errors/src/lib.rs +++ b/compiler/rustc_errors/src/lib.rs @@ -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}; @@ -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, @@ -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 { @@ -193,6 +215,19 @@ impl SubstitutionPart { } impl CodeSuggestion { + fn get_line<'a>(&self, sf: &'a SourceFile, line_number: usize) -> Option> { + self.transcribe(sf.get_line(line_number)) + } + + fn transcribe<'a>(&self, text: Option>) -> Option> { + match self.transcription { + Transcription::Copy => text, + Transcription::Blank => text.map(|s| { + s.chars().map(|c| if c.is_whitespace() { c } else { ' ' }).collect() + }), + } + } + /// 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( @@ -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![]; @@ -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(), @@ -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)); diff --git a/compiler/rustc_resolve/src/diagnostics.rs b/compiler/rustc_resolve/src/diagnostics.rs index 163acebcceacf..565f0c6150477 100644 --- a/compiler/rustc_resolve/src/diagnostics.rs +++ b/compiler/rustc_resolve/src/diagnostics.rs @@ -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}; @@ -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(':'); diff --git a/src/test/ui/proc-macro/amputate-span.rs b/src/test/ui/proc-macro/amputate-span.rs new file mode 100644 index 0000000000000..70ba50e598c0b --- /dev/null +++ b/src/test/ui/proc-macro/amputate-span.rs @@ -0,0 +1,63 @@ +// aux-build:amputate-span.rs +// 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] + } +} diff --git a/src/test/ui/proc-macro/amputate-span.stderr b/src/test/ui/proc-macro/amputate-span.stderr new file mode 100644 index 0000000000000..43e9d7fb5267c --- /dev/null +++ b/src/test/ui/proc-macro/amputate-span.stderr @@ -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`. diff --git a/src/test/ui/proc-macro/auxiliary/amputate-span.rs b/src/test/ui/proc-macro/auxiliary/amputate-span.rs new file mode 100644 index 0000000000000..1a82119ae95e5 --- /dev/null +++ b/src/test/ui/proc-macro/auxiliary/amputate-span.rs @@ -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() +}