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

replace #[allow_internal_unstable] with #[rustc_allow_const_fn_unstable] for const fns #78208

Merged
merged 7 commits into from
Oct 25, 2020

Conversation

liketechnik
Copy link
Contributor

#[allow_internal_unstable] is currently used to side-step feature gate and stability checks.
While it was originally only meant to be used only on macros, its use was expanded to const fns.

This pr adds stricter checks for the usage of #[allow_internal_unstable] (only on macros) and introduces the #[rustc_allow_const_fn_unstable] attribute for usage on const fns.

This pr does not change any of the functionality associated with the use of #[allow_internal_unstable] on macros or the usage of #[rustc_allow_const_fn_unstable] (instead of #[allow_internal_unstable]) on const fns (see #69399 (comment)).

Note: The check for #[rustc_allow_const_fn_unstable] currently only validates that the attribute is used on a function, because I don't know how I would check if the function is a const fn at the place of the check. I therefore openend this as a 'draft pull request'.

Closes #69399

r? @oli-obk

allow_internal_unstable is currently used
to side-step feature gate and stability checks.
While it was originally only meant to be used
only on macros, its use was expanded to
const functions.

This commit prepares stricter checks for the usage of allow_internal_unstable (only on macros)
and introduces the rustc_allow_const_fn_unstable attribute for usage on functions.

See rust-lang#69399
Adds a check to make sure `#[allow_internal_unstable]`
can be applied only to macro definitions.
Adds a check to make sure `#[rustc_allow_const_fn_unstable]`
can be applied only to functions.
@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @oli-obk (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 21, 2020
@jyn514 jyn514 added C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-libs Relevant to the library team, which will review and decide on the PR/issue. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-const-fn labels Oct 22, 2020
compiler/rustc_expand/src/base.rs Outdated Show resolved Hide resolved
compiler/rustc_mir/src/transform/check_consts/mod.rs Outdated Show resolved Hide resolved
compiler/rustc_passes/src/check_attr.rs Outdated Show resolved Hide resolved
@oli-obk
Copy link
Contributor

oli-obk commented Oct 23, 2020

This is really clean, thanks for taking the care to build clear individual commits

Recognition for `rustc_allow_const_fn_unstable` attribute was errorneously
added in 05f4a9a.
…n rustc_mir

Followup rename from 05f4a9a,
which introduced `#[rustc_allow_const_fn_unstable]` for `const fn`s.
The validation was introduced in 3a63bf0
without strict validation of functions, e. g. all function types were
allowed.
Now the validation only allows `const fn`s.
@liketechnik liketechnik marked this pull request as ready for review October 23, 2020 15:57
@liketechnik
Copy link
Contributor Author

liketechnik commented Oct 23, 2020

Note: If I understood the contributing guide correctly, it is discouraged to change already pushed commits after review (e. g. interactive rebase). Instead it is preferred to add new commits containing the necessary changes, which I did now. If I got that wrong, I'm happy to rebase instead.

Edit: It isn't in the documentation, it's directly in rust-highfive's comment to this pr. Sorry for the possible confusion.

@oli-obk
Copy link
Contributor

oli-obk commented Oct 23, 2020

The rule is there to make reviewing easier (since in large PRs it can quickly become infeasible to review the entire PR again), but once everything is approved fixing up history is definitely ok.

So... I'd r+ this as it is, though if you want to fix up history, that's totally alright with me, but not neccessary. Please let me know what you prefer

@liketechnik
Copy link
Contributor Author

I'd leave it as is this time, because the fix-up commits should be descriptive enough on their own.

@oli-obk
Copy link
Contributor

oli-obk commented Oct 23, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Oct 23, 2020

📌 Commit ac2c599 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 Oct 23, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 25, 2020
Rollup of 8 pull requests

Successful merges:

 - rust-lang#77984 (Compute proper module parent during resolution)
 - rust-lang#78085 (MIR validation should check `SwitchInt` values are valid for the type)
 - rust-lang#78208 (replace `#[allow_internal_unstable]` with `#[rustc_allow_const_fn_unstable]` for `const fn`s)
 - rust-lang#78209 (Update `compiler_builtins` to 0.1.36)
 - rust-lang#78276 (Bump backtrace-rs to enable Mach-O support on iOS.)
 - rust-lang#78320 (Link to cargo's `build-std` feature instead of `xargo` in custom target docs)
 - rust-lang#78322 (BTreeMap: stop mistaking node::MIN_LEN for a node level constraint)
 - rust-lang#78326 (Split out statement attributes changes from rust-lang#78306)

Failed merges:

r? `@ghost`
@bors bors merged commit 72e02b0 into rust-lang:master Oct 25, 2020
@rustbot rustbot added this to the 1.49.0 milestone Oct 25, 2020
@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, ...) C-cleanup Category: PRs that clean code up or issues documenting cleanup. 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

#[allow_internal_unstable] does not allow unstable library features when applied on functions
7 participants