rust-lang / libs-team

The home of the library team
Apache License 2.0
128 stars 19 forks source link

Add Vec::last_or_push and friends #465

Open balt-dev opened 1 month ago

balt-dev commented 1 month ago

Proposal

Problem statement

Getting a reference to the value you just pushed to a vector requires an extra call and unwrap:

let mut v = Vec::new();
v.push(42);
*v.last_mut().unwrap() = 43;

Motivating examples or use cases

I'm currently porting https://github.com/Naruyoko/OmegaNum.js/ to Rust, and this is a common pattern all over: x.array[i+1]=(x.array[i+1]||0)+1; This is directly translated as

if x.is_empty() {
    x.push(0);
}
let last_mut = x.last_mut().unwrap();
*last_mut += 1;

With this, this could be the following:

*x.last_mut_or_push(0) += 1;

which is much nicer.

Solution sketch

Add functions for Vec::last_or_push(T) -> &T, Vec::last_mut_or_push(T) -> &mut T, Vec::last_or_push_with(impl FnOnce() -> T) -> &T, Vec::last_mut_or_push_with(impl FnOnce() -> T) -> &mut T, which would get the last element of the vector, or push a value and return a reference to that.

Alternatives

Alternatively, Vec::push could return &mut T, which would remove the possiblity of a panic - maybe it would be better, but both would be nice. See issue #464.

balt-dev commented 1 month ago

I'm willing to make a PR with this if everyone's okay with these changes as-is.

balt-dev commented 1 month ago

Note: It might be a good idea to have first_or_push and friends as well.

programmerjake commented 1 month ago

I think having a Vec::push equivalent that returns a reference to the just-pushed value is independently useful, maybe name it Vec::push_and_get?

balt-dev commented 1 month ago

At that point, why not have Vec::push just return &mut T (barring any concerns over the breaking change)? This is what #464 was about, but I closed it in favor of this because I don't want to focus on two ACPs at once. I might reopen it after this one is settled if it's independently useful like you say, but I'd need a better example for it.

balt-dev commented 1 month ago

Any discussion on this? Like I said, if we're fine with it then I can make a pull request and such.

pitaj commented 1 month ago

The libs team has a backlog of these to get through, be patient.

Personally, I think the push(T) -> &mut T idea is better. Rather than changing push, I would suggest adding a new method, push_mut. That avoids the breaking change issue.

balt-dev commented 1 month ago

Ah, they do? Is it publicly available? Also, yes, the push_mut method seems like a nice compromise.

cuviper commented 1 month ago

Ah, they do? Is it publicly available?

It's all the issues like yours with the ACP label: https://github.com/rust-lang/libs-team/labels/api-change-proposal

joshtriplett commented 3 weeks ago

:+1: for shipping push_mut returning a &mut T.

Sadly I don't think the borrow checker will let you get away with v.last_mut().unwrap_or_else(|| v.push_mut(x)). But I think push_mut will still be a substantial improvement and eliminate the need for .unwrap().

joshtriplett commented 2 weeks ago

We discussed this in a libs-api meeting (with somewhat limited attendance), and those present were in favor of adding push_mut.

With push_mut, it should be possible to write last_mut_or_push as

match v.last_mut() {
    None => v.push_mut(x),
    Some(r) => r,
}

last_mut_or_push_with would be straightforward as well (replace x with f()).

So, we're approving push_mut for now, and we'd like to wait to see if the match patterns arise often enough to warrant helpers.

joshtriplett commented 2 weeks ago

Speaking for myself: if push and push_mut both exist, then I think push_mut should be #[must_use], since there's no reason to call it if not using the return value.

cuviper commented 2 weeks ago

With push_mut, it should be possible to write last_mut_or_push as

match v.last_mut() {
    None => v.push_mut(x),
    Some(r) => r,
}

This is OK in local use, but it falls afoul of borrowck limitations if you need to return that lifetime: playground

scottmcm commented 2 weeks ago

That's the well-known case that's already on the list of things we want to change in the future, right? Makes me still want to hold off on adding a specific API for it unless we find out that it's extraordinarily common. (Or we find out that the borrowck changes are even further out than expected.)

pitaj commented 2 weeks ago

[It already works with polonius](https://rust.godbolt.org/#g:!((g:!((g:!((h:codeEditor,i:(filename:'1',fontScale:14,fontUsePx:'0',j:1,lang:rust,selection:(endColumn:1,endLineNumber:18,positionColumn:1,positionLineNumber:18,selectionStartColumn:1,selectionStartLineNumber:18,startColumn:1,startLineNumber:18),source:'pub+fn+last_mut_or_push%3CT%3E(v:+%26mut+Vec%3CT%3E,+x:+T)+-%3E+%26mut+T+%7B%0A++++match+v.last_mut()+%7B%0A++++++++None+%3D%3E+v.push_mut(x),%0A++++++++Some(r)+%3D%3E+r,%0A++++%7D%0A%7D%0A%0A//+pending+inherent+method%0Atrait+Pending%3CT%3E+%7B%0A++++fn+push_mut(%26mut+self,+value:+T)+-%3E+%26mut+T%3B%0A%7D%0Aimpl%3CT%3E+Pending%3CT%3E+for+Vec%3CT%3E+%7B%0A++++fn+push_mut(%26mut+self,+value:+T)+-%3E+%26mut+T+%7B%0A++++++++self.push(value)%3B%0A++++++++unsafe+%7B+self.last_mut().unwrap_unchecked()+%7D%0A++++%7D%0A%7D%0A'),l:'5',n:'0',o:'Rust+source+%231',t:'0')),k:50,l:'4',n:'0',o:'',s:0,t:'0'),(g:!((g:!((h:compiler,i:(compiler:nightly,filters:(b:'0',binary:'1',binaryObject:'1',commentOnly:'0',debugCalls:'1',demangle:'0',directives:'0',execute:'1',intel:'0',libraryCode:'0',trim:'1',verboseDemangling:'0'),flagsViewOpen:'1',fontScale:14,fontUsePx:'0',j:1,lang:rust,libs:!(),options:'-Zpolonius',overrides:!((name:edition,value:'2021')),selection:(endColumn:1,endLineNumber:1,positionColumn:1,positionLineNumber:1,selectionStartColumn:1,selectionStartLineNumber:1,startColumn:1,startLineNumber:1),source:1),l:'5',n:'0',o:'+rustc+nightly+(Editor+%231)',t:'0')),k:100,l:'4',m:50,n:'0',o:'',s:0,t:'0'),(g:!((h:output,i:(compilerName:'rustc+nightly',editorid:1,fontScale:14,fontUsePx:'0',j:1,wrap:'1'),l:'5',n:'0',o:'Output+of+rustc+nightly+(Compiler+%231)',t:'0')),header:(),k:100,l:'4',m:50,n:'0',o:'',s:0,t:'0')),k:50,l:'3',n:'0',o:'',t:'0')),l:'2',n:'0',o:'',t:'0')),version:4)