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

fix(noirc_driver): Move error printing into nargo #1598

Merged
merged 4 commits into from
Jun 8, 2023

Conversation

phated
Copy link
Contributor

@phated phated commented Jun 7, 2023

Description

Problem*

Resolves #1537

Summary*

This refactors noirc_driver functions to return Vec<FileDiagnostic> if there is a problem compiling. The error reporter calls are then moved into nargo_cli since that is the only place we'd want them reported on stderr.

These changes make it possible for the LSP server to compile a file and send diagnostics back to the client (ref #1584).

I wasn't super happy to return non-Error types in the Err of the result, but I also don't think that these make sense to be an error, since we have to use a custom reporter to display them. I'm open to suggestions on how to clean this up.

Additional work:

  • Added some impl From because it made converting between diagnostics/RuntimeError nicer.
  • Removed try_vecmap because it bails at the first error, but we want to collect all of the FileDiagnostic errors that could occur and then only return a successful result it the errors are empty.
  • Added a file_manager() function to Driver so I could get a reference to the internal file manager. This isn't great and the FileManager should instead be controlled by Nargo and passed by reference to the driver. I'll open an issue for this.
  • Switched the unwrap_or_else calls in the wasm crate to use expect. The unwrap_or_else doesn't work consistently when I switched an API to Option instead of Result and expect does the exact same thing.

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.

TomAFrench
TomAFrench previously approved these changes Jun 8, 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

crates/noirc_driver/src/lib.rs Outdated Show resolved Hide resolved
Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com>
Copy link
Contributor

@kevaundray kevaundray left a comment

Choose a reason for hiding this comment

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

re-approving and running CI again

@kevaundray kevaundray enabled auto-merge June 8, 2023 09:35
@kevaundray kevaundray added this pull request to the merge queue Jun 8, 2023
Merged via the queue into master with commit 561cd63 Jun 8, 2023
@kevaundray kevaundray deleted the phated/driver-diagnostics branch June 8, 2023 11:22
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.

Move printing errors to nargo_cli
3 participants