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 the min_const_fn feature gate #53604

Merged
merged 6 commits into from
Sep 1, 2018
Merged

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Aug 22, 2018

cc @RalfJung @eddyb

r? @Centril

implements the feature gate for #53555

I added a hack so the const_fn feature gate also enables the min_const_fn feature gate. This ensures that nightly users of const_fn don't have to touch their code at all.

The min_const_fn checks are run first, and if they succeeded, the const_fn checks are run additionally to ensure we didn't miss anything.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 22, 2018
@rust-highfive

This comment has been minimized.

src/libsyntax/feature_gate.rs Outdated Show resolved Hide resolved
@@ -206,9 +213,12 @@ declare_features! (
// #23121. Array patterns have some hazards yet.
(active, slice_patterns, "1.0.0", Some(23121), None),

// Allows the definition of `const fn` functions.
// Allows the definition of `const fn` functions with some advanced features
(active, const_fn, "1.2.0", Some(24111), None),
Copy link
Contributor

Choose a reason for hiding this comment

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

We will need to ensure that the standard library has not enabled #![feature(const_fn)]; this includes libstd, liballoc, libcore.

Copy link
Member

Choose a reason for hiding this comment

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

Well, we will want it for NonZero::new_unchecked, right?

Copy link
Contributor

@Centril Centril Aug 22, 2018

Choose a reason for hiding this comment

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

Hmm... I was hoping on using the stabilization of min_const_fn but exclusion of const_fn from libstd so that standard library functions could be const-ified en masse instead of piecemeal. This kinda relies on not having #![feature(const_fn)] enabled so that accidental things don't happen.

Could we handle new_unchecked through some other internal means instead temporarily?

EDIT: like rustc_const_stable or something...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd say:

  • const fn without #[rustc_const_unstable] is stable if it passes min_const_fn
  • const fn not passing min_const_fn can be forced stable with #[rustc_const_fn_force_stable]

This way the active feature gates have no influence on stability, but we're checking the right thing and can't accidentally do anything wrong.

Copy link
Contributor

Choose a reason for hiding this comment

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

@oli-obk this sounds good to me :)

src/libsyntax/feature_gate.rs Outdated Show resolved Hide resolved
src/test/ui/consts/min_const_fn/min_const_fn.rs Outdated Show resolved Hide resolved
src/test/ui/consts/min_const_fn/min_const_fn.rs Outdated Show resolved Hide resolved
src/test/ui/consts/min_const_fn/min_const_fn.rs Outdated Show resolved Hide resolved
src/test/ui/consts/min_const_fn/min_const_fn.rs Outdated Show resolved Hide resolved
src/test/ui/consts/min_const_fn/min_const_fn.rs Outdated Show resolved Hide resolved
src/test/ui/consts/min_const_fn/min_const_fn.rs Outdated Show resolved Hide resolved
Place::Static(_) => Err((span, "cannot access `static` items in const fn".into())),
Place::Projection(proj) => {
match proj.elem {
// raw ptr derefs prevented by `const_raw_ptr_deref`
Copy link
Member

@RalfJung RalfJung Aug 22, 2018

Choose a reason for hiding this comment

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

What is const_raw_ptr_deref?

Can't we somehow check unsafety, instead of trying to get an exhaustive list of unsafe behavior?

Copy link
Contributor

@Centril Centril Aug 22, 2018

Choose a reason for hiding this comment

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

If it aint possible to detect this in MIR perhaps do it in HIR instead?

EDIT: Add a test with an empty unsafe { ... } block in it

Copy link
Member

@RalfJung RalfJung Aug 22, 2018

Choose a reason for hiding this comment

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

AFAIK MIR has some unsafety information; we do some unsafety checks in MIR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Member

Choose a reason for hiding this comment

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

Done as in, the reference to const_raw_ptr_deref is gone? If not, please mention the filename where that function is defined...^^

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As in there's a test. And const_raw_ptr_deref is a feature gate.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I see. That's not clear.^^

But the feature is ruled out independent of whether that gate is set...?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well... it's additionally prevented in min_const_fn due to its unsafety. The same doesn't hold for casting *const T to usize. I'll add some more sanity checks. This will annoy the heck out of nightly users since they are now getting a lot of messages twice in different wording (already happening with a bunch of things in this PR)

Copy link
Member

Choose a reason for hiding this comment

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

That's a consequence of doing multiple analyses, right?

I do not think that is the right approach... even if we have separate analyses, we should only call one.

@RalfJung
Copy link
Member

Is it really better to have all that code duplicated into a second analysis, than changing the existing analysis to qualify every const fn as "minimal, normal, invalid"?

@oli-obk
Copy link
Contributor Author

oli-obk commented Aug 23, 2018

Is it really better to have all that code duplicated into a second analysis, than changing the existing analysis to qualify every const fn as "minimal, normal, invalid"?

The existing analysis is supposed to be split up into three separate analyses. Me adding a fourth one to be split out in the future would not help the process.

@RalfJung
Copy link
Member

Three?!? I can see that promotion is separate, but other than that everything else should be just "const checking", which should (eventually) be the same for const and const fn. What am I missing?

@oli-obk
Copy link
Contributor Author

oli-obk commented Aug 23, 2018

What am I missing?

The checks for Drop and Freeze/UnsafeCell to make sure statics don't contain Freeze and constants don't contain Drop values are two further passes.

@@ -477,6 +491,15 @@ pub fn check_unsafety<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, def_id: DefId) {
.note(&details.as_str()[..])
.emit();
}
UnsafetyViolationKind::MinConstFn => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

fun side effect: the two cases below which are future compat lints are hard errors inside const fns

@rust-highfive

This comment has been minimized.

src/test/ui/consts/min_const_fn/min_const_fn.rs Outdated Show resolved Hide resolved
src/libsyntax/diagnostic_list.rs Outdated Show resolved Hide resolved
src/test/ui/consts/min_const_fn/min_const_fn.rs Outdated Show resolved Hide resolved
src/librustc/ty/context.rs Outdated Show resolved Hide resolved
src/librustc/ty/context.rs Outdated Show resolved Hide resolved
src/librustc_mir/transform/qualify_min_const_fn.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@Centril Centril left a comment

Choose a reason for hiding this comment

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

Some more tests... :)

src/test/ui/consts/min_const_fn/min_const_fn_fn_ptr.rs Outdated Show resolved Hide resolved
src/test/ui/consts/min_const_fn/min_const_fn_fn_ptr.rs Outdated Show resolved Hide resolved
const fn no_inner_dyn_trait2(x: Hide) {
x.0.field;
//~^ ERROR function pointers in const fn
}
const fn no_inner_dyn_trait_ret() -> Hide { Hide(HasPtr { field }) }
//~^ ERROR function pointers in const fn

Copy link
Contributor

Choose a reason for hiding this comment

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

I specifically do want a test here that ensures that even without access, const fn no_inner_fn_ptr2(x: Hide) { } is banned.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That requires a lot of code... I'll impl it, but I don't think it's a good addition to rustc

Copy link
Contributor

@Centril Centril Aug 24, 2018

Choose a reason for hiding this comment

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

So you already have the erroring tests:

const fn no_dyn_trait(_x: &dyn std::fmt::Debug) {}
const fn no_fn_ptrs(_x: fn()) {}

This is good, because it leaves open the possibility that this means &dyn const std::fmt::Debug or const fn(); If we didn't reject this, then users could pass in values where you don't have a const implementation of std::fmt::Debug, and so we wouldn't be able to change this in the future. However, as for situations where you have:

struct Hide<'a>(&'a dyn Debug);
const fn foo(_x: Hide) {}

... this would only prevent a situation in the future where we decided to split up our nominal type universe into one for the const restriction and one for impure... This seems like a very unlikely future... as such... I think the tests you have currently are sufficient.

So you don't need to implement it.

@rust-highfive

This comment has been minimized.

@Centril
Copy link
Contributor

Centril commented Aug 24, 2018

Should be fine to merge once the tests pass. :)
Thanks for doing this!

@rust-highfive

This comment has been minimized.

@bors

This comment has been minimized.

@rust-highfive

This comment has been minimized.

@oli-obk
Copy link
Contributor Author

oli-obk commented Aug 27, 2018

@Centril this is ready now I think

@oli-obk
Copy link
Contributor Author

oli-obk commented Aug 31, 2018

So... while I could actually implement a check that validates the MIR of constants in min_const_fn terms, this would be a major undertaking, requiring a new query and very hacky inspection of the MIR of constants at weird intervals.

Can we just decide to treat const items as boundaries for stability? Meaning a constant may access unstable constants, even if said constant and const fns is used in a min_const_fn. There's basically no forward compat issue, as we can replace any unstable const fn by a macro and get pretty much the exact same thing. And using an unstable or even internal macro to compute a stable constant's value seems uncontroversial.

Alternatively I can rewrite the entire min_const_fn checker and fiddle it into the const_qualif pass. I don't trust myself to implement that blacklisting scheme correctly though.

@RalfJung
Copy link
Member

I am perfectly fine with treating const as boundary for all sorts of analysis -- IMHO we should never inspect the code of another const, only ever its type+value. And for the value, we should be able to rely on it being const-safe for the given type.

@Centril
Copy link
Contributor

Centril commented Aug 31, 2018

@oli-obk I'm fine with this; I guess T-libs will just be a bit more careful with respect to const items :)

@Centril
Copy link
Contributor

Centril commented Sep 1, 2018

@bors r=Centril,varkor

@bors
Copy link
Contributor

bors commented Sep 1, 2018

📌 Commit 2839f4f has been approved by Centril,varkor

@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 Sep 1, 2018
@bors
Copy link
Contributor

bors commented Sep 1, 2018

⌛ Testing commit 2839f4f with merge fea32f1...

bors added a commit that referenced this pull request Sep 1, 2018
Implement the `min_const_fn` feature gate

cc @RalfJung @eddyb

r? @Centril

implements the feature gate for #53555

I added a hack so the `const_fn` feature gate also enables the `min_const_fn` feature gate. This ensures that nightly users of `const_fn` don't have to touch their code at all.

The `min_const_fn` checks are run first, and if they succeeded, the `const_fn` checks are run additionally to ensure we didn't miss anything.
@bors
Copy link
Contributor

bors commented Sep 1, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: Centril,varkor
Pushing fea32f1 to master...

@jethrogb
Copy link
Contributor

jethrogb commented Sep 17, 2018

I had this code in a PR I was working on, how should I write this now?

impl<T> UnsafeList<T> {
    pub const fn new() -> Self {
        unsafe {
            UnsafeList {
                head_tail: NonNull::new_unchecked(1 as _),
                head_tail_entry: None
            }
        }
    }
}

@jethrogb
Copy link
Contributor

jethrogb commented Sep 17, 2018

Minimal repro on playground:

#![feature(staged_api, const_fn)]
use std::ptr::NonNull;

pub struct UnsafeList<T>(NonNull<T>);

impl<T> UnsafeList<T> {
    pub const fn new() -> Self {
        unsafe {
            UnsafeList(NonNull::new_unchecked(1 as _))
        }
    }
}

The only way to get around this is to add an unstable attribute.

@Centril
Copy link
Contributor

Centril commented Sep 17, 2018

@jethrogb Is this for a PR to the standard library or the compiler, or is this for code outside of that?
If it is the latter, it is simply unstable and you won't be able to do it.

@jethrogb
Copy link
Contributor

PR for std

@Centril
Copy link
Contributor

Centril commented Sep 17, 2018

@jethrogb if UnsafeList::new is marked as #[rustc_const_unstable(feature="<name>")] then you can use powers that const_fn gives you, but that will also make the constness unstable. We will not permit any new APIs in the standard library to define stable const fns that do not pass min_const_fn.

@jethrogb
Copy link
Contributor

@Centril that is not sufficient. You also need to add an unstable attribute where this wasn't needed before, even if you explicitly set the const_fn feature, see my minimal repro.

@Centril
Copy link
Contributor

Centril commented Sep 17, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants