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

Deprecate uninit_array #101179

Closed
wants to merge 3 commits into from
Closed

Conversation

SUPERCILEX
Copy link
Contributor

@SUPERCILEX SUPERCILEX commented Aug 30, 2022

Now that #98304 has gone through, uninit_array should be removed since creating a const array is the One True Way of doing things.

Tracking issue: #96097

Closes #66845

Blocked on: rust-lang/libs-team#122 (comment)

@rustbot rustbot added T-compiler Relevant to the compiler 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. labels Aug 30, 2022
@rust-highfive
Copy link
Collaborator

r? @joshtriplett

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 30, 2022
@SUPERCILEX
Copy link
Contributor Author

r? @RalfJung

@rust-log-analyzer

This comment has been minimized.

@RalfJung
Copy link
Member

No objections from my side, but this should go through t-libs.
r? libs

@rust-highfive rust-highfive assigned m-ou-se and unassigned RalfJung Aug 30, 2022
@thomcc
Copy link
Member

thomcc commented Aug 30, 2022

I don't feel that strongly here, but I'm not sure I'm a fan of this removal. I've used MaybeUninit::uninit_array() many times (quite often when optimizing code in std to avoid zeroing buffers), and think it is a nice function to have.

Even if it doesn't have a performance benefit over [const { MaybeUninit::uninit() }; N], it has considerably less syntactic noise.

@thomcc
Copy link
Member

thomcc commented Aug 30, 2022

Actually, now that I look carefully the const { ... } block doesn't seem to be required to avoid the issues? I think I had misread the PR, and don't feel that strongly if that's not needed.

@RalfJung
Copy link
Member

const { ... } is required when T is not Copy.

@clarfonthey
Copy link
Contributor

My gut feeling is also against this, although I feel like the MaybeUninit methods for arrays could use an upgrade.

What if instead of using a method like this, we just directly implemented Index and IndexMut for MaybeUninit<[T; N]>? That way, the existing methods for other types could be used directly, and we'd still have an ergonomic way to use uninit arrays.

@SUPERCILEX
Copy link
Contributor Author

What if instead of using a method like this, we just directly implemented Index and IndexMut for MaybeUninit<[T; N]>? That way, the existing methods for other types could be used directly, and we'd still have an ergonomic way to use uninit arrays.

How would this work when you're trying to read initialized memory? Furthermore, I don't see your suggestion as incompatible with this PR: we still want to nuke this method since the language is now capable of expressing uninit_array in safe code.

@clarfonthey
Copy link
Contributor

Index and IndexMut just offset pointers and don't actually do any reads themselves; if you returned references to MaybeUninit<T> and MaybeUninit<[T]>, it wouldn't cause any issues.

In terms of deleting this method, keep in mind that it's not yet doable entirely with safe code, since T needs to be Copy or you need to enable the const expressions feature, which isn't stable yet. Even though you'll be on nightly for this method anyway, I don't think we should remove it until a valid alternative is stable.

@SUPERCILEX
Copy link
Contributor Author

In terms of deleting this method, keep in mind that it's not yet doable entirely with safe code, since T needs to be Copy or you need to enable the const expressions feature, which isn't stable yet.

That's actually not true, see how to do it in this docs update: 54014b7#diff-5e72af2fe825c2180f437d54adbdbb19bb40a9cc82bc6c53c2d993d72b08ce3fR149

Index and IndexMut just offset pointers and don't actually do any reads themselves

Forgot that those returned references. This was already attempted (#88837), but I can commit to giving it another spin and then we can nuke the last method in this tracking issue.

@clarfonthey
Copy link
Contributor

I would be in favour of doing that, especially if we can make arrays and slices work as expected without any dedicated methods.

In terms of deletion, even though this is unstable, I do think that we should deprecate it for a few weeks before outright removing it, since I and a lot of people have been using this method. This has been done before and feels pretty reasonable in this case.

@SUPERCILEX
Copy link
Contributor Author

I do think that we should deprecate it for a few weeks before outright removing it, since I and a lot of people have been using this method. This has been done before and feels pretty reasonable in this case.

Seems pretty reasonable, done.

@na-sa-do
Copy link

na-sa-do commented Sep 2, 2022

That's actually not true, see how to do it in this docs update: 54014b7#diff-5e72af2fe825c2180f437d54adbdbb19bb40a9cc82bc6c53c2d993d72b08ce3fR149

This technique isn't possible with a generic.

fn foo<T>() -> [MaybeUninit<T>; 4] {
    const UNINIT_T: MaybeUninit<T> = MaybeUninit::uninit(); // E0401
    [UNINIT_T; 4]
}

@SUPERCILEX
Copy link
Contributor Author

Dang, that's a bummer. Haven't had a chance to dig further, but assuming there's no way around this I still think deprecation removal should move forward since you need to be on nightly to use this feature anyway.

To address an earlier comment, I'm not seeing the "but you need to switch to another unstable feature" as a valid counterpoint without further evidence. You could say that other feature might not be stabilized and so we'll need to bring this back, but:

  • Since it's inline const, I find that highly unlikely.
  • Worst case scenario people can use unsafe manually while waiting for uninit_array to be brought back.
  • The index impls should make this point moot since people can use an uninitialized array directly.

@RalfJung
Copy link
Member

RalfJung commented Sep 2, 2022

https://github.com/RalfJung/inline-const works even when there are generics.

But anyway, the main point above was:

Even if it doesn't have a performance benefit over [const { MaybeUninit::uninit() }; N], it has considerably less syntactic noise.

@clarfonthey
Copy link
Contributor

This is why I proposed adding Index and IndexMut to arrays of MaybeUninit, since it would still allow initializing the individual parts of the array before using the non-array methods to initialize it.

This still doesn't provide a dedicated operation to "transpose" the uninit and array parts of the type, but it provides the same functionality without having to sacrifice readability.

@SUPERCILEX
Copy link
Contributor Author

I think a transpose is more valuable than index methods since you can convert back and forth as needed. So is the conclusion that we should do that first before this PR?

@clarfonthey
Copy link
Contributor

I don't think that transposing is a good idea because it makes it unclear which version of the transpose is preferred, and simply giving the option to make the top-level version usable seems preferable. Otherwise, we end up having several different ways of doing things without a clear reason to use one or another, which feels clunky.

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Oct 17, 2022
@@ -879,7 879,7 @@ where
}
}

let mut array = MaybeUninit::uninit_array::<N>();
let mut array = MaybeUninit::uninit().transpose();
Copy link
Member

Choose a reason for hiding this comment

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

Is it worth adding a type annotation?

Suggested change
let mut array = MaybeUninit::uninit().transpose();
let mut array = MaybeUninit::<[???; N]>::uninit().transpose();

There's a lot of inference going on here making the code quite hard to follow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm, yeah I was wondering about this too. My reasoning for this being ok was that if the compiler manages to infer the types then I don't need to think about them. I'm guessing this is flawed because if you have no constraints at all, then the compiler will happily give you an i32. An interesting question arises: are integers and floats the only two places where the compiler will ever pull a type out of its 🍑 if there are no constraints? If so, my gut feeling is that we should get rid of that behavior (separate discussion, I know) since not having 🍑 types would let you do what I'm doing now where I say, "well, the compiler did some inferences so whatever the types are they must be correct". Or maybe I'm thinking too hard about this and type annotations would make human consumption easier (though I'm still curious about the above questions).

Copy link
Member

Choose a reason for hiding this comment

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

In safe code I tend to follow the same approach -- if the compiler found a type that makes it all work, then that seems right (but I will still sometimes add type annotations to make the code more readable).

In unsafe code though, I don't like trusting the compiler like that. Types are much less meaningful in unsafe code (that's why the code is unsafe to begin with), so IMO type inference should only be relied upon for absolutely obvious cases -- which this one is not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, will do.

@m-ou-se
Copy link
Member

m-ou-se commented Oct 18, 2022

Even though you'll be on nightly for this method anyway, I don't think we should remove it until a valid alternative is stable.

I don't think we should try to maintain any sort of stability for nightly features. The point of nightly features is that they're experimental and that we can change or remove them at any time.

I feel like the MaybeUninit methods for arrays could use an upgrade.

Agreed

What if instead of using a method like this, we just directly implemented Index and IndexMut for MaybeUninit<[T; N]>? That way, the existing methods for other types could be used directly, and we'd still have an ergonomic way to use uninit arrays.

This RFC seems relevant: rust-lang/rfcs#3318

@m-ou-se

This comment was marked as off-topic.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-close This PR / issue is in PFCP or FCP with a disposition to close it. labels Oct 18, 2022
@m-ou-se

This comment was marked as off-topic.

@rfcbot

This comment was marked as off-topic.

@rfcbot rfcbot removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-close This PR / issue is in PFCP or FCP with a disposition to close it. labels Oct 18, 2022
@rust-lang rust-lang deleted a comment from rfcbot Oct 18, 2022
@m-ou-se
Copy link
Member

m-ou-se commented Oct 18, 2022

We briefly discussed this in the libs-api meeting. We prefer removing the function over deprecating it, since it's unstable.

@m-ou-se
Copy link
Member

m-ou-se commented Oct 18, 2022

I settled on using transpose when the types could be inferred since that minimizes human thinking. In the other cases I used const arrays.

The sentiment in the libs-api meeting was that the array version is actually easier to understand than the uninit().transpose() version, despite being a bit more verbose.

@clarfonthey
Copy link
Contributor

I still would like to express my discomfort with transposes as… exposing something more generic for the sake of it, instead of promoting a good API.

Right now we have the major hurdle that there have to be different methods for working with arrays, slices, and values. This doesn't actually solve this problem; in order to work with an array, you're still expected to transpose after the initial uninit, and (presumably) before the final assume_init.

IMHO, we can remove this entirely by simply letting people slice directly into a MaybeUninit to retrieve its components. If we accepted MaybeUninit<[T]> as a valid type and provided Index and IndexMut methods to it, callers can perform writes via all the expected means without having to transpose at all. This is perfectly sound as long as we return MaybeUninit<[T]> and MaybeUninit<T> references, since we never let an init reference "escape" with safe code.

This is the main reason why I promoted deprecating the old method as a compromise: if we're going to offer transposes, it seems worthwhile to see if users will actually prefer it. By removing the alternative, everyone's forced to move back to it instead, which just results in more frustration, IMHO.

The one downside of this, which I will admit is rather large, is the fact that the metadata of the unsized value "escapes" from the MaybeUninit and is actually init at all times. This seems reasonable to me for slices, but it won't make sense for other unsized types. As long as we only allow slices this way, I think it would be fine, but I admit this is the biggest flaw with this approach.

@SUPERCILEX
Copy link
Contributor Author

Right now we have the major hurdle that there have to be different methods for working with arrays, slices, and values.

Absolutely agree that this sucks, that's what I'm trying to improve.

If we accepted MaybeUninit<[T]> as a valid type

Not sure how that's possible since [T] is unsized, but otherwise yes this would be great. BTW, not sure if you've seen this discussion where I mentioned slices: rust-lang/libs-team#122 (comment)

Also, I still think you'd need transpose APIs because you'll be passed in an &mut [MaybeUninit<u8>] which means you need to transpose to be able to assume_init the whole array, even if MaybeUninit<[T]> is a thing.

@m-ou-se m-ou-se removed the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Oct 25, 2022
@apiraino
Copy link
Contributor

hello, visiting and checking progress. By skimming the comment history I'm unclear on the status. Perhaps some design work from the author (cc @SUPERCILEX) ? Can anyone provide an update? :) thanks!

@SUPERCILEX
Copy link
Contributor Author

I still think this will go through, but we're now potentially waiting on inline const stabilization: rust-lang/libs-team#122 (comment)

@m-ou-se m-ou-se added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Dec 14, 2022
thomcc pushed a commit to tcdi/postgrestd that referenced this pull request Feb 10, 2023
Add MaybeUninit array transpose From impls

See discussion in rust-lang/rust#101179 and rust-lang/rust#96097. I believe this solution offers the simplest implementation with minimal future API regret.

`@RalfJung` mind doing a correctness review?
@SUPERCILEX
Copy link
Contributor Author

Closing as this made progress elsewhere: #96097 (comment)

@SUPERCILEX SUPERCILEX closed this Jul 27, 2024
@SUPERCILEX SUPERCILEX deleted the uninit_array branch July 27, 2024 05:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. T-compiler Relevant to the compiler 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. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Both the name of and the docs for MaybeUninit::uninit_array() make what it actually does somewhat unclear