From b97487bad8608afe05f34f07016aa6276c1a291d Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Sat, 30 May 2020 15:36:42 +0200 Subject: [PATCH 01/24] Add check for doc alias attribute format --- src/librustdoc/clean/types.rs | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/src/librustdoc/clean/types.rs b/src/librustdoc/clean/types.rs index 381238165274..4f99476ebfa5 100644 --- a/src/librustdoc/clean/types.rs +++ b/src/librustdoc/clean/types.rs @@ -481,6 +481,33 @@ pub fn extract_include(mi: &ast::MetaItem) -> Option<(String, String)> { }) } + /// Enforce the format of attributes inside `#[doc(...)]`. + pub fn check_doc_attributes( + diagnostic: &::rustc_errors::Handler, + mi: &ast::MetaItem, + ) -> Option<(String, String)> { + mi.meta_item_list().and_then(|list| { + for meta in list { + if meta.check_name(sym::alias) { + if !meta.is_value_str() + || meta + .value_str() + .map(|s| s.to_string()) + .unwrap_or_else(String::new) + .is_empty() + { + diagnostic.span_err( + meta.span(), + "doc alias attribute expects a string: #[doc(alias = \"0\")]", + ); + } + } + } + + None + }) + } + pub fn has_doc_flag(&self, flag: Symbol) -> bool { for attr in &self.other_attrs { if !attr.check_name(sym::doc) { @@ -524,6 +551,7 @@ pub fn from_ast(diagnostic: &::rustc_errors::Handler, attrs: &[ast::Attribute]) } else { if attr.check_name(sym::doc) { if let Some(mi) = attr.meta() { + Attributes::check_doc_attributes(&diagnostic, &mi); if let Some(cfg_mi) = Attributes::extract_cfg(&mi) { // Extracted #[doc(cfg(...))] match Cfg::parse(cfg_mi) { From 2d6267a7a8ae0399f2d363b6ac667fee0b53a1a0 Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Sat, 30 May 2020 15:36:57 +0200 Subject: [PATCH 02/24] Add test for doc alias attribute validation --- src/test/rustdoc-ui/check-doc-alias-attr.rs | 9 +++++++++ .../rustdoc-ui/check-doc-alias-attr.stderr | 20 +++++++++++++++++++ 2 files changed, 29 insertions(+) create mode 100644 src/test/rustdoc-ui/check-doc-alias-attr.rs create mode 100644 src/test/rustdoc-ui/check-doc-alias-attr.stderr diff --git a/src/test/rustdoc-ui/check-doc-alias-attr.rs b/src/test/rustdoc-ui/check-doc-alias-attr.rs new file mode 100644 index 000000000000..2f01099107d9 --- /dev/null +++ b/src/test/rustdoc-ui/check-doc-alias-attr.rs @@ -0,0 +1,9 @@ +#![feature(doc_alias)] + +#[doc(alias = "foo")] // ok! +pub struct Bar; + +#[doc(alias)] //~ ERROR +#[doc(alias = 0)] //~ ERROR +#[doc(alias("bar"))] //~ ERROR +pub struct Foo; diff --git a/src/test/rustdoc-ui/check-doc-alias-attr.stderr b/src/test/rustdoc-ui/check-doc-alias-attr.stderr new file mode 100644 index 000000000000..480acc821aaa --- /dev/null +++ b/src/test/rustdoc-ui/check-doc-alias-attr.stderr @@ -0,0 +1,20 @@ +error: doc alias attribute expects a string: #[doc(alias = "0")] + --> $DIR/check-doc-alias-attr.rs:6:7 + | +LL | #[doc(alias)] + | ^^^^^ + +error: doc alias attribute expects a string: #[doc(alias = "0")] + --> $DIR/check-doc-alias-attr.rs:7:7 + | +LL | #[doc(alias = 0)] + | ^^^^^^^^^ + +error: doc alias attribute expects a string: #[doc(alias = "0")] + --> $DIR/check-doc-alias-attr.rs:8:7 + | +LL | #[doc(alias("bar"))] + | ^^^^^^^^^^^^ + +error: aborting due to 3 previous errors + From 0c5c644c91edf6ed949cfa5ffc524f43369df604 Mon Sep 17 00:00:00 2001 From: TrolledWoods Date: Mon, 1 Jun 2020 08:18:34 +0200 Subject: [PATCH 03/24] Mention that BTreeMap::new() doesn't allocate I think it would be nice to mention this, so you don't have to dig through the src to look at the definition of new(). --- src/liballoc/collections/btree/map.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/liballoc/collections/btree/map.rs b/src/liballoc/collections/btree/map.rs index fa1c09d9ece8..688d660a02af 100644 --- a/src/liballoc/collections/btree/map.rs +++ b/src/liballoc/collections/btree/map.rs @@ -490,6 +490,8 @@ struct MergeIter> { impl BTreeMap { /// Makes a new empty BTreeMap with a reasonable choice for B. /// + /// Does not allocate anything on its own. + /// /// # Examples /// /// Basic usage: From 8c7c84b4e8923779ff56a518e4cd39c1c600c7d0 Mon Sep 17 00:00:00 2001 From: Rich Kadel Date: Thu, 18 Jun 2020 13:29:43 -0700 Subject: [PATCH 04/24] code coverage foundation for hash and num_counters Replaced dummy values for hash and num_counters with computed values, and refactored InstrumentCoverage pass to simplify injecting more counters per function in upcoming versions. Improved usage documentation and error messaging. --- src/librustc_codegen_llvm/intrinsic.rs | 37 ++-- src/librustc_codegen_ssa/mir/block.rs | 2 +- src/librustc_codegen_ssa/mir/mod.rs | 4 +- src/librustc_codegen_ssa/traits/intrinsic.rs | 6 +- src/librustc_metadata/locator.rs | 2 + src/librustc_middle/ich/hcx.rs | 33 +++- src/librustc_middle/mir/mod.rs | 23 ++- src/librustc_middle/ty/context.rs | 7 + .../transform/instrument_coverage.rs | 186 +++++++++++++----- src/librustc_session/options.rs | 5 +- src/librustc_span/symbol.rs | 1 + 11 files changed, 225 insertions(+), 81 deletions(-) diff --git a/src/librustc_codegen_llvm/intrinsic.rs b/src/librustc_codegen_llvm/intrinsic.rs index 95465939070a..8c0ccde6b16b 100644 --- a/src/librustc_codegen_llvm/intrinsic.rs +++ b/src/librustc_codegen_llvm/intrinsic.rs @@ -16,6 +16,7 @@ use rustc_codegen_ssa::glue; use rustc_codegen_ssa::mir::operand::{OperandRef, OperandValue}; use rustc_codegen_ssa::mir::place::PlaceRef; +use rustc_codegen_ssa::mir::FunctionCx; use rustc_codegen_ssa::traits::*; use rustc_codegen_ssa::MemFlags; use rustc_hir as hir; @@ -23,7 +24,6 @@ use rustc_middle::ty::{self, Ty}; use rustc_middle::{bug, span_bug}; use rustc_span::Span; -use rustc_span::Symbol; use rustc_target::abi::{self, HasDataLayout, LayoutOf, Primitive}; use rustc_target::spec::PanicStrategy; @@ -82,14 +82,14 @@ fn get_simple_intrinsic(cx: &CodegenCx<'ll, '_>, name: &str) -> Option<&'ll Valu } impl IntrinsicCallMethods<'tcx> for Builder<'a, 'll, 'tcx> { - fn codegen_intrinsic_call( + fn codegen_intrinsic_call<'b, Bx: BuilderMethods<'b, 'tcx>>( &mut self, + fx: &FunctionCx<'b, 'tcx, Bx>, instance: ty::Instance<'tcx>, fn_abi: &FnAbi<'tcx, Ty<'tcx>>, args: &[OperandRef<'tcx, &'ll Value>], llresult: &'ll Value, span: Span, - caller_instance: ty::Instance<'tcx>, ) { let tcx = self.tcx; let callee_ty = instance.monomorphic_ty(tcx); @@ -141,26 +141,17 @@ fn codegen_intrinsic_call( self.call(llfn, &[], None) } "count_code_region" => { - if let ty::InstanceDef::Item(fn_def_id) = caller_instance.def { - let caller_fn_path = tcx.def_path_str(fn_def_id); - debug!( - "count_code_region to llvm.instrprof.increment(fn_name={})", - caller_fn_path - ); - - // FIXME(richkadel): (1) Replace raw function name with mangled function name; - // (2) Replace hardcoded `1234` in `hash` with a computed hash (as discussed in) - // the MCP (compiler-team/issues/278); and replace the hardcoded `1` for - // `num_counters` with the actual number of counters per function (when the - // changes are made to inject more than one counter per function). - let (fn_name, _len_val) = self.const_str(Symbol::intern(&caller_fn_path)); - let index = args[0].immediate(); - let hash = self.const_u64(1234); - let num_counters = self.const_u32(1); - self.instrprof_increment(fn_name, hash, num_counters, index) - } else { - bug!("intrinsic count_code_region: no src.instance"); - } + let coverage_data = fx.mir.coverage_data.as_ref().unwrap(); + let mangled_fn = tcx.symbol_name(fx.instance); + let (mangled_fn_name, _len_val) = self.const_str(mangled_fn.name); + let hash = self.const_u64(coverage_data.hash); + let index = args[0].immediate(); + let num_counters = self.const_u32(coverage_data.num_counters as u32); + debug!( + "count_code_region to LLVM intrinsic instrprof.increment(fn_name={}, hash={:?}, num_counters={:?}, index={:?})", + mangled_fn.name, hash, index, num_counters + ); + self.instrprof_increment(mangled_fn_name, hash, num_counters, index) } "va_start" => self.va_start(args[0].immediate()), "va_end" => self.va_end(args[0].immediate()), diff --git a/src/librustc_codegen_ssa/mir/block.rs b/src/librustc_codegen_ssa/mir/block.rs index d56c816811b3..945c3d484347 100644 --- a/src/librustc_codegen_ssa/mir/block.rs +++ b/src/librustc_codegen_ssa/mir/block.rs @@ -688,12 +688,12 @@ fn codegen_call_terminator( .collect(); bx.codegen_intrinsic_call( + self, *instance.as_ref().unwrap(), &fn_abi, &args, dest, terminator.source_info.span, - self.instance, ); if let ReturnDest::IndirectOperand(dst, _) = ret_dest { diff --git a/src/librustc_codegen_ssa/mir/mod.rs b/src/librustc_codegen_ssa/mir/mod.rs index 00b4bf96afa5..fd2e779f681b 100644 --- a/src/librustc_codegen_ssa/mir/mod.rs +++ b/src/librustc_codegen_ssa/mir/mod.rs @@ -21,9 +21,9 @@ /// Master context for codegenning from MIR. pub struct FunctionCx<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> { - instance: Instance<'tcx>, + pub instance: Instance<'tcx>, - mir: &'tcx mir::Body<'tcx>, + pub mir: &'tcx mir::Body<'tcx>, debug_context: Option>, diff --git a/src/librustc_codegen_ssa/traits/intrinsic.rs b/src/librustc_codegen_ssa/traits/intrinsic.rs index f62019498511..0036eadf4db8 100644 --- a/src/librustc_codegen_ssa/traits/intrinsic.rs +++ b/src/librustc_codegen_ssa/traits/intrinsic.rs @@ -1,5 +1,7 @@ use super::BackendTypes; use crate::mir::operand::OperandRef; +use crate::mir::FunctionCx; +use crate::traits::BuilderMethods; use rustc_middle::ty::{self, Ty}; use rustc_span::Span; use rustc_target::abi::call::FnAbi; @@ -8,14 +10,14 @@ pub trait IntrinsicCallMethods<'tcx>: BackendTypes { /// Remember to add all intrinsics here, in librustc_typeck/check/mod.rs, /// and in libcore/intrinsics.rs; if you need access to any llvm intrinsics, /// add them to librustc_codegen_llvm/context.rs - fn codegen_intrinsic_call( + fn codegen_intrinsic_call<'a, Bx: BuilderMethods<'a, 'tcx>>( &mut self, + fx: &FunctionCx<'a, 'tcx, Bx>, instance: ty::Instance<'tcx>, fn_abi: &FnAbi<'tcx, Ty<'tcx>>, args: &[OperandRef<'tcx, Self::Value>], llresult: Self::Value, span: Span, - caller_instance: ty::Instance<'tcx>, ); fn abort(&mut self); diff --git a/src/librustc_metadata/locator.rs b/src/librustc_metadata/locator.rs index 5a4862d45218..662794f0aa11 100644 --- a/src/librustc_metadata/locator.rs +++ b/src/librustc_metadata/locator.rs @@ -488,6 +488,8 @@ impl<'a> CrateLocator<'a> { && self.triple != TargetTriple::from_triple(config::host_triple()) { err.note(&format!("the `{}` target may not be installed", self.triple)); + } else if self.crate_name == sym::profiler_builtins { + err.note(&"the compiler may have been built without `profiler = true`"); } err.span_label(self.span, "can't find crate"); err diff --git a/src/librustc_middle/ich/hcx.rs b/src/librustc_middle/ich/hcx.rs index 69b4adb3a0e1..f5b0b73c49de 100644 --- a/src/librustc_middle/ich/hcx.rs +++ b/src/librustc_middle/ich/hcx.rs @@ -67,13 +67,15 @@ impl<'a> StableHashingContext<'a> { /// Don't use it for anything else or you'll run the risk of /// leaking data out of the tracking system. #[inline] - pub fn new( + fn new_with_or_without_spans( sess: &'a Session, krate: &'a hir::Crate<'a>, definitions: &'a Definitions, cstore: &'a dyn CrateStore, + always_ignore_spans: bool, ) -> Self { - let hash_spans_initial = !sess.opts.debugging_opts.incremental_ignore_spans; + let hash_spans_initial = + !always_ignore_spans && !sess.opts.debugging_opts.incremental_ignore_spans; StableHashingContext { sess, @@ -88,6 +90,33 @@ pub fn new( } } + #[inline] + pub fn new( + sess: &'a Session, + krate: &'a hir::Crate<'a>, + definitions: &'a Definitions, + cstore: &'a dyn CrateStore, + ) -> Self { + Self::new_with_or_without_spans( + sess, + krate, + definitions, + cstore, + /*always_ignore_spans=*/ false, + ) + } + + #[inline] + pub fn ignore_spans( + sess: &'a Session, + krate: &'a hir::Crate<'a>, + definitions: &'a Definitions, + cstore: &'a dyn CrateStore, + ) -> Self { + let always_ignore_spans = true; + Self::new_with_or_without_spans(sess, krate, definitions, cstore, always_ignore_spans) + } + #[inline] pub fn sess(&self) -> &'a Session { self.sess diff --git a/src/librustc_middle/mir/mod.rs b/src/librustc_middle/mir/mod.rs index 3381b95c2a38..a89a5ef3f821 100644 --- a/src/librustc_middle/mir/mod.rs +++ b/src/librustc_middle/mir/mod.rs @@ -88,6 +88,19 @@ pub fn phase_index(&self) -> usize { } } +/// Coverage data computed by the `InstrumentCoverage` MIR pass, when compiling with +/// `-Zinstrument_coverage`. +#[derive(Clone, RustcEncodable, RustcDecodable, Debug, HashStable, TypeFoldable)] +pub struct CoverageData { + /// A hash value that can be used by the consumer of the coverage profile data to detect + /// changes to the instrumented source of the associated MIR body (typically, for an + /// individual function). + pub hash: u64, + + /// The total number of coverage region counters added to this MIR Body. + pub num_counters: usize, +} + /// The lowered representation of a single function. #[derive(Clone, RustcEncodable, RustcDecodable, Debug, HashStable, TypeFoldable)] pub struct Body<'tcx> { @@ -164,13 +177,17 @@ pub struct Body<'tcx> { /// The user may be writing e.g. `&[(SOME_CELL, 42)][i].1` and this would get promoted, because /// we'd statically know that no thing with interior mutability will ever be available to the /// user without some serious unsafe code. Now this means that our promoted is actually - /// `&[(SOME_CELL, 42)]` and the MIR using it will do the `&promoted[i].1` projection because the - /// index may be a runtime value. Such a promoted value is illegal because it has reachable + /// `&[(SOME_CELL, 42)]` and the MIR using it will do the `&promoted[i].1` projection because + /// the index may be a runtime value. Such a promoted value is illegal because it has reachable /// interior mutability. This flag just makes this situation very obvious where the previous /// implementation without the flag hid this situation silently. /// FIXME(oli-obk): rewrite the promoted during promotion to eliminate the cell components. pub ignore_interior_mut_in_const_validation: bool, + /// If compiling with `-Zinstrument_coverage`, the `InstrumentCoverage` pass stores summary + /// information associated with the MIR, used in code generation of the coverage counters. + pub coverage_data: Option, + predecessor_cache: PredecessorCache, } @@ -211,6 +228,7 @@ pub fn new( required_consts: Vec::new(), ignore_interior_mut_in_const_validation: false, control_flow_destroyed, + coverage_data: None, predecessor_cache: PredecessorCache::new(), } } @@ -238,6 +256,7 @@ pub fn new_cfg_only(basic_blocks: IndexVec>) -> generator_kind: None, var_debug_info: Vec::new(), ignore_interior_mut_in_const_validation: false, + coverage_data: None, predecessor_cache: PredecessorCache::new(), } } diff --git a/src/librustc_middle/ty/context.rs b/src/librustc_middle/ty/context.rs index 62d6de2d71e6..0696cae4810e 100644 --- a/src/librustc_middle/ty/context.rs +++ b/src/librustc_middle/ty/context.rs @@ -1284,6 +1284,13 @@ pub fn create_stable_hashing_context(self) -> StableHashingContext<'tcx> { StableHashingContext::new(self.sess, krate, self.definitions, &*self.cstore) } + #[inline(always)] + pub fn create_no_span_stable_hashing_context(self) -> StableHashingContext<'tcx> { + let krate = self.gcx.untracked_crate; + + StableHashingContext::ignore_spans(self.sess, krate, self.definitions, &*self.cstore) + } + // This method makes sure that we have a DepNode and a Fingerprint for // every upstream crate. It needs to be called once right after the tcx is // created. diff --git a/src/librustc_mir/transform/instrument_coverage.rs b/src/librustc_mir/transform/instrument_coverage.rs index c36614938e10..793ccbb081be 100644 --- a/src/librustc_mir/transform/instrument_coverage.rs +++ b/src/librustc_mir/transform/instrument_coverage.rs @@ -1,8 +1,15 @@ use crate::transform::{MirPass, MirSource}; use crate::util::patch::MirPatch; +use rustc_data_structures::fingerprint::Fingerprint; +use rustc_data_structures::stable_hasher::{HashStable, StableHasher}; use rustc_hir::lang_items; +use rustc_hir::*; +use rustc_middle::ich::StableHashingContext; use rustc_middle::mir::interpret::Scalar; -use rustc_middle::mir::*; +use rustc_middle::mir::{ + self, BasicBlock, BasicBlockData, CoverageData, Operand, Place, SourceInfo, StatementKind, + Terminator, TerminatorKind, START_BLOCK, +}; use rustc_middle::ty; use rustc_middle::ty::TyCtxt; use rustc_span::def_id::DefId; @@ -12,64 +19,104 @@ /// the intrinsic llvm.instrprof.increment. pub struct InstrumentCoverage; +struct Instrumentor<'tcx> { + tcx: TyCtxt<'tcx>, + num_counters: usize, +} + impl<'tcx> MirPass<'tcx> for InstrumentCoverage { - fn run_pass(&self, tcx: TyCtxt<'tcx>, src: MirSource<'tcx>, body: &mut Body<'tcx>) { + fn run_pass(&self, tcx: TyCtxt<'tcx>, src: MirSource<'tcx>, mir_body: &mut mir::Body<'tcx>) { if tcx.sess.opts.debugging_opts.instrument_coverage { - debug!("instrumenting {:?}", src.def_id()); - instrument_coverage(tcx, body); + // If the InstrumentCoverage pass is called on promoted MIRs, skip them. + // See: https://github.com/rust-lang/rust/pull/73011#discussion_r438317601 + if src.promoted.is_none() { + assert!(mir_body.coverage_data.is_none()); + + let hash = hash_mir_source(tcx, &src); + + debug!( + "instrumenting {:?}, hash: {}, span: {}", + src.def_id(), + hash, + tcx.sess.source_map().span_to_string(mir_body.span) + ); + + let num_counters = Instrumentor::new(tcx).inject_counters(mir_body); + + mir_body.coverage_data = Some(CoverageData { hash, num_counters }); + } } } } -// The first counter (start of the function) is index zero. -const INIT_FUNCTION_COUNTER: u32 = 0; +impl<'tcx> Instrumentor<'tcx> { + fn new(tcx: TyCtxt<'tcx>) -> Self { + Self { tcx, num_counters: 0 } + } -/// Injects calls to placeholder function `count_code_region()`. -// FIXME(richkadel): As a first step, counters are only injected at the top of each function. -// The complete solution will inject counters at each conditional code branch. -pub fn instrument_coverage<'tcx>(tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) { - let span = body.span.shrink_to_lo(); + fn next_counter(&mut self) -> u32 { + let next = self.num_counters as u32; + self.num_counters += 1; + next + } - let count_code_region_fn = function_handle( - tcx, - tcx.require_lang_item(lang_items::CountCodeRegionFnLangItem, None), - span, - ); - let counter_index = Operand::const_from_scalar( - tcx, - tcx.types.u32, - Scalar::from_u32(INIT_FUNCTION_COUNTER), - span, - ); + fn inject_counters(&mut self, mir_body: &mut mir::Body<'tcx>) -> usize { + // FIXME(richkadel): As a first step, counters are only injected at the top of each + // function. The complete solution will inject counters at each conditional code branch. + let top_of_function = START_BLOCK; + let entire_function = mir_body.span; - let mut patch = MirPatch::new(body); + self.inject_counter(mir_body, top_of_function, entire_function); - let new_block = patch.new_block(placeholder_block(SourceInfo::outermost(body.span))); - let next_block = START_BLOCK; + self.num_counters + } - let temp = patch.new_temp(tcx.mk_unit(), body.span); - patch.patch_terminator( - new_block, - TerminatorKind::Call { - func: count_code_region_fn, - args: vec![counter_index], - // new_block will swapped with the next_block, after applying patch - destination: Some((Place::from(temp), new_block)), - cleanup: None, - from_hir_call: false, - fn_span: span, - }, - ); + fn inject_counter( + &mut self, + mir_body: &mut mir::Body<'tcx>, + next_block: BasicBlock, + code_region: Span, + ) { + let injection_point = code_region.shrink_to_lo(); - patch.add_statement(new_block.start_location(), StatementKind::StorageLive(temp)); - patch.add_statement(next_block.start_location(), StatementKind::StorageDead(temp)); + let count_code_region_fn = function_handle( + self.tcx, + self.tcx.require_lang_item(lang_items::CountCodeRegionFnLangItem, None), + injection_point, + ); + let counter_index = Operand::const_from_scalar( + self.tcx, + self.tcx.types.u32, + Scalar::from_u32(self.next_counter()), + injection_point, + ); - patch.apply(body); + let mut patch = MirPatch::new(mir_body); - // To insert the `new_block` in front of the first block in the counted branch (for example, - // the START_BLOCK, at the top of the function), just swap the indexes, leaving the rest of the - // graph unchanged. - body.basic_blocks_mut().swap(next_block, new_block); + let temp = patch.new_temp(self.tcx.mk_unit(), code_region); + let new_block = patch.new_block(placeholder_block(code_region)); + patch.patch_terminator( + new_block, + TerminatorKind::Call { + func: count_code_region_fn, + args: vec![counter_index], + // new_block will swapped with the next_block, after applying patch + destination: Some((Place::from(temp), new_block)), + cleanup: None, + from_hir_call: false, + fn_span: injection_point, + }, + ); + + patch.add_statement(new_block.start_location(), StatementKind::StorageLive(temp)); + patch.add_statement(next_block.start_location(), StatementKind::StorageDead(temp)); + + patch.apply(mir_body); + + // To insert the `new_block` in front of the first block in the counted branch (the + // `next_block`), just swap the indexes, leaving the rest of the graph unchanged. + mir_body.basic_blocks_mut().swap(next_block, new_block); + } } fn function_handle<'tcx>(tcx: TyCtxt<'tcx>, fn_def_id: DefId, span: Span) -> Operand<'tcx> { @@ -79,14 +126,59 @@ fn function_handle<'tcx>(tcx: TyCtxt<'tcx>, fn_def_id: DefId, span: Span) -> Ope Operand::function_handle(tcx, fn_def_id, substs, span) } -fn placeholder_block<'tcx>(source_info: SourceInfo) -> BasicBlockData<'tcx> { +fn placeholder_block(span: Span) -> BasicBlockData<'tcx> { BasicBlockData { statements: vec![], terminator: Some(Terminator { - source_info, + source_info: SourceInfo::outermost(span), // this gets overwritten by the counter Call kind: TerminatorKind::Unreachable, }), is_cleanup: false, } } + +fn hash_mir_source<'tcx>(tcx: TyCtxt<'tcx>, src: &MirSource<'tcx>) -> u64 { + let fn_body_id = match tcx.hir().get_if_local(src.def_id()) { + Some(node) => match associated_body(node) { + Some(body_id) => body_id, + _ => bug!("instrumented MirSource does not include a function body: {:?}", node), + }, + None => bug!("instrumented MirSource is not local: {:?}", src), + }; + let hir_body = tcx.hir().body(fn_body_id); + let mut hcx = tcx.create_no_span_stable_hashing_context(); + hash(&mut hcx, &hir_body.value).to_smaller_hash() +} + +fn hash( + hcx: &mut StableHashingContext<'tcx>, + node: &impl HashStable>, +) -> Fingerprint { + let mut stable_hasher = StableHasher::new(); + node.hash_stable(hcx, &mut stable_hasher); + stable_hasher.finish() +} + +fn associated_body<'hir>(node: Node<'hir>) -> Option { + match node { + Node::Item(Item { + kind: ItemKind::Const(_, body) | ItemKind::Static(.., body) | ItemKind::Fn(.., body), + .. + }) + | Node::TraitItem(TraitItem { + kind: + TraitItemKind::Const(_, Some(body)) | TraitItemKind::Fn(_, TraitFn::Provided(body)), + .. + }) + | Node::ImplItem(ImplItem { + kind: ImplItemKind::Const(_, body) | ImplItemKind::Fn(_, body), + .. + }) + | Node::Expr(Expr { kind: ExprKind::Closure(.., body, _, _), .. }) => Some(*body), + + Node::AnonConst(constant) => Some(constant.body), + + _ => None, + } +} diff --git a/src/librustc_session/options.rs b/src/librustc_session/options.rs index 2d231359057f..c2d056ad26d0 100644 --- a/src/librustc_session/options.rs +++ b/src/librustc_session/options.rs @@ -877,8 +877,9 @@ fn parse_target_feature(slot: &mut String, v: Option<&str>) -> bool { (such as entering an empty infinite loop) by inserting llvm.sideeffect \ (default: no)"), instrument_coverage: bool = (false, parse_bool, [TRACKED], - "instrument the generated code with LLVM code region counters to \ - (in the future) generate coverage reports (experimental; default: no)"), + "instrument the generated code with LLVM code region counters to (in the \ + future) generate coverage reports (default: no; note, the compiler build \ + config must include `profiler = true`)"), instrument_mcount: bool = (false, parse_bool, [TRACKED], "insert function instrument code for mcount-based tracing (default: no)"), keep_hygiene_data: bool = (false, parse_bool, [UNTRACKED], diff --git a/src/librustc_span/symbol.rs b/src/librustc_span/symbol.rs index 970a26325926..6efe9b20d0d8 100644 --- a/src/librustc_span/symbol.rs +++ b/src/librustc_span/symbol.rs @@ -587,6 +587,7 @@ proc_macro_mod, proc_macro_non_items, proc_macro_path_invoc, + profiler_builtins, profiler_runtime, ptr_offset_from, pub_restricted, From 8fc2eeb1c81aef42855257adc11791dcffe803cb Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Wed, 10 Jun 2020 14:24:28 -0700 Subject: [PATCH 05/24] Use newtype to map from `Local` to `GeneratorSavedLocal` --- src/librustc_mir/transform/generator.rs | 110 ++++++++++++++---------- 1 file changed, 65 insertions(+), 45 deletions(-) diff --git a/src/librustc_mir/transform/generator.rs b/src/librustc_mir/transform/generator.rs index c8702eeae1d5..1445315c6b18 100644 --- a/src/librustc_mir/transform/generator.rs +++ b/src/librustc_mir/transform/generator.rs @@ -72,7 +72,7 @@ use rustc_target::abi::VariantIdx; use rustc_target::spec::PanicStrategy; use std::borrow::Cow; -use std::iter; +use std::{iter, ops}; pub struct StateTransform; @@ -417,11 +417,7 @@ fn replace_local<'tcx>( struct LivenessInfo { /// Which locals are live across any suspension point. - /// - /// GeneratorSavedLocal is indexed in terms of the elements in this set; - /// i.e. GeneratorSavedLocal::new(1) corresponds to the second local - /// included in this set. - live_locals: BitSet, + saved_locals: GeneratorSavedLocals, /// The set of saved locals live at each suspension point. live_locals_at_suspension_points: Vec>, @@ -524,49 +520,75 @@ fn locals_live_across_suspend_points( live_locals_at_suspension_points.push(live_locals); } } + debug!("live_locals_anywhere = {:?}", live_locals_at_any_suspension_point); + let saved_locals = GeneratorSavedLocals(live_locals_at_any_suspension_point); // Renumber our liveness_map bitsets to include only the locals we are // saving. let live_locals_at_suspension_points = live_locals_at_suspension_points .iter() - .map(|live_here| renumber_bitset(&live_here, &live_locals_at_any_suspension_point)) + .map(|live_here| saved_locals.renumber_bitset(&live_here)) .collect(); let storage_conflicts = compute_storage_conflicts( body_ref, - &live_locals_at_any_suspension_point, + &saved_locals, always_live_locals.clone(), requires_storage_results, ); LivenessInfo { - live_locals: live_locals_at_any_suspension_point, + saved_locals, live_locals_at_suspension_points, storage_conflicts, storage_liveness: storage_liveness_map, } } -/// Renumbers the items present in `stored_locals` and applies the renumbering -/// to 'input`. +/// The set of `Local`s that must be saved across yield points. /// -/// For example, if `stored_locals = [1, 3, 5]`, this would be renumbered to -/// `[0, 1, 2]`. Thus, if `input = [3, 5]` we would return `[1, 2]`. -fn renumber_bitset( - input: &BitSet, - stored_locals: &BitSet, -) -> BitSet { - assert!(stored_locals.superset(&input), "{:?} not a superset of {:?}", stored_locals, input); - let mut out = BitSet::new_empty(stored_locals.count()); - for (idx, local) in stored_locals.iter().enumerate() { - let saved_local = GeneratorSavedLocal::from(idx); - if input.contains(local) { - out.insert(saved_local); - } +/// `GeneratorSavedLocal` is indexed in terms of the elements in this set; +/// i.e. `GeneratorSavedLocal::new(1)` corresponds to the second local +/// included in this set. +struct GeneratorSavedLocals(BitSet); + +impl GeneratorSavedLocals { + /// Returns an iterator over each `GeneratorSavedLocal` along with the `Local` it corresponds + /// to. + fn iter_enumerated(&self) -> impl '_ + Iterator { + self.iter().enumerate().map(|(i, l)| (GeneratorSavedLocal::from(i), l)) + } + + /// Transforms a `BitSet` that contains only locals saved across yield points to the + /// equivalent `BitSet`. + fn renumber_bitset(&self, input: &BitSet) -> BitSet { + assert!(self.superset(&input), "{:?} not a superset of {:?}", self.0, input); + let mut out = BitSet::new_empty(self.count()); + for (saved_local, local) in self.iter_enumerated() { + if input.contains(local) { + out.insert(saved_local); + } + } + out + } + + fn get(&self, local: Local) -> Option { + if !self.contains(local) { + return None; + } + + let idx = self.iter().take_while(|&l| l < local).count(); + Some(GeneratorSavedLocal::new(idx)) + } +} + +impl ops::Deref for GeneratorSavedLocals { + type Target = BitSet; + + fn deref(&self) -> &Self::Target { + &self.0 } - debug!("renumber_bitset({:?}, {:?}) => {:?}", input, stored_locals, out); - out } /// For every saved local, looks for which locals are StorageLive at the same @@ -575,11 +597,11 @@ fn renumber_bitset( /// computation; see `GeneratorLayout` for more. fn compute_storage_conflicts( body: &'mir Body<'tcx>, - stored_locals: &BitSet, + saved_locals: &GeneratorSavedLocals, always_live_locals: storage::AlwaysLiveLocals, requires_storage: dataflow::Results<'tcx, MaybeRequiresStorage<'mir, 'tcx>>, ) -> BitMatrix { - assert_eq!(body.local_decls.len(), stored_locals.domain_size()); + assert_eq!(body.local_decls.len(), saved_locals.domain_size()); debug!("compute_storage_conflicts({:?})", body.span); debug!("always_live = {:?}", always_live_locals); @@ -587,12 +609,12 @@ fn compute_storage_conflicts( // Locals that are always live or ones that need to be stored across // suspension points are not eligible for overlap. let mut ineligible_locals = always_live_locals.into_inner(); - ineligible_locals.intersect(stored_locals); + ineligible_locals.intersect(saved_locals); // Compute the storage conflicts for all eligible locals. let mut visitor = StorageConflictVisitor { body, - stored_locals: &stored_locals, + saved_locals: &saved_locals, local_conflicts: BitMatrix::from_row_n(&ineligible_locals, body.local_decls.len()), }; @@ -609,16 +631,14 @@ fn compute_storage_conflicts( // However, in practice these bitsets are not usually large. The layout code // also needs to keep track of how many conflicts each local has, so it's // simpler to keep it this way for now. - let mut storage_conflicts = BitMatrix::new(stored_locals.count(), stored_locals.count()); - for (idx_a, local_a) in stored_locals.iter().enumerate() { - let saved_local_a = GeneratorSavedLocal::new(idx_a); + let mut storage_conflicts = BitMatrix::new(saved_locals.count(), saved_locals.count()); + for (saved_local_a, local_a) in saved_locals.iter_enumerated() { if ineligible_locals.contains(local_a) { // Conflicts with everything. storage_conflicts.insert_all_into_row(saved_local_a); } else { // Keep overlap information only for stored locals. - for (idx_b, local_b) in stored_locals.iter().enumerate() { - let saved_local_b = GeneratorSavedLocal::new(idx_b); + for (saved_local_b, local_b) in saved_locals.iter_enumerated() { if local_conflicts.contains(local_a, local_b) { storage_conflicts.insert(saved_local_a, saved_local_b); } @@ -630,7 +650,7 @@ fn compute_storage_conflicts( struct StorageConflictVisitor<'mir, 'tcx, 's> { body: &'mir Body<'tcx>, - stored_locals: &'s BitSet, + saved_locals: &'s GeneratorSavedLocals, // FIXME(tmandry): Consider using sparse bitsets here once we have good // benchmarks for generators. local_conflicts: BitMatrix, @@ -666,7 +686,7 @@ fn apply_state(&mut self, flow_state: &BitSet, loc: Location) { } let mut eligible_storage_live = flow_state.clone(); - eligible_storage_live.intersect(&self.stored_locals); + eligible_storage_live.intersect(&self.saved_locals); for local in eligible_storage_live.iter() { self.local_conflicts.union_row_with(&eligible_storage_live, local); @@ -678,7 +698,7 @@ fn apply_state(&mut self, flow_state: &BitSet, loc: Location) { } } -/// Validates the typeck view of the generator against the actual set of types retained between +/// Validates the typeck view of the generator against the actual set of types saved between /// yield points. fn sanitize_witness<'tcx>( tcx: TyCtxt<'tcx>, @@ -686,7 +706,7 @@ fn sanitize_witness<'tcx>( did: DefId, witness: Ty<'tcx>, upvars: &Vec>, - retained: &BitSet, + saved_locals: &GeneratorSavedLocals, ) { let allowed_upvars = tcx.erase_regions(upvars); let allowed = match witness.kind { @@ -703,8 +723,8 @@ fn sanitize_witness<'tcx>( let param_env = tcx.param_env(did); for (local, decl) in body.local_decls.iter_enumerated() { - // Ignore locals which are internal or not retained between yields. - if !retained.contains(local) || decl.internal { + // Ignore locals which are internal or not saved between yields. + if !saved_locals.contains(local) || decl.internal { continue; } let decl_ty = tcx.normalize_erasing_regions(param_env, decl.ty); @@ -738,21 +758,21 @@ fn compute_layout<'tcx>( ) { // Use a liveness analysis to compute locals which are live across a suspension point let LivenessInfo { - live_locals, + saved_locals, live_locals_at_suspension_points, storage_conflicts, storage_liveness, } = locals_live_across_suspend_points(tcx, body, source, always_live_locals, movable); - sanitize_witness(tcx, body, source.def_id(), interior, upvars, &live_locals); + sanitize_witness(tcx, body, source.def_id(), interior, upvars, &saved_locals); // Gather live local types and their indices. let mut locals = IndexVec::::new(); let mut tys = IndexVec::::new(); - for (idx, local) in live_locals.iter().enumerate() { + for (saved_local, local) in saved_locals.iter_enumerated() { locals.push(local); tys.push(body.local_decls[local].ty); - debug!("generator saved local {:?} => {:?}", GeneratorSavedLocal::from(idx), local); + debug!("generator saved local {:?} => {:?}", saved_local, local); } // Leave empty variants for the UNRESUMED, RETURNED, and POISONED states. From c178e6436a4c3f34b8790a908de044bba9401284 Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Wed, 10 Jun 2020 14:00:59 -0700 Subject: [PATCH 06/24] Look for stores between non-conflicting generator saved locals This is to prevent the miscompilation in #73137 from reappearing. Only runs with `-Zvalidate-mir`. --- src/librustc_mir/transform/generator.rs | 160 ++++++++++++++++++++++-- 1 file changed, 147 insertions(+), 13 deletions(-) diff --git a/src/librustc_mir/transform/generator.rs b/src/librustc_mir/transform/generator.rs index 1445315c6b18..9a15fba2bc8a 100644 --- a/src/librustc_mir/transform/generator.rs +++ b/src/librustc_mir/transform/generator.rs @@ -64,7 +64,7 @@ use rustc_hir::lang_items::{GeneratorStateLangItem, PinTypeLangItem}; use rustc_index::bit_set::{BitMatrix, BitSet}; use rustc_index::vec::{Idx, IndexVec}; -use rustc_middle::mir::visit::{MutVisitor, PlaceContext}; +use rustc_middle::mir::visit::{MutVisitor, PlaceContext, Visitor}; use rustc_middle::mir::*; use rustc_middle::ty::subst::SubstsRef; use rustc_middle::ty::GeneratorSubsts; @@ -744,27 +744,19 @@ fn sanitize_witness<'tcx>( } fn compute_layout<'tcx>( - tcx: TyCtxt<'tcx>, - source: MirSource<'tcx>, - upvars: &Vec>, - interior: Ty<'tcx>, - always_live_locals: &storage::AlwaysLiveLocals, - movable: bool, + liveness: LivenessInfo, body: &mut Body<'tcx>, ) -> ( FxHashMap, VariantIdx, usize)>, GeneratorLayout<'tcx>, IndexVec>>, ) { - // Use a liveness analysis to compute locals which are live across a suspension point let LivenessInfo { saved_locals, live_locals_at_suspension_points, storage_conflicts, storage_liveness, - } = locals_live_across_suspend_points(tcx, body, source, always_live_locals, movable); - - sanitize_witness(tcx, body, source.def_id(), interior, upvars, &saved_locals); + } = liveness; // Gather live local types and their indices. let mut locals = IndexVec::::new(); @@ -1280,11 +1272,25 @@ fn run_pass(&self, tcx: TyCtxt<'tcx>, source: MirSource<'tcx>, body: &mut Body<' let always_live_locals = storage::AlwaysLiveLocals::new(&body); + let liveness_info = + locals_live_across_suspend_points(tcx, body, source, &always_live_locals, movable); + + sanitize_witness(tcx, body, def_id, interior, &upvars, &liveness_info.saved_locals); + + if tcx.sess.opts.debugging_opts.validate_mir { + let mut vis = EnsureGeneratorFieldAssignmentsNeverAlias { + assigned_local: None, + saved_locals: &liveness_info.saved_locals, + storage_conflicts: &liveness_info.storage_conflicts, + }; + + vis.visit_body(body); + } + // Extract locals which are live across suspension point into `layout` // `remap` gives a mapping from local indices onto generator struct indices // `storage_liveness` tells us which locals have live storage at suspension points - let (remap, layout, storage_liveness) = - compute_layout(tcx, source, &upvars, interior, &always_live_locals, movable, body); + let (remap, layout, storage_liveness) = compute_layout(liveness_info, body); let can_return = can_return(tcx, body); @@ -1335,3 +1341,131 @@ fn run_pass(&self, tcx: TyCtxt<'tcx>, source: MirSource<'tcx>, body: &mut Body<' create_generator_resume_function(tcx, transform, source, body, can_return); } } + +/// Looks for any assignments between locals (e.g., `_4 = _5`) that will both be converted to fields +/// in the generator state machine but whose storage does not conflict. +/// +/// Validation needs to happen immediately *before* `TransformVisitor` is invoked, not after. +/// +/// This condition would arise when the assignment is the last use of `_5` but the initial +/// definition of `_4` if we weren't extra careful to mark all locals used inside a statement as +/// conflicting. Non-conflicting generator saved locals may be stored at the same location within +/// the generator state machine, which would result in ill-formed MIR: the left-hand and right-hand +/// sides of an assignment may not alias. This caused a miscompilation in [#73137]. +/// +/// [#73137]: https://github.com/rust-lang/rust/issues/73137 +struct EnsureGeneratorFieldAssignmentsNeverAlias<'a> { + saved_locals: &'a GeneratorSavedLocals, + storage_conflicts: &'a BitMatrix, + assigned_local: Option, +} + +impl EnsureGeneratorFieldAssignmentsNeverAlias<'_> { + fn saved_local_for_direct_place(&self, place: Place<'_>) -> Option { + if place.is_indirect() { + return None; + } + + self.saved_locals.get(place.local) + } + + fn check_assigned_place(&mut self, place: Place<'tcx>, f: impl FnOnce(&mut Self)) { + if let Some(assigned_local) = self.saved_local_for_direct_place(place) { + assert!(self.assigned_local.is_none(), "`check_assigned_local` must not recurse"); + + self.assigned_local = Some(assigned_local); + f(self); + self.assigned_local = None; + } + } +} + +impl Visitor<'tcx> for EnsureGeneratorFieldAssignmentsNeverAlias<'_> { + fn visit_place(&mut self, place: &Place<'tcx>, context: PlaceContext, location: Location) { + let lhs = match self.assigned_local { + Some(l) => l, + None => { + // We should be visiting all places used in the MIR. + assert!(!context.is_use()); + return; + } + }; + + let rhs = match self.saved_local_for_direct_place(*place) { + Some(l) => l, + None => return, + }; + + if !self.storage_conflicts.contains(lhs, rhs) { + bug!( + "Assignment between generator saved locals whose storage does not conflict: \ + {:?}: {:?} = {:?}", + location, + lhs, + rhs, + ); + } + } + + fn visit_statement(&mut self, statement: &Statement<'tcx>, location: Location) { + match &statement.kind { + StatementKind::Assign(box (lhs, rhs)) => { + self.check_assigned_place(*lhs, |this| this.visit_rvalue(rhs, location)); + } + + // FIXME: Does `llvm_asm!` have any aliasing requirements? + StatementKind::LlvmInlineAsm(_) => {} + + StatementKind::FakeRead(..) + | StatementKind::SetDiscriminant { .. } + | StatementKind::StorageLive(_) + | StatementKind::StorageDead(_) + | StatementKind::Retag(..) + | StatementKind::AscribeUserType(..) + | StatementKind::Nop => {} + } + } + + fn visit_terminator(&mut self, terminator: &Terminator<'tcx>, location: Location) { + // Checking for aliasing in terminators is probably overkill, but until we have actual + // semantics, we should be conservative here. + match &terminator.kind { + TerminatorKind::Call { + func, + args, + destination: Some((dest, _)), + cleanup: _, + from_hir_call: _, + fn_span: _, + } => { + self.check_assigned_place(*dest, |this| { + this.visit_operand(func, location); + for arg in args { + this.visit_operand(arg, location); + } + }); + } + + TerminatorKind::Yield { value, resume: _, resume_arg, drop: _ } => { + self.check_assigned_place(*resume_arg, |this| this.visit_operand(value, location)); + } + + // FIXME: Does `asm!` have any aliasing requirements? + TerminatorKind::InlineAsm { .. } => {} + + TerminatorKind::Call { .. } + | TerminatorKind::Goto { .. } + | TerminatorKind::SwitchInt { .. } + | TerminatorKind::Resume + | TerminatorKind::Abort + | TerminatorKind::Return + | TerminatorKind::Unreachable + | TerminatorKind::Drop { .. } + | TerminatorKind::DropAndReplace { .. } + | TerminatorKind::Assert { .. } + | TerminatorKind::GeneratorDrop + | TerminatorKind::FalseEdge { .. } + | TerminatorKind::FalseUnwind { .. } => {} + } + } +} From b2ec645d16ce9a3345b2f9cb527ca52e86e54324 Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Fri, 19 Jun 2020 10:27:10 -0700 Subject: [PATCH 07/24] Incorporate review suggestions Co-authored-by: Tyler Mandry --- src/librustc_mir/transform/generator.rs | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/librustc_mir/transform/generator.rs b/src/librustc_mir/transform/generator.rs index 9a15fba2bc8a..59be8dc224de 100644 --- a/src/librustc_mir/transform/generator.rs +++ b/src/librustc_mir/transform/generator.rs @@ -1343,7 +1343,7 @@ fn run_pass(&self, tcx: TyCtxt<'tcx>, source: MirSource<'tcx>, body: &mut Body<' } /// Looks for any assignments between locals (e.g., `_4 = _5`) that will both be converted to fields -/// in the generator state machine but whose storage does not conflict. +/// in the generator state machine but whose storage is not marked as conflicting /// /// Validation needs to happen immediately *before* `TransformVisitor` is invoked, not after. /// @@ -1371,7 +1371,7 @@ fn saved_local_for_direct_place(&self, place: Place<'_>) -> Option, f: impl FnOnce(&mut Self)) { if let Some(assigned_local) = self.saved_local_for_direct_place(place) { - assert!(self.assigned_local.is_none(), "`check_assigned_local` must not recurse"); + assert!(self.assigned_local.is_none(), "`check_assigned_place` must not recurse"); self.assigned_local = Some(assigned_local); f(self); @@ -1385,7 +1385,10 @@ fn visit_place(&mut self, place: &Place<'tcx>, context: PlaceContext, location: let lhs = match self.assigned_local { Some(l) => l, None => { - // We should be visiting all places used in the MIR. + // This visitor only invokes `visit_place` for the right-hand side of an assignment + // and only after setting `self.assigned_local`. However, the default impl of + // `Visitor::super_body` may call `visit_place` with a `NonUseContext` for places + // with debuginfo. Ignore them here. assert!(!context.is_use()); return; } @@ -1398,8 +1401,8 @@ fn visit_place(&mut self, place: &Place<'tcx>, context: PlaceContext, location: if !self.storage_conflicts.contains(lhs, rhs) { bug!( - "Assignment between generator saved locals whose storage does not conflict: \ - {:?}: {:?} = {:?}", + "Assignment between generator saved locals whose storage is not \ + marked as conflicting: {:?}: {:?} = {:?}", location, lhs, rhs, From 95f8daa82b52e95081b66d58953c2329a9f0458e Mon Sep 17 00:00:00 2001 From: Joshua Nelson Date: Fri, 19 Jun 2020 20:06:49 -0400 Subject: [PATCH 08/24] Fix -Z unpretty=everybody_loops It turns out that this has not been working for who knows how long. Previously: ``` pub fn h() { 1 + 2; } ``` After this change: ``` pub fn h() { loop {} } ``` This only affected the pass when run with the command line pretty-printing option, so rustdoc was still replacing bodies with `loop {}`. --- src/librustc_driver/lib.rs | 1 + src/librustc_interface/interface.rs | 1 + src/librustc_interface/passes.rs | 3 +++ src/librustc_interface/queries.rs | 1 + src/librustc_session/config.rs | 7 +++++-- 5 files changed, 11 insertions(+), 2 deletions(-) diff --git a/src/librustc_driver/lib.rs b/src/librustc_driver/lib.rs index ccea041699ee..b45ab0f80ffa 100644 --- a/src/librustc_driver/lib.rs +++ b/src/librustc_driver/lib.rs @@ -307,6 +307,7 @@ pub fn run_compiler( compiler.output_file().as_ref().map(|p| &**p), ); } + trace!("finished pretty-printing"); return early_exit(); } diff --git a/src/librustc_interface/interface.rs b/src/librustc_interface/interface.rs index 5aad64f84cee..920cc6021e68 100644 --- a/src/librustc_interface/interface.rs +++ b/src/librustc_interface/interface.rs @@ -202,6 +202,7 @@ pub fn run_compiler_in_existing_thread_pool( } pub fn run_compiler(mut config: Config, f: impl FnOnce(&Compiler) -> R + Send) -> R { + log::trace!("run_compiler"); let stderr = config.stderr.take(); util::spawn_thread_pool( config.opts.edition, diff --git a/src/librustc_interface/passes.rs b/src/librustc_interface/passes.rs index 1ed9bc3f1f50..c0a67f20a1e8 100644 --- a/src/librustc_interface/passes.rs +++ b/src/librustc_interface/passes.rs @@ -101,6 +101,7 @@ pub fn configure_and_expand( krate: ast::Crate, crate_name: &str, ) -> Result<(ast::Crate, BoxedResolver)> { + log::trace!("configure_and_expand"); // Currently, we ignore the name resolution data structures for the purposes of dependency // tracking. Instead we will run name resolution and include its output in the hash of each // item, much like we do for macro expansion. In other words, the hash reflects not just @@ -230,6 +231,7 @@ fn configure_and_expand_inner<'a>( resolver_arenas: &'a ResolverArenas<'a>, metadata_loader: &'a MetadataLoaderDyn, ) -> Result<(ast::Crate, Resolver<'a>)> { + log::trace!("configure_and_expand_inner"); pre_expansion_lint(sess, lint_store, &krate); let mut resolver = Resolver::new(sess, &krate, crate_name, metadata_loader, &resolver_arenas); @@ -357,6 +359,7 @@ fn configure_and_expand_inner<'a>( should_loop |= true; } if should_loop { + log::debug!("replacing bodies with loop {{}}"); util::ReplaceBodyWithLoop::new(&mut resolver).visit_crate(&mut krate); } diff --git a/src/librustc_interface/queries.rs b/src/librustc_interface/queries.rs index 283be165c192..4a41c3e97caf 100644 --- a/src/librustc_interface/queries.rs +++ b/src/librustc_interface/queries.rs @@ -169,6 +169,7 @@ pub fn crate_name(&self) -> Result<&Query> { pub fn expansion( &self, ) -> Result<&Query<(ast::Crate, Steal>>, Lrc)>> { + log::trace!("expansion"); self.expansion.compute(|| { let crate_name = self.crate_name()?.peek().clone(); let (krate, lint_store) = self.register_plugins()?.take(); diff --git a/src/librustc_session/config.rs b/src/librustc_session/config.rs index 411a6eecbba1..ff94a43c4f1a 100644 --- a/src/librustc_session/config.rs +++ b/src/librustc_session/config.rs @@ -1826,6 +1826,7 @@ fn parse_pretty_inner(efmt: ErrorOutputType, name: &str, extended: bool) -> PpMo } } }; + log::debug!("got unpretty option: {:?}", first); first } } @@ -1954,9 +1955,11 @@ pub fn needs_ast_map(&self) -> bool { use PpMode::*; use PpSourceMode::*; match *self { - PpmSource(PpmNormal | PpmEveryBodyLoops | PpmIdentified) => false, + PpmSource(PpmNormal | PpmIdentified) => false, - PpmSource(PpmExpanded | PpmExpandedIdentified | PpmExpandedHygiene) + PpmSource( + PpmExpanded | PpmEveryBodyLoops | PpmExpandedIdentified | PpmExpandedHygiene, + ) | PpmHir(_) | PpmHirTree(_) | PpmMir From f60513ec8d97414a346cedb755a4cac3abd55ed1 Mon Sep 17 00:00:00 2001 From: marmeladema Date: Sat, 20 Jun 2020 19:59:29 +0100 Subject: [PATCH 09/24] Move remaining `NodeId` APIs from `Definitions` to `Resolver` --- Cargo.lock | 1 + src/librustc_ast_lowering/item.rs | 17 +-- src/librustc_ast_lowering/lib.rs | 50 ++++++--- src/librustc_hir/definitions.rs | 105 ++---------------- src/librustc_metadata/creader.rs | 4 +- src/librustc_resolve/Cargo.toml | 1 + src/librustc_resolve/build_reduced_graph.rs | 67 +++++------ src/librustc_resolve/check_unused.rs | 5 +- src/librustc_resolve/def_collector.rs | 27 +++-- src/librustc_resolve/diagnostics.rs | 8 +- src/librustc_resolve/imports.rs | 3 +- src/librustc_resolve/late.rs | 17 +-- src/librustc_resolve/late/diagnostics.rs | 6 +- src/librustc_resolve/lib.rs | 117 ++++++++++++++++---- src/librustc_resolve/macros.rs | 7 +- 15 files changed, 219 insertions(+), 216 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 5bb6cda64cba..d5bc99e12651 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4242,6 +4242,7 @@ dependencies = [ "rustc_expand", "rustc_feature", "rustc_hir", + "rustc_index", "rustc_metadata", "rustc_middle", "rustc_session", diff --git a/src/librustc_ast_lowering/item.rs b/src/librustc_ast_lowering/item.rs index 8cfbd408e22b..00665c4cafb6 100644 --- a/src/librustc_ast_lowering/item.rs +++ b/src/librustc_ast_lowering/item.rs @@ -253,7 +253,7 @@ fn lower_item_kind( hir::ItemKind::Const(ty, body_id) } ItemKind::Fn(_, FnSig { ref decl, header }, ref generics, ref body) => { - let fn_def_id = self.resolver.definitions().local_def_id(id); + let fn_def_id = self.resolver.local_def_id(id); self.with_new_scopes(|this| { this.current_item = Some(ident.span); @@ -342,7 +342,7 @@ fn lower_item_kind( self_ty: ref ty, items: ref impl_items, } => { - let def_id = self.resolver.definitions().local_def_id(id); + let def_id = self.resolver.local_def_id(id); // Lower the "impl header" first. This ordering is important // for in-band lifetimes! Consider `'a` here: @@ -646,7 +646,7 @@ fn rebuild_vis(&mut self, vis: &hir::Visibility<'hir>) -> hir::Visibility<'hir> } fn lower_foreign_item(&mut self, i: &ForeignItem) -> hir::ForeignItem<'hir> { - let def_id = self.resolver.definitions().local_def_id(i.id); + let def_id = self.resolver.local_def_id(i.id); hir::ForeignItem { hir_id: self.lower_node_id(i.id), ident: i.ident, @@ -747,7 +747,7 @@ fn lower_struct_field(&mut self, (index, f): (usize, &StructField)) -> hir::Stru } fn lower_trait_item(&mut self, i: &AssocItem) -> hir::TraitItem<'hir> { - let trait_item_def_id = self.resolver.definitions().local_def_id(i.id); + let trait_item_def_id = self.resolver.local_def_id(i.id); let (generics, kind) = match i.kind { AssocItemKind::Const(_, ref ty, ref default) => { @@ -812,7 +812,7 @@ fn lower_trait_item_ref(&mut self, i: &AssocItem) -> hir::TraitItemRef { } fn lower_impl_item(&mut self, i: &AssocItem) -> hir::ImplItem<'hir> { - let impl_item_def_id = self.resolver.definitions().local_def_id(i.id); + let impl_item_def_id = self.resolver.local_def_id(i.id); let (generics, kind) = match &i.kind { AssocItemKind::Const(_, ty, expr) => { @@ -1320,12 +1320,7 @@ pub(super) fn lower_generics_mut( if let Some(def_id) = def_id.as_local() { for param in &generics.params { if let GenericParamKind::Type { .. } = param.kind { - if def_id - == self - .resolver - .definitions() - .local_def_id(param.id) - { + if def_id == self.resolver.local_def_id(param.id) { add_bounds .entry(param.id) .or_default() diff --git a/src/librustc_ast_lowering/lib.rs b/src/librustc_ast_lowering/lib.rs index 335cc3e61040..ac8a229da43d 100644 --- a/src/librustc_ast_lowering/lib.rs +++ b/src/librustc_ast_lowering/lib.rs @@ -54,7 +54,7 @@ use rustc_hir::definitions::{DefKey, DefPathData, Definitions}; use rustc_hir::intravisit; use rustc_hir::{ConstArg, GenericArg, ParamName}; -use rustc_index::vec::IndexVec; +use rustc_index::vec::{Idx, IndexVec}; use rustc_session::config::nightly_options; use rustc_session::lint::{builtin::BARE_TRAIT_OBJECTS, BuiltinLintDiagnostics, LintBuffer}; use rustc_session::parse::ParseSess; @@ -205,6 +205,19 @@ fn resolve_str_path( fn next_node_id(&mut self) -> NodeId; fn trait_map(&self) -> &NodeMap>; + + fn opt_local_def_id(&self, node: NodeId) -> Option; + + fn local_def_id(&self, node: NodeId) -> LocalDefId; + + fn create_def_with_parent( + &mut self, + parent: LocalDefId, + node_id: ast::NodeId, + data: DefPathData, + expn_id: ExpnId, + span: Span, + ) -> LocalDefId; } type NtToTokenstream = fn(&Nonterminal, &ParseSess, Span) -> TokenStream; @@ -436,7 +449,7 @@ fn allocate_use_tree_hir_id_counters(&mut self, tree: &UseTree, owner: LocalDefI match tree.kind { UseTreeKind::Simple(_, id1, id2) => { for &id in &[id1, id2] { - self.lctx.resolver.definitions().create_def_with_parent( + self.lctx.resolver.create_def_with_parent( owner, id, DefPathData::Misc, @@ -488,7 +501,7 @@ fn visit_item(&mut self, item: &'tcx Item) { | ItemKind::Enum(_, ref generics) | ItemKind::TyAlias(_, ref generics, ..) | ItemKind::Trait(_, _, ref generics, ..) => { - let def_id = self.lctx.resolver.definitions().local_def_id(item.id); + let def_id = self.lctx.resolver.local_def_id(item.id); let count = generics .params .iter() @@ -564,7 +577,18 @@ fn visit_ty(&mut self, t: &'tcx Ty) { .map(|(&k, v)| (self.node_id_to_hir_id[k].unwrap(), v.clone())) .collect(); - self.resolver.definitions().init_node_id_to_hir_id_mapping(self.node_id_to_hir_id); + let mut def_id_to_hir_id = IndexVec::default(); + + for (node_id, hir_id) in self.node_id_to_hir_id.into_iter_enumerated() { + if let Some(def_id) = self.resolver.opt_local_def_id(node_id) { + if def_id_to_hir_id.len() <= def_id.index() { + def_id_to_hir_id.resize(def_id.index() + 1, None); + } + def_id_to_hir_id[def_id] = hir_id; + } + } + + self.resolver.definitions().init_def_id_to_hir_id_mapping(def_id_to_hir_id); hir::Crate { item: hir::CrateItem { module, attrs, span: c.span }, @@ -628,7 +652,7 @@ fn with_hir_id_owner(&mut self, owner: NodeId, f: impl FnOnce(&mut Self) -> T .item_local_id_counters .insert(owner, HIR_ID_COUNTER_LOCKED) .unwrap_or_else(|| panic!("no `item_local_id_counters` entry for {:?}", owner)); - let def_id = self.resolver.definitions().local_def_id(owner); + let def_id = self.resolver.local_def_id(owner); self.current_hir_id_owner.push((def_id, counter)); let ret = f(self); let (new_def_id, new_counter) = self.current_hir_id_owner.pop().unwrap(); @@ -671,7 +695,7 @@ fn lower_node_id_with_owner(&mut self, ast_node_id: NodeId, owner: NodeId) -> hi debug_assert!(local_id != HIR_ID_COUNTER_LOCKED); *local_id_counter += 1; - let owner = this.resolver.definitions().opt_local_def_id(owner).expect( + let owner = this.resolver.opt_local_def_id(owner).expect( "you forgot to call `create_def_with_parent` or are lowering node-IDs \ that do not belong to the current owner", ); @@ -800,7 +824,7 @@ fn lifetime_to_generic_param( }; // Add a definition for the in-band lifetime def. - self.resolver.definitions().create_def_with_parent( + self.resolver.create_def_with_parent( parent_def_id, node_id, DefPathData::LifetimeNs(str_name), @@ -1088,7 +1112,7 @@ fn lower_assoc_ty_constraint( let impl_trait_node_id = self.resolver.next_node_id(); let parent_def_id = self.current_hir_id_owner.last().unwrap().0; - self.resolver.definitions().create_def_with_parent( + self.resolver.create_def_with_parent( parent_def_id, impl_trait_node_id, DefPathData::ImplTrait, @@ -1154,7 +1178,7 @@ fn lower_generic_arg( let node_id = self.resolver.next_node_id(); // Add a definition for the in-band const def. - self.resolver.definitions().create_def_with_parent( + self.resolver.create_def_with_parent( parent_def_id, node_id, DefPathData::AnonConst, @@ -1339,7 +1363,7 @@ fn lower_ty_direct(&mut self, t: &Ty, mut itctx: ImplTraitContext<'_, 'hir>) -> } ImplTraitContext::Universal(in_band_ty_params) => { // Add a definition for the in-band `Param`. - let def_id = self.resolver.definitions().local_def_id(def_node_id); + let def_id = self.resolver.local_def_id(def_node_id); let hir_bounds = self.lower_param_bounds( bounds, @@ -1428,7 +1452,7 @@ fn lower_opaque_impl_trait( // frequently opened issues show. let opaque_ty_span = self.mark_span_with_reason(DesugaringKind::OpaqueTy, span, None); - let opaque_ty_def_id = self.resolver.definitions().local_def_id(opaque_ty_node_id); + let opaque_ty_def_id = self.resolver.local_def_id(opaque_ty_node_id); self.allocate_hir_id_counter(opaque_ty_node_id); @@ -1620,7 +1644,7 @@ fn visit_lifetime(&mut self, lifetime: &'v hir::Lifetime) { let def_node_id = self.context.resolver.next_node_id(); let hir_id = self.context.lower_node_id_with_owner(def_node_id, self.opaque_ty_id); - self.context.resolver.definitions().create_def_with_parent( + self.context.resolver.create_def_with_parent( self.parent, def_node_id, DefPathData::LifetimeNs(name.ident().name), @@ -1870,7 +1894,7 @@ fn lower_async_fn_ret_ty( let opaque_ty_span = self.mark_span_with_reason(DesugaringKind::Async, span, None); - let opaque_ty_def_id = self.resolver.definitions().local_def_id(opaque_ty_node_id); + let opaque_ty_def_id = self.resolver.local_def_id(opaque_ty_node_id); self.allocate_hir_id_counter(opaque_ty_node_id); diff --git a/src/librustc_hir/definitions.rs b/src/librustc_hir/definitions.rs index 5755a3db92ac..c34607f8a94c 100644 --- a/src/librustc_hir/definitions.rs +++ b/src/librustc_hir/definitions.rs @@ -8,14 +8,12 @@ use crate::def_id::{CrateNum, DefId, DefIndex, LocalDefId, CRATE_DEF_INDEX, LOCAL_CRATE}; use crate::hir; -use rustc_ast::ast; use rustc_ast::crate_disambiguator::CrateDisambiguator; use rustc_data_structures::fx::FxHashMap; use rustc_data_structures::stable_hasher::StableHasher; use rustc_index::vec::IndexVec; use rustc_span::hygiene::ExpnId; use rustc_span::symbol::{sym, Symbol}; -use rustc_span::Span; use log::debug; use std::fmt::Write; @@ -79,11 +77,6 @@ pub fn size(&self) -> usize { pub struct Definitions { table: DefPathTable, - def_id_to_span: IndexVec, - - node_id_to_def_id: FxHashMap, - def_id_to_node_id: IndexVec, - // FIXME(eddyb) ideally all `LocalDefId`s would be HIR owners. pub(super) def_id_to_hir_id: IndexVec>, /// The reverse mapping of `def_id_to_hir_id`. @@ -95,11 +88,6 @@ pub struct Definitions { /// Item with a given `LocalDefId` was defined during macro expansion with ID `ExpnId`. expansions_that_defined: FxHashMap, next_disambiguator: FxHashMap<(LocalDefId, DefPathData), u32>, - /// When collecting definitions from an AST fragment produced by a macro invocation `ExpnId` - /// we know what parent node that fragment should be attached to thanks to this table. - invocation_parents: FxHashMap, - /// Indices of unnamed struct or variant fields with unresolved attributes. - placeholder_field_indices: FxHashMap, } /// A unique identifier that we can use to lookup a definition @@ -319,16 +307,6 @@ pub fn def_path(&self, id: LocalDefId) -> DefPath { }) } - #[inline] - pub fn opt_local_def_id(&self, node: ast::NodeId) -> Option { - self.node_id_to_def_id.get(&node).copied() - } - - #[inline] - pub fn local_def_id(&self, node: ast::NodeId) -> LocalDefId { - self.opt_local_def_id(node).unwrap_or_else(|| panic!("no entry for node id: `{:?}`", node)) - } - #[inline] pub fn as_local_hir_id(&self, def_id: LocalDefId) -> hir::HirId { self.local_def_id_to_hir_id(def_id) @@ -349,12 +327,6 @@ pub fn opt_hir_id_to_local_def_id(&self, hir_id: hir::HirId) -> Option Option { - if let Some(def_id) = def_id.as_local() { Some(self.def_id_to_span[def_id]) } else { None } - } - /// Adds a root definition (no parent) and a few other reserved definitions. pub fn create_root_def( &mut self, @@ -376,12 +348,6 @@ pub fn create_root_def( let root = LocalDefId { local_def_index: self.table.allocate(key, def_path_hash) }; assert_eq!(root.local_def_index, CRATE_DEF_INDEX); - assert_eq!(self.def_id_to_node_id.push(ast::CRATE_NODE_ID), root); - assert_eq!(self.def_id_to_span.push(rustc_span::DUMMY_SP), root); - - self.node_id_to_def_id.insert(ast::CRATE_NODE_ID, root); - self.set_invocation_parent(ExpnId::root(), root); - root } @@ -389,22 +355,12 @@ pub fn create_root_def( pub fn create_def_with_parent( &mut self, parent: LocalDefId, - node_id: ast::NodeId, data: DefPathData, expn_id: ExpnId, - span: Span, ) -> LocalDefId { debug!( - "create_def_with_parent(parent={:?}, node_id={:?}, data={:?})", - parent, node_id, data - ); - - assert!( - !self.node_id_to_def_id.contains_key(&node_id), - "adding a def'n for node-id {:?} and data {:?} but a previous def'n exists: {:?}", - node_id, - data, - self.table.def_key(self.node_id_to_def_id[&node_id].local_def_index), + "create_def_with_parent(parent={:?}, data={:?}, expn_id={:?})", + parent, data, expn_id ); // The root node must be created with `create_root_def()`. @@ -431,17 +387,6 @@ pub fn create_def_with_parent( // Create the definition. let def_id = LocalDefId { local_def_index: self.table.allocate(key, def_path_hash) }; - assert_eq!(self.def_id_to_node_id.push(node_id), def_id); - assert_eq!(self.def_id_to_span.push(span), def_id); - - // Some things for which we allocate `LocalDefId`s don't correspond to - // anything in the AST, so they don't have a `NodeId`. For these cases - // we don't need a mapping from `NodeId` to `LocalDefId`. - if node_id != ast::DUMMY_NODE_ID { - debug!("create_def_with_parent: def_id_to_node_id[{:?}] <-> {:?}", def_id, node_id); - self.node_id_to_def_id.insert(node_id, def_id); - } - if expn_id != ExpnId::root() { self.expansions_that_defined.insert(def_id, expn_id); } @@ -449,32 +394,24 @@ pub fn create_def_with_parent( def_id } - /// Initializes the `ast::NodeId` to `HirId` mapping once it has been generated during + /// Initializes the `LocalDefId` to `HirId` mapping once it has been generated during /// AST to HIR lowering. - pub fn init_node_id_to_hir_id_mapping( + pub fn init_def_id_to_hir_id_mapping( &mut self, - mapping: IndexVec>, + mapping: IndexVec>, ) { assert!( self.def_id_to_hir_id.is_empty(), "trying to initialize `LocalDefId` <-> `HirId` mappings twice" ); - self.def_id_to_hir_id = self - .def_id_to_node_id - .iter() - .map(|&node_id| mapping.get(node_id).and_then(|&hir_id| hir_id)) - .collect(); - // Build the reverse mapping of `def_id_to_hir_id`. self.hir_id_to_def_id = mapping - .into_iter_enumerated() - .filter_map(|(node_id, hir_id)| { - hir_id.and_then(|hir_id| { - self.node_id_to_def_id.get(&node_id).map(|&def_id| (hir_id, def_id)) - }) - }) + .iter_enumerated() + .filter_map(|(def_id, hir_id)| hir_id.map(|hir_id| (hir_id, def_id))) .collect(); + + self.def_id_to_hir_id = mapping; } pub fn expansion_that_defined(&self, id: LocalDefId) -> ExpnId { @@ -488,30 +425,6 @@ pub fn parent_module_of_macro_def(&self, expn_id: ExpnId) -> DefId { pub fn add_parent_module_of_macro_def(&mut self, expn_id: ExpnId, module: DefId) { self.parent_modules_of_macro_defs.insert(expn_id, module); } - - pub fn invocation_parent(&self, invoc_id: ExpnId) -> LocalDefId { - self.invocation_parents[&invoc_id] - } - - pub fn set_invocation_parent(&mut self, invoc_id: ExpnId, parent: LocalDefId) { - let old_parent = self.invocation_parents.insert(invoc_id, parent); - assert!(old_parent.is_none(), "parent `LocalDefId` is reset for an invocation"); - } - - pub fn placeholder_field_index(&self, node_id: ast::NodeId) -> usize { - self.placeholder_field_indices[&node_id] - } - - pub fn set_placeholder_field_index(&mut self, node_id: ast::NodeId, index: usize) { - let old_index = self.placeholder_field_indices.insert(node_id, index); - assert!(old_index.is_none(), "placeholder field index is reset for a node ID"); - } - - pub fn lint_node_id(&mut self, expn_id: ExpnId) -> ast::NodeId { - self.invocation_parents - .get(&expn_id) - .map_or(ast::CRATE_NODE_ID, |id| self.def_id_to_node_id[*id]) - } } impl DefPathData { diff --git a/src/librustc_metadata/creader.rs b/src/librustc_metadata/creader.rs index 0dc007bbfd72..2c80c846681a 100644 --- a/src/librustc_metadata/creader.rs +++ b/src/librustc_metadata/creader.rs @@ -10,7 +10,7 @@ use rustc_data_structures::sync::Lrc; use rustc_errors::struct_span_err; use rustc_expand::base::SyntaxExtension; -use rustc_hir::def_id::{CrateNum, LOCAL_CRATE}; +use rustc_hir::def_id::{CrateNum, LocalDefId, LOCAL_CRATE}; use rustc_hir::definitions::Definitions; use rustc_index::vec::IndexVec; use rustc_middle::middle::cstore::DepKind; @@ -896,6 +896,7 @@ pub fn process_extern_crate( &mut self, item: &ast::Item, definitions: &Definitions, + def_id: LocalDefId, ) -> CrateNum { match item.kind { ast::ItemKind::ExternCrate(orig_name) => { @@ -918,7 +919,6 @@ pub fn process_extern_crate( let cnum = self.resolve_crate(name, item.span, dep_kind, None); - let def_id = definitions.opt_local_def_id(item.id).unwrap(); let path_len = definitions.def_path(def_id).data.len(); self.update_extern_crate( cnum, diff --git a/src/librustc_resolve/Cargo.toml b/src/librustc_resolve/Cargo.toml index fa5c557b5d9c..6f6104c3d693 100644 --- a/src/librustc_resolve/Cargo.toml +++ b/src/librustc_resolve/Cargo.toml @@ -24,6 +24,7 @@ rustc_errors = { path = "../librustc_errors" } rustc_expand = { path = "../librustc_expand" } rustc_feature = { path = "../librustc_feature" } rustc_hir = { path = "../librustc_hir" } +rustc_index = { path = "../librustc_index" } rustc_metadata = { path = "../librustc_metadata" } rustc_session = { path = "../librustc_session" } rustc_span = { path = "../librustc_span" } diff --git a/src/librustc_resolve/build_reduced_graph.rs b/src/librustc_resolve/build_reduced_graph.rs index 8432e34a5271..ef43f597eab4 100644 --- a/src/librustc_resolve/build_reduced_graph.rs +++ b/src/librustc_resolve/build_reduced_graph.rs @@ -19,6 +19,7 @@ use rustc_ast::ast::{AssocItem, AssocItemKind, MetaItemKind, StmtKind}; use rustc_ast::token::{self, Token}; use rustc_ast::visit::{self, AssocCtxt, Visitor}; +use rustc_ast_lowering::Resolver as ResolverAstLowering; use rustc_attr as attr; use rustc_data_structures::sync::Lrc; use rustc_errors::{struct_span_err, Applicability}; @@ -171,7 +172,7 @@ impl<'a> Resolver<'a> { fragment: &AstFragment, parent_scope: ParentScope<'a>, ) -> MacroRulesScope<'a> { - collect_definitions(&mut self.definitions, fragment, parent_scope.expansion); + collect_definitions(self, fragment, parent_scope.expansion); let mut visitor = BuildReducedGraphVisitor { r: self, parent_scope }; fragment.visit_with(&mut visitor); visitor.parent_scope.macro_rules @@ -647,9 +648,9 @@ fn build_reduced_graph_for_item(&mut self, item: &'b Item) { } else if orig_name == Some(kw::SelfLower) { self.r.graph_root } else { - let def_id = self.r.definitions.local_def_id(item.id); + let def_id = self.r.local_def_id(item.id); let crate_id = - self.r.crate_loader.process_extern_crate(item, &self.r.definitions); + self.r.crate_loader.process_extern_crate(item, &self.r.definitions, def_id); self.r.extern_crate_map.insert(def_id, crate_id); self.r.get_module(DefId { krate: crate_id, index: CRATE_DEF_INDEX }) }; @@ -704,7 +705,7 @@ fn build_reduced_graph_for_item(&mut self, item: &'b Item) { ItemKind::Mod(..) if ident.name == kw::Invalid => {} // Crate root ItemKind::Mod(..) => { - let def_id = self.r.definitions.local_def_id(item.id); + let def_id = self.r.local_def_id(item.id); let module_kind = ModuleKind::Def(DefKind::Mod, def_id.to_def_id(), ident.name); let module = self.r.arenas.alloc_module(ModuleData { no_implicit_prelude: parent.no_implicit_prelude || { @@ -727,18 +728,15 @@ fn build_reduced_graph_for_item(&mut self, item: &'b Item) { // These items live in the value namespace. ItemKind::Static(..) => { - let res = - Res::Def(DefKind::Static, self.r.definitions.local_def_id(item.id).to_def_id()); + let res = Res::Def(DefKind::Static, self.r.local_def_id(item.id).to_def_id()); self.r.define(parent, ident, ValueNS, (res, vis, sp, expansion)); } ItemKind::Const(..) => { - let res = - Res::Def(DefKind::Const, self.r.definitions.local_def_id(item.id).to_def_id()); + let res = Res::Def(DefKind::Const, self.r.local_def_id(item.id).to_def_id()); self.r.define(parent, ident, ValueNS, (res, vis, sp, expansion)); } ItemKind::Fn(..) => { - let res = - Res::Def(DefKind::Fn, self.r.definitions.local_def_id(item.id).to_def_id()); + let res = Res::Def(DefKind::Fn, self.r.local_def_id(item.id).to_def_id()); self.r.define(parent, ident, ValueNS, (res, vis, sp, expansion)); // Functions introducing procedural macros reserve a slot @@ -748,15 +746,12 @@ fn build_reduced_graph_for_item(&mut self, item: &'b Item) { // These items live in the type namespace. ItemKind::TyAlias(..) => { - let res = Res::Def( - DefKind::TyAlias, - self.r.definitions.local_def_id(item.id).to_def_id(), - ); + let res = Res::Def(DefKind::TyAlias, self.r.local_def_id(item.id).to_def_id()); self.r.define(parent, ident, TypeNS, (res, vis, sp, expansion)); } ItemKind::Enum(_, _) => { - let def_id = self.r.definitions.local_def_id(item.id).to_def_id(); + let def_id = self.r.local_def_id(item.id).to_def_id(); self.r.variant_vis.insert(def_id, vis); let module_kind = ModuleKind::Def(DefKind::Enum, def_id, ident.name); let module = self.r.new_module( @@ -771,17 +766,14 @@ fn build_reduced_graph_for_item(&mut self, item: &'b Item) { } ItemKind::TraitAlias(..) => { - let res = Res::Def( - DefKind::TraitAlias, - self.r.definitions.local_def_id(item.id).to_def_id(), - ); + let res = Res::Def(DefKind::TraitAlias, self.r.local_def_id(item.id).to_def_id()); self.r.define(parent, ident, TypeNS, (res, vis, sp, expansion)); } // These items live in both the type and value namespaces. ItemKind::Struct(ref vdata, _) => { // Define a name in the type namespace. - let def_id = self.r.definitions.local_def_id(item.id).to_def_id(); + let def_id = self.r.local_def_id(item.id).to_def_id(); let res = Res::Def(DefKind::Struct, def_id); self.r.define(parent, ident, TypeNS, (res, vis, sp, expansion)); @@ -814,7 +806,7 @@ fn build_reduced_graph_for_item(&mut self, item: &'b Item) { } let ctor_res = Res::Def( DefKind::Ctor(CtorOf::Struct, CtorKind::from_ast(vdata)), - self.r.definitions.local_def_id(ctor_node_id).to_def_id(), + self.r.local_def_id(ctor_node_id).to_def_id(), ); self.r.define(parent, ident, ValueNS, (ctor_res, ctor_vis, sp, expansion)); self.r.struct_constructors.insert(def_id, (ctor_res, ctor_vis)); @@ -822,7 +814,7 @@ fn build_reduced_graph_for_item(&mut self, item: &'b Item) { } ItemKind::Union(ref vdata, _) => { - let def_id = self.r.definitions.local_def_id(item.id).to_def_id(); + let def_id = self.r.local_def_id(item.id).to_def_id(); let res = Res::Def(DefKind::Union, def_id); self.r.define(parent, ident, TypeNS, (res, vis, sp, expansion)); @@ -831,7 +823,7 @@ fn build_reduced_graph_for_item(&mut self, item: &'b Item) { } ItemKind::Trait(..) => { - let def_id = self.r.definitions.local_def_id(item.id).to_def_id(); + let def_id = self.r.local_def_id(item.id).to_def_id(); // Add all the items within to a new module. let module_kind = ModuleKind::Def(DefKind::Trait, def_id, ident.name); @@ -856,18 +848,15 @@ fn build_reduced_graph_for_item(&mut self, item: &'b Item) { /// Constructs the reduced graph for one foreign item. fn build_reduced_graph_for_foreign_item(&mut self, item: &ForeignItem) { let (res, ns) = match item.kind { - ForeignItemKind::Fn(..) => ( - Res::Def(DefKind::Fn, self.r.definitions.local_def_id(item.id).to_def_id()), - ValueNS, - ), - ForeignItemKind::Static(..) => ( - Res::Def(DefKind::Static, self.r.definitions.local_def_id(item.id).to_def_id()), - ValueNS, - ), - ForeignItemKind::TyAlias(..) => ( - Res::Def(DefKind::ForeignTy, self.r.definitions.local_def_id(item.id).to_def_id()), - TypeNS, - ), + ForeignItemKind::Fn(..) => { + (Res::Def(DefKind::Fn, self.r.local_def_id(item.id).to_def_id()), ValueNS) + } + ForeignItemKind::Static(..) => { + (Res::Def(DefKind::Static, self.r.local_def_id(item.id).to_def_id()), ValueNS) + } + ForeignItemKind::TyAlias(..) => { + (Res::Def(DefKind::ForeignTy, self.r.local_def_id(item.id).to_def_id()), TypeNS) + } ForeignItemKind::MacCall(_) => unreachable!(), }; let parent = self.parent_scope.module; @@ -1170,7 +1159,7 @@ fn insert_unused_macro( fn define_macro(&mut self, item: &ast::Item) -> MacroRulesScope<'a> { let parent_scope = self.parent_scope; let expansion = parent_scope.expansion; - let def_id = self.r.definitions.local_def_id(item.id); + let def_id = self.r.local_def_id(item.id); let (ext, ident, span, macro_rules) = match &item.kind { ItemKind::MacroDef(def) => { let ext = Lrc::new(self.r.compile_macro(item, self.r.session.edition())); @@ -1315,7 +1304,7 @@ fn visit_assoc_item(&mut self, item: &'b AssocItem, ctxt: AssocCtxt) { } // Add the item to the trait info. - let item_def_id = self.r.definitions.local_def_id(item.id).to_def_id(); + let item_def_id = self.r.local_def_id(item.id).to_def_id(); let (res, ns) = match item.kind { AssocItemKind::Const(..) => (Res::Def(DefKind::AssocConst, item_def_id), ValueNS), AssocItemKind::Fn(_, ref sig, _, _) => { @@ -1417,7 +1406,7 @@ fn visit_variant(&mut self, variant: &'b ast::Variant) { let ident = variant.ident; // Define a name in the type namespace. - let def_id = self.r.definitions.local_def_id(variant.id).to_def_id(); + let def_id = self.r.local_def_id(variant.id).to_def_id(); let res = Res::Def(DefKind::Variant, def_id); self.r.define(parent, ident, TypeNS, (res, vis, variant.span, expn_id)); @@ -1435,7 +1424,7 @@ fn visit_variant(&mut self, variant: &'b ast::Variant) { // It's ok to use the variant's id as a ctor id since an // error will be reported on any use of such resolution anyway. let ctor_node_id = variant.data.ctor_id().unwrap_or(variant.id); - let ctor_def_id = self.r.definitions.local_def_id(ctor_node_id).to_def_id(); + let ctor_def_id = self.r.local_def_id(ctor_node_id).to_def_id(); let ctor_kind = CtorKind::from_ast(&variant.data); let ctor_res = Res::Def(DefKind::Ctor(CtorOf::Variant, ctor_kind), ctor_def_id); self.r.define(parent, ident, ValueNS, (ctor_res, ctor_vis, variant.span, expn_id)); diff --git a/src/librustc_resolve/check_unused.rs b/src/librustc_resolve/check_unused.rs index cc0e97aeb143..0ca01a384e73 100644 --- a/src/librustc_resolve/check_unused.rs +++ b/src/librustc_resolve/check_unused.rs @@ -29,6 +29,7 @@ use rustc_ast::ast; use rustc_ast::node_id::NodeMap; use rustc_ast::visit::{self, Visitor}; +use rustc_ast_lowering::Resolver as ResolverAstLowering; use rustc_data_structures::fx::FxHashSet; use rustc_errors::pluralize; use rustc_middle::ty; @@ -64,7 +65,7 @@ impl<'a, 'b> UnusedImportCheckVisitor<'a, 'b> { fn check_import(&mut self, id: ast::NodeId) { let mut used = false; self.r.per_ns(|this, ns| used |= this.used_imports.contains(&(id, ns))); - let def_id = self.r.definitions.local_def_id(id); + let def_id = self.r.local_def_id(id); if !used { if self.r.maybe_unused_trait_imports.contains(&def_id) { // Check later. @@ -246,7 +247,7 @@ impl Resolver<'_> { } } ImportKind::ExternCrate { .. } => { - let def_id = self.definitions.local_def_id(import.id); + let def_id = self.local_def_id(import.id); self.maybe_unused_extern_crates.push((def_id, import.span)); } ImportKind::MacroUse => { diff --git a/src/librustc_resolve/def_collector.rs b/src/librustc_resolve/def_collector.rs index 71cedb208fcf..7bf039ea8a53 100644 --- a/src/librustc_resolve/def_collector.rs +++ b/src/librustc_resolve/def_collector.rs @@ -1,8 +1,10 @@ +use crate::Resolver; use log::debug; use rustc_ast::ast::*; use rustc_ast::token::{self, Token}; use rustc_ast::visit::{self, FnKind}; use rustc_ast::walk_list; +use rustc_ast_lowering::Resolver as ResolverAstLowering; use rustc_expand::expand::AstFragment; use rustc_hir::def_id::LocalDefId; use rustc_hir::definitions::*; @@ -11,26 +13,26 @@ use rustc_span::Span; crate fn collect_definitions( - definitions: &mut Definitions, + resolver: &mut Resolver<'_>, fragment: &AstFragment, expansion: ExpnId, ) { - let parent_def = definitions.invocation_parent(expansion); - fragment.visit_with(&mut DefCollector { definitions, parent_def, expansion }); + let parent_def = resolver.invocation_parents[&expansion]; + fragment.visit_with(&mut DefCollector { resolver, parent_def, expansion }); } /// Creates `DefId`s for nodes in the AST. -struct DefCollector<'a> { - definitions: &'a mut Definitions, +struct DefCollector<'a, 'b> { + resolver: &'a mut Resolver<'b>, parent_def: LocalDefId, expansion: ExpnId, } -impl<'a> DefCollector<'a> { +impl<'a, 'b> DefCollector<'a, 'b> { fn create_def(&mut self, node_id: NodeId, data: DefPathData, span: Span) -> LocalDefId { let parent_def = self.parent_def; debug!("create_def(node_id={:?}, data={:?}, parent_def={:?})", node_id, data, parent_def); - self.definitions.create_def_with_parent(parent_def, node_id, data, self.expansion, span) + self.resolver.create_def_with_parent(parent_def, node_id, data, self.expansion, span) } fn with_parent(&mut self, parent_def: LocalDefId, f: F) { @@ -43,12 +45,13 @@ fn collect_field(&mut self, field: &'a StructField, index: Option) { let index = |this: &Self| { index.unwrap_or_else(|| { let node_id = NodeId::placeholder_from_expn_id(this.expansion); - this.definitions.placeholder_field_index(node_id) + this.resolver.placeholder_field_indices[&node_id] }) }; if field.is_placeholder { - self.definitions.set_placeholder_field_index(field.id, index(self)); + let old_index = self.resolver.placeholder_field_indices.insert(field.id, index(self)); + assert!(old_index.is_none(), "placeholder field index is reset for a node ID"); self.visit_macro_invoc(field.id); } else { let name = field.ident.map_or_else(|| sym::integer(index(self)), |ident| ident.name); @@ -58,11 +61,13 @@ fn collect_field(&mut self, field: &'a StructField, index: Option) { } fn visit_macro_invoc(&mut self, id: NodeId) { - self.definitions.set_invocation_parent(id.placeholder_to_expn_id(), self.parent_def); + let old_parent = + self.resolver.invocation_parents.insert(id.placeholder_to_expn_id(), self.parent_def); + assert!(old_parent.is_none(), "parent `LocalDefId` is reset for an invocation"); } } -impl<'a> visit::Visitor<'a> for DefCollector<'a> { +impl<'a, 'b> visit::Visitor<'a> for DefCollector<'a, 'b> { fn visit_item(&mut self, i: &'a Item) { debug!("visit_item: {:?}", i); diff --git a/src/librustc_resolve/diagnostics.rs b/src/librustc_resolve/diagnostics.rs index bd2ce5a72e8d..35d71a38bbe0 100644 --- a/src/librustc_resolve/diagnostics.rs +++ b/src/librustc_resolve/diagnostics.rs @@ -107,7 +107,7 @@ impl<'a> Resolver<'a> { match outer_res { Res::SelfTy(maybe_trait_defid, maybe_impl_defid) => { if let Some(impl_span) = - maybe_impl_defid.and_then(|def_id| self.definitions.opt_span(def_id)) + maybe_impl_defid.and_then(|def_id| self.opt_span(def_id)) { err.span_label( reduce_impl_span_to_impl_keyword(sm, impl_span), @@ -126,12 +126,12 @@ impl<'a> Resolver<'a> { return err; } Res::Def(DefKind::TyParam, def_id) => { - if let Some(span) = self.definitions.opt_span(def_id) { + if let Some(span) = self.opt_span(def_id) { err.span_label(span, "type parameter from outer function"); } } Res::Def(DefKind::ConstParam, def_id) => { - if let Some(span) = self.definitions.opt_span(def_id) { + if let Some(span) = self.opt_span(def_id) { err.span_label(span, "const parameter from outer function"); } } @@ -825,7 +825,7 @@ fn lookup_import_candidates_from_module( Applicability::MaybeIncorrect, ); let def_span = suggestion.res.opt_def_id().and_then(|def_id| match def_id.krate { - LOCAL_CRATE => self.definitions.opt_span(def_id), + LOCAL_CRATE => self.opt_span(def_id), _ => Some( self.session .source_map() diff --git a/src/librustc_resolve/imports.rs b/src/librustc_resolve/imports.rs index 74a8b7e2f556..8a6541b399e3 100644 --- a/src/librustc_resolve/imports.rs +++ b/src/librustc_resolve/imports.rs @@ -12,6 +12,7 @@ use rustc_ast::ast::NodeId; use rustc_ast::unwrap_or; use rustc_ast::util::lev_distance::find_best_match_for_name; +use rustc_ast_lowering::Resolver as ResolverAstLowering; use rustc_data_structures::fx::FxHashSet; use rustc_data_structures::ptr_key::PtrKey; use rustc_errors::{pluralize, struct_span_err, Applicability}; @@ -1393,7 +1394,7 @@ fn finalize_resolutions_in(&mut self, module: Module<'b>) { let is_good_import = binding.is_import() && !binding.is_ambiguity() && !ident.span.from_expansion(); if is_good_import || binding.is_macro_def() { - let res = binding.res().map_id(|id| this.definitions.local_def_id(id)); + let res = binding.res().map_id(|id| this.local_def_id(id)); if res != def::Res::Err { reexports.push(Export { ident, res, span: binding.span, vis: binding.vis }); } diff --git a/src/librustc_resolve/late.rs b/src/librustc_resolve/late.rs index 61f20df8cc6c..6f769c3c59ca 100644 --- a/src/librustc_resolve/late.rs +++ b/src/librustc_resolve/late.rs @@ -16,6 +16,7 @@ use rustc_ast::util::lev_distance::find_best_match_for_name; use rustc_ast::visit::{self, AssocCtxt, FnCtxt, FnKind, Visitor}; use rustc_ast::{unwrap_or, walk_list}; +use rustc_ast_lowering::Resolver as ResolverAstLowering; use rustc_data_structures::fx::{FxHashMap, FxHashSet}; use rustc_errors::DiagnosticId; use rustc_hir::def::Namespace::{self, *}; @@ -707,7 +708,7 @@ fn with_rib( } fn with_scope(&mut self, id: NodeId, f: impl FnOnce(&mut Self) -> T) -> T { - let id = self.r.definitions.local_def_id(id); + let id = self.r.local_def_id(id); let module = self.r.module_map.get(&id).cloned(); // clones a reference if let Some(module) = module { // Move down in the graph. @@ -759,7 +760,7 @@ fn resolve_adt(&mut self, item: &'ast Item, generics: &'ast Generics) { debug!("resolve_adt"); self.with_current_self_item(item, |this| { this.with_generic_param_rib(generics, ItemRibKind(HasGenericParams::Yes), |this| { - let item_def_id = this.r.definitions.local_def_id(item.id).to_def_id(); + let item_def_id = this.r.local_def_id(item.id).to_def_id(); this.with_self_rib(Res::SelfTy(None, Some(item_def_id)), |this| { visit::walk_item(this, item); }); @@ -839,7 +840,7 @@ fn resolve_item(&mut self, item: &'ast Item) { ItemKind::Trait(.., ref generics, ref bounds, ref trait_items) => { // Create a new rib for the trait-wide type parameters. self.with_generic_param_rib(generics, ItemRibKind(HasGenericParams::Yes), |this| { - let local_def_id = this.r.definitions.local_def_id(item.id).to_def_id(); + let local_def_id = this.r.local_def_id(item.id).to_def_id(); this.with_self_rib(Res::SelfTy(Some(local_def_id), None), |this| { this.visit_generics(generics); walk_list!(this, visit_param_bound, bounds); @@ -880,7 +881,7 @@ fn resolve_item(&mut self, item: &'ast Item) { ItemKind::TraitAlias(ref generics, ref bounds) => { // Create a new rib for the trait-wide type parameters. self.with_generic_param_rib(generics, ItemRibKind(HasGenericParams::Yes), |this| { - let local_def_id = this.r.definitions.local_def_id(item.id).to_def_id(); + let local_def_id = this.r.local_def_id(item.id).to_def_id(); this.with_self_rib(Res::SelfTy(Some(local_def_id), None), |this| { this.visit_generics(generics); walk_list!(this, visit_param_bound, bounds); @@ -961,7 +962,7 @@ fn with_generic_param_rib<'c, F>(&'c mut self, generics: &'c Generics, kind: Rib seen_bindings.entry(ident).or_insert(param.ident.span); // Plain insert (no renaming). - let res = Res::Def(def_kind, self.r.definitions.local_def_id(param.id).to_def_id()); + let res = Res::Def(def_kind, self.r.local_def_id(param.id).to_def_id()); match param.kind { GenericParamKind::Type { .. } => { @@ -1111,7 +1112,7 @@ fn resolve_implementation( this.with_self_rib(Res::SelfTy(None, None), |this| { // Resolve the trait reference, if necessary. this.with_optional_trait_ref(opt_trait_reference.as_ref(), |this, trait_id| { - let item_def_id = this.r.definitions.local_def_id(item_id).to_def_id(); + let item_def_id = this.r.local_def_id(item_id).to_def_id(); this.with_self_rib(Res::SelfTy(trait_id, Some(item_def_id)), |this| { if let Some(trait_ref) = opt_trait_reference.as_ref() { // Resolve type arguments in the trait path. @@ -2002,7 +2003,7 @@ fn resolve_block(&mut self, block: &'ast Block) { if let StmtKind::Item(ref item) = stmt.kind { if let ItemKind::MacroDef(..) = item.kind { num_macro_definition_ribs += 1; - let res = self.r.definitions.local_def_id(item.id).to_def_id(); + let res = self.r.local_def_id(item.id).to_def_id(); self.ribs[ValueNS].push(Rib::new(MacroDefinition(res))); self.label_ribs.push(Rib::new(MacroDefinition(res))); } @@ -2296,7 +2297,7 @@ fn find_transitive_imports( ) -> SmallVec<[LocalDefId; 1]> { let mut import_ids = smallvec![]; while let NameBindingKind::Import { import, binding, .. } = kind { - let id = self.r.definitions.local_def_id(import.id); + let id = self.r.local_def_id(import.id); self.r.maybe_unused_trait_imports.insert(id); self.r.add_to_glob_map(&import, trait_name); import_ids.push(id); diff --git a/src/librustc_resolve/late/diagnostics.rs b/src/librustc_resolve/late/diagnostics.rs index 05ef0aa0bb68..79c544ec6cc9 100644 --- a/src/librustc_resolve/late/diagnostics.rs +++ b/src/librustc_resolve/late/diagnostics.rs @@ -512,7 +512,7 @@ fn smart_resolve_context_dependent_help( _ => {} } if !suggested { - if let Some(span) = self.r.definitions.opt_span(def_id) { + if let Some(span) = self.r.opt_span(def_id) { err.span_label(span, &format!("`{}` defined here", path_str)); } err.span_label(span, format!("did you mean `{} {{ /* fields */ }}`?", path_str)); @@ -536,7 +536,7 @@ fn smart_resolve_context_dependent_help( if nightly_options::is_nightly_build() { let msg = "you might have meant to use `#![feature(trait_alias)]` instead of a \ `type` alias"; - if let Some(span) = self.r.definitions.opt_span(def_id) { + if let Some(span) = self.r.opt_span(def_id) { err.span_help(span, msg); } else { err.help(msg); @@ -593,7 +593,7 @@ fn smart_resolve_context_dependent_help( bad_struct_syntax_suggestion(def_id); } (Res::Def(DefKind::Ctor(_, CtorKind::Fn), def_id), _) if ns == ValueNS => { - if let Some(span) = self.r.definitions.opt_span(def_id) { + if let Some(span) = self.r.opt_span(def_id) { err.span_label(span, &format!("`{}` defined here", path_str)); } err.span_label(span, format!("did you mean `{}( /* fields */ )`?", path_str)); diff --git a/src/librustc_resolve/lib.rs b/src/librustc_resolve/lib.rs index 91bd15561417..0aca5c4905df 100644 --- a/src/librustc_resolve/lib.rs +++ b/src/librustc_resolve/lib.rs @@ -27,6 +27,7 @@ use rustc_ast::node_id::NodeMap; use rustc_ast::unwrap_or; use rustc_ast::visit::{self, Visitor}; +use rustc_ast_lowering::Resolver as ResolverAstLowering; use rustc_ast_pretty::pprust; use rustc_data_structures::fx::{FxHashMap, FxHashSet, FxIndexMap}; use rustc_data_structures::ptr_key::PtrKey; @@ -36,9 +37,10 @@ use rustc_hir::def::Namespace::*; use rustc_hir::def::{self, CtorOf, DefKind, NonMacroAttrKind, PartialRes}; use rustc_hir::def_id::{CrateNum, DefId, DefIdMap, LocalDefId, CRATE_DEF_INDEX}; -use rustc_hir::definitions::{DefKey, Definitions}; +use rustc_hir::definitions::{DefKey, DefPathData, Definitions}; use rustc_hir::PrimTy::{self, Bool, Char, Float, Int, Str, Uint}; use rustc_hir::TraitCandidate; +use rustc_index::vec::IndexVec; use rustc_metadata::creader::{CStore, CrateLoader}; use rustc_middle::hir::exports::ExportMap; use rustc_middle::middle::cstore::{CrateStore, MetadataLoaderDyn}; @@ -256,31 +258,21 @@ fn from(seg: &'a ast::PathSegment) -> Segment { } } -struct UsePlacementFinder<'d> { - definitions: &'d Definitions, - target_module: LocalDefId, +struct UsePlacementFinder { + target_module: NodeId, span: Option, found_use: bool, } -impl<'d> UsePlacementFinder<'d> { - fn check( - definitions: &'d Definitions, - krate: &Crate, - target_module: DefId, - ) -> (Option, bool) { - if let Some(target_module) = target_module.as_local() { - let mut finder = - UsePlacementFinder { definitions, target_module, span: None, found_use: false }; - visit::walk_crate(&mut finder, krate); - (finder.span, finder.found_use) - } else { - (None, false) - } +impl UsePlacementFinder { + fn check(krate: &Crate, target_module: NodeId) -> (Option, bool) { + let mut finder = UsePlacementFinder { target_module, span: None, found_use: false }; + visit::walk_crate(&mut finder, krate); + (finder.span, finder.found_use) } } -impl<'tcx, 'd> Visitor<'tcx> for UsePlacementFinder<'d> { +impl<'tcx> Visitor<'tcx> for UsePlacementFinder { fn visit_mod( &mut self, module: &'tcx ast::Mod, @@ -291,7 +283,7 @@ fn visit_mod( if self.span.is_some() { return; } - if self.definitions.local_def_id(node_id) != self.target_module { + if node_id != self.target_module { visit::walk_mod(self, module); return; } @@ -979,6 +971,17 @@ pub struct Resolver<'a> { lint_buffer: LintBuffer, next_node_id: NodeId, + + def_id_to_span: IndexVec, + + node_id_to_def_id: FxHashMap, + def_id_to_node_id: IndexVec, + + /// Indices of unnamed struct or variant fields with unresolved attributes. + placeholder_field_indices: FxHashMap, + /// When collecting definitions from an AST fragment produced by a macro invocation `ExpnId` + /// we know what parent node that fragment should be attached to thanks to this table. + invocation_parents: FxHashMap, } /// Nothing really interesting here; it just provides memory for the rest of the crate. @@ -1042,7 +1045,7 @@ fn parent(self, id: DefId) -> Option { /// This interface is used through the AST→HIR step, to embed full paths into the HIR. After that /// the resolver is no longer needed as all the relevant information is inline. -impl rustc_ast_lowering::Resolver for Resolver<'_> { +impl ResolverAstLowering for Resolver<'_> { fn def_key(&mut self, id: DefId) -> DefKey { if let Some(id) = id.as_local() { self.definitions().def_key(id) @@ -1113,6 +1116,47 @@ fn next_node_id(&mut self) -> NodeId { fn trait_map(&self) -> &NodeMap> { &self.trait_map } + + fn opt_local_def_id(&self, node: NodeId) -> Option { + self.node_id_to_def_id.get(&node).copied() + } + + fn local_def_id(&self, node: NodeId) -> LocalDefId { + self.opt_local_def_id(node).unwrap_or_else(|| panic!("no entry for node id: `{:?}`", node)) + } + + /// Adds a definition with a parent definition. + fn create_def_with_parent( + &mut self, + parent: LocalDefId, + node_id: ast::NodeId, + data: DefPathData, + expn_id: ExpnId, + span: Span, + ) -> LocalDefId { + assert!( + !self.node_id_to_def_id.contains_key(&node_id), + "adding a def'n for node-id {:?} and data {:?} but a previous def'n exists: {:?}", + node_id, + data, + self.definitions.def_key(self.node_id_to_def_id[&node_id]), + ); + + let def_id = self.definitions.create_def_with_parent(parent, data, expn_id); + + assert_eq!(self.def_id_to_span.push(span), def_id); + + // Some things for which we allocate `LocalDefId`s don't correspond to + // anything in the AST, so they don't have a `NodeId`. For these cases + // we don't need a mapping from `NodeId` to `LocalDefId`. + if node_id != ast::DUMMY_NODE_ID { + debug!("create_def_with_parent: def_id_to_node_id[{:?}] <-> {:?}", def_id, node_id); + self.node_id_to_def_id.insert(node_id, def_id); + } + assert_eq!(self.def_id_to_node_id.push(node_id), def_id); + + def_id + } } impl<'a> Resolver<'a> { @@ -1144,7 +1188,17 @@ pub fn new( module_map.insert(LocalDefId { local_def_index: CRATE_DEF_INDEX }, graph_root); let mut definitions = Definitions::default(); - definitions.create_root_def(crate_name, session.local_crate_disambiguator()); + let root = definitions.create_root_def(crate_name, session.local_crate_disambiguator()); + + let mut def_id_to_span = IndexVec::default(); + assert_eq!(def_id_to_span.push(rustc_span::DUMMY_SP), root); + let mut def_id_to_node_id = IndexVec::default(); + assert_eq!(def_id_to_node_id.push(CRATE_NODE_ID), root); + let mut node_id_to_def_id = FxHashMap::default(); + node_id_to_def_id.insert(CRATE_NODE_ID, root); + + let mut invocation_parents = FxHashMap::default(); + invocation_parents.insert(ExpnId::root(), root); let mut extern_prelude: FxHashMap> = session .opts @@ -1263,6 +1317,11 @@ pub fn new( variant_vis: Default::default(), lint_buffer: LintBuffer::default(), next_node_id: NodeId::from_u32(1), + def_id_to_span, + node_id_to_def_id, + def_id_to_node_id, + placeholder_field_indices: Default::default(), + invocation_parents, } } @@ -1457,7 +1516,7 @@ fn record_use( #[inline] fn add_to_glob_map(&mut self, import: &Import<'_>, ident: Ident) { if import.is_glob() { - let def_id = self.definitions.local_def_id(import.id); + let def_id = self.local_def_id(import.id); self.glob_map.entry(def_id).or_default().insert(ident.name); } } @@ -2538,7 +2597,11 @@ fn report_with_use_injections(&mut self, krate: &Crate) { for UseError { mut err, candidates, def_id, instead, suggestion } in self.use_injections.drain(..) { - let (span, found_use) = UsePlacementFinder::check(&self.definitions, krate, def_id); + let (span, found_use) = if let Some(def_id) = def_id.as_local() { + UsePlacementFinder::check(krate, self.def_id_to_node_id[def_id]) + } else { + (None, false) + }; if !candidates.is_empty() { diagnostics::show_candidates(&mut err, span, &candidates, instead, found_use); } else if let Some((span, msg, sugg, appl)) = suggestion { @@ -2934,6 +2997,12 @@ pub fn graph_root(&self) -> Module<'a> { pub fn all_macros(&self) -> &FxHashMap { &self.all_macros } + + /// Retrieves the span of the given `DefId` if `DefId` is in the local crate. + #[inline] + pub fn opt_span(&self, def_id: DefId) -> Option { + if let Some(def_id) = def_id.as_local() { Some(self.def_id_to_span[def_id]) } else { None } + } } fn names_to_string(names: &[Symbol]) -> String { diff --git a/src/librustc_resolve/macros.rs b/src/librustc_resolve/macros.rs index 1b49722355e5..398b0e92d9d8 100644 --- a/src/librustc_resolve/macros.rs +++ b/src/librustc_resolve/macros.rs @@ -7,6 +7,7 @@ use crate::{CrateLint, ParentScope, ResolutionError, Resolver, Scope, ScopeSet, Weak}; use crate::{ModuleKind, ModuleOrUniformRoot, NameBinding, PathResult, Segment, ToNameBinding}; use rustc_ast::ast::{self, NodeId}; +use rustc_ast_lowering::Resolver as ResolverAstLowering; use rustc_ast_pretty::pprust; use rustc_attr::{self as attr, StabilityLevel}; use rustc_data_structures::fx::FxHashSet; @@ -190,7 +191,7 @@ fn expansion_for_ast_pass( ))); let parent_scope = if let Some(module_id) = parent_module_id { - let parent_def_id = self.definitions.local_def_id(module_id); + let parent_def_id = self.local_def_id(module_id); self.definitions.add_parent_module_of_macro_def(expn_id, parent_def_id.to_def_id()); self.module_map[&parent_def_id] } else { @@ -340,7 +341,9 @@ fn check_unused_macros(&mut self) { } fn lint_node_id(&mut self, expn_id: ExpnId) -> NodeId { - self.definitions.lint_node_id(expn_id) + self.invocation_parents + .get(&expn_id) + .map_or(ast::CRATE_NODE_ID, |id| self.def_id_to_node_id[*id]) } fn has_derive_copy(&self, expn_id: ExpnId) -> bool { From 1d3f49f53654f12cf9f3501666c0dfd1afe5cf8b Mon Sep 17 00:00:00 2001 From: marmeladema Date: Sun, 21 Jun 2020 15:49:38 +0100 Subject: [PATCH 10/24] Always create a root definition when creating a new `Definitions` object. --- src/librustc_ast_lowering/lib.rs | 14 +++++----- src/librustc_hir/definitions.rs | 40 +++++++++++++++------------ src/librustc_interface/passes.rs | 5 +++- src/librustc_resolve/def_collector.rs | 2 +- src/librustc_resolve/lib.rs | 10 +++---- 5 files changed, 40 insertions(+), 31 deletions(-) diff --git a/src/librustc_ast_lowering/lib.rs b/src/librustc_ast_lowering/lib.rs index ac8a229da43d..39b14ac45883 100644 --- a/src/librustc_ast_lowering/lib.rs +++ b/src/librustc_ast_lowering/lib.rs @@ -210,7 +210,7 @@ fn resolve_str_path( fn local_def_id(&self, node: NodeId) -> LocalDefId; - fn create_def_with_parent( + fn create_def( &mut self, parent: LocalDefId, node_id: ast::NodeId, @@ -449,7 +449,7 @@ fn allocate_use_tree_hir_id_counters(&mut self, tree: &UseTree, owner: LocalDefI match tree.kind { UseTreeKind::Simple(_, id1, id2) => { for &id in &[id1, id2] { - self.lctx.resolver.create_def_with_parent( + self.lctx.resolver.create_def( owner, id, DefPathData::Misc, @@ -696,7 +696,7 @@ fn lower_node_id_with_owner(&mut self, ast_node_id: NodeId, owner: NodeId) -> hi *local_id_counter += 1; let owner = this.resolver.opt_local_def_id(owner).expect( - "you forgot to call `create_def_with_parent` or are lowering node-IDs \ + "you forgot to call `create_def` or are lowering node-IDs \ that do not belong to the current owner", ); @@ -824,7 +824,7 @@ fn lifetime_to_generic_param( }; // Add a definition for the in-band lifetime def. - self.resolver.create_def_with_parent( + self.resolver.create_def( parent_def_id, node_id, DefPathData::LifetimeNs(str_name), @@ -1112,7 +1112,7 @@ fn lower_assoc_ty_constraint( let impl_trait_node_id = self.resolver.next_node_id(); let parent_def_id = self.current_hir_id_owner.last().unwrap().0; - self.resolver.create_def_with_parent( + self.resolver.create_def( parent_def_id, impl_trait_node_id, DefPathData::ImplTrait, @@ -1178,7 +1178,7 @@ fn lower_generic_arg( let node_id = self.resolver.next_node_id(); // Add a definition for the in-band const def. - self.resolver.create_def_with_parent( + self.resolver.create_def( parent_def_id, node_id, DefPathData::AnonConst, @@ -1644,7 +1644,7 @@ fn visit_lifetime(&mut self, lifetime: &'v hir::Lifetime) { let def_node_id = self.context.resolver.next_node_id(); let hir_id = self.context.lower_node_id_with_owner(def_node_id, self.opaque_ty_id); - self.context.resolver.create_def_with_parent( + self.context.resolver.create_def( self.parent, def_node_id, DefPathData::LifetimeNs(name.ident().name), diff --git a/src/librustc_hir/definitions.rs b/src/librustc_hir/definitions.rs index c34607f8a94c..5e072d37eaad 100644 --- a/src/librustc_hir/definitions.rs +++ b/src/librustc_hir/definitions.rs @@ -71,9 +71,9 @@ pub fn size(&self) -> usize { } /// The definition table containing node definitions. -/// It holds the `DefPathTable` for local `DefId`s/`DefPath`s and it also stores a -/// mapping from `NodeId`s to local `DefId`s. -#[derive(Clone, Default)] +/// It holds the `DefPathTable` for `LocalDefId`s/`DefPath`s. +/// It also stores mappings to convert `LocalDefId`s to/from `HirId`s. +#[derive(Clone)] pub struct Definitions { table: DefPathTable, @@ -328,11 +328,7 @@ pub fn opt_hir_id_to_local_def_id(&self, hir_id: hir::HirId) -> Option LocalDefId { + pub fn new(crate_name: &str, crate_disambiguator: CrateDisambiguator) -> Definitions { let key = DefKey { parent: None, disambiguated_data: DisambiguatedDefPathData { @@ -344,24 +340,34 @@ pub fn create_root_def( let parent_hash = DefKey::root_parent_stable_hash(crate_name, crate_disambiguator); let def_path_hash = key.compute_stable_hash(parent_hash); - // Create the definition. - let root = LocalDefId { local_def_index: self.table.allocate(key, def_path_hash) }; + // Create the root definition. + let mut table = DefPathTable::default(); + let root = LocalDefId { local_def_index: table.allocate(key, def_path_hash) }; assert_eq!(root.local_def_index, CRATE_DEF_INDEX); - root + Definitions { + table, + def_id_to_hir_id: Default::default(), + hir_id_to_def_id: Default::default(), + expansions_that_defined: Default::default(), + next_disambiguator: Default::default(), + parent_modules_of_macro_defs: Default::default(), + } + } + + /// Retrieves the root definition. + pub fn get_root_def(&self) -> LocalDefId { + LocalDefId { local_def_index: CRATE_DEF_INDEX } } /// Adds a definition with a parent definition. - pub fn create_def_with_parent( + pub fn create_def( &mut self, parent: LocalDefId, data: DefPathData, expn_id: ExpnId, ) -> LocalDefId { - debug!( - "create_def_with_parent(parent={:?}, data={:?}, expn_id={:?})", - parent, data, expn_id - ); + debug!("create_def(parent={:?}, data={:?}, expn_id={:?})", parent, data, expn_id); // The root node must be created with `create_root_def()`. assert!(data != DefPathData::CrateRoot); @@ -382,7 +388,7 @@ pub fn create_def_with_parent( let parent_hash = self.table.def_path_hash(parent.local_def_index); let def_path_hash = key.compute_stable_hash(parent_hash); - debug!("create_def_with_parent: after disambiguation, key = {:?}", key); + debug!("create_def: after disambiguation, key = {:?}", key); // Create the definition. let def_id = LocalDefId { local_def_index: self.table.allocate(key, def_path_hash) }; diff --git a/src/librustc_interface/passes.rs b/src/librustc_interface/passes.rs index 1ed9bc3f1f50..ea3b19ab4a75 100644 --- a/src/librustc_interface/passes.rs +++ b/src/librustc_interface/passes.rs @@ -734,7 +734,10 @@ pub fn create_global_ctxt<'tcx>( arena: &'tcx WorkerLocal>, ) -> QueryContext<'tcx> { let sess = &compiler.session(); - let defs: &'tcx Definitions = arena.alloc(mem::take(&mut resolver_outputs.definitions)); + let defs: &'tcx Definitions = arena.alloc(mem::replace( + &mut resolver_outputs.definitions, + Definitions::new(crate_name, sess.local_crate_disambiguator()), + )); let query_result_on_disk_cache = rustc_incremental::load_query_result_cache(sess); diff --git a/src/librustc_resolve/def_collector.rs b/src/librustc_resolve/def_collector.rs index 7bf039ea8a53..f1063f42c91e 100644 --- a/src/librustc_resolve/def_collector.rs +++ b/src/librustc_resolve/def_collector.rs @@ -32,7 +32,7 @@ impl<'a, 'b> DefCollector<'a, 'b> { fn create_def(&mut self, node_id: NodeId, data: DefPathData, span: Span) -> LocalDefId { let parent_def = self.parent_def; debug!("create_def(node_id={:?}, data={:?}, parent_def={:?})", node_id, data, parent_def); - self.resolver.create_def_with_parent(parent_def, node_id, data, self.expansion, span) + self.resolver.create_def(parent_def, node_id, data, self.expansion, span) } fn with_parent(&mut self, parent_def: LocalDefId, f: F) { diff --git a/src/librustc_resolve/lib.rs b/src/librustc_resolve/lib.rs index 0aca5c4905df..6005f009cc3d 100644 --- a/src/librustc_resolve/lib.rs +++ b/src/librustc_resolve/lib.rs @@ -1126,7 +1126,7 @@ fn local_def_id(&self, node: NodeId) -> LocalDefId { } /// Adds a definition with a parent definition. - fn create_def_with_parent( + fn create_def( &mut self, parent: LocalDefId, node_id: ast::NodeId, @@ -1142,7 +1142,7 @@ fn create_def_with_parent( self.definitions.def_key(self.node_id_to_def_id[&node_id]), ); - let def_id = self.definitions.create_def_with_parent(parent, data, expn_id); + let def_id = self.definitions.create_def(parent, data, expn_id); assert_eq!(self.def_id_to_span.push(span), def_id); @@ -1150,7 +1150,7 @@ fn create_def_with_parent( // anything in the AST, so they don't have a `NodeId`. For these cases // we don't need a mapping from `NodeId` to `LocalDefId`. if node_id != ast::DUMMY_NODE_ID { - debug!("create_def_with_parent: def_id_to_node_id[{:?}] <-> {:?}", def_id, node_id); + debug!("create_def: def_id_to_node_id[{:?}] <-> {:?}", def_id, node_id); self.node_id_to_def_id.insert(node_id, def_id); } assert_eq!(self.def_id_to_node_id.push(node_id), def_id); @@ -1187,8 +1187,8 @@ pub fn new( let mut module_map = FxHashMap::default(); module_map.insert(LocalDefId { local_def_index: CRATE_DEF_INDEX }, graph_root); - let mut definitions = Definitions::default(); - let root = definitions.create_root_def(crate_name, session.local_crate_disambiguator()); + let definitions = Definitions::new(crate_name, session.local_crate_disambiguator()); + let root = definitions.get_root_def(); let mut def_id_to_span = IndexVec::default(); assert_eq!(def_id_to_span.push(rustc_span::DUMMY_SP), root); From bd4f6f0b7d88baa9a5ecb18a2a700978ddcd58ff Mon Sep 17 00:00:00 2001 From: marmeladema Date: Sun, 21 Jun 2020 23:49:06 +0100 Subject: [PATCH 11/24] Move `next_disambiguator` to `Resolver` --- src/librustc_hir/definitions.rs | 12 ++---------- src/librustc_resolve/lib.rs | 14 +++++++++++++- 2 files changed, 15 insertions(+), 11 deletions(-) diff --git a/src/librustc_hir/definitions.rs b/src/librustc_hir/definitions.rs index 5e072d37eaad..79b706827393 100644 --- a/src/librustc_hir/definitions.rs +++ b/src/librustc_hir/definitions.rs @@ -87,7 +87,6 @@ pub struct Definitions { parent_modules_of_macro_defs: FxHashMap, /// Item with a given `LocalDefId` was defined during macro expansion with ID `ExpnId`. expansions_that_defined: FxHashMap, - next_disambiguator: FxHashMap<(LocalDefId, DefPathData), u32>, } /// A unique identifier that we can use to lookup a definition @@ -350,7 +349,6 @@ pub fn new(crate_name: &str, crate_disambiguator: CrateDisambiguator) -> Definit def_id_to_hir_id: Default::default(), hir_id_to_def_id: Default::default(), expansions_that_defined: Default::default(), - next_disambiguator: Default::default(), parent_modules_of_macro_defs: Default::default(), } } @@ -366,20 +364,14 @@ pub fn create_def( parent: LocalDefId, data: DefPathData, expn_id: ExpnId, + mut next_disambiguator: impl FnMut(LocalDefId, DefPathData) -> u32, ) -> LocalDefId { debug!("create_def(parent={:?}, data={:?}, expn_id={:?})", parent, data, expn_id); // The root node must be created with `create_root_def()`. assert!(data != DefPathData::CrateRoot); - // Find the next free disambiguator for this key. - let disambiguator = { - let next_disamb = self.next_disambiguator.entry((parent, data)).or_insert(0); - let disambiguator = *next_disamb; - *next_disamb = next_disamb.checked_add(1).expect("disambiguator overflow"); - disambiguator - }; - + let disambiguator = next_disambiguator(parent, data); let key = DefKey { parent: Some(parent.local_def_index), disambiguated_data: DisambiguatedDefPathData { data, disambiguator }, diff --git a/src/librustc_resolve/lib.rs b/src/librustc_resolve/lib.rs index 6005f009cc3d..ce068b8ac69a 100644 --- a/src/librustc_resolve/lib.rs +++ b/src/librustc_resolve/lib.rs @@ -982,6 +982,8 @@ pub struct Resolver<'a> { /// When collecting definitions from an AST fragment produced by a macro invocation `ExpnId` /// we know what parent node that fragment should be attached to thanks to this table. invocation_parents: FxHashMap, + + next_disambiguator: FxHashMap<(LocalDefId, DefPathData), u32>, } /// Nothing really interesting here; it just provides memory for the rest of the crate. @@ -1142,7 +1144,16 @@ fn create_def( self.definitions.def_key(self.node_id_to_def_id[&node_id]), ); - let def_id = self.definitions.create_def(parent, data, expn_id); + // Find the next free disambiguator for this key. + let next_disambiguator = &mut self.next_disambiguator; + let next_disambiguator = |parent, data| { + let next_disamb = next_disambiguator.entry((parent, data)).or_insert(0); + let disambiguator = *next_disamb; + *next_disamb = next_disamb.checked_add(1).expect("disambiguator overflow"); + disambiguator + }; + + let def_id = self.definitions.create_def(parent, data, expn_id, next_disambiguator); assert_eq!(self.def_id_to_span.push(span), def_id); @@ -1322,6 +1333,7 @@ pub fn new( def_id_to_node_id, placeholder_field_indices: Default::default(), invocation_parents, + next_disambiguator: Default::default(), } } From 933fe805777e46163c52371d81390ba721a37252 Mon Sep 17 00:00:00 2001 From: Rich Kadel Date: Sun, 21 Jun 2020 23:48:39 -0700 Subject: [PATCH 12/24] num_counters to u32, after implementing TypeFoldable --- src/librustc_codegen_llvm/intrinsic.rs | 2 +- src/librustc_middle/mir/mod.rs | 2 +- src/librustc_middle/ty/structural_impls.rs | 1 + src/librustc_mir/transform/instrument_coverage.rs | 6 +++--- 4 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/librustc_codegen_llvm/intrinsic.rs b/src/librustc_codegen_llvm/intrinsic.rs index 8c0ccde6b16b..c6e7820a60ed 100644 --- a/src/librustc_codegen_llvm/intrinsic.rs +++ b/src/librustc_codegen_llvm/intrinsic.rs @@ -146,7 +146,7 @@ fn codegen_intrinsic_call<'b, Bx: BuilderMethods<'b, 'tcx>>( let (mangled_fn_name, _len_val) = self.const_str(mangled_fn.name); let hash = self.const_u64(coverage_data.hash); let index = args[0].immediate(); - let num_counters = self.const_u32(coverage_data.num_counters as u32); + let num_counters = self.const_u32(coverage_data.num_counters); debug!( "count_code_region to LLVM intrinsic instrprof.increment(fn_name={}, hash={:?}, num_counters={:?}, index={:?})", mangled_fn.name, hash, index, num_counters diff --git a/src/librustc_middle/mir/mod.rs b/src/librustc_middle/mir/mod.rs index a89a5ef3f821..e88329db992f 100644 --- a/src/librustc_middle/mir/mod.rs +++ b/src/librustc_middle/mir/mod.rs @@ -98,7 +98,7 @@ pub struct CoverageData { pub hash: u64, /// The total number of coverage region counters added to this MIR Body. - pub num_counters: usize, + pub num_counters: u32, } /// The lowered representation of a single function. diff --git a/src/librustc_middle/ty/structural_impls.rs b/src/librustc_middle/ty/structural_impls.rs index f04d31601ea5..463e2c57d46c 100644 --- a/src/librustc_middle/ty/structural_impls.rs +++ b/src/librustc_middle/ty/structural_impls.rs @@ -262,6 +262,7 @@ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { bool, usize, ::rustc_target::abi::VariantIdx, + u32, u64, String, crate::middle::region::Scope, diff --git a/src/librustc_mir/transform/instrument_coverage.rs b/src/librustc_mir/transform/instrument_coverage.rs index 793ccbb081be..a24d0acf4212 100644 --- a/src/librustc_mir/transform/instrument_coverage.rs +++ b/src/librustc_mir/transform/instrument_coverage.rs @@ -21,7 +21,7 @@ struct Instrumentor<'tcx> { tcx: TyCtxt<'tcx>, - num_counters: usize, + num_counters: u32, } impl<'tcx> MirPass<'tcx> for InstrumentCoverage { @@ -55,12 +55,12 @@ fn new(tcx: TyCtxt<'tcx>) -> Self { } fn next_counter(&mut self) -> u32 { - let next = self.num_counters as u32; + let next = self.num_counters; self.num_counters += 1; next } - fn inject_counters(&mut self, mir_body: &mut mir::Body<'tcx>) -> usize { + fn inject_counters(&mut self, mir_body: &mut mir::Body<'tcx>) -> u32 { // FIXME(richkadel): As a first step, counters are only injected at the top of each // function. The complete solution will inject counters at each conditional code branch. let top_of_function = START_BLOCK; From 932237b10184f0c70d045a5a8c5c1abad9d04725 Mon Sep 17 00:00:00 2001 From: Bastian Kauschke Date: Mon, 22 Jun 2020 14:42:26 +0200 Subject: [PATCH 13/24] fix `intrinsics::needs_drop` docs --- src/libcore/intrinsics.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libcore/intrinsics.rs b/src/libcore/intrinsics.rs index 9061145a695f..e5bb18d21c48 100644 --- a/src/libcore/intrinsics.rs +++ b/src/libcore/intrinsics.rs @@ -1291,7 +1291,7 @@ /// implements `Copy`. /// /// If the actual type neither requires drop glue nor implements - /// `Copy`, then may return `true` or `false`. + /// `Copy`, then the return value of this function is unspecified. /// /// The stabilized version of this intrinsic is /// [`std::mem::needs_drop`](../../std/mem/fn.needs_drop.html). From 3ed96a6d638f8dc8aa081ca1ad82e61caa8930ca Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Sun, 21 Jun 2020 22:32:35 -0400 Subject: [PATCH 14/24] Point at the call spawn when overflow occurs during monomorphization This improves the output for issue #72577, but there's still more work to be done. Currently, an overflow error during monomorphization results in an error that points at the function we were unable to monomorphize. However, we don't point at the call that caused the monomorphization to happen. In the overflow occurs in a large recursive function, it may be difficult to determine where the issue is. This commit tracks and `Span` information during collection of `MonoItem`s, which is used when emitting an overflow error. `MonoItem` itself is unchanged, so this only affects `src/librustc_mir/monomorphize/collector.rs` --- src/librustc_mir/monomorphize/collector.rs | 137 ++++++++++-------- .../ui/infinite/infinite-instantiation.rs | 10 +- .../ui/infinite/infinite-instantiation.stderr | 11 +- src/test/ui/issues/issue-67552.rs | 2 +- src/test/ui/issues/issue-67552.stderr | 8 +- src/test/ui/issues/issue-8727.rs | 6 +- src/test/ui/issues/issue-8727.stderr | 6 + ...-38591-non-regular-dropck-recursion.stderr | 18 +++ src/test/ui/recursion/recursion.rs | 5 +- src/test/ui/recursion/recursion.stderr | 9 +- 10 files changed, 132 insertions(+), 80 deletions(-) diff --git a/src/librustc_mir/monomorphize/collector.rs b/src/librustc_mir/monomorphize/collector.rs index 36f3947d8301..84ef040741a3 100644 --- a/src/librustc_mir/monomorphize/collector.rs +++ b/src/librustc_mir/monomorphize/collector.rs @@ -178,7 +178,7 @@ use rustc_data_structures::fx::{FxHashMap, FxHashSet}; use rustc_data_structures::sync::{par_iter, MTLock, MTRef, ParallelIterator}; -use rustc_errors::ErrorReported; +use rustc_errors::{ErrorReported, FatalError}; use rustc_hir as hir; use rustc_hir::def_id::{DefId, DefIdMap, LocalDefId, LOCAL_CRATE}; use rustc_hir::itemlikevisit::ItemLikeVisitor; @@ -195,6 +195,7 @@ use rustc_middle::ty::subst::{GenericArgKind, InternalSubsts}; use rustc_middle::ty::{self, GenericParamDefKind, Instance, Ty, TyCtxt, TypeFoldable}; use rustc_session::config::EntryFnType; +use rustc_span::source_map::{dummy_spanned, respan, Span, Spanned, DUMMY_SP}; use smallvec::SmallVec; use std::iter; @@ -294,7 +295,13 @@ pub fn collect_crate_mono_items( tcx.sess.time("monomorphization_collector_graph_walk", || { par_iter(roots).for_each(|root| { let mut recursion_depths = DefIdMap::default(); - collect_items_rec(tcx, root, visited, &mut recursion_depths, inlining_map); + collect_items_rec( + tcx, + dummy_spanned(root), + visited, + &mut recursion_depths, + inlining_map, + ); }); }); } @@ -323,29 +330,30 @@ fn collect_roots(tcx: TyCtxt<'_>, mode: MonoItemCollectionMode) -> Vec( tcx: TyCtxt<'tcx>, - starting_point: MonoItem<'tcx>, + starting_point: Spanned>, visited: MTRef<'_, MTLock>>>, recursion_depths: &mut DefIdMap, inlining_map: MTRef<'_, MTLock>>, ) { - if !visited.lock_mut().insert(starting_point) { + if !visited.lock_mut().insert(starting_point.node) { // We've been here already, no need to search again. return; } - debug!("BEGIN collect_items_rec({})", starting_point.to_string(tcx, true)); + debug!("BEGIN collect_items_rec({})", starting_point.node.to_string(tcx, true)); let mut neighbors = Vec::new(); let recursion_depth_reset; - match starting_point { + match starting_point.node { MonoItem::Static(def_id) => { let instance = Instance::mono(tcx, def_id); @@ -353,7 +361,7 @@ fn collect_items_rec<'tcx>( debug_assert!(should_monomorphize_locally(tcx, &instance)); let ty = instance.monomorphic_ty(tcx); - visit_drop_use(tcx, ty, true, &mut neighbors); + visit_drop_use(tcx, ty, true, starting_point.span, &mut neighbors); recursion_depth_reset = None; @@ -366,7 +374,8 @@ fn collect_items_rec<'tcx>( debug_assert!(should_monomorphize_locally(tcx, &instance)); // Keep track of the monomorphization recursion depth - recursion_depth_reset = Some(check_recursion_limit(tcx, instance, recursion_depths)); + recursion_depth_reset = + Some(check_recursion_limit(tcx, instance, starting_point.span, recursion_depths)); check_type_length_limit(tcx, instance); rustc_data_structures::stack::ensure_sufficient_stack(|| { @@ -378,7 +387,7 @@ fn collect_items_rec<'tcx>( } } - record_accesses(tcx, starting_point, &neighbors[..], inlining_map); + record_accesses(tcx, starting_point.node, neighbors.iter().map(|i| &i.node), inlining_map); for neighbour in neighbors { collect_items_rec(tcx, neighbour, visited, recursion_depths, inlining_map); @@ -388,13 +397,13 @@ fn collect_items_rec<'tcx>( recursion_depths.insert(def_id, depth); } - debug!("END collect_items_rec({})", starting_point.to_string(tcx, true)); + debug!("END collect_items_rec({})", starting_point.node.to_string(tcx, true)); } -fn record_accesses<'tcx>( +fn record_accesses<'a, 'tcx: 'a>( tcx: TyCtxt<'tcx>, caller: MonoItem<'tcx>, - callees: &[MonoItem<'tcx>], + callees: impl Iterator>, inlining_map: MTRef<'_, MTLock>>, ) { let is_inlining_candidate = |mono_item: &MonoItem<'tcx>| { @@ -405,7 +414,7 @@ fn record_accesses<'tcx>( // FIXME: Call `is_inlining_candidate` when pushing to `neighbors` in `collect_items_rec` // instead to avoid creating this `SmallVec`. let accesses: SmallVec<[_; 128]> = - callees.iter().map(|mono_item| (*mono_item, is_inlining_candidate(mono_item))).collect(); + callees.map(|mono_item| (*mono_item, is_inlining_candidate(mono_item))).collect(); inlining_map.lock_mut().record_accesses(caller, &accesses); } @@ -413,6 +422,7 @@ fn record_accesses<'tcx>( fn check_recursion_limit<'tcx>( tcx: TyCtxt<'tcx>, instance: Instance<'tcx>, + span: Span, recursion_depths: &mut DefIdMap, ) -> (DefId, usize) { let def_id = instance.def_id(); @@ -432,12 +442,13 @@ fn check_recursion_limit<'tcx>( // infinite expansion. if !tcx.sess.recursion_limit().value_within_limit(adjusted_recursion_depth) { let error = format!("reached the recursion limit while instantiating `{}`", instance); - if let Some(def_id) = def_id.as_local() { - let hir_id = tcx.hir().as_local_hir_id(def_id); - tcx.sess.span_fatal(tcx.hir().span(hir_id), &error); - } else { - tcx.sess.fatal(&error); - } + let mut err = tcx.sess.struct_span_fatal(span, &error); + err.span_note( + tcx.def_span(def_id), + &format!("`{}` defined here", tcx.def_path_str(def_id)), + ); + err.emit(); + FatalError.raise(); } recursion_depths.insert(def_id, recursion_depth + 1); @@ -498,7 +509,7 @@ fn check_type_length_limit<'tcx>(tcx: TyCtxt<'tcx>, instance: Instance<'tcx>) { struct MirNeighborCollector<'a, 'tcx> { tcx: TyCtxt<'tcx>, body: &'a mir::Body<'tcx>, - output: &'a mut Vec>, + output: &'a mut Vec>>, instance: Instance<'tcx>, } @@ -520,6 +531,8 @@ impl<'a, 'tcx> MirVisitor<'tcx> for MirNeighborCollector<'a, 'tcx> { fn visit_rvalue(&mut self, rvalue: &mir::Rvalue<'tcx>, location: Location) { debug!("visiting rvalue {:?}", *rvalue); + let span = self.body.source_info(location).span; + match *rvalue { // When doing an cast from a regular pointer to a fat pointer, we // have to instantiate all methods of the trait being cast to, so we @@ -542,6 +555,7 @@ fn visit_rvalue(&mut self, rvalue: &mir::Rvalue<'tcx>, location: Location) { self.tcx, target_ty, source_ty, + span, self.output, ); } @@ -553,7 +567,7 @@ fn visit_rvalue(&mut self, rvalue: &mir::Rvalue<'tcx>, location: Location) { ) => { let fn_ty = operand.ty(self.body, self.tcx); let fn_ty = self.monomorphize(fn_ty); - visit_fn_use(self.tcx, fn_ty, false, &mut self.output); + visit_fn_use(self.tcx, fn_ty, false, span, &mut self.output); } mir::Rvalue::Cast( mir::CastKind::Pointer(PointerCast::ClosureFnPointer(_)), @@ -571,7 +585,7 @@ fn visit_rvalue(&mut self, rvalue: &mir::Rvalue<'tcx>, location: Location) { ty::ClosureKind::FnOnce, ); if should_monomorphize_locally(self.tcx, &instance) { - self.output.push(create_fn_mono_item(instance)); + self.output.push(create_fn_mono_item(instance, span)); } } _ => bug!(), @@ -583,7 +597,7 @@ fn visit_rvalue(&mut self, rvalue: &mir::Rvalue<'tcx>, location: Location) { tcx.require_lang_item(ExchangeMallocFnLangItem, None); let instance = Instance::mono(tcx, exchange_malloc_fn_def_id); if should_monomorphize_locally(tcx, &instance) { - self.output.push(create_fn_mono_item(instance)); + self.output.push(create_fn_mono_item(instance, span)); } } mir::Rvalue::ThreadLocalRef(def_id) => { @@ -591,7 +605,7 @@ fn visit_rvalue(&mut self, rvalue: &mir::Rvalue<'tcx>, location: Location) { let instance = Instance::mono(self.tcx, def_id); if should_monomorphize_locally(self.tcx, &instance) { trace!("collecting thread-local static {:?}", def_id); - self.output.push(MonoItem::Static(def_id)); + self.output.push(respan(span, MonoItem::Static(def_id))); } } _ => { /* not interesting */ } @@ -626,32 +640,33 @@ fn visit_const(&mut self, constant: &&'tcx ty::Const<'tcx>, location: Location) fn visit_terminator(&mut self, terminator: &mir::Terminator<'tcx>, location: Location) { debug!("visiting terminator {:?} @ {:?}", terminator, location); + let source = self.body.source_info(location).span; let tcx = self.tcx; match terminator.kind { mir::TerminatorKind::Call { ref func, .. } => { let callee_ty = func.ty(self.body, tcx); let callee_ty = self.monomorphize(callee_ty); - visit_fn_use(self.tcx, callee_ty, true, &mut self.output); + visit_fn_use(self.tcx, callee_ty, true, source, &mut self.output); } mir::TerminatorKind::Drop { ref place, .. } | mir::TerminatorKind::DropAndReplace { ref place, .. } => { let ty = place.ty(self.body, self.tcx).ty; let ty = self.monomorphize(ty); - visit_drop_use(self.tcx, ty, true, self.output); + visit_drop_use(self.tcx, ty, true, source, self.output); } mir::TerminatorKind::InlineAsm { ref operands, .. } => { for op in operands { match *op { mir::InlineAsmOperand::SymFn { ref value } => { let fn_ty = self.monomorphize(value.literal.ty); - visit_fn_use(self.tcx, fn_ty, false, &mut self.output); + visit_fn_use(self.tcx, fn_ty, false, source, &mut self.output); } mir::InlineAsmOperand::SymStatic { def_id } => { let instance = Instance::mono(self.tcx, def_id); if should_monomorphize_locally(self.tcx, &instance) { trace!("collecting asm sym static {:?}", def_id); - self.output.push(MonoItem::Static(def_id)); + self.output.push(respan(source, MonoItem::Static(def_id))); } } _ => {} @@ -687,17 +702,19 @@ fn visit_drop_use<'tcx>( tcx: TyCtxt<'tcx>, ty: Ty<'tcx>, is_direct_call: bool, - output: &mut Vec>, + source: Span, + output: &mut Vec>>, ) { let instance = Instance::resolve_drop_in_place(tcx, ty); - visit_instance_use(tcx, instance, is_direct_call, output); + visit_instance_use(tcx, instance, is_direct_call, source, output); } fn visit_fn_use<'tcx>( tcx: TyCtxt<'tcx>, ty: Ty<'tcx>, is_direct_call: bool, - output: &mut Vec>, + source: Span, + output: &mut Vec>>, ) { if let ty::FnDef(def_id, substs) = ty.kind { let instance = if is_direct_call { @@ -706,7 +723,7 @@ fn visit_fn_use<'tcx>( ty::Instance::resolve_for_fn_ptr(tcx, ty::ParamEnv::reveal_all(), def_id, substs) .unwrap() }; - visit_instance_use(tcx, instance, is_direct_call, output); + visit_instance_use(tcx, instance, is_direct_call, source, output); } } @@ -714,7 +731,8 @@ fn visit_instance_use<'tcx>( tcx: TyCtxt<'tcx>, instance: ty::Instance<'tcx>, is_direct_call: bool, - output: &mut Vec>, + source: Span, + output: &mut Vec>>, ) { debug!("visit_item_use({:?}, is_direct_call={:?})", instance, is_direct_call); if !should_monomorphize_locally(tcx, &instance) { @@ -730,7 +748,7 @@ fn visit_instance_use<'tcx>( ty::InstanceDef::DropGlue(_, None) => { // Don't need to emit noop drop glue if we are calling directly. if !is_direct_call { - output.push(create_fn_mono_item(instance)); + output.push(create_fn_mono_item(instance, source)); } } ty::InstanceDef::DropGlue(_, Some(_)) @@ -740,7 +758,7 @@ fn visit_instance_use<'tcx>( | ty::InstanceDef::Item(..) | ty::InstanceDef::FnPtrShim(..) | ty::InstanceDef::CloneShim(..) => { - output.push(create_fn_mono_item(instance)); + output.push(create_fn_mono_item(instance, source)); } } } @@ -832,7 +850,6 @@ fn find_vtable_types_for_unsizing<'tcx>( let ptr_vtable = |inner_source: Ty<'tcx>, inner_target: Ty<'tcx>| { let param_env = ty::ParamEnv::reveal_all(); let type_has_metadata = |ty: Ty<'tcx>| -> bool { - use rustc_span::DUMMY_SP; if ty.is_sized(tcx.at(DUMMY_SP), param_env) { return false; } @@ -886,9 +903,9 @@ fn find_vtable_types_for_unsizing<'tcx>( } } -fn create_fn_mono_item(instance: Instance<'_>) -> MonoItem<'_> { +fn create_fn_mono_item(instance: Instance<'_>, source: Span) -> Spanned> { debug!("create_fn_mono_item(instance={})", instance); - MonoItem::Fn(instance) + respan(source, MonoItem::Fn(instance)) } /// Creates a `MonoItem` for each method that is referenced by the vtable for @@ -897,7 +914,8 @@ fn create_mono_items_for_vtable_methods<'tcx>( tcx: TyCtxt<'tcx>, trait_ty: Ty<'tcx>, impl_ty: Ty<'tcx>, - output: &mut Vec>, + source: Span, + output: &mut Vec>>, ) { assert!( !trait_ty.needs_subst() @@ -927,12 +945,12 @@ fn create_mono_items_for_vtable_methods<'tcx>( .unwrap() }) .filter(|&instance| should_monomorphize_locally(tcx, &instance)) - .map(create_fn_mono_item); + .map(|item| create_fn_mono_item(item, source)); output.extend(methods); } // Also add the destructor. - visit_drop_use(tcx, impl_ty, false, output); + visit_drop_use(tcx, impl_ty, false, source, output); } } @@ -943,7 +961,7 @@ fn create_mono_items_for_vtable_methods<'tcx>( struct RootCollector<'a, 'tcx> { tcx: TyCtxt<'tcx>, mode: MonoItemCollectionMode, - output: &'a mut Vec>, + output: &'a mut Vec>>, entry_fn: Option<(LocalDefId, EntryFnType)>, } @@ -980,7 +998,7 @@ fn visit_item(&mut self, item: &'v hir::Item<'v>) { let ty = Instance::new(def_id.to_def_id(), InternalSubsts::empty()) .monomorphic_ty(self.tcx); - visit_drop_use(self.tcx, ty, true, self.output); + visit_drop_use(self.tcx, ty, true, DUMMY_SP, self.output); } } } @@ -989,12 +1007,12 @@ fn visit_item(&mut self, item: &'v hir::Item<'v>) { "RootCollector: ItemKind::GlobalAsm({})", def_id_to_string(self.tcx, self.tcx.hir().local_def_id(item.hir_id)) ); - self.output.push(MonoItem::GlobalAsm(item.hir_id)); + self.output.push(dummy_spanned(MonoItem::GlobalAsm(item.hir_id))); } hir::ItemKind::Static(..) => { let def_id = self.tcx.hir().local_def_id(item.hir_id); debug!("RootCollector: ItemKind::Static({})", def_id_to_string(self.tcx, def_id)); - self.output.push(MonoItem::Static(def_id.to_def_id())); + self.output.push(dummy_spanned(MonoItem::Static(def_id.to_def_id()))); } hir::ItemKind::Const(..) => { // const items only generate mono items if they are @@ -1051,7 +1069,7 @@ fn push_if_root(&mut self, def_id: LocalDefId) { debug!("RootCollector::push_if_root: found root def_id={:?}", def_id); let instance = Instance::mono(self.tcx, def_id.to_def_id()); - self.output.push(create_fn_mono_item(instance)); + self.output.push(create_fn_mono_item(instance, DUMMY_SP)); } } @@ -1088,7 +1106,7 @@ fn push_extra_entry_roots(&mut self) { .unwrap() .unwrap(); - self.output.push(create_fn_mono_item(start_instance)); + self.output.push(create_fn_mono_item(start_instance, DUMMY_SP)); } } @@ -1100,7 +1118,7 @@ fn item_requires_monomorphization(tcx: TyCtxt<'_>, def_id: LocalDefId) -> bool { fn create_mono_items_for_default_impls<'tcx>( tcx: TyCtxt<'tcx>, item: &'tcx hir::Item<'tcx>, - output: &mut Vec>, + output: &mut Vec>>, ) { match item.kind { hir::ItemKind::Impl { ref generics, ref items, .. } => { @@ -1145,8 +1163,9 @@ fn create_mono_items_for_default_impls<'tcx>( .unwrap() .unwrap(); - let mono_item = create_fn_mono_item(instance); - if mono_item.is_instantiable(tcx) && should_monomorphize_locally(tcx, &instance) + let mono_item = create_fn_mono_item(instance, DUMMY_SP); + if mono_item.node.is_instantiable(tcx) + && should_monomorphize_locally(tcx, &instance) { output.push(mono_item); } @@ -1158,14 +1177,18 @@ fn create_mono_items_for_default_impls<'tcx>( } /// Scans the miri alloc in order to find function calls, closures, and drop-glue. -fn collect_miri<'tcx>(tcx: TyCtxt<'tcx>, alloc_id: AllocId, output: &mut Vec>) { +fn collect_miri<'tcx>( + tcx: TyCtxt<'tcx>, + alloc_id: AllocId, + output: &mut Vec>>, +) { match tcx.global_alloc(alloc_id) { GlobalAlloc::Static(def_id) => { assert!(!tcx.is_thread_local_static(def_id)); let instance = Instance::mono(tcx, def_id); if should_monomorphize_locally(tcx, &instance) { trace!("collecting static {:?}", def_id); - output.push(MonoItem::Static(def_id)); + output.push(dummy_spanned(MonoItem::Static(def_id))); } } GlobalAlloc::Memory(alloc) => { @@ -1179,7 +1202,7 @@ fn collect_miri<'tcx>(tcx: TyCtxt<'tcx>, alloc_id: AllocId, output: &mut Vec { if should_monomorphize_locally(tcx, &fn_instance) { trace!("collecting {:?} with {:#?}", alloc_id, fn_instance); - output.push(create_fn_mono_item(fn_instance)); + output.push(create_fn_mono_item(fn_instance, DUMMY_SP)); } } } @@ -1189,7 +1212,7 @@ fn collect_miri<'tcx>(tcx: TyCtxt<'tcx>, alloc_id: AllocId, output: &mut Vec( tcx: TyCtxt<'tcx>, instance: Instance<'tcx>, - output: &mut Vec>, + output: &mut Vec>>, ) { debug!("collect_neighbours: {:?}", instance.def_id()); let body = tcx.instance_mir(instance.def); @@ -1207,7 +1230,7 @@ fn def_id_to_string(tcx: TyCtxt<'_>, def_id: LocalDefId) -> String { fn collect_const_value<'tcx>( tcx: TyCtxt<'tcx>, value: ConstValue<'tcx>, - output: &mut Vec>, + output: &mut Vec>>, ) { match value { ConstValue::Scalar(Scalar::Ptr(ptr)) => collect_miri(tcx, ptr.alloc_id, output), diff --git a/src/test/ui/infinite/infinite-instantiation.rs b/src/test/ui/infinite/infinite-instantiation.rs index 6f53680f7c81..9fee01c1ba62 100644 --- a/src/test/ui/infinite/infinite-instantiation.rs +++ b/src/test/ui/infinite/infinite-instantiation.rs @@ -1,9 +1,3 @@ -// -// We get an error message at the top of file (dummy span). -// This is not helpful, but also kind of annoying to prevent, -// so for now just live with it. -// This test case was originally for issue #2258. - // build-fail trait ToOpt: Sized { @@ -23,11 +17,9 @@ fn to_option(&self) -> Option> { } fn function(counter: usize, t: T) { -//~^ ERROR reached the recursion limit while instantiating `function:: 0 { function(counter - 1, t.to_option()); - // FIXME(#4287) Error message should be here. It should be - // a type error to instantiate `test` at a type other than T. + //~^ ERROR reached the recursion limit while instantiating `function::>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>` - --> $DIR/infinite-instantiation.rs:25:1 + --> $DIR/infinite-instantiation.rs:21:9 + | +LL | function(counter - 1, t.to_option()); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +note: `function` defined here + --> $DIR/infinite-instantiation.rs:19:1 | LL | / fn function(counter: usize, t: T) { -LL | | LL | | if counter > 0 { LL | | function(counter - 1, t.to_option()); -... | +LL | | LL | | } LL | | } | |_^ diff --git a/src/test/ui/issues/issue-67552.rs b/src/test/ui/issues/issue-67552.rs index 1400c6f97b60..b0fcb74764b9 100644 --- a/src/test/ui/issues/issue-67552.rs +++ b/src/test/ui/issues/issue-67552.rs @@ -18,7 +18,6 @@ fn identity(x: T) -> T { } fn rec(mut it: T) -//~^ ERROR reached the recursion limit while instantiating where T: Iterator, { @@ -26,5 +25,6 @@ fn rec(mut it: T) T::count(it); } else { rec(identity(&mut it)) + //~^ ERROR reached the recursion limit while instantiating } } diff --git a/src/test/ui/issues/issue-67552.stderr b/src/test/ui/issues/issue-67552.stderr index 881f9d221d6a..3bb2016f07d2 100644 --- a/src/test/ui/issues/issue-67552.stderr +++ b/src/test/ui/issues/issue-67552.stderr @@ -1,10 +1,16 @@ error: reached the recursion limit while instantiating `rec::<&mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut Empty>` + --> $DIR/issue-67552.rs:27:9 + | +LL | rec(identity(&mut it)) + | ^^^^^^^^^^^^^^^^^^^^^^ + | +note: `rec` defined here --> $DIR/issue-67552.rs:20:1 | LL | / fn rec(mut it: T) -LL | | LL | | where LL | | T: Iterator, +LL | | { ... | LL | | } LL | | } diff --git a/src/test/ui/issues/issue-8727.rs b/src/test/ui/issues/issue-8727.rs index 80f360155cb4..14bdd8511119 100644 --- a/src/test/ui/issues/issue-8727.rs +++ b/src/test/ui/issues/issue-8727.rs @@ -3,12 +3,10 @@ // build-fail -fn generic() { +fn generic() { //~ WARN function cannot return without recursing generic::>(); } -//~^^^ ERROR reached the recursion limit while instantiating `generic::>(); = help: a `loop` may express intention better if this is on purpose error: reached the recursion limit while instantiating `generic::>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>` + --> $DIR/issue-8727.rs:7:5 + | +LL | generic::>(); + | ^^^^^^^^^^^^^^^^^^^^^^ + | +note: `generic` defined here --> $DIR/issue-8727.rs:6:1 | LL | / fn generic() { diff --git a/src/test/ui/recursion/issue-38591-non-regular-dropck-recursion.stderr b/src/test/ui/recursion/issue-38591-non-regular-dropck-recursion.stderr index de6df4cd0268..0552847c48ca 100644 --- a/src/test/ui/recursion/issue-38591-non-regular-dropck-recursion.stderr +++ b/src/test/ui/recursion/issue-38591-non-regular-dropck-recursion.stderr @@ -1,4 +1,22 @@ error: reached the recursion limit while instantiating `std::intrinsics::drop_in_place::> - shim(Some(S))` + --> $SRC_DIR/libcore/ptr/mod.rs:LL:COL + | +LL | / pub unsafe fn drop_in_place(to_drop: *mut T) { +LL | | // Code here does not matter - this is replaced by the +LL | | // real drop glue by the compiler. +LL | | drop_in_place(to_drop) +LL | | } + | |_^ + | +note: `std::intrinsics::drop_in_place` defined here + --> $SRC_DIR/libcore/ptr/mod.rs:LL:COL + | +LL | / pub unsafe fn drop_in_place(to_drop: *mut T) { +LL | | // Code here does not matter - this is replaced by the +LL | | // real drop glue by the compiler. +LL | | drop_in_place(to_drop) +LL | | } + | |_^ error: aborting due to previous error diff --git a/src/test/ui/recursion/recursion.rs b/src/test/ui/recursion/recursion.rs index bf1eaef367d6..373cc17d0e0f 100644 --- a/src/test/ui/recursion/recursion.rs +++ b/src/test/ui/recursion/recursion.rs @@ -12,11 +12,10 @@ fn dot(&self, other:Cons) -> isize { self.head * other.head + self.tail.dot(other.tail) } } -fn test (n:isize, i:isize, first:T, second:T) ->isize { //~ ERROR recursion limit +fn test (n:isize, i:isize, first:T, second:T) ->isize { match n { 0 => {first.dot(second)} - // FIXME(#4287) Error message should be here. It should be - // a type error to instantiate `test` at a type other than T. _ => {test (n-1, i+1, Cons {head:2*i+1, tail:first}, Cons{head:i*i, tail:second})} + //~^ ERROR recursion limit } } pub fn main() { diff --git a/src/test/ui/recursion/recursion.stderr b/src/test/ui/recursion/recursion.stderr index 1a65b0e84f6a..0c0eba68c83b 100644 --- a/src/test/ui/recursion/recursion.stderr +++ b/src/test/ui/recursion/recursion.stderr @@ -1,11 +1,16 @@ error: reached the recursion limit while instantiating `test::>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>` + --> $DIR/recursion.rs:17:11 + | +LL | _ => {test (n-1, i+1, Cons {head:2*i+1, tail:first}, Cons{head:i*i, tail:second})} + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +note: `test` defined here --> $DIR/recursion.rs:15:1 | LL | / fn test (n:isize, i:isize, first:T, second:T) ->isize { LL | | match n { 0 => {first.dot(second)} -LL | | // FIXME(#4287) Error message should be here. It should be -LL | | // a type error to instantiate `test` at a type other than T. LL | | _ => {test (n-1, i+1, Cons {head:2*i+1, tail:first}, Cons{head:i*i, tail:second})} +LL | | LL | | } LL | | } | |_^ From f4a79385cf28b263894be9ebd2e541532ae82898 Mon Sep 17 00:00:00 2001 From: Rich Kadel Date: Mon, 22 Jun 2020 14:11:55 -0700 Subject: [PATCH 15/24] implemented query for coverage data This commit adds a query that allows the CoverageData to be pulled from a call on tcx, avoiding the need to change the `codegen_intrinsic_call()` signature (no need to pass in the FunctionCx or any additional arguments. The commit does not change where/when the CoverageData is computed. It's still done in the `pass`, and saved in the MIR `Body`. See discussion (in progress) here: https://github.com/rust-lang/rust/pull/73488#discussion_r443825646 --- src/librustc_codegen_llvm/intrinsic.rs | 12 +++++++----- src/librustc_codegen_ssa/mir/block.rs | 2 +- src/librustc_codegen_ssa/mir/mod.rs | 4 ++-- src/librustc_codegen_ssa/traits/intrinsic.rs | 6 ++---- src/librustc_middle/query/mod.rs | 6 ++++++ src/librustc_mir/transform/mod.rs | 8 +++++++- 6 files changed, 25 insertions(+), 13 deletions(-) diff --git a/src/librustc_codegen_llvm/intrinsic.rs b/src/librustc_codegen_llvm/intrinsic.rs index c6e7820a60ed..f1104ca3a98b 100644 --- a/src/librustc_codegen_llvm/intrinsic.rs +++ b/src/librustc_codegen_llvm/intrinsic.rs @@ -16,7 +16,6 @@ use rustc_codegen_ssa::glue; use rustc_codegen_ssa::mir::operand::{OperandRef, OperandValue}; use rustc_codegen_ssa::mir::place::PlaceRef; -use rustc_codegen_ssa::mir::FunctionCx; use rustc_codegen_ssa::traits::*; use rustc_codegen_ssa::MemFlags; use rustc_hir as hir; @@ -82,14 +81,14 @@ fn get_simple_intrinsic(cx: &CodegenCx<'ll, '_>, name: &str) -> Option<&'ll Valu } impl IntrinsicCallMethods<'tcx> for Builder<'a, 'll, 'tcx> { - fn codegen_intrinsic_call<'b, Bx: BuilderMethods<'b, 'tcx>>( + fn codegen_intrinsic_call( &mut self, - fx: &FunctionCx<'b, 'tcx, Bx>, instance: ty::Instance<'tcx>, fn_abi: &FnAbi<'tcx, Ty<'tcx>>, args: &[OperandRef<'tcx, &'ll Value>], llresult: &'ll Value, span: Span, + caller_instance: ty::Instance<'tcx>, ) { let tcx = self.tcx; let callee_ty = instance.monomorphic_ty(tcx); @@ -141,8 +140,11 @@ fn codegen_intrinsic_call<'b, Bx: BuilderMethods<'b, 'tcx>>( self.call(llfn, &[], None) } "count_code_region" => { - let coverage_data = fx.mir.coverage_data.as_ref().unwrap(); - let mangled_fn = tcx.symbol_name(fx.instance); + let coverage_data = tcx + .coverage_data(caller_instance.def_id()) + .as_ref() + .expect("LLVM intrinsic count_code_region call has associated coverage_data"); + let mangled_fn = tcx.symbol_name(caller_instance); let (mangled_fn_name, _len_val) = self.const_str(mangled_fn.name); let hash = self.const_u64(coverage_data.hash); let index = args[0].immediate(); diff --git a/src/librustc_codegen_ssa/mir/block.rs b/src/librustc_codegen_ssa/mir/block.rs index 945c3d484347..d56c816811b3 100644 --- a/src/librustc_codegen_ssa/mir/block.rs +++ b/src/librustc_codegen_ssa/mir/block.rs @@ -688,12 +688,12 @@ fn codegen_call_terminator( .collect(); bx.codegen_intrinsic_call( - self, *instance.as_ref().unwrap(), &fn_abi, &args, dest, terminator.source_info.span, + self.instance, ); if let ReturnDest::IndirectOperand(dst, _) = ret_dest { diff --git a/src/librustc_codegen_ssa/mir/mod.rs b/src/librustc_codegen_ssa/mir/mod.rs index fd2e779f681b..00b4bf96afa5 100644 --- a/src/librustc_codegen_ssa/mir/mod.rs +++ b/src/librustc_codegen_ssa/mir/mod.rs @@ -21,9 +21,9 @@ /// Master context for codegenning from MIR. pub struct FunctionCx<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> { - pub instance: Instance<'tcx>, + instance: Instance<'tcx>, - pub mir: &'tcx mir::Body<'tcx>, + mir: &'tcx mir::Body<'tcx>, debug_context: Option>, diff --git a/src/librustc_codegen_ssa/traits/intrinsic.rs b/src/librustc_codegen_ssa/traits/intrinsic.rs index 0036eadf4db8..f62019498511 100644 --- a/src/librustc_codegen_ssa/traits/intrinsic.rs +++ b/src/librustc_codegen_ssa/traits/intrinsic.rs @@ -1,7 +1,5 @@ use super::BackendTypes; use crate::mir::operand::OperandRef; -use crate::mir::FunctionCx; -use crate::traits::BuilderMethods; use rustc_middle::ty::{self, Ty}; use rustc_span::Span; use rustc_target::abi::call::FnAbi; @@ -10,14 +8,14 @@ pub trait IntrinsicCallMethods<'tcx>: BackendTypes { /// Remember to add all intrinsics here, in librustc_typeck/check/mod.rs, /// and in libcore/intrinsics.rs; if you need access to any llvm intrinsics, /// add them to librustc_codegen_llvm/context.rs - fn codegen_intrinsic_call<'a, Bx: BuilderMethods<'a, 'tcx>>( + fn codegen_intrinsic_call( &mut self, - fx: &FunctionCx<'a, 'tcx, Bx>, instance: ty::Instance<'tcx>, fn_abi: &FnAbi<'tcx, Ty<'tcx>>, args: &[OperandRef<'tcx, Self::Value>], llresult: Self::Value, span: Span, + caller_instance: ty::Instance<'tcx>, ); fn abort(&mut self); diff --git a/src/librustc_middle/query/mod.rs b/src/librustc_middle/query/mod.rs index 3b6d54a1bc1e..1092e6c30641 100644 --- a/src/librustc_middle/query/mod.rs +++ b/src/librustc_middle/query/mod.rs @@ -214,6 +214,12 @@ fn describe_as_module(def_id: DefId, tcx: TyCtxt<'_>) -> String { cache_on_disk_if { key.is_local() } } + query coverage_data(key: DefId) -> Option { + desc { |tcx| "retrieving coverage data, if computed from MIR for `{}`", tcx.def_path_str(key) } + storage(ArenaCacheSelector<'tcx>) + cache_on_disk_if { key.is_local() } + } + query promoted_mir(key: DefId) -> IndexVec> { desc { |tcx| "optimizing promoted MIR for `{}`", tcx.def_path_str(key) } storage(ArenaCacheSelector<'tcx>) diff --git a/src/librustc_mir/transform/mod.rs b/src/librustc_mir/transform/mod.rs index 846ed1f86d8d..a2fdc7efd184 100644 --- a/src/librustc_mir/transform/mod.rs +++ b/src/librustc_mir/transform/mod.rs @@ -6,7 +6,7 @@ use rustc_hir::intravisit::{self, NestedVisitorMap, Visitor}; use rustc_index::vec::IndexVec; use rustc_middle::mir::visit::Visitor as _; -use rustc_middle::mir::{traversal, Body, ConstQualifs, MirPhase, Promoted}; +use rustc_middle::mir::{traversal, Body, ConstQualifs, CoverageData, MirPhase, Promoted}; use rustc_middle::ty::query::Providers; use rustc_middle::ty::steal::Steal; use rustc_middle::ty::{InstanceDef, TyCtxt, TypeFoldable}; @@ -53,6 +53,7 @@ pub(crate) fn provide(providers: &mut Providers<'_>) { mir_drops_elaborated_and_const_checked, optimized_mir, is_mir_available, + coverage_data, promoted_mir, ..*providers }; @@ -422,6 +423,11 @@ fn run_optimization_passes<'tcx>( ); } +fn coverage_data(tcx: TyCtxt<'_>, def_id: DefId) -> Option { + let body = tcx.optimized_mir(def_id); + body.coverage_data.clone() +} + fn optimized_mir(tcx: TyCtxt<'_>, def_id: DefId) -> Body<'_> { if tcx.is_constructor(def_id) { // There's no reason to run all of the MIR passes on constructors when From f84b7e1b052fd135ae2e754499b4fe286d5ba699 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Mon, 22 Jun 2020 12:28:16 -0700 Subject: [PATCH 16/24] Provide context on E0308 involving fn items --- .../infer/error_reporting/mod.rs | 2 +- src/librustc_typeck/check/demand.rs | 1 + src/librustc_typeck/check/mod.rs | 42 +++++++++++++++++++ src/test/ui/fn/fn-item-type.rs | 34 ++++++++++++--- src/test/ui/fn/fn-item-type.stderr | 31 ++++++++++++-- 5 files changed, 99 insertions(+), 11 deletions(-) diff --git a/src/librustc_infer/infer/error_reporting/mod.rs b/src/librustc_infer/infer/error_reporting/mod.rs index 9cfa11dd7c81..7fdcbd31df3c 100644 --- a/src/librustc_infer/infer/error_reporting/mod.rs +++ b/src/librustc_infer/infer/error_reporting/mod.rs @@ -1256,7 +1256,7 @@ fn lifetime_display(lifetime: Region<'_>) -> String { (ty::FnDef(did1, substs1), ty::FnPtr(sig2)) => { let sig1 = self.tcx.fn_sig(*did1).subst(self.tcx, substs1); let mut values = self.cmp_fn_sig(&sig1, sig2); - values.0.push_normal(format!( + values.0.push_highlighted(format!( " {{{}}}", self.tcx.def_path_str_with_substs(*did1, substs1) )); diff --git a/src/librustc_typeck/check/demand.rs b/src/librustc_typeck/check/demand.rs index 019b4ca66060..f4f630e94a70 100644 --- a/src/librustc_typeck/check/demand.rs +++ b/src/librustc_typeck/check/demand.rs @@ -34,6 +34,7 @@ pub fn emit_coerce_suggestions( } self.suggest_boxing_when_appropriate(err, expr, expected, expr_ty); self.suggest_missing_await(err, expr, expected, expr_ty); + self.note_need_for_fn_pointer(err, expected, expr_ty); } // Requires that the two types unify, and prints an error message if diff --git a/src/librustc_typeck/check/mod.rs b/src/librustc_typeck/check/mod.rs index b60b06567d6f..234a573b725e 100644 --- a/src/librustc_typeck/check/mod.rs +++ b/src/librustc_typeck/check/mod.rs @@ -5496,6 +5496,48 @@ fn suggest_missing_await( } } + fn note_need_for_fn_pointer( + &self, + err: &mut DiagnosticBuilder<'_>, + expected: Ty<'tcx>, + found: Ty<'tcx>, + ) { + match (&expected.kind, &found.kind) { + (ty::FnDef(did1, substs1), ty::FnDef(did2, substs2)) => { + let sig1 = self.tcx.fn_sig(*did1).subst(self.tcx, substs1); + let sig2 = self.tcx.fn_sig(*did2).subst(self.tcx, substs2); + if sig1 != sig2 { + return; + } + err.note( + "different `fn` items always have unique types, even if their signatures are \ + the same", + ); + err.help(&format!("change the expectation to require function pointer `{}`", sig1)); + err.help(&format!( + "if the expectation is due to type inference, cast the expected `fn` to a \ + function pointer: `{} as {}`", + self.tcx.def_path_str_with_substs(*did1, substs1), + sig1 + )); + } + (ty::FnDef(did, substs), ty::FnPtr(sig2)) => { + let sig1 = self.tcx.fn_sig(*did).subst(self.tcx, substs); + if sig1 != *sig2 { + return; + } + err.help(&format!("change the expectation to require function pointer `{}`", sig1)); + err.help(&format!( + "if the expectation is due to type inference, cast the expected `fn` to a \ + function pointer: `{} as {}`", + self.tcx.def_path_str_with_substs(*did, substs), + sig1 + )); + } + _ => {} + } + } + /// A common error is to add an extra semicolon: /// /// ``` diff --git a/src/test/ui/fn/fn-item-type.rs b/src/test/ui/fn/fn-item-type.rs index 68b75c18a43d..256b9d45755c 100644 --- a/src/test/ui/fn/fn-item-type.rs +++ b/src/test/ui/fn/fn-item-type.rs @@ -12,22 +12,44 @@ impl Foo for T { /* `foo` is still default here */ } fn main() { eq(foo::, bar::); //~^ ERROR mismatched types - //~| expected fn item `fn(_) -> _ {foo::}` - //~| found fn item `fn(_) -> _ {bar::}` - //~| expected fn item, found a different fn item + //~| expected fn item `fn(_) -> _ {foo::}` + //~| found fn item `fn(_) -> _ {bar::}` + //~| expected fn item, found a different fn item + //~| different `fn` items always have unique types, even if their signatures are the same + //~| change the expectation to require function pointer + //~| if the expectation is due to type inference, cast the expected `fn` to a function pointer eq(foo::, foo::); //~^ ERROR mismatched types //~| expected `u8`, found `i8` + //~| different `fn` items always have unique types, even if their signatures are the same + //~| change the expectation to require function pointer + //~| if the expectation is due to type inference, cast the expected `fn` to a function pointer eq(bar::, bar::>); //~^ ERROR mismatched types - //~| expected fn item `fn(_) -> _ {bar::}` - //~| found fn item `fn(_) -> _ {bar::>}` - //~| expected struct `std::string::String`, found struct `std::vec::Vec` + //~| expected fn item `fn(_) -> _ {bar::}` + //~| found fn item `fn(_) -> _ {bar::>}` + //~| expected struct `std::string::String`, found struct `std::vec::Vec` + //~| different `fn` items always have unique types, even if their signatures are the same + //~| change the expectation to require function pointer + //~| if the expectation is due to type inference, cast the expected `fn` to a function pointer // Make sure we distinguish between trait methods correctly. eq(::foo, ::foo); //~^ ERROR mismatched types //~| expected `u8`, found `u16` + //~| different `fn` items always have unique types, even if their signatures are the same + //~| change the expectation to require function pointer + //~| if the expectation is due to type inference, cast the expected `fn` to a function pointer + + eq(foo::, bar:: as fn(isize) -> isize); + //~^ ERROR mismatched types + //~| expected fn item `fn(_) -> _ {foo::}` + //~| found fn pointer `fn(_) -> _` + //~| expected fn item, found fn pointer + //~| change the expectation to require function pointer + //~| if the expectation is due to type inference, cast the expected `fn` to a function pointer + + eq(foo:: as fn(isize) -> isize, bar::); // ok! } diff --git a/src/test/ui/fn/fn-item-type.stderr b/src/test/ui/fn/fn-item-type.stderr index 4cce25c43c48..84f5e034340e 100644 --- a/src/test/ui/fn/fn-item-type.stderr +++ b/src/test/ui/fn/fn-item-type.stderr @@ -6,34 +6,57 @@ LL | eq(foo::, bar::); | = note: expected fn item `fn(_) -> _ {foo::}` found fn item `fn(_) -> _ {bar::}` + = note: different `fn` items always have unique types, even if their signatures are the same + = help: change the expectation to require function pointer `fn(isize) -> isize` + = help: if the expectation is due to type inference, cast the expected `fn` to a function pointer: `foo:: as fn(isize) -> isize` error[E0308]: mismatched types - --> $DIR/fn-item-type.rs:19:19 + --> $DIR/fn-item-type.rs:22:19 | LL | eq(foo::, foo::); | ^^^^^^^^^ expected `u8`, found `i8` | = note: expected fn item `fn(_) -> _ {foo::}` found fn item `fn(_) -> _ {foo::}` + = note: different `fn` items always have unique types, even if their signatures are the same + = help: change the expectation to require function pointer `fn(isize) -> isize` + = help: if the expectation is due to type inference, cast the expected `fn` to a function pointer: `foo:: as fn(isize) -> isize` error[E0308]: mismatched types - --> $DIR/fn-item-type.rs:23:23 + --> $DIR/fn-item-type.rs:29:23 | LL | eq(bar::, bar::>); | ^^^^^^^^^^^^^^ expected struct `std::string::String`, found struct `std::vec::Vec` | = note: expected fn item `fn(_) -> _ {bar::}` found fn item `fn(_) -> _ {bar::>}` + = note: different `fn` items always have unique types, even if their signatures are the same + = help: change the expectation to require function pointer `fn(isize) -> isize` + = help: if the expectation is due to type inference, cast the expected `fn` to a function pointer: `bar:: as fn(isize) -> isize` error[E0308]: mismatched types - --> $DIR/fn-item-type.rs:30:26 + --> $DIR/fn-item-type.rs:39:26 | LL | eq(::foo, ::foo); | ^^^^^^^^^^^^^^^^^ expected `u8`, found `u16` | = note: expected fn item `fn() {::foo}` found fn item `fn() {::foo}` + = note: different `fn` items always have unique types, even if their signatures are the same + = help: change the expectation to require function pointer `fn()` + = help: if the expectation is due to type inference, cast the expected `fn` to a function pointer: `::foo as fn()` -error: aborting due to 4 previous errors +error[E0308]: mismatched types + --> $DIR/fn-item-type.rs:46:19 + | +LL | eq(foo::, bar:: as fn(isize) -> isize); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected fn item, found fn pointer + | + = note: expected fn item `fn(_) -> _ {foo::}` + found fn pointer `fn(_) -> _` + = help: change the expectation to require function pointer `fn(isize) -> isize` + = help: if the expectation is due to type inference, cast the expected `fn` to a function pointer: `foo:: as fn(isize) -> isize` + +error: aborting due to 5 previous errors For more information about this error, try `rustc --explain E0308`. From 994d9d03272363d57a655ebacbf58f0069b3b177 Mon Sep 17 00:00:00 2001 From: Rich Kadel Date: Mon, 22 Jun 2020 15:54:28 -0700 Subject: [PATCH 17/24] Address remaining feedback items --- src/librustc_metadata/locator.rs | 2 +- src/librustc_middle/hir/map/mod.rs | 2 +- src/librustc_middle/query/mod.rs | 2 +- .../transform/instrument_coverage.rs | 27 ++----------------- 4 files changed, 5 insertions(+), 28 deletions(-) diff --git a/src/librustc_metadata/locator.rs b/src/librustc_metadata/locator.rs index 662794f0aa11..1bdac1039b55 100644 --- a/src/librustc_metadata/locator.rs +++ b/src/librustc_metadata/locator.rs @@ -489,7 +489,7 @@ impl<'a> CrateLocator<'a> { { err.note(&format!("the `{}` target may not be installed", self.triple)); } else if self.crate_name == sym::profiler_builtins { - err.note(&"the compiler may have been built without `profiler = true`"); + err.note(&"the compiler may have been built without the profiler runtime"); } err.span_label(self.span, "can't find crate"); err diff --git a/src/librustc_middle/hir/map/mod.rs b/src/librustc_middle/hir/map/mod.rs index e3e0856ffc52..d60d24aa9aed 100644 --- a/src/librustc_middle/hir/map/mod.rs +++ b/src/librustc_middle/hir/map/mod.rs @@ -56,7 +56,7 @@ fn fn_sig<'hir>(node: Node<'hir>) -> Option<&'hir FnSig<'hir>> { } } -fn associated_body<'hir>(node: Node<'hir>) -> Option { +pub fn associated_body<'hir>(node: Node<'hir>) -> Option { match node { Node::Item(Item { kind: ItemKind::Const(_, body) | ItemKind::Static(.., body) | ItemKind::Fn(.., body), diff --git a/src/librustc_middle/query/mod.rs b/src/librustc_middle/query/mod.rs index 1092e6c30641..993b48afb7a9 100644 --- a/src/librustc_middle/query/mod.rs +++ b/src/librustc_middle/query/mod.rs @@ -215,7 +215,7 @@ fn describe_as_module(def_id: DefId, tcx: TyCtxt<'_>) -> String { } query coverage_data(key: DefId) -> Option { - desc { |tcx| "retrieving coverage data, if computed from MIR for `{}`", tcx.def_path_str(key) } + desc { |tcx| "retrieving coverage data from MIR for `{}`", tcx.def_path_str(key) } storage(ArenaCacheSelector<'tcx>) cache_on_disk_if { key.is_local() } } diff --git a/src/librustc_mir/transform/instrument_coverage.rs b/src/librustc_mir/transform/instrument_coverage.rs index a24d0acf4212..94aa26b3081e 100644 --- a/src/librustc_mir/transform/instrument_coverage.rs +++ b/src/librustc_mir/transform/instrument_coverage.rs @@ -3,7 +3,7 @@ use rustc_data_structures::fingerprint::Fingerprint; use rustc_data_structures::stable_hasher::{HashStable, StableHasher}; use rustc_hir::lang_items; -use rustc_hir::*; +use rustc_middle::hir; use rustc_middle::ich::StableHashingContext; use rustc_middle::mir::interpret::Scalar; use rustc_middle::mir::{ @@ -140,7 +140,7 @@ fn placeholder_block(span: Span) -> BasicBlockData<'tcx> { fn hash_mir_source<'tcx>(tcx: TyCtxt<'tcx>, src: &MirSource<'tcx>) -> u64 { let fn_body_id = match tcx.hir().get_if_local(src.def_id()) { - Some(node) => match associated_body(node) { + Some(node) => match hir::map::associated_body(node) { Some(body_id) => body_id, _ => bug!("instrumented MirSource does not include a function body: {:?}", node), }, @@ -159,26 +159,3 @@ fn hash( node.hash_stable(hcx, &mut stable_hasher); stable_hasher.finish() } - -fn associated_body<'hir>(node: Node<'hir>) -> Option { - match node { - Node::Item(Item { - kind: ItemKind::Const(_, body) | ItemKind::Static(.., body) | ItemKind::Fn(.., body), - .. - }) - | Node::TraitItem(TraitItem { - kind: - TraitItemKind::Const(_, Some(body)) | TraitItemKind::Fn(_, TraitFn::Provided(body)), - .. - }) - | Node::ImplItem(ImplItem { - kind: ImplItemKind::Const(_, body) | ImplItemKind::Fn(_, body), - .. - }) - | Node::Expr(Expr { kind: ExprKind::Closure(.., body, _, _), .. }) => Some(*body), - - Node::AnonConst(constant) => Some(constant.body), - - _ => None, - } -} From 08ec4cbb9e0672118946c85f410b50ea4001b1dd Mon Sep 17 00:00:00 2001 From: Rich Kadel Date: Mon, 22 Jun 2020 19:21:56 -0700 Subject: [PATCH 18/24] moves coverage data computation from pass to query --- src/librustc_codegen_llvm/intrinsic.rs | 9 +-- src/librustc_middle/mir/mod.rs | 34 +++++------ src/librustc_middle/query/mod.rs | 2 +- .../transform/instrument_coverage.rs | 58 +++++++++++-------- src/librustc_mir/transform/mod.rs | 9 +-- 5 files changed, 56 insertions(+), 56 deletions(-) diff --git a/src/librustc_codegen_llvm/intrinsic.rs b/src/librustc_codegen_llvm/intrinsic.rs index f1104ca3a98b..b9193a85b1e4 100644 --- a/src/librustc_codegen_llvm/intrinsic.rs +++ b/src/librustc_codegen_llvm/intrinsic.rs @@ -140,18 +140,15 @@ fn codegen_intrinsic_call( self.call(llfn, &[], None) } "count_code_region" => { - let coverage_data = tcx - .coverage_data(caller_instance.def_id()) - .as_ref() - .expect("LLVM intrinsic count_code_region call has associated coverage_data"); + let coverage_data = tcx.coverage_data(caller_instance.def_id()); let mangled_fn = tcx.symbol_name(caller_instance); let (mangled_fn_name, _len_val) = self.const_str(mangled_fn.name); let hash = self.const_u64(coverage_data.hash); - let index = args[0].immediate(); let num_counters = self.const_u32(coverage_data.num_counters); + let index = args[0].immediate(); debug!( "count_code_region to LLVM intrinsic instrprof.increment(fn_name={}, hash={:?}, num_counters={:?}, index={:?})", - mangled_fn.name, hash, index, num_counters + mangled_fn.name, hash, num_counters, index ); self.instrprof_increment(mangled_fn_name, hash, num_counters, index) } diff --git a/src/librustc_middle/mir/mod.rs b/src/librustc_middle/mir/mod.rs index e88329db992f..854fda095b65 100644 --- a/src/librustc_middle/mir/mod.rs +++ b/src/librustc_middle/mir/mod.rs @@ -88,19 +88,6 @@ pub fn phase_index(&self) -> usize { } } -/// Coverage data computed by the `InstrumentCoverage` MIR pass, when compiling with -/// `-Zinstrument_coverage`. -#[derive(Clone, RustcEncodable, RustcDecodable, Debug, HashStable, TypeFoldable)] -pub struct CoverageData { - /// A hash value that can be used by the consumer of the coverage profile data to detect - /// changes to the instrumented source of the associated MIR body (typically, for an - /// individual function). - pub hash: u64, - - /// The total number of coverage region counters added to this MIR Body. - pub num_counters: u32, -} - /// The lowered representation of a single function. #[derive(Clone, RustcEncodable, RustcDecodable, Debug, HashStable, TypeFoldable)] pub struct Body<'tcx> { @@ -184,10 +171,6 @@ pub struct Body<'tcx> { /// FIXME(oli-obk): rewrite the promoted during promotion to eliminate the cell components. pub ignore_interior_mut_in_const_validation: bool, - /// If compiling with `-Zinstrument_coverage`, the `InstrumentCoverage` pass stores summary - /// information associated with the MIR, used in code generation of the coverage counters. - pub coverage_data: Option, - predecessor_cache: PredecessorCache, } @@ -228,7 +211,6 @@ pub fn new( required_consts: Vec::new(), ignore_interior_mut_in_const_validation: false, control_flow_destroyed, - coverage_data: None, predecessor_cache: PredecessorCache::new(), } } @@ -256,7 +238,6 @@ pub fn new_cfg_only(basic_blocks: IndexVec>) -> generator_kind: None, var_debug_info: Vec::new(), ignore_interior_mut_in_const_validation: false, - coverage_data: None, predecessor_cache: PredecessorCache::new(), } } @@ -2938,3 +2919,18 @@ pub fn dominates(&self, other: Location, dominators: &Dominators) -> } } } + +/// Coverage data associated with each function (MIR) instrumented with coverage counters, when +/// compiled with `-Zinstrument_coverage`. The query `tcx.coverage_data(DefId)` computes these +/// values on demand (during code generation). This query is only valid after executing the MIR pass +/// `InstrumentCoverage`. +#[derive(Clone, RustcEncodable, RustcDecodable, Debug, HashStable)] +pub struct CoverageData { + /// A hash value that can be used by the consumer of the coverage profile data to detect + /// changes to the instrumented source of the associated MIR body (typically, for an + /// individual function). + pub hash: u64, + + /// The total number of coverage region counters added to the MIR `Body`. + pub num_counters: u32, +} diff --git a/src/librustc_middle/query/mod.rs b/src/librustc_middle/query/mod.rs index 993b48afb7a9..4815f2617b69 100644 --- a/src/librustc_middle/query/mod.rs +++ b/src/librustc_middle/query/mod.rs @@ -214,7 +214,7 @@ fn describe_as_module(def_id: DefId, tcx: TyCtxt<'_>) -> String { cache_on_disk_if { key.is_local() } } - query coverage_data(key: DefId) -> Option { + query coverage_data(key: DefId) -> mir::CoverageData { desc { |tcx| "retrieving coverage data from MIR for `{}`", tcx.def_path_str(key) } storage(ArenaCacheSelector<'tcx>) cache_on_disk_if { key.is_local() } diff --git a/src/librustc_mir/transform/instrument_coverage.rs b/src/librustc_mir/transform/instrument_coverage.rs index 94aa26b3081e..8d263c3089c4 100644 --- a/src/librustc_mir/transform/instrument_coverage.rs +++ b/src/librustc_mir/transform/instrument_coverage.rs @@ -7,10 +7,12 @@ use rustc_middle::ich::StableHashingContext; use rustc_middle::mir::interpret::Scalar; use rustc_middle::mir::{ - self, BasicBlock, BasicBlockData, CoverageData, Operand, Place, SourceInfo, StatementKind, - Terminator, TerminatorKind, START_BLOCK, + self, traversal, BasicBlock, BasicBlockData, CoverageData, Operand, Place, SourceInfo, + StatementKind, Terminator, TerminatorKind, START_BLOCK, }; use rustc_middle::ty; +use rustc_middle::ty::query::Providers; +use rustc_middle::ty::FnDef; use rustc_middle::ty::TyCtxt; use rustc_span::def_id::DefId; use rustc_span::Span; @@ -19,6 +21,31 @@ /// the intrinsic llvm.instrprof.increment. pub struct InstrumentCoverage; +/// The `query` provider for `CoverageData`, requested by `codegen_intrinsic_call()` when +/// constructing the arguments for `llvm.instrprof.increment`. +pub(crate) fn provide(providers: &mut Providers<'_>) { + providers.coverage_data = |tcx, def_id| { + let body = tcx.optimized_mir(def_id); + let count_code_region_fn = + tcx.require_lang_item(lang_items::CountCodeRegionFnLangItem, None); + let mut num_counters: u32 = 0; + for (_, data) in traversal::preorder(body) { + if let Some(terminator) = &data.terminator { + if let TerminatorKind::Call { func: Operand::Constant(func), .. } = &terminator.kind + { + if let FnDef(called_fn_def_id, _) = func.literal.ty.kind { + if called_fn_def_id == count_code_region_fn { + num_counters += 1; + } + } + } + } + } + let hash = if num_counters > 0 { hash_mir_source(tcx, def_id) } else { 0 }; + CoverageData { num_counters, hash } + }; +} + struct Instrumentor<'tcx> { tcx: TyCtxt<'tcx>, num_counters: u32, @@ -30,20 +57,12 @@ fn run_pass(&self, tcx: TyCtxt<'tcx>, src: MirSource<'tcx>, mir_body: &mut mir:: // If the InstrumentCoverage pass is called on promoted MIRs, skip them. // See: https://github.com/rust-lang/rust/pull/73011#discussion_r438317601 if src.promoted.is_none() { - assert!(mir_body.coverage_data.is_none()); - - let hash = hash_mir_source(tcx, &src); - debug!( - "instrumenting {:?}, hash: {}, span: {}", + "instrumenting {:?}, span: {}", src.def_id(), - hash, tcx.sess.source_map().span_to_string(mir_body.span) ); - - let num_counters = Instrumentor::new(tcx).inject_counters(mir_body); - - mir_body.coverage_data = Some(CoverageData { hash, num_counters }); + Instrumentor::new(tcx).inject_counters(mir_body); } } } @@ -60,15 +79,13 @@ fn next_counter(&mut self) -> u32 { next } - fn inject_counters(&mut self, mir_body: &mut mir::Body<'tcx>) -> u32 { + fn inject_counters(&mut self, mir_body: &mut mir::Body<'tcx>) { // FIXME(richkadel): As a first step, counters are only injected at the top of each // function. The complete solution will inject counters at each conditional code branch. let top_of_function = START_BLOCK; let entire_function = mir_body.span; self.inject_counter(mir_body, top_of_function, entire_function); - - self.num_counters } fn inject_counter( @@ -138,14 +155,9 @@ fn placeholder_block(span: Span) -> BasicBlockData<'tcx> { } } -fn hash_mir_source<'tcx>(tcx: TyCtxt<'tcx>, src: &MirSource<'tcx>) -> u64 { - let fn_body_id = match tcx.hir().get_if_local(src.def_id()) { - Some(node) => match hir::map::associated_body(node) { - Some(body_id) => body_id, - _ => bug!("instrumented MirSource does not include a function body: {:?}", node), - }, - None => bug!("instrumented MirSource is not local: {:?}", src), - }; +fn hash_mir_source<'tcx>(tcx: TyCtxt<'tcx>, def_id: DefId) -> u64 { + let hir_node = tcx.hir().get_if_local(def_id).expect("DefId is local"); + let fn_body_id = hir::map::associated_body(hir_node).expect("HIR node is a function with body"); let hir_body = tcx.hir().body(fn_body_id); let mut hcx = tcx.create_no_span_stable_hashing_context(); hash(&mut hcx, &hir_body.value).to_smaller_hash() diff --git a/src/librustc_mir/transform/mod.rs b/src/librustc_mir/transform/mod.rs index a2fdc7efd184..8ca240d2c7da 100644 --- a/src/librustc_mir/transform/mod.rs +++ b/src/librustc_mir/transform/mod.rs @@ -6,7 +6,7 @@ use rustc_hir::intravisit::{self, NestedVisitorMap, Visitor}; use rustc_index::vec::IndexVec; use rustc_middle::mir::visit::Visitor as _; -use rustc_middle::mir::{traversal, Body, ConstQualifs, CoverageData, MirPhase, Promoted}; +use rustc_middle::mir::{traversal, Body, ConstQualifs, MirPhase, Promoted}; use rustc_middle::ty::query::Providers; use rustc_middle::ty::steal::Steal; use rustc_middle::ty::{InstanceDef, TyCtxt, TypeFoldable}; @@ -53,10 +53,10 @@ pub(crate) fn provide(providers: &mut Providers<'_>) { mir_drops_elaborated_and_const_checked, optimized_mir, is_mir_available, - coverage_data, promoted_mir, ..*providers }; + instrument_coverage::provide(providers); } fn is_mir_available(tcx: TyCtxt<'_>, def_id: DefId) -> bool { @@ -423,11 +423,6 @@ fn run_optimization_passes<'tcx>( ); } -fn coverage_data(tcx: TyCtxt<'_>, def_id: DefId) -> Option { - let body = tcx.optimized_mir(def_id); - body.coverage_data.clone() -} - fn optimized_mir(tcx: TyCtxt<'_>, def_id: DefId) -> Body<'_> { if tcx.is_constructor(def_id) { // There's no reason to run all of the MIR passes on constructors when From 3d0192e7c80b4aaa53f7609c1a73118bc91d30aa Mon Sep 17 00:00:00 2001 From: Rich Kadel Date: Mon, 22 Jun 2020 19:27:48 -0700 Subject: [PATCH 19/24] PR no longer requires u32 impl TypeFoldable --- src/librustc_middle/ty/structural_impls.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/librustc_middle/ty/structural_impls.rs b/src/librustc_middle/ty/structural_impls.rs index 463e2c57d46c..f04d31601ea5 100644 --- a/src/librustc_middle/ty/structural_impls.rs +++ b/src/librustc_middle/ty/structural_impls.rs @@ -262,7 +262,6 @@ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { bool, usize, ::rustc_target::abi::VariantIdx, - u32, u64, String, crate::middle::region::Scope, From a04514026824f9342ab93d9b608e3ec5dab53dad Mon Sep 17 00:00:00 2001 From: Rich Kadel Date: Mon, 22 Jun 2020 19:30:52 -0700 Subject: [PATCH 20/24] using "mir_body" (vs "body") in InstrumentCoverage The mod uses both MIR bodies and HIR bodies, so I'm trying to maintain consistency with these names. --- src/librustc_mir/transform/instrument_coverage.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/librustc_mir/transform/instrument_coverage.rs b/src/librustc_mir/transform/instrument_coverage.rs index 8d263c3089c4..27aaf47bbf2a 100644 --- a/src/librustc_mir/transform/instrument_coverage.rs +++ b/src/librustc_mir/transform/instrument_coverage.rs @@ -25,11 +25,11 @@ /// constructing the arguments for `llvm.instrprof.increment`. pub(crate) fn provide(providers: &mut Providers<'_>) { providers.coverage_data = |tcx, def_id| { - let body = tcx.optimized_mir(def_id); + let mir_body = tcx.optimized_mir(def_id); let count_code_region_fn = tcx.require_lang_item(lang_items::CountCodeRegionFnLangItem, None); let mut num_counters: u32 = 0; - for (_, data) in traversal::preorder(body) { + for (_, data) in traversal::preorder(mir_body) { if let Some(terminator) = &data.terminator { if let TerminatorKind::Call { func: Operand::Constant(func), .. } = &terminator.kind { From 977ce57d915914139c4aa643e63f368913e5f437 Mon Sep 17 00:00:00 2001 From: Rich Kadel Date: Mon, 22 Jun 2020 23:31:41 -0700 Subject: [PATCH 21/24] Updated query for num_counters to compute from max index Also added FIXME comments to note the possible need to accommodate counter increment calls in source-based functions that differ from the function context of the caller instance (e.g., inline functions). --- src/librustc_codegen_llvm/intrinsic.rs | 3 ++ .../transform/instrument_coverage.rs | 28 ++++++++++++++++--- 2 files changed, 27 insertions(+), 4 deletions(-) diff --git a/src/librustc_codegen_llvm/intrinsic.rs b/src/librustc_codegen_llvm/intrinsic.rs index b9193a85b1e4..dfe97b1ee2e9 100644 --- a/src/librustc_codegen_llvm/intrinsic.rs +++ b/src/librustc_codegen_llvm/intrinsic.rs @@ -140,6 +140,9 @@ fn codegen_intrinsic_call( self.call(llfn, &[], None) } "count_code_region" => { + // FIXME(richkadel): The current implementation assumes the MIR for the given + // caller_instance represents a single function. Validate and/or correct if inlining + // and/or monomorphization invalidates these assumptions. let coverage_data = tcx.coverage_data(caller_instance.def_id()); let mangled_fn = tcx.symbol_name(caller_instance); let (mangled_fn_name, _len_val) = self.const_str(mangled_fn.name); diff --git a/src/librustc_mir/transform/instrument_coverage.rs b/src/librustc_mir/transform/instrument_coverage.rs index 27aaf47bbf2a..06b648ab5a90 100644 --- a/src/librustc_mir/transform/instrument_coverage.rs +++ b/src/librustc_mir/transform/instrument_coverage.rs @@ -5,15 +5,15 @@ use rustc_hir::lang_items; use rustc_middle::hir; use rustc_middle::ich::StableHashingContext; -use rustc_middle::mir::interpret::Scalar; +use rustc_middle::mir::interpret::{ConstValue, Scalar}; use rustc_middle::mir::{ self, traversal, BasicBlock, BasicBlockData, CoverageData, Operand, Place, SourceInfo, StatementKind, Terminator, TerminatorKind, START_BLOCK, }; use rustc_middle::ty; use rustc_middle::ty::query::Providers; -use rustc_middle::ty::FnDef; use rustc_middle::ty::TyCtxt; +use rustc_middle::ty::{ConstKind, FnDef}; use rustc_span::def_id::DefId; use rustc_span::Span; @@ -26,16 +26,36 @@ pub(crate) fn provide(providers: &mut Providers<'_>) { providers.coverage_data = |tcx, def_id| { let mir_body = tcx.optimized_mir(def_id); + // FIXME(richkadel): The current implementation assumes the MIR for the given DefId + // represents a single function. Validate and/or correct if inlining and/or monomorphization + // invalidates these assumptions. let count_code_region_fn = tcx.require_lang_item(lang_items::CountCodeRegionFnLangItem, None); let mut num_counters: u32 = 0; + // The `num_counters` argument to `llvm.instrprof.increment` is the number of injected + // counters, with each counter having an index from `0..num_counters-1`. MIR optimization + // may split and duplicate some BasicBlock sequences. Simply counting the calls may not + // not work; but computing the num_counters by adding `1` to the highest index (for a given + // instrumented function) is valid. for (_, data) in traversal::preorder(mir_body) { if let Some(terminator) = &data.terminator { - if let TerminatorKind::Call { func: Operand::Constant(func), .. } = &terminator.kind + if let TerminatorKind::Call { func: Operand::Constant(func), args, .. } = + &terminator.kind { if let FnDef(called_fn_def_id, _) = func.literal.ty.kind { if called_fn_def_id == count_code_region_fn { - num_counters += 1; + if let Operand::Constant(constant) = + args.get(0).expect("count_code_region has at least one arg") + { + if let ConstKind::Value(ConstValue::Scalar(value)) = + constant.literal.val + { + let index = value + .to_u32() + .expect("count_code_region index at arg0 is u32"); + num_counters = std::cmp::max(num_counters, index + 1); + } + } } } } From 5fa8b0880825d83eb01151e43e7de1e94e05cd2d Mon Sep 17 00:00:00 2001 From: Oliver Scherer Date: Mon, 22 Jun 2020 14:03:18 +0200 Subject: [PATCH 22/24] The const propagator cannot trace references. Thus we avoid propagation of a local the moment we encounter references to it. --- src/librustc_mir/transform/const_prop.rs | 54 +++++++++---- .../32bit/rustc.main.ConstProp.diff | 18 +---- .../64bit/rustc.main.ConstProp.diff | 18 +---- src/test/mir-opt/const_prop_miscompile.rs | 22 ++++++ .../rustc.bar.ConstProp.diff | 75 +++++++++++++++++++ .../rustc.foo.ConstProp.diff | 63 ++++++++++++++++ src/test/ui/mir/mir_detects_invalid_ops.rs | 2 +- .../ui/mir/mir_detects_invalid_ops.stderr | 8 +- 8 files changed, 204 insertions(+), 56 deletions(-) create mode 100644 src/test/mir-opt/const_prop_miscompile.rs create mode 100644 src/test/mir-opt/const_prop_miscompile/rustc.bar.ConstProp.diff create mode 100644 src/test/mir-opt/const_prop_miscompile/rustc.foo.ConstProp.diff diff --git a/src/librustc_mir/transform/const_prop.rs b/src/librustc_mir/transform/const_prop.rs index 529e63ab9679..0044ed36bdf5 100644 --- a/src/librustc_mir/transform/const_prop.rs +++ b/src/librustc_mir/transform/const_prop.rs @@ -575,8 +575,16 @@ fn const_prop( } // Do not try creating references (#67862) - Rvalue::Ref(_, _, place_ref) => { - trace!("skipping Ref({:?})", place_ref); + Rvalue::AddressOf(_, place) | Rvalue::Ref(_, _, place) => { + trace!("skipping AddressOf | Ref for {:?}", place); + + // This may be creating mutable references or immutable references to cells. + // If that happens, the pointed to value could be mutated via that reference. + // Since we aren't tracking references, the const propagator loses track of what + // value the local has right now. + // Thus, all locals that have their reference taken + // must not take part in propagation. + Self::remove_const(&mut self.ecx, place.local); return None; } @@ -716,6 +724,9 @@ enum ConstPropMode { OnlyInsideOwnBlock, /// The `Local` can be propagated into but reads cannot be propagated. OnlyPropagateInto, + /// The `Local` cannot be part of propagation at all. Any statement + /// referencing it either for reading or writing will not get propagated. + NoPropagation, } struct CanConstProp { @@ -781,7 +792,9 @@ fn visit_local(&mut self, &local: &Local, context: PlaceContext, _: Location) { // end of the block anyway, and inside the block we overwrite previous // states as applicable. ConstPropMode::OnlyInsideOwnBlock => {} - other => { + ConstPropMode::NoPropagation => {} + ConstPropMode::OnlyPropagateInto => {} + other @ ConstPropMode::FullConstProp => { trace!( "local {:?} can't be propagated because of multiple assignments", local, @@ -813,7 +826,7 @@ fn visit_local(&mut self, &local: &Local, context: PlaceContext, _: Location) { | MutatingUse(MutatingUseContext::Borrow) | MutatingUse(MutatingUseContext::AddressOf) => { trace!("local {:?} can't be propagaged because it's used: {:?}", local, context); - self.can_const_prop[local] = ConstPropMode::OnlyPropagateInto; + self.can_const_prop[local] = ConstPropMode::NoPropagation; } } } @@ -858,19 +871,22 @@ fn visit_statement(&mut self, statement: &mut Statement<'tcx>, location: Locatio } } } - if can_const_prop == ConstPropMode::OnlyInsideOwnBlock { - trace!( - "found local restricted to its block. Will remove it from const-prop after block is finished. Local: {:?}", - place.local - ); - self.locals_of_current_block.insert(place.local); - } - - if can_const_prop == ConstPropMode::OnlyPropagateInto { - trace!("can't propagate into {:?}", place); - if place.local != RETURN_PLACE { - Self::remove_const(&mut self.ecx, place.local); + match can_const_prop { + ConstPropMode::OnlyInsideOwnBlock => { + trace!( + "found local restricted to its block. \ + Will remove it from const-prop after block is finished. Local: {:?}", + place.local + ); + self.locals_of_current_block.insert(place.local); } + ConstPropMode::OnlyPropagateInto | ConstPropMode::NoPropagation => { + trace!("can't propagate into {:?}", place); + if place.local != RETURN_PLACE { + Self::remove_const(&mut self.ecx, place.local); + } + } + ConstPropMode::FullConstProp => {} } } else { // Const prop failed, so erase the destination, ensuring that whatever happens @@ -890,6 +906,12 @@ fn visit_statement(&mut self, statement: &mut Statement<'tcx>, location: Locatio ); Self::remove_const(&mut self.ecx, place.local); } + } else { + trace!( + "cannot propagate into {:?}, because the type of the local is generic.", + place, + ); + Self::remove_const(&mut self.ecx, place.local); } } else { match statement.kind { diff --git a/src/test/mir-opt/const_prop/bad_op_unsafe_oob_for_slices/32bit/rustc.main.ConstProp.diff b/src/test/mir-opt/const_prop/bad_op_unsafe_oob_for_slices/32bit/rustc.main.ConstProp.diff index 7071f31dbf10..7ceec94d81e7 100644 --- a/src/test/mir-opt/const_prop/bad_op_unsafe_oob_for_slices/32bit/rustc.main.ConstProp.diff +++ b/src/test/mir-opt/const_prop/bad_op_unsafe_oob_for_slices/32bit/rustc.main.ConstProp.diff @@ -46,22 +46,8 @@ // mir::Constant // + span: $DIR/bad_op_unsafe_oob_for_slices.rs:7:23: 7:24 // + literal: Const { ty: usize, val: Value(Scalar(0x00000003)) } -- _7 = Len((*_1)); // scope 2 at $DIR/bad_op_unsafe_oob_for_slices.rs:7:18: 7:25 -- _8 = Lt(_6, _7); // scope 2 at $DIR/bad_op_unsafe_oob_for_slices.rs:7:18: 7:25 -+ _7 = const 3usize; // scope 2 at $DIR/bad_op_unsafe_oob_for_slices.rs:7:18: 7:25 -+ // ty::Const -+ // + ty: usize -+ // + val: Value(Scalar(0x00000003)) -+ // mir::Constant -+ // + span: $DIR/bad_op_unsafe_oob_for_slices.rs:7:18: 7:25 -+ // + literal: Const { ty: usize, val: Value(Scalar(0x00000003)) } -+ _8 = const false; // scope 2 at $DIR/bad_op_unsafe_oob_for_slices.rs:7:18: 7:25 -+ // ty::Const -+ // + ty: bool -+ // + val: Value(Scalar(0x00)) -+ // mir::Constant -+ // + span: $DIR/bad_op_unsafe_oob_for_slices.rs:7:18: 7:25 -+ // + literal: Const { ty: bool, val: Value(Scalar(0x00)) } + _7 = Len((*_1)); // scope 2 at $DIR/bad_op_unsafe_oob_for_slices.rs:7:18: 7:25 + _8 = Lt(_6, _7); // scope 2 at $DIR/bad_op_unsafe_oob_for_slices.rs:7:18: 7:25 assert(move _8, "index out of bounds: the len is {} but the index is {}", move _7, _6) -> bb1; // scope 2 at $DIR/bad_op_unsafe_oob_for_slices.rs:7:18: 7:25 } diff --git a/src/test/mir-opt/const_prop/bad_op_unsafe_oob_for_slices/64bit/rustc.main.ConstProp.diff b/src/test/mir-opt/const_prop/bad_op_unsafe_oob_for_slices/64bit/rustc.main.ConstProp.diff index 15995ab07001..483a6f232ef7 100644 --- a/src/test/mir-opt/const_prop/bad_op_unsafe_oob_for_slices/64bit/rustc.main.ConstProp.diff +++ b/src/test/mir-opt/const_prop/bad_op_unsafe_oob_for_slices/64bit/rustc.main.ConstProp.diff @@ -46,22 +46,8 @@ // mir::Constant // + span: $DIR/bad_op_unsafe_oob_for_slices.rs:7:23: 7:24 // + literal: Const { ty: usize, val: Value(Scalar(0x0000000000000003)) } -- _7 = Len((*_1)); // scope 2 at $DIR/bad_op_unsafe_oob_for_slices.rs:7:18: 7:25 -- _8 = Lt(_6, _7); // scope 2 at $DIR/bad_op_unsafe_oob_for_slices.rs:7:18: 7:25 -+ _7 = const 3usize; // scope 2 at $DIR/bad_op_unsafe_oob_for_slices.rs:7:18: 7:25 -+ // ty::Const -+ // + ty: usize -+ // + val: Value(Scalar(0x0000000000000003)) -+ // mir::Constant -+ // + span: $DIR/bad_op_unsafe_oob_for_slices.rs:7:18: 7:25 -+ // + literal: Const { ty: usize, val: Value(Scalar(0x0000000000000003)) } -+ _8 = const false; // scope 2 at $DIR/bad_op_unsafe_oob_for_slices.rs:7:18: 7:25 -+ // ty::Const -+ // + ty: bool -+ // + val: Value(Scalar(0x00)) -+ // mir::Constant -+ // + span: $DIR/bad_op_unsafe_oob_for_slices.rs:7:18: 7:25 -+ // + literal: Const { ty: bool, val: Value(Scalar(0x00)) } + _7 = Len((*_1)); // scope 2 at $DIR/bad_op_unsafe_oob_for_slices.rs:7:18: 7:25 + _8 = Lt(_6, _7); // scope 2 at $DIR/bad_op_unsafe_oob_for_slices.rs:7:18: 7:25 assert(move _8, "index out of bounds: the len is {} but the index is {}", move _7, _6) -> bb1; // scope 2 at $DIR/bad_op_unsafe_oob_for_slices.rs:7:18: 7:25 } diff --git a/src/test/mir-opt/const_prop_miscompile.rs b/src/test/mir-opt/const_prop_miscompile.rs new file mode 100644 index 000000000000..043b22870f49 --- /dev/null +++ b/src/test/mir-opt/const_prop_miscompile.rs @@ -0,0 +1,22 @@ +#![feature(raw_ref_op)] + +// EMIT_MIR rustc.foo.ConstProp.diff +fn foo() { + let mut u = (1,); + *&mut u.0 = 5; + let y = { u.0 } == 5; +} + +// EMIT_MIR rustc.bar.ConstProp.diff +fn bar() { + let mut v = (1,); + unsafe { + *&raw mut v.0 = 5; + } + let y = { v.0 } == 5; +} + +fn main() { + foo(); + bar(); +} diff --git a/src/test/mir-opt/const_prop_miscompile/rustc.bar.ConstProp.diff b/src/test/mir-opt/const_prop_miscompile/rustc.bar.ConstProp.diff new file mode 100644 index 000000000000..c87f67bf9f58 --- /dev/null +++ b/src/test/mir-opt/const_prop_miscompile/rustc.bar.ConstProp.diff @@ -0,0 +1,75 @@ +- // MIR for `bar` before ConstProp ++ // MIR for `bar` after ConstProp + + fn bar() -> () { + let mut _0: (); // return place in scope 0 at $DIR/const_prop_miscompile.rs:11:10: 11:10 + let mut _1: (i32,); // in scope 0 at $DIR/const_prop_miscompile.rs:12:9: 12:14 + let _2: (); // in scope 0 at $DIR/const_prop_miscompile.rs:13:5: 15:6 + let mut _3: *mut i32; // in scope 0 at $DIR/const_prop_miscompile.rs:14:10: 14:22 + let mut _5: i32; // in scope 0 at $DIR/const_prop_miscompile.rs:16:13: 16:20 + scope 1 { + debug v => _1; // in scope 1 at $DIR/const_prop_miscompile.rs:12:9: 12:14 + let _4: bool; // in scope 1 at $DIR/const_prop_miscompile.rs:16:9: 16:10 + scope 2 { + } + scope 3 { + debug y => _4; // in scope 3 at $DIR/const_prop_miscompile.rs:16:9: 16:10 + } + } + + bb0: { + StorageLive(_1); // scope 0 at $DIR/const_prop_miscompile.rs:12:9: 12:14 +- _1 = (const 1i32,); // scope 0 at $DIR/const_prop_miscompile.rs:12:17: 12:21 ++ _1 = const (1i32,); // scope 0 at $DIR/const_prop_miscompile.rs:12:17: 12:21 + // ty::Const +- // + ty: i32 ++ // + ty: (i32,) + // + val: Value(Scalar(0x00000001)) + // mir::Constant +- // + span: $DIR/const_prop_miscompile.rs:12:18: 12:19 +- // + literal: Const { ty: i32, val: Value(Scalar(0x00000001)) } ++ // + span: $DIR/const_prop_miscompile.rs:12:17: 12:21 ++ // + literal: Const { ty: (i32,), val: Value(Scalar(0x00000001)) } + StorageLive(_2); // scope 1 at $DIR/const_prop_miscompile.rs:13:5: 15:6 + StorageLive(_3); // scope 2 at $DIR/const_prop_miscompile.rs:14:10: 14:22 + _3 = &raw mut (_1.0: i32); // scope 2 at $DIR/const_prop_miscompile.rs:14:10: 14:22 + (*_3) = const 5i32; // scope 2 at $DIR/const_prop_miscompile.rs:14:9: 14:26 + // ty::Const + // + ty: i32 + // + val: Value(Scalar(0x00000005)) + // mir::Constant + // + span: $DIR/const_prop_miscompile.rs:14:25: 14:26 + // + literal: Const { ty: i32, val: Value(Scalar(0x00000005)) } + StorageDead(_3); // scope 2 at $DIR/const_prop_miscompile.rs:14:26: 14:27 + _2 = const (); // scope 2 at $DIR/const_prop_miscompile.rs:13:5: 15:6 + // ty::Const + // + ty: () + // + val: Value(Scalar()) + // mir::Constant + // + span: $DIR/const_prop_miscompile.rs:13:5: 15:6 + // + literal: Const { ty: (), val: Value(Scalar()) } + StorageDead(_2); // scope 1 at $DIR/const_prop_miscompile.rs:15:5: 15:6 + StorageLive(_4); // scope 1 at $DIR/const_prop_miscompile.rs:16:9: 16:10 + StorageLive(_5); // scope 1 at $DIR/const_prop_miscompile.rs:16:13: 16:20 + _5 = (_1.0: i32); // scope 1 at $DIR/const_prop_miscompile.rs:16:15: 16:18 + _4 = Eq(move _5, const 5i32); // scope 1 at $DIR/const_prop_miscompile.rs:16:13: 16:25 + // ty::Const + // + ty: i32 + // + val: Value(Scalar(0x00000005)) + // mir::Constant + // + span: $DIR/const_prop_miscompile.rs:16:24: 16:25 + // + literal: Const { ty: i32, val: Value(Scalar(0x00000005)) } + StorageDead(_5); // scope 1 at $DIR/const_prop_miscompile.rs:16:24: 16:25 + _0 = const (); // scope 0 at $DIR/const_prop_miscompile.rs:11:10: 17:2 + // ty::Const + // + ty: () + // + val: Value(Scalar()) + // mir::Constant + // + span: $DIR/const_prop_miscompile.rs:11:10: 17:2 + // + literal: Const { ty: (), val: Value(Scalar()) } + StorageDead(_4); // scope 1 at $DIR/const_prop_miscompile.rs:17:1: 17:2 + StorageDead(_1); // scope 0 at $DIR/const_prop_miscompile.rs:17:1: 17:2 + return; // scope 0 at $DIR/const_prop_miscompile.rs:17:2: 17:2 + } + } + diff --git a/src/test/mir-opt/const_prop_miscompile/rustc.foo.ConstProp.diff b/src/test/mir-opt/const_prop_miscompile/rustc.foo.ConstProp.diff new file mode 100644 index 000000000000..8a6850d2fe3a --- /dev/null +++ b/src/test/mir-opt/const_prop_miscompile/rustc.foo.ConstProp.diff @@ -0,0 +1,63 @@ +- // MIR for `foo` before ConstProp ++ // MIR for `foo` after ConstProp + + fn foo() -> () { + let mut _0: (); // return place in scope 0 at $DIR/const_prop_miscompile.rs:4:10: 4:10 + let mut _1: (i32,); // in scope 0 at $DIR/const_prop_miscompile.rs:5:9: 5:14 + let mut _2: &mut i32; // in scope 0 at $DIR/const_prop_miscompile.rs:6:6: 6:14 + let mut _4: i32; // in scope 0 at $DIR/const_prop_miscompile.rs:7:13: 7:20 + scope 1 { + debug u => _1; // in scope 1 at $DIR/const_prop_miscompile.rs:5:9: 5:14 + let _3: bool; // in scope 1 at $DIR/const_prop_miscompile.rs:7:9: 7:10 + scope 2 { + debug y => _3; // in scope 2 at $DIR/const_prop_miscompile.rs:7:9: 7:10 + } + } + + bb0: { + StorageLive(_1); // scope 0 at $DIR/const_prop_miscompile.rs:5:9: 5:14 +- _1 = (const 1i32,); // scope 0 at $DIR/const_prop_miscompile.rs:5:17: 5:21 ++ _1 = const (1i32,); // scope 0 at $DIR/const_prop_miscompile.rs:5:17: 5:21 + // ty::Const +- // + ty: i32 ++ // + ty: (i32,) + // + val: Value(Scalar(0x00000001)) + // mir::Constant +- // + span: $DIR/const_prop_miscompile.rs:5:18: 5:19 +- // + literal: Const { ty: i32, val: Value(Scalar(0x00000001)) } ++ // + span: $DIR/const_prop_miscompile.rs:5:17: 5:21 ++ // + literal: Const { ty: (i32,), val: Value(Scalar(0x00000001)) } + StorageLive(_2); // scope 1 at $DIR/const_prop_miscompile.rs:6:6: 6:14 + _2 = &mut (_1.0: i32); // scope 1 at $DIR/const_prop_miscompile.rs:6:6: 6:14 + (*_2) = const 5i32; // scope 1 at $DIR/const_prop_miscompile.rs:6:5: 6:18 + // ty::Const + // + ty: i32 + // + val: Value(Scalar(0x00000005)) + // mir::Constant + // + span: $DIR/const_prop_miscompile.rs:6:17: 6:18 + // + literal: Const { ty: i32, val: Value(Scalar(0x00000005)) } + StorageDead(_2); // scope 1 at $DIR/const_prop_miscompile.rs:6:18: 6:19 + StorageLive(_3); // scope 1 at $DIR/const_prop_miscompile.rs:7:9: 7:10 + StorageLive(_4); // scope 1 at $DIR/const_prop_miscompile.rs:7:13: 7:20 + _4 = (_1.0: i32); // scope 1 at $DIR/const_prop_miscompile.rs:7:15: 7:18 + _3 = Eq(move _4, const 5i32); // scope 1 at $DIR/const_prop_miscompile.rs:7:13: 7:25 + // ty::Const + // + ty: i32 + // + val: Value(Scalar(0x00000005)) + // mir::Constant + // + span: $DIR/const_prop_miscompile.rs:7:24: 7:25 + // + literal: Const { ty: i32, val: Value(Scalar(0x00000005)) } + StorageDead(_4); // scope 1 at $DIR/const_prop_miscompile.rs:7:24: 7:25 + _0 = const (); // scope 0 at $DIR/const_prop_miscompile.rs:4:10: 8:2 + // ty::Const + // + ty: () + // + val: Value(Scalar()) + // mir::Constant + // + span: $DIR/const_prop_miscompile.rs:4:10: 8:2 + // + literal: Const { ty: (), val: Value(Scalar()) } + StorageDead(_3); // scope 1 at $DIR/const_prop_miscompile.rs:8:1: 8:2 + StorageDead(_1); // scope 0 at $DIR/const_prop_miscompile.rs:8:1: 8:2 + return; // scope 0 at $DIR/const_prop_miscompile.rs:8:2: 8:2 + } + } + diff --git a/src/test/ui/mir/mir_detects_invalid_ops.rs b/src/test/ui/mir/mir_detects_invalid_ops.rs index 0940dbe6a5e8..136c03cd9f1b 100644 --- a/src/test/ui/mir/mir_detects_invalid_ops.rs +++ b/src/test/ui/mir/mir_detects_invalid_ops.rs @@ -19,6 +19,6 @@ fn mod_by_zero() { fn oob_error_for_slices() { let a: *const [_] = &[1, 2, 3]; unsafe { - let _b = (*a)[3]; //~ ERROR this operation will panic at runtime [unconditional_panic] + let _b = (*a)[3]; } } diff --git a/src/test/ui/mir/mir_detects_invalid_ops.stderr b/src/test/ui/mir/mir_detects_invalid_ops.stderr index 41f03789f237..0b6dbfd7c3d8 100644 --- a/src/test/ui/mir/mir_detects_invalid_ops.stderr +++ b/src/test/ui/mir/mir_detects_invalid_ops.stderr @@ -12,11 +12,5 @@ error: this operation will panic at runtime LL | let _z = 1 % y; | ^^^^^ attempt to calculate the remainder with a divisor of zero -error: this operation will panic at runtime - --> $DIR/mir_detects_invalid_ops.rs:22:18 - | -LL | let _b = (*a)[3]; - | ^^^^^^^ index out of bounds: the len is 3 but the index is 3 - -error: aborting due to 3 previous errors +error: aborting due to 2 previous errors From 0c2b02536c857b78cd9560e447fa669d4ca2ba3e Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Tue, 23 Jun 2020 09:41:56 -0700 Subject: [PATCH 23/24] rustc: Modernize wasm checks for atomics This commit modernizes how rustc checks for whether the `atomics` feature is enabled for the wasm target. The `sess.target_features` set is consulted instead of fiddling around with dealing with various aspects of LLVM and that syntax. --- src/librustc_codegen_llvm/back/write.rs | 3 ++- src/librustc_codegen_ssa/back/linker.rs | 5 ++--- src/librustc_span/symbol.rs | 1 + 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/librustc_codegen_llvm/back/write.rs b/src/librustc_codegen_llvm/back/write.rs index 868ce876a819..54271d3dd049 100644 --- a/src/librustc_codegen_llvm/back/write.rs +++ b/src/librustc_codegen_llvm/back/write.rs @@ -23,6 +23,7 @@ use rustc_middle::ty::TyCtxt; use rustc_session::config::{self, Lto, OutputType, Passes, SanitizerSet, SwitchWithOptPath}; use rustc_session::Session; +use rustc_span::symbol::sym; use rustc_span::InnerSpan; use rustc_target::spec::{CodeModel, RelocModel}; @@ -140,7 +141,7 @@ pub fn target_machine_factory( // lower atomic operations to single-threaded operations. if singlethread && sess.target.target.llvm_target.contains("wasm32") - && features.iter().any(|s| *s == "+atomics") + && sess.target_features.contains(&sym::atomics) { singlethread = false; } diff --git a/src/librustc_codegen_ssa/back/linker.rs b/src/librustc_codegen_ssa/back/linker.rs index 6011d422ca68..d6ef94bfd172 100644 --- a/src/librustc_codegen_ssa/back/linker.rs +++ b/src/librustc_codegen_ssa/back/linker.rs @@ -1,6 +1,7 @@ use super::archive; use super::command::Command; use super::symbol_export; +use rustc_span::symbol::sym; use std::ffi::{OsStr, OsString}; use std::fs::{self, File}; @@ -1036,9 +1037,7 @@ fn new(mut cmd: Command, sess: &'a Session, info: &'a LinkerInfo) -> WasmLd<'a> // // * `--export=*tls*` - when `#[thread_local]` symbols are used these // symbols are how the TLS segments are initialized and configured. - let atomics = sess.opts.cg.target_feature.contains("+atomics") - || sess.target.target.options.features.contains("+atomics"); - if atomics { + if sess.target_features.contains(&sym::atomics) { cmd.arg("--shared-memory"); cmd.arg("--max-memory=1073741824"); cmd.arg("--import-memory"); diff --git a/src/librustc_span/symbol.rs b/src/librustc_span/symbol.rs index 06d1f36622b9..6dab25dccad4 100644 --- a/src/librustc_span/symbol.rs +++ b/src/librustc_span/symbol.rs @@ -159,6 +159,7 @@ assume_init, async_await, async_closure, + atomics, attr, attributes, attr_literals, From 6e8aa1ff27297694e5092f19f5e2d6d244d076a7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Tue, 23 Jun 2020 13:01:24 -0700 Subject: [PATCH 24/24] review comments: wording and style --- src/librustc_typeck/check/mod.rs | 29 ++++++++++++----------------- src/test/ui/fn/fn-item-type.rs | 20 ++++++++++---------- src/test/ui/fn/fn-item-type.stderr | 20 ++++++++++---------- 3 files changed, 32 insertions(+), 37 deletions(-) diff --git a/src/librustc_typeck/check/mod.rs b/src/librustc_typeck/check/mod.rs index 234a573b725e..761807213d2a 100644 --- a/src/librustc_typeck/check/mod.rs +++ b/src/librustc_typeck/check/mod.rs @@ -5502,7 +5502,7 @@ fn note_need_for_fn_pointer( expected: Ty<'tcx>, found: Ty<'tcx>, ) { - match (&expected.kind, &found.kind) { + let (sig, did, substs) = match (&expected.kind, &found.kind) { (ty::FnDef(did1, substs1), ty::FnDef(did2, substs2)) => { let sig1 = self.tcx.fn_sig(*did1).subst(self.tcx, substs1); let sig2 = self.tcx.fn_sig(*did2).subst(self.tcx, substs2); @@ -5513,29 +5513,24 @@ fn note_need_for_fn_pointer( "different `fn` items always have unique types, even if their signatures are \ the same", ); - err.help(&format!("change the expectation to require function pointer `{}`", sig1)); - err.help(&format!( - "if the expectation is due to type inference, cast the expected `fn` to a \ - function pointer: `{} as {}`", - self.tcx.def_path_str_with_substs(*did1, substs1), - sig1 - )); + (sig1, *did1, substs1) } (ty::FnDef(did, substs), ty::FnPtr(sig2)) => { let sig1 = self.tcx.fn_sig(*did).subst(self.tcx, substs); if sig1 != *sig2 { return; } - err.help(&format!("change the expectation to require function pointer `{}`", sig1)); - err.help(&format!( - "if the expectation is due to type inference, cast the expected `fn` to a \ - function pointer: `{} as {}`", - self.tcx.def_path_str_with_substs(*did, substs), - sig1 - )); + (sig1, *did, substs) } - _ => {} - } + _ => return, + }; + err.help(&format!("change the expected type to be function pointer `{}`", sig)); + err.help(&format!( + "if the expected type is due to type inference, cast the expected `fn` to a function \ + pointer: `{} as {}`", + self.tcx.def_path_str_with_substs(did, substs), + sig + )); } /// A common error is to add an extra semicolon: diff --git a/src/test/ui/fn/fn-item-type.rs b/src/test/ui/fn/fn-item-type.rs index 256b9d45755c..abae40162a0f 100644 --- a/src/test/ui/fn/fn-item-type.rs +++ b/src/test/ui/fn/fn-item-type.rs @@ -16,15 +16,15 @@ fn main() { //~| found fn item `fn(_) -> _ {bar::}` //~| expected fn item, found a different fn item //~| different `fn` items always have unique types, even if their signatures are the same - //~| change the expectation to require function pointer - //~| if the expectation is due to type inference, cast the expected `fn` to a function pointer + //~| change the expected type to be function pointer + //~| if the expected type is due to type inference, cast the expected `fn` to a function pointer eq(foo::, foo::); //~^ ERROR mismatched types //~| expected `u8`, found `i8` //~| different `fn` items always have unique types, even if their signatures are the same - //~| change the expectation to require function pointer - //~| if the expectation is due to type inference, cast the expected `fn` to a function pointer + //~| change the expected type to be function pointer + //~| if the expected type is due to type inference, cast the expected `fn` to a function pointer eq(bar::, bar::>); //~^ ERROR mismatched types @@ -32,24 +32,24 @@ fn main() { //~| found fn item `fn(_) -> _ {bar::>}` //~| expected struct `std::string::String`, found struct `std::vec::Vec` //~| different `fn` items always have unique types, even if their signatures are the same - //~| change the expectation to require function pointer - //~| if the expectation is due to type inference, cast the expected `fn` to a function pointer + //~| change the expected type to be function pointer + //~| if the expected type is due to type inference, cast the expected `fn` to a function pointer // Make sure we distinguish between trait methods correctly. eq(::foo, ::foo); //~^ ERROR mismatched types //~| expected `u8`, found `u16` //~| different `fn` items always have unique types, even if their signatures are the same - //~| change the expectation to require function pointer - //~| if the expectation is due to type inference, cast the expected `fn` to a function pointer + //~| change the expected type to be function pointer + //~| if the expected type is due to type inference, cast the expected `fn` to a function pointer eq(foo::, bar:: as fn(isize) -> isize); //~^ ERROR mismatched types //~| expected fn item `fn(_) -> _ {foo::}` //~| found fn pointer `fn(_) -> _` //~| expected fn item, found fn pointer - //~| change the expectation to require function pointer - //~| if the expectation is due to type inference, cast the expected `fn` to a function pointer + //~| change the expected type to be function pointer + //~| if the expected type is due to type inference, cast the expected `fn` to a function pointer eq(foo:: as fn(isize) -> isize, bar::); // ok! } diff --git a/src/test/ui/fn/fn-item-type.stderr b/src/test/ui/fn/fn-item-type.stderr index 84f5e034340e..bfa9efa219f4 100644 --- a/src/test/ui/fn/fn-item-type.stderr +++ b/src/test/ui/fn/fn-item-type.stderr @@ -7,8 +7,8 @@ LL | eq(foo::, bar::); = note: expected fn item `fn(_) -> _ {foo::}` found fn item `fn(_) -> _ {bar::}` = note: different `fn` items always have unique types, even if their signatures are the same - = help: change the expectation to require function pointer `fn(isize) -> isize` - = help: if the expectation is due to type inference, cast the expected `fn` to a function pointer: `foo:: as fn(isize) -> isize` + = help: change the expected type to be function pointer `fn(isize) -> isize` + = help: if the expected type is due to type inference, cast the expected `fn` to a function pointer: `foo:: as fn(isize) -> isize` error[E0308]: mismatched types --> $DIR/fn-item-type.rs:22:19 @@ -19,8 +19,8 @@ LL | eq(foo::, foo::); = note: expected fn item `fn(_) -> _ {foo::}` found fn item `fn(_) -> _ {foo::}` = note: different `fn` items always have unique types, even if their signatures are the same - = help: change the expectation to require function pointer `fn(isize) -> isize` - = help: if the expectation is due to type inference, cast the expected `fn` to a function pointer: `foo:: as fn(isize) -> isize` + = help: change the expected type to be function pointer `fn(isize) -> isize` + = help: if the expected type is due to type inference, cast the expected `fn` to a function pointer: `foo:: as fn(isize) -> isize` error[E0308]: mismatched types --> $DIR/fn-item-type.rs:29:23 @@ -31,8 +31,8 @@ LL | eq(bar::, bar::>); = note: expected fn item `fn(_) -> _ {bar::}` found fn item `fn(_) -> _ {bar::>}` = note: different `fn` items always have unique types, even if their signatures are the same - = help: change the expectation to require function pointer `fn(isize) -> isize` - = help: if the expectation is due to type inference, cast the expected `fn` to a function pointer: `bar:: as fn(isize) -> isize` + = help: change the expected type to be function pointer `fn(isize) -> isize` + = help: if the expected type is due to type inference, cast the expected `fn` to a function pointer: `bar:: as fn(isize) -> isize` error[E0308]: mismatched types --> $DIR/fn-item-type.rs:39:26 @@ -43,8 +43,8 @@ LL | eq(::foo, ::foo); = note: expected fn item `fn() {::foo}` found fn item `fn() {::foo}` = note: different `fn` items always have unique types, even if their signatures are the same - = help: change the expectation to require function pointer `fn()` - = help: if the expectation is due to type inference, cast the expected `fn` to a function pointer: `::foo as fn()` + = help: change the expected type to be function pointer `fn()` + = help: if the expected type is due to type inference, cast the expected `fn` to a function pointer: `::foo as fn()` error[E0308]: mismatched types --> $DIR/fn-item-type.rs:46:19 @@ -54,8 +54,8 @@ LL | eq(foo::, bar:: as fn(isize) -> isize); | = note: expected fn item `fn(_) -> _ {foo::}` found fn pointer `fn(_) -> _` - = help: change the expectation to require function pointer `fn(isize) -> isize` - = help: if the expectation is due to type inference, cast the expected `fn` to a function pointer: `foo:: as fn(isize) -> isize` + = help: change the expected type to be function pointer `fn(isize) -> isize` + = help: if the expected type is due to type inference, cast the expected `fn` to a function pointer: `foo:: as fn(isize) -> isize` error: aborting due to 5 previous errors