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

Stabilize the re_rebalance_coherence feature #65879

Merged
merged 4 commits into from
Nov 9, 2019
Merged
Show file tree
Hide file tree
Changes from 3 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

This file was deleted.

27 changes: 12 additions & 15 deletions src/libcore/convert.rs
Original file line number Diff line number Diff line change
Expand Up @@ -205,11 +205,12 @@ pub trait AsMut<T: ?Sized> {
/// A value-to-value conversion that consumes the input value. The
/// opposite of [`From`].
///
/// One should only implement [`Into`] if a conversion to a type outside the current crate is
/// required. Otherwise one should always prefer implementing [`From`] over [`Into`] because
/// implementing [`From`] automatically provides one with a implementation of [`Into`] thanks to
/// the blanket implementation in the standard library. [`From`] cannot do these type of
/// conversions because of Rust's orphaning rules.
/// One should avoid implementing [`Into`] and implement [`From`] instead.
/// Implementing [`From`] automatically provides one with an implementation of [`Into`]
/// thanks to the blanket implementation in the standard library.
///
/// Prefer using [`Into`] over [`From`] when specifying trait bounds on a generic function
/// to ensure that types that only implement [`Into`] can be used as well.
///
/// **Note: This trait must not fail**. If the conversion can fail, use [`TryInto`].
///
Expand All @@ -218,23 +219,22 @@ pub trait AsMut<T: ?Sized> {
/// - [`From`]`<T> for U` implies `Into<U> for T`
/// - [`Into`] is reflexive, which means that `Into<T> for T` is implemented
///
/// # Implementing [`Into`] for conversions to external types
/// # Implementing [`Into`] for conversions to external types in old versions of Rust
///
/// If the destination type is not part of the current crate
/// then you can't implement [`From`] directly.
/// Prior to Rust 1.40, if the destination type was not part of the current crate
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should strengthen this -- unless we have a good example of when you do need to implement Into, we should probably start by telling people to avoid implementing Into directly, and then continue with this content?

Copy link
Contributor

Choose a reason for hiding this comment

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

This should say 1.41 instead?

/// then you couldn't implement [`From`] directly.
/// For example, take this code:
///
/// ```compile_fail
/// ```
/// struct Wrapper<T>(Vec<T>);
/// impl<T> From<Wrapper<T>> for Vec<T> {
/// fn from(w: Wrapper<T>) -> Vec<T> {
/// w.0
/// }
/// }
/// ```
/// This will fail to compile because we cannot implement a trait for a type
/// if both the trait and the type are not defined by the current crate.
/// This is due to Rust's orphaning rules. To bypass this, you can implement [`Into`] directly:
/// This will fail to compile in older versions of the language because Rust's orphaning rules
/// used to be a little bit more strict. To bypass this, you could implement [`Into`] directly:
///
/// ```
/// struct Wrapper<T>(Vec<T>);
Expand All @@ -249,9 +249,6 @@ pub trait AsMut<T: ?Sized> {
/// (as [`From`] does with [`Into`]). Therefore, you should always try to implement [`From`]
/// and then fall back to [`Into`] if [`From`] can't be implemented.
///
/// Prefer using [`Into`] over [`From`] when specifying trait bounds on a generic function
/// to ensure that types that only implement [`Into`] can be used as well.
///
/// # Examples
///
/// [`String`] implements [`Into`]`<`[`Vec`]`<`[`u8`]`>>`:
Expand Down
146 changes: 40 additions & 106 deletions src/librustc/traits/coherence.rs
Original file line number Diff line number Diff line change
Expand Up @@ -367,118 +367,52 @@ fn orphan_check_trait_ref<'tcx>(
trait_ref);
}

if tcx.features().re_rebalance_coherence {
// Given impl<P1..=Pn> Trait<T1..=Tn> for T0, an impl is valid only
// if at least one of the following is true:
//
// - Trait is a local trait
// (already checked in orphan_check prior to calling this function)
// - All of
// - At least one of the types T0..=Tn must be a local type.
// Let Ti be the first such type.
// - No uncovered type parameters P1..=Pn may appear in T0..Ti (excluding Ti)
//
fn uncover_fundamental_ty<'tcx>(
tcx: TyCtxt<'tcx>,
ty: Ty<'tcx>,
in_crate: InCrate,
) -> Vec<Ty<'tcx>> {
if fundamental_ty(ty) && ty_is_non_local(tcx, ty, in_crate).is_some() {
ty.walk_shallow().flat_map(|ty| uncover_fundamental_ty(tcx, ty, in_crate)).collect()
} else {
vec![ty]
}
// Given impl<P1..=Pn> Trait<T1..=Tn> for T0, an impl is valid only
// if at least one of the following is true:
//
// - Trait is a local trait
// (already checked in orphan_check prior to calling this function)
// - All of
// - At least one of the types T0..=Tn must be a local type.
// Let Ti be the first such type.
// - No uncovered type parameters P1..=Pn may appear in T0..Ti (excluding Ti)
//
fn uncover_fundamental_ty<'tcx>(
tcx: TyCtxt<'tcx>,
ty: Ty<'tcx>,
in_crate: InCrate,
) -> Vec<Ty<'tcx>> {
if fundamental_ty(ty) && ty_is_non_local(tcx, ty, in_crate).is_some() {
ty.walk_shallow().flat_map(|ty| uncover_fundamental_ty(tcx, ty, in_crate)).collect()
} else {
vec![ty]
}
}

let mut non_local_spans = vec![];
for (i, input_ty) in trait_ref
.input_types()
.flat_map(|ty| uncover_fundamental_ty(tcx, ty, in_crate))
.enumerate()
{
debug!("orphan_check_trait_ref: check ty `{:?}`", input_ty);
let non_local_tys = ty_is_non_local(tcx, input_ty, in_crate);
if non_local_tys.is_none() {
debug!("orphan_check_trait_ref: ty_is_local `{:?}`", input_ty);
return Ok(());
} else if let ty::Param(_) = input_ty.kind {
debug!("orphan_check_trait_ref: uncovered ty: `{:?}`", input_ty);
return Err(OrphanCheckErr::UncoveredTy(input_ty))
}
if let Some(non_local_tys) = non_local_tys {
for input_ty in non_local_tys {
non_local_spans.push((input_ty, i == 0));
}
}
let mut non_local_spans = vec![];
for (i, input_ty) in trait_ref
.input_types()
.flat_map(|ty| uncover_fundamental_ty(tcx, ty, in_crate))
.enumerate()
{
debug!("orphan_check_trait_ref: check ty `{:?}`", input_ty);
let non_local_tys = ty_is_non_local(tcx, input_ty, in_crate);
if non_local_tys.is_none() {
debug!("orphan_check_trait_ref: ty_is_local `{:?}`", input_ty);
return Ok(());
} else if let ty::Param(_) = input_ty.kind {
debug!("orphan_check_trait_ref: uncovered ty: `{:?}`", input_ty);
return Err(OrphanCheckErr::UncoveredTy(input_ty))
}
// If we exit above loop, never found a local type.
debug!("orphan_check_trait_ref: no local type");
Err(OrphanCheckErr::NonLocalInputType(non_local_spans))
} else {
let mut non_local_spans = vec![];
// First, create an ordered iterator over all the type
// parameters to the trait, with the self type appearing
// first. Find the first input type that either references a
// type parameter OR some local type.
for (i, input_ty) in trait_ref.input_types().enumerate() {
let non_local_tys = ty_is_non_local(tcx, input_ty, in_crate);
if non_local_tys.is_none() {
debug!("orphan_check_trait_ref: ty_is_local `{:?}`", input_ty);

// First local input type. Check that there are no
// uncovered type parameters.
let uncovered_tys = uncovered_tys(tcx, input_ty, in_crate);
for uncovered_ty in uncovered_tys {
if let Some(param) = uncovered_ty.walk()
.find(|t| is_possibly_remote_type(t, in_crate))
{
debug!("orphan_check_trait_ref: uncovered type `{:?}`", param);
return Err(OrphanCheckErr::UncoveredTy(param));
}
}

// OK, found local type, all prior types upheld invariant.
return Ok(());
}

// Otherwise, enforce invariant that there are no type
// parameters reachable.
if let Some(param) = input_ty.walk()
.find(|t| is_possibly_remote_type(t, in_crate))
{
debug!("orphan_check_trait_ref: uncovered type `{:?}`", param);
return Err(OrphanCheckErr::UncoveredTy(param));
}

if let Some(non_local_tys) = non_local_tys {
for input_ty in non_local_tys {
non_local_spans.push((input_ty, i == 0));
}
if let Some(non_local_tys) = non_local_tys {
for input_ty in non_local_tys {
non_local_spans.push((input_ty, i == 0));
}
}
// If we exit above loop, never found a local type.
debug!("orphan_check_trait_ref: no local type");
Err(OrphanCheckErr::NonLocalInputType(non_local_spans))
}
}

fn uncovered_tys<'tcx>(tcx: TyCtxt<'tcx>, ty: Ty<'tcx>, in_crate: InCrate) -> Vec<Ty<'tcx>> {
if ty_is_non_local_constructor(tcx, ty, in_crate).is_none() {
vec![]
} else if fundamental_ty(ty) {
ty.walk_shallow()
.flat_map(|t| uncovered_tys(tcx, t, in_crate))
.collect()
} else {
vec![ty]
}
}

fn is_possibly_remote_type(ty: Ty<'_>, _in_crate: InCrate) -> bool {
match ty.kind {
ty::Projection(..) | ty::Param(..) => true,
_ => false,
}
// If we exit above loop, never found a local type.
debug!("orphan_check_trait_ref: no local type");
Err(OrphanCheckErr::NonLocalInputType(non_local_spans))
}

fn ty_is_non_local<'t>(tcx: TyCtxt<'t>, ty: Ty<'t>, in_crate: InCrate) -> Option<Vec<Ty<'t>>> {
Expand Down
3 changes: 3 additions & 0 deletions src/libsyntax/feature_gate/accepted.rs
Original file line number Diff line number Diff line change
Expand Up @@ -253,6 +253,9 @@ declare_features! (
(accepted, const_constructor, "1.40.0", Some(61456), None),
/// Allows the use of `#[cfg(doctest)]`, set when rustdoc is collecting doctests.
(accepted, cfg_doctest, "1.40.0", Some(62210), None),
/// Allows relaxing the coherence rules such that
/// `impl<T> ForeignTrait<LocalType> for ForeignType<T>` is permitted.
(accepted, re_rebalance_coherence, "1.40.0", Some(55437), None),
ohadravid marked this conversation as resolved.
Show resolved Hide resolved

// -------------------------------------------------------------------------
// feature-group-end: accepted features
Expand Down
4 changes: 0 additions & 4 deletions src/libsyntax/feature_gate/active.rs
Original file line number Diff line number Diff line change
Expand Up @@ -469,10 +469,6 @@ declare_features! (
/// Allows exhaustive integer pattern matching on `usize` and `isize`.
(active, precise_pointer_size_matching, "1.32.0", Some(56354), None),

/// Allows relaxing the coherence rules such that
/// `impl<T> ForeignTrait<LocalType> for ForeignType<T>` is permitted.
(active, re_rebalance_coherence, "1.32.0", Some(55437), None),

/// Allows using `#[ffi_returns_twice]` on foreign functions.
(active, ffi_returns_twice, "1.34.0", Some(58314), None),

Expand Down
11 changes: 0 additions & 11 deletions src/test/ui/coherence/coherence-all-remote.re.stderr

This file was deleted.

6 changes: 1 addition & 5 deletions src/test/ui/coherence/coherence-all-remote.rs
Original file line number Diff line number Diff line change
@@ -1,13 +1,9 @@
// aux-build:coherence_lib.rs
// revisions: old re

#![cfg_attr(re, feature(re_rebalance_coherence))]

extern crate coherence_lib as lib;
use lib::Remote1;

impl<T> Remote1<T> for isize { }
//[old]~^ ERROR E0210
//[re]~^^ ERROR E0210
//~^ ERROR E0210

fn main() { }
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
error[E0210]: type parameter `T` must be used as the type parameter for some local type (e.g., `MyStruct<T>`)
--> $DIR/coherence-all-remote.rs:9:6
--> $DIR/coherence-all-remote.rs:6:6
|
LL | impl<T> Remote1<T> for isize { }
| ^ type parameter `T` must be used as the type parameter for some local type
Expand Down
3 changes: 0 additions & 3 deletions src/test/ui/coherence/coherence-bigint-int.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,5 @@
// run-pass
// aux-build:coherence_lib.rs
// revisions: old re

#![cfg_attr(re, feature(re_rebalance_coherence))]

// pretty-expanded FIXME #23616

Expand Down
11 changes: 0 additions & 11 deletions src/test/ui/coherence/coherence-bigint-param.old.stderr

This file was deleted.

6 changes: 1 addition & 5 deletions src/test/ui/coherence/coherence-bigint-param.rs
Original file line number Diff line number Diff line change
@@ -1,15 +1,11 @@
// aux-build:coherence_lib.rs
// revisions: old re

#![cfg_attr(re, feature(re_rebalance_coherence))]

extern crate coherence_lib as lib;
use lib::Remote1;

pub struct BigInt;

impl<T> Remote1<BigInt> for T { }
//[old]~^ ERROR type parameter `T` must be used as the type parameter for some local type
//[re]~^^ ERROR E0210
//~^ ERROR E0210

fn main() { }
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
error[E0210]: type parameter `T` must be used as the type parameter for some local type (e.g., `MyStruct<T>`)
--> $DIR/coherence-bigint-param.rs:11:6
--> $DIR/coherence-bigint-param.rs:8:6
|
LL | impl<T> Remote1<BigInt> for T { }
| ^ type parameter `T` must be used as the type parameter for some local type
Expand Down
3 changes: 0 additions & 3 deletions src/test/ui/coherence/coherence-bigint-vecint.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,5 @@
// run-pass
// aux-build:coherence_lib.rs
// revisions: old re

#![cfg_attr(re, feature(re_rebalance_coherence))]

// pretty-expanded FIXME #23616

Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
@@ -1,7 +1,3 @@
// revisions: old re

#![cfg_attr(re, feature(re_rebalance_coherence))]

use std::fmt::Debug;
use std::default::Default;

Expand All @@ -26,8 +22,7 @@ impl<T:Even> MyTrait for T {
}

impl<T:Odd> MyTrait for T {
//[old]~^ ERROR E0119
//[re]~^^ ERROR E0119
//~^ ERROR E0119

fn get(&self) -> usize { 0 }
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
error[E0119]: conflicting implementations of trait `MyTrait`:
--> $DIR/coherence-blanket-conflicts-with-blanket-implemented.rs:28:1
--> $DIR/coherence-blanket-conflicts-with-blanket-implemented.rs:24:1
|
LL | impl<T:Even> MyTrait for T {
| -------------------------- first implementation here
Expand Down
Loading