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

Const stability on impls doesn't make sense #5

Open
compiler-errors opened this issue Oct 24, 2024 · 8 comments
Open

Const stability on impls doesn't make sense #5

compiler-errors opened this issue Oct 24, 2024 · 8 comments

Comments

@compiler-errors
Copy link
Member

Const stability on impls makes no sense in the same way that stability on impls makes no sense, since you can always put a const call behind some generic type parameter and only see the impl after substitution/monomorphization.

We currently have code in the MIR const validation to handle const impls, and we shouldn't.

@compiler-errors
Copy link
Member Author

The dilemma here is that we allow unstable on impls, so I guess we should keep allowing rustc_const_unstable on impls, too. We just obviously shouldn't enforce anything about them.

@compiler-errors
Copy link
Member Author

compiler-errors commented Oct 25, 2024

Notice how, on 1.65:

#![feature(const_trait_impl)]

use std::ops::Add;
use std::marker::Destruct;

const fn foo(t: i32, t2: i32) {
    Add::add(t, t2);
}

const fn test() {
    foo(1, 2);
}

errors with:

error: `<i32 as Add>::add` is not yet stable as a const fn
 --> <source>:7:5
  |
7 |     Add::add(t, t2);
  |     ^^^^^^^^^^^^^^^
  |
  = help: add `#![feature(const_ops)]` to the crate attributes to enable

But I can write:

#![feature(const_trait_impl)]

use std::ops::Add;
use std::marker::Destruct;

const fn foo<T: ~const Add<Output = T> + ~const Destruct>(t: T, t2: T) {
    Add::add(t, t2);
}

const fn test() {
    foo(1, 2);
}

and it works totally fine.

@RalfJung
Copy link
Member

The dilemma here is that we allow unstable on impls, so I guess we should keep allowing rustc_const_unstable on impls, too. We just obviously shouldn't enforce anything about them.

Why should we repeat the mistake? We should disallow both, since they can't reasonably have an effect with the current solver.

@RalfJung
Copy link
Member

RalfJung commented Oct 25, 2024

Regarding the examples -- we can stage the rollout of const trait in various ways:

  1. All at once, i.e. once const_trait_impl is stable all const impl for stable traits are immediately available on stable
  2. Trait-for-trait, i.e. a stable trait has to be separately marked as #[rustc_const_stable] to allow ~const Trait bounds and trait method calls in const fn
  3. Trait-fn-trait-fn, i.e. even in a const-stable trait we can separately control which functions can be const-called on stable.

It's hard for me to say how much granularity we will need.

@compiler-errors
Copy link
Member Author

I think (2.) makes sense.

Thinking about what additional stability guarantees should be imposed to fix this:

  1. Traits need to have their own const stability. Probably with the same rules as @RalfJung's const stability rework, i.e. either marked const-stable/const-unstable if theyre stable (idk if we want to allow them to be marked const-indirect-stable?), or unmarked/const-unstable if they're unstable.
  2. I think we should additionally validate that all const impls of a const-stable trait are marked const-stable (or perhaps const-indirect-stable if the impl is unstable?), and probably that all const impls of an const-unstable trait remain marked const-unstable, unless we want to make it so that calls to specific const impls are stable before the trait, for example, becomes stable.

@compiler-errors
Copy link
Member Author

For now, I guess we can just always require const traits and impls to be const-unstable (either explicitly, or implicitly via #[unstable], like regular const fn)

@compiler-errors
Copy link
Member Author

cc rust-lang/rust#132280

@RalfJung
Copy link
Member

I think (2.) makes sense.

Okay, I opened #16 for some more concrete discussion of how that could look like.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants