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

Consider types appearing in const expressions to be invariant #89829

Merged
merged 3 commits into from
Oct 23, 2021

Conversation

voidc
Copy link
Contributor

@voidc voidc commented Oct 12, 2021

This is an approach to fix #80977.
Currently, a type parameter which is only used in a constant expression is considered bivariant and will trigger error E0392 "parameter T is never used".
Here is a short example:

pub trait Foo {
    const N: usize;
}

struct Bar<T: Foo>([u8; T::N])
where [(); T::N]:;

(playgound)

While it is possible to silence this error by adding a PhantomData<T> field, I think the better solution would be to make T invariant.
This would be analogous to the invariance constraints added for associated types.
However, I'm quite new to the compiler and unsure whether this is the right approach.

r? @varkor (since you authored #60058)

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

This comment has been minimized.

@varkor
Copy link
Member

varkor commented Oct 12, 2021

Thanks for looking into this issue! I don't have a lot of time to review right now, so I'm going to reassign to @lcnr, who's up-to-date on the const generics internals.

r? @lcnr

@rust-highfive rust-highfive assigned lcnr and unassigned varkor Oct 12, 2021
@lcnr
Copy link
Contributor

lcnr commented Oct 15, 2021

Thanks for the PR 👍

I am a bit unsure about this. As variance shouldn't influence the result of const eval (and any well formed requirements are checked for the parent anyways) we could let parameters only used in constants be bivariant afaik. Now, doing that doesn't actually fix the "unused parameter" errors, as these get triggered for all bivariant parameters which are not the target of a projection. We could change this to also allow bivariant parameters if they are used by constants, which I probably prefer 🤔

Making the substs invariant prevents the following from compiling, which is a bit unfortunate:

#![feature(generic_const_exprs)]
struct Foo<'a>([&'a u32; std::mem::size_of::<&'a u32>()]);
fn covariant<'a>(v: &'a Foo<'static>) -> &'a Foo<'a> {
    v
}

We generally have the issue that unevaluated.substs(tcx) isn't perfect. The generic arguments of an anonymous constant include parameters which aren't actually used by the constant. Right now we include all generic arguments, but even after filtering them, we will still include slightly too many as we can't wait until after we've typechecked the constant to figure out which generic parameters are used.

Considering the generic parameters used by constants to be "actually used" therefore has some forward compatibility concerns (once generic_const_exprs is stable):

#![feature(generic_const_exprs)]
struct Foo<T, const N: usize>([u8; N + 1]) where [u8; N + 1]: Trait<T>;
trait Trait<T> {}
impl<T, U> Trait<T> for U {}

The way we currently intend to filter the generic arguments supplied to anonymous constants should end up considering T to be used by N + 1. So this would compile. If we then find a need to improve this check or think of a better approach, changing this would be a break this code asT would then be unused.

This will probably be fine as writing code like that by accident is probably pretty rare, so even if this causes theoretical breakage, it shouldn't be an issue in real code.

Another solution to this far off potential breakage would be to change the "parameter never used" errors to be a deny by default lint. Having generic parameters be unused or bivariant isn't unsound so this way we should be able to allow this as a lint.


We could change this to also allow bivariant parameters if they are used by constants, which I probably prefer 🤔

@rust-lang/project-const-generics thoughts here?

@lcnr
Copy link
Contributor

lcnr commented Oct 21, 2021

talked with @nikomatsakis about this and came to the following conclusions if i remember correctly:

  • as long as the coherence_leak_check future compat lint is not an error having the substs of constants be bivariant is unsound, consider the following example
#![feature(generic_const_exprs)]
use std::marker::PhantomData;

trait SadBee {
    const ASSOC: usize;
}
// fn(&'static ())` is a supertype of `for<'a> fn(&'a ())` while
// we allow two different impls for these types, leading
// to different const eval results.
impl SadBee for for<'a> fn(&'a ()) {
    const ASSOC: usize = 0;
}
impl SadBee for fn(&'static ()) {
    const ASSOC: usize = 100;
}

struct Foo<T: SadBee>([u8; <T as SadBee>::ASSOC], PhantomData<T>) where [(); <T as SadBee>::ASSOC]:,;

fn covariant(v: &'static Foo<for<'a> fn(&'a ())>) -> &'static Foo<fn(&'static ())> {
    v
}

fn main() {
    let y = covariant(&Foo([], PhantomData));
    println!("{:?}", y.0);
}
  • making the substs invariant is therefore probably the right choice, if a bit unfortunate
  • I am going to open an issue to improve this in the future once this PR is merged and also add the above example as a test.

@lcnr lcnr marked this pull request as ready for review October 21, 2021 19:11
@lcnr
Copy link
Contributor

lcnr commented Oct 21, 2021

going to approve this PR even if it was previously a draft as it looks ready to me. Good job 👍

@bors r+

@bors
Copy link
Contributor

bors commented Oct 21, 2021

📌 Commit a400f10 has been approved by lcnr

@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 21, 2021
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 23, 2021
Consider types appearing in const expressions to be invariant

This is an approach to fix rust-lang#80977.
Currently, a type parameter which is only used in a constant expression is considered bivariant and will trigger error E0392 *"parameter T is never used"*.
Here is a short example:

```rust
pub trait Foo {
    const N: usize;
}

struct Bar<T: Foo>([u8; T::N])
where [(); T::N]:;
```
([playgound](https://play.rust-lang.org/?version=nightly&mode=debug&edition=2015&gist=b51a272853f75925e72efc1597478aa5))

While it is possible to silence this error by adding a `PhantomData<T>` field, I think the better solution would be to make `T` invariant.
This would be analogous to the invariance constraints added for associated types.
However, I'm quite new to the compiler and unsure whether this is the right approach.

r? `@varkor` (since you authored rust-lang#60058)
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 23, 2021
…askrgr

Rollup of 5 pull requests

Successful merges:

 - rust-lang#85833 (Scrape code examples from examples/ directory for Rustdoc)
 - rust-lang#88041 (Make all proc-macro back-compat lints deny-by-default)
 - rust-lang#89829 (Consider types appearing in const expressions to be invariant)
 - rust-lang#90168 (Reset qualifs when a storage of a local ends)
 - rust-lang#90198 (Add caveat about changing parallelism and function call overhead)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 2b874f0 into rust-lang:master Oct 23, 2021
@rustbot rustbot added this to the 1.58.0 milestone Oct 23, 2021
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 8, 2021
add const generics test

cc rust-lang#89829 (comment)

r? rust-lang/project-const-generics
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 8, 2021
add const generics test

cc rust-lang#89829 (comment)

r? rust-lang/project-const-generics
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 8, 2021
add const generics test

cc rust-lang#89829 (comment)

r? rust-lang/project-const-generics
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 18, 2021
add const generics test

cc rust-lang#89829 (comment)

r? rust-lang/project-const-generics
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Nov 18, 2021
add const generics test

cc rust-lang#89829 (comment)

r? rust-lang/project-const-generics
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 18, 2021
add const generics test

cc rust-lang#89829 (comment)

r? rust-lang/project-const-generics
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Nov 19, 2021
add const generics test

cc rust-lang#89829 (comment)

r? rust-lang/project-const-generics
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.

Using associated constants for generics is not considered using a type generic
7 participants