-
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
[12/12] On-demand type-checking, const-evaluation, MIR building & const-qualification. #40008
Conversation
(rust_highfive has picked a reviewer for you, use r? to override) |
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 looks amazing as usual. My concern is mainly about the dependency graph management. The handling here is definitely dubious, but maybe we want to address it in follow-up PRs.
let default = def.default.map(|default| { | ||
type_variable::Default { | ||
let default = if def.has_default { | ||
let default = self.tcx.item_type(def.def_id); |
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.
Man, we really need to document all the things that can have def-ids and what each table means for each kind of thing. Is this written down anywhere? I guess not.
src/librustc_resolve/lib.rs
Outdated
@@ -718,6 +754,11 @@ enum RibKind<'a> { | |||
|
|||
// We passed through a `macro_rules!` statement with the given expansion | |||
MacroDefinition(Mark), | |||
|
|||
// All bindings in this rib aretype parameters that can't be used |
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.
Nit: s/aretype/are type/
pub struct Foo<Bar=Bar>; //~ ERROR E0128 | ||
//~| NOTE defaulted type parameters cannot be forward declared | ||
pub struct Foo<Bar=Bar>(Bar); //~ ERROR E0128 | ||
//~| NOTE defaulted type parameters cannot be forward declared |
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.
Is there a test demonstrating that bounds can reference "future" type parameters? e.g., fn foo<T: Foo<U>, U>() { ... }
? It seemed to me that your resolve changes were intending to keep this working...
} | ||
} | ||
|
||
impl<'tcx, T: Default> Value<'tcx> for T { |
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'm not sure how I feel about this. Reporting "some random but otherwise valid" value when an error occurs seems likely to lead to downstream errors that don't make any sense. For example, if the result is Vec<Predicate>
, and we return vec![]
, then we'll generate a bunch of errors that are based on the fact that we see no predicates. If we returned instead Result<Vec<Predicate>, ErrorReported>
, we would know that we can suppress errors in the downstream work.
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.
Regardless having a blanket impl for all T: Default
seems like it will do surprising things.
pub fn destructor(&self) -> Option<DefId> { | ||
self.destructor.get() | ||
pub fn destructor(&self, tcx: TyCtxt<'a, 'gcx, 'tcx>) -> Option<DefId> { | ||
if self.flags.get().intersects(AdtFlags::IS_DTOR_VALID) { |
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 like it will create incorrect dependencies in the dep-graph. The destructor
field is shared mutable state -- it needs to be tracked. Consider this:
TASK A: calls foo.destructor()
. As this is the first call, it will create read edges to TASK A, then update the destructor cache.
TASK B: calls foo.destructor()
-- cache is populated, no read edges result.
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 is pre-existing though. The correct solution IMO is to query the trait system but fixing all the users is a bit tricky.
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.
ah, it's pre-existing? ok, I guess we can live with it then. I thought I closed up the cells in there, but I guess not.
@@ -155,6 +164,13 @@ impl<'a, 'gcx, 'tcx> TraitDef { | |||
assert!(impl_def_id.is_local()); | |||
let was_new = self.record_impl(tcx, impl_def_id, impl_trait_ref); | |||
assert!(was_new); | |||
|
|||
self.local_impl_count.set(self.local_impl_count.get() + 1); |
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 is also shared mutable state ... as are a number of other fields in the TraitDef
etc ... I'm not sure whether it will cause problems here. The hack ignoring edges for force
may... be ok... at least for now. I was figuring I would take a stab at cleaning up that stuff once this PR lands...
tcx: TyCtxt<'a, 'tcx, 'tcx>, | ||
hooks: &mut [Box<for<'s> MirPassHook<'s>>]) | ||
{ | ||
let def_ids = tcx.maps.mir.borrow().keys(); |
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.
it is pretty dubious to iterate over keys -- seems like we should talk over this -- but it may make sense to land the PR and then do a follow-up
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.
Yeah things like this should probably be switched to iterate over bodies instead.
@eddyb you seen the travis failures? |
Yeah, I've been meaning to fix them - they're just diagnostics changes in UI tests. |
@eddyb ok, so to summarize what we said on IRC (and "for the record"):
So -- yeah, r=me, let's just land this thing already. |
@bors r=nikomatsakis |
📌 Commit 97d84c8 has been approved by |
@bors r=nikomatsakis |
📌 Commit 002a449 has been approved by |
Rollup of 28 pull requests - Successful merges: #39859, #39864, #39888, #39903, #39905, #39914, #39945, #39950, #39953, #39961, #39980, #39988, #39993, #39995, #40019, #40020, #40022, #40024, #40025, #40026, #40027, #40031, #40035, #40037, #40038, #40064, #40069, #40086 - Failed merges: #39927, #40008, #40047
☔ The latest upstream changes (presumably #40091) made this pull request unmergeable. Please resolve the merge conflicts. |
Note to self: never use the GitHub conflict resolution UI, it makes merge commits. |
@bors p=20 |
💔 Test failed - status-travis |
Welp, it was me: I compared the log with a previous build and it went from |
I can't reproduce a slowdown locally, so I'm still confused. EDIT: No wonder, |
@bors retry
|
[12/12] On-demand type-checking, const-evaluation, MIR building & const-qualification. _This is the last of a series ([prev](#38813)) of patches designed to rework rustc into an out-of-order on-demand pipeline model for both better feature support (e.g. [MIR-based](https://github.com/solson/miri) early constant evaluation) and incremental execution of compiler passes (e.g. type-checking), with beneficial consequences to IDE support as well. If any motivation is unclear, please ask for additional PR description clarifications or code comments._ <hr> As this contains all of the changes that didn't fit neatly into other PRs, I'll be elaborating a bit: ### User-facing changes * when determining whether an `impl Trait` type implements an auto-trait (e.g. `Send` or `Sync`), the function the `impl Trait` came from has to be inferred and type-checking, disallowing cycles * this results from not having an obvious place to put the "deferred obligation" in on-demand atm * while we could model side-effects like that and "post-processing passes" better, it's still more limiting than being able to know the result in the original function (e.g. specialization) *and* there are serious problems around region-checking (if a `Send` impl required `'static`, it wasn't enforced) * early const-eval requires type-checking and const-qualification to be performed first, which means: * you get the intended errors before (if any) constant evaluation error that is simply fallout * associated consts should always work now, and `const fn` type parameters are properly tracked * don't get too excited, array lengths still can't depend on type parameters * #38864 works as intended now, with `Self` being allowed in `impl` bounds * #32205 is largely improved, with associated types being limited to "exact match" `impl`s (as opposed to traversing the specialization graph to resolve unspecified type parameters to their defaults in another `impl` or in the `trait`) *while* checking for overlaps building the specialization graph for that trait - once all the trait impls' have been checked for coherence (including ahead-of-time/on-demand), it's uniform * [crater report](https://gist.github.com/eddyb/bbb869072468c7e08d6d808e75938051) looks clean (aside from `clippy` which broke due to `rustc` internal changes) ### Compiler-internal changes * `ty::Generics` * no longer contains the actual type parameter defaults, instead they're associated with the type parameter's `DefId`, like associated types in a trait definition * this allows computing `ty::Generics` as a leaf (reading only its own HIR) * holds a mapping from `DefIndex` of type parameters to their indices * `ty::AdtDef` * only tracks `#[repr(simd)]` in its `ReprOptions` `repr` field * doesn't contain `enum` discriminant values, but instead each variant either refers to either an explicit value for its discriminant, or the distance from the last explicit discriminant, if any * the `.discriminants(tcx)` method produces an iterator of `ConstInt` values, looking up explicit discriminants in a separate map, if necessary * this allows computing `ty::AdtDef` as a leaf (reading only its own HIR) * Small note: the two above (`Generics`, `AdtDef`), `TraitDef` and `AssociatedItem` should probably end up as part of the HIR, eventually, as they're trivially constructed from it * `ty::FnSig` * now also holds ABI and unsafety, alongside argument types, return type and C variadicity * `&ty::BareFnTy` and `ty::ClosureTy` have been replaced with `PolyFnSig = Binder<FnSig>` * `BareFnTy` was interned and `ClosureTy` was treated as non-trivial to `Clone` because they had a `PolyFnSig` and so used to contain a `Vec<Ty>` (now `&[Ty]`) * `ty::maps` * all the `DepTrackingMap`s have been grouped in a structure available at `tcx.maps` * when creating the `tcx`, a set of `Providers` (one `fn` pointer per map) is required for the local crate, and one for all other crates (i.e. metadata loading), `librustc_driver` plugging the various crates (e.g. `librustc_metadata`, `librustc_typeck`, `librustc_mir`) into it * when a map is queried and the value is missing, the appropriate `fn` pointer from the `Providers` of that crate is called with the `TyCtxt` and the key being queried, to produce the value on-demand * `rustc_const_eval` * demands both `typeck_tables` and `mir_const_qualif` (in preparation for miri) * tracks `Substs` in `ConstVal::Function` for `const fn` calls * returns `TypeckError` if type-checking has failed (or cases that can only be reached if it had) * this error kind is never reported, resulting in less noisy/redundant diagnostics * fixes #39548 (testcase by @larsluthman, taken from #39812, which this supersedes) * on-demand has so far been hooked up to: * `rustc_metadata::cstore_impl`: `ty`, `generics`, `predicates`, `super_predicates`, `trait_def`, `adt_def`, `variances`, `associated_item_def_ids`, `associated_item`, `impl_trait_ref`, `custom_coerce_unsized_kind`, `mir`, `mir_const_qualif`, `typeck_tables`, `closure_kind`, `closure_type` * `rustc_typeck::collect`: `ty`, `generics`, `predicates`, `super_predicates`, `type_param_predicates`, `trait_def`, `adt_def`, `impl_trait_ref` * `rustc_typeck::coherence`: `coherent_trait`, `coherent_inherent_impls` * `rustc_typeck::check`: `typeck_tables`, `closure_type`, `closure_kind` * `rustc_mir::mir_map`: `mir` * `rustc_mir::transform::qualify_consts`: `mir_const_qualif`
💔 Test failed - status-travis |
FWIW, investigation is ongoing in #40114. EDIT: Suspect found: |
@bors r=nikomatsakis Indeed, it appears those filemaps were adding As to what this PR does differently: when doing any request without an explicit Now, that may seem expensive, but it's empirically unmeasurable (except maybe this PR would speed other things up so what I'm seeing is speedup + slowdown => neutral), unless those cross-crate |
📌 Commit f702b20 has been approved by |
[12/12] On-demand type-checking, const-evaluation, MIR building & const-qualification. _This is the last of a series ([prev](#38813)) of patches designed to rework rustc into an out-of-order on-demand pipeline model for both better feature support (e.g. [MIR-based](https://github.com/solson/miri) early constant evaluation) and incremental execution of compiler passes (e.g. type-checking), with beneficial consequences to IDE support as well. If any motivation is unclear, please ask for additional PR description clarifications or code comments._ <hr> As this contains all of the changes that didn't fit neatly into other PRs, I'll be elaborating a bit: ### User-facing changes * when determining whether an `impl Trait` type implements an auto-trait (e.g. `Send` or `Sync`), the function the `impl Trait` came from has to be inferred and type-checking, disallowing cycles * this results from not having an obvious place to put the "deferred obligation" in on-demand atm * while we could model side-effects like that and "post-processing passes" better, it's still more limiting than being able to know the result in the original function (e.g. specialization) *and* there are serious problems around region-checking (if a `Send` impl required `'static`, it wasn't enforced) * early const-eval requires type-checking and const-qualification to be performed first, which means: * you get the intended errors before (if any) constant evaluation error that is simply fallout * associated consts should always work now, and `const fn` type parameters are properly tracked * don't get too excited, array lengths still can't depend on type parameters * #38864 works as intended now, with `Self` being allowed in `impl` bounds * #32205 is largely improved, with associated types being limited to "exact match" `impl`s (as opposed to traversing the specialization graph to resolve unspecified type parameters to their defaults in another `impl` or in the `trait`) *while* checking for overlaps building the specialization graph for that trait - once all the trait impls' have been checked for coherence (including ahead-of-time/on-demand), it's uniform * [crater report](https://gist.github.com/eddyb/bbb869072468c7e08d6d808e75938051) looks clean (aside from `clippy` which broke due to `rustc` internal changes) ### Compiler-internal changes * `ty::Generics` * no longer contains the actual type parameter defaults, instead they're associated with the type parameter's `DefId`, like associated types in a trait definition * this allows computing `ty::Generics` as a leaf (reading only its own HIR) * holds a mapping from `DefIndex` of type parameters to their indices * `ty::AdtDef` * only tracks `#[repr(simd)]` in its `ReprOptions` `repr` field * doesn't contain `enum` discriminant values, but instead each variant either refers to either an explicit value for its discriminant, or the distance from the last explicit discriminant, if any * the `.discriminants(tcx)` method produces an iterator of `ConstInt` values, looking up explicit discriminants in a separate map, if necessary * this allows computing `ty::AdtDef` as a leaf (reading only its own HIR) * Small note: the two above (`Generics`, `AdtDef`), `TraitDef` and `AssociatedItem` should probably end up as part of the HIR, eventually, as they're trivially constructed from it * `ty::FnSig` * now also holds ABI and unsafety, alongside argument types, return type and C variadicity * `&ty::BareFnTy` and `ty::ClosureTy` have been replaced with `PolyFnSig = Binder<FnSig>` * `BareFnTy` was interned and `ClosureTy` was treated as non-trivial to `Clone` because they had a `PolyFnSig` and so used to contain a `Vec<Ty>` (now `&[Ty]`) * `ty::maps` * all the `DepTrackingMap`s have been grouped in a structure available at `tcx.maps` * when creating the `tcx`, a set of `Providers` (one `fn` pointer per map) is required for the local crate, and one for all other crates (i.e. metadata loading), `librustc_driver` plugging the various crates (e.g. `librustc_metadata`, `librustc_typeck`, `librustc_mir`) into it * when a map is queried and the value is missing, the appropriate `fn` pointer from the `Providers` of that crate is called with the `TyCtxt` and the key being queried, to produce the value on-demand * `rustc_const_eval` * demands both `typeck_tables` and `mir_const_qualif` (in preparation for miri) * tracks `Substs` in `ConstVal::Function` for `const fn` calls * returns `TypeckError` if type-checking has failed (or cases that can only be reached if it had) * this error kind is never reported, resulting in less noisy/redundant diagnostics * fixes #39548 (testcase by @larsluthman, taken from #39812, which this supersedes) * on-demand has so far been hooked up to: * `rustc_metadata::cstore_impl`: `ty`, `generics`, `predicates`, `super_predicates`, `trait_def`, `adt_def`, `variances`, `associated_item_def_ids`, `associated_item`, `impl_trait_ref`, `custom_coerce_unsized_kind`, `mir`, `mir_const_qualif`, `typeck_tables`, `closure_kind`, `closure_type` * `rustc_typeck::collect`: `ty`, `generics`, `predicates`, `super_predicates`, `type_param_predicates`, `trait_def`, `adt_def`, `impl_trait_ref` * `rustc_typeck::coherence`: `coherent_trait`, `coherent_inherent_impls` * `rustc_typeck::check`: `typeck_tables`, `closure_type`, `closure_kind` * `rustc_mir::mir_map`: `mir` * `rustc_mir::transform::qualify_consts`: `mir_const_qualif`
☀️ Test successful - status-appveyor, status-travis |
Don't duplicate the extern providers once for each crate This should give a small perf improvement for small crates by avoiding a memcpy of a pretty big struct for each loaded crate. In addition would be useful for replacing the sequential `CrateNum` everywhere with the hash based `StableCrateId` introduced in rust-lang#81635, which would allow avoiding remapping of `CrateNum`'s when loading crate metadata. While this PR is not strictly needed for that, it is necessary to prevent a performance loss due to it. I think this duplication was done in rust-lang#40008 (which introduced the query system) to make it possible to compile multiple crates in a single session in the future. I think this is unlikely to be implemented any time soon. In addition this PR can easily be reverted if necessary to implement this.
This is the last of a series (prev) of patches designed to rework rustc into an out-of-order on-demand pipeline model for both better feature support (e.g. MIR-based early constant evaluation) and incremental execution of compiler passes (e.g. type-checking), with beneficial consequences to IDE support as well.
If any motivation is unclear, please ask for additional PR description clarifications or code comments.
As this contains all of the changes that didn't fit neatly into other PRs, I'll be elaborating a bit:
User-facing changes
impl Trait
type implements an auto-trait (e.g.Send
orSync
), the function theimpl Trait
came from has to be inferred and type-checking, disallowing cyclesSend
impl required'static
, it wasn't enforced)const fn
type parameters are properly trackedSelf
to appear in the where clause of trait impls' #38864 works as intended now, withSelf
being allowed inimpl
boundsimpl
s (as opposed to traversing the specialization graph to resolve unspecified type parameters to their defaults in anotherimpl
or in thetrait
) while checking for overlaps building the specialization graph for that trait - once all the trait impls' have been checked for coherence (including ahead-of-time/on-demand), it's uniformclippy
which broke due torustc
internal changes)Compiler-internal changes
ty::Generics
DefId
, like associated types in a trait definitionty::Generics
as a leaf (reading only its own HIR)DefIndex
of type parameters to their indicesty::AdtDef
#[repr(simd)]
in itsReprOptions
repr
fieldenum
discriminant values, but instead each variant either refers to either an explicit value for its discriminant, or the distance from the last explicit discriminant, if any.discriminants(tcx)
method produces an iterator ofConstInt
values, looking up explicit discriminants in a separate map, if necessaryty::AdtDef
as a leaf (reading only its own HIR)Generics
,AdtDef
),TraitDef
andAssociatedItem
should probably end up as part of the HIR, eventually, as they're trivially constructed from itty::FnSig
&ty::BareFnTy
andty::ClosureTy
have been replaced withPolyFnSig = Binder<FnSig>
BareFnTy
was interned andClosureTy
was treated as non-trivial toClone
because they had aPolyFnSig
and so used to contain aVec<Ty>
(now&[Ty]
)ty::maps
DepTrackingMap
s have been grouped in a structure available attcx.maps
tcx
, a set ofProviders
(onefn
pointer per map) is required for the local crate, and one for all other crates (i.e. metadata loading),librustc_driver
plugging the various crates (e.g.librustc_metadata
,librustc_typeck
,librustc_mir
) into itfn
pointer from theProviders
of that crate is called with theTyCtxt
and the key being queried, to produce the value on-demandrustc_const_eval
typeck_tables
andmir_const_qualif
(in preparation for miri)Substs
inConstVal::Function
forconst fn
callsTypeckError
if type-checking has failed (or cases that can only be reached if it had)rustc_metadata::cstore_impl
:ty
,generics
,predicates
,super_predicates
,trait_def
,adt_def
,variances
,associated_item_def_ids
,associated_item
,impl_trait_ref
,custom_coerce_unsized_kind
,mir
,mir_const_qualif
,typeck_tables
,closure_kind
,closure_type
rustc_typeck::collect
:ty
,generics
,predicates
,super_predicates
,type_param_predicates
,trait_def
,adt_def
,impl_trait_ref
rustc_typeck::coherence
:coherent_trait
,coherent_inherent_impls
rustc_typeck::check
:typeck_tables
,closure_type
,closure_kind
rustc_mir::mir_map
:mir
rustc_mir::transform::qualify_consts
:mir_const_qualif