From 67d762d896c8748009d1843ebf9e2e0760ed33a0 Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Tue, 9 May 2017 10:04:24 +0200 Subject: [PATCH 1/7] Refactor suggestion diagnostic API to allow for multiple suggestions --- src/librustc_errors/diagnostic.rs | 18 +++-- src/librustc_errors/diagnostic_builder.rs | 5 ++ src/librustc_errors/emitter.rs | 73 +++++++++++-------- src/librustc_errors/lib.rs | 85 ++++++++++++++--------- src/libsyntax/json.rs | 68 +++++++++--------- 5 files changed, 148 insertions(+), 101 deletions(-) diff --git a/src/librustc_errors/diagnostic.rs b/src/librustc_errors/diagnostic.rs index 0822f713499..e129e313626 100644 --- a/src/librustc_errors/diagnostic.rs +++ b/src/librustc_errors/diagnostic.rs @@ -23,7 +23,7 @@ pub struct Diagnostic { pub code: Option, pub span: MultiSpan, pub children: Vec, - pub suggestion: Option, + pub suggestions: Vec, } /// For example a note attached to an error. @@ -87,7 +87,7 @@ impl Diagnostic { code: code, span: MultiSpan::new(), children: vec![], - suggestion: None, + suggestions: vec![], } } @@ -204,10 +204,16 @@ impl Diagnostic { /// /// See `diagnostic::CodeSuggestion` for more information. pub fn span_suggestion(&mut self, sp: Span, msg: &str, suggestion: String) -> &mut Self { - assert!(self.suggestion.is_none()); - self.suggestion = Some(CodeSuggestion { - msp: sp.into(), - substitutes: vec![suggestion], + self.suggestions.push(CodeSuggestion { + substitutes: vec![(sp, vec![suggestion])], + msg: msg.to_owned(), + }); + self + } + + pub fn span_suggestions(&mut self, sp: Span, msg: &str, suggestions: Vec) -> &mut Self { + self.suggestions.push(CodeSuggestion { + substitutes: vec![(sp, suggestions)], msg: msg.to_owned(), }); self diff --git a/src/librustc_errors/diagnostic_builder.rs b/src/librustc_errors/diagnostic_builder.rs index a9c2bbeba2a..d03a4acb9fc 100644 --- a/src/librustc_errors/diagnostic_builder.rs +++ b/src/librustc_errors/diagnostic_builder.rs @@ -148,6 +148,11 @@ impl<'a> DiagnosticBuilder<'a> { msg: &str, suggestion: String) -> &mut Self); + forward!(pub fn span_suggestions(&mut self, + sp: Span, + msg: &str, + suggestions: Vec) + -> &mut Self); forward!(pub fn set_span>(&mut self, sp: S) -> &mut Self); forward!(pub fn code(&mut self, s: String) -> &mut Self); diff --git a/src/librustc_errors/emitter.rs b/src/librustc_errors/emitter.rs index 53999eb9138..564c472305c 100644 --- a/src/librustc_errors/emitter.rs +++ b/src/librustc_errors/emitter.rs @@ -35,22 +35,37 @@ impl Emitter for EmitterWriter { let mut primary_span = db.span.clone(); let mut children = db.children.clone(); - if let Some(sugg) = db.suggestion.clone() { - assert_eq!(sugg.msp.primary_spans().len(), sugg.substitutes.len()); + if db.suggestions.len() == 1 { + let sugg = &db.suggestions[0]; // don't display multispans as labels if sugg.substitutes.len() == 1 && + // don't display multi-suggestions as labels + sugg.substitutes[0].1.len() == 1 && // don't display long messages as labels sugg.msg.split_whitespace().count() < 10 && // don't display multiline suggestions as labels - sugg.substitutes[0].find('\n').is_none() { - let msg = format!("help: {} `{}`", sugg.msg, sugg.substitutes[0]); - primary_span.push_span_label(sugg.msp.primary_spans()[0], msg); + sugg.substitutes[0].1[0].find('\n').is_none() { + let msg = format!("help: {} `{}`", sugg.msg, sugg.substitutes[0].1[0]); + primary_span.push_span_label(sugg.substitutes[0].0, msg); } else { children.push(SubDiagnostic { level: Level::Help, message: Vec::new(), span: MultiSpan::new(), - render_span: Some(Suggestion(sugg)), + render_span: Some(Suggestion(sugg.clone())), + }); + } + } 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 + for sugg in &db.suggestions { + children.push(SubDiagnostic { + level: Level::Help, + message: Vec::new(), + span: MultiSpan::new(), + render_span: Some(Suggestion(sugg.clone())), }); } } @@ -1054,38 +1069,38 @@ impl EmitterWriter { -> io::Result<()> { use std::borrow::Borrow; - let primary_span = suggestion.msp.primary_span().unwrap(); + let primary_span = suggestion.substitutes[0].0; if let Some(ref cm) = self.cm { let mut buffer = StyledBuffer::new(); - buffer.append(0, &level.to_string(), Style::Level(level.clone())); - buffer.append(0, ": ", Style::HeaderMsg); - self.msg_to_buffer(&mut buffer, - &[(suggestion.msg.to_owned(), Style::NoStyle)], - max_line_num_len, - "suggestion", - Some(Style::HeaderMsg)); - let lines = cm.span_to_lines(primary_span).unwrap(); assert!(!lines.lines.is_empty()); - let complete = suggestion.splice_lines(cm.borrow()); + for complete in suggestion.splice_lines(cm.borrow()) { + buffer.append(0, &level.to_string(), Style::Level(level.clone())); + buffer.append(0, ": ", Style::HeaderMsg); + self.msg_to_buffer(&mut buffer, + &[(suggestion.msg.to_owned(), Style::NoStyle)], + max_line_num_len, + "suggestion", + Some(Style::HeaderMsg)); - // print the suggestion without any line numbers, but leave - // space for them. This helps with lining up with previous - // snippets from the actual error being reported. - let mut lines = complete.lines(); - let mut row_num = 1; - for line in lines.by_ref().take(MAX_HIGHLIGHT_LINES) { - draw_col_separator(&mut buffer, row_num, max_line_num_len + 1); - buffer.append(row_num, line, Style::NoStyle); - row_num += 1; - } + // print the suggestion without any line numbers, but leave + // space for them. This helps with lining up with previous + // snippets from the actual error being reported. + let mut lines = complete.lines(); + let mut row_num = 1; + for line in lines.by_ref().take(MAX_HIGHLIGHT_LINES) { + draw_col_separator(&mut buffer, row_num, max_line_num_len + 1); + buffer.append(row_num, line, Style::NoStyle); + row_num += 1; + } - // if we elided some lines, add an ellipsis - if let Some(_) = lines.next() { - buffer.append(row_num, "...", Style::NoStyle); + // if we elided some lines, add an ellipsis + if let Some(_) = lines.next() { + buffer.append(row_num, "...", Style::NoStyle); + } } emit_to_destination(&buffer.render(), level, &mut self.dst)?; } diff --git a/src/librustc_errors/lib.rs b/src/librustc_errors/lib.rs index db8c9ac306b..8e378935094 100644 --- a/src/librustc_errors/lib.rs +++ b/src/librustc_errors/lib.rs @@ -65,8 +65,25 @@ pub enum RenderSpan { #[derive(Clone, Debug, PartialEq, RustcEncodable, RustcDecodable)] pub struct CodeSuggestion { - pub msp: MultiSpan, - pub substitutes: Vec, + /// Each substitute can have multiple variants due to multiple + /// applicable suggestions + /// + /// `foo.bar` might be replaced with `a.b` or `x.y` by replacing + /// `foo` and `bar` on their own: + /// + /// ``` + /// vec![ + /// (0..3, vec!["a", "x"]), + /// (4..7, vec!["b", "y"]), + /// ] + /// ``` + /// + /// or by replacing the entire span: + /// + /// ``` + /// vec![(0..7, vec!["a.b", "x.y"])] + /// ``` + pub substitutes: Vec<(Span, Vec)>, pub msg: String, } @@ -79,8 +96,8 @@ pub trait CodeMapper { } impl CodeSuggestion { - /// Returns the assembled code suggestion. - pub fn splice_lines(&self, cm: &CodeMapper) -> String { + /// Returns the assembled code suggestions. + pub fn splice_lines(&self, cm: &CodeMapper) -> Vec { use syntax_pos::{CharPos, Loc, Pos}; fn push_trailing(buf: &mut String, @@ -102,20 +119,22 @@ impl CodeSuggestion { } } - let mut primary_spans = self.msp.primary_spans().to_owned(); - - assert_eq!(primary_spans.len(), self.substitutes.len()); - if primary_spans.is_empty() { - return format!(""); + if self.substitutes.is_empty() { + return vec![String::new()]; } + let mut primary_spans: Vec<_> = self.substitutes + .iter() + .map(|&(sp, ref sub)| (sp, sub)) + .collect(); + // Assumption: all spans are in the same file, and all spans // are disjoint. Sort in ascending order. - primary_spans.sort_by_key(|sp| sp.lo); + primary_spans.sort_by_key(|sp| sp.0.lo); // Find the bounding span. - let lo = primary_spans.iter().map(|sp| sp.lo).min().unwrap(); - let hi = primary_spans.iter().map(|sp| sp.hi).min().unwrap(); + let lo = primary_spans.iter().map(|sp| sp.0.lo).min().unwrap(); + let hi = primary_spans.iter().map(|sp| sp.0.hi).min().unwrap(); let bounding_span = Span { lo: lo, hi: hi, @@ -138,33 +157,37 @@ impl CodeSuggestion { prev_hi.col = CharPos::from_usize(0); let mut prev_line = fm.get_line(lines.lines[0].line_index); - let mut buf = String::new(); + let mut bufs = vec![String::new(); self.substitutes[0].1.len()]; - for (sp, substitute) in primary_spans.iter().zip(self.substitutes.iter()) { + for (sp, substitutes) in primary_spans { let cur_lo = cm.lookup_char_pos(sp.lo); - if prev_hi.line == cur_lo.line { - push_trailing(&mut buf, prev_line, &prev_hi, Some(&cur_lo)); - } else { - push_trailing(&mut buf, prev_line, &prev_hi, None); - // push lines between the previous and current span (if any) - for idx in prev_hi.line..(cur_lo.line - 1) { - if let Some(line) = fm.get_line(idx) { - buf.push_str(line); - buf.push('\n'); + for (buf, substitute) in bufs.iter_mut().zip(substitutes) { + if prev_hi.line == cur_lo.line { + push_trailing(buf, prev_line, &prev_hi, Some(&cur_lo)); + } else { + push_trailing(buf, prev_line, &prev_hi, None); + // push lines between the previous and current span (if any) + for idx in prev_hi.line..(cur_lo.line - 1) { + if let Some(line) = fm.get_line(idx) { + buf.push_str(line); + buf.push('\n'); + } + } + if let Some(cur_line) = fm.get_line(cur_lo.line - 1) { + buf.push_str(&cur_line[..cur_lo.col.to_usize()]); } } - if let Some(cur_line) = fm.get_line(cur_lo.line - 1) { - buf.push_str(&cur_line[..cur_lo.col.to_usize()]); - } + buf.push_str(substitute); } - buf.push_str(substitute); prev_hi = cm.lookup_char_pos(sp.hi); prev_line = fm.get_line(prev_hi.line - 1); } - push_trailing(&mut buf, prev_line, &prev_hi, None); - // remove trailing newline - buf.pop(); - buf + for buf in &mut bufs { + push_trailing(buf, prev_line, &prev_hi, None); + // remove trailing newline + buf.pop(); + } + bufs } } diff --git a/src/libsyntax/json.rs b/src/libsyntax/json.rs index 0271ddbccbf..3d0b0b228a8 100644 --- a/src/libsyntax/json.rs +++ b/src/libsyntax/json.rs @@ -22,9 +22,8 @@ use codemap::{CodeMap, FilePathMapping}; use syntax_pos::{self, MacroBacktrace, Span, SpanLabel, MultiSpan}; use errors::registry::Registry; -use errors::{Level, DiagnosticBuilder, SubDiagnostic, RenderSpan, CodeSuggestion, CodeMapper}; +use errors::{DiagnosticBuilder, SubDiagnostic, RenderSpan, CodeSuggestion, CodeMapper}; use errors::emitter::Emitter; -use errors::snippet::Style; use std::rc::Rc; use std::io::{self, Write}; @@ -154,23 +153,26 @@ impl Diagnostic { fn from_diagnostic_builder(db: &DiagnosticBuilder, je: &JsonEmitter) -> Diagnostic { - let sugg = db.suggestion.as_ref().map(|sugg| { - SubDiagnostic { - level: Level::Help, - message: vec![(sugg.msg.clone(), Style::NoStyle)], - span: MultiSpan::new(), - render_span: Some(RenderSpan::Suggestion(sugg.clone())), - } + let sugg = db.suggestions.iter().flat_map(|sugg| { + je.render(sugg).into_iter().map(move |rendered| { + Diagnostic { + message: sugg.msg.clone(), + code: None, + level: "help", + spans: DiagnosticSpan::from_suggestion(sugg, je), + children: vec![], + rendered: Some(rendered), + } + }) }); - let sugg = sugg.as_ref(); Diagnostic { message: db.message(), code: DiagnosticCode::map_opt_string(db.code.clone(), je), level: db.level.to_str(), spans: DiagnosticSpan::from_multispan(&db.span, je), - children: db.children.iter().chain(sugg).map(|c| { + children: db.children.iter().map(|c| { Diagnostic::from_sub_diagnostic(c, je) - }).collect(), + }).chain(sugg).collect(), rendered: None, } } @@ -184,8 +186,7 @@ impl Diagnostic { .map(|sp| DiagnosticSpan::from_render_span(sp, je)) .unwrap_or_else(|| DiagnosticSpan::from_multispan(&db.span, je)), children: vec![], - rendered: db.render_span.as_ref() - .and_then(|rsp| je.render(rsp)), + rendered: None, } } } @@ -278,14 +279,19 @@ impl DiagnosticSpan { fn from_suggestion(suggestion: &CodeSuggestion, je: &JsonEmitter) -> Vec { - assert_eq!(suggestion.msp.span_labels().len(), suggestion.substitutes.len()); - suggestion.msp.span_labels() - .into_iter() - .zip(&suggestion.substitutes) - .map(|(span_label, suggestion)| { - DiagnosticSpan::from_span_label(span_label, - Some(suggestion), - je) + suggestion.substitutes + .iter() + .flat_map(|&(span, ref suggestion)| { + suggestion.iter().map(move |suggestion| { + let span_label = SpanLabel { + span, + is_primary: true, + label: None, + }; + DiagnosticSpan::from_span_label(span_label, + Some(suggestion), + je) + }) }) .collect() } @@ -294,8 +300,9 @@ impl DiagnosticSpan { match *rsp { RenderSpan::FullSpan(ref msp) => DiagnosticSpan::from_multispan(msp, je), - RenderSpan::Suggestion(ref suggestion) => - DiagnosticSpan::from_suggestion(suggestion, je), + // regular diagnostics don't produce this anymore + // will be removed in a later commit + RenderSpan::Suggestion(_) => unreachable!(), } } } @@ -351,17 +358,8 @@ impl DiagnosticCode { } impl JsonEmitter { - fn render(&self, render_span: &RenderSpan) -> Option { - use std::borrow::Borrow; - - match *render_span { - RenderSpan::FullSpan(_) => { - None - } - RenderSpan::Suggestion(ref suggestion) => { - Some(suggestion.splice_lines(self.cm.borrow())) - } - } + fn render(&self, suggestion: &CodeSuggestion) -> Vec { + suggestion.splice_lines(&*self.cm) } } From e2f781c7ead3a9fe69020189decc6c3eebf6f25c Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Wed, 10 May 2017 13:19:29 +0200 Subject: [PATCH 2/7] Example usage of multiple suggestions --- src/librustc_errors/emitter.rs | 28 +++++++---- src/librustc_errors/lib.rs | 5 +- src/librustc_resolve/build_reduced_graph.rs | 37 +++++++++----- src/librustc_resolve/lib.rs | 48 +++++++++---------- src/librustc_resolve/macros.rs | 6 +-- .../ui/resolve/enums-are-namespaced-xc.stderr | 12 ++--- src/test/ui/resolve/issue-16058.stderr | 8 ++-- src/test/ui/resolve/issue-17518.stderr | 4 +- src/test/ui/resolve/issue-21221-1.stderr | 24 +++++----- src/test/ui/resolve/issue-21221-2.stderr | 4 +- src/test/ui/resolve/issue-21221-3.stderr | 4 +- src/test/ui/resolve/issue-21221-4.stderr | 4 +- src/test/ui/resolve/issue-3907.stderr | 4 +- .../ui/resolve/privacy-struct-ctor.stderr | 12 ++--- src/test/ui/span/issue-35987.stderr | 4 +- 15 files changed, 113 insertions(+), 91 deletions(-) diff --git a/src/librustc_errors/emitter.rs b/src/librustc_errors/emitter.rs index 564c472305c..cd72941146c 100644 --- a/src/librustc_errors/emitter.rs +++ b/src/librustc_errors/emitter.rs @@ -81,6 +81,10 @@ impl Emitter for EmitterWriter { /// maximum number of lines we will print for each error; arbitrary. pub const MAX_HIGHLIGHT_LINES: usize = 6; +/// maximum number of suggestions to be shown +/// +/// Arbitrary, but taken from trait import suggestion limit +pub const MAX_SUGGESTIONS: usize = 4; #[derive(Clone, Copy, Debug, PartialEq, Eq)] pub enum ColorConfig { @@ -1077,20 +1081,22 @@ impl EmitterWriter { assert!(!lines.lines.is_empty()); - for complete in suggestion.splice_lines(cm.borrow()) { - buffer.append(0, &level.to_string(), Style::Level(level.clone())); - buffer.append(0, ": ", Style::HeaderMsg); - self.msg_to_buffer(&mut buffer, - &[(suggestion.msg.to_owned(), Style::NoStyle)], - max_line_num_len, - "suggestion", - Some(Style::HeaderMsg)); + buffer.append(0, &level.to_string(), Style::Level(level.clone())); + buffer.append(0, ": ", Style::HeaderMsg); + self.msg_to_buffer(&mut buffer, + &[(suggestion.msg.to_owned(), Style::NoStyle)], + max_line_num_len, + "suggestion", + Some(Style::HeaderMsg)); + + let suggestions = suggestion.splice_lines(cm.borrow()); + let mut row_num = 1; + for complete in suggestions.iter().take(MAX_SUGGESTIONS) { // print the suggestion without any line numbers, but leave // space for them. This helps with lining up with previous // snippets from the actual error being reported. let mut lines = complete.lines(); - let mut row_num = 1; for line in lines.by_ref().take(MAX_HIGHLIGHT_LINES) { draw_col_separator(&mut buffer, row_num, max_line_num_len + 1); buffer.append(row_num, line, Style::NoStyle); @@ -1102,6 +1108,10 @@ impl EmitterWriter { buffer.append(row_num, "...", Style::NoStyle); } } + if suggestions.len() > MAX_SUGGESTIONS { + let msg = format!("and {} other candidates", suggestions.len() - MAX_SUGGESTIONS); + buffer.append(row_num, &msg, Style::NoStyle); + } emit_to_destination(&buffer.render(), level, &mut self.dst)?; } Ok(()) diff --git a/src/librustc_errors/lib.rs b/src/librustc_errors/lib.rs index 8e378935094..82d688d6ba6 100644 --- a/src/librustc_errors/lib.rs +++ b/src/librustc_errors/lib.rs @@ -183,7 +183,10 @@ impl CodeSuggestion { prev_line = fm.get_line(prev_hi.line - 1); } for buf in &mut bufs { - push_trailing(buf, prev_line, &prev_hi, None); + // if the replacement already ends with a newline, don't print the next line + if !buf.ends_with('\n') { + push_trailing(buf, prev_line, &prev_hi, None); + } // remove trailing newline buf.pop(); } diff --git a/src/librustc_resolve/build_reduced_graph.rs b/src/librustc_resolve/build_reduced_graph.rs index c797c151de6..d1f0cdedde8 100644 --- a/src/librustc_resolve/build_reduced_graph.rs +++ b/src/librustc_resolve/build_reduced_graph.rs @@ -150,7 +150,7 @@ impl<'a> Resolver<'a> { view_path.span, ResolutionError::SelfImportsOnlyAllowedWithin); } else if source_name == "$crate" && full_path.segments.len() == 1 { - let crate_root = self.resolve_crate_var(source.ctxt); + let crate_root = self.resolve_crate_var(source.ctxt, item.span); let crate_name = match crate_root.kind { ModuleKind::Def(_, name) => name, ModuleKind::Block(..) => unreachable!(), @@ -247,7 +247,7 @@ impl<'a> Resolver<'a> { // n.b. we don't need to look at the path option here, because cstore already did let crate_id = self.session.cstore.extern_mod_stmt_cnum(item.id).unwrap(); - let module = self.get_extern_crate_root(crate_id); + let module = self.get_extern_crate_root(crate_id, item.span); self.populate_module_if_necessary(module); let used = self.process_legacy_macro_imports(item, module, expansion); let binding = @@ -279,7 +279,7 @@ impl<'a> Resolver<'a> { no_implicit_prelude: parent.no_implicit_prelude || { attr::contains_name(&item.attrs, "no_implicit_prelude") }, - ..ModuleData::new(Some(parent), module_kind, def_id) + ..ModuleData::new(Some(parent), module_kind, def_id, item.span) }); self.define(parent, ident, TypeNS, (module, vis, sp, expansion)); self.module_map.insert(def_id, module); @@ -314,7 +314,10 @@ impl<'a> Resolver<'a> { ItemKind::Enum(ref enum_definition, _) => { let def = Def::Enum(self.definitions.local_def_id(item.id)); let module_kind = ModuleKind::Def(def, ident.name); - let module = self.new_module(parent, module_kind, parent.normal_ancestor_id); + let module = self.new_module(parent, + module_kind, + parent.normal_ancestor_id, + item.span); self.define(parent, ident, TypeNS, (module, vis, sp, expansion)); for variant in &(*enum_definition).variants { @@ -370,7 +373,10 @@ impl<'a> Resolver<'a> { // Add all the items within to a new module. let module_kind = ModuleKind::Def(Def::Trait(def_id), ident.name); - let module = self.new_module(parent, module_kind, parent.normal_ancestor_id); + let module = self.new_module(parent, + module_kind, + parent.normal_ancestor_id, + item.span); self.define(parent, ident, TypeNS, (module, vis, sp, expansion)); self.current_module = module; } @@ -419,7 +425,7 @@ impl<'a> Resolver<'a> { let parent = self.current_module; if self.block_needs_anonymous_module(block) { let module = - self.new_module(parent, ModuleKind::Block(block.id), parent.normal_ancestor_id); + self.new_module(parent, ModuleKind::Block(block.id), parent.normal_ancestor_id, block.span); self.block_map.insert(block.id, module); self.current_module = module; // Descend into the block. } @@ -431,10 +437,14 @@ impl<'a> Resolver<'a> { let def = child.def; let def_id = def.def_id(); let vis = self.session.cstore.visibility(def_id); + let span = child.span; match def { Def::Mod(..) | Def::Enum(..) => { - let module = self.new_module(parent, ModuleKind::Def(def, ident.name), def_id); + let module = self.new_module(parent, + ModuleKind::Def(def, ident.name), + def_id, + span); self.define(parent, ident, TypeNS, (module, vis, DUMMY_SP, Mark::root())); } Def::Variant(..) | Def::TyAlias(..) => { @@ -454,7 +464,10 @@ impl<'a> Resolver<'a> { } Def::Trait(..) => { let module_kind = ModuleKind::Def(def, ident.name); - let module = self.new_module(parent, module_kind, parent.normal_ancestor_id); + let module = self.new_module(parent, + module_kind, + parent.normal_ancestor_id, + span); self.define(parent, ident, TypeNS, (module, vis, DUMMY_SP, Mark::root())); for child in self.session.cstore.item_children(def_id) { @@ -483,18 +496,18 @@ impl<'a> Resolver<'a> { } } - fn get_extern_crate_root(&mut self, cnum: CrateNum) -> Module<'a> { + fn get_extern_crate_root(&mut self, cnum: CrateNum, span: Span) -> Module<'a> { let def_id = DefId { krate: cnum, index: CRATE_DEF_INDEX }; let name = self.session.cstore.crate_name(cnum); let macros_only = self.session.cstore.dep_kind(cnum).macros_only(); let module_kind = ModuleKind::Def(Def::Mod(def_id), name); let arenas = self.arenas; *self.extern_crate_roots.entry((cnum, macros_only)).or_insert_with(|| { - arenas.alloc_module(ModuleData::new(None, module_kind, def_id)) + arenas.alloc_module(ModuleData::new(None, module_kind, def_id, span)) }) } - pub fn macro_def_scope(&mut self, expansion: Mark) -> Module<'a> { + pub fn macro_def_scope(&mut self, expansion: Mark, span: Span) -> Module<'a> { let def_id = self.macro_defs[&expansion]; if let Some(id) = self.definitions.as_local_node_id(def_id) { self.local_macro_def_scopes[&id] @@ -503,7 +516,7 @@ impl<'a> Resolver<'a> { self.graph_root } else { let module_def_id = ty::DefIdTree::parent(&*self, def_id).unwrap(); - self.get_extern_crate_root(module_def_id.krate) + self.get_extern_crate_root(module_def_id.krate, span) } } diff --git a/src/librustc_resolve/lib.rs b/src/librustc_resolve/lib.rs index ac556270886..fd964c7d7d1 100644 --- a/src/librustc_resolve/lib.rs +++ b/src/librustc_resolve/lib.rs @@ -865,12 +865,15 @@ pub struct ModuleData<'a> { // access the children must be preceded with a // `populate_module_if_necessary` call. populated: Cell, + + /// Span of the module itself. Used for error reporting. + span: Span, } pub type Module<'a> = &'a ModuleData<'a>; impl<'a> ModuleData<'a> { - fn new(parent: Option>, kind: ModuleKind, normal_ancestor_id: DefId) -> Self { + fn new(parent: Option>, kind: ModuleKind, normal_ancestor_id: DefId, span: Span) -> Self { ModuleData { parent: parent, kind: kind, @@ -884,6 +887,7 @@ impl<'a> ModuleData<'a> { globs: RefCell::new((Vec::new())), traits: RefCell::new(None), populated: Cell::new(normal_ancestor_id.is_local()), + span: span, } } @@ -1298,7 +1302,7 @@ impl<'a> Resolver<'a> { let root_module_kind = ModuleKind::Def(Def::Mod(root_def_id), keywords::Invalid.name()); let graph_root = arenas.alloc_module(ModuleData { no_implicit_prelude: attr::contains_name(&krate.attrs, "no_implicit_prelude"), - ..ModuleData::new(None, root_module_kind, root_def_id) + ..ModuleData::new(None, root_module_kind, root_def_id, krate.span) }); let mut module_map = FxHashMap(); module_map.insert(DefId::local(CRATE_DEF_INDEX), graph_root); @@ -1430,9 +1434,9 @@ impl<'a> Resolver<'a> { self.crate_loader.postprocess(krate); } - fn new_module(&self, parent: Module<'a>, kind: ModuleKind, normal_ancestor_id: DefId) + fn new_module(&self, parent: Module<'a>, kind: ModuleKind, normal_ancestor_id: DefId, span: Span) -> Module<'a> { - self.arenas.alloc_module(ModuleData::new(Some(parent), kind, normal_ancestor_id)) + self.arenas.alloc_module(ModuleData::new(Some(parent), kind, normal_ancestor_id, span)) } fn record_use(&mut self, ident: Ident, ns: Namespace, binding: &'a NameBinding<'a>, span: Span) @@ -1535,12 +1539,12 @@ impl<'a> Resolver<'a> { None } - fn resolve_crate_var(&mut self, crate_var_ctxt: SyntaxContext) -> Module<'a> { + fn resolve_crate_var(&mut self, crate_var_ctxt: SyntaxContext, span: Span) -> Module<'a> { let mut ctxt_data = crate_var_ctxt.data(); while ctxt_data.prev_ctxt != SyntaxContext::empty() { ctxt_data = ctxt_data.prev_ctxt.data(); } - let module = self.macro_def_scope(ctxt_data.outer_mark); + let module = self.macro_def_scope(ctxt_data.outer_mark, span); if module.is_local() { self.graph_root } else { module } } @@ -2271,8 +2275,10 @@ impl<'a> Resolver<'a> { let name = path.last().unwrap().name; let candidates = this.lookup_import_candidates(name, ns, is_expected); if !candidates.is_empty() { + let mut module_span = this.current_module.span; + module_span.hi = module_span.lo; // Report import candidates as help and proceed searching for labels. - show_candidates(&mut err, &candidates, def.is_some()); + show_candidates(&mut err, module_span, &candidates, def.is_some()); } else if is_expected(Def::Enum(DefId::local(CRATE_DEF_INDEX))) { let enum_candidates = this.lookup_import_candidates(name, ns, is_enum_variant); let mut enum_candidates = enum_candidates.iter() @@ -2584,7 +2590,7 @@ impl<'a> Resolver<'a> { module = Some(self.graph_root); continue } else if i == 0 && ns == TypeNS && ident.name == "$crate" { - module = Some(self.resolve_crate_var(ident.ctxt)); + module = Some(self.resolve_crate_var(ident.ctxt, DUMMY_SP)); continue } @@ -3463,12 +3469,10 @@ fn import_candidate_to_paths(suggestion: &ImportSuggestion) -> (Span, String, St /// When an entity with a given name is not available in scope, we search for /// entities with that name in all crates. This method allows outputting the /// results of this search in a programmer-friendly way -fn show_candidates(session: &mut DiagnosticBuilder, +fn show_candidates(err: &mut DiagnosticBuilder, + span: Span, candidates: &[ImportSuggestion], better: bool) { - // don't show more than MAX_CANDIDATES results, so - // we're consistent with the trait suggestions - const MAX_CANDIDATES: usize = 4; // we want consistent results across executions, but candidates are produced // by iterating through a hash map, so make sure they are ordered: @@ -3481,21 +3485,13 @@ fn show_candidates(session: &mut DiagnosticBuilder, 1 => " is found in another module, you can import it", _ => "s are found in other modules, you can import them", }; + let msg = format!("possible {}candidate{} into scope", better, msg_diff); - let end = cmp::min(MAX_CANDIDATES, path_strings.len()); - session.help(&format!("possible {}candidate{} into scope:{}{}", - better, - msg_diff, - &path_strings[0..end].iter().map(|candidate| { - format!("\n `use {};`", candidate) - }).collect::(), - if path_strings.len() > MAX_CANDIDATES { - format!("\nand {} other candidates", - path_strings.len() - MAX_CANDIDATES) - } else { - "".to_owned() - } - )); + for candidate in &mut path_strings { + *candidate = format!("use {};\n", candidate); + } + + err.span_suggestions(span, &msg, path_strings); } /// A somewhat inefficient routine to obtain the name of a module. diff --git a/src/librustc_resolve/macros.rs b/src/librustc_resolve/macros.rs index 106f421f39e..fffccada7d6 100644 --- a/src/librustc_resolve/macros.rs +++ b/src/librustc_resolve/macros.rs @@ -123,14 +123,14 @@ impl<'a> base::Resolver for Resolver<'a> { } fn eliminate_crate_var(&mut self, item: P) -> P { - struct EliminateCrateVar<'b, 'a: 'b>(&'b mut Resolver<'a>); + struct EliminateCrateVar<'b, 'a: 'b>(&'b mut Resolver<'a>, Span); impl<'a, 'b> Folder for EliminateCrateVar<'a, 'b> { fn fold_path(&mut self, mut path: ast::Path) -> ast::Path { let ident = path.segments[0].identifier; if ident.name == "$crate" { path.segments[0].identifier.name = keywords::CrateRoot.name(); - let module = self.0.resolve_crate_var(ident.ctxt); + let module = self.0.resolve_crate_var(ident.ctxt, self.1); if !module.is_local() { let span = path.segments[0].span; path.segments.insert(1, match module.kind { @@ -149,7 +149,7 @@ impl<'a> base::Resolver for Resolver<'a> { } } - EliminateCrateVar(self).fold_item(item).expect_one("") + EliminateCrateVar(self, item.span).fold_item(item).expect_one("") } fn is_whitelisted_legacy_custom_derive(&self, name: Name) -> bool { diff --git a/src/test/ui/resolve/enums-are-namespaced-xc.stderr b/src/test/ui/resolve/enums-are-namespaced-xc.stderr index dd04c5ce356..17c5d5d15d4 100644 --- a/src/test/ui/resolve/enums-are-namespaced-xc.stderr +++ b/src/test/ui/resolve/enums-are-namespaced-xc.stderr @@ -4,8 +4,8 @@ error[E0425]: cannot find value `A` in module `namespaced_enums` 15 | let _ = namespaced_enums::A; | ^ not found in `namespaced_enums` | - = help: possible candidate is found in another module, you can import it into scope: - `use namespaced_enums::Foo::A;` +help: possible candidate is found in another module, you can import it into scope + | use namespaced_enums::Foo::A; error[E0425]: cannot find function `B` in module `namespaced_enums` --> $DIR/enums-are-namespaced-xc.rs:18:31 @@ -13,8 +13,8 @@ error[E0425]: cannot find function `B` in module `namespaced_enums` 18 | let _ = namespaced_enums::B(10); | ^ not found in `namespaced_enums` | - = help: possible candidate is found in another module, you can import it into scope: - `use namespaced_enums::Foo::B;` +help: possible candidate is found in another module, you can import it into scope + | use namespaced_enums::Foo::B; error[E0422]: cannot find struct, variant or union type `C` in module `namespaced_enums` --> $DIR/enums-are-namespaced-xc.rs:21:31 @@ -22,8 +22,8 @@ error[E0422]: cannot find struct, variant or union type `C` in module `namespace 21 | let _ = namespaced_enums::C { a: 10 }; | ^ not found in `namespaced_enums` | - = help: possible candidate is found in another module, you can import it into scope: - `use namespaced_enums::Foo::C;` +help: possible candidate is found in another module, you can import it into scope + | use namespaced_enums::Foo::C; error: aborting due to 3 previous errors diff --git a/src/test/ui/resolve/issue-16058.stderr b/src/test/ui/resolve/issue-16058.stderr index 69c48cc1f32..63d2ce10914 100644 --- a/src/test/ui/resolve/issue-16058.stderr +++ b/src/test/ui/resolve/issue-16058.stderr @@ -4,10 +4,10 @@ error[E0574]: expected struct, variant or union type, found enum `Result` 19 | Result { | ^^^^^^ not a struct, variant or union type | - = help: possible better candidates are found in other modules, you can import them into scope: - `use std::fmt::Result;` - `use std::io::Result;` - `use std::thread::Result;` +help: possible better candidates are found in other modules, you can import them into scope + | use std::fmt::Result; + | use std::io::Result; + | use std::thread::Result; error: aborting due to previous error diff --git a/src/test/ui/resolve/issue-17518.stderr b/src/test/ui/resolve/issue-17518.stderr index ea6841e6009..c0438abfe43 100644 --- a/src/test/ui/resolve/issue-17518.stderr +++ b/src/test/ui/resolve/issue-17518.stderr @@ -4,8 +4,8 @@ error[E0422]: cannot find struct, variant or union type `E` in this scope 16 | E { name: "foobar" }; //~ ERROR unresolved struct, variant or union type `E` | ^ not found in this scope | - = help: possible candidate is found in another module, you can import it into scope: - `use SomeEnum::E;` +help: possible candidate is found in another module, you can import it into scope + | use SomeEnum::E; error: aborting due to previous error diff --git a/src/test/ui/resolve/issue-21221-1.stderr b/src/test/ui/resolve/issue-21221-1.stderr index f38491d5362..7315d295f7b 100644 --- a/src/test/ui/resolve/issue-21221-1.stderr +++ b/src/test/ui/resolve/issue-21221-1.stderr @@ -4,10 +4,10 @@ error[E0405]: cannot find trait `Mul` in this scope 53 | impl Mul for Foo { | ^^^ not found in this scope | - = help: possible candidates are found in other modules, you can import them into scope: - `use mul1::Mul;` - `use mul2::Mul;` - `use std::ops::Mul;` +help: possible candidates are found in other modules, you can import them into scope + | use mul1::Mul; + | use mul2::Mul; + | use std::ops::Mul; error[E0412]: cannot find type `Mul` in this scope --> $DIR/issue-21221-1.rs:72:16 @@ -15,12 +15,12 @@ error[E0412]: cannot find type `Mul` in this scope 72 | fn getMul() -> Mul { | ^^^ not found in this scope | - = help: possible candidates are found in other modules, you can import them into scope: - `use mul1::Mul;` - `use mul2::Mul;` - `use mul3::Mul;` - `use mul4::Mul;` - and 2 other candidates +help: possible candidates are found in other modules, you can import them into scope + | use mul1::Mul; + | use mul2::Mul; + | use mul3::Mul; + | use mul4::Mul; +and 2 other candidates error[E0405]: cannot find trait `ThisTraitReallyDoesntExistInAnyModuleReally` in this scope --> $DIR/issue-21221-1.rs:83:6 @@ -34,8 +34,8 @@ error[E0405]: cannot find trait `Div` in this scope 88 | impl Div for Foo { | ^^^ not found in this scope | - = help: possible candidate is found in another module, you can import it into scope: - `use std::ops::Div;` +help: possible candidate is found in another module, you can import it into scope + | use std::ops::Div; error: cannot continue compilation due to previous error diff --git a/src/test/ui/resolve/issue-21221-2.stderr b/src/test/ui/resolve/issue-21221-2.stderr index 14dac7de4b2..f0b22754e64 100644 --- a/src/test/ui/resolve/issue-21221-2.stderr +++ b/src/test/ui/resolve/issue-21221-2.stderr @@ -4,8 +4,8 @@ error[E0405]: cannot find trait `T` in this scope 28 | impl T for Foo { } | ^ not found in this scope | - = help: possible candidate is found in another module, you can import it into scope: - `use foo::bar::T;` +help: possible candidate is found in another module, you can import it into scope + | use foo::bar::T; error: main function not found diff --git a/src/test/ui/resolve/issue-21221-3.stderr b/src/test/ui/resolve/issue-21221-3.stderr index e1e00571e5d..a4a2496b19a 100644 --- a/src/test/ui/resolve/issue-21221-3.stderr +++ b/src/test/ui/resolve/issue-21221-3.stderr @@ -4,8 +4,8 @@ error[E0405]: cannot find trait `OuterTrait` in this scope 25 | impl OuterTrait for Foo {} | ^^^^^^^^^^ not found in this scope | - = help: possible candidate is found in another module, you can import it into scope: - `use issue_21221_3::outer::OuterTrait;` +help: possible candidate is found in another module, you can import it into scope + | use issue_21221_3::outer::OuterTrait; error: cannot continue compilation due to previous error diff --git a/src/test/ui/resolve/issue-21221-4.stderr b/src/test/ui/resolve/issue-21221-4.stderr index 569315a59cf..dc2f2271731 100644 --- a/src/test/ui/resolve/issue-21221-4.stderr +++ b/src/test/ui/resolve/issue-21221-4.stderr @@ -4,8 +4,8 @@ error[E0405]: cannot find trait `T` in this scope 20 | impl T for Foo {} | ^ not found in this scope | - = help: possible candidate is found in another module, you can import it into scope: - `use issue_21221_4::T;` +help: possible candidate is found in another module, you can import it into scope + | use issue_21221_4::T; error: cannot continue compilation due to previous error diff --git a/src/test/ui/resolve/issue-3907.stderr b/src/test/ui/resolve/issue-3907.stderr index a7dd494d75b..0bf39dc55ce 100644 --- a/src/test/ui/resolve/issue-3907.stderr +++ b/src/test/ui/resolve/issue-3907.stderr @@ -4,8 +4,8 @@ error[E0404]: expected trait, found type alias `Foo` 20 | impl Foo for S { //~ ERROR expected trait, found type alias `Foo` | ^^^ type aliases cannot be used for traits | - = help: possible better candidate is found in another module, you can import it into scope: - `use issue_3907::Foo;` +help: possible better candidate is found in another module, you can import it into scope + | use issue_3907::Foo; error: cannot continue compilation due to previous error diff --git a/src/test/ui/resolve/privacy-struct-ctor.stderr b/src/test/ui/resolve/privacy-struct-ctor.stderr index 940e4acabb2..19940ff4586 100644 --- a/src/test/ui/resolve/privacy-struct-ctor.stderr +++ b/src/test/ui/resolve/privacy-struct-ctor.stderr @@ -8,8 +8,8 @@ error[E0423]: expected value, found struct `Z` | did you mean `S`? | constructor is not visible here due to private fields | - = help: possible better candidate is found in another module, you can import it into scope: - `use m::n::Z;` +help: possible better candidate is found in another module, you can import it into scope + | use m::n::Z; error[E0423]: expected value, found struct `S` --> $DIR/privacy-struct-ctor.rs:36:5 @@ -20,8 +20,8 @@ error[E0423]: expected value, found struct `S` | did you mean `S { /* fields */ }`? | constructor is not visible here due to private fields | - = help: possible better candidate is found in another module, you can import it into scope: - `use m::S;` +help: possible better candidate is found in another module, you can import it into scope + | use m::S; error[E0423]: expected value, found struct `xcrate::S` --> $DIR/privacy-struct-ctor.rs:42:5 @@ -32,8 +32,8 @@ error[E0423]: expected value, found struct `xcrate::S` | did you mean `xcrate::S { /* fields */ }`? | constructor is not visible here due to private fields | - = help: possible better candidate is found in another module, you can import it into scope: - `use m::S;` +help: possible better candidate is found in another module, you can import it into scope + | use m::S; error: tuple struct `Z` is private --> $DIR/privacy-struct-ctor.rs:25:9 diff --git a/src/test/ui/span/issue-35987.stderr b/src/test/ui/span/issue-35987.stderr index 9dab2f77898..e53ea6a55af 100644 --- a/src/test/ui/span/issue-35987.stderr +++ b/src/test/ui/span/issue-35987.stderr @@ -4,8 +4,8 @@ error[E0404]: expected trait, found type parameter `Add` 15 | impl Add for Foo { | ^^^ not a trait | - = help: possible better candidate is found in another module, you can import it into scope: - `use std::ops::Add;` +help: possible better candidate is found in another module, you can import it into scope + | use std::ops::Add; error: main function not found From 9d51d6bc30cbea2bec56c1a2ba351a91ced9dcb0 Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Wed, 10 May 2017 13:58:41 +0200 Subject: [PATCH 3/7] Fix tidy issues --- src/librustc_resolve/build_reduced_graph.rs | 6 ++++-- src/librustc_resolve/lib.rs | 14 +++++++++++--- 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/src/librustc_resolve/build_reduced_graph.rs b/src/librustc_resolve/build_reduced_graph.rs index d1f0cdedde8..9d774b96325 100644 --- a/src/librustc_resolve/build_reduced_graph.rs +++ b/src/librustc_resolve/build_reduced_graph.rs @@ -424,8 +424,10 @@ impl<'a> Resolver<'a> { fn build_reduced_graph_for_block(&mut self, block: &Block) { let parent = self.current_module; if self.block_needs_anonymous_module(block) { - let module = - self.new_module(parent, ModuleKind::Block(block.id), parent.normal_ancestor_id, block.span); + let module = self.new_module(parent, + ModuleKind::Block(block.id), + parent.normal_ancestor_id, + block.span); self.block_map.insert(block.id, module); self.current_module = module; // Descend into the block. } diff --git a/src/librustc_resolve/lib.rs b/src/librustc_resolve/lib.rs index fd964c7d7d1..57d04bee92b 100644 --- a/src/librustc_resolve/lib.rs +++ b/src/librustc_resolve/lib.rs @@ -873,7 +873,10 @@ pub struct ModuleData<'a> { pub type Module<'a> = &'a ModuleData<'a>; impl<'a> ModuleData<'a> { - fn new(parent: Option>, kind: ModuleKind, normal_ancestor_id: DefId, span: Span) -> Self { + fn new(parent: Option>, + kind: ModuleKind, + normal_ancestor_id: DefId, + span: Span) -> Self { ModuleData { parent: parent, kind: kind, @@ -1434,8 +1437,13 @@ impl<'a> Resolver<'a> { self.crate_loader.postprocess(krate); } - fn new_module(&self, parent: Module<'a>, kind: ModuleKind, normal_ancestor_id: DefId, span: Span) - -> Module<'a> { + fn new_module( + &self, + parent: Module<'a>, + kind: ModuleKind, + normal_ancestor_id: DefId, + span: Span, + ) -> Module<'a> { self.arenas.alloc_module(ModuleData::new(Some(parent), kind, normal_ancestor_id, span)) } From f859f2585b0167dfd9ef22d88d992ba6fe55e158 Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Wed, 10 May 2017 16:02:49 +0200 Subject: [PATCH 4/7] Update a compile-fail test --- src/test/compile-fail/issue-35675.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/test/compile-fail/issue-35675.rs b/src/test/compile-fail/issue-35675.rs index f990c2c42fe..001c1f2eddc 100644 --- a/src/test/compile-fail/issue-35675.rs +++ b/src/test/compile-fail/issue-35675.rs @@ -8,7 +8,9 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. -enum Fruit { +// these two HELPs are actually in a new line between this line and the `enum Fruit` line +enum Fruit { //~ HELP possible candidate is found in another module, you can import it into scope + //~^ HELP possible candidate is found in another module, you can import it into scope Apple(i64), //~^ HELP there is an enum variant `Fruit::Apple`, did you mean to use `Fruit`? //~| HELP there is an enum variant `Fruit::Apple`, did you mean to use `Fruit`? @@ -21,7 +23,6 @@ fn should_return_fruit() -> Apple { Apple(5) //~^ ERROR cannot find function `Apple` in this scope //~| NOTE not found in this scope - //~| HELP possible candidate is found in another module, you can import it into scope } fn should_return_fruit_too() -> Fruit::Apple { @@ -30,7 +31,6 @@ fn should_return_fruit_too() -> Fruit::Apple { Apple(5) //~^ ERROR cannot find function `Apple` in this scope //~| NOTE not found in this scope - //~| HELP possible candidate is found in another module, you can import it into scope } fn foo() -> Ok { From 644ce5e535f74be304a77dfadb9ff46c743554c7 Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Thu, 11 May 2017 15:26:22 +0200 Subject: [PATCH 5/7] Address PR reviews --- src/librustc_errors/diagnostic.rs | 11 +++++-- src/librustc_errors/emitter.rs | 50 ++++++++++++++----------------- src/librustc_errors/lib.rs | 28 +++++++++++++---- src/libsyntax/json.rs | 10 +++---- 4 files changed, 59 insertions(+), 40 deletions(-) diff --git a/src/librustc_errors/diagnostic.rs b/src/librustc_errors/diagnostic.rs index e129e313626..861880aa265 100644 --- a/src/librustc_errors/diagnostic.rs +++ b/src/librustc_errors/diagnostic.rs @@ -9,6 +9,7 @@ // except according to those terms. use CodeSuggestion; +use Substitution; use Level; use RenderSpan; use std::fmt; @@ -205,7 +206,10 @@ impl Diagnostic { /// See `diagnostic::CodeSuggestion` for more information. pub fn span_suggestion(&mut self, sp: Span, msg: &str, suggestion: String) -> &mut Self { self.suggestions.push(CodeSuggestion { - substitutes: vec![(sp, vec![suggestion])], + substitution_parts: vec![Substitution { + span: sp, + substitutions: vec![suggestion], + }], msg: msg.to_owned(), }); self @@ -213,7 +217,10 @@ impl Diagnostic { pub fn span_suggestions(&mut self, sp: Span, msg: &str, suggestions: Vec) -> &mut Self { self.suggestions.push(CodeSuggestion { - substitutes: vec![(sp, suggestions)], + substitution_parts: vec![Substitution { + span: sp, + substitutions: suggestions, + }], msg: msg.to_owned(), }); self diff --git a/src/librustc_errors/emitter.rs b/src/librustc_errors/emitter.rs index cd72941146c..d1ec1be47b8 100644 --- a/src/librustc_errors/emitter.rs +++ b/src/librustc_errors/emitter.rs @@ -35,38 +35,32 @@ impl Emitter for EmitterWriter { let mut primary_span = db.span.clone(); let mut children = db.children.clone(); - if db.suggestions.len() == 1 { - let sugg = &db.suggestions[0]; - // don't display multispans as labels - if sugg.substitutes.len() == 1 && + if let Some((sugg, rest)) = db.suggestions.split_first() { + if rest.is_empty() && + // don't display multipart suggestions as labels + sugg.substitution_parts.len() == 1 && // don't display multi-suggestions as labels - sugg.substitutes[0].1.len() == 1 && + sugg.substitutions() == 1 && // don't display long messages as labels sugg.msg.split_whitespace().count() < 10 && // don't display multiline suggestions as labels - sugg.substitutes[0].1[0].find('\n').is_none() { - let msg = format!("help: {} `{}`", sugg.msg, sugg.substitutes[0].1[0]); - primary_span.push_span_label(sugg.substitutes[0].0, msg); + sugg.substitution_parts[0].substitutions[0].find('\n').is_none() { + let substitution = &sugg.substitution_parts[0].substitutions[0]; + let msg = format!("help: {} `{}`", sugg.msg, substitution); + primary_span.push_span_label(sugg.substitution_spans().next().unwrap(), msg); } else { - children.push(SubDiagnostic { - level: Level::Help, - message: Vec::new(), - span: MultiSpan::new(), - render_span: Some(Suggestion(sugg.clone())), - }); - } - } 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 - for sugg in &db.suggestions { - children.push(SubDiagnostic { - level: Level::Help, - message: Vec::new(), - span: MultiSpan::new(), - render_span: Some(Suggestion(sugg.clone())), - }); + // 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 + for sugg in &db.suggestions { + children.push(SubDiagnostic { + level: Level::Help, + message: Vec::new(), + span: MultiSpan::new(), + render_span: Some(Suggestion(sugg.clone())), + }); + } } } @@ -1073,7 +1067,7 @@ impl EmitterWriter { -> io::Result<()> { use std::borrow::Borrow; - let primary_span = suggestion.substitutes[0].0; + let primary_span = suggestion.substitution_spans().next().unwrap(); if let Some(ref cm) = self.cm { let mut buffer = StyledBuffer::new(); diff --git a/src/librustc_errors/lib.rs b/src/librustc_errors/lib.rs index 82d688d6ba6..e1ec23479ab 100644 --- a/src/librustc_errors/lib.rs +++ b/src/librustc_errors/lib.rs @@ -23,6 +23,7 @@ #![feature(staged_api)] #![feature(range_contains)] #![feature(libc)] +#![feature(conservative_impl_trait)] extern crate term; extern crate libc; @@ -83,10 +84,17 @@ pub struct CodeSuggestion { /// ``` /// vec![(0..7, vec!["a.b", "x.y"])] /// ``` - pub substitutes: Vec<(Span, Vec)>, + pub substitution_parts: Vec, pub msg: String, } +#[derive(Clone, Debug, PartialEq, RustcEncodable, RustcDecodable)] +/// See the docs on `CodeSuggestion::substitutions` +pub struct Substitution { + pub span: Span, + pub substitutions: Vec, +} + pub trait CodeMapper { fn lookup_char_pos(&self, pos: BytePos) -> Loc; fn span_to_lines(&self, sp: Span) -> FileLinesResult; @@ -96,6 +104,16 @@ pub trait CodeMapper { } impl CodeSuggestion { + /// Returns the number of substitutions + fn substitutions(&self) -> usize { + self.substitution_parts[0].substitutions.len() + } + + /// Returns the number of substitutions + pub fn substitution_spans<'a>(&'a self) -> impl Iterator + 'a { + self.substitution_parts.iter().map(|sub| sub.span) + } + /// Returns the assembled code suggestions. pub fn splice_lines(&self, cm: &CodeMapper) -> Vec { use syntax_pos::{CharPos, Loc, Pos}; @@ -119,13 +137,13 @@ impl CodeSuggestion { } } - if self.substitutes.is_empty() { + if self.substitution_parts.is_empty() { return vec![String::new()]; } - let mut primary_spans: Vec<_> = self.substitutes + let mut primary_spans: Vec<_> = self.substitution_parts .iter() - .map(|&(sp, ref sub)| (sp, sub)) + .map(|sub| (sub.span, &sub.substitutions)) .collect(); // Assumption: all spans are in the same file, and all spans @@ -157,7 +175,7 @@ impl CodeSuggestion { prev_hi.col = CharPos::from_usize(0); let mut prev_line = fm.get_line(lines.lines[0].line_index); - let mut bufs = vec![String::new(); self.substitutes[0].1.len()]; + let mut bufs = vec![String::new(); self.substitutions()]; for (sp, substitutes) in primary_spans { let cur_lo = cm.lookup_char_pos(sp.lo); diff --git a/src/libsyntax/json.rs b/src/libsyntax/json.rs index 3d0b0b228a8..06335584c96 100644 --- a/src/libsyntax/json.rs +++ b/src/libsyntax/json.rs @@ -279,12 +279,12 @@ impl DiagnosticSpan { fn from_suggestion(suggestion: &CodeSuggestion, je: &JsonEmitter) -> Vec { - suggestion.substitutes + suggestion.substitution_parts .iter() - .flat_map(|&(span, ref suggestion)| { - suggestion.iter().map(move |suggestion| { + .flat_map(|substitution| { + substitution.substitutions.iter().map(move |suggestion| { let span_label = SpanLabel { - span, + span: substitution.span, is_primary: true, label: None, }; @@ -301,7 +301,7 @@ impl DiagnosticSpan { RenderSpan::FullSpan(ref msp) => DiagnosticSpan::from_multispan(msp, je), // regular diagnostics don't produce this anymore - // will be removed in a later commit + // FIXME(oli_obk): remove it entirely RenderSpan::Suggestion(_) => unreachable!(), } } From a54bbf2cb34d4a8bc28f99bb9a4677aa1dc2558c Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Fri, 12 May 2017 11:21:11 +0200 Subject: [PATCH 6/7] Weave the span of an import through the resolve code --- src/librustc_resolve/build_reduced_graph.rs | 5 +- src/librustc_resolve/lib.rs | 80 ++++++++++++--------- src/librustc_resolve/macros.rs | 34 +++++---- src/librustc_resolve/resolve_imports.rs | 46 ++++++++---- 4 files changed, 101 insertions(+), 64 deletions(-) diff --git a/src/librustc_resolve/build_reduced_graph.rs b/src/librustc_resolve/build_reduced_graph.rs index 9d774b96325..57639a1ecef 100644 --- a/src/librustc_resolve/build_reduced_graph.rs +++ b/src/librustc_resolve/build_reduced_graph.rs @@ -608,7 +608,8 @@ impl<'a> Resolver<'a> { } else { for (name, span) in legacy_imports.imports { let ident = Ident::with_empty_ctxt(name); - let result = self.resolve_ident_in_module(module, ident, MacroNS, false, None); + let result = self.resolve_ident_in_module(module, ident, MacroNS, + false, false, span); if let Ok(binding) = result { let directive = macro_use_directive(span); self.potentially_unused_imports.push(directive); @@ -622,7 +623,7 @@ impl<'a> Resolver<'a> { for (name, span) in legacy_imports.reexports { self.session.cstore.export_macros(module.def_id().unwrap().krate); let ident = Ident::with_empty_ctxt(name); - let result = self.resolve_ident_in_module(module, ident, MacroNS, false, None); + let result = self.resolve_ident_in_module(module, ident, MacroNS, false, false, span); if let Ok(binding) = result { self.macro_exports.push(Export { name: name, def: binding.def(), span: span }); } else { diff --git a/src/librustc_resolve/lib.rs b/src/librustc_resolve/lib.rs index 57d04bee92b..6011a680dcb 100644 --- a/src/librustc_resolve/lib.rs +++ b/src/librustc_resolve/lib.rs @@ -613,7 +613,7 @@ impl<'a, 'tcx> Visitor<'tcx> for Resolver<'a> { self.smart_resolve_path(ty.id, qself.as_ref(), path, PathSource::Type); } else if let TyKind::ImplicitSelf = ty.node { let self_ty = keywords::SelfType.ident(); - let def = self.resolve_ident_in_lexical_scope(self_ty, TypeNS, Some(ty.span)) + let def = self.resolve_ident_in_lexical_scope(self_ty, TypeNS, true, ty.span) .map_or(Def::Err, |d| d.def()); self.record_def(ty.id, PathResolution::new(def)); } else if let TyKind::Array(ref element, ref length) = ty.node { @@ -1267,11 +1267,11 @@ impl<'a> hir::lowering::Resolver for Resolver<'a> { let namespace = if is_value { ValueNS } else { TypeNS }; let hir::Path { ref segments, span, ref mut def } = *path; let path: Vec<_> = segments.iter().map(|seg| Ident::with_empty_ctxt(seg.name)).collect(); - match self.resolve_path(&path, Some(namespace), Some(span)) { + match self.resolve_path(&path, Some(namespace), true, span) { PathResult::Module(module) => *def = module.def().unwrap(), PathResult::NonModule(path_res) if path_res.unresolved_segments() == 0 => *def = path_res.base_def(), - PathResult::NonModule(..) => match self.resolve_path(&path, None, Some(span)) { + PathResult::NonModule(..) => match self.resolve_path(&path, None, true, span) { PathResult::Failed(msg, _) => { resolve_error(self, span, ResolutionError::FailedToResolve(&msg)); } @@ -1502,7 +1502,8 @@ impl<'a> Resolver<'a> { fn resolve_ident_in_lexical_scope(&mut self, mut ident: Ident, ns: Namespace, - record_used: Option) + record_used: bool, + path_span: Span) -> Option> { if ns == TypeNS { ident = ident.unhygienize(); @@ -1513,12 +1514,13 @@ impl<'a> Resolver<'a> { if let Some(def) = self.ribs[ns][i].bindings.get(&ident).cloned() { // The ident resolves to a type parameter or local variable. return Some(LexicalScopeBinding::Def( - self.adjust_local_def(ns, i, def, record_used) + self.adjust_local_def(ns, i, def, record_used, path_span) )); } if let ModuleRibKind(module) = self.ribs[ns][i].kind { - let item = self.resolve_ident_in_module(module, ident, ns, false, record_used); + let item = self.resolve_ident_in_module(module, ident, ns, false, + record_used, path_span); if let Ok(binding) = item { // The ident resolves to an item. return Some(LexicalScopeBinding::Item(binding)); @@ -1527,7 +1529,8 @@ impl<'a> Resolver<'a> { if let ModuleKind::Block(..) = module.kind { // We can see through blocks } else if !module.no_implicit_prelude { return self.prelude.and_then(|prelude| { - self.resolve_ident_in_module(prelude, ident, ns, false, None).ok() + self.resolve_ident_in_module(prelude, ident, ns, false, + false, path_span).ok() }).map(LexicalScopeBinding::Item) } else { return None; @@ -2147,7 +2150,8 @@ impl<'a> Resolver<'a> { PatKind::Ident(bmode, ref ident, ref opt_pat) => { // First try to resolve the identifier as some existing // entity, then fall back to a fresh binding. - let binding = self.resolve_ident_in_lexical_scope(ident.node, ValueNS, None) + let binding = self.resolve_ident_in_lexical_scope(ident.node, ValueNS, + false, pat.span) .and_then(LexicalScopeBinding::item); let resolution = binding.map(NameBinding::def).and_then(|def| { let always_binding = !pat_src.is_refutable() || opt_pat.is_some() || @@ -2253,7 +2257,7 @@ impl<'a> Resolver<'a> { (format!(""), format!("the crate root")) } else { let mod_path = &path[..path.len() - 1]; - let mod_prefix = match this.resolve_path(mod_path, Some(TypeNS), None) { + let mod_prefix = match this.resolve_path(mod_path, Some(TypeNS), false, span) { PathResult::Module(module) => module.def(), _ => None, }.map_or(format!(""), |def| format!("{} ", def.kind_name())); @@ -2303,9 +2307,9 @@ impl<'a> Resolver<'a> { } } } - if path.len() == 1 && this.self_type_is_available() { + if path.len() == 1 && this.self_type_is_available(span) { if let Some(candidate) = this.lookup_assoc_candidate(name, ns, is_expected) { - let self_is_available = this.self_value_is_available(path[0].ctxt); + let self_is_available = this.self_value_is_available(path[0].ctxt, span); match candidate { AssocSuggestion::Field => { err.span_label(span, format!("did you mean `self.{}`?", path_str)); @@ -2329,7 +2333,7 @@ impl<'a> Resolver<'a> { let mut levenshtein_worked = false; // Try Levenshtein. - if let Some(candidate) = this.lookup_typo_candidate(path, ns, is_expected) { + if let Some(candidate) = this.lookup_typo_candidate(path, ns, is_expected, span) { err.span_label(ident_span, format!("did you mean `{}`?", candidate)); levenshtein_worked = true; } @@ -2434,14 +2438,15 @@ impl<'a> Resolver<'a> { resolution } - fn self_type_is_available(&mut self) -> bool { - let binding = self.resolve_ident_in_lexical_scope(keywords::SelfType.ident(), TypeNS, None); + fn self_type_is_available(&mut self, span: Span) -> bool { + let binding = self.resolve_ident_in_lexical_scope(keywords::SelfType.ident(), + TypeNS, false, span); if let Some(LexicalScopeBinding::Def(def)) = binding { def != Def::Err } else { false } } - fn self_value_is_available(&mut self, ctxt: SyntaxContext) -> bool { + fn self_value_is_available(&mut self, ctxt: SyntaxContext, span: Span) -> bool { let ident = Ident { name: keywords::SelfValue.name(), ctxt: ctxt }; - let binding = self.resolve_ident_in_lexical_scope(ident, ValueNS, None); + let binding = self.resolve_ident_in_lexical_scope(ident, ValueNS, false, span); if let Some(LexicalScopeBinding::Def(def)) = binding { def != Def::Err } else { false } } @@ -2505,7 +2510,7 @@ impl<'a> Resolver<'a> { )); } - let result = match self.resolve_path(&path, Some(ns), Some(span)) { + let result = match self.resolve_path(&path, Some(ns), true, span) { PathResult::NonModule(path_res) => path_res, PathResult::Module(module) if !module.is_normal() => { PathResolution::new(module.def().unwrap()) @@ -2551,7 +2556,7 @@ impl<'a> Resolver<'a> { if path.len() > 1 && !global_by_default && result.base_def() != Def::Err && path[0].name != keywords::CrateRoot.name() && path[0].name != "$crate" { let unqualified_result = { - match self.resolve_path(&[*path.last().unwrap()], Some(ns), None) { + match self.resolve_path(&[*path.last().unwrap()], Some(ns), false, span) { PathResult::NonModule(path_res) => path_res.base_def(), PathResult::Module(module) => module.def().unwrap(), _ => return Some(result), @@ -2569,7 +2574,8 @@ impl<'a> Resolver<'a> { fn resolve_path(&mut self, path: &[Ident], opt_ns: Option, // `None` indicates a module path - record_used: Option) + record_used: bool, + path_span: Span) -> PathResult<'a> { let mut module = None; let mut allow_super = true; @@ -2603,12 +2609,12 @@ impl<'a> Resolver<'a> { } let binding = if let Some(module) = module { - self.resolve_ident_in_module(module, ident, ns, false, record_used) + self.resolve_ident_in_module(module, ident, ns, false, record_used, path_span) } else if opt_ns == Some(MacroNS) { - self.resolve_lexical_macro_path_segment(ident, ns, record_used) + self.resolve_lexical_macro_path_segment(ident, ns, record_used, path_span) .map(MacroBinding::binding) } else { - match self.resolve_ident_in_lexical_scope(ident, ns, record_used) { + match self.resolve_ident_in_lexical_scope(ident, ns, record_used, path_span) { Some(LexicalScopeBinding::Item(binding)) => Ok(binding), Some(LexicalScopeBinding::Def(def)) if opt_ns == Some(TypeNS) || opt_ns == Some(ValueNS) => { @@ -2616,7 +2622,7 @@ impl<'a> Resolver<'a> { def, path.len() - 1 )); } - _ => Err(if record_used.is_some() { Determined } else { Undetermined }), + _ => Err(if record_used { Determined } else { Undetermined }), } }; @@ -2673,12 +2679,13 @@ impl<'a> Resolver<'a> { ns: Namespace, rib_index: usize, mut def: Def, - record_used: Option) -> Def { + record_used: bool, + span: Span) -> Def { let ribs = &self.ribs[ns][rib_index + 1..]; // An invalid forward use of a type parameter from a previous default. if let ForwardTyParamBanRibKind = self.ribs[ns][rib_index].kind { - if let Some(span) = record_used { + if record_used { resolve_error(self, span, ResolutionError::ForwardDeclaredTyParam); } @@ -2688,7 +2695,7 @@ impl<'a> Resolver<'a> { match def { Def::Upvar(..) => { - span_bug!(record_used.unwrap_or(DUMMY_SP), "unexpected {:?} in bindings", def) + span_bug!(span, "unexpected {:?} in bindings", def) } Def::Local(def_id) => { for rib in ribs { @@ -2714,7 +2721,7 @@ impl<'a> Resolver<'a> { let depth = vec.len(); def = Def::Upvar(def_id, depth, function_id); - if let Some(span) = record_used { + if record_used { vec.push(Freevar { def: prev_def, span: span, @@ -2726,7 +2733,7 @@ impl<'a> Resolver<'a> { // This was an attempt to access an upvar inside a // named function item. This is not allowed, so we // report an error. - if let Some(span) = record_used { + if record_used { resolve_error(self, span, ResolutionError::CannotCaptureDynamicEnvironmentInFnItem); } @@ -2734,7 +2741,7 @@ impl<'a> Resolver<'a> { } ConstantItemRibKind => { // Still doesn't deal with upvars - if let Some(span) = record_used { + if record_used { resolve_error(self, span, ResolutionError::AttemptToUseNonConstantValueInConstant); } @@ -2753,7 +2760,7 @@ impl<'a> Resolver<'a> { ItemRibKind => { // This was an attempt to use a type parameter outside // its scope. - if let Some(span) = record_used { + if record_used { resolve_error(self, span, ResolutionError::TypeParametersFromOuterFunction); } @@ -2761,7 +2768,7 @@ impl<'a> Resolver<'a> { } ConstantItemRibKind => { // see #9186 - if let Some(span) = record_used { + if record_used { resolve_error(self, span, ResolutionError::OuterTypeParameterContext); } @@ -2857,7 +2864,8 @@ impl<'a> Resolver<'a> { fn lookup_typo_candidate(&mut self, path: &[Ident], ns: Namespace, - filter_fn: FilterFn) + filter_fn: FilterFn, + span: Span) -> Option where FilterFn: Fn(Def) -> bool { @@ -2909,7 +2917,8 @@ impl<'a> Resolver<'a> { } else { // Search in module. let mod_path = &path[..path.len() - 1]; - if let PathResult::Module(module) = self.resolve_path(mod_path, Some(TypeNS), None) { + if let PathResult::Module(module) = self.resolve_path(mod_path, Some(TypeNS), + false, span) { add_module_candidates(module, &mut names); } } @@ -3410,7 +3419,10 @@ impl<'a> Resolver<'a> { continue } let ident = attr.path.segments[0].identifier; - let result = self.resolve_lexical_macro_path_segment(ident, MacroNS, None); + let result = self.resolve_lexical_macro_path_segment(ident, + MacroNS, + false, + attr.path.span); if let Ok(binding) = result { if let SyntaxExtension::AttrProcMacro(..) = *binding.binding().get_macro(self) { attr::mark_known(attr); diff --git a/src/librustc_resolve/macros.rs b/src/librustc_resolve/macros.rs index fffccada7d6..c08421cb937 100644 --- a/src/librustc_resolve/macros.rs +++ b/src/librustc_resolve/macros.rs @@ -379,7 +379,7 @@ impl<'a> Resolver<'a> { return Err(Determinacy::Determined); } - let def = match self.resolve_path(&path, Some(MacroNS), None) { + let def = match self.resolve_path(&path, Some(MacroNS), false, span) { PathResult::NonModule(path_res) => match path_res.base_def() { Def::Err => Err(Determinacy::Determined), def @ _ => Ok(def), @@ -401,7 +401,7 @@ impl<'a> Resolver<'a> { let result = if let Some(MacroBinding::Legacy(binding)) = legacy_resolution { Ok(Def::Macro(binding.def_id, MacroKind::Bang)) } else { - match self.resolve_lexical_macro_path_segment(path[0], MacroNS, None) { + match self.resolve_lexical_macro_path_segment(path[0], MacroNS, false, span) { Ok(binding) => Ok(binding.binding().def_ignoring_ambiguity()), Err(Determinacy::Undetermined) if !force => return Err(Determinacy::Undetermined), Err(_) => { @@ -421,18 +421,19 @@ impl<'a> Resolver<'a> { pub fn resolve_lexical_macro_path_segment(&mut self, ident: Ident, ns: Namespace, - record_used: Option) + record_used: bool, + path_span: Span) -> Result, Determinacy> { let mut module = Some(self.current_module); let mut potential_illegal_shadower = Err(Determinacy::Determined); let determinacy = - if record_used.is_some() { Determinacy::Determined } else { Determinacy::Undetermined }; + if record_used { Determinacy::Determined } else { Determinacy::Undetermined }; loop { let result = if let Some(module) = module { // Since expanded macros may not shadow the lexical scope and // globs may not shadow global macros (both enforced below), // we resolve with restricted shadowing (indicated by the penultimate argument). - self.resolve_ident_in_module(module, ident, ns, true, record_used) + self.resolve_ident_in_module(module, ident, ns, true, record_used, path_span) .map(MacroBinding::Modern) } else { self.global_macros.get(&ident.name).cloned().ok_or(determinacy) @@ -441,15 +442,18 @@ impl<'a> Resolver<'a> { match result.map(MacroBinding::binding) { Ok(binding) => { - let span = match record_used { - Some(span) => span, - None => return result, - }; + if !record_used { + return result; + } if let Ok(MacroBinding::Modern(shadower)) = potential_illegal_shadower { if shadower.def() != binding.def() { let name = ident.name; self.ambiguity_errors.push(AmbiguityError { - span: span, name: name, b1: shadower, b2: binding, lexical: true, + span: path_span, + name: name, + b1: shadower, + b2: binding, + lexical: true, legacy: false, }); return potential_illegal_shadower; @@ -543,7 +547,7 @@ impl<'a> Resolver<'a> { pub fn finalize_current_module_macro_resolutions(&mut self) { let module = self.current_module; for &(ref path, span) in module.macro_resolutions.borrow().iter() { - match self.resolve_path(path, Some(MacroNS), Some(span)) { + match self.resolve_path(path, Some(MacroNS), true, span) { PathResult::NonModule(_) => {}, PathResult::Failed(msg, _) => { resolve_error(self, span, ResolutionError::FailedToResolve(&msg)); @@ -555,7 +559,7 @@ impl<'a> Resolver<'a> { for &(mark, ident, span, kind) in module.legacy_macro_resolutions.borrow().iter() { let legacy_scope = &self.invocations[&mark].legacy_scope; let legacy_resolution = self.resolve_legacy_scope(legacy_scope, ident.name, true); - let resolution = self.resolve_lexical_macro_path_segment(ident, MacroNS, Some(span)); + let resolution = self.resolve_lexical_macro_path_segment(ident, MacroNS, true, span); match (legacy_resolution, resolution) { (Some(MacroBinding::Legacy(legacy_binding)), Ok(MacroBinding::Modern(binding))) => { let msg1 = format!("`{}` could refer to the macro defined here", ident); @@ -579,7 +583,7 @@ impl<'a> Resolver<'a> { format!("cannot find derive macro `{}` in this scope", ident), }; let mut err = self.session.struct_span_err(span, &msg); - self.suggest_macro_name(&ident.name.as_str(), kind, &mut err); + self.suggest_macro_name(&ident.name.as_str(), kind, &mut err, span); err.emit(); }, _ => {}, @@ -588,7 +592,7 @@ impl<'a> Resolver<'a> { } fn suggest_macro_name(&mut self, name: &str, kind: MacroKind, - err: &mut DiagnosticBuilder<'a>) { + err: &mut DiagnosticBuilder<'a>, span: Span) { // First check if this is a locally-defined bang macro. let suggestion = if let MacroKind::Bang = kind { find_best_match_for_name(self.macro_names.iter(), name, None) @@ -619,7 +623,7 @@ impl<'a> Resolver<'a> { } }; let ident = Ident::from_str(name); - self.lookup_typo_candidate(&vec![ident], MacroNS, is_macro) + self.lookup_typo_candidate(&vec![ident], MacroNS, is_macro, span) }); if let Some(suggestion) = suggestion { diff --git a/src/librustc_resolve/resolve_imports.rs b/src/librustc_resolve/resolve_imports.rs index 804e1ea740f..1d4ba4ed100 100644 --- a/src/librustc_resolve/resolve_imports.rs +++ b/src/librustc_resolve/resolve_imports.rs @@ -146,7 +146,8 @@ impl<'a> Resolver<'a> { ident: Ident, ns: Namespace, restricted_shadowing: bool, - record_used: Option) + record_used: bool, + path_span: Span) -> Result<&'a NameBinding<'a>, Determinacy> { self.populate_module_if_necessary(module); @@ -154,7 +155,7 @@ impl<'a> Resolver<'a> { .try_borrow_mut() .map_err(|_| Determined)?; // This happens when there is a cycle of imports - if let Some(span) = record_used { + if record_used { if let Some(binding) = resolution.binding { if let Some(shadowed_glob) = resolution.shadows_glob { let name = ident.name; @@ -164,16 +165,20 @@ impl<'a> Resolver<'a> { ns != MacroNS && // In MacroNS, `try_define` always forbids this shadowing binding.def() != shadowed_glob.def() { self.ambiguity_errors.push(AmbiguityError { - span: span, name: name, lexical: false, b1: binding, b2: shadowed_glob, + span: path_span, + name: name, + lexical: false, + b1: binding, + b2: shadowed_glob, legacy: false, }); } } - if self.record_use(ident, ns, binding, span) { + if self.record_use(ident, ns, binding, path_span) { return Ok(self.dummy_binding); } if !self.is_accessible(binding.vis) { - self.privacy_errors.push(PrivacyError(span, ident.name, binding)); + self.privacy_errors.push(PrivacyError(path_span, ident.name, binding)); } } @@ -205,7 +210,7 @@ impl<'a> Resolver<'a> { SingleImport { source, .. } => source, _ => unreachable!(), }; - match self.resolve_ident_in_module(module, ident, ns, false, None) { + match self.resolve_ident_in_module(module, ident, ns, false, false, path_span) { Err(Determined) => {} _ => return Err(Undetermined), } @@ -230,7 +235,12 @@ impl<'a> Resolver<'a> { for directive in module.globs.borrow().iter() { if self.is_accessible(directive.vis.get()) { if let Some(module) = directive.imported_module.get() { - let result = self.resolve_ident_in_module(module, ident, ns, false, None); + let result = self.resolve_ident_in_module(module, + ident, + ns, + false, + false, + path_span); if let Err(Undetermined) = result { return Err(Undetermined); } @@ -499,7 +509,7 @@ impl<'a, 'b:'a> ImportResolver<'a, 'b> { // For better failure detection, pretend that the import will not define any names // while resolving its module path. directive.vis.set(ty::Visibility::Invisible); - let result = self.resolve_path(&directive.module_path, None, None); + let result = self.resolve_path(&directive.module_path, None, false, directive.span); directive.vis.set(vis); match result { @@ -523,7 +533,12 @@ impl<'a, 'b:'a> ImportResolver<'a, 'b> { let mut indeterminate = false; self.per_ns(|this, ns| if !type_ns_only || ns == TypeNS { if let Err(Undetermined) = result[ns].get() { - result[ns].set(this.resolve_ident_in_module(module, source, ns, false, None)); + result[ns].set(this.resolve_ident_in_module(module, + source, + ns, + false, + false, + directive.span)); } else { return }; @@ -563,14 +578,14 @@ impl<'a, 'b:'a> ImportResolver<'a, 'b> { self.current_module = directive.parent; let ImportDirective { ref module_path, span, .. } = *directive; - let module_result = self.resolve_path(&module_path, None, Some(span)); + let module_result = self.resolve_path(&module_path, None, true, span); let module = match module_result { PathResult::Module(module) => module, PathResult::Failed(msg, _) => { let (mut self_path, mut self_result) = (module_path.clone(), None); if !self_path.is_empty() && !token::Ident(self_path[0]).is_path_segment_keyword() { self_path[0].name = keywords::SelfValue.name(); - self_result = Some(self.resolve_path(&self_path, None, None)); + self_result = Some(self.resolve_path(&self_path, None, false, span)); } return if let Some(PathResult::Module(..)) = self_result { Some(format!("Did you mean `{}`?", names_to_string(&self_path))) @@ -609,7 +624,12 @@ impl<'a, 'b:'a> ImportResolver<'a, 'b> { Some(this.dummy_binding); } } - } else if let Ok(binding) = this.resolve_ident_in_module(module, ident, ns, false, None) { + } else if let Ok(binding) = this.resolve_ident_in_module(module, + ident, + ns, + false, + false, + directive.span) { legacy_self_import = Some(directive); let binding = this.arenas.alloc_name_binding(NameBinding { kind: NameBindingKind::Import { @@ -630,7 +650,7 @@ impl<'a, 'b:'a> ImportResolver<'a, 'b> { } let mut all_ns_failed = true; self.per_ns(|this, ns| if !type_ns_only || ns == TypeNS { - match this.resolve_ident_in_module(module, ident, ns, false, Some(span)) { + match this.resolve_ident_in_module(module, ident, ns, false, true, span) { Ok(_) => all_ns_failed = false, _ => {} } From 3f2bbe3f4f69379b13190bfdc191a8e394b04998 Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Fri, 12 May 2017 11:21:34 +0200 Subject: [PATCH 7/7] Don't use a DUMMY_SP for reporting issues with crate imports --- src/librustc_resolve/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/librustc_resolve/lib.rs b/src/librustc_resolve/lib.rs index 6011a680dcb..6054f46370e 100644 --- a/src/librustc_resolve/lib.rs +++ b/src/librustc_resolve/lib.rs @@ -2604,7 +2604,7 @@ impl<'a> Resolver<'a> { module = Some(self.graph_root); continue } else if i == 0 && ns == TypeNS && ident.name == "$crate" { - module = Some(self.resolve_crate_var(ident.ctxt, DUMMY_SP)); + module = Some(self.resolve_crate_var(ident.ctxt, path_span)); continue }