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

Tracking issue for Vec::leak #62195

Closed
cramertj opened this issue Jun 27, 2019 · 20 comments · Fixed by #74605
Closed

Tracking issue for Vec::leak #62195

cramertj opened this issue Jun 27, 2019 · 20 comments · Fixed by #74605
Labels
A-collections Area: std::collections. B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. Libs-Tracked Libs issues that are tracked on the team's project board. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@cramertj
Copy link
Member

Similar to Box::leak, this function would allow leaking a Vec and getting back an &'a mut [T].

@jonas-schievink jonas-schievink added A-collections Area: std::collections. B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Jun 27, 2019
@jonas-schievink
Copy link
Contributor

Implementation in #62196

@SimonSapin
Copy link
Contributor

Box::leak is an associated function instead of a method to avoid shadowing a possible Foo::leak method in Box<Foo>, normally accessible through auto-deref.

This is less relevant to Vec::leak since Vec<T> dereferences to [T]. There is no [T]::leak method in the standard library, so making Vec::leak a method would only shadow such a method if it’s defined in a user trait.

@cramertj Do you think it should still be an associated function?

@cramertj
Copy link
Member Author

I don't have any particularly strong opinion. .leak() seems pretty clear to me, and users can always call it as Vec::leak if they prefer.

@Lucretiel
Copy link
Contributor

Lucretiel commented Jan 12, 2020

Is there any reason to not have this method return an &'static mut [T]? It simplifies the code (no generic) and Rust can coerce the lifetime freely.

@rodrigorc
Copy link

I find weird that there is a Vec::leak, but not a String::leak, PathBuf::leak, OsString::leak or CString::leak.

IMHO, if Vec::leak exists, then any standard type that owns data and has a into_boxed_whatever member function should be able to leak too.

@cramertj
Copy link
Member Author

cramertj commented Mar 9, 2020

IMHO, if Vec::leak exists, then any standard type that owns data and has a into_boxed_whatever member function should be able to leak too.

Seems fine to me. Sadly, I don't think there's a trait that covers these, so they'll all have to be added independently.

@rodrigorc
Copy link

Sadly, I don't think there's a trait that covers these, so they'll all have to be added independently.

I don't think the lack of trait is a problem at all. For example I would expect any standard type that has a len() function to have an is_empty() too, no trait involved. It's like an expected pattern.

@lambda-fairy
Copy link
Contributor

@Lucretiel the lifetime parameter is necessary in case T has non-'static lifetimes, e.g. if it's a reference (playground).

Box::leak has a lifetime parameter for the same reason.

@shepmaster
Copy link
Member

What do people think about changing the argument type to be more generic:

pub fn leak<'a>(vec: impl Into<Vec<T>>) -> &'a mut [T]
where
    T: 'a, 

I'm thinking about adding a similar String::leak and it'd be nice to do String::leak("hello").

@Lucretiel
Copy link
Contributor

If we were to change it in this way, wouldn't it be preferable to use the Borrow machinery so that we can leak Vec<T> -> &[T], String -> &str, etc?

@SimonSapin
Copy link
Contributor

I’d prefer to go in the opposite direction and change Vec::leak to be a proper method that takes a self parameter instead of an associated function with a vec: Self parameter. Any conversion can be a separate (chained) call.

Box::<T>::leak (like Box::into_raw etc) is an associated function rather than a method in order to avoid shadowing a potential T::leak method for some T types. But that does not apply to Vec which does not have Deref to a fully generic target type.

@shepmaster
Copy link
Member

a proper method that takes a self parameter instead of an associated function

You and I have had this discussion for other types so I won’t restate my previous positions against this.

Any conversion can be a separate (chained) call

Vec::leak itself doesn’t need to exist and can be a separate (chained) call — vec -> boxed slice -> box::leak

@shepmaster
Copy link
Member

use the Borrow machinery

I’m not following how this would work; would you mind expanding a bit?

@Lucretiel
Copy link
Contributor

So, your proposal is that we have leak be generic over Into<Vec<T>>, with the motivating example being that you can leak a String. However, my argument is that it's surprising for a String to leak into a &[u8], and that instead the Borrow trait (which universally genericizes & associates between owned and borrowed versions of things (eg, Box<T> -> &T, Vec<T> -> &[T], String -> &str) could be used instead.

Though, now that I'm thinking about it, I think it also does T->&T, so it may actually be too abstract for this purpose.

@SimonSapin
Copy link
Contributor

@shepmaster I’m sorry, I don’t remember the specifics so if you have a link to those discussions I wouldn’t mind.

By "chained" I was referring specifically to method chaining syntax .foo().bar(), where parens do not need to be nested. In this case, .into::<Vec<_>>().leak().

@shepmaster
Copy link
Member

shepmaster commented Jul 13, 2020

with the motivating example being that you can leak a String

Ah, sorry, that's not what I meant. I mean to add another method: String::leak in the same vein as Vec::leak and Box::leak.

impl String {
    pub fn leak(s: impl Into<String>) -> &'a mut str {}
}

@Lucretiel
Copy link
Contributor

Ah, yes, I definitely can support that.

@Lucretiel
Copy link
Contributor

Lucretiel commented Jul 13, 2020

But wait, so- that means (for example) that something like Vec::leak("Hello".to_string()) would be valid? I'm not sure I love that; it seems like it'd be more confusing than other cases of Into<Self> in method definitions. My expectation is to "leak a Vec", and I'd argue it's reasonable in this case for the user to manage getting a Vec they want to leak in the first place.

You and I have had this [self vs associated method] discussion for other types so I won’t restate my previous positions against this.

Additionally, can I ask you to elaborate on this a bit? It makes perfect sense to me for smart-pointer types like Box, Arc, etc, but I'm not sure I see the argument for Vec::leak or String::leak.

@shepmaster
Copy link
Member

Additionally, can I ask you to elaborate on this a bit?

I'd rather not as I've always lost that particular argument. No reason to believe that this time will be any different.

@SimonSapin
Copy link
Contributor

I’ve proposed FCP to stabilize with a signature change at #74605

@KodrAus KodrAus added the Libs-Tracked Libs issues that are tracked on the team's project board. label Jul 29, 2020
@bors bors closed this as completed in 5ef872f Aug 2, 2020
the8472 added a commit to the8472/rust that referenced this issue Oct 13, 2021
Avoid allocations and copying in Vec::leak

The [`Vec::leak`] method (rust-lang#62195) is currently implemented by calling `Vec::into_boxed_slice` and `Box::leak`.  This shrinks the vector before leaking it, which potentially causes a reallocation and copies the vector's contents.

By avoiding the conversion to `Box`, we can instead leak the vector without any expensive operations, just by returning a slice reference and forgetting the `Vec`.  Users who *want* to shrink the vector first can still do so by calling `shrink_to_fit` explicitly.

**Note:**  This could break code that uses `Box::from_raw` to “un-leak” the slice returned by `Vec::leak`.  However, the `Vec::leak` docs explicitly forbid this, so such code is already incorrect.

[`Vec::leak`]: https://doc.rust-lang.org/stable/std/vec/struct.Vec.html#method.leak
bors added a commit to rust-lang-ci/rust that referenced this issue Oct 15, 2021
Avoid allocations and copying in Vec::leak

The [`Vec::leak`] method (rust-lang#62195) is currently implemented by calling `Vec::into_boxed_slice` and `Box::leak`.  This shrinks the vector before leaking it, which potentially causes a reallocation and copies the vector's contents.

By avoiding the conversion to `Box`, we can instead leak the vector without any expensive operations, just by returning a slice reference and forgetting the `Vec`.  Users who *want* to shrink the vector first can still do so by calling `shrink_to_fit` explicitly.

**Note:**  This could break code that uses `Box::from_raw` to “un-leak” the slice returned by `Vec::leak`.  However, the `Vec::leak` docs explicitly forbid this, so such code is already incorrect.

[`Vec::leak`]: https://doc.rust-lang.org/stable/std/vec/struct.Vec.html#method.leak
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-collections Area: std::collections. B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. Libs-Tracked Libs issues that are tracked on the team's project board. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants