Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move String into std-lib #4710

Closed
wants to merge 5 commits into from
Closed

Move String into std-lib #4710

wants to merge 5 commits into from

Conversation

bitzoic
Copy link
Member

@bitzoic bitzoic commented Jun 28, 2023

Description

The String type is more of a primitive type than a library that provides special functionality. It should live in the std-lib.

Checklist

  • I have linked to any relevant issues.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have updated the documentation where relevant (API docs, the reference, and the Sway book).
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added (or requested a maintainer to add) the necessary Breaking* or New Feature labels where relevant.
  • I have done my best to ensure that my PR adheres to the Fuel Labs Code Review Standards.
  • I have requested a review from the relevant team or maintainers.

@bitzoic bitzoic added the lib: std Standard library label Jun 28, 2023
@bitzoic bitzoic requested a review from a team June 28, 2023 10:29
@bitzoic bitzoic self-assigned this Jun 28, 2023
@bitzoic bitzoic linked an issue Jun 28, 2023 that may be closed by this pull request
Copy link
Contributor

@IGI-111 IGI-111 left a comment

Choose a reason for hiding this comment

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

This isn't a String, it's just Bytes with convenience methods. I don't see the point of standardizing it if we're not standardizing the actual behavior of characters and unicode handling in Sway, which this doesn't do.

All it does is add overhead and breakage to our later attempts at doing that. So unless there is a good reason I don't see to do it, I don't think we should do it.

@bitzoic
Copy link
Member Author

bitzoic commented Jun 28, 2023

This isn't a String, it's just Bytes with convenience methods. I don't see the point of standardizing it if we're not standardizing the actual behavior of characters and unicode handling in Sway, which this doesn't do.

All it does is add overhead and breakage to our later attempts at doing that. So unless there is a good reason I don't see to do it, I don't think we should do it.

This is pulled from the Sway-Libs String which addresses this specific problem. Until we have char there is no way to 1. convert a str to a String or 2. guarantee a valid UTF-8 string.

The intent to move it into the std-lib was based on the fact that String is a type used in many of the Sway-Standards

Copy link
Contributor

@Braqzen Braqzen left a comment

Choose a reason for hiding this comment

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

The standard library should consist of components that can be used directly or libraries can stitch together.

The string is a convenience type, a wrapper, which is a library rather than some atomic component.
I do not believe that this type should be in the standard library for that reason.

It's nice for convenience of importing but it's not much to point at the libs repo.
Just to throw an idea in here, which I'm not really a fan of, we could have core, std and XYZ which would contain these convenience types / mix between a component and a library.
That creates a grey area for the amount of functionality between a lib and component and it probably does not entirely eliminate the libs repo so it fragments the content even more.

@IGI-111
Copy link
Contributor

IGI-111 commented Jun 28, 2023

Convenience types should indeed belong in user packages, I think Rust's approach of doing it with things like regex is sound. As they say, standard libraries are where things go to die.

But this isn't really the reason not to add this. Rather, this convenience string type will not be what we settle on when proper strings actually get standardized. Because it does not actually solve the Unicode problem, it just sidesteps it by pretending a collection of bytes is a Unicode string without checking, and doesn't actually let you manipulate code points in any meaningful way. As you say, this isn't actually possible to implement without char.

I don't think it's proper to standardize an interface we know we will break in the near future.
I may be compelled to do it if there was a strong reason, but people (including us) using a convenience interface isn't a strong reason.

@bitzoic
Copy link
Member Author

bitzoic commented Jun 28, 2023

The standard library should consist of components that can be used directly or libraries can stitch together.

The string is a convenience type, a wrapper, which is a library rather than some atomic component. I do not believe that this type should be in the standard library for that reason.

By this logic, the Rust String type shouldn't be in the standard library either as it is just a wrapper of Vec<u8>s. This was also once a Vec<u8>, but the Bytes type is more efficient for the fact that the FuelVM pads to the size of a word.

The main problem is that any str is treated as it's own type and the length must be defined. A str[8] cannot be compared with a str[7] as within the VM they are different types.
Hence, the use of the String type in the standard, I cannot define a standard using the str type. Unfortunately, a side effect of this is any use of a String in a standard is reading from storage and not something like a constant.
It is also impossible to create any conversions to the String type, unless I unrealistically define a separate From implementation for every possible length of str. Any uses of something like EIP-191 cannot be implemented either.

Fundamentally, yes, in it's current state the String type is just a convenience type. I personally don't see a type sitting in the same repo as something like a merkle proof library, but we can leave it there for now with the expectation that every use must add an additional dependency to the project.

@bitzoic bitzoic closed this Jun 28, 2023
@IGI-111 IGI-111 mentioned this pull request Jun 28, 2023
7 tasks
IGI-111 added a commit that referenced this pull request Jun 30, 2023
## Description

Create a minimal, forward compatible unicode string type that only
accepts ASCII input and doesn't provide mutation of codepoints.

This should provide Sway-Standards with a proper black box type to use
while still giving us a good platform to later implement a proper,
`char` based unicode string without compatibility issues.

Follow-up from #4710

## Checklist

- [x] I have linked to any relevant issues.
- [x] I have commented my code, particularly in hard-to-understand
areas.
- [ ] I have updated the documentation where relevant (API docs, the
reference, and the Sway book).
- [x] I have added tests that prove my fix is effective or that my
feature works.
- [x] I have added (or requested a maintainer to add) the necessary
`Breaking*` or `New Feature` labels where relevant.
- [x] I have done my best to ensure that my PR adheres to [the Fuel Labs
Code Review
Standards](https://github.com/FuelLabs/rfcs/blob/master/text/code-standards/external-contributors.md).
- [ ] I have requested a review from the relevant team or maintainers.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lib: std Standard library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move String type into the std-lib
3 participants