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 Default for all arrays #84838

Closed
wants to merge 3 commits into from
Closed

Conversation

lcnr
Copy link
Contributor

@lcnr lcnr commented May 2, 2021

Using feature(marker_trait_attr) and a new lang item array_default_hack, we're able to implement Default for generic arrays without relying on any too unstable features.

cc https://rust-lang.zulipchat.com/#narrow/stream/260443-project-const-generics/topic/array.20default.20impls

r? @nikomatsakis 💖

@lcnr lcnr 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. labels May 2, 2021
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 2, 2021
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@lcnr lcnr marked this pull request as draft May 2, 2021 22:49
@lcnr
Copy link
Contributor Author

lcnr commented May 2, 2021

ok, we definitely need to use marker_trait_attr.

We only consider the auto trait impl if the manual impl does not shallowly match. So if our manual impl has a ty param as the self type, we never actually use the auto trait one.

If we manually impl the 0 case it influences type inference which is also bad.

@clarfonthey
Copy link
Contributor

Is there a reason why this has to add extra lang items? Why not just use MaybeUninit::uninit_array and set Default::default() to each item?

I'm a bit confused why the N = 0 case is a problem,

@lcnr
Copy link
Contributor Author

lcnr commented May 3, 2021

I'm a bit confused why the N = 0 case is a problem,

Default is implemented for [T; 0] even if T does not implement default. We can't use <T as Default>::default() in the implementation directly because T might not implement it.

@jhpratt
Copy link
Member

jhpratt commented May 3, 2021

Worth noting that with min_specialization the lang item wouldn't be necessary as you could impl<T: Default, const N: usize> Default for [T; N] and impl<T> Default for [T; 0] at the same time. As such, this lang item should be temporary in the long-term.

@coolreader18
Copy link
Contributor

you could impl<T: Default, const N: usize> Default for [T; N] and impl<T> Default for [T; 0]

Could you? I was under the impression specialization on const generics was more complicated/unimplemented/at least requires const_evaluatable_unchecked

@jhpratt
Copy link
Member

jhpratt commented May 3, 2021

Yeah, looks like currently on nightly this doesn't work. I would think that this could be done in the future, though. My point of it being temporary in the long-term is still relevant.

error[E0275]: overflow evaluating the requirement `[_; 0]: Default`
  |
  = help: consider adding a `#![recursion_limit="256"]` attribute to your crate (`default-demo`)
  = note: required because of the requirements on the impl of `Default` for `[_; 0]`
  = note: 127 redundant requirements hidden
  = note: required because of the requirements on the impl of `Default` for `[_; 0]`

@lcnr lcnr force-pushed the array-default branch from 8066470 to 1ed03a2 Compare May 4, 2021 14:40
@nikomatsakis
Copy link
Contributor

We discussed this in the const generics meeting this morning. This intersects and somewhat constraints specialization in that it uses a lang-item to effectively simulate a pair of impls:

  • one for [T; 0]
  • and one for [T; N] where T: Default

This cannot be covered by the specialization plans as they stand today. It is conceivable, though, to imagine accepting it in the future. The key would be that we don't need to know whether T: Default in order to know if the second impl applies, we only hav to know if N == 0 or not (and the type checker will have guaranteed T: Default). So we don't need all the "always applicable" machinery. I'm not sure how to express this idea logically yet, haven't tried.

There would be a problem if you have a further impl like [T; N] where T: Default + Debug, but that's a separate issue.

@nikomatsakis
Copy link
Contributor

nikomatsakis commented May 12, 2021

I'm going to nominate for @rust-lang/lang discussion on the previous comment.

@bors
Copy link
Contributor

bors commented May 12, 2021

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

@m-ou-se m-ou-se added T-libs Relevant to the library team, which will review and decide on the PR/issue. and removed T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Jun 23, 2021
@joshtriplett
Copy link
Member

We discussed this in today's @rust-lang/libs-api meeting, and we're in favor of having this Default impl, as long as @rust-lang/lang is comfortable with the hack(s) necessary to make it possible.

@m-ou-se
Copy link
Member

m-ou-se commented Jun 29, 2021

From a library implementation perspective: The code that this PR adds in library/core/src/array/mod.rs does not look like something we can maintain. If a PR comes in that changes that code, I do not think we have the right reviewers to be able to judge not only correctness, but also what such a change means for our stability guarantees. I'd like to avoid having blobs of 'don't touch this'-code in the library.

@nikomatsakis
Copy link
Contributor

So in our @rust-lang/lang meeting on 2021-06-29, we discussed this proposal again, and in particular we discussed @m-ou-se's implementation concerns. Our sense is that we feel comfortable with the idea of [T; N] implementing Default when either T: Default or N = 0, but if this particular route of achieving that goal feels like an overly hacky or risky maintenance burden, we don't want to pursue it. We'd prefer to add a cleaner language feature to that end. Therefore, I'm going to close this PR. Thanks @lcnr for the clever idea. 💜

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants