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

Use const generics for array Default impl #61415

Open
1 of 3 tasks
varkor opened this issue May 31, 2019 · 19 comments
Open
1 of 3 tasks

Use const generics for array Default impl #61415

varkor opened this issue May 31, 2019 · 19 comments
Labels
A-array Area: `[T; N]` A-const-generics Area: const generics (parameters and arguments) A-slice Area: `[T]` Libs-Tracked Libs issues that are tracked on the team's project board. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@varkor
Copy link
Member

varkor commented May 31, 2019

Currently, we generate array impls for every size up to 32 manually using macros, but with const generics at a suitable level of implementation, we can switch to properly parameterising over all lengths.

  • Replace most manual impls with const generic impls (PR: Use const generics for array impls [part 1] #62435)
  • Replace Default impl with const generic impl (more difficult due to Switch libcore array implementations to const generics. #60466 (comment)).
  • Reimplement impls removed due to bloat using const generic impls (see

    rust/src/libcore/array.rs

    Lines 221 to 227 in 7840a0b

    // NOTE: some less important impls are omitted to reduce code bloat
    __impl_slice_eq1! { [A; $N], [B; $N] }
    __impl_slice_eq2! { [A; $N], [B] }
    __impl_slice_eq2! { [A; $N], &'b [B] }
    __impl_slice_eq2! { [A; $N], &'b mut [B] }
    // __impl_slice_eq2! { [A; $N], &'b [B; $N] }
    // __impl_slice_eq2! { [A; $N], &'b mut [B; $N] }
    and

    rust/src/liballoc/vec.rs

    Lines 2199 to 2204 in bfdfa85

    // NOTE: some less important impls are omitted to reduce code bloat
    // FIXME(Centril): Reconsider this?
    //__impl_slice_eq1! { [const N: usize] Vec<A>, &mut [B; N], [B; N]: LengthAtMost32 }
    //__impl_slice_eq1! { [const N: usize] Cow<'a, [A]>, [B; N], [B; N]: LengthAtMost32 }
    //__impl_slice_eq1! { [const N: usize] Cow<'a, [A]>, &[B; N], [B; N]: LengthAtMost32 }
    //__impl_slice_eq1! { [const N: usize] Cow<'a, [A]>, &mut [B; N], [B; N]: LengthAtMost32 }
    )
@varkor varkor added T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API 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-generics Area: const generics (parameters and arguments) labels May 31, 2019
@Centril Centril added the C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC label May 31, 2019
@Centril
Copy link
Contributor

Centril commented May 31, 2019

As I noted in #60466 (comment) and #60466 (comment) I will not sign off on using const generics in stable Rust until such time that const generics are stable. However, we can provide unstable wrapper types meanwhile.

@petrochenkov
Copy link
Contributor

We can probably both keep the exact observable behavior (impls for 0-32) and reduce metadata bloat by using a trick similar to #60466 (comment) (an empty marker trait implemented using macros + the real impl using const generics and a where clause with the marker trait).

This way const generics would be an implementation detail, so their stabilization wouldn't be required.

@varkor
Copy link
Member Author

varkor commented Jun 3, 2019

The results from the crater run and perf run for using const generics for array impls are in (#60466).

I think there's good reason to go with @petrochenkov's suggestion (#61415 (comment)) and potentially consider lifting the restriction for array sizes.

@pnkfelix
Copy link
Member

pnkfelix commented Jun 6, 2019

does T-compiler actually need to be tagged on this issue? Unless its exposing bugs for const generics themselves, I would think this is solely a T-libs issue, maybe T-lang, but not T-compiler?

@alexcrichton
Copy link
Member

This libs team discussed this yesterday and agreed that we do not want to expand the stable surface API of the standard library, but changing implementaitons to use const generics seems fine so long as it doesn't expand the surface area of what's exposed (e.g. via @petrochenkov's idea)

@nikomatsakis
Copy link
Contributor

The @rust-lang/lang team discussed this and we agree with the libs team. =)

@scottmcm
Copy link
Member

scottmcm commented Jul 6, 2019

No-expanded-surface area (#61415 (comment)) PR up at #62435

bors added a commit that referenced this issue Jul 7, 2019
Use const generics for array impls [part 1]

Part 1 of #61415, following the plan in #61415 (comment)

Found a way that works 🙃
@RalfJung
Copy link
Member

Turns out using const generics inside the actual code (which currently only try_from does) breaks Miri. @oli-obk will hopefully be able to look into this next week, but until then it would be great if we couldn't land more PRs that use const generic in this way in libstd.

@eddyb
Copy link
Member

eddyb commented Jul 16, 2019

@RalfJung Can you trigger that from CTFE, or does it require runtime features?
It would be nice to have a issue with a testcase that ICEs/errors from miri.

@RalfJung
Copy link
Member

RalfJung commented Jul 16, 2019

ICE from Miri is easy:

use std::convert::TryFrom;

fn main() {
    const N: usize = 16;
    type Array = [u8; N];
    let array: Array = [0; N];
    let slice: &[u8] = &array[..];

    let result = <&Array>::try_from(slice);
    assert_eq!(&array, result.unwrap());
}

Here is a self-contained version.

@eddyb
Copy link
Member

eddyb commented Jul 16, 2019

If I try to make fn len a const fn I get this:

error: const parameters are not permitted in `const fn`

So I'd suggest fixing that first (seems like an unnecessary limitation while const generics are unstable - worst case it can just get its own feature gate).

After that, the entire repro is this:

#![feature(const_generics)]
const fn len<T, const N: usize>(_: [T; N]) -> usize { N }
const FOO: usize = len([0]);

My guess is that miri isn't monomorphizing ("substituting"?) Operand::Constants.

@RalfJung
Copy link
Member

My guess is that miri isn't monomorphizing ("substituting"?) Operand::Constants.

Indeed, and I have a patch for that but it ICEs. See #61041 (comment).

@eddyb
Copy link
Member

eddyb commented Jul 16, 2019

@RalfJung I left a comment, you shouldn't be able to trigger that ICE that easily.

bors added a commit that referenced this issue Jul 28, 2019
In which we constantly improve the Vec(Deque) array PartialEq impls

Use the same approach as in #62435 as sanctioned by #61415 (comment).

r? @scottmcm
@alex
Copy link
Member

alex commented Aug 9, 2020

As of #74060 these are now being used on a bunch of traits -- possibly everywhere except Default, which is challenging for reasons discussed (but is being expiremented with in #74254)

@MikailBag
Copy link
Contributor

MikailBag commented Aug 9, 2020

TL;DR there are two problems with my PR:

  1. If we want that Default impls play nicely with the rest of compiler, we have to use marker traits + specialization. It seems they are not going to be stable in the enar future. There were other approaches proposed, but they have problems with trait selection, type inference and similar stuff. AFAIK this problem waits for a T-lang decision.
  2. Current implementation suffers from codegen issue MaybeUninit::assume_init optimizes poorly #74267: generated code is worse than with current macro-generated impls, which leads to performance loss.

ColinFinck added a commit to ColinFinck/binread that referenced this issue Mar 27, 2021
This increases the minimum supported Rust version to 1.51.0.
I sadly had to introduce another unsafe statement, because:
* `Default` is only implemented for array lengths up to 32
  (rust-lang/rust#61415)
* arr_macro doesn't support const generics
  (JoshMcguigan/arr_macro#2)
* Direct initialization via [T; N] adds a trait bound
  BinRead: Copy
ColinFinck added a commit to ColinFinck/binread that referenced this issue Mar 27, 2021
This increases the minimum supported Rust version to 1.51.0.
I sadly had to introduce another unsafe statement, because:
* `Default` is only implemented for array lengths up to 32
  (rust-lang/rust#61415)
* arr_macro doesn't support const generics
  (JoshMcguigan/arr_macro#2)
* Direct initialization via [T; N] adds a trait bound
  `BinRead: Copy`
jam1garner pushed a commit to jam1garner/binread that referenced this issue Mar 28, 2021
This increases the minimum supported Rust version to 1.51.0.
I sadly had to introduce another unsafe statement, because:
* `Default` is only implemented for array lengths up to 32
  (rust-lang/rust#61415)
* arr_macro doesn't support const generics
  (JoshMcguigan/arr_macro#2)
* Direct initialization via [T; N] adds a trait bound
  `BinRead: Copy`
@joshtriplett
Copy link
Member

joshtriplett commented Jun 1, 2022

@oli-obk @rust-lang/wg-const-eval @rust-lang/types Is there any potential future change on the horizon that would allow handling "either T: Default or N == 0" without needing full overlapping implementations?

@oli-obk
Copy link
Contributor

oli-obk commented Jun 1, 2022

Not that I know of. Though I would love to be able to specify patterns like in matches to restrict const generic impls to something that we already know how to check for overlap (or lack thereof)

@ryoqun
Copy link
Contributor

ryoqun commented Jun 9, 2024

dumb question. Can this be fixed for the edition 2024?

@Skgland
Copy link
Contributor

Skgland commented Jul 31, 2024

I think the problem of the overlapping impl where T: Default and N == 0 is still open.

impl <T> Default for [T; 0] {...}
impl <T, const N: usize> Default for [T; N] {...}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-array Area: `[T; N]` A-const-generics Area: const generics (parameters and arguments) A-slice Area: `[T]` Libs-Tracked Libs issues that are tracked on the team's project board. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests