[stdlib] Add split overloads

[martinvuyk]

Add all split overloads with examples. Part of #3528

cc: @ConnorGray

[ConnorGray]

This looks fantastic and is much easier to keep all the context in my head, making review snappy. Thank you so much for doing that Martin 😄

Other than a question about method placement, this looks ready to go.

[ConnorGray]

!sync

[ConnorGray]

Hmm, it seems this is running into Mojo compiler assertion failures in CI internally.

I'm able to reproduce a crash using $ mojo package ./stdlib/src/ to build this PR. (Curious that the CI here succeeds? That's odd.). The crash I've been seeing looks like this:

1.  Crash resolving decl body at loc("collections/string/string.mojo":1445:8)
    >>     fn split(self, sep: StringSlice, maxsplit: Int) raises -> List[String]:
              ^...................................................................
    >>         """Split the string by a separator.
    >> 
    >>         Args:
    >>             sep: The string to split on.
2.  Crash parsing statement at loc("collections/string/string.mojo":1466:9)
    >>         return _to_string_list(self.as_string_slice().split(sep, maxsplit))
               ^..................................................................<

Are you able to call these new split() functions in any example programs locally, without running into a compiler crash?

I think this is running into the same parameter inference problem I ran into when I recently did some work on StringSlice.split(), which led me to use the explicit parameters you see in the signature here:

    fn split[
        sep_mut: Bool,
        sep_origin: Origin[sep_mut], //,
    ](
        self,
        sep: StringSlice[sep_origin],
        maxsplit: Int = -1,
    ) raises -> List[
        String
    ]:

I notice that you still change StringSlice.split() to return a List[StringSlice] (spelled as List[Self]) in this PR — I'm fairly certain that's the cause of the crash.

Originally I thought this PR was writing out each of these overloads as a way you found to workaround that crash, but it appears that may not have been the goal?

My best guess is that this issue occurs when something goes wrong inferring origin parameters when StringSlice appears in both as the sep: StringSlice argument and in the return List[StringSlice[..]], but I haven't narrowed down a minimal repro myself.

[martinvuyk]

@ConnorGray I can't repro the crash

I notice that you still change StringSlice.split() to return a List[StringSlice] (spelled as List[Self]) in this PR — I'm fairly certain that's the cause of the crash.

AFAIK the Self type has the origin bound defined and is not the same as some_argument: StringSlice

Originally I thought this PR was writing out each of these overloads as a way you found to workaround that crash, but it appears that may not have been the goal?

I didn't experience any crash 🤷‍♂️ . The List[Self] was mainly because I found List[StringSlice[origin]] verbose and I'm lazy

Could you test it with reverting back to parametrizing the origin?

[martinvuyk]

@ConnorGray gentle ping and also to ask whether the crash is still there, and if I can help in any way to repro/fix it

[JoeLoser]

!sync

[JoeLoser]

This is still crashing parameter inference in the compiler internally. Perhaps we can help it out by explicitly specifying origins etc here.

[martinvuyk]

This is quite unfortunate. Should we add a TODO to indicate future cleanups?

Probably, but I'm not even sure what is causing the errors or how to replicate them to open an issue

[martinvuyk]

@JoeLoser @ConnorGray I think I found the issue, could any of you test it?

fn _to_string_list[
    O: ImmutableOrigin, //
](items: List[StringSlice[O]]) -> List[String]:

Changed it to O: Origin

[JoeLoser]

!sync

[JoeLoser]

!sync

[martinvuyk]

@JoeLoser I think I found the issue, when a StringSlice[MutableOrigin] is passed to split() and it returned a List[StringSlice[MutableOrigin]] it allowed writing to an already referenced memory location. So I just made it so that it returns a List[StringSlice[ImmutableOrigin]] regardless of the initial origin's mutability.

[martinvuyk]

@JoeLoser this PR is ready for testing as well

[ConnorGray]

!sync

[ConnorGray]

!sync

[ConnorGray]

!sync

[modularbot]

✅🟣 This contribution has been merged 🟣✅

Your pull request has been merged to the internal upstream Mojo sources. It will be reflected here in the Mojo repository on the main branch during the next Mojo nightly release, typically within the next 24-48 hours.

We use Copybara to merge external contributions, click here to learn more.

[modularbot]

Landed in 4e1c074! Thank you for your contribution 🎉