[stdlib] Fix String.split() and start fixing String.__len__()

[martinvuyk]

Fix string split to take in NoneType as input and its body to use isspace(self) method. Fixes #2880

Leave the fix for lstrip and rstrip to use String.isspace() as TODO once llvm intrinsics can be used at comp time

Added a method String.byte_length() but left the builtin String.__len__() alone for now since many other methods assume it returns byte length and it will require mayor refactoring of too many places for 1 PR. Added deprecation warning to _byte_length()

[martinvuyk]

@laszlokindrat no idea what this error is.

When calling fn lstrip(self) -> String:

/Users/runner/work/mojo/mojo/stdlib/src/builtin/string.mojo:1874:8: note:                             failed to interpret function @stdlib::builtin::string::String::lstrip(stdlib::builtin::string::String)
    fn lstrip(self) -> String:
       ^
/Users/runner/work/mojo/mojo/stdlib/src/builtin/string.mojo:1881:18: note:                               failed to evaluate call
        for s in self:
                 ^
/Users/runner/work/mojo/mojo/stdlib/src/builtin/string.mojo:1194:8: note:                                 failed to interpret function @stdlib::builtin::string::String::__iter__[__mlir_type.i1,__mlir_type.!lit.lifetime<*(0,0)>](stdlib::builtin::string::String%)_REMOVED_ARG
    fn __iter__(ref [_]self) -> _StringIter[__lifetime_of(self)]:
       ^
/Users/runner/work/mojo/mojo/stdlib/src/builtin/string.mojo:1200:48: note:                                   failed to evaluate call
        return _StringIter[__lifetime_of(self)](
                                               ^
/Users/runner/work/mojo/mojo/stdlib/src/builtin/string.mojo:700:8: note:                                     failed to interpret function @stdlib::builtin::string::_StringIter::__init__(stdlib::builtin::string::_StringIter[$0, $1, $2]=&,stdlib::memory::unsafe_pointer::UnsafePointer[stdlib::builtin::simd::SIMD[{uint8}, {1}], {{0}}],stdlib::builtin::int::Int)_REMOVED_ARG,forward=1
    fn __init__(
       ^
/Users/runner/work/mojo/mojo/stdlib/src/builtin/string.mojo:708:31: note:                                       failed to evaluate call
            if _utf8_byte_type(unsafe_pointer[i]) == 1:
                              ^
/Users/runner/work/mojo/mojo/stdlib/src/builtin/string.mojo:[66](https://github.com/modularml/mojo/actions/runs/9390428176/job/25860270012?pr=2960#step:9:67)7:4: note:                                         failed to interpret function @stdlib::builtin::string::_utf8_byte_type(stdlib::builtin::simd::SIMD[{uint8}, {1}])
fn _utf8_byte_type(b: UInt8) -> UInt8:
   ^
/Users/runner/work/mojo/mojo/stdlib/src/sys/intrinsics.mojo:192:10: note:                                           failed to fold operation pop.call_llvm_intrinsic{fastmathFlags: #pop<fmf none>, hasSideEffects: false, intrin: "llvm.ctlz" : !kgen.string}(#pop<simd 223> : !pop.scalar<ui8>, false)
        ](arg0, arg1)
         ^
mojo: error: failed to run the pass manager```

[laszlokindrat]

@martinvuyk Something (not sure where) is calling lstrip at compile time, but it can only work at runtime because its implementation somewhere uses an llvm intrinsic, and we don't support them at compile time yet. This is a known limitation that we are planning to lift.

[martinvuyk]

@laszlokindrat ready for review :D

[laszlokindrat]

Thanks for the patch! I have a couple questions, but I like the direction you're going! Could you please rebase on the latest nightly?

[martinvuyk]

Hey @laszlokindrat I ended up touching a lot more places than I thought I'd have to. Changed String._byte_length() to String.byte_length() since it is a method that is pretty commonly used before using pointers.

I used a @deprecated decorator to help me find all places in the stdlib that use String.__len__ assuming it's byte_length before doing pointer stuff and changed it to String.byte_length().

I also added some code in places where we should use unicode code point indexing and currently assume ASCII, but we still need issue #933 solved to be able to switch to that. And also the code is the first thing that came to me, not necessarily efficient.

[laszlokindrat]

Thanks! Can we split up this patch then? The length related stuff will touch a lot more things internally, and it's also semantically separate.

[martinvuyk]

I honestly don't know where to even split this since fn split() needs byte_length and so does _StringSliceIter instantiation. I didn't change any behavior regarding length methods except for StringSlice.__len__ that was already marked as returning unicode length

    fn __len__(self) -> Int:
        """Nominally returns the _length in Unicode codepoints_ (not bytes!).

        Returns:
            The length in Unicode codepoints.
        """
        # FIXME(MSTDL-160):
        #   Actually perform UTF-8 decoding here to count the codepoints.
        return len(self._slice)

    fn __len__(self) -> Int:
        """Nominally returns the _length in Unicode codepoints_ (not bytes!).

        Returns:
            The length in Unicode codepoints.
        """
        var unicode_length = self.byte_length()

        for i in range(unicode_length):
            if _utf8_byte_type(self._slice[i]) == 1:
                unicode_length -= 1

        return unicode_length

The length related stuff will touch a lot more things internally

That's what I was afraid of. For now the only breaking change is StringLiteral, StringSlice and String got their _byte_length() method renamed to byte_length() to be used before doing pointer stuff (using the internal method directly was a bad idea anyway). I could also just add a _byte_length() method with a deprecation warning to all WDYT?

[laszlokindrat]

Let's start with the _byte_length -> byte_length renaming, as well as all the miscellaneous updates you did to move from len to byte_length. Then we can iterate again. Thanks for you patience with this, I promise it will eventually land :)

[laszlokindrat]

Thanks! Can you please add a changelog entry describing the changes?

[laszlokindrat]

Thank you!

[laszlokindrat]

!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 nightly 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 cd65e1b! Thank you for your contribution 🎉