-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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
Initial support for auto traits with default bounds #120706
base: master
Are you sure you want to change the base?
Conversation
@rustbot author |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
So, what are the goals here:
|
The issue right now is that there are regressions, some previously passing code now fails due to cycles in trait solver or something similar, @Bryanskiy has been trying to investigate it, but without success. @lcnr, this is the work I've been talking about today. |
This comment has been minimized.
This comment has been minimized.
I want to reproduce regressions in CI @rustbot author |
it would be good to get an MVCE for the new cycles, otherwise debugging this without fully going through the PR is hard |
Support ?Trait bounds in supertraits and dyn Trait under a feature gate This patch allows `maybe` polarity bounds under a feature gate. The only language change here is that corresponding hard errors are replaced by feature gates. Example: ```rust #![feature(allow_maybe_polarity)] ... trait Trait1 : ?Trait { ... } // ok fn foo(_: Box<(dyn Trait2 + ?Trait)>) {} // ok fn bar<T: ?Sized + ?Trait>(_: &T) {} // ok ``` Maybe bounds still don't do anything (except for `Sized` trait), however this patch will allow us to [experiment with default auto traits](rust-lang#120706 (comment)). This is a part of the [MCP: Low level components for async drop](rust-lang/compiler-team#727)
Support ?Trait bounds in supertraits and dyn Trait under a feature gate This patch allows `maybe` polarity bounds under a feature gate. The only language change here is that corresponding hard errors are replaced by feature gates. Example: ```rust #![feature(allow_maybe_polarity)] ... trait Trait1 : ?Trait { ... } // ok fn foo(_: Box<(dyn Trait2 + ?Trait)>) {} // ok fn bar<T: ?Sized + ?Trait>(_: &T) {} // ok ``` Maybe bounds still don't do anything (except for `Sized` trait), however this patch will allow us to [experiment with default auto traits](rust-lang/rust#120706 (comment)). This is a part of the [MCP: Low level components for async drop](rust-lang/compiler-team#727)
Support ?Trait bounds in supertraits and dyn Trait under a feature gate This patch allows `maybe` polarity bounds under a feature gate. The only language change here is that corresponding hard errors are replaced by feature gates. Example: ```rust #![feature(allow_maybe_polarity)] ... trait Trait1 : ?Trait { ... } // ok fn foo(_: Box<(dyn Trait2 + ?Trait)>) {} // ok fn bar<T: ?Sized + ?Trait>(_: &T) {} // ok ``` Maybe bounds still don't do anything (except for `Sized` trait), however this patch will allow us to [experiment with default auto traits](rust-lang/rust#120706 (comment)). This is a part of the [MCP: Low level components for async drop](rust-lang/compiler-team#727)
@rustbot ready |
☔ The latest upstream changes (presumably #129665) made this pull request unmergeable. Please resolve the merge conflicts. |
I didn't know about this effort and wrote a Pre-RFC for exact same use case... I guess it is not needed, considering this? I'll attach it there (gist) |
I appreciate your effort of formalizing default generic bounds for new auto-traits. This is something I've planned to do, but I've struggled to solve one problem that was stated to me by decision makers, as to add a new auto-trait would require reducing ergonomics of existing libraries, that obviously don't implement these auto-traits, thus making experience of using those libraries or auto-traits unergonomic. I am working on something, that could potentially partially resolve this issue, but it also may be too complex of a change for the Rust language project anyway. |
I have tried to make a more specific argument, that adding the https://zetanumbers.github.io/book/myosotis.html Anyways, thank you. This suggestion wasn't written down before to this level of detail you have provided. |
@Bryanskiy any update on this PR? Thanks. |
@LFS6502 Hi, I'll come back to this PR next week. |
@rustbot ready |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some nits for this PR, otherwise it seems good given that it's all still gated behind a feature flag.
As you mention in your doc: adding default auto traits as super trait bounds causes a huge performance costs (mostly) by increasing the ParamEnv
size, it'll also cause a huge performance cost by just having to prove the auto trait bounds themselves. What's worse is that it will also just cause code to hit the recursion limit, causing many crates with deeply nested types to require changes.
Adding a fast path for auto traits which does not result in undesirable breakage or is difficult to reason about and may be unsound seems challenging.
I am personally very doubtful we will ever be able to stabilize additional default traits unless they stop at indirection like Sized
. I am fine with experimentation here, but do not want us to land any changes for it which are difficult to revert or greatly increase the complexity of code paths also used on stable/by other features. I would also like you to be careful when talking about this feature to make sure people are aware that actually stabilizing such traits will be challenging.
} | ||
_ => {} | ||
} | ||
}; | ||
|
||
if let Node::TraitItem(item) = node { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do not put this in gather_explicit_predicates_of
. This is for predicates written by the user. Push this one level out to the caller
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not clear to me. From what I see Sized
bounds are added in gather_explicit_predicates_of
, but they aren't explicit in the ordinary sense. Do they considered explicit because of the user written T: ?Sized
bounds? For default_auto_traits
we have the same situation, except that T: ?Trait
bounds can be used in more contexts, and here is one such context.
Perhaps you were confused by the implicit
word in new methods from bounds.rs
. Renamed them.
// FIXME(experimental_default_bounds): Default bounds on `Pointee::Metadata` | ||
// causes a normalization fail. | ||
if Some(trait_def_id.to_def_id()) == self.tcx().lang_items().pointee_trait() { | ||
return true; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels questionable to land this experimentation as long as this bug exists. It needs an explanation for why it won't affect user written impls as these bounds are otherwise a likely unacceptable breaking change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A double check showed that the bug had been fixed.
); | ||
} | ||
|
||
pub(crate) fn requires_implicit_supertraits( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this very much needs an explanation of what it's doing and why
/// For optimization purposes instead of adding default supertraits, bounds | ||
/// are added to the associative items: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// For optimization purposes instead of adding default supertraits, bounds | |
/// are added to the associative items: | |
/// For optimization purposes instead of adding default supertraits, bounds | |
/// are added to the associated items: |
} | ||
} | ||
|
||
fn check_for_implicit_trait( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this bool
return type is confusing without reading the source, please change the fn name to something like "do_not_provide_implicit_trait_bound" (in this case, invert the condition to avoid an unnecessary negation) or change the return type to a newtyped enum
@@ -46,7 +46,8 @@ impl<'tcx> LateLintPass<'tcx> for MultipleSupertraitUpcastable { | |||
.tcx | |||
.explicit_super_predicates_of(def_id) | |||
.iter_identity_copied() | |||
.filter_map(|(pred, _)| pred.as_trait_clause()); | |||
.filter_map(|(pred, _)| pred.as_trait_clause()) | |||
.filter(|pred| !cx.tcx.is_default_trait(pred.def_id())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hack for old editions. The dyn-compatible trait with multiple DefaultAutoTrait
super bounds will trigger the lint. But in old editions we can't relax bounds with ?DefaultAutoTrait
syntax, so it can be annoying.
But yeah, it's better to remove it.
@@ -31,6 +31,7 @@ where | |||
| ty::Float(_) | |||
| ty::FnDef(..) | |||
| ty::FnPtr(..) | |||
| ty::Foreign(..) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please keep this in a separate branch and add a comment/debug_assert that it only affects the default auto traits
@@ -1096,6 +1115,10 @@ where | |||
Some(self.forced_ambiguity(MaybeCause::Ambiguity)) | |||
} | |||
|
|||
// Backward compatibility for default auto traits. | |||
// Test: ui/traits/default_auto_traits/extern-types.rs | |||
ty::Foreign(..) if self.cx().is_default_trait(goal.predicate.def_id()) => check_impls(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this feels undesirable. Foreign types should not implement Leak
/Move
, should they?
given that we don't enable the feature on stable, I feel like this hack should be removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Foreign types from old editions should implement DefaultAutoTrait
's to keep backward compatibility(I have example in explainer). For new editions, they shouldn't, but an edition-based migration mechanism has not yet been designed/implemented.
@@ -598,8 +599,26 @@ fn receiver_is_dispatchable<'tcx>( | |||
ty::TraitRef::new_from_args(tcx, trait_def_id, args).upcast(tcx) | |||
}; | |||
|
|||
let caller_bounds = | |||
param_env.caller_bounds().iter().chain([unsize_predicate, trait_predicate]); | |||
// U: `experimental_default_bounds` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more hack for std build. Implicit T: DefaultAutoTrait
in DispatchFromDyn<T>
produces U: DefaultAutoTrait
goal. Have been removed.
@@ -2180,6 +2180,8 @@ options! { | |||
"Use WebAssembly error handling for wasm32-unknown-emscripten"), | |||
enforce_type_length_limit: bool = (false, parse_bool, [TRACKED], | |||
"enforce the type length limit when monomorphizing instances in codegen"), | |||
experimental_default_bounds: bool = (false, parse_bool, [TRACKED], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please open a tracking issue for this and list the current known issues
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
☔ The latest upstream changes (presumably #138388) made this pull request unmergeable. Please resolve the merge conflicts. |
This PR is part of "MCP: Low level components for async drop"
Summary: #120706 (comment)
Intro
Sometimes we want to use type system to express specific behavior and provide safety guarantees. This behavior can be specified by various "marker" traits. For example, we use
Send
andSync
to keep track of which types are thread safe. As the language develops, there are more problems that could be solved by adding new marker traits:SyncDrop
could be used Async destructors, async genericity and completion futures.Leak
(orForget
) trait.Move
trait instead of aPin
.All the traits proposed above are supposed to be auto traits implemented for most types, and usually implemented automatically by compiler.
For backward compatibility these traits have to be added implicitly to all bound lists in old code (see below). Adding new default bounds involves many difficulties: many standard library interfaces may need to opt out of those default bounds, and therefore be infected with confusing
?Trait
syntax, migration to a new edition may contain backward compatibility holes, supporting new traits in the compiler can be quite difficult and so forth. Anyway, it's hard to evaluate the complexity until we try the system on a practice.In this PR we introduce new optional lang items for traits that are added to all bound lists by default, similarly to existing
Sized
. The examples of such traits could beLeak
,Move
,SyncDrop
or something else, it doesn't matter much right now (further I will call themDefaultAutoTrait
's). We want to land this change into rustc under an option, so it becomes available in bootstrap compiler. Then we'll be able to do standard library experiments with the aforementioned traits without adding hundreds of#[cfg(not(bootstrap))]
s. Based on the experiments, we can come up with some scheme for the next edition, in which such bounds are added in a more targeted way, and not just everywhere.Most of the implementation is basically a refactoring that replaces hardcoded uses of
Sized
with iterating over a list of traits including bothSized
and the new traits when-Zexperimental-default-bounds
is enabled (or justSized
as before, if the option is not enabled).Default bounds for old editions
All existing types, including generic parameters, are considered
Leak
/Move
/SyncDrop
and can be forgotten, moved or destroyed in generic contexts without specifying any bounds. New types that cannot be, for example, forgotten and do not implementLeak
can be added at some point, and they should not be usable in such generic contexts in existing code.To both maintain this property and keep backward compatibility with existing code, the new traits should be added as default bounds everywhere in previous editions. Besides the implicit
Sized
bound contexts that includes supertrait lists and trait lists in trait objects (dyn Trait1 + ... + TraitN
). Compiler should also generate implicitDefaultAutoTrait
implementations for foreign types (extern { type Foo; }
) because they are also currently usable in generic contexts without any bounds.Supertraits
Adding the new traits as supertraits to all existing traits is potentially necessary, because, for example, using a
Self
param in a trait's associated item may be a breaking change otherwise:However, default supertraits can significantly affect compiler performance. For example, if we know that
T: Trait
, the compiler would deduce thatT: DefaultAutoTrait
. It also implies provingF: DefaultAutoTrait
for each fieldF
of typeT
until an explicit impl is be provided.If the standard library is not modified, then even traits like
Copy
orSend
would get these supertraits.In this PR for optimization purposes instead of adding default supertraits, bounds are added to the associated items:
It is not always possible to do this optimization because of backward compatibility:
or
Therefore,
DefaultAutoTrait
's are still being added to supertraits if theSelf
params or type bindings were found in the trait header.Trait objects
Trait objects requires explicit
+ Trait
bound to implement corresponding trait which is not backward compatible:So, for a trait object
dyn Trait
we should add an implicit bounddyn Trait + DefaultAutoTrait
to make it usable, and allow relaxing it with a question mark syntaxdyn Trait + ?DefaultAutoTrait
when it's not necessary.Foreign types
If compiler doesn't generate auto trait implementations for a foreign type, then it's a breaking change if the default bounds are added everywhere else:
We'll have to enable implicit
DefaultAutoTrait
implementations for foreign types at least for previous editions:Unresolved questions
New default bounds affect all existing Rust code complicating an already complex type system.
Also migration to a new edition could be quite ugly and enormous, but that's actually what we want to solve. For other issues there's a chance that they could be solved by a new solver.