Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] binops: don't eagerly unify the types of LHS and RHS if one of them is a primitive #22993

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/libcollections/bench.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,13 @@ macro_rules! map_insert_rand_bench {
let mut rng = rand::weak_rng();

for _ in 0..n {
let i = rng.gen() % n;
let i = rng.gen::<usize>() % n;
map.insert(i, i);
}

// measure
b.iter(|| {
let k = rng.gen() % n;
let k = rng.gen::<usize>() % n;
map.insert(k, k);
map.remove(&k);
});
Expand Down Expand Up @@ -77,7 +77,7 @@ macro_rules! map_find_rand_bench {

// setup
let mut rng = rand::weak_rng();
let mut keys: Vec<_> = (0..n).map(|_| rng.gen() % n).collect();
let mut keys: Vec<_> = (0..n).map(|_| rng.gen::<usize>() % n).collect();

for &k in &keys {
map.insert(k, k);
Expand Down
3 changes: 2 additions & 1 deletion src/libcore/iter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1146,7 +1146,8 @@ pub trait AdditiveIterator<A> {
///
/// let a = [1i32, 2, 3, 4, 5];
/// let mut it = a.iter().cloned();
/// assert!(it.sum() == 15);
/// let sum: i32 = it.sum();
/// assert!(sum == 15);
/// ```
fn sum(self) -> A;
}
Expand Down
2 changes: 1 addition & 1 deletion src/librand/distributions/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -257,7 +257,7 @@ fn ziggurat<R: Rng, P, Z>(
return zero_case(rng, u);
}
// algebraically equivalent to f1 + DRanU()*(f0 - f1) < 1
if f_tab[i + 1] + (f_tab[i] - f_tab[i + 1]) * rng.gen() < pdf(x) {
if f_tab[i + 1] + (f_tab[i] - f_tab[i + 1]) * rng.gen::<f64>() < pdf(x) {
return x;
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/librand/distributions/range.rs
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ macro_rules! float_impl {
}
}
fn sample_range<R: Rng>(r: &Range<$ty>, rng: &mut R) -> $ty {
r.low + r.range * rng.gen()
r.low + r.range * rng.gen::<$ty>()
}
}
}
Expand Down
7 changes: 5 additions & 2 deletions src/librustc/middle/check_match.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ use middle::ty::*;
use middle::ty;
use std::cmp::Ordering;
use std::fmt;
use std::iter::{range_inclusive, AdditiveIterator, FromIterator, IntoIterator, repeat};
use std::iter::{range_inclusive, FromIterator, IntoIterator, repeat};
use std::num::Float;
use std::slice;
use syntax::ast::{self, DUMMY_NODE_ID, NodeId, Pat};
Expand Down Expand Up @@ -61,6 +61,8 @@ struct Matrix<'a>(Vec<Vec<&'a Pat>>);
/// ++++++++++++++++++++++++++
impl<'a> fmt::Debug for Matrix<'a> {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
use std::ops::Add;

try!(write!(f, "\n"));

let &Matrix(ref m) = self;
Expand All @@ -76,7 +78,8 @@ impl<'a> fmt::Debug for Matrix<'a> {
pretty_printed_matrix.iter().map(|row| row[col].len()).max().unwrap_or(0)
}).collect();

let total_width = column_widths.iter().cloned().sum() + column_count * 3 + 1;
// FIXME(japaric) using `sum()` asks for type annotations
let total_width = column_widths.iter().cloned().fold(0, Add::add) + column_count * 3 + 1;
let br = repeat('+').take(total_width).collect::<String>();
try!(write!(f, "{}\n", br));
for row in pretty_printed_matrix {
Expand Down
58 changes: 34 additions & 24 deletions src/librustc/middle/ty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5755,26 +5755,30 @@ pub fn closure_upvars<'tcx>(typer: &mc::Typer<'tcx>,
}
}

pub fn is_binopable<'tcx>(cx: &ctxt<'tcx>, ty: Ty<'tcx>, op: ast::BinOp) -> bool {
pub fn is_builtin_binop<'tcx>(cx: &ctxt<'tcx>,
lhs: Ty<'tcx>,
rhs: Ty<'tcx>,
op: ast::BinOp)
-> bool {
#![allow(non_upper_case_globals)]
static tycat_other: int = 0;
static tycat_bool: int = 1;
static tycat_char: int = 2;
static tycat_int: int = 3;
static tycat_float: int = 4;
static tycat_raw_ptr: int = 6;

static opcat_add: int = 0;
static opcat_sub: int = 1;
static opcat_mult: int = 2;
static opcat_shift: int = 3;
static opcat_rel: int = 4;
static opcat_eq: int = 5;
static opcat_bit: int = 6;
static opcat_logic: int = 7;
static opcat_mod: int = 8;

fn opcat(op: ast::BinOp) -> int {
const tycat_other: usize = 0;
const tycat_bool: usize = 1;
const tycat_char: usize = 2;
const tycat_int: usize = 3;
const tycat_float: usize = 4;
const tycat_raw_ptr: usize = 5;

const opcat_add: usize = 0;
const opcat_sub: usize = 1;
const opcat_mult: usize = 2;
const opcat_shift: usize = 3;
const opcat_rel: usize = 4;
const opcat_eq: usize = 5;
const opcat_bit: usize = 6;
const opcat_logic: usize = 7;
const opcat_mod: usize = 8;

fn opcat(op: ast::BinOp) -> usize {
match op.node {
ast::BiAdd => opcat_add,
ast::BiSub => opcat_sub,
Expand All @@ -5797,7 +5801,7 @@ pub fn is_binopable<'tcx>(cx: &ctxt<'tcx>, ty: Ty<'tcx>, op: ast::BinOp) -> bool
}
}

fn tycat<'tcx>(cx: &ctxt<'tcx>, ty: Ty<'tcx>) -> int {
fn tycat<'tcx>(cx: &ctxt<'tcx>, ty: Ty<'tcx>) -> usize {
if type_is_simd(cx, ty) {
return tycat(cx, simd_type(cx, ty))
}
Expand All @@ -5811,8 +5815,8 @@ pub fn is_binopable<'tcx>(cx: &ctxt<'tcx>, ty: Ty<'tcx>, op: ast::BinOp) -> bool
}
}

static t: bool = true;
static f: bool = false;
const t: bool = true;
const f: bool = false;

let tbl = [
// +, -, *, shift, rel, ==, bit, logic, mod
Expand All @@ -5821,10 +5825,16 @@ pub fn is_binopable<'tcx>(cx: &ctxt<'tcx>, ty: Ty<'tcx>, op: ast::BinOp) -> bool
/*char*/ [f, f, f, f, t, t, f, f, f],
/*int*/ [t, t, t, t, t, t, t, f, t],
/*float*/ [t, t, t, f, t, t, f, f, f],
/*bot*/ [t, t, t, t, t, t, t, t, t],
/*raw ptr*/ [f, f, f, f, t, t, f, f, f]];

return tbl[tycat(cx, ty) as uint ][opcat(op) as uint];
let lhs_cat = tycat(cx, lhs);
let rhs_cat = tycat(cx, rhs);

if lhs_cat == rhs_cat {
tbl[lhs_cat][opcat(op)]
} else {
false
}
}

// Returns the repeat count for a repeating vector expression.
Expand Down
111 changes: 57 additions & 54 deletions src/librustc_typeck/check/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2683,8 +2683,14 @@ fn check_expr_with_unifier<'a, 'tcx, F>(fcx: &FnCtxt<'a, 'tcx>,
unifier: F) where
F: FnOnce(),
{
debug!(">> typechecking: expr={} expected={}",
expr.repr(fcx.tcx()), expected.repr(fcx.tcx()));
if fcx.inh.node_types.borrow().get(&expr.id).is_some() {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without this, the RHS ended being type checked (check_expr_*) twice. And that causes problems when RHS is a generic enum, eg Some(0) == None didn't type check (RUST_LOG reported unfulfilled _: Sized obligations on None), and the error message was that None needed type annotations: Some(0) == None::<i32>().

This was the least invasive solution that I could think of to avoid type cheking the RHS twice. If you have any other idea, let me know.

debug!(">> already checked, skipping expr={}", expr.repr(fcx.tcx()));

return unifier();
} else {
debug!(">> typechecking: expr={} expected={}",
expr.repr(fcx.tcx()), expected.repr(fcx.tcx()));
}

// Checks a method call.
fn check_method_call<'a, 'tcx>(fcx: &FnCtxt<'a, 'tcx>,
Expand Down Expand Up @@ -2881,46 +2887,21 @@ fn check_expr_with_unifier<'a, 'tcx, F>(fcx: &FnCtxt<'a, 'tcx>,
SimpleBinop => NoPreference
};
check_expr_with_lvalue_pref(fcx, lhs, lvalue_pref);
check_expr(fcx, rhs);

// Callee does bot / err checking
let lhs_t =
structurally_resolve_type_or_else(fcx, lhs.span, fcx.expr_ty(lhs), || {
if ast_util::is_symmetric_binop(op.node) {
// Try RHS first
check_expr(fcx, &**rhs);
fcx.expr_ty(&**rhs)
} else {
fcx.tcx().types.err
}
});
let lhs_t = fcx.resolve_type_vars_if_possible(fcx.expr_ty(lhs));
let rhs_t = fcx.resolve_type_vars_if_possible(fcx.expr_ty(rhs));

if ty::type_is_integral(lhs_t) && ast_util::is_shift_binop(op.node) {
// Shift is a special case: rhs must be uint, no matter what lhs is
check_expr(fcx, &**rhs);
let rhs_ty = fcx.expr_ty(&**rhs);
let rhs_ty = structurally_resolved_type(fcx, rhs.span, rhs_ty);
if ty::type_is_integral(rhs_ty) {
fcx.write_ty(expr.id, lhs_t);
} else {
fcx.type_error_message(
expr.span,
|actual| {
format!(
"right-hand-side of a shift operation must have integral type, \
not `{}`",
actual)
},
rhs_ty,
None);
fcx.write_ty(expr.id, fcx.tcx().types.err);
if ty::is_builtin_binop(tcx, lhs_t, rhs_t, op) {
match op.node {
ast::BiShl | ast::BiShr => {
// NB most built-in binops are restricted to arguments of the same type, `>>`
// and `<<` are the exception.
},
_ => {
demand::suptype(fcx, rhs.span, rhs_t, lhs_t);
}
}
return;
}

if ty::is_binopable(tcx, lhs_t, op) {
let tvar = fcx.infcx().next_ty_var();
demand::suptype(fcx, expr.span, tvar, lhs_t);
check_expr_has_type(fcx, &**rhs, tvar);

let result_t = match op.node {
ast::BiEq | ast::BiNe | ast::BiLt | ast::BiLe | ast::BiGe |
Expand Down Expand Up @@ -2955,25 +2936,49 @@ fn check_expr_with_unifier<'a, 'tcx, F>(fcx: &FnCtxt<'a, 'tcx>,
}

if op.node == ast::BiOr || op.node == ast::BiAnd {
// This is an error; one of the operands must have the wrong
// type
fcx.write_error(expr.id);
fcx.write_error(rhs.id);
fcx.type_error_message(expr.span,
|actual| {
format!("binary operation `{}` cannot be applied \
to type `{}`",
ast_util::binop_to_string(op.node),
actual)
},
lhs_t,
None)
if ty::type_is_bool(lhs_t) {
demand::suptype(fcx, rhs.span, rhs_t, lhs_t);
fcx.write_ty(expr.id, lhs_t);
return
} else if ty::type_is_bool(rhs_t) {
demand::suptype(fcx, lhs.span, lhs_t, rhs_t);
fcx.write_ty(expr.id, rhs_t);
return
} else {
// This is an error; one of the operands must have the wrong
// type
fcx.write_error(expr.id);
fcx.write_error(rhs.id);
fcx.type_error_message(expr.span,
|actual| {
format!("binary operation `{}` cannot be applied \
to type `{}`",
ast_util::binop_to_string(op.node),
actual)
},
lhs_t,
None)
}
}

// Check for overloaded operators if not an assignment.
let result_t = if is_binop_assignment == SimpleBinop {
check_user_binop(fcx, expr, lhs, lhs_t, op, rhs)
} else {
// NB currently, `lhs += rhs` is only valid if `lhs` and `rhs` have the same type. This
// won't be true once the `OpAssign` traits land as they'll allow overloading the RHS
if ty::is_builtin_binop(tcx, lhs_t, lhs_t, op) {
demand::suptype(fcx, rhs.span, rhs_t, lhs_t);

fcx.write_ty(expr.id, lhs_t);
return
} else if ty::is_builtin_binop(tcx, rhs_t, rhs_t, op) {
demand::suptype(fcx, lhs.span, lhs_t, rhs_t);

fcx.write_ty(expr.id, rhs_t);
return
}

fcx.type_error_message(expr.span,
|actual| {
format!("binary assignment \
Expand All @@ -2985,7 +2990,6 @@ fn check_expr_with_unifier<'a, 'tcx, F>(fcx: &FnCtxt<'a, 'tcx>,
},
lhs_t,
None);
check_expr(fcx, &**rhs);
fcx.tcx().types.err
};

Expand Down Expand Up @@ -3021,7 +3025,6 @@ fn check_expr_with_unifier<'a, 'tcx, F>(fcx: &FnCtxt<'a, 'tcx>,
ast::BiEq => ("eq", lang.eq_trait()),
ast::BiNe => ("ne", lang.eq_trait()),
ast::BiAnd | ast::BiOr => {
check_expr(fcx, &**rhs);
return tcx.types.err;
}
};
Expand Down
2 changes: 1 addition & 1 deletion src/libstd/rand/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -386,7 +386,7 @@ impl Rng for ThreadRng {
/// ```
/// use std::rand;
///
/// let x = rand::random();
/// let x = rand::random::<u8>();
/// println!("{}", 2u8 * x);
///
/// let y = rand::random::<f64>();
Expand Down
14 changes: 0 additions & 14 deletions src/libsyntax/ast_util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -102,20 +102,6 @@ pub fn is_by_value_binop(b: BinOp_) -> bool {
}
}

/// Returns `true` if the binary operator is symmetric in the sense that LHS
/// and RHS must have the same type. So the type of LHS can serve as an hint
/// for the type of RHS and vice versa.
pub fn is_symmetric_binop(b: BinOp_) -> bool {
match b {
BiAdd | BiSub | BiMul | BiDiv | BiRem |
BiBitXor | BiBitAnd | BiBitOr |
BiEq | BiLt | BiLe | BiNe | BiGt | BiGe => {
true
}
_ => false
}
}

/// Returns `true` if the unary operator takes its argument by value
pub fn is_by_value_unop(u: UnOp) -> bool {
match u {
Expand Down
2 changes: 1 addition & 1 deletion src/test/bench/noise.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ fn lerp(a: f32, b: f32, v: f32) -> f32 { a * (1.0 - v) + b * v }
fn smooth(v: f32) -> f32 { v * v * (3.0 - 2.0 * v) }

fn random_gradient<R: Rng>(r: &mut R) -> Vec2 {
let v = PI * 2.0 * r.gen();
let v = PI * 2.0 * r.gen::<f32>();
Vec2 { x: v.cos(), y: v.sin() }
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,5 +35,7 @@ trait Add<RHS=Self> {
fn ice<A>(a: A) {
let r = loop {};
r = r + a;
//~^ ERROR binary operation `+` cannot be applied to type `A`
//~^ ERROR the trait `Add<A>` is not implemented for the type `()`
//~| ERROR
// FIXME(#21528) the error should be reported once, not twice
}
2 changes: 2 additions & 0 deletions src/test/compile-fail/binop-fail-3.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@
// option. This file may not be copied, modified, or distributed
// except according to those terms.

// ignore-test FIXME(japaric) remove test?

fn foo() -> ! { panic!("quux"); }
fn main() {
foo() //~ ERROR the type of this value must be known in this context
Expand Down
14 changes: 14 additions & 0 deletions src/test/compile-fail/binop-unknown-lhs.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
// 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.

fn main() {
let x = "5".parse().unwrap(); //~ ERROR unable to infer enough type information about `_`
let y = x + 0u8;
}
Loading