-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Remove the ty
field from type system Const
s
#125958
Conversation
changes to the core type system Some changes occurred in compiler/rustc_codegen_cranelift cc @bjorn3 Some changes occurred in compiler/rustc_sanitizers cc @rust-lang/project-exploit-mitigations, @rcvalle HIR ty lowering was modified cc @fmease This PR changes Stable MIR cc @oli-obk, @celinval, @ouz-a Some changes occurred in match checking cc @Nadrieril Some changes occurred in exhaustiveness checking cc @Nadrieril Some changes occurred in need_type_info.rs cc @lcnr rustdoc-json-types is a public (although nightly-only) API. If possible, consider changing cc @CraftSpider, @aDotInTheVoid, @Enselic, @obi1kenobi Some changes occurred to the core trait solver cc @rust-lang/initiative-trait-system-refactor Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt The Miri subtree was changed cc @rust-lang/miri changes to the core type system Some changes occurred in src/tools/clippy cc @rust-lang/clippy Some changes occurred in src/librustdoc/clean/types.rs cc @camelid |
@@ -8,7 +8,7 @@ use serde::{Deserialize, Serialize}; | |||
use std::path::PathBuf; | |||
|
|||
/// rustdoc format-version. | |||
pub const FORMAT_VERSION: u32 = 29; | |||
pub const FORMAT_VERSION: u32 = 30; |
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.
:)
ty
field from Const
ty
field from type system Const
s
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
This comment has been minimized.
This comment has been minimized.
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.
partial review
// FIXME(BoxyUwU): this doesnt seem right | ||
ty::ConstKind::Placeholder(_) => { | ||
return ProcessResult::Changed(mk_pending(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.
will this just always stall? Is this even reachable? (and why don't we do the same as the new solver and look it up in the param env?
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.
In the new solver we canonicalize ConstKind::Param
to ConstKind::Placeholder
so we turn param env's with ConstArgHasType(T/#0, usize)
into ConstArgHasType(!0, usize)
so we will actually have candidate for placeholders. (But we still wont have any in the new solver for higher ranked consts, i.e. for<const N: usize>
) after instantiating the binder with placeholders) In the old solver we keep everything as ConstKind::Param
.
So both solvers do not handle for<const N: usize>
correctly in ConstArgHasType
. I meant to check whether this is an issue for some of the wfck stuff of gats which constructs ConstKind::Bound
but forgot.
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 also this won't stall it will just succeed (i.e. equivalent Ok(Certainty::Yes)
in the new solver)
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.
So I think this is probably unreachable because the only way we should get placeholders in the old solver is via instantiating a binder such as for<const N: usize>
. Those are only writeable with the unstable feature(non_lifetime_binders)
and that is.... very unstable (i.e. it doesn't really matter if it's a bit broken here). compare_impl_item
artificially creates a ParamEnv
with for<const N: usize>
however since its a ParamEnv
we're only ever going to end up instantiating that binder with inference variables. (we dont seem to normalize this env afaict).
I think I should probably just turn this into an unreachable!()
so worst case we hit an ICE here instead of unsoundness. But I think its okay... I'd like lcnr to double check my logic here 😅
42e1789
to
0946460
Compare
This comment has been minimized.
This comment has been minimized.
c.ty().walk().any(|arg| arg == dummy_self.into()) | ||
} | ||
// FIXME(associated_const_equality): We should walk the const instead of not doing anything | ||
ty::TermKind::Const(_) => false, |
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.
Self { kind, args } | ||
} | ||
|
||
pub fn args(&self) -> ty::GenericArgsRef<'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.
pub fn args(&self) -> ty::GenericArgsRef<'tcx> { | |
pub fn args(self) -> ty::GenericArgsRef<'tcx> { |
may resolve in a later PR, but why is that a method instead of just accessing the field?
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.
originally I just didnt want to give access to the args since they're always in a specific format and the GenericArgsRef
loses that information. turned out I needed it for structurally_relate_tys
and now I just sort of kept it because all the other ways of accessing the args go through a method so,,
@@ -311,7 +307,7 @@ impl<'tcx> Const<'tcx> { | |||
tcx: TyCtxt<'tcx>, | |||
param_env: ParamEnv<'tcx>, | |||
span: Span, | |||
) -> Result<ValTree<'tcx>, ErrorHandled> { | |||
) -> Result<(Ty<'tcx>, ValTree<'tcx>), ErrorHandled> { |
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.
) -> Result<(Ty<'tcx>, ValTree<'tcx>), ErrorHandled> { | |
) -> Result<(ValTree<'tcx>, Ty<'tcx>)>, ErrorHandled> |
or tbh, maybe add
struct ConstValue {
data: ValTree<'tcx>,
ty: Ty<'tcx>,
}
would be open to leave that for a separate PR though, we should merge this fairly quickly as it's quite prone to merge conflicts
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.
nods will follow up with this
Remove the `ty` field from type system `Const`s Fixes rust-lang#125556 Fixes rust-lang#122908 Part of the work on `adt_const_params`/`generic_const_param_types`/`min_generic_const_exprs`/generally making the compiler nicer. cc rust-lang/project-const-generics#44 Please review commit-by-commit otherwise I wasted a lot of time not just squashing this into a giant mess (and also it'll be SO much nicer because theres a lot of fluff changes mixed in with other more careful changes if looking via File Changes --- Why do this? - The `ty` field keeps causing ICEs and weird behaviour due to it either being treated as "part of the const" or it being forgotten about leading to ICEs. - As we move forward with `adt_const_params` and a potential `min_generic_const_exprs` it's going to become more complex to actually lower the correct `Ty<'tcx>` - It muddles the idea behind how we check `Const` arguments have the correct type. By having the `ty` field it may seem like we ought to be relating it when we relate two types, or that its generally important information about the `Const`. - Brings the compiler more in line with `a-mir-formality` as that also tracks the type of type system `Const`s via `ConstArgHasType` bounds in the env instead of on the `Const` itself. - A lot of stuff is a lot nicer when you dont have to pass around the type of a const lol. Everywhere we construct `Const` is now significantly nicer 😅 See rust-lang#125671's description for some more information about the `ty` field --- General summary of changes in this PR: - Add `Ty` to `ConstKind::Value` as otherwise there is no way to implement `ConstArgHasType` to ensure that const arguments are correctly typed for the parameter when we stop creating anon consts for all const args. It's also just incredibly difficult/annoying to thread the correct `Ty` around to a bunch of ctfe functions otherwise. - Fully implement `ConstArgHasType` in both the old and new solver. Since it now has no reliance on the `ty` field it serves its originally intended purpose of being able to act as a double check that trait vs impls have correctly typed const parameters. It also will now be able to be responsible for checking types of const arguments to parameters under `min_generic_const_exprs`. - Add `Ty` to `mir::Const::Ty`. I dont have a great understanding of why mir constants are setup like this to be honest. Regardless they need to be able to determine the type of the const and the easiest way to make this happen was to simply store the `Ty` along side the `ty::Const`. Maybe we can do better here in the future but I'd have to spend way more time looking at everywhere we use `mir::Const`. - rustdoc has its own `Const` which also has a `ty` field. It was relatively easy to remove this. --- r? `@lcnr` `@compiler-errors`
A job failed! Check out the build log: (web) (plain) Click to see the possible cause of the failure (guessed by this bot)
|
💔 Test failed - checks-actions |
@bors retry |
☀️ Test successful - checks-actions |
Finished benchmarking commit (003a902): comparison URL. Overall result: ✅ improvements - no action needed@rustbot label: -perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)Results (primary 2.2%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResults (secondary 3.5%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeResults (primary 0.1%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Bootstrap: 669.842s -> 668.946s (-0.13%) |
Stall dropaStall computing instance for drop shim until it has no unsubstituted const params Stall resolving the drop shim instance for types that still have unsubstituted const params. ## Why? rust-lang#127030 ICEs because it tries to inline the drop shim for a type with an unsubstituted const param. In order to generate this shim, this requires calling the drop shim builder, which invokes the trait solver to compute whether constituent types need drop (since we compute if a type is copy to disqualify any `Drop` behavior): https://github.com/rust-lang/rust/blob/9c3bc805dd9cb84019c124b9a50fdff1e62a7ec9/compiler/rustc_mir_dataflow/src/elaborate_drops.rs#L378 However, since we don't keep the param-env of the instance we resolved the item for, we use the wrong param-env: https://github.com/rust-lang/rust/blob/9c3bc805dd9cb84019c124b9a50fdff1e62a7ec9/compiler/rustc_mir_transform/src/shim.rs#L278 (which is the param-env of `std::ptr::drop_in_place`) This param-env is notably missing `ConstParamHasTy` predicates, and since we removed the type from consts in rust-lang#125958, we literally cannot prove these predicates in this (relatively) empty param-env. This currently happens in places like the MIR inliner, but may happen elsewhere such as in lints that resolve terminators. ## What? We delay the resolution (`Instance::resolve`) of calls for `drop_in_place` for types that have unsubstituted const params. This should be OK, since all cases that deal with polymorphic code should handle `Instance::resolve` returning `None` gracefully.
Stall computing instance for drop shim until it has no unsubstituted const params Do not inline the drop shim for types that still have unsubstituted const params. ## Why? rust-lang#127030 ICEs because it tries to inline the drop shim for a type with an unsubstituted const param. In order to generate this shim, this requires calling the drop shim builder, which invokes the trait solver to compute whether constituent types need drop (since we compute if a type is copy to disqualify any `Drop` behavior): https://github.com/rust-lang/rust/blob/9c3bc805dd9cb84019c124b9a50fdff1e62a7ec9/compiler/rustc_mir_dataflow/src/elaborate_drops.rs#L378 However, since we don't keep the param-env of the instance we resolved the item for, we use the wrong param-env: https://github.com/rust-lang/rust/blob/9c3bc805dd9cb84019c124b9a50fdff1e62a7ec9/compiler/rustc_mir_transform/src/shim.rs#L278 (which is the param-env of `std::ptr::drop_in_place`) This param-env is notably missing `ConstParamHasTy` predicates, and since we removed the type from consts in rust-lang#125958, we literally cannot prove these predicates in this (relatively) empty param-env. This currently happens in places like the MIR inliner, but may happen elsewhere such as in lints that resolve terminators. ## What? We force the inliner to not consider calls for `drop_in_place` for types that have unsubstituted const params. ## So what? This may negatively affect MIR inlining, but I doubt this matters in practice, and fixes a beta regression, so let's fix it. I will look into approaches for fixing this in a more maintainable way, perhaps delaying the creation of drop shim bodies until codegen (like how intrinsics work).
Rollup merge of rust-lang#127068 - compiler-errors:stall-drop, r=BoxyUwU Stall computing instance for drop shim until it has no unsubstituted const params Do not inline the drop shim for types that still have unsubstituted const params. ## Why? rust-lang#127030 ICEs because it tries to inline the drop shim for a type with an unsubstituted const param. In order to generate this shim, this requires calling the drop shim builder, which invokes the trait solver to compute whether constituent types need drop (since we compute if a type is copy to disqualify any `Drop` behavior): https://github.com/rust-lang/rust/blob/9c3bc805dd9cb84019c124b9a50fdff1e62a7ec9/compiler/rustc_mir_dataflow/src/elaborate_drops.rs#L378 However, since we don't keep the param-env of the instance we resolved the item for, we use the wrong param-env: https://github.com/rust-lang/rust/blob/9c3bc805dd9cb84019c124b9a50fdff1e62a7ec9/compiler/rustc_mir_transform/src/shim.rs#L278 (which is the param-env of `std::ptr::drop_in_place`) This param-env is notably missing `ConstParamHasTy` predicates, and since we removed the type from consts in rust-lang#125958, we literally cannot prove these predicates in this (relatively) empty param-env. This currently happens in places like the MIR inliner, but may happen elsewhere such as in lints that resolve terminators. ## What? We force the inliner to not consider calls for `drop_in_place` for types that have unsubstituted const params. ## So what? This may negatively affect MIR inlining, but I doubt this matters in practice, and fixes a beta regression, so let's fix it. I will look into approaches for fixing this in a more maintainable way, perhaps delaying the creation of drop shim bodies until codegen (like how intrinsics work).
Fixes #125556
Fixes #122908
Part of the work on
adt_const_params
/generic_const_param_types
/min_generic_const_exprs
/generally making the compiler nicer. cc rust-lang/project-const-generics#44Please review commit-by-commit otherwise I wasted a lot of time not just squashing this into a giant mess (and also it'll be SO much nicer because theres a lot of fluff changes mixed in with other more careful changes if looking via File Changes
Why do this?
ty
field keeps causing ICEs and weird behaviour due to it either being treated as "part of the const" or it being forgotten about leading to ICEs.adt_const_params
and a potentialmin_generic_const_exprs
it's going to become more complex to actually lower the correctTy<'tcx>
Const
arguments have the correct type. By having thety
field it may seem like we ought to be relating it when we relate two types, or that its generally important information about theConst
.a-mir-formality
as that also tracks the type of type systemConst
s viaConstArgHasType
bounds in the env instead of on theConst
itself.Const
is now significantly nicer 😅See #125671's description for some more information about the
ty
fieldGeneral summary of changes in this PR:
Ty
toConstKind::Value
as otherwise there is no way to implementConstArgHasType
to ensure that const arguments are correctly typed for the parameter when we stop creating anon consts for all const args. It's also just incredibly difficult/annoying to thread the correctTy
around to a bunch of ctfe functions otherwise.ConstArgHasType
in both the old and new solver. Since it now has no reliance on thety
field it serves its originally intended purpose of being able to act as a double check that trait vs impls have correctly typed const parameters. It also will now be able to be responsible for checking types of const arguments to parameters undermin_generic_const_exprs
.Ty
tomir::Const::Ty
. I dont have a great understanding of why mir constants are setup like this to be honest. Regardless they need to be able to determine the type of the const and the easiest way to make this happen was to simply store theTy
along side thety::Const
. Maybe we can do better here in the future but I'd have to spend way more time looking at everywhere we usemir::Const
.Const
which also has aty
field. It was relatively easy to remove this.r? @lcnr @compiler-errors