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: Refactor Logging to use Brillig foreign calls #1917

Merged
merged 26 commits into from
Jul 20, 2023
Merged

Conversation

vezenovm
Copy link
Contributor

@vezenovm vezenovm commented Jul 12, 2023

Description

Reimplement logging in a more generic way uses Brillig foreign calls and AbiType for de/serialization

Problem*

Resolves #1615, #1910, #1925 and partial work towards #1911

Summary*

There is now a new logging function

unconstrained fn println<T>(input: T)

As this matches the println used by the old SSA, some logic had to be adding to filter for the correct println function in the stdlib depending on whether the experimental ssa is activated.

During monomorphization of a function call we check whether we have an oracle for println. We then serialize the AbiType to a string literal and attach it as the latest argument of the function. nargo who is executing the println oracles then know how to interpret these compiler attached arguments.

The example program under crates/nargo_cli/tests/test_data_ssa_refactor/debug_logs/src/main.nr will give this output:

"*** println ***"
"0x05"
["0x05","0x0a"]
{"foo":"0x0f","my_struct":{"x":"0x0a","y":"0x05"}}

The enable_slices flag was renamed to experimental_ssa as it is needed in this PR.
In order to avoid the struct serialization set by aos_to_soa in monomorphization I also added checks of the experimental_ssa flag to monomorphization to avoid using aos_to_soa when we are compiling down to the new SSA pass.

Documentation

  • This PR requires documentation updates when merged.

    • I will submit a noir-lang/docs PR.
    • I will request for and support Dev Rel's help in documenting this PR.

Additional Context

PR Checklist*

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

noir_stdlib/src/lib.nr Outdated Show resolved Hide resolved
@vezenovm vezenovm marked this pull request as ready for review July 13, 2023 16:25
@jfecher
Copy link
Contributor

jfecher commented Jul 13, 2023

Fixes #1925

crates/nargo/src/ops/foreign_calls.rs Outdated Show resolved Hide resolved
crates/nargo/src/ops/foreign_calls.rs Outdated Show resolved Hide resolved
crates/nargo/src/ops/foreign_calls.rs Outdated Show resolved Hide resolved
crates/nargo/src/ops/foreign_calls.rs Outdated Show resolved Hide resolved
crates/noirc_frontend/src/monomorphization/mod.rs Outdated Show resolved Hide resolved
noir_stdlib/src/lib.nr Outdated Show resolved Hide resolved
@vezenovm vezenovm requested a review from jfecher July 19, 2023 16:51
crates/nargo/src/ops/foreign_calls.rs Show resolved Hide resolved
crates/noirc_frontend/src/hir/def_map/mod.rs Outdated Show resolved Hide resolved
noir_stdlib/src/lib.nr Show resolved Hide resolved
vezenovm and others added 2 commits July 20, 2023 14:23
Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com>
@vezenovm
Copy link
Contributor Author

vezenovm commented Jul 20, 2023

I was thinking that we'd have a Display implementation for InputValue as that's ultimately what we're wanting to display.

@TomAFrench I also need the AbiType though in order to convert an InputValue to the expected JsonType. Am I missing something in how the Display would work on InputValue?

This PR is blocking for these issues: #1925 and #1953 that are caused by coverting arrays of structs to structs of arrays. Perhaps we can merge this in and revisit moving the Display to InputValue?

@vezenovm vezenovm requested a review from jfecher July 20, 2023 15:08
@jfecher
Copy link
Contributor

jfecher commented Jul 20, 2023

If this is blocking for another issue, but the Display issue with this PR is blocking this one, we can split out the relevant change and merge only that. I'll leave the call to @TomAFrench whether Display is blocking this PR or not but it doesn't sound too major to me.

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.

My issues are all solved. I still think passing along the json type data as an extra argument is a bit hacky but I suppose we can remove this once we add traits in Noir.

@vezenovm vezenovm enabled auto-merge July 20, 2023 18:19
@vezenovm vezenovm added this pull request to the merge queue Jul 20, 2023
Merged via the queue into master with commit c15f9aa Jul 20, 2023
@vezenovm vezenovm deleted the mv/brillig_logs branch July 20, 2023 18:41
@critesjosh
Copy link
Contributor

As far as updating the docs that result from this change, is it just that we are now able to log structs in addition to Fields, integers and arrays?

@Savio-Sou
Copy link
Collaborator

Seems like so, @vezenovm could you confirm?

@vezenovm
Copy link
Contributor Author

vezenovm commented Aug 3, 2023

@critesjosh yes you can now log structs, but also expressions now (such as x + y). You can also perform string interpolation as of this PR: #1952, so you can construct more complex logs using nargo.

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.

Refactor Logging to use Brillig
5 participants