[Mojo-stdlib] Remove `FLAG_IS_STATIC_CONSTANT` from the `String` struct #4661

gabrieldemarmiesse · 2025-05-22T04:33:45Z

While poking around in the String struct, I noticed that it was actually possible to know if we point to a StaticString without the need to read the flag FLAG_IS_STATIC_CONSTANT. We can use the condition "pointer is not null and capacity is 0".
I also simplify the String() constructor in the process. Let's remove the flag and make it available for future use cases.

See https://github.com/modular/modular/blob/main/mojo/proposals/string-design.md for more info on the subject:

In addition to setting the bit to know whether the pointer is to a constant string, it is important to know that the String type also sets its notion of "capacity" to zero. This ensures that any attempt to append or access will immediately reallocate without extra checks. This is a minor optimization and simplification of code, but it means that static constant strings will have capacity=0 but have length=n where n > 0

@lattner expressed his desire to do this in this discord message

Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>

JoeLoser

Nice clean up, thanks @gabrieldemarmiesse! 👍

JoeLoser · 2025-05-22T17:21:21Z

!sync

lattner

This is fantastic, thank you @gabrieldemarmiesse . Please update the String Design doc if needed as well.

Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>

gabrieldemarmiesse · 2025-05-22T20:31:33Z

@lattner I changed the string proposal document to reflect this change.

@JoeLoser I also addressed your comment in this PR. It's unreleated, so let me know if you want a separate PR instead for a cleaner git history.

Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>

JoeLoser · 2025-05-22T20:37:06Z

@lattner I changed the string proposal document to reflect this change.

@JoeLoser I also addressed your comment in this PR. It's unreleated, so let me know if you want a separate PR instead for a cleaner git history.

Let's tease that out to a separate PR that I'll sync and land immediately, please. I suspect this PR may run into a few test failures and I don't want to hold that changelog entry up with this.

Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>

gabrieldemarmiesse · 2025-05-22T20:44:57Z

@JoeLoser done :)

JoeLoser · 2025-05-22T22:45:25Z

@gabrieldemarmiesse looks like a few tests are failing as you can see in the Bazel builds (e.g. https://github.com/modular/modular/actions/runs/15196447847/job/42741618384?pr=4661). You can repro with ./bazelw test //mojo/stdlib/test:algorithm/test_elementwise.mojo.test for example. I haven't dug into them yet, but may be good to poke at them to understand how it's related to your changes. These tests are also run internally, so I can't land your PR internally until we fix them.

Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>

gabrieldemarmiesse · 2025-05-23T13:31:45Z

@JoeLoser I fixed the unit tests.

After a long investigation, I've put in evidence that there is currently (in main) either a UB in the stdlib, or a bug in the compiler causing a miscompilation. You can see the proof here and can play with the toy example:

[Do not merge] Miscompilation or UB in String #4679

After finding the exact pattern which caused the error to trigger, it was quite easy to find a solution. Assigning local variables worked, but i've opt in to just do b and a instead of a and b. See this commit: 4217e64 which fixes the issue.

I'm not particularily eager to try to find the root of the UB or the miscompilation, so if someone else wants to do it, feel free to

JoeLoser · 2025-05-23T17:26:58Z

@JoeLoser I fixed the unit tests.

After a long investigation, I've put in evidence that there is currently (in main) either a UB in the stdlib, or a bug in the compiler causing a miscompilation. You can see the proof here and can play with the toy example:

[Do not merge] Miscompilation or UB in String #4679

After finding the exact pattern which caused the error to trigger, it was quite easy to find a solution. Assigning local variables worked, but i've opt in to just do b and a instead of a and b. See this commit: 4217e64 which fixes the issue.

I'm not particularily eager to try to find the root of the UB or the miscompilation, so if someone else wants to do it, feel free to

Ooof, this looks like a fun one to track down. Awesome work chasing this down and fixing the unit tests @gabrieldemarmiesse! I'll mention this internally and someone will take a look; the community is also free to take a look (@soraros may be interested). I won't be able to take a look for a little while since I'm out on PTO next week.

I'll sync this in so we can land it 👍 🎉

JoeLoser · 2025-05-23T17:27:21Z

!sync

mojo/stdlib/stdlib/collections/string/string.mojo

soraros · 2025-05-23T20:28:57Z

Ooof, this looks like a fun one to track down. Awesome work chasing this down and fixing the unit tests @gabrieldemarmiesse! I'll mention this internally and someone will take a look; the community is also free to take a look (@soraros may be interested). I won't be able to take a look for a little while since I'm out on PTO next week.

I will try to track down this UB. It's gonna be so fun. 🙃

mojo/proposals/string-design.md

mojo/stdlib/stdlib/collections/string/string.mojo

Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>

github-actions · 2025-06-02T15:04:26Z

Thank you for your submission, we really appreciate it. Like many open-source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution. You can sign the CLA by just posting a Pull Request Comment same as the below format.

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

1 out of 2 committers have signed the CLA.
✅ (gabrieldemarmiesse)[https://github.com/gabrieldemarmiesse]
❌ @lattner
_{You can retrigger this bot by commenting recheck in this Pull Request.}_{Posted by the CLA Assistant Lite bot.}

gabrieldemarmiesse · 2025-06-02T15:04:55Z

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

Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>

Signed-off-by: Chris Lattner <lattner@users.noreply.github.com>

lattner · 2025-06-03T20:12:12Z

@JoeLoser is there a path to getting this landed, Gabriel has been very patient here and it is a nice improvement 🙏

JoeLoser · 2025-06-03T20:16:41Z

!sync

JoeLoser · 2025-06-03T20:17:10Z

@JoeLoser is there a path to getting this landed, Gabriel has been very patient here and it is a nice improvement 🙏

Agreed! Sorry for the delay - I was on PTO and just came back today. Now that CI is passing, I've resynced this and it should land soon.

modularbot · 2025-06-03T22:51:10Z

✅🟣 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 · 2025-06-04T19:04:49Z

Landed in a423ce1! Thank you for your contribution 🎉

gabrieldemarmiesse added 4 commits May 21, 2025 14:57

Trying to understand the need for FLAG_IS_STATIC_CONSTANT

3a769b0

Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>

Formatting and set unused bit

de359db

Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>

Merge branch 'main' into cratyyy

4a51f5a

Added small comment

d2db482

Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>

gabrieldemarmiesse requested a review from a team as a code owner May 22, 2025 04:33

gabrieldemarmiesse changed the title ~~Remove FLAG_IS_STATIC_CONSTANT from the String struct, freeing one bit~~ [Mojo-stdlib] Remove FLAG_IS_STATIC_CONSTANT from the String struct May 22, 2025

gabrieldemarmiesse mentioned this pull request May 22, 2025

[DO NOT MERGE] Trying to understand the need for FLAG_IS_STATIC_CONSTANT #4655

Closed

JoeLoser approved these changes May 22, 2025

View reviewed changes

modular-automation bot assigned JoeLoser May 22, 2025

modularbot added the imported-internally Signals that a given pull request has been imported internally. label May 22, 2025

lattner approved these changes May 22, 2025

View reviewed changes

gabrieldemarmiesse added 2 commits May 22, 2025 21:58

Merge branch 'main' into remove_FLAG_IS_STATIC_CONSTANT

3705093

Added more info in the string proposal and the changelog

588509a

Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>

gabrieldemarmiesse requested a review from a team as a code owner May 22, 2025 20:28

Fix markdown formatting

e9916d2

Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>

Removed changelog entry

ebca032

Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>

Merge branch 'main' into remove_FLAG_IS_STATIC_CONSTANT

5850132

gabrieldemarmiesse mentioned this pull request May 23, 2025

[stdlib] Simpler String() implementation #4678

Closed

Fix tests by reversing the "and" operator

4217e64

Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>

soraros reviewed May 23, 2025

View reviewed changes

mojo/stdlib/stdlib/collections/string/string.mojo Outdated Show resolved Hide resolved

Merge branch 'main' into remove_FLAG_IS_STATIC_CONSTANT

5f26e90

lattner approved these changes May 24, 2025

View reviewed changes

mojo/proposals/string-design.md Outdated Show resolved Hide resolved

mojo/stdlib/stdlib/collections/string/string.mojo Outdated Show resolved Hide resolved

Adressed comments

0af95c4

Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>

gabrieldemarmiesse force-pushed the remove_FLAG_IS_STATIC_CONSTANT branch from 2e7d26d to 0af95c4 Compare May 24, 2025 23:09

gabrieldemarmiesse added 4 commits May 25, 2025 01:10

Merge branch 'main' into remove_FLAG_IS_STATIC_CONSTANT

777903e

Merge branch 'main' into remove_FLAG_IS_STATIC_CONSTANT

5730622

Merge branch 'main' into remove_FLAG_IS_STATIC_CONSTANT

a397f86

Merge branch 'main' into remove_FLAG_IS_STATIC_CONSTANT

8663ef6

gabrieldemarmiesse added 2 commits June 2, 2025 15:16

[Tooling] Fix the pre-commit

1de8a31

Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>

Merge branch 'fix_pre-commit' into remove_FLAG_IS_STATIC_CONSTANT

59f9a46

Signed-off-by: gabrieldemarmiesse <gabrieldemarmiesse@gmail.com>

gabrieldemarmiesse requested a review from JoeLoser June 2, 2025 15:21

lattner and others added 2 commits June 2, 2025 12:29

Merge branch 'main' into remove_FLAG_IS_STATIC_CONSTANT

feb9b5d

Signed-off-by: Chris Lattner <lattner@users.noreply.github.com>

Merge branch 'main' into remove_FLAG_IS_STATIC_CONSTANT

dce6890

modularbot added merged-internally Indicates that this pull request has been merged internally merged-externally Merged externally in public mojo repo labels Jun 3, 2025

modularbot closed this in a423ce1 Jun 4, 2025

github-actions bot locked and limited conversation to collaborators Jun 4, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Mojo-stdlib] Remove `FLAG_IS_STATIC_CONSTANT` from the `String` struct #4661

[Mojo-stdlib] Remove `FLAG_IS_STATIC_CONSTANT` from the `String` struct #4661

Uh oh!

Uh oh!

JoeLoser left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

[Mojo-stdlib] Remove FLAG_IS_STATIC_CONSTANT from the String struct #4661

[Mojo-stdlib] Remove FLAG_IS_STATIC_CONSTANT from the String struct #4661

Uh oh!

Conversation

Uh oh!

Uh oh!

JoeLoser left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

[Mojo-stdlib] Remove `FLAG_IS_STATIC_CONSTANT` from the `String` struct #4661

[Mojo-stdlib] Remove `FLAG_IS_STATIC_CONSTANT` from the `String` struct #4661