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

update coerce docs and unify relevant tests #72791

Merged
merged 3 commits into from
Jun 20, 2020
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
49 changes: 18 additions & 31 deletions src/librustc_typeck/check/coercion.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,45 +10,30 @@
//!
//! Note that if we are expecting a reference, we will *reborrow*
//! even if the argument provided was already a reference. This is
//! useful for freezing mut/const things (that is, when the expected is &T
//! but you have &const T or &mut T) and also for avoiding the linearity
//! useful for freezing mut things (that is, when the expected type is &T
//! but you have &mut T) and also for avoiding the linearity
//! of mut things (when the expected is &mut T and you have &mut T). See
//! the various `src/test/ui/coerce-reborrow-*.rs` tests for
//! the various `src/test/ui/coerce/*.rs` tests for
//! examples of where this is useful.
//!
//! ## Subtle note
//!
//! When deciding what type coercions to consider, we do not attempt to
//! resolve any type variables we may encounter. This is because `b`
//! represents the expected type "as the user wrote it", meaning that if
//! the user defined a generic function like
//! When infering the generic arguments of functions, the argument
//! order is relevant, which can lead to the following edge case:
//!
//! fn foo<A>(a: A, b: A) { ... }
//! ```rust
//! fn foo<T>(a: T, b: T) {
//! // ...
//! }
//!
//! and then we wrote `foo(&1, @2)`, we will not auto-borrow
//! either argument. In older code we went to some lengths to
//! resolve the `b` variable, which could mean that we'd
//! auto-borrow later arguments but not earlier ones, which
//! seems very confusing.
//! foo(&7i32, &mut 7i32);
//! // This compiles, as we first infer `T` to be `&i32`,
//! // and then coerce `&mut 7i32` to `&7i32`.
//!
//! ## Subtler note
//!
//! However, right now, if the user manually specifies the
//! values for the type variables, as so:
//!
//! foo::<&int>(@1, @2)
//!
//! then we *will* auto-borrow, because we can't distinguish this from a
//! function that declared `&int`. This is inconsistent but it's easiest
//! at the moment. The right thing to do, I think, is to consider the
//! *unsubstituted* type when deciding whether to auto-borrow, but the
//! *substituted* type when considering the bounds and so forth. But most
//! of our methods don't give access to the unsubstituted type, and
//! rightly so because they'd be error-prone. So maybe the thing to do is
//! to actually determine the kind of coercions that should occur
//! separately and pass them in. Or maybe it's ok as is. Anyway, it's
//! sort of a minor point so I've opted to leave it for later -- after all,
//! we may want to adjust precisely when coercions occur.
//! foo(&mut 7i32, &7i32);
//! // This does not compile, as we first infer `T` to be `&mut i32`
//! // and are then unable to coerce `&7i32` to `&mut i32`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add an explanation for the foo::<&i32>(&7, &mut 7) case?

Copy link
Contributor Author

@lcnr lcnr Jun 19, 2020

Choose a reason for hiding this comment

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

Hmm isn't foo::<&i32>(&7, &mut 7) fairly trivial as there are no type variables at play here?

Or do you want me to add a more detailed explanation for foo(&7, &mut 7)? My current explanation is
fairly short.

foo(&7i32, &mut 7i32);
// This compiles, as we first infer `T` to be `&i32`,
// and then coerce `&mut 7i32` to `&7i32`.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm referring at keeping part of the following info around, unless it is no longer valid:

//! ## Subtler note
//!
//! However, right now, if the user manually specifies the
//! values for the type variables, as so:
//!
//! foo::<&int>(@1, @2)
//!
//! then we *will* auto-borrow, because we can't distinguish this from a
//! function that declared `&int`. This is inconsistent but it's easiest
//! at the moment. The right thing to do, I think, is to consider the
//! *unsubstituted* type when deciding whether to auto-borrow, but the
//! *substituted* type when considering the bounds and so forth. But most
//! of our methods don't give access to the unsubstituted type, and
//! rightly so because they'd be error-prone. So maybe the thing to do is
//! to actually determine the kind of coercions that should occur
//! separately and pass them in. Or maybe it's ok as is. Anyway, it's
//! sort of a minor point so I've opted to leave it for later -- after all,
//! we may want to adjust precisely when coercions occur.

Copy link
Contributor Author

@lcnr lcnr Jun 19, 2020

Choose a reason for hiding this comment

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

I think this isn't relevant anymore as we now auto-borrow both foo::<&i32>(&7, &mut 7) and foo(&7, &mut 7).

Afaict there was a time where we did not compile foo(&7, &mut 7) to prevent the inconsistency we have rn (where foo(&7, &mut 7) compiles and foo(&mut 7, &7) does not).

So the fact that coercion doesn't distinguish foo<&i32> from fn foo(_: &i32, _: &i32) shouldn't matter anymore.

//! ```

use crate::astconv::AstConv;
use crate::check::{FnCtxt, Needs};
Expand Down Expand Up @@ -96,6 +81,8 @@ impl<'a, 'tcx> Deref for Coerce<'a, 'tcx> {

type CoerceResult<'tcx> = InferResult<'tcx, (Vec<Adjustment<'tcx>>, Ty<'tcx>)>;

/// Coercing a mutable reference to an immutable works, while
/// coercing `&T` to `&mut T` should be forbidden.
fn coerce_mutbls<'tcx>(
from_mutbl: hir::Mutability,
to_mutbl: hir::Mutability,
Expand Down
68 changes: 0 additions & 68 deletions src/test/ui/coerce/coerce-overloaded-autoderef.rs

This file was deleted.

32 changes: 32 additions & 0 deletions src/test/ui/coercion/coerce-overloaded-autoderef-fail.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
fn borrow_mut<T>(x: &mut T) -> &mut T { x }
fn borrow<T>(x: &T) -> &T { x }

fn borrow_mut2<T>(_: &mut T, _: &mut T) {}
fn borrow2<T>(_: &mut T, _: &T) {}

fn double_mut_borrow<T>(x: &mut Box<T>) {
let y = borrow_mut(x);
let z = borrow_mut(x);
//~^ ERROR cannot borrow `*x` as mutable more than once at a time
drop((y, z));
}

fn double_imm_borrow(x: &mut Box<i32>) {
let y = borrow(x);
let z = borrow(x);
**x += 1;
//~^ ERROR cannot assign to `**x` because it is borrowed
drop((y, z));
}

fn double_mut_borrow2<T>(x: &mut Box<T>) {
borrow_mut2(x, x);
//~^ ERROR cannot borrow `*x` as mutable more than once at a time
}

fn double_borrow2<T>(x: &mut Box<T>) {
borrow2(x, x);
//~^ ERROR cannot borrow `*x` as mutable because it is also borrowed as immutable
}

pub fn main() {}
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
error[E0499]: cannot borrow `*x` as mutable more than once at a time
--> $DIR/coerce-overloaded-autoderef.rs:9:24
--> $DIR/coerce-overloaded-autoderef-fail.rs:9:24
|
LL | let y = borrow_mut(x);
| - first mutable borrow occurs here
Expand All @@ -10,7 +10,7 @@ LL | drop((y, z));
| - first borrow later used here

error[E0506]: cannot assign to `**x` because it is borrowed
--> $DIR/coerce-overloaded-autoderef.rs:17:5
--> $DIR/coerce-overloaded-autoderef-fail.rs:17:5
|
LL | let y = borrow(x);
| - borrow of `**x` occurs here
Expand All @@ -22,7 +22,7 @@ LL | drop((y, z));
| - borrow later used here

error[E0499]: cannot borrow `*x` as mutable more than once at a time
--> $DIR/coerce-overloaded-autoderef.rs:23:20
--> $DIR/coerce-overloaded-autoderef-fail.rs:23:20
|
LL | borrow_mut2(x, x);
| ----------- - ^ second mutable borrow occurs here
Expand All @@ -31,7 +31,7 @@ LL | borrow_mut2(x, x);
| first borrow later used by call

error[E0502]: cannot borrow `*x` as mutable because it is also borrowed as immutable
--> $DIR/coerce-overloaded-autoderef.rs:28:5
--> $DIR/coerce-overloaded-autoderef-fail.rs:28:5
|
LL | borrow2(x, x);
| -------^^^^-^
Expand Down
78 changes: 57 additions & 21 deletions src/test/ui/coercion/coerce-overloaded-autoderef.rs
Original file line number Diff line number Diff line change
@@ -1,32 +1,68 @@
fn borrow_mut<T>(x: &mut T) -> &mut T { x }
fn borrow<T>(x: &T) -> &T { x }
// run-pass
#![allow(unused_braces)]
#![allow(dead_code)]
// pretty-expanded FIXME #23616

fn borrow_mut2<T>(_: &mut T, _: &mut T) {}
fn borrow2<T>(_: &mut T, _: &T) {}
use std::rc::Rc;

fn double_mut_borrow<T>(x: &mut Box<T>) {
let y = borrow_mut(x);
let z = borrow_mut(x);
//~^ ERROR cannot borrow `*x` as mutable more than once at a time
drop((y, z));
// Examples from the "deref coercions" RFC, at rust-lang/rfcs#241.

fn use_ref<T>(_: &T) {}
fn use_mut<T>(_: &mut T) {}

fn use_rc<T>(t: Rc<T>) {
use_ref(&*t); // what you have to write today
use_ref(&t); // what you'd be able to write
use_ref(&&&&&&t);
use_ref(&mut &&&&&t);
use_ref(&&&mut &&&t);
}

fn use_mut_box<T>(mut t: &mut Box<T>) {
use_mut(&mut *t); // what you have to write today
use_mut(t); // what you'd be able to write
use_mut(&mut &mut &mut t);

use_ref(&*t); // what you have to write today
use_ref(t); // what you'd be able to write
use_ref(&&&&&&t);
use_ref(&mut &&&&&t);
use_ref(&&&mut &&&t);
}

fn double_imm_borrow(x: &mut Box<i32>) {
let y = borrow(x);
let z = borrow(x);
**x += 1;
//~^ ERROR cannot assign to `**x` because it is borrowed
drop((y, z));
fn use_nested<T>(t: &Box<T>) {
use_ref(&**t); // what you have to write today
use_ref(t); // what you'd be able to write (note: recursive deref)
use_ref(&&&&&&t);
use_ref(&mut &&&&&t);
use_ref(&&&mut &&&t);
}

fn use_slice(_: &[u8]) {}
fn use_slice_mut(_: &mut [u8]) {}

fn use_vec(mut v: Vec<u8>) {
use_slice_mut(&mut v[..]); // what you have to write today
use_slice_mut(&mut v); // what you'd be able to write
use_slice_mut(&mut &mut &mut v);

use_slice(&v[..]); // what you have to write today
use_slice(&v); // what you'd be able to write
use_slice(&&&&&&v);
use_slice(&mut &&&&&v);
use_slice(&&&mut &&&v);
}

fn double_mut_borrow2<T>(x: &mut Box<T>) {
borrow_mut2(x, x);
//~^ ERROR cannot borrow `*x` as mutable more than once at a time
fn use_vec_ref(v: &Vec<u8>) {
use_slice(&v[..]); // what you have to write today
use_slice(v); // what you'd be able to write
use_slice(&&&&&&v);
use_slice(&mut &&&&&v);
use_slice(&&&mut &&&v);
}

fn double_borrow2<T>(x: &mut Box<T>) {
borrow2(x, x);
//~^ ERROR cannot borrow `*x` as mutable because it is also borrowed as immutable
fn use_op_rhs(s: &mut String) {
*s += {&String::from(" ")};
}

pub fn main() {}
6 changes: 6 additions & 0 deletions src/test/ui/coercion/coerce-reborrow-multi-arg-fail.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
fn test<T>(_a: T, _b: T) {}

fn main() {
test(&mut 7, &7);
//~^ mismatched types
}
12 changes: 12 additions & 0 deletions src/test/ui/coercion/coerce-reborrow-multi-arg-fail.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
error[E0308]: mismatched types
--> $DIR/coerce-reborrow-multi-arg-fail.rs:4:18
|
LL | test(&mut 7, &7);
| ^^ types differ in mutability
|
= note: expected mutable reference `&mut {integer}`
found reference `&{integer}`

error: aborting due to previous error

For more information about this error, try `rustc --explain E0308`.
9 changes: 9 additions & 0 deletions src/test/ui/coercion/coerce-reborrow-multi-arg.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
// build-pass
fn test<T>(_a: T, _b: T) {}

fn main() {
test(&7, &7);
test(&7, &mut 7);
test::<&i32>(&mut 7, &7);
test::<&i32>(&mut 7, &mut 7);
}
2 changes: 0 additions & 2 deletions src/test/ui/coercion/coerce-to-bang-cast.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
#![feature(never_type)]

fn foo(x: usize, y: !, z: usize) { }

fn cast_a() {
let y = {return; 22} as !;
//~^ ERROR non-primitive cast
Expand Down
4 changes: 2 additions & 2 deletions src/test/ui/coercion/coerce-to-bang-cast.stderr
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
error[E0605]: non-primitive cast: `i32` as `!`
--> $DIR/coerce-to-bang-cast.rs:6:13
--> $DIR/coerce-to-bang-cast.rs:4:13
|
LL | let y = {return; 22} as !;
| ^^^^^^^^^^^^^^^^^ an `as` expression can only be used to convert between primitive types or to coerce to a specific trait object

error[E0605]: non-primitive cast: `i32` as `!`
--> $DIR/coerce-to-bang-cast.rs:11:13
--> $DIR/coerce-to-bang-cast.rs:9:13
|
LL | let y = 22 as !;
| ^^^^^^^ an `as` expression can only be used to convert between primitive types or to coerce to a specific trait object
Expand Down