From cc36f88ed4ac59a5d98cf2493072faf9dbe216b5 Mon Sep 17 00:00:00 2001
From: Shotaro Yamada <sinkuu@sinkuu.xyz>
Date: Fri, 10 Nov 2017 23:06:06 +0900
Subject: [PATCH 1/4] Handle closures correctly in MIR inlining

---
 src/librustc_mir/transform/inline.rs | 66 +++++++++++++++++++++++-----
 src/test/mir-opt/inline-closure.rs   | 53 ++++++++++++++++++++++
 2 files changed, 109 insertions(+), 10 deletions(-)
 create mode 100644 src/test/mir-opt/inline-closure.rs

diff --git a/src/librustc_mir/transform/inline.rs b/src/librustc_mir/transform/inline.rs
index ed809836c4f..1e308ccdc94 100644
--- a/src/librustc_mir/transform/inline.rs
+++ b/src/librustc_mir/transform/inline.rs
@@ -20,6 +20,7 @@ use rustc::mir::transform::{MirPass, MirSource};
 use rustc::mir::visit::*;
 use rustc::ty::{self, Instance, Ty, TyCtxt, TypeFoldable};
 use rustc::ty::subst::{Subst,Substs};
+use rustc::hir::map::definitions::DefPathData;
 
 use std::collections::VecDeque;
 use super::simplify::{remove_dead_blocks, CfgSimplifier};
@@ -550,22 +551,31 @@ impl<'a, 'tcx> Inliner<'a, 'tcx> {
         Operand::Consume(cast_tmp)
     }
 
-    fn make_call_args(&self, args: Vec<Operand<'tcx>>,
-                      callsite: &CallSite<'tcx>, caller_mir: &mut Mir<'tcx>) -> Vec<Operand<'tcx>> {
-        let tcx = self.tcx;
+    fn make_call_args(
+        &self,
+        args: Vec<Operand<'tcx>>,
+        callsite: &CallSite<'tcx>,
+        caller_mir: &mut Mir<'tcx>,
+    ) -> Vec<Operand<'tcx>> {
         // FIXME: Analysis of the usage of the arguments to avoid
         // unnecessary temporaries.
-        args.into_iter().map(|a| {
-            if let Operand::Consume(Lvalue::Local(local)) = a {
+
+        fn create_temp_if_necessary<'a, 'tcx: 'a>(
+            arg: Operand<'tcx>,
+            tcx: TyCtxt<'a, 'tcx, 'tcx>,
+            callsite: &CallSite<'tcx>,
+            caller_mir: &mut Mir<'tcx>,
+        ) -> Operand<'tcx> {
+            if let Operand::Consume(Lvalue::Local(local)) = arg {
                 if caller_mir.local_kind(local) == LocalKind::Temp {
                     // Reuse the operand if it's a temporary already
-                    return a;
+                    return arg;
                 }
             }
 
-            debug!("Creating temp for argument");
+            debug!("Creating temp for argument {:?}", arg);
             // Otherwise, create a temporary for the arg
-            let arg = Rvalue::Use(a);
+            let arg = Rvalue::Use(arg);
 
             let ty = arg.ty(caller_mir, tcx);
 
@@ -575,11 +585,47 @@ impl<'a, 'tcx> Inliner<'a, 'tcx> {
 
             let stmt = Statement {
                 source_info: callsite.location,
-                kind: StatementKind::Assign(arg_tmp.clone(), arg)
+                kind: StatementKind::Assign(arg_tmp.clone(), arg),
             };
             caller_mir[callsite.bb].statements.push(stmt);
             Operand::Consume(arg_tmp)
-        }).collect()
+        }
+
+        let tcx = self.tcx;
+
+        // A closure is passed its self-type and a tuple like `(arg1, arg2, ...)`,
+        // hence mappings to tuple fields are needed.
+        if tcx.def_key(callsite.callee).disambiguated_data.data == DefPathData::ClosureExpr {
+            let mut args = args.into_iter();
+
+            let self_ = create_temp_if_necessary(args.next().unwrap(), tcx, callsite, caller_mir);
+
+            let tuple = if let Operand::Consume(lvalue) =
+                create_temp_if_necessary(args.next().unwrap(), tcx, callsite, caller_mir)
+            {
+                lvalue
+            } else {
+                unreachable!()
+            };
+            assert!(args.next().is_none());
+
+            let tuple_tys = if let ty::TyTuple(s, _) = tuple.ty(caller_mir, tcx).to_ty(tcx).sty {
+                s
+            } else {
+                bug!("Closure arguments are not passed as a tuple");
+            };
+
+            let mut res = Vec::with_capacity(1 + tuple_tys.len());
+            res.push(self_);
+            res.extend(tuple_tys.iter().enumerate().map(|(i, ty)| {
+                Operand::Consume(tuple.clone().field(Field::new(i), ty))
+            }));
+            res
+        } else {
+            args.into_iter()
+                .map(|a| create_temp_if_necessary(a, tcx, callsite, caller_mir))
+                .collect()
+        }
     }
 }
 
diff --git a/src/test/mir-opt/inline-closure.rs b/src/test/mir-opt/inline-closure.rs
new file mode 100644
index 00000000000..bf5761ba5b7
--- /dev/null
+++ b/src/test/mir-opt/inline-closure.rs
@@ -0,0 +1,53 @@
+// Copyright 2017 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+// compile-flags: -Z span_free_formats
+
+// Tests that MIR inliner can handle closure arguments. (#45894)
+
+fn main() {
+    println!("{}", foo(0, 14));
+}
+
+fn foo<T: Copy>(_t: T, q: i32) -> i32 {
+    let x = |_t, _q| _t;
+    x(q*2, q*3)
+}
+
+// END RUST SOURCE
+// START rustc.foo.Inline.after.mir
+// ...
+// bb0: {
+//     StorageLive(_3);
+//     _3 = [closure@NodeId(28)];
+//     StorageLive(_4);
+//     _4 = &_3;
+//     StorageLive(_5);
+//     StorageLive(_6);
+//     StorageLive(_7);
+//     _7 = _2;
+//     _6 = Mul(_7, const 2i32);
+//     StorageDead(_7);
+//     StorageLive(_8);
+//     StorageLive(_9);
+//     _9 = _2;
+//     _8 = Mul(_9, const 3i32);
+//     StorageDead(_9);
+//     _5 = (_6, _8);
+//     _0 = (_5.0: i32);
+//     StorageDead(_5);
+//     StorageDead(_8);
+//     StorageDead(_6);
+//     StorageDead(_4);
+//     StorageDead(_3);
+//     return;
+// }
+// ...
+// END rustc.foo.Inline.after.mir
\ No newline at end of file

From c3ec1758571fdf75a4970f9f130b3b83d451ff62 Mon Sep 17 00:00:00 2001
From: Shotaro Yamada <sinkuu@sinkuu.xyz>
Date: Sat, 11 Nov 2017 02:05:29 +0900
Subject: [PATCH 2/4] Make create_temp_necessary a method

---
 src/librustc_mir/transform/inline.rs | 86 +++++++++++++---------------
 1 file changed, 40 insertions(+), 46 deletions(-)

diff --git a/src/librustc_mir/transform/inline.rs b/src/librustc_mir/transform/inline.rs
index 1e308ccdc94..460af371e04 100644
--- a/src/librustc_mir/transform/inline.rs
+++ b/src/librustc_mir/transform/inline.rs
@@ -557,56 +557,14 @@ impl<'a, 'tcx> Inliner<'a, 'tcx> {
         callsite: &CallSite<'tcx>,
         caller_mir: &mut Mir<'tcx>,
     ) -> Vec<Operand<'tcx>> {
-        // FIXME: Analysis of the usage of the arguments to avoid
-        // unnecessary temporaries.
-
-        fn create_temp_if_necessary<'a, 'tcx: 'a>(
-            arg: Operand<'tcx>,
-            tcx: TyCtxt<'a, 'tcx, 'tcx>,
-            callsite: &CallSite<'tcx>,
-            caller_mir: &mut Mir<'tcx>,
-        ) -> Operand<'tcx> {
-            if let Operand::Consume(Lvalue::Local(local)) = arg {
-                if caller_mir.local_kind(local) == LocalKind::Temp {
-                    // Reuse the operand if it's a temporary already
-                    return arg;
-                }
-            }
-
-            debug!("Creating temp for argument {:?}", arg);
-            // Otherwise, create a temporary for the arg
-            let arg = Rvalue::Use(arg);
-
-            let ty = arg.ty(caller_mir, tcx);
-
-            let arg_tmp = LocalDecl::new_temp(ty, callsite.location.span);
-            let arg_tmp = caller_mir.local_decls.push(arg_tmp);
-            let arg_tmp = Lvalue::Local(arg_tmp);
-
-            let stmt = Statement {
-                source_info: callsite.location,
-                kind: StatementKind::Assign(arg_tmp.clone(), arg),
-            };
-            caller_mir[callsite.bb].statements.push(stmt);
-            Operand::Consume(arg_tmp)
-        }
-
         let tcx = self.tcx;
 
         // A closure is passed its self-type and a tuple like `(arg1, arg2, ...)`,
         // hence mappings to tuple fields are needed.
         if tcx.def_key(callsite.callee).disambiguated_data.data == DefPathData::ClosureExpr {
             let mut args = args.into_iter();
-
-            let self_ = create_temp_if_necessary(args.next().unwrap(), tcx, callsite, caller_mir);
-
-            let tuple = if let Operand::Consume(lvalue) =
-                create_temp_if_necessary(args.next().unwrap(), tcx, callsite, caller_mir)
-            {
-                lvalue
-            } else {
-                unreachable!()
-            };
+            let self_ = self.create_temp_if_necessary(args.next().unwrap(), callsite, caller_mir);
+            let tuple = self.create_temp_if_necessary(args.next().unwrap(), callsite, caller_mir);
             assert!(args.next().is_none());
 
             let tuple_tys = if let ty::TyTuple(s, _) = tuple.ty(caller_mir, tcx).to_ty(tcx).sty {
@@ -616,17 +574,53 @@ impl<'a, 'tcx> Inliner<'a, 'tcx> {
             };
 
             let mut res = Vec::with_capacity(1 + tuple_tys.len());
-            res.push(self_);
+            res.push(Operand::Consume(self_));
             res.extend(tuple_tys.iter().enumerate().map(|(i, ty)| {
                 Operand::Consume(tuple.clone().field(Field::new(i), ty))
             }));
             res
         } else {
             args.into_iter()
-                .map(|a| create_temp_if_necessary(a, tcx, callsite, caller_mir))
+                .map(|a| Operand::Consume(self.create_temp_if_necessary(a, callsite, caller_mir)))
                 .collect()
         }
     }
+
+    /// If `arg` is already a temporary, returns it. Otherwise, introduces a fresh
+    /// temporary `T` and an instruction `T = arg`, and returns `T`.
+    fn create_temp_if_necessary(
+        &self,
+        arg: Operand<'tcx>,
+        callsite: &CallSite<'tcx>,
+        caller_mir: &mut Mir<'tcx>,
+    ) -> Lvalue<'tcx> {
+        // FIXME: Analysis of the usage of the arguments to avoid
+        // unnecessary temporaries.
+
+        if let Operand::Consume(Lvalue::Local(local)) = arg {
+            if caller_mir.local_kind(local) == LocalKind::Temp {
+                // Reuse the operand if it's a temporary already
+                return Lvalue::Local(local);
+            }
+        }
+
+        debug!("Creating temp for argument {:?}", arg);
+        // Otherwise, create a temporary for the arg
+        let arg = Rvalue::Use(arg);
+
+        let ty = arg.ty(caller_mir, self.tcx);
+
+        let arg_tmp = LocalDecl::new_temp(ty, callsite.location.span);
+        let arg_tmp = caller_mir.local_decls.push(arg_tmp);
+        let arg_tmp = Lvalue::Local(arg_tmp);
+
+        let stmt = Statement {
+            source_info: callsite.location,
+            kind: StatementKind::Assign(arg_tmp.clone(), arg),
+        };
+        caller_mir[callsite.bb].statements.push(stmt);
+        arg_tmp
+    }
 }
 
 fn type_size_of<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>,

From ec2ff8f734bb2f3ff2c58fc8e5df089444da58fd Mon Sep 17 00:00:00 2001
From: Shotaro Yamada <sinkuu@sinkuu.xyz>
Date: Sat, 11 Nov 2017 02:20:53 +0900
Subject: [PATCH 3/4] Add TyCtxt::is_closure

---
 src/librustc/ty/util.rs                      |  6 +++++-
 src/librustc_mir/transform/check_unsafety.rs | 11 +++++------
 src/librustc_mir/transform/inline.rs         |  3 +--
 3 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/src/librustc/ty/util.rs b/src/librustc/ty/util.rs
index aa07a6b070e..a0219f2f95b 100644
--- a/src/librustc/ty/util.rs
+++ b/src/librustc/ty/util.rs
@@ -621,9 +621,13 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> {
         result
     }
 
+    pub fn is_closure(self, def_id: DefId) -> bool {
+        self.def_key(def_id).disambiguated_data.data == DefPathData::ClosureExpr
+    }
+
     pub fn closure_base_def_id(self, def_id: DefId) -> DefId {
         let mut def_id = def_id;
-        while self.def_key(def_id).disambiguated_data.data == DefPathData::ClosureExpr {
+        while self.is_closure(def_id) {
             def_id = self.parent_def_id(def_id).unwrap_or_else(|| {
                 bug!("closure {:?} has no parent", def_id);
             });
diff --git a/src/librustc_mir/transform/check_unsafety.rs b/src/librustc_mir/transform/check_unsafety.rs
index cbf1b0b0899..302e65a0e34 100644
--- a/src/librustc_mir/transform/check_unsafety.rs
+++ b/src/librustc_mir/transform/check_unsafety.rs
@@ -15,7 +15,6 @@ use rustc::ty::maps::Providers;
 use rustc::ty::{self, TyCtxt};
 use rustc::hir;
 use rustc::hir::def_id::DefId;
-use rustc::hir::map::DefPathData;
 use rustc::lint::builtin::{SAFE_EXTERN_STATICS, UNUSED_UNSAFE};
 use rustc::mir::*;
 use rustc::mir::visit::{LvalueContext, Visitor};
@@ -362,11 +361,11 @@ fn report_unused_unsafe(tcx: TyCtxt, used_unsafe: &FxHashSet<ast::NodeId>, id: a
 
 pub fn check_unsafety<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, def_id: DefId) {
     debug!("check_unsafety({:?})", def_id);
-    match tcx.def_key(def_id).disambiguated_data.data {
-        // closures are handled by their parent fn.
-        DefPathData::ClosureExpr => return,
-        _ => {}
-    };
+
+    // closures are handled by their parent fn.
+    if tcx.is_closure(def_id) {
+        return;
+    }
 
     let UnsafetyCheckResult {
         violations,
diff --git a/src/librustc_mir/transform/inline.rs b/src/librustc_mir/transform/inline.rs
index 460af371e04..b9e87fb9ec6 100644
--- a/src/librustc_mir/transform/inline.rs
+++ b/src/librustc_mir/transform/inline.rs
@@ -20,7 +20,6 @@ use rustc::mir::transform::{MirPass, MirSource};
 use rustc::mir::visit::*;
 use rustc::ty::{self, Instance, Ty, TyCtxt, TypeFoldable};
 use rustc::ty::subst::{Subst,Substs};
-use rustc::hir::map::definitions::DefPathData;
 
 use std::collections::VecDeque;
 use super::simplify::{remove_dead_blocks, CfgSimplifier};
@@ -561,7 +560,7 @@ impl<'a, 'tcx> Inliner<'a, 'tcx> {
 
         // A closure is passed its self-type and a tuple like `(arg1, arg2, ...)`,
         // hence mappings to tuple fields are needed.
-        if tcx.def_key(callsite.callee).disambiguated_data.data == DefPathData::ClosureExpr {
+        if tcx.is_closure(callsite.callee) {
             let mut args = args.into_iter();
             let self_ = self.create_temp_if_necessary(args.next().unwrap(), callsite, caller_mir);
             let tuple = self.create_temp_if_necessary(args.next().unwrap(), callsite, caller_mir);

From a1f7bad4a71667a80a4fb9bcb31ca440de17f7c8 Mon Sep 17 00:00:00 2001
From: Shotaro Yamada <sinkuu@sinkuu.xyz>
Date: Tue, 14 Nov 2017 22:29:09 +0900
Subject: [PATCH 4/4] Fix test

---
 src/test/mir-opt/inline-closure.rs | 27 ++++++++-------------------
 1 file changed, 8 insertions(+), 19 deletions(-)

diff --git a/src/test/mir-opt/inline-closure.rs b/src/test/mir-opt/inline-closure.rs
index bf5761ba5b7..3f3428714d1 100644
--- a/src/test/mir-opt/inline-closure.rs
+++ b/src/test/mir-opt/inline-closure.rs
@@ -18,35 +18,24 @@ fn main() {
 
 fn foo<T: Copy>(_t: T, q: i32) -> i32 {
     let x = |_t, _q| _t;
-    x(q*2, q*3)
+    x(q, q)
 }
 
 // END RUST SOURCE
 // START rustc.foo.Inline.after.mir
 // ...
 // bb0: {
-//     StorageLive(_3);
+//     ...
 //     _3 = [closure@NodeId(28)];
-//     StorageLive(_4);
+//     ...
 //     _4 = &_3;
-//     StorageLive(_5);
-//     StorageLive(_6);
-//     StorageLive(_7);
+//     ...
+//     _6 = _2;
+//     ...
 //     _7 = _2;
-//     _6 = Mul(_7, const 2i32);
-//     StorageDead(_7);
-//     StorageLive(_8);
-//     StorageLive(_9);
-//     _9 = _2;
-//     _8 = Mul(_9, const 3i32);
-//     StorageDead(_9);
-//     _5 = (_6, _8);
+//     _5 = (_6, _7);
 //     _0 = (_5.0: i32);
-//     StorageDead(_5);
-//     StorageDead(_8);
-//     StorageDead(_6);
-//     StorageDead(_4);
-//     StorageDead(_3);
+//     ...
 //     return;
 // }
 // ...