modularml / mojo

The Mojo Programming Language
https://docs.modular.com/mojo/manual/
Other
22.8k stars 2.57k forks source link

[BUG][mojo-stdlib] List.extend shouldn't take other as owned #2185

Open arthurevans opened 5 months ago

arthurevans commented 5 months ago

Bug description

When looking at https://github.com/modularml/mojo/pull/2182/files I was trying to find existing methods that do something similar (consume another value) to see how we've documented this in the past.

Looking at extend(), which does something (sort of) similar, we're trying to put other in an invalid state by setting its size to 0.

https://github.com/modularml/mojo/blob/main/stdlib/src/collections/list.mojo#L205

The problem is, other is owned, so we're actually modifying a copy, not the original. I think if we want to modify the original, other should actually be passed as inout. (Note that the unit tests for List.extend() don't test the length of other after the call.)

from collections import List

fn main():
    var l1 = List[Int](1, 2, 3, 4)
    var l2 = List[Int](5, 6, 7, 8)
    print(l1.size, l2.size)
    l1.extend(l2)
    print(l1.size, l2.size)

Steps to reproduce

Run the following code:

from collections import List

fn main():
    var l1 = List[Int](1, 2, 3, 4)
    var l2 = List[Int](5, 6, 7, 8)
    print(l1.size, l2.size)
    l1.extend(l2)
    print(l1.size, l2.size)

Note that after calling extend(), l2's length is unchanged.

System information

macOS
dev build mojo 0.0.0 (06f074b4)
StandinKP commented 5 months ago

Should I raise a PR updating the other param and adding test case for the length?

JoeLoser commented 5 months ago

@ConnorGray thoughts here? If I recall correctly, you added this somewhat recently in Q1.

soraros commented 4 months ago

@past-moon l2 is not copied if it's not being used again, even if the transfers operator is omitted.