Skip to content

Conversation

martinvuyk
Copy link
Contributor

Split off from #4016. Part of #3528

@martinvuyk martinvuyk requested a review from a team as a code owner April 9, 2025 11:07
@martinvuyk martinvuyk force-pushed the make-stringslice-split-ret-stringslice branch 4 times, most recently from 0085e41 to 356eb7f Compare April 11, 2025 10:56
@martinvuyk martinvuyk force-pushed the make-stringslice-split-ret-stringslice branch from 356eb7f to 05b2886 Compare April 11, 2025 10:57
@JoeLoser
Copy link
Collaborator

!sync

@modularbot modularbot added the imported-internally Signals that a given pull request has been imported internally. label Apr 11, 2025
@JoeLoser
Copy link
Collaborator

Sadly, this is still crashing parameter inference internally (CC: @zyx-billy):

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

I'm going to mark this as "blocked" label here for my own book-keeping. I've asked the compiler team to take a look at this internally.

A few things:

  • We eventually want String.split(...) to return List[StringSlice], but this was blocked by similar issues above with parameter inference. See [stdlib] Make String.split() return List[StringSlice] #4016 that this particular PR was split off from.
  • It didn't crash/fail in the open source CI which is strange - I don't know why yet.

@JoeLoser JoeLoser added the blocked Blocked by another issue that must be resolved first label Apr 12, 2025
@JoeLoser
Copy link
Collaborator

Sadly, this is still crashing parameter inference internally (CC: @zyx-billy):

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

I'm going to mark this as "blocked" label here for my own book-keeping. I've asked the compiler team to take a look at this internally.

A few things:

  • We eventually want String.split(...) to return List[StringSlice], but this was blocked by similar issues above with parameter inference. See [stdlib] Make split return List[StringSlice] #4016 that this particular PR was split off from.
  • It didn't crash/fail in the open source CI which is strange - I don't know why yet.

@martinvuyk update here: @zyx-billy fixed the parameter inference issue and I just rebased this synced PR internally. It's about to pass CI and land. This should unblock #3894 and friends as well in the next nightly. It was hitting similar parameter inference bugs IIRC. So feel free to rebase those when this fix lands and we can get those in. Thank you everyone for chasing this down!

@modularbot
Copy link
Collaborator

✅🟣 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 modularbot added merged-internally Indicates that this pull request has been merged internally merged-externally Merged externally in public mojo repo labels Apr 15, 2025
@modularbot
Copy link
Collaborator

Landed in b375020! Thank you for your contribution 🎉

@martinvuyk martinvuyk deleted the make-stringslice-split-ret-stringslice branch April 21, 2025 11:10
modularbot pushed a commit that referenced this pull request May 6, 2025
…ce]` (#59490)

[External] [stdlib] Make `StringSlice.split()` return `List[StringSlice]`

Split off from #4016. Part of
#3528.

Co-authored-by: martinvuyk <[email protected]>
Closes #4324
MODULAR_ORIG_COMMIT_REV_ID: a13fa523e6a378510a980c320d2e8ab21b7e18ce
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked Blocked by another issue that must be resolved first imported-internally Signals that a given pull request has been imported internally. merged-externally Merged externally in public mojo repo merged-internally Indicates that this pull request has been merged internally
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants