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

Require const stability attribute on all stable functions that are const #90998

Merged
merged 2 commits into from
Feb 7, 2022

Conversation

jhpratt
Copy link
Member

@jhpratt jhpratt commented Nov 18, 2021

This PR requires all stable functions (of all kinds) that are const fn to have a #[rustc_const_stable] or #[rustc_const_unstable] attribute. Stability was previously implied if omitted; a follow-up PR is planned to change the fallback to be unstable.

@rustbot rustbot added A-attributes Area: Attributes (`#[…]`, `#![…]`) A-const-fn C-bug Category: This is a bug. C-enhancement Category: An issue proposing an enhancement or a PR with one. S-experimental Status: Ongoing experiment that does not require reviewing and won't be merged in its current state. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Nov 18, 2021
@jhpratt
Copy link
Member Author

jhpratt commented Nov 18, 2021

Do we actually want to require #[rustc_const_unstable] on #[unstable] items (and those called from them)? There's a slew of annotations in core alone that don't ultimately have any effect (because they're unstable anyways). The recursive aspect of this (in that every method called from an annotated const fn has to itself have an annotation) goes quite deep — I stopped adding annotations when I hit the hashbrown crate. Alternatively, I could remove the recursive requirement when a method is unstable.

With the changes already pushed, when a method is stabilized errors will be thrown if there's a method used that shouldn't/can't be. It will additionally not be possible to "accidentally" stabilize an item as const, as such annotation will be required on associated functions (this was previously not the case and is unquestionably a bug).

@RalfJung
Copy link
Member

Do we actually want to require #[rustc_const_unstable] on #[unstable] items (and those called from them)? There's a slew of annotations in core alone that don't ultimately have any effect (because they're unstable anyways).

If we treat the absence of a const stability attribute as being equivalent to having a rustc_const_unstable, then I think it also makes sense to not require adding that attribute everywhere.

@jhpratt jhpratt force-pushed the require-const-stability branch from 6623e5b to 660fb95 Compare December 20, 2021 07:07
@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Dec 20, 2021
@jhpratt
Copy link
Member Author

jhpratt commented Dec 20, 2021

Coming back around to this, I think it would make the most sense to do these two changes separately. I intend on submitting a follow-up wherein the omission of #[rustc_const_stable] is interpreted to be unstable rather than the current stable.

Marking this PR as ready for review. It ensures that all stable functions that are const have a #[rustc_const_stable] or #[rustc_const_unstable] attribute. Unstable functions that are const do not require this per the above review. The intent is to prevent accidental stabilization of a function as const fn.

r? @RalfJung

@rustbot label -S-experimental -S-waiting-on-author +S-waiting-on-review

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-experimental Status: Ongoing experiment that does not require reviewing and won't be merged in its current state. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 20, 2021
@jhpratt jhpratt marked this pull request as ready for review December 20, 2021 07:11
@jhpratt jhpratt changed the title [WIP] Require const stability on all const items Require const stability attribute on all stable functions that are const Dec 20, 2021
@rust-log-analyzer

This comment has been minimized.

@jhpratt jhpratt force-pushed the require-const-stability branch from 660fb95 to a4bab4a Compare December 20, 2021 07:20
@RalfJung
Copy link
Member

What you suggest makes sense, but I am afraid that check_missing_const_stability is full of APIs I have never used, so I have no idea if this is implemented correctly.

r? @oli-obk

@rust-highfive rust-highfive assigned oli-obk and unassigned RalfJung Dec 20, 2021
@jhpratt jhpratt force-pushed the require-const-stability branch from a4bab4a to 1300f57 Compare December 22, 2021 07:01
Copy link
Member Author

@jhpratt jhpratt left a comment

Choose a reason for hiding this comment

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

Given what I found in the above comment, I think this PR is set now that I've updated the comments in code.

compiler/rustc_passes/src/stability.rs Show resolved Hide resolved
@RalfJung
Copy link
Member

RalfJung commented Dec 22, 2021

After looking into this, the reason it was like that originally is that only a LocalDefId and Span are passed as parameters. If the check were pushed into the method itself, multiple types would have to be passed. I suppose it could be done with a trait, but that seems less good than the code as written.

I am confused.

The other call site checks 2 things:

  • self.tcx.features().staged_api. Why would that check only be needed in one place but not the other? That doesn't make any sense.
  • sig.header.is_const(). This seems to be unnecessary now since you changed check_missing_const_stability to do an is_const check itself, which it previously did not do. It makes no sense to check for constness twice. The two checks are also different, no idea which one is right.

@JohnCSimon
Copy link
Member

Ping from triage:
@jhpratt - can you please address the comment from RalfJung?

This was supposed to be the case previously, but a missed method call
meant that trait impls were not checked.
@jhpratt jhpratt force-pushed the require-const-stability branch from 1300f57 to 1911eb8 Compare February 4, 2022 00:16
@jhpratt
Copy link
Member Author

jhpratt commented Feb 4, 2022

Comments have been addressed. Should be ready for review @oli-obk. A follow-up is planned for making the default unstable — I think it'll be simple but a quick modification leads to a failed test (I'm looking into it further).

@oli-obk
Copy link
Contributor

oli-obk commented Feb 4, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Feb 4, 2022

📌 Commit 1911eb8 has been approved by oli-obk

@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-review Status: Awaiting review from the assignee but also interested parties. labels Feb 4, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 6, 2022
…askrgr

Rollup of 2 pull requests

Successful merges:

 - rust-lang#90998 (Require const stability attribute on all stable functions that are `const`)
 - rust-lang#93489 (Mark the panic_no_unwind lang item as nounwind)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 9f4559c into rust-lang:master Feb 7, 2022
@rustbot rustbot added this to the 1.60.0 milestone Feb 7, 2022
@jhpratt jhpratt deleted the require-const-stability branch February 7, 2022 03:15
@RalfJung RalfJung added the A-const-eval Area: Constant evaluation, covers all const contexts (static, const fn, ...) label Dec 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-attributes Area: Attributes (`#[…]`, `#![…]`) A-const-eval Area: Constant evaluation, covers all const contexts (static, const fn, ...) C-bug Category: This is a bug. C-enhancement Category: An issue proposing an enhancement or a PR with one. 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-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants