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

Strings Library #12

Merged
merged 49 commits into from
Oct 10, 2022
Merged

Strings Library #12

merged 49 commits into from
Oct 10, 2022

Conversation

bitzoic
Copy link
Member

@bitzoic bitzoic commented Aug 3, 2022

Type of change

  • New feature

Changes

The following changes have been made:

  • Add Basic String library with the following functionality:
    • as_bytes: Returns the stored string as bytes
    • capacity: Returns the capacity allocated for the string
    • clear: Truncates the string
    • from_utf8: Takes a Vec and returns a String
    • insert: Inserts an element at an index
    • is_empty: Returns true if the string is empty
    • len: Returns the length of the string
    • new: Creates an empty string
    • nth: Returns the element at the specified index
    • pop: Removes and returns the last element of the string
    • push: Appends to the end of the string
    • remove: Removes an element at the specified index
    • with_capacity: Creates a string with a specific capacity

Notes

  • More advanced features may be added later such as appending another string. This should likely be done with the Vec in the standard library and not with the string stuct.
  • A StorageString should be considered.
  • There is currently no relation between str and String. This is due to the compiler's handling of str.
  • The naming convention of functions is taken from Rust's String.

Related Issues

Closes #11

@bitzoic bitzoic added New Feature New addition that does not currently exist Awaiting SDK feature(s) SDK does not support desired functionality, yet labels Aug 3, 2022
@bitzoic bitzoic self-assigned this Aug 3, 2022
@bitzoic bitzoic linked an issue Aug 3, 2022 that may be closed by this pull request
4 tasks
@SwayStar123
Copy link
Member

features like lower and upper might be useful to normalize inputs

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.

Lots of suggestions about conveying information. Take it as you see fit. If any of my suggestions seem like an improvement then I suggest to not just copy and paste them in since I have put some thought into them but more careful wording may be selected.

README.md Outdated Show resolved Hide resolved
sway_libs/src/string/string.sw Show resolved Hide resolved
sway_libs/src/string/string.sw Show resolved Hide resolved
sway_libs/src/string/string.sw Outdated Show resolved Hide resolved
sway_libs/src/string/string.sw Outdated Show resolved Hide resolved
sway_libs/src/string/SPECIFICATION.md Outdated Show resolved Hide resolved
sway_libs/src/string/SPECIFICATION.md Outdated Show resolved Hide resolved
sway_libs/src/string/SPECIFICATION.md Outdated Show resolved Hide resolved
sway_libs/src/string/SPECIFICATION.md Outdated Show resolved Hide resolved
sway_libs/src/string/SPECIFICATION.md Outdated Show resolved Hide resolved
Copy link

@matt-user matt-user left a comment

Choose a reason for hiding this comment

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

Can we use the string library with chars such as 'a' ? If so I would like to see some tests using characters instead of numbers


For more information please see the [specification](./SPECIFICATION.md).

> **Note** There is currently no way to convert a `str` to a `String`.

Choose a reason for hiding this comment

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

If a str cannot be iterated over than I would agree with this note as Braqzen has noted as well. Did you mean perhaps to say an str cannot be casted to a String? IMO you should be able to do let s1: String = String::from("this is an str");

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't believe this will be possible until type constraints are implemented. I think that is part of the declaration engine which afaik @emilyaherbert and @tritao are working on?

sway_libs/src/string/README.md Outdated Show resolved Hide resolved
sway_libs/src/string/README.md Outdated Show resolved Hide resolved
sway_libs/src/string/SPECIFICATION.md Outdated Show resolved Hide resolved
sway_libs/src/string/SPECIFICATION.md Show resolved Hide resolved
@bitzoic
Copy link
Member Author

bitzoic commented Oct 6, 2022

Can we use the string library with chars such as 'a' ? If so I would like to see some tests using characters instead of numbers

Character in Sway do not exist. There is an issue that has been proposed here

Copy link

@Centril Centril left a comment

Choose a reason for hiding this comment

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

Perhaps we only want to start off with the interface, but then we should state that this does no validation and essentially is just Vec<u8> (which Rust's String is also backed by, but the API is unicode-aware).

Comment on lines 25 to 42
/// Converts a vector of bytes to a `String`.
///
/// # Arguments
///
/// * `bytes` - The vector of `u8` bytes which will be converted into a `String`.
pub fn from_utf8(bytes: Vec<u8>) -> String {
String { bytes }
}

/// Inserts a byte at the index within the `String`.
///
/// # Arguments
///
/// * `byte` - The element which will be added to the `String`.
/// * `index` - The position in the `String` where the element will be inserted.
pub fn insert(self, byte: u8, index: u64) {
self.bytes.insert(index, byte);
}
Copy link

Choose a reason for hiding this comment

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

These functions, as well as push, do not seem to be "sound" with respect to the stated purpose of String:

  • String is used for dynamic length strings that are UTF-8 encoded.

To start with, from_utf8 does no validation that the byte array actually is valid UTF-8. It might not be.
Then, let's assume that your string is valid. This can be broken by insertting bad bytes. Similarly, you can push the bad bytes onto the hitherto valid string.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes absolutely this is correct, my documentation is definitely misinforming in regards to this.

Comment on lines 71 to 74
/// Removes the last character from the `String` buffer and returns it.
pub fn pop(self) -> Option<u8> {
self.bytes.pop()
}
Copy link

Choose a reason for hiding this comment

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

This and remove are problematic as well in terms of unicode.

@bitzoic
Copy link
Member Author

bitzoic commented Oct 7, 2022

Perhaps we only want to start off with the interface, but then we should state that this does no validation and essentially is just Vec<u8> (which Rust's String is also backed by, but the API is unicode-aware).

As the use for strings in blockchains is pretty minimal and with the current support for characters and str in Sway, this is definitely more of an interface than anything. I see the primary use case being to pass a string to a contract rather than building a string in the contract, for example needing to store a URI for some NFT metadata. This is something that has been asked for by developers previously.

Nonetheless, I do hope to incorporate a safer use of strings once chars are available.

Centril
Centril previously approved these changes Oct 8, 2022
Copy link

@Centril Centril left a comment

Choose a reason for hiding this comment

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

Looks good with the new docs 🚀

@bitzoic bitzoic requested review from matt-user and Braqzen October 10, 2022 14:58
matt-user
matt-user previously approved these changes Oct 10, 2022
Copy link

@matt-user matt-user left a comment

Choose a reason for hiding this comment

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

lgtm


For more information please see the [specification](./SPECIFICATION.md).

> **Note** There is currently no way to convert a `str` to a `String`.

## Known Issues

It is important to note that unlike Rust's `String`, this `String` library does **not** guarantee a valid UTF-8 string. The `String` currently behaves only as a `vec` and does not perform any validation. This intended to be supported in the future with the introduction of [`char`](https://github.com/FuelLabs/sway/issues/2937) to the Sway language.

Choose a reason for hiding this comment

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

Good comment, this clears up a lot of the confusion I was having with the lib

sway_libs/src/string/README.md Outdated Show resolved Hide resolved
sway_libs/src/string/README.md Outdated Show resolved Hide resolved
sway_libs/src/string/README.md Outdated Show resolved Hide resolved
sway_libs/src/string/README.md Outdated Show resolved Hide resolved
sway_libs/src/string/README.md Outdated Show resolved Hide resolved
tests/src/test_projects/string/tests/functions/nth.rs Outdated Show resolved Hide resolved
tests/src/test_projects/string/tests/functions/pop.rs Outdated Show resolved Hide resolved
tests/src/test_projects/string/tests/functions/push.rs Outdated Show resolved Hide resolved
tests/src/test_projects/string/tests/functions/remove.rs Outdated Show resolved Hide resolved
@bitzoic bitzoic merged commit d250e34 into master Oct 10, 2022
@bitzoic bitzoic deleted the bitzoic-11 branch October 10, 2022 17:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Lib: Strings Label used to filter for the library issue New Feature New addition that does not currently exist
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Strings Library
5 participants