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

Basic strings #622

Merged
merged 34 commits into from
Jan 25, 2023
Merged

Basic strings #622

merged 34 commits into from
Jan 25, 2023

Conversation

vezenovm
Copy link
Contributor

@vezenovm vezenovm commented Jan 9, 2023

Related issue(s)

Resolves #383

The issue linked is to add log statements. Although not directly related, strings are being added to ultimately enable logging.

Description

This PR adds a basic string implementation. To correctly resolve string size we follow the same resolution strategy as arrays and under the hood we utilize Rust's String type and convert to a list of u8 integers.

Summary of changes

I added a String type to various type enums such as InputValue, UnresolvedType, Type in the HIR, Literal in the monomorphisation AST, etc. Strings are allowed to be passed into the ABI and compared against each other.

However, in this current implementation strings do not fully mock the functionality of arrays. This means we cannot replace individual elements of arrays, but a mutable string is possible by replacing the entire string value.

I debated whether or not to implement a string type directly or rather create a char type and then enable arrays to handle this char type. This felt like it would be a little bit more complex as we would then have to extend arrays to support string literals, and char literals would have to be added as well. To start, I went with a separate string type as it seems highly unlikely that many people are going to be doing string manipulation in Noir and the main goal of adding strings was to enable logging which will just require string literals. I could make this change right now, or I could open an issue for later to add chars and enable formatting/manipulation of strings.

Dependency additions / changes

(If applicable.)

Test additions / changes

I added a basic test checking whether or not we can pass string data to the ABI and compare it against string literals inside of Noir.

Checklist

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt with default settings.
  • I have linked this PR to the issue(s) that it resolves.
  • I have reviewed the changes on GitHub, line by line.
  • I have ensured all changes are covered in the description.

Additional context

(If applicable.)

@vezenovm vezenovm requested review from jfecher, guipublic and kevaundray and removed request for jfecher January 9, 2023 21:22
crates/nargo/src/cli/prove_cmd.rs Outdated Show resolved Hide resolved
crates/nargo/tests/test_data/strings/src/main.nr Outdated Show resolved Hide resolved
crates/noirc_abi/src/input_parser/toml.rs Outdated Show resolved Hide resolved
crates/noirc_abi/src/lib.rs Show resolved Hide resolved
crates/noirc_abi/src/lib.rs Outdated Show resolved Hide resolved
crates/noirc_evaluator/src/ssa/code_gen.rs Outdated Show resolved Hide resolved
crates/noirc_frontend/src/hir/type_check/expr.rs Outdated Show resolved Hide resolved
@vezenovm vezenovm requested a review from jfecher January 11, 2023 17:54
crates/noirc_abi/src/input_parser/toml.rs Outdated Show resolved Hide resolved
crates/noirc_abi/src/lib.rs Show resolved Hide resolved
crates/noirc_evaluator/src/ssa/code_gen.rs Outdated Show resolved Hide resolved
crates/noirc_frontend/src/hir/type_check/expr.rs Outdated Show resolved Hide resolved
crates/noirc_frontend/src/parser/parser.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@jfecher jfecher 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. I take it it is currently blocked until #620 is merged

crates/noirc_abi/src/lib.rs Show resolved Hide resolved
@vezenovm vezenovm requested a review from jfecher January 23, 2023 18:42
@vezenovm vezenovm requested a review from TomAFrench January 24, 2023 16:23
Copy link
Contributor

@jfecher jfecher 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, just some minor cleanup with removing unneeded comments then we can merge

crates/noirc_evaluator/src/ssa/code_gen.rs Outdated Show resolved Hide resolved
crates/noirc_frontend/src/parser/parser.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@jfecher jfecher left a comment

Choose a reason for hiding this comment

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

LGTM

@vezenovm vezenovm merged commit ae1b38b into master Jan 25, 2023
@vezenovm vezenovm deleted the mv/strings branch January 25, 2023 17:41
@kevaundray kevaundray mentioned this pull request Jan 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add log statements
4 participants