Make expr_is_lval more robust

Previously it also tried to find out the best way to translate the
expression, which could ICE during type-checking.

Fixes #23173
Fixes #24322
Fixes #25757

5 files changed, 256 insertions, 140 deletions

diff --git a/src/librustc/middle/ty.rs b/src/librustc/middle/ty.rs
index 170b98b8831..3497a8d6904 100644
--- a/src/librustc/middle/ty.rs
+++ b/src/librustc/middle/ty.rs
@@ -20,7 +20,6 @@ pub use self::Variance::*;
 pub use self::AutoAdjustment::*;
 pub use self::Representability::*;
 pub use self::AutoRef::*;
-pub use self::ExprKind::*;
 pub use self::DtorKind::*;
 pub use self::ExplicitSelfCategory::*;
 pub use self::FnOutput::*;
@@ -87,7 +86,7 @@ use syntax::abi;
 use syntax::ast::{CrateNum, DefId, ItemImpl, ItemTrait, LOCAL_CRATE};
 use syntax::ast::{MutImmutable, MutMutable, Name, NamedField, NodeId};
 use syntax::ast::{StmtExpr, StmtSemi, StructField, UnnamedField, Visibility};
-use syntax::ast_util::{self, is_local, lit_is_str, local_def};
+use syntax::ast_util::{self, is_local, local_def};
 use syntax::attr::{self, AttrMetaMethods, SignedInt, UnsignedInt};
 use syntax::codemap::Span;
 use syntax::parse::token::{self, InternedString, special_idents};
@@ -5106,96 +5105,27 @@ pub fn resolve_expr(tcx: &ctxt, expr: &ast::Expr) -> def::Def {
     }
 }
 
-pub fn expr_is_lval(tcx: &ctxt, e: &ast::Expr) -> bool {
-    match expr_kind(tcx, e) {
-        LvalueExpr => true,
-        RvalueDpsExpr | RvalueDatumExpr | RvalueStmtExpr => false
-    }
-}
-
-/// We categorize expressions into three kinds.  The distinction between
-/// lvalue/rvalue is fundamental to the language.  The distinction between the
-/// two kinds of rvalues is an artifact of trans which reflects how we will
-/// generate code for that kind of expression.  See trans/expr.rs for more
-/// information.
-#[derive(Copy, Clone)]
-pub enum ExprKind {
-    LvalueExpr,
-    RvalueDpsExpr,
-    RvalueDatumExpr,
-    RvalueStmtExpr
-}
-
-pub fn expr_kind(tcx: &ctxt, expr: &ast::Expr) -> ExprKind {
-    if tcx.method_map.borrow().contains_key(&MethodCall::expr(expr.id)) {
-        // Overloaded operations are generally calls, and hence they are
-        // generated via DPS, but there are a few exceptions:
-        return match expr.node {
-            // `a += b` has a unit result.
-            ast::ExprAssignOp(..) => RvalueStmtExpr,
-
-            // the deref method invoked for `*a` always yields an `&T`
-            ast::ExprUnary(ast::UnDeref, _) => LvalueExpr,
-
-            // the index method invoked for `a[i]` always yields an `&T`
-            ast::ExprIndex(..) => LvalueExpr,
-
-            // in the general case, result could be any type, use DPS
-            _ => RvalueDpsExpr
-        };
-    }
-
-    match expr.node {
+pub fn expr_is_lval(tcx: &ctxt, expr: &ast::Expr) -> bool {
+     match expr.node {
         ast::ExprPath(..) => {
-            match resolve_expr(tcx, expr) {
-                def::DefVariant(tid, vid, _) => {
-                    let variant_info = enum_variant_with_id(tcx, tid, vid);
-                    if !variant_info.args.is_empty() {
-                        // N-ary variant.
-                        RvalueDatumExpr
-                    } else {
-                        // Nullary variant.
-                        RvalueDpsExpr
-                    }
+            // We can't use resolve_expr here, as this needs to run on broken
+            // programs. We don't need to through - associated items are all
+            // rvalues.
+            match tcx.def_map.borrow().get(&expr.id) {
+                Some(&def::PathResolution {
+                    base_def: def::DefStatic(..), ..
+                }) | Some(&def::PathResolution {
+                    base_def: def::DefUpvar(..), ..
+                }) | Some(&def::PathResolution {
+                    base_def: def::DefLocal(..), ..
+                }) => {
+                    true
                 }
 
-                def::DefStruct(_) => {
-                    match tcx.node_types.borrow().get(&expr.id) {
-                        Some(ty) => match ty.sty {
-                            TyBareFn(..) => RvalueDatumExpr,
-                            _ => RvalueDpsExpr
-                        },
-                        // See ExprCast below for why types might be missing.
-                        None => RvalueDatumExpr
-                     }
-                }
+                Some(..) => false,
 
-                // Special case: A unit like struct's constructor must be called without () at the
-                // end (like `UnitStruct`) which means this is an ExprPath to a DefFn. But in case
-                // of unit structs this is should not be interpreted as function pointer but as
-                // call to the constructor.
-                def::DefFn(_, true) => RvalueDpsExpr,
-
-                // Fn pointers are just scalar values.
-                def::DefFn(..) | def::DefMethod(..) => RvalueDatumExpr,
-
-                // Note: there is actually a good case to be made that
-                // DefArg's, particularly those of immediate type, ought to
-                // considered rvalues.
-                def::DefStatic(..) |
-                def::DefUpvar(..) |
-                def::DefLocal(..) => LvalueExpr,
-
-                def::DefConst(..) |
-                def::DefAssociatedConst(..) => RvalueDatumExpr,
-
-                def => {
-                    tcx.sess.span_bug(
-                        expr.span,
-                        &format!("uncategorized def for expr {}: {:?}",
-                                expr.id,
-                                def));
-                }
+                None => tcx.sess.span_bug(expr.span, &format!(
+                    "no def for path {}", expr.id))
             }
         }
 
@@ -5203,7 +5133,7 @@ pub fn expr_kind(tcx: &ctxt, expr: &ast::Expr) -> ExprKind {
         ast::ExprField(..) |
         ast::ExprTupField(..) |
         ast::ExprIndex(..) => {
-            LvalueExpr
+            true
         }
 
         ast::ExprCall(..) |
@@ -5216,25 +5146,7 @@ pub fn expr_kind(tcx: &ctxt, expr: &ast::Expr) -> ExprKind {
         ast::ExprClosure(..) |
         ast::ExprBlock(..) |
         ast::ExprRepeat(..) |
-        ast::ExprVec(..) => {
-            RvalueDpsExpr
-        }
-
-        ast::ExprIfLet(..) => {
-            tcx.sess.span_bug(expr.span, "non-desugared ExprIfLet");
-        }
-        ast::ExprWhileLet(..) => {
-            tcx.sess.span_bug(expr.span, "non-desugared ExprWhileLet");
-        }
-
-        ast::ExprForLoop(..) => {
-            tcx.sess.span_bug(expr.span, "non-desugared ExprForLoop");
-        }
-
-        ast::ExprLit(ref lit) if lit_is_str(&**lit) => {
-            RvalueDpsExpr
-        }
-
+        ast::ExprVec(..) |
         ast::ExprBreak(..) |
         ast::ExprAgain(..) |
         ast::ExprRet(..) |
@@ -5242,34 +5154,21 @@ pub fn expr_kind(tcx: &ctxt, expr: &ast::Expr) -> ExprKind {
         ast::ExprLoop(..) |
         ast::ExprAssign(..) |
         ast::ExprInlineAsm(..) |
-        ast::ExprAssignOp(..) => {
-            RvalueStmtExpr
-        }
-
-        ast::ExprLit(_) | // Note: LitStr is carved out above
+        ast::ExprAssignOp(..) |
+        ast::ExprLit(_) |
         ast::ExprUnary(..) |
-        ast::ExprBox(None, _) |
+        ast::ExprBox(..) |
         ast::ExprAddrOf(..) |
         ast::ExprBinary(..) |
         ast::ExprCast(..) => {
-            RvalueDatumExpr
-        }
-
-        ast::ExprBox(Some(ref place), _) => {
-            // Special case `Box<T>` for now:
-            let def_id = match tcx.def_map.borrow().get(&place.id) {
-                Some(def) => def.def_id(),
-                None => panic!("no def for place"),
-            };
-            if tcx.lang_items.exchange_heap() == Some(def_id) {
-                RvalueDatumExpr
-            } else {
-                RvalueDpsExpr
-            }
+            false
         }
 
-        ast::ExprParen(ref e) => expr_kind(tcx, &**e),
+        ast::ExprParen(ref e) => expr_is_lval(tcx, e),
 
+        ast::ExprIfLet(..) |
+        ast::ExprWhileLet(..) |
+        ast::ExprForLoop(..) |
         ast::ExprMac(..) => {
             tcx.sess.span_bug(
                 expr.span,
diff --git a/src/librustc_trans/trans/expr.rs b/src/librustc_trans/trans/expr.rs
index 68385259c36..5ee71594935 100644
--- a/src/librustc_trans/trans/expr.rs
+++ b/src/librustc_trans/trans/expr.rs
@@ -188,15 +188,15 @@ pub fn trans_into<'blk, 'tcx>(bcx: Block<'blk, 'tcx>,
                                                                           false);
     bcx.fcx.push_ast_cleanup_scope(cleanup_debug_loc);
 
-    let kind = ty::expr_kind(bcx.tcx(), expr);
+    let kind = expr_kind(bcx.tcx(), expr);
     bcx = match kind {
-        ty::LvalueExpr | ty::RvalueDatumExpr => {
+        ExprKind::Lvalue | ExprKind::RvalueDatum => {
             trans_unadjusted(bcx, expr).store_to_dest(dest, expr.id)
         }
-        ty::RvalueDpsExpr => {
+        ExprKind::RvalueDps => {
             trans_rvalue_dps_unadjusted(bcx, expr, dest)
         }
-        ty::RvalueStmtExpr => {
+        ExprKind::RvalueStmt => {
             trans_rvalue_stmt_unadjusted(bcx, expr)
         }
     };
@@ -582,8 +582,8 @@ fn trans_unadjusted<'blk, 'tcx>(bcx: Block<'blk, 'tcx>,
 
     debuginfo::set_source_location(bcx.fcx, expr.id, expr.span);
 
-    return match ty::expr_kind(bcx.tcx(), expr) {
-        ty::LvalueExpr | ty::RvalueDatumExpr => {
+    return match expr_kind(bcx.tcx(), expr) {
+        ExprKind::Lvalue | ExprKind::RvalueDatum => {
             let datum = unpack_datum!(bcx, {
                 trans_datum_unadjusted(bcx, expr)
             });
@@ -591,12 +591,12 @@ fn trans_unadjusted<'blk, 'tcx>(bcx: Block<'blk, 'tcx>,
             DatumBlock {bcx: bcx, datum: datum}
         }
 
-        ty::RvalueStmtExpr => {
+        ExprKind::RvalueStmt => {
             bcx = trans_rvalue_stmt_unadjusted(bcx, expr);
             nil(bcx, expr_ty(bcx, expr))
         }
 
-        ty::RvalueDpsExpr => {
+        ExprKind::RvalueDps => {
             let ty = expr_ty(bcx, expr);
             if type_is_zero_size(bcx.ccx(), ty) {
                 bcx = trans_rvalue_dps_unadjusted(bcx, expr, Ignore);
@@ -1531,11 +1531,13 @@ pub fn trans_adt<'a, 'blk, 'tcx>(mut bcx: Block<'blk, 'tcx>,
         // Second, trans the base to the dest.
         assert_eq!(discr, 0);
 
-        match ty::expr_kind(bcx.tcx(), &*base.expr) {
-            ty::RvalueDpsExpr | ty::RvalueDatumExpr if !bcx.fcx.type_needs_drop(ty) => {
+        match expr_kind(bcx.tcx(), &*base.expr) {
+            ExprKind::RvalueDps | ExprKind::RvalueDatum if !bcx.fcx.type_needs_drop(ty) => {
                 bcx = trans_into(bcx, &*base.expr, SaveIn(addr));
             },
-            ty::RvalueStmtExpr => bcx.tcx().sess.bug("unexpected expr kind for struct base expr"),
+            ExprKind::RvalueStmt => {
+                bcx.tcx().sess.bug("unexpected expr kind for struct base expr")
+            }
             _ => {
                 let base_datum = unpack_datum!(bcx, trans_to_lvalue(bcx, &*base.expr, "base"));
                 for &(i, t) in &base.fields {
@@ -2592,3 +2594,155 @@ fn with_overflow_check<'blk, 'tcx>(bcx: Block<'blk, 'tcx>, oop: OverflowOp, info
         (bcx, res)
     }
 }
+
+/// We categorize expressions into three kinds.  The distinction between
+/// lvalue/rvalue is fundamental to the language.  The distinction between the
+/// two kinds of rvalues is an artifact of trans which reflects how we will
+/// generate code for that kind of expression.  See trans/expr.rs for more
+/// information.
+#[derive(Copy, Clone)]
+enum ExprKind {
+    Lvalue,
+    RvalueDps,
+    RvalueDatum,
+    RvalueStmt
+}
+
+fn expr_kind(tcx: &ty::ctxt, expr: &ast::Expr) -> ExprKind {
+    if tcx.method_map.borrow().contains_key(&MethodCall::expr(expr.id)) {
+        // Overloaded operations are generally calls, and hence they are
+        // generated via DPS, but there are a few exceptions:
+        return match expr.node {
+            // `a += b` has a unit result.
+            ast::ExprAssignOp(..) => ExprKind::RvalueStmt,
+
+            // the deref method invoked for `*a` always yields an `&T`
+            ast::ExprUnary(ast::UnDeref, _) => ExprKind::Lvalue,
+
+            // the index method invoked for `a[i]` always yields an `&T`
+            ast::ExprIndex(..) => ExprKind::Lvalue,
+
+            // in the general case, result could be any type, use DPS
+            _ => ExprKind::RvalueDps
+        };
+    }
+
+    match expr.node {
+        ast::ExprPath(..) => {
+            match ty::resolve_expr(tcx, expr) {
+                def::DefStruct(_) | def::DefVariant(..) => {
+                    if let ty::TyBareFn(..) = ty::node_id_to_type(tcx, expr.id).sty {
+                        // ctor function
+                        ExprKind::RvalueDatum
+                    } else {
+                        ExprKind::RvalueDps
+                    }
+                }
+
+                // Special case: A unit like struct's constructor must be called without () at the
+                // end (like `UnitStruct`) which means this is an ExprPath to a DefFn. But in case
+                // of unit structs this is should not be interpreted as function pointer but as
+                // call to the constructor.
+                def::DefFn(_, true) => ExprKind::RvalueDps,
+
+                // Fn pointers are just scalar values.
+                def::DefFn(..) | def::DefMethod(..) => ExprKind::RvalueDatum,
+
+                // Note: there is actually a good case to be made that
+                // DefArg's, particularly those of immediate type, ought to
+                // considered rvalues.
+                def::DefStatic(..) |
+                def::DefUpvar(..) |
+                def::DefLocal(..) => ExprKind::Lvalue,
+
+                def::DefConst(..) |
+                def::DefAssociatedConst(..) => ExprKind::RvalueDatum,
+
+                def => {
+                    tcx.sess.span_bug(
+                        expr.span,
+                        &format!("uncategorized def for expr {}: {:?}",
+                                expr.id,
+                                def));
+                }
+            }
+        }
+
+        ast::ExprUnary(ast::UnDeref, _) |
+        ast::ExprField(..) |
+        ast::ExprTupField(..) |
+        ast::ExprIndex(..) => {
+            ExprKind::Lvalue
+        }
+
+        ast::ExprCall(..) |
+        ast::ExprMethodCall(..) |
+        ast::ExprStruct(..) |
+        ast::ExprRange(..) |
+        ast::ExprTup(..) |
+        ast::ExprIf(..) |
+        ast::ExprMatch(..) |
+        ast::ExprClosure(..) |
+        ast::ExprBlock(..) |
+        ast::ExprRepeat(..) |
+        ast::ExprVec(..) => {
+            ExprKind::RvalueDps
+        }
+
+        ast::ExprIfLet(..) => {
+            tcx.sess.span_bug(expr.span, "non-desugared ExprIfLet");
+        }
+        ast::ExprWhileLet(..) => {
+            tcx.sess.span_bug(expr.span, "non-desugared ExprWhileLet");
+        }
+
+        ast::ExprForLoop(..) => {
+            tcx.sess.span_bug(expr.span, "non-desugared ExprForLoop");
+        }
+
+        ast::ExprLit(ref lit) if ast_util::lit_is_str(&**lit) => {
+            ExprKind::RvalueDps
+        }
+
+        ast::ExprBreak(..) |
+        ast::ExprAgain(..) |
+        ast::ExprRet(..) |
+        ast::ExprWhile(..) |
+        ast::ExprLoop(..) |
+        ast::ExprAssign(..) |
+        ast::ExprInlineAsm(..) |
+        ast::ExprAssignOp(..) => {
+            ExprKind::RvalueStmt
+        }
+
+        ast::ExprLit(_) | // Note: LitStr is carved out above
+        ast::ExprUnary(..) |
+        ast::ExprBox(None, _) |
+        ast::ExprAddrOf(..) |
+        ast::ExprBinary(..) |
+        ast::ExprCast(..) => {
+            ExprKind::RvalueDatum
+        }
+
+        ast::ExprBox(Some(ref place), _) => {
+            // Special case `Box<T>` for now:
+            let def_id = match tcx.def_map.borrow().get(&place.id) {
+                Some(def) => def.def_id(),
+                None => panic!("no def for place"),
+            };
+            if tcx.lang_items.exchange_heap() == Some(def_id) {
+                ExprKind::RvalueDatum
+            } else {
+                ExprKind::RvalueDps
+            }
+        }
+
+        ast::ExprParen(ref e) => expr_kind(tcx, &**e),
+
+        ast::ExprMac(..) => {
+            tcx.sess.span_bug(
+                expr.span,
+                "macro expression remains after expansion");
+        }
+    }
+}
diff --git a/src/test/compile-fail/issue-23173.rs b/src/test/compile-fail/issue-23173.rs
new file mode 100644
index 00000000000..946e4b9e96e
--- /dev/null
+++ b/src/test/compile-fail/issue-23173.rs
@@ -0,0 +1,17 @@
+// Copyright 2015 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.
+
+enum Token { LeftParen, RightParen, Plus, Minus, /* etc */ }
+
+fn use_token(token: &Token) { unimplemented!() }
+
+fn main() {
+    use_token(&Token::Homura); //~ ERROR no associated item named
+}
diff --git a/src/test/compile-fail/issue-24322.rs b/src/test/compile-fail/issue-24322.rs
new file mode 100644
index 00000000000..bb726aca921
--- /dev/null
+++ b/src/test/compile-fail/issue-24322.rs
@@ -0,0 +1,19 @@
+// Copyright 2015 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.
+
+struct B;
+
+impl B {
+    fn func(&self) -> u32 { 42 }
+}
+
+fn main() {
+    let x: &fn(&B) -> u32 = &B::func; //~ ERROR mismatched types
+}
diff --git a/src/test/run-pass/issue-25757.rs b/src/test/run-pass/issue-25757.rs
new file mode 100644
index 00000000000..8deb2e244ae
--- /dev/null
+++ b/src/test/run-pass/issue-25757.rs
@@ -0,0 +1,27 @@
+// Copyright 2015 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.
+
+struct Foo {
+    a: u32
+}
+
+impl Foo {
+    fn x(&mut self) {
+        self.a = 5;
+    }
+}
+
+const FUNC: &'static Fn(&mut Foo) -> () = &Foo::x;
+
+fn main() {
+    let mut foo = Foo { a: 137 };
+    FUNC(&mut foo); //~ ERROR bad
+    assert_eq!(foo.a, 5);
+}