2093 infer outlives requirements #50070
Conversation
pietroalbini
added
the
S-waiting-on-review
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
|
||
|
|
||
| #[rustc_outlives] | ||
| struct Foo<'a, T> { //~ ERROR 17:1: 20:2: [Binder(OutlivesPredicate(T, ReEarlyBound(0, 'a)))] |
There was a problem hiding this comment.
Nit: you might prefer to have the "error message" be something very simple, like "rustc_outlives", and add the details via calls to note. this way, the //~ ERROR can be short and sweet, and the details are in the stderr file
| @@ -0,0 +1,21 @@ | |||
| error: ["/'a : /'a", "U : /'a"] | |||
There was a problem hiding this comment.
The \ here is unfortunate -- where does that come from...?
src/librustc_typeck/outlives/mod.rs
Outdated
| .collect(); | ||
|
|
||
| tcx.sess | ||
| .span_err(tcx.def_span(item_def_id), &format!("{:?}", &pred)); |
There was a problem hiding this comment.
Ah, I guess the \ somehow comes from this {:?}. I think i'd change this to:
let mut err = tcx.sess.span_err(..., "rustc_outlives");
for p in &pred {
err.note(p);
}
err.emit();| LL | | } | ||
| | |_^ | ||
|
|
||
| error[E0640]: [Binder(OutlivesPredicate(ReEarlyBound(0, 'a), ReEarlyBound(0, 'a))), Binder(OutlivesPredicate(U, ReEarlyBound(0, 'a)))] |
There was a problem hiding this comment.
doesn't really matter, but during inference we should probably filter out identity predicates like 'a: 'a, just for efficiency / to make things easier to read
nikomatsakis
added
S-waiting-on-author
|
☔ The latest upstream changes (presumably #50016) made this pull request unmergeable. Please resolve the merge conflicts. |
| @@ -0,0 +1,4 @@ | |||
| error: internal compiler error: librustc/ty/subst.rs:493: Type parameter `T/#2` (T/2) out of range when substituting (root type=Some(T)) substs=[ReEarlyBound(0, 'x)] | |||
There was a problem hiding this comment.
@nikomatsakis Not sure how to tackle this error. From some debugging it seems like we are trying to infer Box: 'x here. Here is a pastebin link to my debug logs. https://pastebin.com/3fMkHPjb
These are the new debug statements I added: https://github.com/rust-lang/rust/pull/50070/files#diff-a43200d3360f31959b9f923bc33ed1edR251
| // type `ty`. `ty` is being passes here as | ||
| // a dummy value since there is no concrete | ||
| // `Self` for a dyn Trait at this stage. | ||
| ex_trait_ref.with_self_ty(tcx, ty).substs(), |
toidiu
force-pushed
the
ak-2093-outlives
branch
2 times, most recently
from
a31f9f6 to
f8639a1
Compare
| debug!("predicate = {:?}", &predicate); | ||
|
|
||
| // FIXME explain why its ok to ignore Self. | ||
| if let (Some(self_ty), UnpackedKind::Type(ty)) = (ignore_self_ty, predicate.0.unpack()) |
There was a problem hiding this comment.
@nikomatsakis I need some help with explaining why its ok to ignore Self here.
Even though I am passing in Self and I was thinking more on our conversation about the different ways we could infer for TyDynamic:
- pass Self and dont infer
- pass self, infer and then remove Self predicates
- pass a Fake ty and dont worry about inferring
The 'Fake ty' seems like the cleanest choice since it will never be in explicit_map. By using Self or any other type, we need to mentally check and comment why its ok to not infer.
There was a problem hiding this comment.
This isn't quite what I had in mind. I was thinking more something like this:
ignore_self_tywould be a boolean- we would do:
if let Some(explicit_predicates) = explicit_map.get(def_id) {
for outlives_predicate in explicit_predicates.iter() {
// Careful: If we are inferring the effects of a `dyn Trait<..>`
// type, then when we look up the predicates for `Trait`,
// we may find some that reference `Self`. e.g., perhaps the
// definition of `Trait` was:
//
// ```
// trait Trait<'a, T> where Self: 'a { .. }
// ```
//
// we want to ignore such predicates here, because
// there is no type parameter for them to affect. Consider
// a struct containing `dyn Trait`:
//
// ```
// struct MyStruct<'x, X> { field: Box<dyn Trait<'x, X>> }
// ```
//
// The `where Self: 'a` predicate refers to the *existential, hidden type*
// that is represented by the `dyn Trait`, not to the `X` type parameter
// (or any other generic parameter) declared on `MyStruct`.
//
// Note that we do this check for self **before** applying `substs`. In the
// case that `substs` come from a `dyn Trait` type, our caller will have
// included `Self = dyn Trait<'x, X>` as the value for `Self`. If we were to apply
// the substs, and not filter this predicate, we might then falsely conclude
// that e.g. `X: 'x` was a reasonable inferred requirement.
if let UnpackedKind::Type(ty) = predicate.0.unpack() {
if ignore_self_ty && ty.is_self() {
continue;
}
}
let predicate = outlives_predicate.subst(tcx, substs);
... // as before
}
| // to apply the substs, and not filter this predicate, we might then falsely | ||
| // conclude that e.g. `X: 'x` was a reasonable inferred requirement. | ||
| if let UnpackedKind::Type(ty) = predicate.0.unpack() { | ||
| if ty.is_self() && ignore_self_ty { |
There was a problem hiding this comment.
@nikomatsakis think we should have a test for ignore_self_ty and that it should be work for dyn but not for other things. I'll have some time this week to look at things.
|
Seems like a problem — this will mess with incremental compilation too. I wonder why that is. Usually it's due to a use of a hashmap somewhere (instead of a |
| // included `Self = dyn Trait<'x, X>` as the value for `Self`. If we were | ||
| // to apply the substs, and not filter this predicate, we might then falsely | ||
| // conclude that e.g. `X: 'x` was a reasonable inferred requirement. | ||
| if let UnpackedKind::Type(ty) = predicate.0.unpack() { |
There was a problem hiding this comment.
This is checking predicate, which is after substitution, but it should be checking outlives_predicate -- before substitution.
src/librustc_typeck/outlives/mod.rs
Outdated
|
|
||
| let span = tcx.def_span(item_def_id); | ||
| let mut err = tcx.sess.struct_span_err(span, "rustc_outlives"); | ||
| for p in &pred { |
There was a problem hiding this comment.
Put a pred.sort() before this to get deterministic testing order
src/librustc_typeck/outlives/mod.rs
Outdated
|
|
||
| crate_map | ||
| let predicates = crate_map |
There was a problem hiding this comment.
Ah, I think the order problem comes from the fact that this is a hashmap here, and hence iteration order is undefined. That's.. probably good news, because I think its effects should be limited? In any case, for the test output, I would sort the vec of strings (see comment below).
|
I'm wondering though if the non-deterministic ordering will be a problem when we read the results of inference in the |
|
In that case, we could either use a |
|
@nikomatsakis Going to take a stab at that this weekend and see if I run into any road blocks. |
|
@nikomatsakis implemented |
kennytm
added
S-waiting-on-review
src/librustc/ty/subst.rs
Outdated
| @@ -30,7 +31,7 @@ use std::num::NonZeroUsize; | |||
| /// To reduce memory usage, a `Kind` is a interned pointer, | |||
| /// with the lowest 2 bits being reserved for a tag to | |||
| /// indicate the type (`Ty` or `Region`) it points to. | |||
| #[derive(Copy, Clone, PartialEq, Eq, Hash)] | |||
| #[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Hash)] | |||
There was a problem hiding this comment.
I don't really think PartialOrd deriving calls into Ord::cmp, but rather it compares the fields.
So when you need a custom impl for Ord, you also need to implement PartialOrd.
But if you have Ord::cmp, you can just call it from PartialOrd::partial_cmp.
toidiu
force-pushed
the
ak-2093-outlives
branch
2 times, most recently
from
33811e6 to
3ad9e55
Compare
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Add tests, documentation and attr for feature.
|
@nikomatsakis rebased to master and uses |
|
@bors r+ |
|
📌 Commit 3da7123 has been approved by |
bors
added
S-waiting-on-bors
|
⌛ Testing commit 3da7123 with merge 25144e8183e0d9cc8d166c279be956a559a48359... |
|
💔 Test failed - status-travis |
bors
added
S-waiting-on-review
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
1 similar comment
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
@bors retry |
bors
added
S-waiting-on-bors
|
@kennytm curious what caused this error? seems to me either bad cache or travis error? |
2093 infer outlives requirements Tracking issue: #44493 RFC: rust-lang/rfcs#2093 - [x] add `rustc_attrs` flag - [x] use `RequirePredicates` type - [x] handle explicit predicates on `dyn` Trait - [x] handle explicit predicates on projections - [x] more tests - [x] remove `unused`, `dead_code` and etc.. - [x] documentation
|
☀️ Test successful - status-appveyor, status-travis |
Tracking issue: #44493
RFC: rust-lang/rfcs#2093
rustc_attrsflagRequirePredicatestypedynTraitunused,dead_codeand etc..