-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Ak 44493 infer predicate #45298
Ak 44493 infer predicate #45298
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
☔ The latest upstream changes (presumably #45297) made this pull request unmergeable. Please resolve the merge conflicts. |
I'm confused -- what is the difference between this PR and #44857 ? |
@nikomatsakis sry for the confusion, this is supposed to be the 3rd PR where I actually implement the inferred outlives. I think #44857 is ready to merge and this PR is rebased from that one. |
@toidiu ah, nice, ok! |
@toidiu now that the other PR has merged, can you rebase this one? |
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.
good start :) left a few minor comments
src/librustc/ty/mod.rs
Outdated
// /// This relation tracks the dependencies between the variance of | ||
// /// various items. In particular, if `a < b`, then the variance of | ||
// /// `a` depends on the sources of `b`. | ||
// pub dependencies: TransitiveRelation<DefId>, |
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.
we should not need this; in fact, we can remove it from variance too, but that's a separate issue I suppose
src/librustc_typeck/outlives/mod.rs
Outdated
_ => Vec::new() | ||
} | ||
-> Rc<CratePredicatesMap> { | ||
Rc::new(Vec::new()) |
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.
shouldn't we be doing something like Rc::new(CratesPredicateMap::new())
(BTW, if it's useful to you, we can likely iterate within this PR for a while, before we land the next PR. Bors is awfully busy right now.) |
1840fb9
to
46fa0d4
Compare
@nikomatsakis yep totally fine with iterating here. I could use some guidance at this step. It seems like the function type_of is now encountering a I tried to find an appropriate method for Trait and came up with I was also unable to implement |
@toidiu yeah, |
src/librustc_typeck/outlives/mod.rs
Outdated
-> Vec<ty::Predicate<'tcx>> { | ||
Vec::new() | ||
|
||
if let ty::TyAdt(_def, _) = tcx.type_of(def_id).sty { |
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.
Invoking type_of
indiscriminanetly is not really ok. What we want to do is to first convert the def_id
into a local node-id, which will then allow us to query the HIR map to find out what kind of node we are looking at.
Something like this:
use hir::map as hir_map;
// Assert that this is a local node-id:
let node_id = self.tcx.as_local_node_id(def_id).unwrap();
match tcx.hir.get(node_id) {
hir_map::NodeItem(item) => match item.node {
hir::ItemTrait(..) => ..,
hir::ItemStruct(..) => ..,
hir::ItemEnum(..) => ..,
_ => ...,
}
_ => ...
}
Here, the get()
method returns a hir::map::Node
. You can see that it has many variants. I think we are just interested in struct/enum here, so that would be NodeItem
, and then we must further match on the item.node
to find out what kind of item we have. (As shown above.) For everything that is not a struct or enum, I think we can just return an empty vector.
4bff4ab
to
f559419
Compare
@nikomatsakis thanks for the help. now that this compiles i am going to work on the crate-wide computation. Will iterate here. |
@toidiu excellent, will watch =) let me know if you run into questions |
@toidiu how goes? had any time to hack on this? |
src/librustc_typeck/outlives/mod.rs
Outdated
} | ||
|
||
#[allow(dead_code)] | ||
fn inferred_outlives_crate <'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, def_id: DefId) |
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.
What is the def_id
parameter here supposed to refer to? My thought was that this function would infer the outlives
relationships for all structs/enums in the crate, therefore it probably doesn't need to take a def_id
(rather, in the body it will loop over the set of items to find those structs/enums).
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.
got it. yes I had a little mis-understanding about the function.
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.
btw planning to use tcx.hir.krate().visit_all_item_likes(&mut terms_cx)
along with impl ItemLikeVisitor
as we do in variance.
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.
sg, this function should presumably take a CrateNum
then -- at least, that's what we've done elsewhere. (I might be inclined to change the convention for singleton queries like this, but let's not do it in this PR.)
src/librustc_typeck/outlives/mod.rs
Outdated
_ => false, | ||
} | ||
) | ||
.collect::<Vec<_>>(); |
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 seems ok -- the idea is then that, in here, we will look over the structs/enums. We'll have (say) a DefIdMap
(a kind of hashtable keyed byDefId
) and, for each struct/enum with def-id S, we'll initialize map[S]
to be filtered
.
Though actually maybe it's better to do it slightly differently. Maybe the map[S]
should be initialized to an empty vector. Then we walk over the fields of S -- if we find that the struct S has a field F that references some other struct S2:
struct Foo<'a, T> { // Foo is S
bar: Bar<'a, T> // Bar is S2
}
struct Bar<'b, U: 'b> { ... }
then we will (a) compute the filtered, explicit predicates for Bar
-- this would include U: 'b
-- and add in any "extra" predicates for map[Bar]
(in this case, an empty vector). This is the set of requirements that Bar
imposes to be well-formed; if we translate those back to Foo
by applying the substitution [U => T, 'b => 'a]
, then we get the requirements T: 'a
, which would get added to Map[Foo]
as one of the implicits we inferred.
In other words, it makes sense to separate out the filtered explicits since we don't really want to add them to the list of implicits, or else we're just duplicating stuff.
Anyway, this is kind of high-level -- would it be helpful if I spelled it out in more low-level detail? Does what I wrote here make sense to you?
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.
high-level is actually good and one of the things that I am still a bit fuzzy on. I am going to layout the code-plan as my next step; which should help. After that the implementation details should become easier and plug-and-play.
@nikomatsakis just pushed some changes for how i envision the structure. it definitely doesnt compile and is a mix of rust+pseudo code. The point of this is that I wanted to get some confirmation that i am thinking about this correctly before expanding and and fixing syntax. |
//now infer outlive prediacates from the filtered predicates | ||
for p in filtered { | ||
the_great_inferring(p) | ||
} |
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 doesn't look quite right. This is taking a single item (def_id
) -- e.g., a single struct or enum type -- and computing its explicit predicates, and then iterating over those predicates. What we want to do be doing is something more like this.
☔ The latest upstream changes (presumably #45825) made this pull request unmergeable. Please resolve the merge conflicts. |
@toidiu I was wondering -- it seems like you're having a hard time getting started here, which is totally understandable. Maybe we could schedule some time and do a kind of "pair programming" session just to get things off the ground? |
@nikomatsakis that would be most helpful. Will message you over irc to discuss details. |
35e98b1
to
ba0e3fd
Compare
// for the type. | ||
//for def_id in all_types() { | ||
// let explicit_predicates = tcx.explicit_predicates(def_id); | ||
let def_id = DefId.local(item.hir_id.owner); |
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.
@nikomatsakis I dont know how to retrieve a DefId here from Item. Can you suggest a method from this struct. https://github.com/rust-lang/rust/blob/master/src/librustc/hir/mod.rs#L1816
dc93e94
to
9e8f8a4
Compare
let local_explicit_predicate = self.tcx.explicit_predicates_of(def_id); | ||
|
||
let filtered_predicates = local_explicit_predicate | ||
.predicates.into_iter() |
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.
I wonder if I should be doing into_iter
or iter()
since that calculation will be cached and used by incremental?
crate_num: CrateNum, | ||
} | ||
|
||
impl<'tcx, 'a, 'p, 'v> ItemLikeVisitor<'v> for ExplicitVisitor<'tcx, 'a, 'p> { |
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.
@nikomatsakis I am getting lifetime errors but not sure how to fix this.
error[E0495]: cannot infer an appropriate lifetime for lifetime parameter `'tcx` due to conflicting requirements
--> src/librustc_typeck/outlives/explicit.rs:59:49
|
59 | let local_explicit_predicate = self.tcx.explicit_predicates_of(def_id);
| ^^^^^^^^^^^^^^^^^^^^^^
|
note: first, the lifetime cannot outlive the lifetime 'tcx as defined on the impl at 54:1...
--> src/librustc_typeck/outlives/explicit.rs:54:1
|
54 | / impl<'tcx, 'a, 'p, 'v> ItemLikeVisitor<'v> for ExplicitVisitor<'tcx, 'a, 'p> {
55 | | fn visit_item(&mut self, item: &hir::Item) {
56 | |
57 | | //let def_id = LocalDefId::new(item.hir_id.owner).to_def_id();
... |
77 | | }
78 | | }
| |_^
note: ...so that types are compatible (expected rustc::ty::TyCtxt<'_, '_, '_>, found rustc::ty::TyCtxt<'tcx, 'tcx, 'tcx>)
--> src/librustc_typeck/outlives/explicit.rs:59:49
|
59 | let local_explicit_predicate = self.tcx.explicit_predicates_of(def_id);
| ^^^^^^^^^^^^^^^^^^^^^^
note: but, the lifetime must be valid for the lifetime 'p as defined on the impl at 54:1...
--> src/librustc_typeck/outlives/explicit.rs:54:1
|
54 | / impl<'tcx, 'a, 'p, 'v> ItemLikeVisitor<'v> for ExplicitVisitor<'tcx, 'a, 'p> {
55 | | fn visit_item(&mut self, item: &hir::Item) {
56 | |
57 | | //let def_id = LocalDefId::new(item.hir_id.owner).to_def_id();
... |
77 | | }
78 | | }
| |_^
note: ...so that expression is assignable (expected std::rc::Rc<std::vec::Vec<rustc::ty::Predicate<'p>>>, found std::rc::Rc<std::vec::Vec<rustc::ty::Predicate<'_>>>)
--> src/librustc_typeck/outlives/explicit.rs:70:49
|
70 | self.explicit_predicates.insert(def_id, Rc::new(filtered_predicates));
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
@bors r+ |
📌 Commit c88e59a has been approved by |
⌛ Testing commit c88e59abd8453401ef6679c5c3119a8de49606db with merge 7646fc426428f6915fe3b20734615580b9730b54... |
💔 Test failed - status-travis |
Your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. 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
Your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. 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 |
@@ -139,7 +140,11 @@ define_maps! { <'tcx> | |||
[] fn variances_of: ItemVariances(DefId) -> Lrc<Vec<ty::Variance>>, | |||
|
|||
/// Maps from def-id of a type to its (inferred) outlives. | |||
[] fn inferred_outlives_of: InferredOutlivesOf(DefId) -> Vec<ty::Predicate<'tcx>>, | |||
[] fn inferred_outlives_of: InferredOutlivesOf(DefId) -> Lrc<Vec<ty::Predicate<'tcx>>>, |
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.
I think the Lrc
is causing the error if executed with parallel on. Need to update the outlives code to return an Lrc
rather than a Rc
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.
sounds right
☔ The latest upstream changes (presumably #48528) made this pull request unmergeable. Please resolve the merge conflicts. |
…um, union, and projection types. added a feature gate and tests for these scenarios.
c88e59a
to
6a229cb
Compare
@bors r+ |
📌 Commit 6a229cb has been approved by |
Ak 44493 infer predicate **WIP** Implements #44493 Things to do: - [x] add feature gate and appropriate tests (see [forge](https://forge.rust-lang.org/feature-guide.html) for some details) - [x] add a unit testing system similar to `#[rustc_variance]` - [x] to see how, maybe `rg rustc_variance` and take some notes - [ ] add more tests: - [x] we need to decide how to handle `struct Foo<'a, T> { x: &'a T::Item }` - [x] handle explicit predicates on types - [ ] handle explicit predicates on `dyn Trait` (this could be put off to a follow-up PR) - [ ] handle explicit predicates on projections (this could be put off to a follow-up PR)
☀️ Test successful - status-appveyor, status-travis |
WIP Implements #44493
Things to do:
#[rustc_variance]
rg rustc_variance
and take some notesstruct Foo<'a, T> { x: &'a T::Item }
dyn Trait
(this could be put off to a follow-up PR)