Yeet the effects feature, move it onto const_trait_impl - #132479
Conversation
|
HIR ty lowering was modified cc @fmease Some changes occurred in src/tools/clippy cc @rust-lang/clippy |
|
Lots of files changed, but most of it is just removing |
This comment has been minimized.
This comment has been minimized.
e86e5aa to
6be0947
Compare
fee1-dead
left a comment
There was a problem hiding this comment.
r=me after nits and rebases that become necessary
6be0947 to
6b96103
Compare
commented
Nov 3, 2024
|
The @bors r=fee1-dead |
commented
Nov 3, 2024
commented
Nov 3, 2024
commented
Nov 3, 2024
|
☀️ Test successful - checks-actions |
commented
Nov 4, 2024
|
Finished benchmarking commit (b8c8287): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)Results (primary 1.8%, secondary 4.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.
CyclesResults (primary 3.7%)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 sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 780.261s -> 780.466s (0.03%) |
Do we have checks ensuring that const trait fns cannot be called from stable const fns? I don't think so (just some leftovers in check.rs you said you were not happy with, and I don't know if we can trust them). IMO that is a blocker for enabling the feature in the standard library. |
| if tcx.features().effects() && trait_is_const { | ||
| // FIXME(const_trait_impl) we might consider moving const stability checks | ||
| // to typeck as well. | ||
| if tcx.features().const_trait_impl() && trait_is_const { |
There was a problem hiding this comment.
The effects feature name still appears a few lines down in this file
rust/compiler/rustc_const_eval/src/check_consts/check.rs
Lines 680 to 684 in 6b96103
So specifically, I am concerned about this line This does not take into account whether the current function must be recursively const-stable. So I think what the logic here should be more like if let Some(trait_did) = tcx.trait_of_item(callee) {
if !trait_is_const {
// Trigger `ops::FnCallNonConst`.
// ...
return;
}
// Ensure we have `const_trait_impl` and are allowed to use it in this function.
self.check_op(ops::ConstTraitCall);
// We don't know which function is being called here, so we can't run the checks below.
// FIXME: apply more fine-grained per-trait const stability checks
// (see https://github.com/rust-lang/project-const-traits/issues/5).
return;
}Also it seems a bit odd that |
commented
Nov 15, 2024
noting here for completeness, that was done in #132823. |
This PR merges the
effectsfeature into theconst_trait_implfeature. There's really no need to have two feature gates for one feature.After this PR, if
const_trait_implis enabled:HostEffectconst conditions will be enforced on the HIRAnd if
const_trait_implis not enabled:HostEffectconst conditions are not enforced on the HIR~constin their where clasues)This should be the last step for us to be able to enable const traits in the standard library. We still need to re-constify
DropandDestructand stuff for const traits to be particularly useful for some cases, but this is a good step :Dr? fee1-dead
cc @rust-lang/project-const-traits