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(std_lib): println statements #630

Merged
merged 66 commits into from
Feb 14, 2023
Merged

feat(std_lib): println statements #630

merged 66 commits into from
Feb 14, 2023

Conversation

vezenovm
Copy link
Contributor

@vezenovm vezenovm commented Jan 12, 2023

Related issue(s)

Resolves #383

Description

The new Directive::Log was added in acvm release 0.4.1. The backend has been already updated to reflect this new acvm release.

This PR adds logging statements to Noir. This will enable developers to print out respective variables as they are computed, arrays, and string literals. This will greatly help improve the developer experience as Noir devs will now have an easy (and popular) debugging tool available until more complex debugging is possible with Noir.

Currently, for dev purposes I am restricting the number of arguments in a log statement to one. This can be expanded in a separate PR.

Summary of changes

A builtin function called println has been added to the Noir std lib. There are some differences between println our existing builtin functions as println does not return anything and should not be optimized out.

The majority of the changes are in the noirc_evaluator as logs need to be able to print out the variables according to how they are used in the program. The most significant addition is the evaluate_println where the final string to be logged is constructed and a gate with a new Directive::Log is added to the evaluator. The directive in the acvm can be found here: noir-lang/acvm#61. This new directive can contain either a single string or a vector of witnesses. In the case of a finalized output string the directive simply prints the string it contains and always resolves in the PWG. For the case where our output is witnesses we must fetch the witness values to output when solving the PWG and construct the final string there.

evaluate_println needs some extra information that other builtin methods don't require. It must determine whether to print an array as field elements or a utf8 and thus must know if the expression passed to it is a string. I added Println(bool) to the builtin::Opcode enum. The bool contained in this enum specifies whether the argument to be printed in a string. If a future PR expands println's to accept multiple variables this will have to be changed.

Dependency additions / changes

This is branched off of mv/strings and will re-use some of the decoding information from this PR for printing string arrays.

mv/strings is now in master so no heavy dependency changes here.

Test additions / changes

I am adding usage of std::println to the strings test.

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.)

@TomAFrench
Copy link
Member

log() may not be the best name as it clashes with logarithms, maybe change to println() to mirror rust? /bikeshedding

@vezenovm
Copy link
Contributor Author

log() may not be the best name as it clashes with logarithms, maybe change to println() to mirror rust? /bikeshedding

I agree. I will change this

@kevaundray kevaundray requested a review from jfecher February 14, 2023 16:14
jfecher
jfecher previously approved these changes Feb 14, 2023
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.

👍 Thank you

@vezenovm
Copy link
Contributor Author

Corresponding ACVM issue stemming from this PR: noir-lang/acvm#94

Will push something update Directive::Log in a moment

TomAFrench
TomAFrench previously approved these changes Feb 14, 2023
Copy link
Member

@TomAFrench TomAFrench left a comment

Choose a reason for hiding this comment

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

LGTM. My main issue with the printing during tests was that you'd get output on the same line as the test name instead than the one below, rather than the test status being printed afterwards. I see that's fixed so happy to merge this now.

@vezenovm vezenovm added this pull request to the merge queue Feb 14, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 14, 2023
@vezenovm vezenovm added this pull request to the merge queue Feb 14, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 14, 2023
@vezenovm vezenovm dismissed stale reviews from TomAFrench and jfecher via 0a93b7f February 14, 2023 17:31
@kevaundray kevaundray enabled auto-merge February 14, 2023 17:44
@kevaundray kevaundray added this pull request to the merge queue Feb 14, 2023
Merged via the queue into master with commit d5d1be2 Feb 14, 2023
@kevaundray kevaundray deleted the mv/logging branch February 14, 2023 18:48
@vezenovm
Copy link
Contributor Author

@Globallager @signorecello

Forgot to label this doc needed, just pinging y'all that this is now merged

TomAFrench added a commit that referenced this pull request Feb 15, 2023
* master:
  chore(ssa): Predicate for store instruction (#829)
  feat(std_lib): println statements (#630)
  chore(ci): Make Kev the committer on release lockfile updates (#845)
  feat(stdlib): Add higher order array functions (#833)
  chore(ssa): Remove values from array type (#771)
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
5 participants