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

support anon consts in binders #79313

Closed
wants to merge 10 commits into from
Closed

support anon consts in binders #79313

wants to merge 10 commits into from

Conversation

lcnr
Copy link
Contributor

@lcnr lcnr commented Nov 22, 2020

closes #72129

The first 2 commits are taken from #79298 which are used in some of the added tests.

This allows us to deal with anonymous constants inside of binders, for example struct Foo<T>(T) where for<'a> [T; { let _: &'a (); 3 }]: Sized;.

What's the problem and how am I trying to solve it?

Looking at the above example, 'a is bound region. As we typeck the anonymous constant separately we would pretty much always work inside of the binder and having bound regions is not supported there.

What we instead want is to treat 'a inside of the anon const as an early bound region. To achieve this we to add some additional generic params to the created AnonConst.

So we pretty much want to desugar the above example into.
Note that we add another generic param to ANON_CONST.

const ANON_CONST<T, 'a> = { let _: &'a (); 3 };
struct Foo<T>(T) where for<'a> [T; ANON_CONST::<T, 'a>]: Sized;

To achieve this we extend hir::AnonConst to remember both it's new synthetic generic parameters as well as the potentially use lifetime arguments originating from a binder. This is now working well enough to warrant a review.

Once the general approach is ironed out I still want to update lifetime resolution to warn for for<'a> [u8; 3 + 4]: Sized that 'a is unused. We also still have to think a bit about perf, though we can probably put the generics into a optional allocation.

r? @matthewjasper

@lcnr lcnr added A-const-generics Area: const generics (parameters and arguments) F-const_generics `#![feature(const_generics)]` labels Nov 22, 2020
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 22, 2020
));
// FIXME(const_generics): Do we actually end up having to deal with
// this bound later or is this unsound?
if !substs.has_escaping_bound_vars() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

cc @eddyb

Can we somehow circumvent this? 🤔

I don't think we can just replace the bound vars with placeholders here either.

I think that places which actually try to evaluate the constant do still have to satisfy the ConstEvaluatable predicate but it does allow for

// check-pass
#![feature(const_generics)]
#![allow(incomplete_features)]

trait Baz<const N: usize> {}

fn test<T>() {
    // FIXME(const_generics): This should error.
    let _a: Box<dyn for<'a> Baz<{
        let _: &'a ();
        std::mem::size_of::<T>()
    }>>;
}

fn main() {
    test::<u32>();
}

which is less than ideal

@lcnr lcnr force-pushed the narnia branch 3 times, most recently from 641de1b to fd6e7c0 Compare November 24, 2020 18:33
@lcnr lcnr marked this pull request as ready for review November 24, 2020 18:38
@lcnr lcnr changed the title [WIP] support anon consts in binders support anon consts in binders Nov 24, 2020
@lcnr
Copy link
Contributor Author

lcnr commented Nov 24, 2020

cc @varkor @eddyb @nikomatsakis, this might also be interesting to you

@bors
Copy link
Contributor

bors commented Nov 29, 2020

☔ The latest upstream changes (presumably #79511) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

@@ -1371,7 +1373,7 @@ pub struct Expr<'hir> {

// `Expr` is used a lot. Make sure it doesn't unintentionally get bigger.
#[cfg(target_arch = "x86_64")]
rustc_data_structures::static_assert_size!(Expr<'static>, 72);
rustc_data_structures::static_assert_size!(Expr<'static>, 144);
Copy link
Contributor

Choose a reason for hiding this comment

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

Would replacing AnonConst by &'hir AnonConst<'hir> help with the size?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, probably 👍

I want to wait for a first review before looking into perf though, so I don't waste time on an approach we don't want in the end

@crlf0710 crlf0710 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 18, 2020
@bors
Copy link
Contributor

bors commented Dec 20, 2020

☔ The latest upstream changes (presumably #80163) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

@crlf0710 crlf0710 removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 15, 2021
@crlf0710 crlf0710 added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 15, 2021
@JohnCSimon
Copy link
Member

@rustbot label: -S-waiting-on-review +S-waiting-on-author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 1, 2021
@Dylan-DPC-zz
Copy link

@lcnr any updates on this?

@lcnr
Copy link
Contributor Author

lcnr commented Feb 18, 2021

want to talk about this with project-const-generic in the near future before doing much here.

@JohnCSimon JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 7, 2021
@crlf0710 crlf0710 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. S-experimental Status: Ongoing experiment that does not require reviewing and won't be merged in its current state. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 26, 2021
@crlf0710 crlf0710 added S-experimental Status: Ongoing experiment that does not require reviewing and won't be merged in its current state. and removed S-experimental Status: Ongoing experiment that does not require reviewing and won't be merged in its current state. labels Apr 17, 2021
@crlf0710 crlf0710 added S-experimental Status: Ongoing experiment that does not require reviewing and won't be merged in its current state. and removed S-experimental Status: Ongoing experiment that does not require reviewing and won't be merged in its current state. labels May 8, 2021
@crlf0710 crlf0710 added S-experimental Status: Ongoing experiment that does not require reviewing and won't be merged in its current state. and removed S-experimental Status: Ongoing experiment that does not require reviewing and won't be merged in its current state. labels Jun 5, 2021
@crlf0710 crlf0710 added S-experimental Status: Ongoing experiment that does not require reviewing and won't be merged in its current state. and removed S-experimental Status: Ongoing experiment that does not require reviewing and won't be merged in its current state. labels Jun 25, 2021
@lcnr
Copy link
Contributor Author

lcnr commented Jul 8, 2021

probably want to wait with this pr until @rust-lang/project-const-generics figures out how to deal with unused params in general as that will influence the way we want to implement this change.

@lcnr lcnr closed this Jul 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-const-generics Area: const generics (parameters and arguments) F-const_generics `#![feature(const_generics)]` S-experimental Status: Ongoing experiment that does not require reviewing and won't be merged in its current state.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Post lazy normalization for consts cleanup (#71973)
9 participants