From 9be593032d24758f2c80df3e85d27f10bf4127bb Mon Sep 17 00:00:00 2001 From: Ariel Ben-Yehuda Date: Thu, 21 Dec 2017 00:35:19 +0200 Subject: [PATCH] fix debuginfo scoping of let-statements --- src/librustc/ich/impls_mir.rs | 2 +- src/librustc/mir/mod.rs | 82 +++++++++++++++++++++++-- src/librustc/mir/visit.rs | 4 +- src/librustc_mir/build/expr/into.rs | 2 +- src/librustc_mir/build/matches/mod.rs | 22 +++---- src/librustc_mir/build/mod.rs | 5 +- src/librustc_mir/shim.rs | 2 +- src/librustc_mir/transform/generator.rs | 6 +- src/test/debuginfo/shadowed-variable.rs | 32 ++++++++++ 9 files changed, 132 insertions(+), 25 deletions(-) diff --git a/src/librustc/ich/impls_mir.rs b/src/librustc/ich/impls_mir.rs index df67c3abbe86c..1e486106f261d 100644 --- a/src/librustc/ich/impls_mir.rs +++ b/src/librustc/ich/impls_mir.rs @@ -28,7 +28,7 @@ impl_stable_hash_for!(struct mir::LocalDecl<'tcx> { name, source_info, internal, - lexical_scope, + syntactic_scope, is_user_variable }); impl_stable_hash_for!(struct mir::UpvarDecl { debug_name, by_ref, mutability }); diff --git a/src/librustc/mir/mod.rs b/src/librustc/mir/mod.rs index efde224c3e120..1a62036ae687a 100644 --- a/src/librustc/mir/mod.rs +++ b/src/librustc/mir/mod.rs @@ -480,11 +480,83 @@ pub struct LocalDecl<'tcx> { /// Source info of the local. pub source_info: SourceInfo, - /// The *lexical* visibility scope the local is defined + /// The *syntactic* visibility scope the local is defined /// in. If the local was defined in a let-statement, this /// is *within* the let-statement, rather than outside /// of it. - pub lexical_scope: VisibilityScope, + /// + /// This is needed because visibility scope of locals within a let-statement + /// is weird. + /// + /// The reason is that we want the local to be *within* the let-statement + /// for lint purposes, but we want the local to be *after* the let-statement + /// for names-in-scope purposes. + /// + /// That's it, if we have a let-statement like the one in this + /// function: + /// ``` + /// fn foo(x: &str) { + /// #[allow(unused_mut)] + /// let mut x: u32 = { // <- one unused mut + /// let mut y: u32 = x.parse().unwrap(); + /// y + 2 + /// }; + /// drop(x); + /// } + /// ``` + /// + /// Then, from a lint point of view, the declaration of `x: u32` + /// (and `y: u32`) are within the `#[allow(unused_mut)]` scope - the + /// lint scopes are the same as the AST/HIR nesting. + /// + /// However, from a name lookup point of view, the scopes look more like + /// as if the let-statements were `match` expressions: + /// + /// ``` + /// fn foo(x: &str) { + /// match { + /// match x.parse().unwrap() { + /// y => y + 2 + /// } + /// } { + /// x => drop(x) + /// }; + /// } + /// ``` + /// + /// We care about the name-lookup scopes for debuginfo - if the + /// debuginfo instruction pointer is at the call to `x.parse()`, we + /// want `x` to refer to `x: &str`, but if it is at the call to + /// `drop(x)`, we want it to refer to `x: u32`. + /// + /// To allow both uses to work, we need to have more than a single scope + /// for a local. We have the `syntactic_scope` represent the + /// "syntactic" lint scope (with a variable being under its let + /// block) while the source-info scope represents the "local variable" + /// scope (where the "rest" of a block is under all prior let-statements). + /// + /// The end result looks like this: + /// + /// ROOT SCOPE + /// │{ argument x: &str } + /// │ + /// │ │{ #[allow(unused_mut] } // this is actually split into 2 scopes + /// │ │ // in practice because I'm lazy. + /// │ │ + /// │ │← x.syntactic_scope + /// │ │← `x.parse().unwrap()` + /// │ │ + /// │ │ │← y.syntactic_scope + /// │ │ + /// │ │ │{ let y: u32 } + /// │ │ │ + /// │ │ │← y.source_info.scope + /// │ │ │← `y + 2` + /// │ + /// │ │{ let x: u32 } + /// │ │← x.source_info.scope + /// │ │← `drop(x)` // this accesses `x: u32` + pub syntactic_scope: VisibilityScope, } impl<'tcx> LocalDecl<'tcx> { @@ -499,7 +571,7 @@ impl<'tcx> LocalDecl<'tcx> { span, scope: ARGUMENT_VISIBILITY_SCOPE }, - lexical_scope: ARGUMENT_VISIBILITY_SCOPE, + syntactic_scope: ARGUMENT_VISIBILITY_SCOPE, internal: false, is_user_variable: false } @@ -516,7 +588,7 @@ impl<'tcx> LocalDecl<'tcx> { span, scope: ARGUMENT_VISIBILITY_SCOPE }, - lexical_scope: ARGUMENT_VISIBILITY_SCOPE, + syntactic_scope: ARGUMENT_VISIBILITY_SCOPE, internal: true, is_user_variable: false } @@ -534,7 +606,7 @@ impl<'tcx> LocalDecl<'tcx> { span, scope: ARGUMENT_VISIBILITY_SCOPE }, - lexical_scope: ARGUMENT_VISIBILITY_SCOPE, + syntactic_scope: ARGUMENT_VISIBILITY_SCOPE, internal: false, name: None, // FIXME maybe we do want some name here? is_user_variable: false diff --git a/src/librustc/mir/visit.rs b/src/librustc/mir/visit.rs index a50a9c819f6ec..e2f1c1c13b054 100644 --- a/src/librustc/mir/visit.rs +++ b/src/librustc/mir/visit.rs @@ -702,7 +702,7 @@ macro_rules! make_mir_visitor { name: _, ref $($mutability)* source_info, internal: _, - ref $($mutability)* lexical_scope, + ref $($mutability)* syntactic_scope, is_user_variable: _, } = *local_decl; @@ -711,7 +711,7 @@ macro_rules! make_mir_visitor { source_info: *source_info, }); self.visit_source_info(source_info); - self.visit_visibility_scope(lexical_scope); + self.visit_visibility_scope(syntactic_scope); } fn super_visibility_scope(&mut self, diff --git a/src/librustc_mir/build/expr/into.rs b/src/librustc_mir/build/expr/into.rs index ed339110537f6..3e0ccc7d07260 100644 --- a/src/librustc_mir/build/expr/into.rs +++ b/src/librustc_mir/build/expr/into.rs @@ -237,7 +237,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { ty: ptr_ty, name: None, source_info, - lexical_scope: source_info.scope, + syntactic_scope: source_info.scope, internal: true, is_user_variable: false }); diff --git a/src/librustc_mir/build/matches/mod.rs b/src/librustc_mir/build/matches/mod.rs index 23095bc4269b5..6cb9217776648 100644 --- a/src/librustc_mir/build/matches/mod.rs +++ b/src/librustc_mir/build/matches/mod.rs @@ -196,16 +196,18 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { pattern: &Pattern<'tcx>) -> Option { assert!(!(var_scope.is_some() && lint_level.is_explicit()), - "can't have both a var and a lint scope at the same time"); + "can't have both a var and a lint scope at the same time"); + let mut syntactic_scope = self.visibility_scope; self.visit_bindings(pattern, &mut |this, mutability, name, var, span, ty| { if var_scope.is_none() { var_scope = Some(this.new_visibility_scope(scope_span, LintLevel::Inherited, None)); // If we have lints, create a new visibility scope - // that marks the lints for the locals. + // that marks the lints for the locals. See the comment + // on the `syntactic_scope` field for why this is needed. if lint_level.is_explicit() { - this.visibility_scope = + syntactic_scope = this.new_visibility_scope(scope_span, lint_level, None); } } @@ -213,12 +215,8 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { span, scope: var_scope.unwrap() }; - this.declare_binding(source_info, mutability, name, var, ty); + this.declare_binding(source_info, syntactic_scope, mutability, name, var, ty); }); - // Pop any scope we created for the locals. - if let Some(var_scope) = var_scope { - self.visibility_scope = var_scope; - } var_scope } @@ -783,21 +781,23 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { fn declare_binding(&mut self, source_info: SourceInfo, + syntactic_scope: VisibilityScope, mutability: Mutability, name: Name, var_id: NodeId, var_ty: Ty<'tcx>) -> Local { - debug!("declare_binding(var_id={:?}, name={:?}, var_ty={:?}, source_info={:?})", - var_id, name, var_ty, source_info); + debug!("declare_binding(var_id={:?}, name={:?}, var_ty={:?}, source_info={:?}, \ + syntactic_scope={:?})", + var_id, name, var_ty, source_info, syntactic_scope); let var = self.local_decls.push(LocalDecl::<'tcx> { mutability, ty: var_ty.clone(), name: Some(name), source_info, - lexical_scope: self.visibility_scope, + syntactic_scope, internal: false, is_user_variable: true, }); diff --git a/src/librustc_mir/build/mod.rs b/src/librustc_mir/build/mod.rs index d814b092c9d69..011880f0ca94f 100644 --- a/src/librustc_mir/build/mod.rs +++ b/src/librustc_mir/build/mod.rs @@ -409,6 +409,9 @@ fn construct_fn<'a, 'gcx, 'tcx, A>(hir: Cx<'a, 'gcx, 'tcx>, // RustCall pseudo-ABI untuples the last argument. spread_arg = Some(Local::new(arguments.len())); } + let closure_expr_id = tcx.hir.local_def_id(fn_id); + info!("fn_id {:?} has attrs {:?}", closure_expr_id, + tcx.get_attrs(closure_expr_id)); // Gather the upvars of a closure, if any. let upvar_decls: Vec<_> = tcx.with_freevars(fn_id, |freevars| { @@ -571,7 +574,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { scope: ARGUMENT_VISIBILITY_SCOPE, span: pattern.map_or(self.fn_span, |pat| pat.span) }, - lexical_scope: ARGUMENT_VISIBILITY_SCOPE, + syntactic_scope: ARGUMENT_VISIBILITY_SCOPE, name, internal: false, is_user_variable: false, diff --git a/src/librustc_mir/shim.rs b/src/librustc_mir/shim.rs index 46193dedf8968..89e3e7e0b6027 100644 --- a/src/librustc_mir/shim.rs +++ b/src/librustc_mir/shim.rs @@ -142,7 +142,7 @@ fn temp_decl(mutability: Mutability, ty: Ty, span: Span) -> LocalDecl { LocalDecl { mutability, ty, name: None, source_info: SourceInfo { scope: ARGUMENT_VISIBILITY_SCOPE, span }, - lexical_scope: ARGUMENT_VISIBILITY_SCOPE, + syntactic_scope: ARGUMENT_VISIBILITY_SCOPE, internal: false, is_user_variable: false } diff --git a/src/librustc_mir/transform/generator.rs b/src/librustc_mir/transform/generator.rs index 455a07c04cfc0..6be5b21cae6aa 100644 --- a/src/librustc_mir/transform/generator.rs +++ b/src/librustc_mir/transform/generator.rs @@ -301,7 +301,7 @@ fn replace_result_variable<'tcx>(ret_ty: Ty<'tcx>, ty: ret_ty, name: None, source_info: source_info(mir), - lexical_scope: ARGUMENT_VISIBILITY_SCOPE, + syntactic_scope: ARGUMENT_VISIBILITY_SCOPE, internal: false, is_user_variable: false, }; @@ -562,7 +562,7 @@ fn create_generator_drop_shim<'a, 'tcx>( ty: tcx.mk_nil(), name: None, source_info, - lexical_scope: ARGUMENT_VISIBILITY_SCOPE, + syntactic_scope: ARGUMENT_VISIBILITY_SCOPE, internal: false, is_user_variable: false, }; @@ -578,7 +578,7 @@ fn create_generator_drop_shim<'a, 'tcx>( }), name: None, source_info, - lexical_scope: ARGUMENT_VISIBILITY_SCOPE, + syntactic_scope: ARGUMENT_VISIBILITY_SCOPE, internal: false, is_user_variable: false, }; diff --git a/src/test/debuginfo/shadowed-variable.rs b/src/test/debuginfo/shadowed-variable.rs index 4883853f72fce..bf47ebe5aa7d1 100644 --- a/src/test/debuginfo/shadowed-variable.rs +++ b/src/test/debuginfo/shadowed-variable.rs @@ -34,6 +34,17 @@ // gdb-check:$6 = 20 // gdb-command:continue +// gdb-command:print x +// gdb-check:$5 = 10.5 +// gdb-command:print y +// gdb-check:$6 = 20 +// gdb-command:continue + +// gdb-command:print x +// gdb-check:$5 = 11.5 +// gdb-command:print y +// gdb-check:$6 = 20 +// gdb-command:continue // === LLDB TESTS ================================================================================== @@ -57,6 +68,18 @@ // lldb-check:[...]$5 = 20 // lldb-command:continue +// lldb-command:print x +// lldb-check:[...]$4 = 10.5 +// lldb-command:print y +// lldb-check:[...]$5 = 20 +// lldb-command:continue + +// lldb-command:print x +// lldb-check:[...]$4 = 11.5 +// lldb-command:print y +// lldb-check:[...]$5 = 20 +// lldb-command:continue + #![feature(omit_gdb_pretty_printer_section)] #![omit_gdb_pretty_printer_section] @@ -77,6 +100,15 @@ fn main() { zzz(); // #break sentinel(); + + let x = { + zzz(); // #break + sentinel(); + 11.5 + }; + + zzz(); // #break + sentinel() } fn zzz() {()}