Skip to content

Yeet the effects feature, move it onto const_trait_impl - #132479

Merged
bors merged 3 commits into
rust-lang:masterfrom
compiler-errors:fx-feat-yeet
Nov 3, 2024
Merged

bors merged 3 commits into
rust-lang:masterfrom
compiler-errors:fx-feat-yeet

Conversation

@compiler-errors

Copy link
Copy Markdown
Contributor

This PR merges the effects feature into the const_trait_impl feature. There's really no need to have two feature gates for one feature.

After this PR, if const_trait_impl is enabled:

  • Users can use and define const traits
  • HostEffect const conditions will be enforced on the HIR
  • We re-check the predicates in MIR just to make sure that we don't "leak" anything during MIR lowering

And if const_trait_impl is not enabled:

  • Users cannot use nor define const traits
  • HostEffect const conditions are not enforced on the HIR
  • We will raise a const validation error if we call a function that has any const conditions (i.e. const traits and functions with any ~const in 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 Drop and Destruct and stuff for const traits to be particularly useful for some cases, but this is a good step :D

r? fee1-dead
cc @rust-lang/project-const-traits

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) labels Nov 1, 2024
@rustbot

rustbot commented Nov 1, 2024

Copy link
Copy Markdown
Collaborator

HIR ty lowering was modified

cc @fmease

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

@compiler-errors

Copy link
Copy Markdown
Contributor Author

Lots of files changed, but most of it is just removing feature(effects) from the ui tests, and renaming FIXME(effects) to FIXME(const_trait_impl)

@rust-log-analyzer

This comment has been minimized.

@fee1-dead fee1-dead left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=me after nits and rebases that become necessary

Comment thread tests/rustdoc/rfc-2632-const-trait-impl.rs Outdated
Comment thread tests/ui/consts/auxiliary/closure-in-foreign-crate.rs Outdated
Comment thread tests/ui/consts/const-try.rs Outdated
Comment thread tests/ui/consts/const_cmp_type_id.rs Outdated
Comment thread tests/ui/consts/rustc-impl-const-stability.rs Outdated
Comment thread tests/ui/traits/const-traits/cross-crate.rs
Comment thread tests/ui/traits/const-traits/do-not-const-check-override.rs
Comment thread tests/ui/traits/const-traits/ice-119717-constant-lifetime.rs
Comment thread tests/ui/traits/const-traits/ice-126148-failed-to-normalize.rs
@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 3, 2024
@compiler-errors

ghost commented Nov 3, 2024

Copy link
Copy Markdown
Contributor Author

The const_closure and some other feature gates remain incomplete, so there are still a few effects tests that still need allow(incomplete_features). I tried to remove the rest though.

@bors r=fee1-dead

@bors

ghost commented Nov 3, 2024

Copy link
Copy Markdown
Collaborator

📌 Commit 6b96103 has been approved by fee1-dead

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 3, 2024
@bors

ghost commented Nov 3, 2024

Copy link
Copy Markdown
Collaborator

⌛ Testing commit 6b96103 with merge b8c8287...

@bors

ghost commented Nov 3, 2024

Copy link
Copy Markdown
Collaborator

☀️ Test successful - checks-actions
Approved by: fee1-dead
Pushing b8c8287 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 3, 2024
@bors
bors merged commit b8c8287 into rust-lang:master Nov 3, 2024
@rustbot rustbot added this to the 1.84.0 milestone Nov 3, 2024
@rust-timer

ghost commented Nov 4, 2024

Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (b8c8287): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This 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.

mean range count
Regressions ❌
(primary)
1.8% [1.8%, 1.8%] 1
Regressions ❌
(secondary)
4.5% [4.5%, 4.5%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 1.8% [1.8%, 1.8%] 1

Cycles

Results (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.

mean range count
Regressions ❌
(primary)
3.7% [3.7%, 3.7%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 3.7% [3.7%, 3.7%] 1

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 780.261s -> 780.466s (0.03%)
Artifact size: 335.33 MiB -> 335.33 MiB (0.00%)

@RalfJung

ghost commented Nov 4, 2024

Copy link
Copy Markdown
Member

This should be the last step for us to be able to enable const traits in the standard library.

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 {

ghost Nov 4, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The effects feature name still appears a few lines down in this file

Some(if tcx.features().const_trait_impl() {
sym::effects
} else {
sym::const_trait_impl
})

@RalfJung

ghost commented Nov 4, 2024

Copy link
Copy Markdown
Member

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).

So specifically, I am concerned about this line

if tcx.features().const_trait_impl() && trait_is_const {

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 const_trait_impl guards calling const trait functions. Maybe const_trait would make more sense as the name for this feature?

@fee1-dead

ghost commented Nov 15, 2024

Copy link
Copy Markdown
Member

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).

noting here for completeness, that was done in #132823.

@fmease fmease added the PG-const-traits Project group: Const traits label Nov 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merged-by-bors This PR was explicitly merged by bors. PG-const-traits Project group: Const traits S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants