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

rustc_typeck: use subtyping on the LHS of binops. #45435

Merged
merged 1 commit into from
Nov 1, 2017
Merged
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
105 changes: 57 additions & 48 deletions src/librustc_typeck/check/demand.rs
Original file line number Diff line number Diff line change
Expand Up @@ -74,10 +74,16 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
}
}

pub fn demand_coerce(&self, expr: &hir::Expr, checked_ty: Ty<'tcx>, expected: Ty<'tcx>) {
if let Some(mut err) = self.demand_coerce_diag(expr, checked_ty, expected) {
pub fn demand_coerce(&self,
expr: &hir::Expr,
checked_ty: Ty<'tcx>,
expected: Ty<'tcx>)
-> Ty<'tcx> {
let (ty, err) = self.demand_coerce_diag(expr, checked_ty, expected);
if let Some(mut err) = err {
err.emit();
}
ty
}

// Checks that the type of `expr` can be coerced to `expected`.
Expand All @@ -88,61 +94,64 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
pub fn demand_coerce_diag(&self,
expr: &hir::Expr,
checked_ty: Ty<'tcx>,
expected: Ty<'tcx>) -> Option<DiagnosticBuilder<'tcx>> {
expected: Ty<'tcx>)
-> (Ty<'tcx>, Option<DiagnosticBuilder<'tcx>>) {
let expected = self.resolve_type_vars_with_obligations(expected);

if let Err(e) = self.try_coerce(expr, checked_ty, self.diverges.get(), expected) {
let cause = self.misc(expr.span);
let expr_ty = self.resolve_type_vars_with_obligations(checked_ty);
let mut err = self.report_mismatched_types(&cause, expected, expr_ty, e);
let e = match self.try_coerce(expr, checked_ty, self.diverges.get(), expected) {
Ok(ty) => return (ty, None),
Err(e) => e
};

// If the expected type is an enum with any variants whose sole
// field is of the found type, suggest such variants. See Issue
// #42764.
if let ty::TyAdt(expected_adt, substs) = expected.sty {
let mut compatible_variants = vec![];
for variant in &expected_adt.variants {
if variant.fields.len() == 1 {
let sole_field = &variant.fields[0];
let sole_field_ty = sole_field.ty(self.tcx, substs);
if self.can_coerce(expr_ty, sole_field_ty) {
let mut variant_path = self.tcx.item_path_str(variant.did);
variant_path = variant_path.trim_left_matches("std::prelude::v1::")
.to_string();
compatible_variants.push(variant_path);
}
let cause = self.misc(expr.span);
let expr_ty = self.resolve_type_vars_with_obligations(checked_ty);
let mut err = self.report_mismatched_types(&cause, expected, expr_ty, e);

// If the expected type is an enum with any variants whose sole
// field is of the found type, suggest such variants. See Issue
// #42764.
if let ty::TyAdt(expected_adt, substs) = expected.sty {
let mut compatible_variants = vec![];
for variant in &expected_adt.variants {
if variant.fields.len() == 1 {
let sole_field = &variant.fields[0];
let sole_field_ty = sole_field.ty(self.tcx, substs);
if self.can_coerce(expr_ty, sole_field_ty) {
let mut variant_path = self.tcx.item_path_str(variant.did);
variant_path = variant_path.trim_left_matches("std::prelude::v1::")
.to_string();
compatible_variants.push(variant_path);
}
}
if !compatible_variants.is_empty() {
let expr_text = print::to_string(print::NO_ANN, |s| s.print_expr(expr));
let suggestions = compatible_variants.iter()
.map(|v| format!("{}({})", v, expr_text)).collect::<Vec<_>>();
err.span_suggestions(expr.span,
"try using a variant of the expected type",
suggestions);
}
}
if !compatible_variants.is_empty() {
let expr_text = print::to_string(print::NO_ANN, |s| s.print_expr(expr));
let suggestions = compatible_variants.iter()
.map(|v| format!("{}({})", v, expr_text)).collect::<Vec<_>>();
err.span_suggestions(expr.span,
"try using a variant of the expected type",
suggestions);
}
}

if let Some(suggestion) = self.check_ref(expr,
checked_ty,
expected) {
err.help(&suggestion);
} else {
let mode = probe::Mode::MethodCall;
let suggestions = self.probe_for_return_type(syntax_pos::DUMMY_SP,
mode,
expected,
checked_ty,
ast::DUMMY_NODE_ID);
if suggestions.len() > 0 {
err.help(&format!("here are some functions which \
might fulfill your needs:\n{}",
self.get_best_match(&suggestions).join("\n")));
}
if let Some(suggestion) = self.check_ref(expr,
checked_ty,
expected) {
err.help(&suggestion);
} else {
let mode = probe::Mode::MethodCall;
let suggestions = self.probe_for_return_type(syntax_pos::DUMMY_SP,
mode,
expected,
checked_ty,
ast::DUMMY_NODE_ID);
if suggestions.len() > 0 {
err.help(&format!("here are some functions which \
might fulfill your needs:\n{}",
self.get_best_match(&suggestions).join("\n")));
}
return Some(err);
}
None
(expected, Some(err))
}

fn format_method_suggestion(&self, method: &AssociatedItem) -> String {
Expand Down
16 changes: 13 additions & 3 deletions src/librustc_typeck/check/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2755,9 +2755,19 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
fn check_expr_coercable_to_type(&self,
expr: &'gcx hir::Expr,
expected: Ty<'tcx>) -> Ty<'tcx> {
let ty = self.check_expr_with_hint(expr, expected);
self.demand_coerce(expr, ty, expected);
ty
self.check_expr_coercable_to_type_with_lvalue_pref(expr, expected, NoPreference)
}

fn check_expr_coercable_to_type_with_lvalue_pref(&self,
expr: &'gcx hir::Expr,
expected: Ty<'tcx>,
lvalue_pref: LvaluePreference)
-> Ty<'tcx> {
let ty = self.check_expr_with_expectation_and_lvalue_pref(
expr,
ExpectHasType(expected),
lvalue_pref);
self.demand_coerce(expr, ty, expected)
}

fn check_expr_with_hint(&self, expr: &'gcx hir::Expr,
Expand Down
48 changes: 27 additions & 21 deletions src/librustc_typeck/check/op.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@

use super::FnCtxt;
use super::method::MethodCallee;
use rustc::ty::{self, Ty, TypeFoldable, PreferMutLvalue, TypeVariants};
use rustc::ty::{self, Ty, TypeFoldable, NoPreference, PreferMutLvalue, TypeVariants};
use rustc::ty::TypeVariants::{TyStr, TyRef};
use rustc::ty::adjustment::{Adjustment, Adjust, AutoBorrow};
use rustc::infer::type_variable::TypeVariableOrigin;
Expand All @@ -29,12 +29,8 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
lhs_expr: &'gcx hir::Expr,
rhs_expr: &'gcx hir::Expr) -> Ty<'tcx>
{
let lhs_ty = self.check_expr_with_lvalue_pref(lhs_expr, PreferMutLvalue);

let lhs_ty = self.resolve_type_vars_with_obligations(lhs_ty);
let (rhs_ty, return_ty) =
self.check_overloaded_binop(expr, lhs_expr, lhs_ty, rhs_expr, op, IsAssign::Yes);
let rhs_ty = self.resolve_type_vars_with_obligations(rhs_ty);
let (lhs_ty, rhs_ty, return_ty) =
self.check_overloaded_binop(expr, lhs_expr, rhs_expr, op, IsAssign::Yes);

let ty = if !lhs_ty.is_ty_var() && !rhs_ty.is_ty_var()
&& is_builtin_binop(lhs_ty, rhs_ty, op) {
Expand Down Expand Up @@ -73,27 +69,24 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
lhs_expr,
rhs_expr);

let lhs_ty = self.check_expr(lhs_expr);
let lhs_ty = self.resolve_type_vars_with_obligations(lhs_ty);

match BinOpCategory::from(op) {
BinOpCategory::Shortcircuit => {
// && and || are a simple case.
self.check_expr_coercable_to_type(lhs_expr, tcx.types.bool);
let lhs_diverges = self.diverges.get();
self.demand_suptype(lhs_expr.span, tcx.mk_bool(), lhs_ty);
self.check_expr_coercable_to_type(rhs_expr, tcx.mk_bool());
self.check_expr_coercable_to_type(rhs_expr, tcx.types.bool);

// Depending on the LHS' value, the RHS can never execute.
self.diverges.set(lhs_diverges);

tcx.mk_bool()
tcx.types.bool
}
_ => {
// Otherwise, we always treat operators as if they are
// overloaded. This is the way to be most flexible w/r/t
// types that get inferred.
let (rhs_ty, return_ty) =
self.check_overloaded_binop(expr, lhs_expr, lhs_ty,
let (lhs_ty, rhs_ty, return_ty) =
self.check_overloaded_binop(expr, lhs_expr,
rhs_expr, op, IsAssign::No);

// Supply type inference hints if relevant. Probably these
Expand All @@ -108,7 +101,6 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
// deduce that the result type should be `u32`, even
// though we don't know yet what type 2 has and hence
// can't pin this down to a specific impl.
let rhs_ty = self.resolve_type_vars_with_obligations(rhs_ty);
if
!lhs_ty.is_ty_var() && !rhs_ty.is_ty_var() &&
is_builtin_binop(lhs_ty, rhs_ty, op)
Expand Down Expand Up @@ -164,17 +156,30 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
fn check_overloaded_binop(&self,
expr: &'gcx hir::Expr,
lhs_expr: &'gcx hir::Expr,
lhs_ty: Ty<'tcx>,
rhs_expr: &'gcx hir::Expr,
op: hir::BinOp,
is_assign: IsAssign)
-> (Ty<'tcx>, Ty<'tcx>)
-> (Ty<'tcx>, Ty<'tcx>, Ty<'tcx>)
{
debug!("check_overloaded_binop(expr.id={}, lhs_ty={:?}, is_assign={:?})",
debug!("check_overloaded_binop(expr.id={}, op={:?}, is_assign={:?})",
expr.id,
lhs_ty,
op,
is_assign);

let lhs_pref = match is_assign {
IsAssign::Yes => PreferMutLvalue,
IsAssign::No => NoPreference
};
// Find a suitable supertype of the LHS expression's type, by coercing to
// a type variable, to pass as the `Self` to the trait, avoiding invariant
// trait matching creating lifetime constraints that are too strict.
// E.g. adding `&'a T` and `&'b T`, given `&'x T: Add<&'x T>`, will result
// in `&'a T <: &'x T` and `&'b T <: &'x T`, instead of `'a = 'b = 'x`.
let lhs_ty = self.check_expr_coercable_to_type_with_lvalue_pref(lhs_expr,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why use coercion here rather than subtyping? I guess it is equivalent anyway, at least for the way we handle coercions now, since if you coerce with a target type of a variable you just get subtyping anyway, right?

OTOH, if we ever support "deferred coercion", I don't see any reason not to use coercion here, at least for by-value operators like + -- not entirely sure about ==.

Copy link
Member Author

Choose a reason for hiding this comment

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

Subtyping is trickier to get right, I'm far more comfortable using coercion instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

Trickier in what sense? Coercing is a superset of subtyping, so using it can only cause more things to happen.

Copy link
Member Author

Choose a reason for hiding this comment

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

I mean doing subtyping manually - I know coercion uses subtyping correctly and I'd rather use an API where it's obvious how not to misuse it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess I'm just nervous that, if coercion gets smarter, we'll now be performing random coercions here -- as opposed to just subtyping -- which maybe we do not want to do. But I can't really think of a good reason not to use coercion.

Copy link
Member Author

Choose a reason for hiding this comment

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

We already coerce the RHS FWIW.

Copy link
Contributor

Choose a reason for hiding this comment

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

That does make me feel better =)

self.next_ty_var(TypeVariableOrigin::MiscVariable(lhs_expr.span)),
lhs_pref);
let lhs_ty = self.resolve_type_vars_with_obligations(lhs_ty);

// NB: As we have not yet type-checked the RHS, we don't have the
// type at hand. Make a variable to represent it. The whole reason
// for this indirection is so that, below, we can check the expr
Expand All @@ -187,6 +192,7 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {

// see `NB` above
let rhs_ty = self.check_expr_coercable_to_type(rhs_expr, rhs_ty_var);
let rhs_ty = self.resolve_type_vars_with_obligations(rhs_ty);

let return_ty = match result {
Ok(method) => {
Expand Down Expand Up @@ -296,7 +302,7 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
}
};

(rhs_ty_var, return_ty)
(lhs_ty, rhs_ty, return_ty)
}

fn check_str_addition(&self,
Expand Down
2 changes: 1 addition & 1 deletion src/test/compile-fail/issue-41394.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@

enum Foo {
A = "" + 1
//~^ ERROR binary operation `+` cannot be applied to type `&'static str`
//~^ ERROR binary operation `+` cannot be applied to type `&str`
}

enum Bar {
Expand Down
2 changes: 2 additions & 0 deletions src/test/run-fail/binop-fail-3.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@
fn foo() -> ! {
panic!("quux");
}

#[allow(resolve_trait_on_defaulted_unit)]
fn main() {
foo() == foo(); // these types wind up being defaulted to ()
}
35 changes: 35 additions & 0 deletions src/test/run-pass/issue-32008.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
// 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.

// Tests that binary operators allow subtyping on both the LHS and RHS,
// and as such do not introduce unnecesarily strict lifetime constraints.

use std::ops::Add;

struct Foo;

impl<'a> Add<&'a Foo> for &'a Foo {
type Output = ();
fn add(self, rhs: &'a Foo) {}
}

fn try_to_add(input: &Foo) {
let local = Foo;

// Manual reborrow worked even with invariant trait search.
&*input + &local;

// Direct use of the reference on the LHS requires additional
// subtyping before searching (invariantly) for `LHS: Add<RHS>`.
input + &local;
}

fn main() {
}
20 changes: 20 additions & 0 deletions src/test/run-pass/issue-45425.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
// 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.

use std::ops::Add;

fn ref_add<T>(a: &T, b: &T) -> T
where
for<'x> &'x T: Add<&'x T, Output = T>,
{
a + b
}

fn main() {}
2 changes: 1 addition & 1 deletion src/test/ui/span/issue-39018.stderr
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
error[E0369]: binary operation `+` cannot be applied to type `&'static str`
error[E0369]: binary operation `+` cannot be applied to type `&str`
--> $DIR/issue-39018.rs:12:13
|
12 | let x = "Hello " + "World!";
Expand Down