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

Deny #[rustc_const_stable] on #[unstable] functions #90356

Closed
wants to merge 3 commits into from

Conversation

jhpratt
Copy link
Member

@jhpratt jhpratt commented Oct 27, 2021

This adds a lint against both function declarations and extern function declarations that are #[unstable] but declared #[rustc_const_stable], as this is almost certainly a mistake.

Resolves #79551

@rustbot label A-const-fn A-lint A-stability C-enhancement T-libs

cc @oli-obk

Verified

This commit was signed with the committer’s verified signature. The key has expired.
jhpratt Jacob Pratt

Verified

This commit was signed with the committer’s verified signature. The key has expired.
jhpratt Jacob Pratt
@rustbot rustbot added A-const-fn A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. A-stability Area: `#[stable]`, `#[unstable]` etc. C-enhancement Category: An issue proposing an enhancement or a PR with one. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Oct 27, 2021
@rust-highfive
Copy link
Collaborator

r? @petrochenkov

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 27, 2021
@jhpratt

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

Verified

This commit was signed with the committer’s verified signature. The key has expired.
jhpratt Jacob Pratt
@jhpratt jhpratt force-pushed the incompatible-stability branch from 1db92b6 to a11d1c5 Compare October 28, 2021 04:24
@rust-log-analyzer
Copy link
Collaborator

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

Click to see the possible cause of the failure (guessed by this bot)
 Documenting core v0.0.0 (/checkout/library/core)
error: functions cannot be const-stable if they are unstable
  --> library/core/src/../../stdarch/crates/core_arch/src/wasm32/simd128.rs:58:17
   |
58 | /                 const fn v128(self) -> v128 {
59 | |                     unsafe { mem::transmute(self) }
   | |_________________^
...
...
66 | / conversions! {
67 | |     (as_u8x16 = simd::u8x16)
68 | |     (as_u16x8 = simd::u16x8)
69 | |     (as_u32x4 = simd::u32x4)
...  |
76 | |     (as_f64x2 = simd::f64x2)
   | |_- in this macro invocation
   |
   |
   = note: `#[deny(rustc::incompatible_stability)]` on by default
   = note: this error originates in the macro `conversions` (in Nightly builds, run with -Z macro-backtrace for more info)
error: could not document `core`

Caused by:
Caused by:
  process didn't exit successfully: `/checkout/obj/build/bootstrap/debug/rustdoc --edition=2018 --crate-type lib --crate-name core library/core/src/lib.rs --target x86_64-unknown-linux-gnu -o /checkout/obj/build/x86_64-unknown-linux-gnu/stage2-std/x86_64-unknown-linux-gnu/doc --error-format=json --json=diagnostic-rendered-ansi --markdown-css rust.css --markdown-no-toc -Z unstable-options --resource-suffix 1.58.0 --index-page /checkout/src/doc/index.md -L dependency=/checkout/obj/build/x86_64-unknown-linux-gnu/stage2-std/x86_64-unknown-linux-gnu/release/deps -L dependency=/checkout/obj/build/x86_64-unknown-linux-gnu/stage2-std/release/deps -Zsymbol-mangling-version=legacy -Dwarnings '-Wrustdoc::invalid_codeblock_attributes' --crate-version '1.58.0-nightly
  (347467193
  2021-10-28)' '-Zcrate-attr=doc(html_root_url="https://doc.rust-lang.org/nightly/")'` (exit status: 1)


command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "rustdoc" "--target" "x86_64-unknown-linux-gnu" "-Zbinary-dep-depinfo" "-j" "16" "--release" "--locked" "--color" "always" "--features" "panic-unwind backtrace compiler-builtins-c" "--manifest-path" "/checkout/library/test/Cargo.toml" "-p" "core" "-Zskip-rustdoc-fingerprint" "--" "--markdown-css" "rust.css" "--markdown-no-toc" "-Z" "unstable-options" "--resource-suffix" "1.58.0" "--index-page" "/checkout/src/doc/index.md"


Build completed unsuccessfully in 0:32:25

@oli-obk
Copy link
Contributor

oli-obk commented Oct 28, 2021

I thought we needed this to allow unstable const fns to be called from stable const fns. But the cases you adjusted do seem like accidents.

Much more problematic to me are stable const fns that have no rustc_const_* attribute at all, as that seems like an oversight and may be an accidental constness stabilization

@jhpratt
Copy link
Member Author

jhpratt commented Oct 28, 2021

I thought we needed this to allow unstable const fns to be called from stable const fns

Not sure I follow. This has nothing to do with the body of the function. You can still use #[rustc_allow_const_fn_unstable(foo)] and the lint neither knows nor cares.

But the cases you adjusted do seem like accidents.

Definitely. I was actually surprised there weren't more.

Much more problematic to me are stable const fns that have no rustc_const_* attribute at all, as that seems like an oversight and may be an accidental constness stabilization

I was planning on addressing that in a follow-up PR. My intent was to create a lint very similar to this that requires the #[rustc_const_(un)stable] attribute if a public method is const. I don't see any reason to declare stability on internal methods — that's incidentally the current build failure in stdarch.

@oli-obk
Copy link
Contributor

oli-obk commented Oct 28, 2021

Not sure I follow. This has nothing to do with the body of the function. You can still use #[rustc_allow_const_fn_unstable(foo)] and the lint neither knows nor cares.

that only works for language features afaicr. But... if libcore compiles with your changes, then this seems fine to me

@jhpratt
Copy link
Member Author

jhpratt commented Oct 28, 2021

You'd know the details of that attribute better than myself. But yeah, libcore doesn't have any instances of the lint firing other than intrinsics (silenced for well-known reasons) and the two erroneous places. Once stdarch is updated then CI should be green.

Also, was I correct in that I needed to gate the #[allow] behind bootstrapping? It makes sense intuitively.

@oli-obk
Copy link
Contributor

oli-obk commented Oct 30, 2021

Also, was I correct in that I needed to gate the #[allow] behind bootstrapping? It makes sense intuitively.

Yes

libcore doesn't have any instances of the lint firing other than intrinsics (silenced for well-known reasons) and the two erroneous places. Once stdarch is updated then CI should be green.

Cool! The PR lgtm

You'd know the details of that attribute better than myself

My brain is a sieve XD I just thought we needed that, but looks like we either don't or we don't anymore.

@jhpratt
Copy link
Member Author

jhpratt commented Nov 1, 2021

Turns out your understanding was correct — even private methods need to be #[rustc_const_stable]. While I don't necessarily agree with this, let's assume this isn't going to change. What should be done with this PR?

My initial thought is to limit this lint to pub methods. The only instance where this would then matter is if a const-unstable pub method is called from a const-stable pub method. It could be worked around by having the former be a transparent wrapper around a const-stable internal method. I'm not sure that's something we want to encourage, however.

@jhpratt
Copy link
Member Author

jhpratt commented Nov 7, 2021

Running into another use case for this as I tackle a bug I introduced a little bit ago re. const panic. As this doesn't seem to apply to only intrinsics as suspected, I'm going to go ahead and close this. My recommendation is to close #79551 as well.

@jhpratt jhpratt closed this Nov 7, 2021
@jhpratt jhpratt deleted the incompatible-stability branch November 7, 2021 03:43
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 10, 2021

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
…, r=dtolnay

Fix incorrect stability attributes

These two instances were caught in rust-lang#90356, but that PR isn't going to be merged. I've extracted these to ensure it's still correct.

`@rustbot` label: +A-stability +C-cleanup +S-waiting-on-review
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 10, 2021

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
…, r=dtolnay

Fix incorrect stability attributes

These two instances were caught in rust-lang#90356, but that PR isn't going to be merged. I've extracted these to ensure it's still correct.

``@rustbot`` label: +A-stability +C-cleanup +S-waiting-on-review
@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-const-eval Area: Constant evaluation, covers all const contexts (static, const fn, ...) A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. A-stability Area: `#[stable]`, `#[unstable]` etc. C-enhancement Category: An issue proposing an enhancement or a PR with one. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. 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.

Forbid #[rustc_const_stable] without #[stable]
7 participants