Skip to content
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

Implement trait const stability #133999

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

fee1-dead
Copy link
Member

.. By enforcing it in three locations:

  • When a bound during lowering is ~const or const
  • When an impl is const
  • When a MIR call is on a trait's method

Reopens #90080
Reopens #88955
Closes rust-lang/project-const-traits#5
Closes rust-lang/project-const-traits#16

cc @RalfJung

@rustbot
Copy link
Collaborator

rustbot commented Dec 7, 2024

r? @oli-obk

rustbot has assigned @oli-obk.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@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. labels Dec 7, 2024
@rustbot
Copy link
Collaborator

rustbot commented Dec 7, 2024

HIR ty lowering was modified

cc @fmease

Some changes occurred to the CTFE machinery

cc @rust-lang/wg-const-eval

@fee1-dead
Copy link
Member Author

also cc @rust-lang/project-const-traits

@oli-obk
Copy link
Contributor

oli-obk commented Dec 7, 2024

I'm guessing with the current const trait impl we'd have the same issues with making impls unstable as we'd have for nonconst impls?

@RalfJung
Copy link
Member

RalfJung commented Dec 7, 2024

Yeah... we should enforce that if a trait is marked const-stable then all its impls (in staged_api crates) are const-stable, like we do for regular stability.

@fee1-dead
Copy link
Member Author

The current code doesn't deal with impls' const stability yet, that work can be incremental on top of this (i will personally probably have more cycles to spend until one week or more, so i'd prefer us landing a reviewed version of this first)

@RalfJung
Copy link
Member

RalfJung commented Dec 7, 2024

That's fine as long as this is recorded somewhere as a blocker.

@bors
Copy link
Contributor

bors commented Dec 10, 2024

☔ The latest upstream changes (presumably #134108) made this pull request unmergeable. Please resolve the merge conflicts.

@fee1-dead fee1-dead 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 Dec 26, 2024
@rust-log-analyzer

This comment has been minimized.

@fee1-dead
Copy link
Member Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 26, 2024
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jan 7, 2025
Rollup merge of rust-lang#135139 - c410-f3r:8-years-rfc, r=jhpratt

[generic_assert] Constify methods used by the formatting system

cc rust-lang#44838

Starts the "constification" of all the elements required to allow the execution of the formatting system in constant environments.

```rust
const _: () = { panic!("{:?}", 1i32); };
```

Further stuff is blocked by rust-lang#133999.
compiler/rustc_middle/messages.ftl Outdated Show resolved Hide resolved
compiler/rustc_middle/src/middle/stability.rs Outdated Show resolved Hide resolved
@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 Jan 7, 2025
@oli-obk
Copy link
Contributor

oli-obk commented Jan 7, 2025

r=me with diagnostic and readability nits resolved

@fee1-dead
Copy link
Member Author

@bors r=oli-obk

@bors
Copy link
Contributor

bors commented Jan 7, 2025

📌 Commit 05ca6ae has been approved by oli-obk

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 Jan 7, 2025
@RalfJung
Copy link
Member

RalfJung commented Jan 7, 2025

@bors r-
I just started taking a look concurrently with Oli as I somehow thought it was assigned to me, sorry. I'll have some comments for compiler/rustc_middle/src/middle/stability.rs as that logic should very closely mirror compiler/rustc_const_eval/src/check_consts/check.rs.

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 7, 2025
@rust-log-analyzer

This comment has been minimized.

Comment on lines 620 to 621
let unstable_feature_allowed = span.allows_unstable(feature)
|| implied_feature.is_some_and(|f| span.allows_unstable(f));
Copy link
Member

@RalfJung RalfJung Jan 7, 2025

Choose a reason for hiding this comment

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

For function calls we only honor allows_unstable if the callee is safe-to-expose on stable. So here I guess the question would be, when is a trait safe to expose on stable? Right now I think the answer is "only when it is stable", we don't support something like #[const_stable_indirect] on traits. That means we should not check span.allows_unstable here.

Copy link
Member Author

Choose a reason for hiding this comment

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

removed

span: Span,
parent_def: Option<LocalDefId>,
) {
let Some(ConstStability {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
let Some(ConstStability {
// This should work pretty much exactly like the function stability logic in
// `compiler/rustc_const_eval/src/check_consts/check.rs`.
// FIXME: Find some way to not duplicate that logic.
let Some(ConstStability {

});
self.disabled_nightly_features(&mut diag, None, [(String::new(), feature)]);
diag.emit();
} else if let Some(parent) = parent_def {
Copy link
Member

@RalfJung RalfJung Jan 7, 2025

Choose a reason for hiding this comment

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

I don't understand this -- what is the "parent" here?

The equivalent code for this entire if-else-if in check.rs is

                        if !feature_enabled || !callee_safe_to_expose_on_stable {
                            self.check_op(ops::FnCallUnstable {
                                def_id: callee,
                                feature,
                                feature_enabled,
                                safe_to_expose_on_stable: callee_safe_to_expose_on_stable,
                            });
                        }

Here callee_safe_to_expose_on_stable is always false so we should always behave as-if check_op was called.

Copy link
Member Author

Choose a reason for hiding this comment

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

Either a const fn that contains the ~const Tr bound or the callee of a const method.

we should always behave as-if check_op was called.

Yes, this is why this code is derived from check_op_spanned.

Copy link
Member

@RalfJung RalfJung Jan 10, 2025

Choose a reason for hiding this comment

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

Either a const fn that contains the ~const Tr bound or the callee of a const method.

Sorry I don't follow. If you had said "always the const fn that contains the thing we are stability-checking", i.e., the caller, that would make more sense to me. Is this a typo?

The existing const checks have nothing like this, as far as I can see. I have no idea what it means here that we are skipping the check if there is no parent. We should always know the function we are in and that defines whether we are in recursive-const-stability mode.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this is why this code is derived from check_op_spanned.

Can you make the structure parallel, i.e. also have a function like that here with essentially the same inputs (without the NonConstOp trait of course). Or ideally share that code rather than duplicate it... maybe we need a compiler/rustc_middle/src/middle/const_stability.rs file for those functions. (stability.rs is already huge.)

// other unstably-const features with `const_stable_indirect`
stab.is_const_stable() || stab.const_stable_indirect
}
};
Copy link
Member

Choose a reason for hiding this comment

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

What is this logic? This has to be a duplicate of something in check.rs, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is duplicating enforce_recursive_const_stability.

Copy link
Member

Choose a reason for hiding this comment

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

That logic is fairly critical and definitely should not be duplicated.

@bors
Copy link
Contributor

bors commented Jan 9, 2025

☔ The latest upstream changes (presumably #135286) made this pull request unmergeable. Please resolve the merge conflicts.

@rustbot rustbot added the A-rustc-dev-guide Area: rustc-dev-guide label Jan 10, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jan 10, 2025

The rustc-dev-guide subtree was changed. If this PR only touches the dev guide consider submitting a PR directly to rust-lang/rustc-dev-guide otherwise thank you for updating the dev guide with your changes.

cc @BoxyUwU, @jieyouxu, @Kobzol

@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-tools failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
   Compiling compiler_builtins v0.1.140
error: trait has missing const stability attribute
  --> library/core/src/intrinsics/fallback.rs:11:1
   |
11 | / pub trait CarryingMulAdd: Copy + 'static {
12 | |     type Unsigned: Copy + 'static;
14 | |         self,
...  |
18 | |     ) -> (Self::Unsigned, Self);
19 | | }

@BoxyUwU
Copy link
Member

BoxyUwU commented Jan 10, 2025

image
ive never seen a better diff in my life

@RalfJung
Copy link
Member

RalfJung commented Jan 10, 2025 via email

@bors
Copy link
Contributor

bors commented Jan 10, 2025

☔ The latest upstream changes (presumably #135327) made this pull request unmergeable. Please resolve the merge conflicts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rustc-dev-guide Area: rustc-dev-guide S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add const stability tracking for traits Const stability on impls doesn't make sense
8 participants