.last() to .next_back() requires a mutable receiver

[samueltardieu]

In the case where iter is a DoubleEndedIterator, replacing a call to iter.last() (which consumes iter) by iter.next_back() (which requires a mutable reference to iter) cannot be done when iter is a non-mutable binding which is not a mutable reference. When possible, a local immutable binding is made into a mutable one.

Also, the applicability is switched to MaybeIncorrect and a note is added to the output when the element types have a significant drop, because the drop order will potentially be modified because .next_back() does not consume the iterator nor the elements before the last one.

Fix #14139

changelog: [double_ended_iterator_last]: do not trigger on non-reference immutable receiver, and warn about possible drop order change

[rustbot]

r? @Centri3

rustbot has assigned @Centri3.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[samueltardieu]

Commits logically separated for easier review.

[samueltardieu]

Rebased

[Centri3]

Just one thing: I suppose we shouldn't just change it to be mutable in the suggestion? I'm not sure if there'd be any case that doesn't work/causes an error considering .last() will already consume it.

[samueltardieu]

Just one thing: I suppose we shouldn't just change it to be mutable in the suggestion? I'm not sure if there'd be any case that doesn't work/causes an error considering .last() will already consume it.

Good question, let me try that, I'll switch to @rustbot author in the meantime.

On an unrelated topic, I also realized that this lint should probably not be MachineApplicable if the elements have a significant drop, as they will be dropped in a different order with .last() and .next_back().

[samueltardieu]

Changes done, mutability is turned on whenever possible (good idea @Centri3). Also, a note is added and applicability is adjusted when the drop order may change.

@rustbot ready

[samueltardieu]

Just one thing: I suppose we shouldn't just change it to be mutable in the suggestion? I'm not sure if there'd be any case that doesn't work/causes an error considering .last() will already consume it.

In the case of a structure or a tuple, .last() might partially unconstruct it and consume a field, while .next_back() would require the structure or tuple to be mutable. In most other cases though, it is possible to make the object mutable by fixing its definition.

[Centri3]

might partially unconstruct it and consume a field

While it does change the meaning of the code, wouldn't any access already be a compiler error? If this doesn't always hold true I'll merge this

[samueltardieu]

might partially unconstruct it and consume a field

While it does change the meaning of the code, wouldn't any access already be a compiler error? If this doesn't always hold true I'll merge this

Yes. I was just trying to say that if you own a struct or a tuple which isn't mutable, .last() might work on one component, while .next_back() would require the whole struct or tuple to be made mutable, which might not be desirable – or we would have to generate more convoluted code to get the field and make it mutable.

All other cases have been handled by making the object mutable when it designates the iterator.

[samueltardieu]

@Centri3 For cases where we don't want to make the structure or tuple mutable, we could still consume its value by creating a temporary that would be dropped at the end of the statement. For example, something like:

fn f(x: (u32, RangeInclusive::<u32>)) -> u32 {
    x.0 + x.1.last().unwrap()
}

could be replaced by

fn f(x: (u32, RangeInclusive::<u32>)) -> u32 {
    x.0 + { x.1 }.next_back().unwrap()
}

instead of not linting it. This is also an operation that would be also useful when fixing #14238.

[samueltardieu]

I've implemented the { receiver }.next_back() for the cases where we cannot make receiver mutable easily.

[Centri3]

Well, ok, now one other thing: is suggesting to use a block really what we wanna do? I don't really recall ever seeing this intentionally done and honestly seems useless at a first glance (especially to beginners who may not understand the intricacies of this). I do not know how to properly state our thoughts! But I believe you understand.

I believe we should lint this, but I want to bring up: Is this really worth it over just making it mut? Or suggesting either one in a help message? What are your thoughts? Is this a common enough pattern that I've just missed?

[samueltardieu]

Well, ok, now one other thing: is suggesting to use a block really what we wanna do? I don't really recall ever seeing this intentionally done and honestly seems useless at a first glance (especially to beginners who may not understand the intricacies of this). I do not know how to properly state our thoughts! But I believe you understand.

Agreed. Although this is a pattern I sometimes use in my code, this may not be common. Some others use identity(aggregate.field) to take mutable ownership of a field while deconstructing an owned struct contained in an immutable var.

I believe we should lint this, but I want to bring up: Is this really worth it over just making it mut? Or suggesting either one in a help message? What are your thoughts? Is this a common enough pattern that I've just missed?

I will test alternatives when I get a chance.

@rustbot author

[samueltardieu]

@Centri3 The lint now gives more explanation of what needs to be made when the receiver cannot be made mutable "easily" (being defined here as "not to intrusive", we only make receivers which are local bindings mutable). WDYT?

@rustbot review

[Centri3]

Suggested change

	diag.note("this might cause unretrieved elements to be dropped after the retrieved one");
	diag.note("this change will alter drop order which may be undesirable");

I find this note to be unhelpful and too wordy. I think it doesn't put enough emphasis on the specific issue being drop order and even confuses me.

Feel free to make your own. Assuming this is the only change made ( 😅 ) I am happy with this version

[samueltardieu]

Feel free to make your own. Assuming this is the only change made ( 😅 ) I am happy with this version

I went with this change, and it has been pushed.

[Centri3]

Agreed. Although this is a pattern I sometimes use in my code, this may not be common. Some others use identity(aggregate.field) to take mutable ownership of a field while deconstructing an owned struct contained in an immutable var.

I also find this to be an ok alternative to suggest.