Auto merge of #46896 - arielb1:shadow-scope, r=eddyb

fix debuginfo scoping of let-statements

r? @eddyb

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 });

src/librustc/mir/mod.rs

@@ -481,11 +481,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> {
@@ -500,7 +572,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
         }
@@ -517,7 +589,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
         }
@@ -535,7 +607,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

src/librustc/mir/visit.rs

@@ -703,7 +703,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;
 
@@ -712,7 +712,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,

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
                     });

src/librustc_mir/build/matches/mod.rs

@@ -196,16 +196,18 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
                             pattern: &Pattern<'tcx>)
                             -> Option<VisibilityScope> {
         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,
         });

src/librustc_mir/build/mod.rs

@@ -435,6 +435,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| {
@@ -604,7 +607,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,

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
     }

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,
     };

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() {()}