-
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
Use the recorded types in MIR to determine generator auto-trait implementations #65782
Conversation
r? @varkor (rust_highfive has picked a reviewer for you, use r? to override) |
I've filed a separate PR for making all generators |
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 |
4b1e618
to
58c81c2
Compare
From PR #65783:
But Are you concerned that these optimization passes will have some effect on the calculation of If so, I see two possibilities:
Did you have something else in mind? |
r? @Zoxc |
The behaviour of the |
☔ The latest upstream changes (presumably #65845) made this pull request unmergeable. Please resolve the merge conflicts. |
The only way for this to happen would be:
This would mean that there's a bug in the MIR generation - we're leaking a local without an explicit It might make sense to add a debug check where we run the generator liveness analysis both before and after the removal of |
MIR optimization passes are supposed to be able to optimize away locals, and we want this to happen since it reduces the memory usage of generators. I also want MIR optimizations that will inline generators into other generators giving nice optimal state machines. I suggest you make a function that calculates the locals that is live across a generator. Basically just copy
This seem a bit sketchy to me. I'd break this cycle by changing the representation of generator types to not include it's witness or content, so you can refer to it without computing the witness or having type variables. There might be some problems with this approach that I don't see, but I'd like to keep all type checking in typeck. I had imagined something like where we construct MIR for the generator with type variables in it, only requiring just the types needed to correctly construct MIR. Starting out with something that requires all the types in the generator is easier though and it is why I required all types in the generator to be known in order to calculate generator interior. |
I'm not sure what you mean. The cycles are caused by the fact that we end up trying to compute |
My plan was to build the MIR for the generators during type checking, so if there is a |
@Zoxc: As @nikomatsakis described here, we can run into a cycle when doing this. |
Ping from triage: Thanks! |
Pinging again from triage: Thanks! |
f086c6c
to
7fe7bc1
Compare
I've addressed the merge conflicts. |
Thanks for addressing the merge conflicts @Aaron1011 |
@Aaron1011 Yeah, the From a high-level view my feedback would be to copy the I'm not currently able to review this in detail and I'm not too familiar with the trait system anyway, so I'll assign Niko for now. |
Co-Authored-By: Ralf Jung <post@ralfj.de>
Co-Authored-By: Ralf Jung <post@ralfj.de>
a743477
to
71c9dd5
Compare
☔ The latest upstream changes (presumably #67140) made this pull request unmergeable. Please resolve the merge conflicts. |
@@ -117,6 +117,8 @@ pub struct Body<'tcx> { | |||
/// to be created. | |||
pub generator_kind: Option<GeneratorKind>, | |||
|
|||
pub generator_interior_tys: Option<Vec<Ty<'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.
These shouldn't be stored in the MIR, and should instead be the return value of the query.
/// Compute the generator interior types for a given `DefId` | ||
/// (if it corresponds to a generator), for use in determining | ||
/// generator auto trait implementation | ||
query mir_generator_interior(_: DefId) -> &'tcx Steal<mir::BodyCache<'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.
This won't need to modify the MIR so it should return a &'tcx List<Ty<'tcx>>
(and can be renamed).
@@ -98,7 +98,12 @@ fn mir_keys(tcx: TyCtxt<'_>, krate: CrateNum) -> &DefIdSet { | |||
} | |||
|
|||
fn mir_built(tcx: TyCtxt<'_>, def_id: DefId) -> &Steal<BodyCache<'_>> { | |||
let mir = build::mir_build(tcx, def_id); | |||
let mut mir = build::mir_build(tcx, def_id); | |||
if let ty::Generator(..) = tcx.type_of(def_id).kind { |
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 still isn't quite what I meant. I've left a couple of comments explaining this.
We discussed this PR on Zulip today, in this thread. In short, I'm concerned about the interactions of this sort of change with specialization. It seems like we might able to resolve it but this is a fairly "fundamental" addition that we do want to consider carefully. |
Ping from triage: @Aaron1011 any progress on this? Should this be marked as blocked? |
I think it should, but we need to come up with a strategy to unblock. Not sure about that right now though. |
Triaged |
@Aaron1011 any updates? |
Hmm, I'm not sure what @Aaron1011 thinks, but I'm inclined to close this PR for the time being. I don't feel comfortable with this strategy yet, for all the reasons we talked about on Zulip. |
I guess I think I will feel more comfortable taking this approach once we've cleaned up our trait solver, so we are able to describe the way we are "deferring" auto-trait resolution in a more disciplined way. (I do think that, in our conversation, we discussed some vague ideas that were making sense.) |
I'm going to go ahead and close this PR, since I think the approach is "sufficiently blocked" that we won't be able to land it. We can always re-open. |
See #57017
When we construct a generator type, we build a
ty::GeneratorWitness
from a list of types that may live across a suspend point. This types is
used to determine the 'constitutent types' for a generator when
selecting an auto-trait predicate. Any types appearing in the
GeneratorWitness are required to implement the auto trait (e.g.
Send
or
Sync
).This analysis
is based on the HIR - as a result, it is unable to take liveness of
variables into account. This often results in unecessary bounds being
computing (e.g requiring that a
Rc
beSync
even if it is droppedbefore a suspend point), making generators and
async fn
s much lessergonomic to write.
This commit uses the generator MIR to determine the actual 'constituent
types' of a generator. Specifically, a type in the generator witness is
considered to be a 'constituent type' of the witness if it appears in
the
field_tys
of the computedGeneratorLayout
. Any type which isstored across an suspend point must be a constituent type (since it could
be used again after a suspend), while any type that is not stored across
a suspend point cannot possible be a consituent type (since it is
impossible for it to be used again).
By re-using the existing generator layout computation logic, we get some
nice properties for free:
constituent types
scope extends across an await point (assuming that they are never used
after an await point).
Note that this only affects
ty::GeneratorWitness
, notty::Generator
itself. Upvars (captured types from the parent scope)are considered to be constituent types of the base
ty::Generator
, notthe inner
ty::GeneratorWitness
. This means that upvars are alwaysconsidered constituent types - this is because by defintion, they always
live across the first implicit suspend point.
Implementation:
The most significant part of this PR is the introduction of a new
'delayed generator witness mode' to
TraitEngine
. As @nikomatsakispointed out, attmepting to compute generator MIR during type-checking
results in the following cycle:
predicate of the form
<generator>: AutoTrait
TraitEngine
, and attempt to fulfill it.for
<generator>
type_of(generator_def_id)
, whichresults in us attempting to type-check the generator's parent function.
To break this cycle, we defer processing of all auto-trait predicates
involving
ty::GeneratorWitness
. These predicates are recorded in theTypeckTables
for the parent function. During MIR type-checking of theparent function, we actually attempt to fulfill these predicates,
reporting any errors that occur.
The rest of the PR is mostly fallout from this change:
ty::GeneratorWitness
now stores theDefId
of its generator. Thisallows us to retrieve the MIR for the generator when
SelectionContext
processes a predicate involving a
ty::GeneratorWitness
PredicateObligations
inTypeckTables
, severaldifferent types have now become
RustcEncodable
/RustcDecodable
. Theseare purely mechanical changes (adding new
#[derives]
), with oneexception - a new
SpecializedDecoder
impl forList<Predicate>
.This was essentialy identical to other
SpecializedDecoder
imps, but itwould be good to have someone check it over.
Predicate
, we move it from oneInferCtxt
to another. This requires us to prevent any inferencevariables from leaking out from the first
InferCtxt
- if used inanother
InferCtxt
, they will either be non-existent or refer to thethe wrong variable. Fortunately, the predicate itself has no region
variables - the
ty::GeneratorWitness
has only late-bound regions,while auto-traits have no generic parameters whatsoever.
However, we still need to deal with the
ObligationCause
stored by thePredicateObligation
. AnObligationCause
(or a nested cause) may haveany number of region variables stored inside it (e.g. from stored
types). Luckily,
ObligationCause
is only uesd for error reporting, sowe can safely erase all regions variables from it, without affecting the
actual processing of the obligation.
To accomplish this, I took the somewhat unusual approach of implementing
TypeFoldable
forObligationCause
, but did not change theTypeFoldable
implementation of
Obligation
to fold its containedObligationCause. Other than this one odd case, all other callers of
TypeFoldablehave no interest in folding an
ObligationCause. As a result, we explicitly fold the
ObligationCausewhen computing our deferred generator witness predicates. Since
ObligationCause` is onlyused for displaying error messages, the worst that can happen is that a
slightly odd error message is displayed to a user.
With this change, several tests now have fewer errors than they did
previously, due to the improved generator analysis. Unfortunately, this
does not resolve issue #64960. The MIR generator transformation stores
format temporaries in the generator, due to the fact that the
format!
macros takes a refernece to them. As a result, they are still considered
constituent types of the
GeneratorWitness
, and are still required toimplement
Send
and `Sync.