2093 infer outlives requirements#50070
Conversation
emilyalbini
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 |
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
There was a problem hiding this comment.
The \ here is unfortunate -- where does that come from...?
nikomatsakis
left a comment
nikomatsakis
left a comment
There was a problem hiding this comment.
Looking good! Left a few notes.
src/librustc_typeck/outlives/mod.rs
Outdated
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();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. |
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
toidiu
force-pushed
the
ak-2093-outlives
branch
2 times, most recently
from
a31f9f6 to
f8639a1
Compare
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
}
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 |
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
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
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
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 |
7 participants
Tracking issue: #44493
RFC: rust-lang/rfcs#2093
rustc_attrsflagRequirePredicatestypedynTraitunused,dead_codeand etc..