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

feat: Implement print method #3576

Closed

Conversation

grasshopper47
Copy link
Contributor

@grasshopper47 grasshopper47 commented Nov 25, 2023

Description

Problem*

Resolves #3575

Summary*

Adds print method alongside println to ease printing in special use-cases, such as elements of slices.
Updates appropriate tests, and logging.md documentation.
Add a subchapter to 03_string.md documentation for the fmtstr type

Additional Context

Documentation*

Check one:

  • No documentation needed.
  • Documentation included in this PR.
  • [Exceptional Case] Documentation to be submitted in a separate PR.

PR Checklist*

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt on default settings.

@grasshopper47
Copy link
Contributor Author

@TomAFrench @vezenovm review please

Copy link
Contributor

@vezenovm vezenovm 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. Could you also add a couple print tests to nargo_cli/tests/execution_success/strings?

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.

If we add a print function, I think we should reimplement println in terms of that so that we only have 1 primitive instead of two. println could be rewritten as

unconstrained pub fn println<T>(input: T) {
    print(input);
    print("\n");
}

@grasshopper47
Copy link
Contributor Author

grasshopper47 commented Nov 27, 2023

If we add a print function, I think we should reimplement println in terms of that so that we only have 1 primitive instead of two. println could be rewritten as

unconstrained pub fn println<T>(input: T) {
    print(input);
    print("\n");
}

Would

print(f"{input}
");

be a quicker alternative than 2 calls?

@jfecher
Copy link
Contributor

jfecher commented Nov 28, 2023

Would [snippet] be a quicker alternative than 2 calls?

Possibly, I'm not quite sure. Feel free to go with whichever approach is easier / more straightforward.

@TomAFrench
Copy link
Member

I think this would be preferable to avoid multiple foreign calls being performed.

@grasshopper47
Copy link
Contributor Author

I think this would be preferable to avoid multiple foreign calls being performed.

It creates a problem: what if you pass a fmtstr down to println, and it passes that down to print ... formatted format strings cannot be printed:

image

I get the crash above when I run a personal test Noir project through the newly built compiler

I'm surprised this isn't caught at test-time....

As specified in the docs, I ran nix flake check -L, thinking this is how to run the tests. There are countless std::println(f" statements in the .nr tests, why didn't I get an error during testing?

@grasshopper47
Copy link
Contributor Author

grasshopper47 commented Nov 29, 2023

I think this would be preferable to avoid multiple foreign calls being performed.

Pending a reply on the previous point, I have another suggestion:

image image

@TomAFrench Let me know if this is the right idea

@jfecher
Copy link
Contributor

jfecher commented Nov 29, 2023

@grasshopper47 format strings should be able to be printed. If I had to guess what was happening, when you switched println to print, we're missing an explicit check for Println to handle format strings before it is actually called. That check, along with any others, would need to be changed to check for Print instead. Alternatively, I'm fine with the oracle change as well (we'll just need to test it to make sure it still works for tuples). Although we should name the intermediate value to make it more obvious what it represents E.g. print_newline.

@grasshopper47
Copy link
Contributor Author

grasshopper47 commented Nov 29, 2023

@grasshopper47 format strings should be able to be printed. If I had to guess what was happening, when you switched println to print, we're missing an explicit check for Println to handle format strings before it is actually called. That check, along with any others, would need to be changed to check for Print instead. Alternatively, I'm fine with the oracle change as well (we'll just need to test it to make sure it still works for tuples). Although we should name the intermediate value to make it more obvious what it represents E.g. print_newline.

Format strings ARE printable, just someplace else, and duplicating that in execute_print would be (needless) extra work IMO. _with_newline param looks "cleaner".

For tuples, my guess is that we'll need to switch the params around, the bool being first, maybe.
I need to understand how to run the tests in order to check, can you help out?

@jfecher
Copy link
Contributor

jfecher commented Nov 29, 2023

I need to understand how to run the tests in order to check, can you help out?

You should be able to run them all with cargo test locally

@grasshopper47
Copy link
Contributor Author

grasshopper47 commented Nov 30, 2023

I need to understand how to run the tests in order to check, can you help out?

You should be able to run them all with cargo test locally

Unfortunately it fails to work, hence the question. Look-see:

image

@grasshopper47
Copy link
Contributor Author

@TomAFrench @jfecher

I've been trying to run the tests locally to see why 2 of the checks are failing, but I'm unable to do so, see previous comment.

Currently I'm blocked on this.

I've tried:

#[oracle(print)]
unconstrained fn print_oracle<T>(_input: T)

unconstrained pub fn print<T>(input: T) {
    print_oracle(input);
}
unconstrained pub fn println<T>(input: T) {
print(f"{input}
");

This will not work when passing fmtstr to println, as it would lead to a doubly-formatted string. Extracting that is needless extra work.

2nd thing I've tried:

#[oracle(print)]
unconstrained fn print_oracle<T>(_input: T, _with_newline: bool) {}

unconstrained pub fn print<T>(input: T) {
    print_oracle(input, false);
}

unconstrained pub fn println<T>(input: T) {
    print_oracle(input, true);
}

This works fine, except for the failing checks.

If there's no help further possible on this, then I suggest we revert to

unconstrained pub fn println<T>(input: T) {
    print(input);
    print("\n");
}

and call it a day

@jfecher
Copy link
Contributor

jfecher commented Nov 30, 2023

@grasshopper47 it runs past that point for me locally. Do you have clang, the c++ stdlib, and libomp installed? If you have nix/direnv I would have thought that'd be handled for you.

@grasshopper47
Copy link
Contributor Author

@grasshopper47 it runs past that point for me locally. Do you have clang, the c++ stdlib, and libomp installed? If you have nix/direnv I would have thought that'd be handled for you.

Clang install did the trick, many thanks!

@grasshopper47 grasshopper47 deleted the feat-implement-print branch November 30, 2023 20:21
@grasshopper47 grasshopper47 restored the feat-implement-print branch November 30, 2023 20:21
@grasshopper47
Copy link
Contributor Author

grasshopper47 commented Nov 30, 2023

@jfecher Something wonky about this branch, I remade the PR.

It contains all the changes requested here.

github-merge-queue bot pushed a commit that referenced this pull request Dec 5, 2023
# Description

## Problem\*

Resolves  **#3575
Resolves **#2912 (duplicate)

Continuation of #3576
## Summary\*

Refactors `println_oracle` method into `print` with boolean indicator,
allowing `print` to coexist with `println` under the same `ForeignCall`
string symbol.
Updates appropriate tests, and logging.md documentation.
Add a subchapter to `03_string.md` documentation for the `fmtstr` type

## Additional Context



## Documentation\*

Check one:
- [ ] No documentation needed.
- [x] Documentation included in this PR.
- [ ] **[Exceptional Case]** Documentation to be submitted in a separate
PR.

# PR Checklist\*

- [x] I have tested the changes locally.
- [x] I have formatted the changes with [Prettier](https://prettier.io/)
and/or `cargo fmt` on default settings.

---------

Co-authored-by: jfecher <[email protected]>
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.

Allow print without newline
4 participants