[mojo-stdlib] Create InlineArray type

[lsh]

This PR creates an InlineArray type that takes any CollectionElement rather than just AnyRegType. See also this thread on Discord.

[JoeLoser]

In general, looks pretty good! Thanks for pushing on this. This will be a critical core type and will simplify several things. 🎉

[lattner]

This is super cool @lsh great work!

[mikowals]

There is a problem with how __refitem__ provides __getitem__ when InlineArray is used as an alias. Maybe since in this case self is immutable in __refitem__? A failing test to demonstrate this is:

alias arr_const = InlineArray[Int, 3](1, 2, 3)
assert_equal(arr_const[0], 1)

A workaround is adding a __getitem__ with immutable self.

fn __getitem__(self, idx: Int) -> Self.ElementType:
    return self.__refitem__(idx)

[JoeLoser]

There is a problem with how __refitem__ provides __getitem__ when InlineArray is used as an alias. Maybe since in this case self is immutable in __refitem__? A failing test to demonstrate this is:

alias arr_const = InlineArray[Int, 3](1, 2, 3)
assert_equal(arr_const[0], 1)

A workaround is adding a __getitem__ with immutable self.

fn __getitem__(self, idx: Int) -> Self.ElementType:
    return self.__refitem__(idx)

Thanks for calling this out. @lattner would this sort of thing work with the latest top-of-tree mojo based on your changes yesterday with handling the case when both __getitem__ and __refitem__ are present for a type? I worry that we may be able to make it work with the latest public nightly/mojo but it may be rejected on top-of-tree (unreleased). I have not verified that though.

Edit: confirmed this won't compile on top-of-tree internally. You get an error:

stdlib/utils/static_tuple.mojo:389:32: error: cannot implicitly convert 'Reference[ElementType, 0, self, 0]' value to 'ElementType' in return value
        return self.__refitem__(idx)

So, to move forward with this PR, I recommend we file a GitHub issue and (potentially) a commented out failing test case with this GitHub issue number. To make this work at comptime, it can't be solved in the library alone AFAICT — some compiler work is needed. So, we'll handle that internally if that works.

[JoeLoser]

Almost there! Just one minor comment about the use of VariadicList, and then I'd be happy to merge this.

[lsh]

I want to flag this constructor, which I'm using as a way to enforce safety guarantees. What do you think about this API? @JoeLoser @ConnorGray

[JoeLoser]

This makes sense, since otherwise a zero-argument call like so:

var arr = InlinedArray[Int, 10]()

would call the variadic constructor. Doing it as an explicit constructor rather than a @parameter if in the variadic constructor makes sense for now. I don't have a good gauge as to the compile time cost difference between the additional overload (constructor) vs. a @parameter if in the variadic constructor case. Nothing to worry about for now as we're not at the level of compile time perf problems right now 😄

[JoeLoser]

FYI this constructor will need public doc strings since it's a public entity still. We have a PR in-flight internally that adds a decorator (such as @doc_private) that we could apply to things like this in the future. The API doc string checker wouldn't apply in such cases, and the constructor decl would be hidden in our generated docs too.

I moved your comment from above the constructor to be the doc string internally when I just imported this PR. We also have a PR in-flight in the public mojo repo so the external CI here can do API doc string validation.

[lsh]

bikeshed: is fill the right word here?

[arthurevans]

SIMD just uses value for this kind of constructor. Not sure if that's a great precedent or not, although it's probably clear enough. From the name I wouldn't be sure if fill was a value or a boolean. Maybe fill_value or fill_with?

Also, have you tried to create an InlineArray of size 1? It looks like the two constructors would be ambiguous in that case. Making this a keyword-only argument would avoid that potential issue.

InlineArray[Int, 10](fill_with=0xFF)

[lsh]

At least on my machine, the constructor of size 1 seems to work fine. I'll add a test to make that clear.

[JoeLoser]

They're not ambiguous as @lsh points out. Consider the following smaller program that shows this is the case:

struct S:
    fn __init__(inout self, value: Int):
        print("single argument constructor")

    fn __init__(inout self, *values: Int):
        print("variadic argument constructor")

fn main():
    var s1 = S(42) # prints single argument constructor
    var s2 = S(42, 100) # prints variadic argument constructor

Re the name, I'm fine with fill, default_value, value, or any of them. I don't feel strongly and we can easily change it at any time since this isn't a keyword-argument-only constructor (therefore it's just an implementation detail at this point).

[JoeLoser]

This is great, @lsh! Thanks so much for your patience and working on this, even amidst us making you rebase and rework the code a bit as we changed several things across nightly versions.

I've imported this PR internally and I'll keep you updated on the progress of it.

[JoeLoser]

They're not ambiguous as @lsh points out. Consider the following smaller program that shows this is the case:

struct S:
    fn __init__(inout self, value: Int):
        print("single argument constructor")

    fn __init__(inout self, *values: Int):
        print("variadic argument constructor")

fn main():
    var s1 = S(42) # prints single argument constructor
    var s2 = S(42, 100) # prints variadic argument constructor

Re the name, I'm fine with fill, default_value, value, or any of them. I don't feel strongly and we can easily change it at any time since this isn't a keyword-argument-only constructor (therefore it's just an implementation detail at this point).

[JoeLoser]

This makes sense, since otherwise a zero-argument call like so:

var arr = InlinedArray[Int, 10]()

would call the variadic constructor. Doing it as an explicit constructor rather than a @parameter if in the variadic constructor makes sense for now. I don't have a good gauge as to the compile time cost difference between the additional overload (constructor) vs. a @parameter if in the variadic constructor case. Nothing to worry about for now as we're not at the level of compile time perf problems right now 😄

[JoeLoser]

Quibble I think we could do this arithmetic at compile time since size is a compile time parameter. But totally fine for now. 😄

[JoeLoser]

FYI this constructor will need public doc strings since it's a public entity still. We have a PR in-flight internally that adds a decorator (such as @doc_private) that we could apply to things like this in the future. The API doc string checker wouldn't apply in such cases, and the constructor decl would be hidden in our generated docs too.

I moved your comment from above the constructor to be the doc string internally when I just imported this PR. We also have a PR in-flight in the public mojo repo so the external CI here can do API doc string validation.

[JoeLoser]

✅🟣 This contribution has been merged 🟣✅

Hey @lsh,

As mentioned, we're moving to a new infrastructure for merging contributions to Mojo (we're using a tool called Copybara), and your contribution has now been merged into our internal copy of the Mojo Standard Library. I've added the "merged-internally" label on this PR (FYI @ConnorGray).

The changes in this PR will appear here in the mojo repo nightly branch when we do our next outbound synchronization at the time that the next Mojo nightly is released. That should happen on this coming Monday.

Please let me know if you have any questions or concerns.

[JoeLoser]

Closing as this got merged into the latest nightly as 904ac4e