Plumb inference obligations through selection #33301
Conversation
Candidate assembly during selection should not affect the inference context.
|
☔ The latest upstream changes (presumably #33138) made this pull request unmergeable. Please resolve the merge conflicts. |
|
|
||
| /// Obligations generated during inference. If None, no obligations are to be collected (e.g. | ||
| /// this is swapped out to a `None` when probing). | ||
| inferred_obligations: Option<PredicateObligations<'tcx>> |
There was a problem hiding this comment.
Hmm, so I don't know how I feel about this field being in here. I feel like in the case of SelectionContext, because it doesn't represent a single atomic operation, it'd be better to just thread the obligations through explicitly, rather than collecting them into a vector stored in the selcx.
The need to swap this to None when probing, for example, feels like a warning sign to me: probing shouldn't have to swap anything, we should be able to just throw away the obligations if we don't want them.
OTOH, I can imagine that this is more efficient, since we'll probably create fewer "throw-away" vectors.
There was a problem hiding this comment.
The last time I remember digging into this PR and trying to pull this field out and make the flow of obligations explicit, and it wound up leading me to problems in project.rs (the fix for this still hasn't landed; grr). So it might even be a bit hard to do this PR without those fixes having landed, or else we'd need to add more assert-is-empty points in project.rs.
There was a problem hiding this comment.
What about using SnapshotVec? It's roughly the same principle (even using fewer allocs, perhaps), but: would that alleviate concerns w.r.t. the swapping?
There was a problem hiding this comment.
Hmm. I might prefer SnapshotVec, yeah, coupled with some assertions
that the vector is empty each time we begin an operation. One thing that
worries me in the current setup is that we have None by default, so if
we forget to swap in a vec, things get dropped.
On Fri, May 06, 2016 at 03:11:31PM -0700, Masood Malekghassemi wrote:
@@ -78,8 +77,40 @@ pub struct SelectionContext<'cx, 'tcx:'cx> {
/// there is no type that the user could actually name that
/// would satisfy it. This avoids crippling inference, basically.
intercrate: bool,
+
- /// Obligations generated during inference. If None, no obligations are to be collected (e.g.
- /// this is swapped out to a
Nonewhen probing).- inferred_obligations: Option<PredicateObligations<'tcx>>
What about using
SnapshotVec? It's roughly the same principle (even using fewer allocs, perhaps), but: would that alleviate concerns w.r.t. the swapping?
You are receiving this because you were assigned.
Reply to this email directly or view it on GitHub:
https://github.com/rust-lang/rust/pull/33301/files/b7caae30c7389c3b92a8399577e007116c28f075..bf98976ec559d36f2aac16c1552155f417046afc#r62397872
There was a problem hiding this comment.
A'ight, and do you have a preference between this PR and #33451? The basic gist of the difference is that this one has a SelectionOk ok-result while the other one puts the inferred obligations into the nested member of each Vtable* variant.
There was a problem hiding this comment.
@soltanmm I'm not sure, been trying to find out on IRC, think we could sync up at some point?
|
There'll be a new PR with a tweaked approach soon-ish. |
An alternative to this implementation of plumbing is adding a
nestedfield to theVtableObjectDataandVtableFnPointervariants and dumping the inferred obligations right into thenestedfields of theVtable*types.... And by 'alternative', given that I'm not familiar with the particulars of caching and how candidates and/or their bounds may or may not get invalidated or may or may no longer be obligated to fulfill a bound in the future, I mean: 'possibly correct implementation and what's currently in this PR is not only inefficient but outright awfully wrong'.
Next (in-parallel?) step is removing the
consider_unification_despite_ambiguityin projection...r? @nikomatsakis