Skip to content

[stdlib] Require explicit byte= keyword for String/StringSlice span indexing#6152

Closed
mahendrarathore1742 wants to merge 2 commits intomodular:mainfrom
mahendrarathore1742:fix/string-span-implicit-byte-indexing
Closed

[stdlib] Require explicit byte= keyword for String/StringSlice span indexing#6152
mahendrarathore1742 wants to merge 2 commits intomodular:mainfrom
mahendrarathore1742:fix/string-span-implicit-byte-indexing

Conversation

@mahendrarathore1742
Copy link
Copy Markdown
Contributor

…an indexing

Fixes #6024 — String/StringSlice slice syntax like s[0:1] silently did byte-level indexing. Now requires s[byte=0:1] to make byte-based slicing explicit. Updated all stdlib callers and tests.

@mahendrarathore1742 mahendrarathore1742 requested a review from a team as a code owner March 12, 2026 11:24
Copilot AI review requested due to automatic review settings March 12, 2026 11:24
@github-actions github-actions bot added mojo-stdlib Tag for issues related to standard library waiting-on-review labels Mar 12, 2026
@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 12, 2026

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@mahendrarathore1742
Copy link
Copy Markdown
Contributor Author

I have read the CLA Document and I hereby sign the CLA

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR addresses #6024 by making byte-based slicing of String / StringSlice explicit, preventing accidental UTF-8-invalid slicing via implicit byte indexing.

Changes:

  • Make StringSlice.__getitem__ span slicing keyword-only ([byte=...]) to disallow implicit byte slicing.
  • Update String.__getitem__ span slicing to forward to StringSlice using the explicit byte= keyword.
  • Update stdlib call sites and tests to use [byte=...] where byte-based slicing is intended.

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
mojo/stdlib/test/collections/string/test_string_unicode_panic.mojo Updates a unicode boundary panic test to use explicit byte slicing.
mojo/stdlib/test/collections/string/test_string_slice.mojo Updates StringSlice byte-slicing tests to use [byte=...].
mojo/stdlib/test/collections/string/test_string.mojo Updates String slicing/indexing tests to use explicit byte slicing.
mojo/stdlib/test/builtin/test_string_literal.mojo Updates comparisons that slice StringSlice inputs to use [byte=...].
mojo/stdlib/test/builtin/test_print.mojo Updates output trimming/prefix checks to use explicit byte slicing.
mojo/stdlib/test/builtin/test_file.mojo Updates exception-message prefix extraction to use explicit byte slicing.
mojo/stdlib/std/python/python_object.mojo Updates inplace-method name construction to slice via [byte=2:].
mojo/stdlib/std/pathlib/path.mojo Updates path substring extraction to use explicit byte slicing.
mojo/stdlib/std/os/path/path.mojo Updates root/suffix splitting logic to use explicit byte slicing.
mojo/stdlib/std/collections/string/string_slice.mojo Makes span slicing keyword-only and updates docs/examples accordingly.
mojo/stdlib/std/collections/string/string.mojo Makes span slicing keyword-only and forwards to StringSlice via byte=.
mojo/stdlib/std/collections/string/_parsing_numbers/parsing_floats.mojo Updates sign stripping to use explicit byte slicing.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

modular-cla-bot bot added a commit to modular/cla that referenced this pull request Mar 12, 2026
@mahendrarathore1742 mahendrarathore1742 changed the title fix(stdlib): require explicit byte= keyword for String/StringSlice sp… fix(stdlib): Require explicit byte= keyword for String/StringSlice span indexing Mar 12, 2026
@mahendrarathore1742 mahendrarathore1742 changed the title fix(stdlib): Require explicit byte= keyword for String/StringSlice span indexing [stdlib] Require explicit byte= keyword for String/StringSlice span indexing Mar 12, 2026
…ndexing

Fixes modular#6024 — String/StringSlice slice syntax like s[0:1] silently did
byte-level indexing. Now requires s[byte=0:1] to make byte-based slicing
explicit. Updated all stdlib callers and tests.
@mahendrarathore1742 mahendrarathore1742 force-pushed the fix/string-span-implicit-byte-indexing branch from f82cb02 to e535b76 Compare March 12, 2026 12:15
@JoeLoser JoeLoser requested a review from ConnorGray March 12, 2026 16:33
Copy link
Copy Markdown
Contributor

@barcharcraz barcharcraz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good, this is the second PR to do this (actually the third, I had one internally). It's about time we got one merged. I'll have to migrate the rest of max internally so expect some additional changes.

@barcharcraz
Copy link
Copy Markdown
Contributor

!sync

@modularbot modularbot added the imported-internally Signals that a given pull request has been imported internally. label Mar 12, 2026
@modularbot
Copy link
Copy Markdown
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 the merged-internally Indicates that this pull request has been merged internally label Mar 12, 2026
@modularbot
Copy link
Copy Markdown
Collaborator

Landed in b90e36f! Thank you for your contribution 🎉

@modularbot modularbot added the merged-externally Merged externally in public mojo repo label Mar 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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 mojo-stdlib Tag for issues related to standard library

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] Span indexing on a String or StringLiteral does implicit byte-based indexing

5 participants