From 126ac9ef6c11ced54ba9191aba916dd7d5206159 Mon Sep 17 00:00:00 2001 From: David Wood Date: Sun, 7 Apr 2019 17:56:50 +0200 Subject: [PATCH 01/11] Add test with current behaviour. This commit adds a test demonstrating the current behaviour when a macro defined in a module with the `#[macro_export]` is imported from the module rather than the crate root. --- src/test/ui/auxiliary/issue-59764.rs | 8 ++++++++ src/test/ui/issue-59764.rs | 14 ++++++++++++++ src/test/ui/issue-59764.stderr | 24 ++++++++++++++++++++++++ 3 files changed, 46 insertions(+) create mode 100644 src/test/ui/auxiliary/issue-59764.rs create mode 100644 src/test/ui/issue-59764.rs create mode 100644 src/test/ui/issue-59764.stderr diff --git a/src/test/ui/auxiliary/issue-59764.rs b/src/test/ui/auxiliary/issue-59764.rs new file mode 100644 index 0000000000000..0243ef5c6c74a --- /dev/null +++ b/src/test/ui/auxiliary/issue-59764.rs @@ -0,0 +1,8 @@ +pub mod foo { + #[macro_export] + macro_rules! makro { + ($foo:ident) => { + fn $foo() { } + } + } +} diff --git a/src/test/ui/issue-59764.rs b/src/test/ui/issue-59764.rs new file mode 100644 index 0000000000000..8ac9b9eaba181 --- /dev/null +++ b/src/test/ui/issue-59764.rs @@ -0,0 +1,14 @@ +// aux-build:issue-59764.rs +// compile-flags:--extern issue_59764 +// edition:2018 + +use issue_59764::foo::makro; +//~^ ERROR unresolved import `issue_59764::foo::makro` [E0432] + +makro!(bar); +//~^ ERROR cannot determine resolution for the macro `makro` + +fn main() { + bar(); + //~^ ERROR cannot find function `bar` in this scope [E0425] +} diff --git a/src/test/ui/issue-59764.stderr b/src/test/ui/issue-59764.stderr new file mode 100644 index 0000000000000..a428488b009ec --- /dev/null +++ b/src/test/ui/issue-59764.stderr @@ -0,0 +1,24 @@ +error[E0432]: unresolved import `issue_59764::foo::makro` + --> $DIR/issue-59764.rs:5:5 + | +LL | use issue_59764::foo::makro; + | ^^^^^^^^^^^^^^^^^^^^^^^ no `makro` in `foo` + +error: cannot determine resolution for the macro `makro` + --> $DIR/issue-59764.rs:8:1 + | +LL | makro!(bar); + | ^^^^^ + | + = note: import resolution is stuck, try simplifying macro imports + +error[E0425]: cannot find function `bar` in this scope + --> $DIR/issue-59764.rs:12:5 + | +LL | bar(); + | ^^^ not found in this scope + +error: aborting due to 3 previous errors + +Some errors occurred: E0425, E0432. +For more information about an error, try `rustc --explain E0425`. From 724ca0584e2be0714edf2f30b86230666d7a9d19 Mon Sep 17 00:00:00 2001 From: Michael Woerister Date: Tue, 9 Apr 2019 14:47:00 +0200 Subject: [PATCH 02/11] Exclude profiler-generated symbols from MSVC __imp_-symbol workaround. --- src/librustc_codegen_llvm/back/write.rs | 18 +++++++++++++++++- .../pgo-gen-no-imp-symbols/Makefile | 11 +++++++++++ .../pgo-gen-no-imp-symbols/test.rs | 1 + 3 files changed, 29 insertions(+), 1 deletion(-) create mode 100644 src/test/run-make-fulldeps/pgo-gen-no-imp-symbols/Makefile create mode 100644 src/test/run-make-fulldeps/pgo-gen-no-imp-symbols/test.rs diff --git a/src/librustc_codegen_llvm/back/write.rs b/src/librustc_codegen_llvm/back/write.rs index f0ed201ad5c27..3628adeb3e9a7 100644 --- a/src/librustc_codegen_llvm/back/write.rs +++ b/src/librustc_codegen_llvm/back/write.rs @@ -795,6 +795,7 @@ fn create_msvc_imps( } else { "\x01__imp_" }; + unsafe { let i8p_ty = Type::i8p_llcx(llcx); let globals = base::iter_globals(llmod) @@ -802,14 +803,23 @@ fn create_msvc_imps( llvm::LLVMRustGetLinkage(val) == llvm::Linkage::ExternalLinkage && llvm::LLVMIsDeclaration(val) == 0 }) - .map(move |val| { + .filter_map(|val| { + // Exclude some symbols that we know are not Rust symbols. let name = CStr::from_ptr(llvm::LLVMGetValueName(val)); + if ignored(name.to_bytes()) { + None + } else { + Some((val, name)) + } + }) + .map(move |(val, name)| { let mut imp_name = prefix.as_bytes().to_vec(); imp_name.extend(name.to_bytes()); let imp_name = CString::new(imp_name).unwrap(); (imp_name, val) }) .collect::>(); + for (imp_name, val) in globals { let imp = llvm::LLVMAddGlobal(llmod, i8p_ty, @@ -818,4 +828,10 @@ fn create_msvc_imps( llvm::LLVMRustSetLinkage(imp, llvm::Linkage::ExternalLinkage); } } + + // Use this function to exclude certain symbols from `__imp` generation. + fn ignored(symbol_name: &[u8]) -> bool { + // These are symbols generated by LLVM's profiling instrumentation + symbol_name.starts_with(b"__llvm_profile_") + } } diff --git a/src/test/run-make-fulldeps/pgo-gen-no-imp-symbols/Makefile b/src/test/run-make-fulldeps/pgo-gen-no-imp-symbols/Makefile new file mode 100644 index 0000000000000..dc52e91317a5a --- /dev/null +++ b/src/test/run-make-fulldeps/pgo-gen-no-imp-symbols/Makefile @@ -0,0 +1,11 @@ +-include ../tools.mk + +all: +ifeq ($(PROFILER_SUPPORT),1) + $(RUSTC) -O -Ccodegen-units=1 -Z pgo-gen="$(TMPDIR)/test.profraw" --emit=llvm-ir test.rs + # We expect symbols starting with "__llvm_profile_". + $(CGREP) "__llvm_profile_" < $(TMPDIR)/test.ll + # We do NOT expect the "__imp_" version of these symbols. + $(CGREP) -v "__imp___llvm_profile_" < $(TMPDIR)/test.ll # 64 bit + $(CGREP) -v "__imp____llvm_profile_" < $(TMPDIR)/test.ll # 32 bit +endif diff --git a/src/test/run-make-fulldeps/pgo-gen-no-imp-symbols/test.rs b/src/test/run-make-fulldeps/pgo-gen-no-imp-symbols/test.rs new file mode 100644 index 0000000000000..f328e4d9d04c3 --- /dev/null +++ b/src/test/run-make-fulldeps/pgo-gen-no-imp-symbols/test.rs @@ -0,0 +1 @@ +fn main() {} From 7b1df42accbfdf54148072c7a3d1e80177c388b0 Mon Sep 17 00:00:00 2001 From: Michael Woerister Date: Wed, 10 Apr 2019 13:46:37 +0200 Subject: [PATCH 03/11] Clean up handling of -Zpgo-gen commandline option. --- src/librustc/session/config.rs | 38 ++++++++++++++++--- src/librustc_codegen_llvm/attributes.rs | 2 +- src/librustc_codegen_llvm/back/link.rs | 2 +- src/librustc_codegen_llvm/back/write.rs | 22 ++++++++--- .../back/symbol_export.rs | 2 +- src/librustc_codegen_ssa/back/write.rs | 7 ++-- src/librustc_metadata/creader.rs | 2 +- .../run-make-fulldeps/pgo-gen-lto/Makefile | 6 +-- src/test/run-make-fulldeps/pgo-gen/Makefile | 6 +-- 9 files changed, 60 insertions(+), 27 deletions(-) diff --git a/src/librustc/session/config.rs b/src/librustc/session/config.rs index 988e4a9ff23a2..7c0eab26b09b6 100644 --- a/src/librustc/session/config.rs +++ b/src/librustc/session/config.rs @@ -113,6 +113,21 @@ impl LinkerPluginLto { } } +#[derive(Clone, PartialEq, Hash)] +pub enum PgoGenerate { + Enabled(Option), + Disabled, +} + +impl PgoGenerate { + pub fn enabled(&self) -> bool { + match *self { + PgoGenerate::Enabled(_) => true, + PgoGenerate::Disabled => false, + } + } +} + #[derive(Clone, Copy, PartialEq, Hash)] pub enum DebugInfo { None, @@ -826,13 +841,15 @@ macro_rules! options { pub const parse_linker_plugin_lto: Option<&str> = Some("either a boolean (`yes`, `no`, `on`, `off`, etc), \ or the path to the linker plugin"); + pub const parse_pgo_generate: Option<&str> = + Some("an optional path to the profiling data output directory"); pub const parse_merge_functions: Option<&str> = Some("one of: `disabled`, `trampolines`, or `aliases`"); } #[allow(dead_code)] mod $mod_set { - use super::{$struct_name, Passes, Sanitizer, LtoCli, LinkerPluginLto}; + use super::{$struct_name, Passes, Sanitizer, LtoCli, LinkerPluginLto, PgoGenerate}; use rustc_target::spec::{LinkerFlavor, MergeFunctions, PanicStrategy, RelroLevel}; use std::path::PathBuf; use std::str::FromStr; @@ -1087,6 +1104,14 @@ macro_rules! options { true } + fn parse_pgo_generate(slot: &mut PgoGenerate, v: Option<&str>) -> bool { + *slot = match v { + None => PgoGenerate::Enabled(None), + Some(path) => PgoGenerate::Enabled(Some(PathBuf::from(path))), + }; + true + } + fn parse_merge_functions(slot: &mut Option, v: Option<&str>) -> bool { match v.and_then(|s| MergeFunctions::from_str(s).ok()) { Some(mergefunc) => *slot = Some(mergefunc), @@ -1363,7 +1388,7 @@ options! {DebuggingOptions, DebuggingSetter, basic_debugging_options, "extra arguments to prepend to the linker invocation (space separated)"), profile: bool = (false, parse_bool, [TRACKED], "insert profiling code"), - pgo_gen: Option = (None, parse_opt_string, [TRACKED], + pgo_gen: PgoGenerate = (PgoGenerate::Disabled, parse_pgo_generate, [TRACKED], "Generate PGO profile data, to a given file, or to the default location if it's empty."), pgo_use: String = (String::new(), parse_string, [TRACKED], "Use PGO profile data from the given profile file."), @@ -1980,7 +2005,7 @@ pub fn build_session_options_and_crate_config( ); } - if debugging_opts.pgo_gen.is_some() && !debugging_opts.pgo_use.is_empty() { + if debugging_opts.pgo_gen.enabled() && !debugging_opts.pgo_use.is_empty() { early_error( error_format, "options `-Z pgo-gen` and `-Z pgo-use` are exclusive", @@ -2490,7 +2515,7 @@ mod dep_tracking { use std::path::PathBuf; use std::collections::hash_map::DefaultHasher; use super::{CrateType, DebugInfo, ErrorOutputType, OptLevel, OutputTypes, - Passes, Sanitizer, LtoCli, LinkerPluginLto}; + Passes, Sanitizer, LtoCli, LinkerPluginLto, PgoGenerate}; use syntax::feature_gate::UnstableFeatures; use rustc_target::spec::{MergeFunctions, PanicStrategy, RelroLevel, TargetTriple}; use syntax::edition::Edition; @@ -2558,6 +2583,7 @@ mod dep_tracking { impl_dep_tracking_hash_via_hash!(TargetTriple); impl_dep_tracking_hash_via_hash!(Edition); impl_dep_tracking_hash_via_hash!(LinkerPluginLto); + impl_dep_tracking_hash_via_hash!(PgoGenerate); impl_dep_tracking_hash_for_sortable_vec_of!(String); impl_dep_tracking_hash_for_sortable_vec_of!(PathBuf); @@ -2625,7 +2651,7 @@ mod tests { build_session_options_and_crate_config, to_crate_config }; - use crate::session::config::{LtoCli, LinkerPluginLto}; + use crate::session::config::{LtoCli, LinkerPluginLto, PgoGenerate}; use crate::session::build_session; use crate::session::search_paths::SearchPath; use std::collections::{BTreeMap, BTreeSet}; @@ -3124,7 +3150,7 @@ mod tests { assert!(reference.dep_tracking_hash() != opts.dep_tracking_hash()); opts = reference.clone(); - opts.debugging_opts.pgo_gen = Some(String::from("abc")); + opts.debugging_opts.pgo_gen = PgoGenerate::Enabled(None); assert_ne!(reference.dep_tracking_hash(), opts.dep_tracking_hash()); opts = reference.clone(); diff --git a/src/librustc_codegen_llvm/attributes.rs b/src/librustc_codegen_llvm/attributes.rs index 77fa34e74dd70..b15a64c966b1b 100644 --- a/src/librustc_codegen_llvm/attributes.rs +++ b/src/librustc_codegen_llvm/attributes.rs @@ -104,7 +104,7 @@ pub fn set_probestack(cx: &CodegenCx<'ll, '_>, llfn: &'ll Value) { } // probestack doesn't play nice either with pgo-gen. - if cx.sess().opts.debugging_opts.pgo_gen.is_some() { + if cx.sess().opts.debugging_opts.pgo_gen.enabled() { return; } diff --git a/src/librustc_codegen_llvm/back/link.rs b/src/librustc_codegen_llvm/back/link.rs index 19419a72b94dd..6a3c2adc85618 100644 --- a/src/librustc_codegen_llvm/back/link.rs +++ b/src/librustc_codegen_llvm/back/link.rs @@ -1014,7 +1014,7 @@ fn link_args(cmd: &mut dyn Linker, cmd.build_static_executable(); } - if sess.opts.debugging_opts.pgo_gen.is_some() { + if sess.opts.debugging_opts.pgo_gen.enabled() { cmd.pgo_gen(); } diff --git a/src/librustc_codegen_llvm/back/write.rs b/src/librustc_codegen_llvm/back/write.rs index f0ed201ad5c27..4de65325637ff 100644 --- a/src/librustc_codegen_llvm/back/write.rs +++ b/src/librustc_codegen_llvm/back/write.rs @@ -13,7 +13,7 @@ use crate::LlvmCodegenBackend; use rustc::hir::def_id::LOCAL_CRATE; use rustc_codegen_ssa::back::write::{CodegenContext, ModuleConfig, run_assembler}; use rustc_codegen_ssa::traits::*; -use rustc::session::config::{self, OutputType, Passes, Lto}; +use rustc::session::config::{self, OutputType, Passes, Lto, PgoGenerate}; use rustc::session::Session; use rustc::ty::TyCtxt; use rustc_codegen_ssa::{ModuleCodegen, CompiledModule}; @@ -26,7 +26,7 @@ use errors::{Handler, FatalError}; use std::ffi::{CString, CStr}; use std::fs; use std::io::{self, Write}; -use std::path::Path; +use std::path::{Path, PathBuf}; use std::str; use std::sync::Arc; use std::slice; @@ -708,10 +708,20 @@ pub unsafe fn with_llvm_pmb(llmod: &llvm::Module, .unwrap_or(llvm::CodeGenOptSizeNone); let inline_threshold = config.inline_threshold; - let pgo_gen_path = config.pgo_gen.as_ref().map(|s| { - let s = if s.is_empty() { "default_%m.profraw" } else { s }; - CString::new(s.as_bytes()).unwrap() - }); + let pgo_gen_path = match config.pgo_gen { + PgoGenerate::Enabled(ref opt_dir_path) => { + let path = if let Some(dir_path) = opt_dir_path { + dir_path.join("default_%m.profraw") + } else { + PathBuf::from("default_%m.profraw") + }; + + Some(CString::new(format!("{}", path.display())).unwrap()) + } + PgoGenerate::Disabled => { + None + } + }; let pgo_use_path = if config.pgo_use.is_empty() { None diff --git a/src/librustc_codegen_ssa/back/symbol_export.rs b/src/librustc_codegen_ssa/back/symbol_export.rs index 336f41b784a81..a55f783df43a3 100644 --- a/src/librustc_codegen_ssa/back/symbol_export.rs +++ b/src/librustc_codegen_ssa/back/symbol_export.rs @@ -209,7 +209,7 @@ fn exported_symbols_provider_local<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, } } - if tcx.sess.opts.debugging_opts.pgo_gen.is_some() { + if tcx.sess.opts.debugging_opts.pgo_gen.enabled() { // These are weak symbols that point to the profile version and the // profile name, which need to be treated as exported so LTO doesn't nix // them. diff --git a/src/librustc_codegen_ssa/back/write.rs b/src/librustc_codegen_ssa/back/write.rs index fa8c4177eafe2..7fc43f29701ad 100644 --- a/src/librustc_codegen_ssa/back/write.rs +++ b/src/librustc_codegen_ssa/back/write.rs @@ -12,7 +12,8 @@ use rustc_incremental::{copy_cgu_workproducts_to_incr_comp_cache_dir, use rustc::dep_graph::{WorkProduct, WorkProductId, WorkProductFileKind}; use rustc::dep_graph::cgu_reuse_tracker::CguReuseTracker; use rustc::middle::cstore::EncodedMetadata; -use rustc::session::config::{self, OutputFilenames, OutputType, Passes, Sanitizer, Lto}; +use rustc::session::config::{self, OutputFilenames, OutputType, Passes, Lto, + Sanitizer, PgoGenerate}; use rustc::session::Session; use rustc::util::nodemap::FxHashMap; use rustc::hir::def_id::{CrateNum, LOCAL_CRATE}; @@ -56,7 +57,7 @@ pub struct ModuleConfig { /// Some(level) to optimize binary size, or None to not affect program size. pub opt_size: Option, - pub pgo_gen: Option, + pub pgo_gen: PgoGenerate, pub pgo_use: String, // Flags indicating which outputs to produce. @@ -94,7 +95,7 @@ impl ModuleConfig { opt_level: None, opt_size: None, - pgo_gen: None, + pgo_gen: PgoGenerate::Disabled, pgo_use: String::new(), emit_no_opt_bc: false, diff --git a/src/librustc_metadata/creader.rs b/src/librustc_metadata/creader.rs index 36d9bf9f50dd5..66daa4518bef6 100644 --- a/src/librustc_metadata/creader.rs +++ b/src/librustc_metadata/creader.rs @@ -862,7 +862,7 @@ impl<'a> CrateLoader<'a> { fn inject_profiler_runtime(&mut self) { if self.sess.opts.debugging_opts.profile || - self.sess.opts.debugging_opts.pgo_gen.is_some() + self.sess.opts.debugging_opts.pgo_gen.enabled() { info!("loading profiler"); diff --git a/src/test/run-make-fulldeps/pgo-gen-lto/Makefile b/src/test/run-make-fulldeps/pgo-gen-lto/Makefile index e358314c0d09e..7c19961b1e420 100644 --- a/src/test/run-make-fulldeps/pgo-gen-lto/Makefile +++ b/src/test/run-make-fulldeps/pgo-gen-lto/Makefile @@ -1,10 +1,8 @@ -include ../tools.mk -# ignore-windows - all: ifeq ($(PROFILER_SUPPORT),1) - $(RUSTC) -Copt-level=3 -Clto=fat -Z pgo-gen="$(TMPDIR)/test.profraw" test.rs + $(RUSTC) -Copt-level=3 -Clto=fat -Z pgo-gen="$(TMPDIR)" test.rs $(call RUN,test) || exit 1 - [ -e "$(TMPDIR)/test.profraw" ] || (echo "No .profraw file"; exit 1) + [ -e "$(TMPDIR)"/default_*.profraw ] || (echo "No .profraw file"; exit 1) endif diff --git a/src/test/run-make-fulldeps/pgo-gen/Makefile b/src/test/run-make-fulldeps/pgo-gen/Makefile index 1961dff8d783e..0469c4443d85a 100644 --- a/src/test/run-make-fulldeps/pgo-gen/Makefile +++ b/src/test/run-make-fulldeps/pgo-gen/Makefile @@ -1,10 +1,8 @@ -include ../tools.mk -# ignore-windows - all: ifeq ($(PROFILER_SUPPORT),1) - $(RUSTC) -g -Z pgo-gen="$(TMPDIR)/test.profraw" test.rs + $(RUSTC) -g -Z pgo-gen="$(TMPDIR)" test.rs $(call RUN,test) || exit 1 - [ -e "$(TMPDIR)/test.profraw" ] || (echo "No .profraw file"; exit 1) + [ -e "$(TMPDIR)"/default_*.profraw ] || (echo "No .profraw file"; exit 1) endif From d84907bbccf0430470ba7b8f121bb4f924cdffa4 Mon Sep 17 00:00:00 2001 From: David Wood Date: Sun, 7 Apr 2019 19:56:41 +0200 Subject: [PATCH 04/11] Suggest macro import from crate root. This commit suggests importing a macro from the root of a crate as the intent may have been to import a macro from the definition location that was annotated with `#[macro_export]`. --- src/librustc_resolve/error_reporting.rs | 82 +++++++++++++++++++++---- src/librustc_resolve/lib.rs | 10 +++ src/librustc_resolve/resolve_imports.rs | 55 +++++++++-------- src/test/ui/issue-59764.fixed | 15 +++++ src/test/ui/issue-59764.rs | 1 + src/test/ui/issue-59764.stderr | 12 +++- 6 files changed, 134 insertions(+), 41 deletions(-) create mode 100644 src/test/ui/issue-59764.fixed diff --git a/src/librustc_resolve/error_reporting.rs b/src/librustc_resolve/error_reporting.rs index 461d02e515d38..99aa6e39f7b71 100644 --- a/src/librustc_resolve/error_reporting.rs +++ b/src/librustc_resolve/error_reporting.rs @@ -5,15 +5,16 @@ use log::debug; use rustc::hir::def::{Def, CtorKind, Namespace::*}; use rustc::hir::def_id::{CRATE_DEF_INDEX, DefId}; use rustc::session::config::nightly_options; -use syntax::ast::{Expr, ExprKind}; +use syntax::ast::{Expr, ExprKind, Ident}; +use syntax::ext::base::MacroKind; use syntax::symbol::keywords; use syntax_pos::Span; use crate::macros::ParentScope; -use crate::resolve_imports::ImportResolver; +use crate::resolve_imports::{ImportDirective, ImportResolver}; use crate::{import_candidate_to_enum_paths, is_self_type, is_self_value, path_names_to_string}; use crate::{AssocSuggestion, CrateLint, ImportSuggestion, ModuleOrUniformRoot, PathResult, - PathSource, Resolver, Segment}; + PathSource, Resolver, Segment, Suggestion}; impl<'a> Resolver<'a> { /// Handles error reporting for `smart_resolve_path_fragment` function. @@ -428,7 +429,7 @@ impl<'a, 'b:'a> ImportResolver<'a, 'b> { span: Span, mut path: Vec, parent_scope: &ParentScope<'b>, - ) -> Option<(Vec, Option)> { + ) -> Option<(Vec, Vec)> { debug!("make_path_suggestion: span={:?} path={:?}", span, path); match (path.get(0), path.get(1)) { @@ -463,13 +464,13 @@ impl<'a, 'b:'a> ImportResolver<'a, 'b> { span: Span, mut path: Vec, parent_scope: &ParentScope<'b>, - ) -> Option<(Vec, Option)> { + ) -> Option<(Vec, Vec)> { // Replace first ident with `self` and check if that is valid. path[0].ident.name = keywords::SelfLower.name(); let result = self.resolve_path(&path, None, parent_scope, false, span, CrateLint::No); debug!("make_missing_self_suggestion: path={:?} result={:?}", path, result); if let PathResult::Module(..) = result { - Some((path, None)) + Some((path, Vec::new())) } else { None } @@ -487,7 +488,7 @@ impl<'a, 'b:'a> ImportResolver<'a, 'b> { span: Span, mut path: Vec, parent_scope: &ParentScope<'b>, - ) -> Option<(Vec, Option)> { + ) -> Option<(Vec, Vec)> { // Replace first ident with `crate` and check if that is valid. path[0].ident.name = keywords::Crate.name(); let result = self.resolve_path(&path, None, parent_scope, false, span, CrateLint::No); @@ -495,11 +496,11 @@ impl<'a, 'b:'a> ImportResolver<'a, 'b> { if let PathResult::Module(..) = result { Some(( path, - Some( + vec![ "`use` statements changed in Rust 2018; read more at \ ".to_string() - ), + ], )) } else { None @@ -518,13 +519,13 @@ impl<'a, 'b:'a> ImportResolver<'a, 'b> { span: Span, mut path: Vec, parent_scope: &ParentScope<'b>, - ) -> Option<(Vec, Option)> { + ) -> Option<(Vec, Vec)> { // Replace first ident with `crate` and check if that is valid. path[0].ident.name = keywords::Super.name(); let result = self.resolve_path(&path, None, parent_scope, false, span, CrateLint::No); debug!("make_missing_super_suggestion: path={:?} result={:?}", path, result); if let PathResult::Module(..) = result { - Some((path, None)) + Some((path, Vec::new())) } else { None } @@ -545,7 +546,7 @@ impl<'a, 'b:'a> ImportResolver<'a, 'b> { span: Span, mut path: Vec, parent_scope: &ParentScope<'b>, - ) -> Option<(Vec, Option)> { + ) -> Option<(Vec, Vec)> { if path[1].ident.span.rust_2015() { return None; } @@ -564,10 +565,65 @@ impl<'a, 'b:'a> ImportResolver<'a, 'b> { debug!("make_external_crate_suggestion: name={:?} path={:?} result={:?}", name, path, result); if let PathResult::Module(..) = result { - return Some((path, None)); + return Some((path, Vec::new())); } } None } + + /// Suggests importing a macro from the root of the crate rather than a module within + /// the crate. + /// + /// ``` + /// help: a macro with this name exists at the root of the crate + /// | + /// LL | use issue_59764::makro; + /// | ^^^^^^^^^^^^^^^^^^ + /// | + /// = note: this could be because a macro annotated with `#[macro_export]` will be exported + /// at the root of the crate instead of the module where it is defined + /// ``` + pub(crate) fn check_for_module_export_macro( + &self, + directive: &'b ImportDirective<'b>, + module: ModuleOrUniformRoot<'b>, + ident: Ident, + ) -> Option<(Option, Vec)> { + let mut crate_module = if let ModuleOrUniformRoot::Module(module) = module { + module + } else { + return None; + }; + + while let Some(parent) = crate_module.parent { + crate_module = parent; + } + + if ModuleOrUniformRoot::same_def(ModuleOrUniformRoot::Module(crate_module), module) { + // Don't make a suggestion if the import was already from the root of the + // crate. + return None; + } + + let resolutions = crate_module.resolutions.borrow(); + let resolution = resolutions.get(&(ident, MacroNS))?; + let binding = resolution.borrow().binding()?; + if let Def::Macro(_, MacroKind::Bang) = binding.def() { + let name = crate_module.kind.name().unwrap(); + let suggestion = Some(( + directive.span, + String::from("a macro with this name exists at the root of the crate"), + format!("{}::{}", name, ident), + Applicability::MaybeIncorrect, + )); + let note = vec![ + "this could be because a macro annotated with `#[macro_export]` will be exported \ + at the root of the crate instead of the module where it is defined".to_string(), + ]; + Some((suggestion, note)) + } else { + None + } + } } diff --git a/src/librustc_resolve/lib.rs b/src/librustc_resolve/lib.rs index 5216156c0cab8..0fb1ca4298358 100644 --- a/src/librustc_resolve/lib.rs +++ b/src/librustc_resolve/lib.rs @@ -1091,6 +1091,16 @@ enum ModuleKind { Def(Def, Name), } +impl ModuleKind { + /// Get name of the module. + pub fn name(&self) -> Option { + match self { + ModuleKind::Block(..) => None, + ModuleKind::Def(_, name) => Some(*name), + } + } +} + /// One node in the tree of modules. pub struct ModuleData<'a> { parent: Option>, diff --git a/src/librustc_resolve/resolve_imports.rs b/src/librustc_resolve/resolve_imports.rs index 97b630ba5f298..6a01af5d1153a 100644 --- a/src/librustc_resolve/resolve_imports.rs +++ b/src/librustc_resolve/resolve_imports.rs @@ -145,7 +145,7 @@ pub struct NameResolution<'a> { impl<'a> NameResolution<'a> { // Returns the binding for the name if it is known or None if it not known. - fn binding(&self) -> Option<&'a NameBinding<'a>> { + pub(crate) fn binding(&self) -> Option<&'a NameBinding<'a>> { self.binding.and_then(|binding| { if !binding.is_glob_import() || self.single_imports.is_empty() { Some(binding) } else { None } @@ -636,7 +636,7 @@ impl<'a> Resolver<'a> { struct UnresolvedImportError { span: Span, label: Option, - note: Option, + note: Vec, suggestion: Option, } @@ -756,8 +756,8 @@ impl<'a, 'b:'a> ImportResolver<'a, 'b> { /// Upper limit on the number of `span_label` messages. const MAX_LABEL_COUNT: usize = 10; - let (span, msg, note) = if errors.is_empty() { - (span.unwrap(), "unresolved import".to_string(), None) + let (span, msg) = if errors.is_empty() { + (span.unwrap(), "unresolved import".to_string()) } else { let span = MultiSpan::from_spans( errors @@ -766,11 +766,6 @@ impl<'a, 'b:'a> ImportResolver<'a, 'b> { .collect(), ); - let note = errors - .iter() - .filter_map(|(_, err)| err.note.as_ref()) - .last(); - let paths = errors .iter() .map(|(path, _)| format!("`{}`", path)) @@ -782,13 +777,15 @@ impl<'a, 'b:'a> ImportResolver<'a, 'b> { paths.join(", "), ); - (span, msg, note) + (span, msg) }; let mut diag = struct_span_err!(self.resolver.session, span, E0432, "{}", &msg); - if let Some(note) = ¬e { - diag.note(note); + if let Some((_, UnresolvedImportError { note, .. })) = errors.iter().last() { + for message in note { + diag.note(&message); + } } for (_, err) in errors.into_iter().take(MAX_LABEL_COUNT) { @@ -961,7 +958,7 @@ impl<'a, 'b:'a> ImportResolver<'a, 'b> { UnresolvedImportError { span, label: Some(label), - note: None, + note: Vec::new(), suggestion, } } @@ -1006,7 +1003,7 @@ impl<'a, 'b:'a> ImportResolver<'a, 'b> { return Some(UnresolvedImportError { span: directive.span, label: Some(String::from("cannot glob-import a module into itself")), - note: None, + note: Vec::new(), suggestion: None, }); } @@ -1114,15 +1111,22 @@ impl<'a, 'b:'a> ImportResolver<'a, 'b> { } }); - let lev_suggestion = - find_best_match_for_name(names, &ident.as_str(), None).map(|suggestion| { - ( - ident.span, - String::from("a similar name exists in the module"), - suggestion.to_string(), - Applicability::MaybeIncorrect, - ) - }); + let (suggestion, note) = if let Some((suggestion, note)) = + self.check_for_module_export_macro(directive, module, ident) + { + + ( + suggestion.or_else(|| + find_best_match_for_name(names, &ident.as_str(), None) + .map(|suggestion| + (ident.span, String::from("a similar name exists in the module"), + suggestion.to_string(), Applicability::MaybeIncorrect) + )), + note, + ) + } else { + (None, Vec::new()) + }; let label = match module { ModuleOrUniformRoot::Module(module) => { @@ -1143,11 +1147,12 @@ impl<'a, 'b:'a> ImportResolver<'a, 'b> { } } }; + Some(UnresolvedImportError { span: directive.span, label: Some(label), - note: None, - suggestion: lev_suggestion, + note, + suggestion, }) } else { // `resolve_ident_in_module` reported a privacy error. diff --git a/src/test/ui/issue-59764.fixed b/src/test/ui/issue-59764.fixed new file mode 100644 index 0000000000000..d7a01ce5f4d74 --- /dev/null +++ b/src/test/ui/issue-59764.fixed @@ -0,0 +1,15 @@ +// aux-build:issue-59764.rs +// compile-flags:--extern issue_59764 +// edition:2018 +// run-rustfix + +use issue_59764::makro; +//~^ ERROR unresolved import `issue_59764::foo::makro` [E0432] + +makro!(bar); +//~^ ERROR cannot determine resolution for the macro `makro` + +fn main() { + bar(); + //~^ ERROR cannot find function `bar` in this scope [E0425] +} diff --git a/src/test/ui/issue-59764.rs b/src/test/ui/issue-59764.rs index 8ac9b9eaba181..e6f7205bfca33 100644 --- a/src/test/ui/issue-59764.rs +++ b/src/test/ui/issue-59764.rs @@ -1,6 +1,7 @@ // aux-build:issue-59764.rs // compile-flags:--extern issue_59764 // edition:2018 +// run-rustfix use issue_59764::foo::makro; //~^ ERROR unresolved import `issue_59764::foo::makro` [E0432] diff --git a/src/test/ui/issue-59764.stderr b/src/test/ui/issue-59764.stderr index a428488b009ec..6b3237e9272c5 100644 --- a/src/test/ui/issue-59764.stderr +++ b/src/test/ui/issue-59764.stderr @@ -1,11 +1,17 @@ error[E0432]: unresolved import `issue_59764::foo::makro` - --> $DIR/issue-59764.rs:5:5 + --> $DIR/issue-59764.rs:6:5 | LL | use issue_59764::foo::makro; | ^^^^^^^^^^^^^^^^^^^^^^^ no `makro` in `foo` + | + = note: this could be because a macro annotated with `#[macro_export]` will be exported at the root of the crate instead of the module where it is defined +help: a macro with this name exists at the root of the crate + | +LL | use issue_59764::makro; + | ^^^^^^^^^^^^^^^^^^ error: cannot determine resolution for the macro `makro` - --> $DIR/issue-59764.rs:8:1 + --> $DIR/issue-59764.rs:9:1 | LL | makro!(bar); | ^^^^^ @@ -13,7 +19,7 @@ LL | makro!(bar); = note: import resolution is stuck, try simplifying macro imports error[E0425]: cannot find function `bar` in this scope - --> $DIR/issue-59764.rs:12:5 + --> $DIR/issue-59764.rs:13:5 | LL | bar(); | ^^^ not found in this scope From d589cf911109149564c8898ad1cb1d906f91caea Mon Sep 17 00:00:00 2001 From: David Wood Date: Sun, 7 Apr 2019 20:34:21 +0200 Subject: [PATCH 05/11] Handle renamed imports. This commit extends the suggestion to handle imports that are aliased to another name. --- src/librustc_resolve/error_reporting.rs | 11 ++++++++--- src/librustc_resolve/resolve_imports.rs | 26 +++++++++++-------------- src/test/ui/issue-59764.fixed | 15 ++++++++++++++ src/test/ui/issue-59764.rs | 15 ++++++++++++++ src/test/ui/issue-59764.stderr | 20 +++++++++++++++---- 5 files changed, 65 insertions(+), 22 deletions(-) diff --git a/src/librustc_resolve/error_reporting.rs b/src/librustc_resolve/error_reporting.rs index 99aa6e39f7b71..b7ccbbf607913 100644 --- a/src/librustc_resolve/error_reporting.rs +++ b/src/librustc_resolve/error_reporting.rs @@ -11,7 +11,7 @@ use syntax::symbol::keywords; use syntax_pos::Span; use crate::macros::ParentScope; -use crate::resolve_imports::{ImportDirective, ImportResolver}; +use crate::resolve_imports::{ImportDirective, ImportDirectiveSubclass, ImportResolver}; use crate::{import_candidate_to_enum_paths, is_self_type, is_self_value, path_names_to_string}; use crate::{AssocSuggestion, CrateLint, ImportSuggestion, ModuleOrUniformRoot, PathResult, PathSource, Resolver, Segment, Suggestion}; @@ -610,11 +610,16 @@ impl<'a, 'b:'a> ImportResolver<'a, 'b> { let resolution = resolutions.get(&(ident, MacroNS))?; let binding = resolution.borrow().binding()?; if let Def::Macro(_, MacroKind::Bang) = binding.def() { - let name = crate_module.kind.name().unwrap(); + let module_name = crate_module.kind.name().unwrap(); + let import = match directive.subclass { + ImportDirectiveSubclass::SingleImport { source, target, .. } if source != target => + format!("{} as {}", source, target), + _ => format!("{}", ident), + }; let suggestion = Some(( directive.span, String::from("a macro with this name exists at the root of the crate"), - format!("{}::{}", name, ident), + format!("{}::{}", module_name, import), Applicability::MaybeIncorrect, )); let note = vec![ diff --git a/src/librustc_resolve/resolve_imports.rs b/src/librustc_resolve/resolve_imports.rs index 6a01af5d1153a..ef5f0f54ee369 100644 --- a/src/librustc_resolve/resolve_imports.rs +++ b/src/librustc_resolve/resolve_imports.rs @@ -1111,21 +1111,17 @@ impl<'a, 'b:'a> ImportResolver<'a, 'b> { } }); - let (suggestion, note) = if let Some((suggestion, note)) = - self.check_for_module_export_macro(directive, module, ident) - { - - ( - suggestion.or_else(|| - find_best_match_for_name(names, &ident.as_str(), None) - .map(|suggestion| - (ident.span, String::from("a similar name exists in the module"), - suggestion.to_string(), Applicability::MaybeIncorrect) - )), - note, - ) - } else { - (None, Vec::new()) + let lev_suggestion = find_best_match_for_name(names, &ident.as_str(), None) + .map(|suggestion| + (ident.span, String::from("a similar name exists in the module"), + suggestion.to_string(), Applicability::MaybeIncorrect) + ); + + let (suggestion, note) = match self.check_for_module_export_macro( + directive, module, ident, + ) { + Some((suggestion, note)) => (suggestion.or(lev_suggestion), note), + _ => (lev_suggestion, Vec::new()), }; let label = match module { diff --git a/src/test/ui/issue-59764.fixed b/src/test/ui/issue-59764.fixed index d7a01ce5f4d74..34b9fc4edd9b7 100644 --- a/src/test/ui/issue-59764.fixed +++ b/src/test/ui/issue-59764.fixed @@ -3,6 +3,21 @@ // edition:2018 // run-rustfix +#![allow(warnings)] + +// This tests the suggestion to import macros from the root of a crate. This aims to capture +// the case where a user attempts to import a macro from the definition location instead of the +// root of the crate and the macro is annotated with `#![macro_export]`. + +// Edge cases.. + +mod renamed_import { + use issue_59764::makro as baz; + //~^ ERROR unresolved import `issue_59764::foo::makro` [E0432] +} + +// Simple case.. + use issue_59764::makro; //~^ ERROR unresolved import `issue_59764::foo::makro` [E0432] diff --git a/src/test/ui/issue-59764.rs b/src/test/ui/issue-59764.rs index e6f7205bfca33..b33b8e0cf5c57 100644 --- a/src/test/ui/issue-59764.rs +++ b/src/test/ui/issue-59764.rs @@ -3,6 +3,21 @@ // edition:2018 // run-rustfix +#![allow(warnings)] + +// This tests the suggestion to import macros from the root of a crate. This aims to capture +// the case where a user attempts to import a macro from the definition location instead of the +// root of the crate and the macro is annotated with `#![macro_export]`. + +// Edge cases.. + +mod renamed_import { + use issue_59764::foo::makro as baz; + //~^ ERROR unresolved import `issue_59764::foo::makro` [E0432] +} + +// Simple case.. + use issue_59764::foo::makro; //~^ ERROR unresolved import `issue_59764::foo::makro` [E0432] diff --git a/src/test/ui/issue-59764.stderr b/src/test/ui/issue-59764.stderr index 6b3237e9272c5..8bb4d03fc5dd6 100644 --- a/src/test/ui/issue-59764.stderr +++ b/src/test/ui/issue-59764.stderr @@ -1,5 +1,17 @@ error[E0432]: unresolved import `issue_59764::foo::makro` - --> $DIR/issue-59764.rs:6:5 + --> $DIR/issue-59764.rs:15:9 + | +LL | use issue_59764::foo::makro as baz; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ no `makro` in `foo` + | + = note: this could be because a macro annotated with `#[macro_export]` will be exported at the root of the crate instead of the module where it is defined +help: a macro with this name exists at the root of the crate + | +LL | use issue_59764::makro as baz; + | ^^^^^^^^^^^^^^^^^^^^^^^^^ + +error[E0432]: unresolved import `issue_59764::foo::makro` + --> $DIR/issue-59764.rs:21:5 | LL | use issue_59764::foo::makro; | ^^^^^^^^^^^^^^^^^^^^^^^ no `makro` in `foo` @@ -11,7 +23,7 @@ LL | use issue_59764::makro; | ^^^^^^^^^^^^^^^^^^ error: cannot determine resolution for the macro `makro` - --> $DIR/issue-59764.rs:9:1 + --> $DIR/issue-59764.rs:24:1 | LL | makro!(bar); | ^^^^^ @@ -19,12 +31,12 @@ LL | makro!(bar); = note: import resolution is stuck, try simplifying macro imports error[E0425]: cannot find function `bar` in this scope - --> $DIR/issue-59764.rs:13:5 + --> $DIR/issue-59764.rs:28:5 | LL | bar(); | ^^^ not found in this scope -error: aborting due to 3 previous errors +error: aborting due to 4 previous errors Some errors occurred: E0425, E0432. For more information about an error, try `rustc --explain E0425`. From 7c955409e3cb06b2a5c008e80c6856cb25a74b33 Mon Sep 17 00:00:00 2001 From: David Wood Date: Sun, 7 Apr 2019 23:18:13 +0200 Subject: [PATCH 06/11] Handle edge cases. This commit introduces more dirty span manipulation into the compiler in order to handle the various edge cases in moving/renaming the macro import so it is at the root of the import. --- src/librustc_resolve/error_reporting.rs | 230 +++++++++++++++++++++++- src/librustc_resolve/lib.rs | 77 ++------ src/test/ui/auxiliary/issue-59764.rs | 10 ++ src/test/ui/issue-59764.fixed | 93 ++++++++++ src/test/ui/issue-59764.rs | 93 ++++++++++ src/test/ui/issue-59764.stderr | 195 +++++++++++++++++++- 6 files changed, 625 insertions(+), 73 deletions(-) diff --git a/src/librustc_resolve/error_reporting.rs b/src/librustc_resolve/error_reporting.rs index b7ccbbf607913..eea8cb80f69ef 100644 --- a/src/librustc_resolve/error_reporting.rs +++ b/src/librustc_resolve/error_reporting.rs @@ -4,11 +4,11 @@ use errors::{Applicability, DiagnosticBuilder, DiagnosticId}; use log::debug; use rustc::hir::def::{Def, CtorKind, Namespace::*}; use rustc::hir::def_id::{CRATE_DEF_INDEX, DefId}; -use rustc::session::config::nightly_options; +use rustc::session::{Session, config::nightly_options}; use syntax::ast::{Expr, ExprKind, Ident}; use syntax::ext::base::MacroKind; -use syntax::symbol::keywords; -use syntax_pos::Span; +use syntax::symbol::{Symbol, keywords}; +use syntax_pos::{BytePos, Span}; use crate::macros::ParentScope; use crate::resolve_imports::{ImportDirective, ImportDirectiveSubclass, ImportResolver}; @@ -616,10 +616,84 @@ impl<'a, 'b:'a> ImportResolver<'a, 'b> { format!("{} as {}", source, target), _ => format!("{}", ident), }; + + // Assume this is the easy case of `use issue_59764::foo::makro;` and just remove + // intermediate segments. + let (mut span, mut correction) = (directive.span, + format!("{}::{}", module_name, import)); + + if directive.is_nested() { + span = directive.use_span; + + // Find the binding span (and any trailing commas and spaces). + // ie. `use a::b::{c, d, e};` + // ^^^ + let (found_closing_brace, binding_span) = find_span_of_binding_until_next_binding( + self.resolver.session, directive.span, directive.use_span, + ); + debug!("check_for_module_export_macro: found_closing_brace={:?} binding_span={:?}", + found_closing_brace, binding_span); + + let mut removal_span = binding_span; + if found_closing_brace { + // If the binding span ended with a closing brace, as in the below example: + // ie. `use a::b::{c, d};` + // ^ + // Then expand the span of characters to remove to include the previous + // binding's trailing comma. + // ie. `use a::b::{c, d};` + // ^^^ + if let Some(previous_span) = extend_span_to_previous_binding( + self.resolver.session, binding_span, + ) { + debug!("check_for_module_export_macro: previous_span={:?}", previous_span); + removal_span = removal_span.with_lo(previous_span.lo()); + } + } + debug!("check_for_module_export_macro: removal_span={:?}", removal_span); + + // Find the span after the crate name and if it has nested imports immediatately + // after the crate name already. + // ie. `use a::b::{c, d};` + // ^^^^^^^^^ + // or `use a::{b, c, d}};` + // ^^^^^^^^^^^ + let (has_nested, after_crate_name) = find_span_immediately_after_crate_name( + self.resolver.session, module_name, directive.use_span, + ); + debug!("check_for_module_export_macro: has_nested={:?} after_crate_name={:?}", + has_nested, after_crate_name); + + let source_map = self.resolver.session.source_map(); + + // Remove two bytes at the end to keep all but the `};` characters. + // ie. `{b::{c, d}, e::{f, g}};` + // ^^^^^^^^^^^^^^^^^^^^^ + let end_bytes = BytePos(if has_nested { 2 } else { 1 }); + let mut remaining_span = after_crate_name.with_hi( + after_crate_name.hi() - end_bytes); + if has_nested { + // Remove two bytes at the start to keep all but the initial `{` character. + // ie. `{b::{c, d}, e::{f, g}` + // ^^^^^^^^^^^^^^^^^^^^ + remaining_span = remaining_span.with_lo(after_crate_name.lo() + BytePos(1)); + } + + // Calculate the number of characters into a snippet to remove the removal + // span. + let lo = removal_span.lo() - remaining_span.lo(); + let hi = lo + (removal_span.hi() - removal_span.lo()); + if let Ok(mut remaining) = source_map.span_to_snippet(remaining_span) { + // Remove the original location of the binding. + remaining.replace_range((lo.0 as usize)..(hi.0 as usize), ""); + correction = format!("use {}::{{{}, {}}};", module_name, import, remaining); + } + } + let suggestion = Some(( - directive.span, + span, String::from("a macro with this name exists at the root of the crate"), - format!("{}::{}", module_name, import), + correction, Applicability::MaybeIncorrect, )); let note = vec![ @@ -632,3 +706,149 @@ impl<'a, 'b:'a> ImportResolver<'a, 'b> { } } } + +/// Given a `binding_span` of a binding within a use statement: +/// +/// ``` +/// use foo::{a, b, c}; +/// ^ +/// ``` +/// +/// then return the span until the next binding or the end of the statement: +/// +/// ``` +/// use foo::{a, b, c}; +/// ^^^ +/// ``` +pub(crate) fn find_span_of_binding_until_next_binding( + sess: &Session, + binding_span: Span, + use_span: Span, +) -> (bool, Span) { + let source_map = sess.source_map(); + + // Find the span of everything after the binding. + // ie. `a, e};` or `a};` + let binding_until_end = binding_span.with_hi(use_span.hi()); + + // Find everything after the binding but not including the binding. + // ie. `, e};` or `};` + let after_binding_until_end = binding_until_end.with_lo(binding_span.hi()); + + // Keep characters in the span until we encounter something that isn't a comma or + // whitespace. + // ie. `, ` or ``. + // + // Also note whether a closing brace character was encountered. If there + // was, then later go backwards to remove any trailing commas that are left. + let mut found_closing_brace = false; + let after_binding_until_next_binding = source_map.span_take_while( + after_binding_until_end, + |&ch| { + if ch == '}' { found_closing_brace = true; } + ch == ' ' || ch == ',' + } + ); + + // Combine the two spans. + // ie. `a, ` or `a`. + // + // Removing these would leave `issue_52891::{d, e};` or `issue_52891::{d, e, };` + let span = binding_span.with_hi(after_binding_until_next_binding.hi()); + + (found_closing_brace, span) +} + +/// Given a `binding_span`, return the span through to the comma or opening brace of the previous +/// binding. +/// +/// ``` +/// use foo::a::{a, b, c}; +/// ^^--- binding span +/// | +/// returned span +/// +/// use foo::{a, b, c}; +/// --- binding span +/// ``` +pub(crate) fn extend_span_to_previous_binding( + sess: &Session, + binding_span: Span, +) -> Option { + let source_map = sess.source_map(); + + // `prev_source` will contain all of the source that came before the span. + // Then split based on a command and take the first (ie. closest to our span) + // snippet. In the example, this is a space. + let prev_source = source_map.span_to_prev_source(binding_span).ok()?; + + let prev_comma = prev_source.rsplit(',').collect::>(); + let prev_starting_brace = prev_source.rsplit('{').collect::>(); + if prev_comma.len() <= 1 || prev_starting_brace.len() <= 1 { + return None; + } + + let prev_comma = prev_comma.first().unwrap(); + let prev_starting_brace = prev_starting_brace.first().unwrap(); + + // If the amount of source code before the comma is greater than + // the amount of source code before the starting brace then we've only + // got one item in the nested item (eg. `issue_52891::{self}`). + if prev_comma.len() > prev_starting_brace.len() { + return None; + } + + Some(binding_span.with_lo(BytePos( + // Take away the number of bytes for the characters we've found and an + // extra for the comma. + binding_span.lo().0 - (prev_comma.as_bytes().len() as u32) - 1 + ))) +} + +/// Given a `use_span` of a binding within a use statement, returns the highlighted span and if +/// it is a nested use tree. +/// +/// ``` +/// use foo::a::{b, c}; +/// ^^^^^^^^^^ // false +/// +/// use foo::{a, b, c}; +/// ^^^^^^^^^^ // true +/// +/// use foo::{a, b::{c, d}}; +/// ^^^^^^^^^^^^^^^ // true +/// ``` +fn find_span_immediately_after_crate_name( + sess: &Session, + module_name: Symbol, + use_span: Span, +) -> (bool, Span) { + debug!("find_span_immediately_after_crate_name: module_name={:?} use_span={:?}", + module_name, use_span); + let source_map = sess.source_map(); + + // Get position of the first `{` character for the use statement. + // ie. `use foo::{a, b::{c, d}};` + // ^ + let pos_of_use_tree_left_bracket = source_map.span_until_char(use_span, '{').hi(); + debug!("find_span_immediately_after_crate_name: pos_of_use_tree_left_bracket={:?}", + pos_of_use_tree_left_bracket); + + // Calculate the expected difference between the first `{` character and the start of a + // use statement. + // ie. `use foo::{..};` + // ^^^^ + // | ^^^ + // 4 | ^^ + // 3 | + // 2 + let expected_difference = BytePos((module_name.as_str().len() + 4 + 2) as u32); + debug!("find_span_immediately_after_crate_name: expected_difference={:?}", + expected_difference); + let actual_difference = pos_of_use_tree_left_bracket - use_span.lo(); + debug!("find_span_immediately_after_crate_name: actual_difference={:?}", + actual_difference); + + (expected_difference == actual_difference, + use_span.with_lo(use_span.lo() + expected_difference)) +} diff --git a/src/librustc_resolve/lib.rs b/src/librustc_resolve/lib.rs index 0fb1ca4298358..b33356c2ba7ae 100644 --- a/src/librustc_resolve/lib.rs +++ b/src/librustc_resolve/lib.rs @@ -50,7 +50,7 @@ use syntax::ast::{QSelf, TraitItemKind, TraitRef, Ty, TyKind}; use syntax::ptr::P; use syntax::{span_err, struct_span_err, unwrap_or, walk_list}; -use syntax_pos::{BytePos, Span, DUMMY_SP, MultiSpan}; +use syntax_pos::{Span, DUMMY_SP, MultiSpan}; use errors::{Applicability, DiagnosticBuilder, DiagnosticId}; use log::debug; @@ -62,6 +62,7 @@ use std::mem::replace; use rustc_data_structures::ptr_key::PtrKey; use rustc_data_structures::sync::Lrc; +use error_reporting::{find_span_of_binding_until_next_binding, extend_span_to_previous_binding}; use resolve_imports::{ImportDirective, ImportDirectiveSubclass, NameResolution, ImportResolver}; use macros::{InvocationData, LegacyBinding, ParentScope}; @@ -5144,7 +5145,6 @@ impl<'a> Resolver<'a> { ) { assert!(directive.is_nested()); let message = "remove unnecessary import"; - let source_map = self.session.source_map(); // Two examples will be used to illustrate the span manipulations we're doing: // @@ -5153,73 +5153,24 @@ impl<'a> Resolver<'a> { // - Given `use issue_52891::{d, e, a};` where `a` is a duplicate then `binding_span` is // `a` and `directive.use_span` is `issue_52891::{d, e, a};`. - // Find the span of everything after the binding. - // ie. `a, e};` or `a};` - let binding_until_end = binding_span.with_hi(directive.use_span.hi()); - - // Find everything after the binding but not including the binding. - // ie. `, e};` or `};` - let after_binding_until_end = binding_until_end.with_lo(binding_span.hi()); - - // Keep characters in the span until we encounter something that isn't a comma or - // whitespace. - // ie. `, ` or ``. - // - // Also note whether a closing brace character was encountered. If there - // was, then later go backwards to remove any trailing commas that are left. - let mut found_closing_brace = false; - let after_binding_until_next_binding = source_map.span_take_while( - after_binding_until_end, - |&ch| { - if ch == '}' { found_closing_brace = true; } - ch == ' ' || ch == ',' - } + let (found_closing_brace, span) = find_span_of_binding_until_next_binding( + self.session, binding_span, directive.use_span, ); - // Combine the two spans. - // ie. `a, ` or `a`. - // - // Removing these would leave `issue_52891::{d, e};` or `issue_52891::{d, e, };` - let span = binding_span.with_hi(after_binding_until_next_binding.hi()); - // If there was a closing brace then identify the span to remove any trailing commas from // previous imports. if found_closing_brace { - if let Ok(prev_source) = source_map.span_to_prev_source(span) { - // `prev_source` will contain all of the source that came before the span. - // Then split based on a command and take the first (ie. closest to our span) - // snippet. In the example, this is a space. - let prev_comma = prev_source.rsplit(',').collect::>(); - let prev_starting_brace = prev_source.rsplit('{').collect::>(); - if prev_comma.len() > 1 && prev_starting_brace.len() > 1 { - let prev_comma = prev_comma.first().unwrap(); - let prev_starting_brace = prev_starting_brace.first().unwrap(); - - // If the amount of source code before the comma is greater than - // the amount of source code before the starting brace then we've only - // got one item in the nested item (eg. `issue_52891::{self}`). - if prev_comma.len() > prev_starting_brace.len() { - // So just remove the entire line... - err.span_suggestion( - directive.use_span_with_attributes, - message, - String::new(), - Applicability::MaybeIncorrect, - ); - return; - } - - let span = span.with_lo(BytePos( - // Take away the number of bytes for the characters we've found and an - // extra for the comma. - span.lo().0 - (prev_comma.as_bytes().len() as u32) - 1 - )); - err.tool_only_span_suggestion( - span, message, String::new(), Applicability::MaybeIncorrect, - ); - return; - } + if let Some(span) = extend_span_to_previous_binding(self.session, span) { + err.tool_only_span_suggestion(span, message, String::new(), + Applicability::MaybeIncorrect); + } else { + // Remove the entire line if we cannot extend the span back, this indicates a + // `issue_52891::{self}` case. + err.span_suggestion(directive.use_span_with_attributes, message, String::new(), + Applicability::MaybeIncorrect); } + + return; } err.span_suggestion(span, message, String::new(), Applicability::MachineApplicable); diff --git a/src/test/ui/auxiliary/issue-59764.rs b/src/test/ui/auxiliary/issue-59764.rs index 0243ef5c6c74a..a92eed968d060 100644 --- a/src/test/ui/auxiliary/issue-59764.rs +++ b/src/test/ui/auxiliary/issue-59764.rs @@ -5,4 +5,14 @@ pub mod foo { fn $foo() { } } } + + pub fn baz() {} + + pub fn foobar() {} + + pub mod barbaz { + pub fn barfoo() {} + } } + +pub fn foobaz() {} diff --git a/src/test/ui/issue-59764.fixed b/src/test/ui/issue-59764.fixed index 34b9fc4edd9b7..ff193fce7eb1d 100644 --- a/src/test/ui/issue-59764.fixed +++ b/src/test/ui/issue-59764.fixed @@ -11,11 +11,104 @@ // Edge cases.. +mod multiple_imports_same_line_at_end { + use issue_59764::{makro, foo::{baz}}; + //~^ ERROR unresolved import `issue_59764::foo::makro` [E0432] +} + +mod multiple_imports_multiline_at_end_trailing_comma { + use issue_59764::{makro, foo::{ + baz, + //~ ERROR unresolved import `issue_59764::foo::makro` [E0432] + }}; +} + +mod multiple_imports_multiline_at_end { + use issue_59764::{makro, foo::{ + baz, + //~ ERROR unresolved import `issue_59764::foo::makro` [E0432] + }}; +} + +mod multiple_imports_same_line_in_middle { + use issue_59764::{makro, foo::{baz, foobar}}; + //~^ ERROR unresolved import `issue_59764::foo::makro` [E0432] +} + +mod multiple_imports_multiline_in_middle_trailing_comma { + use issue_59764::{makro, foo::{ + baz, + //~ ERROR unresolved import `issue_59764::foo::makro` [E0432] + foobar, + }}; +} + +mod multiple_imports_multiline_in_middle { + use issue_59764::{makro, foo::{ + baz, + //~ ERROR unresolved import `issue_59764::foo::makro` [E0432] + foobar + }}; +} + +mod nested_imports { + use issue_59764::{makro, foobaz}; + //~^ ERROR unresolved import `issue_59764::foo::makro` [E0432] +} + +mod nested_multiple_imports { + use issue_59764::{makro, foobaz, foo::{baz}}; + //~^ ERROR unresolved import `issue_59764::foo::makro` [E0432] +} + +mod nested_multiline_multiple_imports_trailing_comma { + use issue_59764::{makro, + foobaz, + foo::{ + baz, + //~ ERROR unresolved import `issue_59764::foo::makro` [E0432] + }, + }; +} + +mod nested_multiline_multiple_imports { + use issue_59764::{makro, + foobaz, + foo::{ + baz, + //~ ERROR unresolved import `issue_59764::foo::makro` [E0432] + } + }; +} + +mod doubly_nested_multiple_imports { + use issue_59764::{makro, foobaz, foo::{baz, barbaz::{barfoo}}}; + //~^ ERROR unresolved import `issue_59764::foo::makro` [E0432] +} + +mod doubly_multiline_nested_multiple_imports { + use issue_59764::{makro, + foobaz, + foo::{ + baz, + //~ ERROR unresolved import `issue_59764::foo::makro` [E0432] + barbaz::{ + barfoo, + } + } + }; +} + mod renamed_import { use issue_59764::makro as baz; //~^ ERROR unresolved import `issue_59764::foo::makro` [E0432] } +mod renamed_multiple_imports { + use issue_59764::{makro as foobar, foo::{baz}}; + //~^ ERROR unresolved import `issue_59764::foo::makro` [E0432] +} + // Simple case.. use issue_59764::makro; diff --git a/src/test/ui/issue-59764.rs b/src/test/ui/issue-59764.rs index b33b8e0cf5c57..c68471adaad56 100644 --- a/src/test/ui/issue-59764.rs +++ b/src/test/ui/issue-59764.rs @@ -11,11 +11,104 @@ // Edge cases.. +mod multiple_imports_same_line_at_end { + use issue_59764::foo::{baz, makro}; + //~^ ERROR unresolved import `issue_59764::foo::makro` [E0432] +} + +mod multiple_imports_multiline_at_end_trailing_comma { + use issue_59764::foo::{ + baz, + makro, //~ ERROR unresolved import `issue_59764::foo::makro` [E0432] + }; +} + +mod multiple_imports_multiline_at_end { + use issue_59764::foo::{ + baz, + makro //~ ERROR unresolved import `issue_59764::foo::makro` [E0432] + }; +} + +mod multiple_imports_same_line_in_middle { + use issue_59764::foo::{baz, makro, foobar}; + //~^ ERROR unresolved import `issue_59764::foo::makro` [E0432] +} + +mod multiple_imports_multiline_in_middle_trailing_comma { + use issue_59764::foo::{ + baz, + makro, //~ ERROR unresolved import `issue_59764::foo::makro` [E0432] + foobar, + }; +} + +mod multiple_imports_multiline_in_middle { + use issue_59764::foo::{ + baz, + makro, //~ ERROR unresolved import `issue_59764::foo::makro` [E0432] + foobar + }; +} + +mod nested_imports { + use issue_59764::{foobaz, foo::makro}; + //~^ ERROR unresolved import `issue_59764::foo::makro` [E0432] +} + +mod nested_multiple_imports { + use issue_59764::{foobaz, foo::{baz, makro}}; + //~^ ERROR unresolved import `issue_59764::foo::makro` [E0432] +} + +mod nested_multiline_multiple_imports_trailing_comma { + use issue_59764::{ + foobaz, + foo::{ + baz, + makro, //~ ERROR unresolved import `issue_59764::foo::makro` [E0432] + }, + }; +} + +mod nested_multiline_multiple_imports { + use issue_59764::{ + foobaz, + foo::{ + baz, + makro //~ ERROR unresolved import `issue_59764::foo::makro` [E0432] + } + }; +} + +mod doubly_nested_multiple_imports { + use issue_59764::{foobaz, foo::{baz, makro, barbaz::{barfoo}}}; + //~^ ERROR unresolved import `issue_59764::foo::makro` [E0432] +} + +mod doubly_multiline_nested_multiple_imports { + use issue_59764::{ + foobaz, + foo::{ + baz, + makro, //~ ERROR unresolved import `issue_59764::foo::makro` [E0432] + barbaz::{ + barfoo, + } + } + }; +} + mod renamed_import { use issue_59764::foo::makro as baz; //~^ ERROR unresolved import `issue_59764::foo::makro` [E0432] } +mod renamed_multiple_imports { + use issue_59764::foo::{baz, makro as foobar}; + //~^ ERROR unresolved import `issue_59764::foo::makro` [E0432] +} + // Simple case.. use issue_59764::foo::makro; diff --git a/src/test/ui/issue-59764.stderr b/src/test/ui/issue-59764.stderr index 8bb4d03fc5dd6..fe93547318468 100644 --- a/src/test/ui/issue-59764.stderr +++ b/src/test/ui/issue-59764.stderr @@ -1,5 +1,178 @@ error[E0432]: unresolved import `issue_59764::foo::makro` - --> $DIR/issue-59764.rs:15:9 + --> $DIR/issue-59764.rs:15:33 + | +LL | use issue_59764::foo::{baz, makro}; + | ^^^^^ no `makro` in `foo` + | + = note: this could be because a macro annotated with `#[macro_export]` will be exported at the root of the crate instead of the module where it is defined +help: a macro with this name exists at the root of the crate + | +LL | use issue_59764::{makro, foo::{baz}}; + | + +error[E0432]: unresolved import `issue_59764::foo::makro` + --> $DIR/issue-59764.rs:22:9 + | +LL | makro, + | ^^^^^ no `makro` in `foo` + | + = note: this could be because a macro annotated with `#[macro_export]` will be exported at the root of the crate instead of the module where it is defined +help: a macro with this name exists at the root of the crate + | +LL | use issue_59764::{makro, foo::{ +LL | baz, +LL | +LL | }}; + | + +error[E0432]: unresolved import `issue_59764::foo::makro` + --> $DIR/issue-59764.rs:29:9 + | +LL | makro + | ^^^^^ no `makro` in `foo` + | + = note: this could be because a macro annotated with `#[macro_export]` will be exported at the root of the crate instead of the module where it is defined +help: a macro with this name exists at the root of the crate + | +LL | use issue_59764::{makro, foo::{ +LL | baz, +LL | +LL | }}; + | + +error[E0432]: unresolved import `issue_59764::foo::makro` + --> $DIR/issue-59764.rs:34:33 + | +LL | use issue_59764::foo::{baz, makro, foobar}; + | ^^^^^ no `makro` in `foo` + | + = note: this could be because a macro annotated with `#[macro_export]` will be exported at the root of the crate instead of the module where it is defined +help: a macro with this name exists at the root of the crate + | +LL | use issue_59764::{makro, foo::{baz, foobar}}; + | + +error[E0432]: unresolved import `issue_59764::foo::makro` + --> $DIR/issue-59764.rs:41:9 + | +LL | makro, + | ^^^^^ no `makro` in `foo` + | + = note: this could be because a macro annotated with `#[macro_export]` will be exported at the root of the crate instead of the module where it is defined +help: a macro with this name exists at the root of the crate + | +LL | use issue_59764::{makro, foo::{ +LL | baz, +LL | +LL | foobar, +LL | }}; + | + +error[E0432]: unresolved import `issue_59764::foo::makro` + --> $DIR/issue-59764.rs:49:9 + | +LL | makro, + | ^^^^^ no `makro` in `foo` + | + = note: this could be because a macro annotated with `#[macro_export]` will be exported at the root of the crate instead of the module where it is defined +help: a macro with this name exists at the root of the crate + | +LL | use issue_59764::{makro, foo::{ +LL | baz, +LL | +LL | foobar +LL | }}; + | + +error[E0432]: unresolved import `issue_59764::foo::makro` + --> $DIR/issue-59764.rs:55:31 + | +LL | use issue_59764::{foobaz, foo::makro}; + | ^^^^^^^^^^ no `makro` in `foo` + | + = note: this could be because a macro annotated with `#[macro_export]` will be exported at the root of the crate instead of the module where it is defined +help: a macro with this name exists at the root of the crate + | +LL | use issue_59764::{makro, foobaz}; + | + +error[E0432]: unresolved import `issue_59764::foo::makro` + --> $DIR/issue-59764.rs:60:42 + | +LL | use issue_59764::{foobaz, foo::{baz, makro}}; + | ^^^^^ no `makro` in `foo` + | + = note: this could be because a macro annotated with `#[macro_export]` will be exported at the root of the crate instead of the module where it is defined +help: a macro with this name exists at the root of the crate + | +LL | use issue_59764::{makro, foobaz, foo::{baz}}; + | + +error[E0432]: unresolved import `issue_59764::foo::makro` + --> $DIR/issue-59764.rs:69:13 + | +LL | makro, + | ^^^^^ no `makro` in `foo` + | + = note: this could be because a macro annotated with `#[macro_export]` will be exported at the root of the crate instead of the module where it is defined +help: a macro with this name exists at the root of the crate + | +LL | use issue_59764::{makro, +LL | foobaz, +LL | foo::{ +LL | baz, +LL | +LL | }, + ... + +error[E0432]: unresolved import `issue_59764::foo::makro` + --> $DIR/issue-59764.rs:79:13 + | +LL | makro + | ^^^^^ no `makro` in `foo` + | + = note: this could be because a macro annotated with `#[macro_export]` will be exported at the root of the crate instead of the module where it is defined +help: a macro with this name exists at the root of the crate + | +LL | use issue_59764::{makro, +LL | foobaz, +LL | foo::{ +LL | baz, +LL | +LL | } + ... + +error[E0432]: unresolved import `issue_59764::foo::makro` + --> $DIR/issue-59764.rs:85:42 + | +LL | use issue_59764::{foobaz, foo::{baz, makro, barbaz::{barfoo}}}; + | ^^^^^ no `makro` in `foo` + | + = note: this could be because a macro annotated with `#[macro_export]` will be exported at the root of the crate instead of the module where it is defined +help: a macro with this name exists at the root of the crate + | +LL | use issue_59764::{makro, foobaz, foo::{baz, barbaz::{barfoo}}}; + | + +error[E0432]: unresolved import `issue_59764::foo::makro` + --> $DIR/issue-59764.rs:94:13 + | +LL | makro, + | ^^^^^ no `makro` in `foo` + | + = note: this could be because a macro annotated with `#[macro_export]` will be exported at the root of the crate instead of the module where it is defined +help: a macro with this name exists at the root of the crate + | +LL | use issue_59764::{makro, +LL | foobaz, +LL | foo::{ +LL | baz, +LL | +LL | barbaz::{ + ... + +error[E0432]: unresolved import `issue_59764::foo::makro` + --> $DIR/issue-59764.rs:103:9 | LL | use issue_59764::foo::makro as baz; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ no `makro` in `foo` @@ -11,7 +184,19 @@ LL | use issue_59764::makro as baz; | ^^^^^^^^^^^^^^^^^^^^^^^^^ error[E0432]: unresolved import `issue_59764::foo::makro` - --> $DIR/issue-59764.rs:21:5 + --> $DIR/issue-59764.rs:108:33 + | +LL | use issue_59764::foo::{baz, makro as foobar}; + | ^^^^^^^^^^^^^^^ no `makro` in `foo` + | + = note: this could be because a macro annotated with `#[macro_export]` will be exported at the root of the crate instead of the module where it is defined +help: a macro with this name exists at the root of the crate + | +LL | use issue_59764::{makro as foobar, foo::{baz}}; + | + +error[E0432]: unresolved import `issue_59764::foo::makro` + --> $DIR/issue-59764.rs:114:5 | LL | use issue_59764::foo::makro; | ^^^^^^^^^^^^^^^^^^^^^^^ no `makro` in `foo` @@ -23,7 +208,7 @@ LL | use issue_59764::makro; | ^^^^^^^^^^^^^^^^^^ error: cannot determine resolution for the macro `makro` - --> $DIR/issue-59764.rs:24:1 + --> $DIR/issue-59764.rs:117:1 | LL | makro!(bar); | ^^^^^ @@ -31,12 +216,12 @@ LL | makro!(bar); = note: import resolution is stuck, try simplifying macro imports error[E0425]: cannot find function `bar` in this scope - --> $DIR/issue-59764.rs:28:5 + --> $DIR/issue-59764.rs:121:5 | LL | bar(); | ^^^ not found in this scope -error: aborting due to 4 previous errors +error: aborting due to 17 previous errors Some errors occurred: E0425, E0432. For more information about an error, try `rustc --explain E0425`. From 137ffa10229bfec497a446d24045388ee3c418d6 Mon Sep 17 00:00:00 2001 From: David Wood Date: Fri, 12 Apr 2019 01:19:02 +0200 Subject: [PATCH 07/11] Improve robustness of nested check. This commit removes the assumption that the start of a use statement will always be on one line with a single space - which was silly in the first place. --- src/librustc_resolve/error_reporting.rs | 49 +++++++++++++------------ src/test/ui/issue-59764.fixed | 11 ++++++ src/test/ui/issue-59764.rs | 14 +++++++ src/test/ui/issue-59764.stderr | 25 +++++++++++-- 4 files changed, 71 insertions(+), 28 deletions(-) diff --git a/src/librustc_resolve/error_reporting.rs b/src/librustc_resolve/error_reporting.rs index eea8cb80f69ef..5372c4ee44bdc 100644 --- a/src/librustc_resolve/error_reporting.rs +++ b/src/librustc_resolve/error_reporting.rs @@ -827,28 +827,29 @@ fn find_span_immediately_after_crate_name( module_name, use_span); let source_map = sess.source_map(); - // Get position of the first `{` character for the use statement. - // ie. `use foo::{a, b::{c, d}};` - // ^ - let pos_of_use_tree_left_bracket = source_map.span_until_char(use_span, '{').hi(); - debug!("find_span_immediately_after_crate_name: pos_of_use_tree_left_bracket={:?}", - pos_of_use_tree_left_bracket); - - // Calculate the expected difference between the first `{` character and the start of a - // use statement. - // ie. `use foo::{..};` - // ^^^^ - // | ^^^ - // 4 | ^^ - // 3 | - // 2 - let expected_difference = BytePos((module_name.as_str().len() + 4 + 2) as u32); - debug!("find_span_immediately_after_crate_name: expected_difference={:?}", - expected_difference); - let actual_difference = pos_of_use_tree_left_bracket - use_span.lo(); - debug!("find_span_immediately_after_crate_name: actual_difference={:?}", - actual_difference); - - (expected_difference == actual_difference, - use_span.with_lo(use_span.lo() + expected_difference)) + // Using `use issue_59764::foo::{baz, makro};` as an example throughout.. + let mut num_colons = 0; + // Find second colon.. `use issue_59764:` + let until_second_colon = source_map.span_take_while(use_span, |c| { + if *c == ':' { num_colons += 1; } + match c { + ':' if num_colons == 2 => false, + _ => true, + } + }); + // Find everything after the second colon.. `foo::{baz, makro};` + let from_second_colon = use_span.with_lo(until_second_colon.hi() + BytePos(1)); + + let mut found_a_non_whitespace_character = false; + // Find the first non-whitespace character in `from_second_colon`.. `f` + let after_second_colon = source_map.span_take_while(from_second_colon, |c| { + if found_a_non_whitespace_character { return false; } + if !c.is_whitespace() { found_a_non_whitespace_character = true; } + true + }); + + // Find the first `{` in from_second_colon.. `foo::{` + let next_left_bracket = source_map.span_through_char(from_second_colon, '{'); + + (next_left_bracket == after_second_colon, from_second_colon) } diff --git a/src/test/ui/issue-59764.fixed b/src/test/ui/issue-59764.fixed index ff193fce7eb1d..ad3bab50cfca3 100644 --- a/src/test/ui/issue-59764.fixed +++ b/src/test/ui/issue-59764.fixed @@ -109,6 +109,17 @@ mod renamed_multiple_imports { //~^ ERROR unresolved import `issue_59764::foo::makro` [E0432] } +mod lots_of_whitespace { + use issue_59764::{makro as foobar, + + foobaz, + + + foo::{baz} //~ ERROR unresolved import `issue_59764::foo::makro` [E0432] + + }; +} + // Simple case.. use issue_59764::makro; diff --git a/src/test/ui/issue-59764.rs b/src/test/ui/issue-59764.rs index c68471adaad56..0c6787691de0a 100644 --- a/src/test/ui/issue-59764.rs +++ b/src/test/ui/issue-59764.rs @@ -109,6 +109,20 @@ mod renamed_multiple_imports { //~^ ERROR unresolved import `issue_59764::foo::makro` [E0432] } +mod lots_of_whitespace { + use + issue_59764::{ + + foobaz, + + + foo::{baz, + + makro as foobar} //~ ERROR unresolved import `issue_59764::foo::makro` [E0432] + + }; +} + // Simple case.. use issue_59764::foo::makro; diff --git a/src/test/ui/issue-59764.stderr b/src/test/ui/issue-59764.stderr index fe93547318468..89b1f84605dc7 100644 --- a/src/test/ui/issue-59764.stderr +++ b/src/test/ui/issue-59764.stderr @@ -196,7 +196,24 @@ LL | use issue_59764::{makro as foobar, foo::{baz}}; | error[E0432]: unresolved import `issue_59764::foo::makro` - --> $DIR/issue-59764.rs:114:5 + --> $DIR/issue-59764.rs:121:17 + | +LL | makro as foobar} + | ^^^^^^^^^^^^^^^ no `makro` in `foo` + | + = note: this could be because a macro annotated with `#[macro_export]` will be exported at the root of the crate instead of the module where it is defined +help: a macro with this name exists at the root of the crate + | +LL | use issue_59764::{makro as foobar, +LL | +LL | foobaz, +LL | +LL | +LL | foo::{baz} + ... + +error[E0432]: unresolved import `issue_59764::foo::makro` + --> $DIR/issue-59764.rs:128:5 | LL | use issue_59764::foo::makro; | ^^^^^^^^^^^^^^^^^^^^^^^ no `makro` in `foo` @@ -208,7 +225,7 @@ LL | use issue_59764::makro; | ^^^^^^^^^^^^^^^^^^ error: cannot determine resolution for the macro `makro` - --> $DIR/issue-59764.rs:117:1 + --> $DIR/issue-59764.rs:131:1 | LL | makro!(bar); | ^^^^^ @@ -216,12 +233,12 @@ LL | makro!(bar); = note: import resolution is stuck, try simplifying macro imports error[E0425]: cannot find function `bar` in this scope - --> $DIR/issue-59764.rs:121:5 + --> $DIR/issue-59764.rs:135:5 | LL | bar(); | ^^^ not found in this scope -error: aborting due to 17 previous errors +error: aborting due to 18 previous errors Some errors occurred: E0425, E0432. For more information about an error, try `rustc --explain E0425`. From 5158063c3ec1e1dc8d9b0a0806e29d6c6e54d765 Mon Sep 17 00:00:00 2001 From: David Wood Date: Fri, 12 Apr 2019 01:53:32 +0200 Subject: [PATCH 08/11] Switch to multipart suggestions. This commit changes the suggestion so that it is split into multiple parts in an effort to reduce the impact the applied suggestion could have on formatting. --- src/librustc_resolve/error_reporting.rs | 57 +++++----- src/librustc_resolve/lib.rs | 11 +- src/librustc_resolve/resolve_imports.rs | 12 +-- src/test/ui/issue-59764.fixed | 134 ------------------------ src/test/ui/issue-59764.rs | 1 - src/test/ui/issue-59764.stderr | 61 +++++------ 6 files changed, 68 insertions(+), 208 deletions(-) delete mode 100644 src/test/ui/issue-59764.fixed diff --git a/src/librustc_resolve/error_reporting.rs b/src/librustc_resolve/error_reporting.rs index 5372c4ee44bdc..931bce91d7d43 100644 --- a/src/librustc_resolve/error_reporting.rs +++ b/src/librustc_resolve/error_reporting.rs @@ -617,14 +617,12 @@ impl<'a, 'b:'a> ImportResolver<'a, 'b> { _ => format!("{}", ident), }; - // Assume this is the easy case of `use issue_59764::foo::makro;` and just remove - // intermediate segments. - let (mut span, mut correction) = (directive.span, - format!("{}::{}", module_name, import)); - - if directive.is_nested() { - span = directive.use_span; - + let mut corrections: Vec<(Span, String)> = Vec::new(); + if !directive.is_nested() { + // Assume this is the easy case of `use issue_59764::foo::makro;` and just remove + // intermediate segments. + corrections.push((directive.span, format!("{}::{}", module_name, import))); + } else { // Find the binding span (and any trailing commas and spaces). // ie. `use a::b::{c, d, e};` // ^^^ @@ -652,6 +650,9 @@ impl<'a, 'b:'a> ImportResolver<'a, 'b> { } debug!("check_for_module_export_macro: removal_span={:?}", removal_span); + // Remove the `removal_span`. + corrections.push((removal_span, "".to_string())); + // Find the span after the crate name and if it has nested imports immediatately // after the crate name already. // ie. `use a::b::{c, d};` @@ -666,34 +667,32 @@ impl<'a, 'b:'a> ImportResolver<'a, 'b> { let source_map = self.resolver.session.source_map(); - // Remove two bytes at the end to keep all but the `};` characters. - // ie. `{b::{c, d}, e::{f, g}};` - // ^^^^^^^^^^^^^^^^^^^^^ - let end_bytes = BytePos(if has_nested { 2 } else { 1 }); - let mut remaining_span = after_crate_name.with_hi( - after_crate_name.hi() - end_bytes); - if has_nested { - // Remove two bytes at the start to keep all but the initial `{` character. - // ie. `{b::{c, d}, e::{f, g}` - // ^^^^^^^^^^^^^^^^^^^^ - remaining_span = remaining_span.with_lo(after_crate_name.lo() + BytePos(1)); + // Add the import to the start, with a `{` if required. + let start_point = source_map.start_point(after_crate_name); + if let Ok(start_snippet) = source_map.span_to_snippet(start_point) { + corrections.push(( + start_point, + if has_nested { + // In this case, `start_snippet` must equal '{'. + format!("{}{}, ", start_snippet, import) + } else { + // In this case, add a `{`, then the moved import, then whatever + // was there before. + format!("{{{}, {}", import, start_snippet) + } + )); } - // Calculate the number of characters into a snippet to remove the removal - // span. - let lo = removal_span.lo() - remaining_span.lo(); - let hi = lo + (removal_span.hi() - removal_span.lo()); - if let Ok(mut remaining) = source_map.span_to_snippet(remaining_span) { - // Remove the original location of the binding. - remaining.replace_range((lo.0 as usize)..(hi.0 as usize), ""); - correction = format!("use {}::{{{}, {}}};", module_name, import, remaining); + // Add a `};` to the end if nested, matching the `{` added at the start. + if !has_nested { + corrections.push((source_map.end_point(after_crate_name), + "};".to_string())); } } let suggestion = Some(( - span, + corrections, String::from("a macro with this name exists at the root of the crate"), - correction, Applicability::MaybeIncorrect, )); let note = vec![ diff --git a/src/librustc_resolve/lib.rs b/src/librustc_resolve/lib.rs index b33356c2ba7ae..c1353d771c26f 100644 --- a/src/librustc_resolve/lib.rs +++ b/src/librustc_resolve/lib.rs @@ -139,8 +139,8 @@ impl Ord for BindingError { } } -/// A span, message, replacement text, and applicability. -type Suggestion = (Span, String, String, Applicability); +/// A vector of spans and replacements, a message and applicability. +type Suggestion = (Vec<(Span, String)>, String, Applicability); enum ResolutionError<'a> { /// Error E0401: can't use type or const parameters from outer function. @@ -390,8 +390,8 @@ fn resolve_struct_error<'sess, 'a>(resolver: &'sess Resolver<'_>, "failed to resolve: {}", &label); err.span_label(span, label); - if let Some((span, msg, suggestion, applicability)) = suggestion { - err.span_suggestion(span, &msg, suggestion, applicability); + if let Some((suggestions, msg, applicability)) = suggestion { + err.multipart_suggestion(&msg, suggestions, applicability); } err @@ -3774,9 +3774,8 @@ impl<'a> Resolver<'a> { ( String::from("unresolved import"), Some(( - ident.span, + vec![(ident.span, candidate.path.to_string())], String::from("a similar path exists"), - candidate.path.to_string(), Applicability::MaybeIncorrect, )), ) diff --git a/src/librustc_resolve/resolve_imports.rs b/src/librustc_resolve/resolve_imports.rs index ef5f0f54ee369..62af6e19603c4 100644 --- a/src/librustc_resolve/resolve_imports.rs +++ b/src/librustc_resolve/resolve_imports.rs @@ -793,8 +793,8 @@ impl<'a, 'b:'a> ImportResolver<'a, 'b> { diag.span_label(err.span, label); } - if let Some((span, msg, suggestion, applicability)) = err.suggestion { - diag.span_suggestion(span, &msg, suggestion, applicability); + if let Some((suggestions, msg, applicability)) = err.suggestion { + diag.multipart_suggestion(&msg, suggestions, applicability); } } @@ -947,9 +947,8 @@ impl<'a, 'b:'a> ImportResolver<'a, 'b> { label: None, note, suggestion: Some(( - span, + vec![(span, Segment::names_to_string(&suggestion))], String::from("a similar path exists"), - Segment::names_to_string(&suggestion), Applicability::MaybeIncorrect, )), } @@ -1113,8 +1112,9 @@ impl<'a, 'b:'a> ImportResolver<'a, 'b> { let lev_suggestion = find_best_match_for_name(names, &ident.as_str(), None) .map(|suggestion| - (ident.span, String::from("a similar name exists in the module"), - suggestion.to_string(), Applicability::MaybeIncorrect) + (vec![(ident.span, suggestion.to_string())], + String::from("a similar name exists in the module"), + Applicability::MaybeIncorrect) ); let (suggestion, note) = match self.check_for_module_export_macro( diff --git a/src/test/ui/issue-59764.fixed b/src/test/ui/issue-59764.fixed deleted file mode 100644 index ad3bab50cfca3..0000000000000 --- a/src/test/ui/issue-59764.fixed +++ /dev/null @@ -1,134 +0,0 @@ -// aux-build:issue-59764.rs -// compile-flags:--extern issue_59764 -// edition:2018 -// run-rustfix - -#![allow(warnings)] - -// This tests the suggestion to import macros from the root of a crate. This aims to capture -// the case where a user attempts to import a macro from the definition location instead of the -// root of the crate and the macro is annotated with `#![macro_export]`. - -// Edge cases.. - -mod multiple_imports_same_line_at_end { - use issue_59764::{makro, foo::{baz}}; - //~^ ERROR unresolved import `issue_59764::foo::makro` [E0432] -} - -mod multiple_imports_multiline_at_end_trailing_comma { - use issue_59764::{makro, foo::{ - baz, - //~ ERROR unresolved import `issue_59764::foo::makro` [E0432] - }}; -} - -mod multiple_imports_multiline_at_end { - use issue_59764::{makro, foo::{ - baz, - //~ ERROR unresolved import `issue_59764::foo::makro` [E0432] - }}; -} - -mod multiple_imports_same_line_in_middle { - use issue_59764::{makro, foo::{baz, foobar}}; - //~^ ERROR unresolved import `issue_59764::foo::makro` [E0432] -} - -mod multiple_imports_multiline_in_middle_trailing_comma { - use issue_59764::{makro, foo::{ - baz, - //~ ERROR unresolved import `issue_59764::foo::makro` [E0432] - foobar, - }}; -} - -mod multiple_imports_multiline_in_middle { - use issue_59764::{makro, foo::{ - baz, - //~ ERROR unresolved import `issue_59764::foo::makro` [E0432] - foobar - }}; -} - -mod nested_imports { - use issue_59764::{makro, foobaz}; - //~^ ERROR unresolved import `issue_59764::foo::makro` [E0432] -} - -mod nested_multiple_imports { - use issue_59764::{makro, foobaz, foo::{baz}}; - //~^ ERROR unresolved import `issue_59764::foo::makro` [E0432] -} - -mod nested_multiline_multiple_imports_trailing_comma { - use issue_59764::{makro, - foobaz, - foo::{ - baz, - //~ ERROR unresolved import `issue_59764::foo::makro` [E0432] - }, - }; -} - -mod nested_multiline_multiple_imports { - use issue_59764::{makro, - foobaz, - foo::{ - baz, - //~ ERROR unresolved import `issue_59764::foo::makro` [E0432] - } - }; -} - -mod doubly_nested_multiple_imports { - use issue_59764::{makro, foobaz, foo::{baz, barbaz::{barfoo}}}; - //~^ ERROR unresolved import `issue_59764::foo::makro` [E0432] -} - -mod doubly_multiline_nested_multiple_imports { - use issue_59764::{makro, - foobaz, - foo::{ - baz, - //~ ERROR unresolved import `issue_59764::foo::makro` [E0432] - barbaz::{ - barfoo, - } - } - }; -} - -mod renamed_import { - use issue_59764::makro as baz; - //~^ ERROR unresolved import `issue_59764::foo::makro` [E0432] -} - -mod renamed_multiple_imports { - use issue_59764::{makro as foobar, foo::{baz}}; - //~^ ERROR unresolved import `issue_59764::foo::makro` [E0432] -} - -mod lots_of_whitespace { - use issue_59764::{makro as foobar, - - foobaz, - - - foo::{baz} //~ ERROR unresolved import `issue_59764::foo::makro` [E0432] - - }; -} - -// Simple case.. - -use issue_59764::makro; -//~^ ERROR unresolved import `issue_59764::foo::makro` [E0432] - -makro!(bar); -//~^ ERROR cannot determine resolution for the macro `makro` - -fn main() { - bar(); - //~^ ERROR cannot find function `bar` in this scope [E0425] -} diff --git a/src/test/ui/issue-59764.rs b/src/test/ui/issue-59764.rs index 0c6787691de0a..09dee8c273268 100644 --- a/src/test/ui/issue-59764.rs +++ b/src/test/ui/issue-59764.rs @@ -1,7 +1,6 @@ // aux-build:issue-59764.rs // compile-flags:--extern issue_59764 // edition:2018 -// run-rustfix #![allow(warnings)] diff --git a/src/test/ui/issue-59764.stderr b/src/test/ui/issue-59764.stderr index 89b1f84605dc7..924e69f5f9703 100644 --- a/src/test/ui/issue-59764.stderr +++ b/src/test/ui/issue-59764.stderr @@ -1,5 +1,5 @@ error[E0432]: unresolved import `issue_59764::foo::makro` - --> $DIR/issue-59764.rs:15:33 + --> $DIR/issue-59764.rs:14:33 | LL | use issue_59764::foo::{baz, makro}; | ^^^^^ no `makro` in `foo` @@ -8,10 +8,10 @@ LL | use issue_59764::foo::{baz, makro}; help: a macro with this name exists at the root of the crate | LL | use issue_59764::{makro, foo::{baz}}; - | + | ^^^^^^^^^ --^^ error[E0432]: unresolved import `issue_59764::foo::makro` - --> $DIR/issue-59764.rs:22:9 + --> $DIR/issue-59764.rs:21:9 | LL | makro, | ^^^^^ no `makro` in `foo` @@ -26,7 +26,7 @@ LL | }}; | error[E0432]: unresolved import `issue_59764::foo::makro` - --> $DIR/issue-59764.rs:29:9 + --> $DIR/issue-59764.rs:28:9 | LL | makro | ^^^^^ no `makro` in `foo` @@ -41,7 +41,7 @@ LL | }}; | error[E0432]: unresolved import `issue_59764::foo::makro` - --> $DIR/issue-59764.rs:34:33 + --> $DIR/issue-59764.rs:33:33 | LL | use issue_59764::foo::{baz, makro, foobar}; | ^^^^^ no `makro` in `foo` @@ -50,10 +50,10 @@ LL | use issue_59764::foo::{baz, makro, foobar}; help: a macro with this name exists at the root of the crate | LL | use issue_59764::{makro, foo::{baz, foobar}}; - | + | ^^^^^^^^^ -- ^^ error[E0432]: unresolved import `issue_59764::foo::makro` - --> $DIR/issue-59764.rs:41:9 + --> $DIR/issue-59764.rs:40:9 | LL | makro, | ^^^^^ no `makro` in `foo` @@ -69,7 +69,7 @@ LL | }}; | error[E0432]: unresolved import `issue_59764::foo::makro` - --> $DIR/issue-59764.rs:49:9 + --> $DIR/issue-59764.rs:48:9 | LL | makro, | ^^^^^ no `makro` in `foo` @@ -85,7 +85,7 @@ LL | }}; | error[E0432]: unresolved import `issue_59764::foo::makro` - --> $DIR/issue-59764.rs:55:31 + --> $DIR/issue-59764.rs:54:31 | LL | use issue_59764::{foobaz, foo::makro}; | ^^^^^^^^^^ no `makro` in `foo` @@ -94,10 +94,10 @@ LL | use issue_59764::{foobaz, foo::makro}; help: a macro with this name exists at the root of the crate | LL | use issue_59764::{makro, foobaz}; - | + | ^^^^^^^ -- error[E0432]: unresolved import `issue_59764::foo::makro` - --> $DIR/issue-59764.rs:60:42 + --> $DIR/issue-59764.rs:59:42 | LL | use issue_59764::{foobaz, foo::{baz, makro}}; | ^^^^^ no `makro` in `foo` @@ -106,10 +106,10 @@ LL | use issue_59764::{foobaz, foo::{baz, makro}}; help: a macro with this name exists at the root of the crate | LL | use issue_59764::{makro, foobaz, foo::{baz}}; - | + | ^^^^^^^ -- error[E0432]: unresolved import `issue_59764::foo::makro` - --> $DIR/issue-59764.rs:69:13 + --> $DIR/issue-59764.rs:68:13 | LL | makro, | ^^^^^ no `makro` in `foo` @@ -122,11 +122,10 @@ LL | foobaz, LL | foo::{ LL | baz, LL | -LL | }, - ... + | error[E0432]: unresolved import `issue_59764::foo::makro` - --> $DIR/issue-59764.rs:79:13 + --> $DIR/issue-59764.rs:78:13 | LL | makro | ^^^^^ no `makro` in `foo` @@ -139,11 +138,10 @@ LL | foobaz, LL | foo::{ LL | baz, LL | -LL | } - ... + | error[E0432]: unresolved import `issue_59764::foo::makro` - --> $DIR/issue-59764.rs:85:42 + --> $DIR/issue-59764.rs:84:42 | LL | use issue_59764::{foobaz, foo::{baz, makro, barbaz::{barfoo}}}; | ^^^^^ no `makro` in `foo` @@ -152,10 +150,10 @@ LL | use issue_59764::{foobaz, foo::{baz, makro, barbaz::{barfoo}}}; help: a macro with this name exists at the root of the crate | LL | use issue_59764::{makro, foobaz, foo::{baz, barbaz::{barfoo}}}; - | + | ^^^^^^^ -- error[E0432]: unresolved import `issue_59764::foo::makro` - --> $DIR/issue-59764.rs:94:13 + --> $DIR/issue-59764.rs:93:13 | LL | makro, | ^^^^^ no `makro` in `foo` @@ -168,11 +166,10 @@ LL | foobaz, LL | foo::{ LL | baz, LL | -LL | barbaz::{ - ... + | error[E0432]: unresolved import `issue_59764::foo::makro` - --> $DIR/issue-59764.rs:103:9 + --> $DIR/issue-59764.rs:102:9 | LL | use issue_59764::foo::makro as baz; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ no `makro` in `foo` @@ -184,7 +181,7 @@ LL | use issue_59764::makro as baz; | ^^^^^^^^^^^^^^^^^^^^^^^^^ error[E0432]: unresolved import `issue_59764::foo::makro` - --> $DIR/issue-59764.rs:108:33 + --> $DIR/issue-59764.rs:107:33 | LL | use issue_59764::foo::{baz, makro as foobar}; | ^^^^^^^^^^^^^^^ no `makro` in `foo` @@ -193,10 +190,10 @@ LL | use issue_59764::foo::{baz, makro as foobar}; help: a macro with this name exists at the root of the crate | LL | use issue_59764::{makro as foobar, foo::{baz}}; - | + | ^^^^^^^^^^^^^^^^^^^ --^^ error[E0432]: unresolved import `issue_59764::foo::makro` - --> $DIR/issue-59764.rs:121:17 + --> $DIR/issue-59764.rs:120:17 | LL | makro as foobar} | ^^^^^^^^^^^^^^^ no `makro` in `foo` @@ -204,16 +201,16 @@ LL | makro as foobar} = note: this could be because a macro annotated with `#[macro_export]` will be exported at the root of the crate instead of the module where it is defined help: a macro with this name exists at the root of the crate | -LL | use issue_59764::{makro as foobar, +LL | issue_59764::{makro as foobar, LL | LL | foobaz, LL | LL | LL | foo::{baz} - ... + | error[E0432]: unresolved import `issue_59764::foo::makro` - --> $DIR/issue-59764.rs:128:5 + --> $DIR/issue-59764.rs:127:5 | LL | use issue_59764::foo::makro; | ^^^^^^^^^^^^^^^^^^^^^^^ no `makro` in `foo` @@ -225,7 +222,7 @@ LL | use issue_59764::makro; | ^^^^^^^^^^^^^^^^^^ error: cannot determine resolution for the macro `makro` - --> $DIR/issue-59764.rs:131:1 + --> $DIR/issue-59764.rs:130:1 | LL | makro!(bar); | ^^^^^ @@ -233,7 +230,7 @@ LL | makro!(bar); = note: import resolution is stuck, try simplifying macro imports error[E0425]: cannot find function `bar` in this scope - --> $DIR/issue-59764.rs:135:5 + --> $DIR/issue-59764.rs:134:5 | LL | bar(); | ^^^ not found in this scope From 633fc9eef0d26a1c9b59fad8e807632f234a8aae Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Fri, 12 Apr 2019 12:30:21 +0200 Subject: [PATCH 09/11] Revert PR #59401 to fix issue #59652 (a stable-to-beta regression). This is result of squashing two revert commits: Revert "compile all crates under test w/ -Zemit-stack-sizes" This reverts commit 7d365cf27f4249fc9b61ba8abfc813abe43f1cb7. Revert "bootstrap: build compiler-builtins with -Z emit-stack-sizes" This reverts commit 8b8488ce8fc047282e7159343f30609417f9fa39. --- src/bootstrap/bin/rustc.rs | 27 --------------------------- src/bootstrap/compile.rs | 4 ---- 2 files changed, 31 deletions(-) diff --git a/src/bootstrap/bin/rustc.rs b/src/bootstrap/bin/rustc.rs index 86ce5fd01a812..a76584093fc76 100644 --- a/src/bootstrap/bin/rustc.rs +++ b/src/bootstrap/bin/rustc.rs @@ -187,33 +187,6 @@ fn main() { cmd.arg("-C").arg(format!("debug-assertions={}", debug_assertions)); } - // Build all crates in the `std` facade with `-Z emit-stack-sizes` to add stack usage - // information. - // - // When you use this `-Z` flag with Cargo you get stack usage information on all crates - // compiled from source, and when you are using LTO you also get information on pre-compiled - // crates like `core` and `std`, even if they were not compiled with `-Z emit-stack-sizes`. - // However, there's an exception: `compiler_builtins`. This crate is special and doesn't - // participate in LTO because it's always linked as a separate object file. For this reason - // it's impossible to get stack usage information about `compiler-builtins` using - // `RUSTFLAGS` + Cargo, or `cargo rustc`. - // - // To make the stack usage information of all crates under the `std` facade available to - // Cargo based stack usage analysis tools, in both LTO and non-LTO mode, we compile them - // with the `-Z emit-stack-sizes` flag. The `RUSTC_EMIT_STACK_SIZES` var helps us apply this - // flag only to the crates in the `std` facade. The `-Z` flag is known to currently work - // with targets that produce ELF files so we limit its use flag to those targets. - // - // NOTE(japaric) if this ever causes problem with an LLVM upgrade or any PR feel free to - // remove it or comment it out - if env::var_os("RUSTC_EMIT_STACK_SIZES").is_some() - && (target.contains("-linux-") - || target.contains("-none-eabi") - || target.ends_with("-none-elf")) - { - cmd.arg("-Zemit-stack-sizes"); - } - if let Ok(s) = env::var("RUSTC_CODEGEN_UNITS") { cmd.arg("-C").arg(format!("codegen-units={}", s)); } diff --git a/src/bootstrap/compile.rs b/src/bootstrap/compile.rs index 08316b71ea85b..66443d472d334 100644 --- a/src/bootstrap/compile.rs +++ b/src/bootstrap/compile.rs @@ -97,8 +97,6 @@ impl Step for Std { let _folder = builder.fold_output(|| format!("stage{}-std", compiler.stage)); builder.info(&format!("Building stage{} std artifacts ({} -> {})", compiler.stage, &compiler.host, target)); - // compile with `-Z emit-stack-sizes`; see bootstrap/src/rustc.rs for more details - cargo.env("RUSTC_EMIT_STACK_SIZES", "1"); run_cargo(builder, &mut cargo, &libstd_stamp(builder, compiler, target), @@ -397,8 +395,6 @@ impl Step for Test { let _folder = builder.fold_output(|| format!("stage{}-test", compiler.stage)); builder.info(&format!("Building stage{} test artifacts ({} -> {})", compiler.stage, &compiler.host, target)); - // compile with `-Z emit-stack-sizes`; see bootstrap/src/rustc.rs for more details - cargo.env("RUSTC_EMIT_STACK_SIZES", "1"); run_cargo(builder, &mut cargo, &libtest_stamp(builder, compiler, target), From 796e6e37d6fa76b719dbf9b38872daffdc9caa70 Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Thu, 11 Apr 2019 22:48:53 +0200 Subject: [PATCH 10/11] Don't generate empty json variables --- src/librustdoc/html/render.rs | 19 +++++++++--- src/librustdoc/html/static/source-script.js | 34 ++++++++++++--------- 2 files changed, 34 insertions(+), 19 deletions(-) diff --git a/src/librustdoc/html/render.rs b/src/librustdoc/html/render.rs index 866d8fe682a7b..3f2eaf8ec5527 100644 --- a/src/librustdoc/html/render.rs +++ b/src/librustdoc/html/render.rs @@ -1060,11 +1060,22 @@ themePicker.onblur = handleThemeButtonsBlur; .expect("invalid osstring conversion"))) .collect::>(); files.sort_unstable_by(|a, b| a.cmp(b)); - // FIXME(imperio): we could avoid to generate "dirs" and "files" if they're empty. - format!("{{\"name\":\"{name}\",\"dirs\":[{subs}],\"files\":[{files}]}}", + let subs = subs.iter().map(|s| s.to_json_string()).collect::>().join(","); + let dirs = if subs.is_empty() { + String::new() + } else { + format!(",\"dirs\":[{}]", subs) + }; + let files = files.join(","); + let files = if files.is_empty() { + String::new() + } else { + format!(",\"files\":[{}]", files) + }; + format!("{{\"name\":\"{name}\"{dirs}{files}}}", name=self.elem.to_str().expect("invalid osstring conversion"), - subs=subs.iter().map(|s| s.to_json_string()).collect::>().join(","), - files=files.join(",")) + dirs=dirs, + files=files) } } diff --git a/src/librustdoc/html/static/source-script.js b/src/librustdoc/html/static/source-script.js index 509c628ce5a46..567022b4139ad 100644 --- a/src/librustdoc/html/static/source-script.js +++ b/src/librustdoc/html/static/source-script.js @@ -39,28 +39,32 @@ function createDirEntry(elem, parent, fullPath, currentFile, hasFoundFile) { children.className = "children"; var folders = document.createElement("div"); folders.className = "folders"; - for (var i = 0; i < elem.dirs.length; ++i) { - if (createDirEntry(elem.dirs[i], folders, fullPath, currentFile, - hasFoundFile) === true) { - addClass(name, "expand"); - hasFoundFile = true; + if (elem.dirs) { + for (var i = 0; i < elem.dirs.length; ++i) { + if (createDirEntry(elem.dirs[i], folders, fullPath, currentFile, + hasFoundFile) === true) { + addClass(name, "expand"); + hasFoundFile = true; + } } } children.appendChild(folders); var files = document.createElement("div"); files.className = "files"; - for (i = 0; i < elem.files.length; ++i) { - var file = document.createElement("a"); - file.innerText = elem.files[i]; - file.href = window.rootPath + "src/" + fullPath + elem.files[i] + ".html"; - if (hasFoundFile === false && - currentFile === fullPath + elem.files[i]) { - file.className = "selected"; - addClass(name, "expand"); - hasFoundFile = true; + if (elem.files) { + for (i = 0; i < elem.files.length; ++i) { + var file = document.createElement("a"); + file.innerText = elem.files[i]; + file.href = window.rootPath + "src/" + fullPath + elem.files[i] + ".html"; + if (hasFoundFile === false && + currentFile === fullPath + elem.files[i]) { + file.className = "selected"; + addClass(name, "expand"); + hasFoundFile = true; + } + files.appendChild(file); } - files.appendChild(file); } search.fullPath = fullPath; children.appendChild(files); From 4f28431e39fa2abc16703e067bcf9a7e3a0502d4 Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Sun, 7 Apr 2019 18:09:28 +0200 Subject: [PATCH 11/11] Apply resource-suffix to search-index and source-files scripts as well --- src/librustdoc/html/layout.rs | 4 ++-- src/librustdoc/html/render.rs | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/librustdoc/html/layout.rs b/src/librustdoc/html/layout.rs index 6ff3917a265ed..acf019fd2254d 100644 --- a/src/librustdoc/html/layout.rs +++ b/src/librustdoc/html/layout.rs @@ -157,11 +157,11 @@ pub fn render( window.rootPath = \"{root_path}\";\ window.currentCrate = \"{krate}\";\ \ - \ + \ \ {static_extra_scripts}\ {extra_scripts}\ - \ + \ \ ", css_extension = if css_file_extension { diff --git a/src/librustdoc/html/render.rs b/src/librustdoc/html/render.rs index 866d8fe682a7b..65922dfb24b2c 100644 --- a/src/librustdoc/html/render.rs +++ b/src/librustdoc/html/render.rs @@ -1009,7 +1009,7 @@ themePicker.onblur = handleThemeButtonsBlur; }) } - let dst = cx.dst.join("aliases.js"); + let dst = cx.dst.join(&format!("aliases{}.js", cx.shared.resource_suffix)); { let (mut all_aliases, _, _) = try_err!(collect(&dst, &krate.name, "ALIASES", false), &dst); let mut w = try_err!(File::create(&dst), &dst); @@ -1097,7 +1097,7 @@ themePicker.onblur = handleThemeButtonsBlur; } } - let dst = cx.dst.join("source-files.js"); + let dst = cx.dst.join(&format!("source-files{}.js", cx.shared.resource_suffix)); let (mut all_sources, _krates, _) = try_err!(collect(&dst, &krate.name, "sourcesIndex", false), &dst); @@ -1113,7 +1113,7 @@ themePicker.onblur = handleThemeButtonsBlur; } // Update the search index - let dst = cx.dst.join("search-index.js"); + let dst = cx.dst.join(&format!("search-index{}.js", cx.shared.resource_suffix)); let (mut all_indexes, mut krates, variables) = try_err!(collect(&dst, &krate.name, "searchIndex", @@ -1476,7 +1476,7 @@ impl<'a> SourceCollector<'a> { description: &desc, keywords: BASIC_KEYWORDS, resource_suffix: &self.scx.resource_suffix, - extra_scripts: &["source-files"], + extra_scripts: &[&format!("source-files{}", self.scx.resource_suffix)], static_extra_scripts: &[&format!("source-script{}", self.scx.resource_suffix)], }; layout::render(&mut w, &self.scx.layout,