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: Add full call stacks to runtime errors #2310

Merged
merged 7 commits into from
Aug 15, 2023
Merged

Conversation

jfecher
Copy link
Contributor

@jfecher jfecher commented Aug 14, 2023

Description

Problem*

Resolves #2214

Summary*

Adds full call stacks to runtime errors. I'll update this PR when it is ready to be merged with examples of what the new errors look like.

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.

@jfecher jfecher mentioned this pull request Aug 14, 2023
2 tasks
@jfecher
Copy link
Contributor Author

jfecher commented Aug 14, 2023

I have a local commit that changes the errors to format as part of one error:

error: Failed constraint
   ┌─ /.../noir/short/src/main.nr:17:12
   │
17 │     assert(x == 246);
   │            --------
   │
   = Call stack:
     1. /.../noir/short/src/main
     2. /.../noir/short/src/main

But the issue is in generating the call stack string. Once codespan no longer handles the error spans, we have no way of converting the byte start and end offsets into line and column numbers. I'll push a commit tomorrow to add these by manually recounting them from the file contents.

@jfecher jfecher marked this pull request as ready for review August 15, 2023 15:08
@jfecher
Copy link
Contributor Author

jfecher commented Aug 15, 2023

This PR is now ready to review. The final formatting I've settled on is:

error: Failed constraint
   ┌─ /.../noir/short/src/main.nr:17:12
   │
17 │     assert(x == 246);
   │            --------
   │
   = Call stack:
     1. /.../noir/short/src/main.nr:9:5
     2. /.../noir/short/src/main.nr:13:5
     3. /.../noir/short/src/main.nr:17:12

Error: could not satisfy all constraints

In the future, we should add function names to each location as well.

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.

Generally looks good to me; I had one nit regarding interchangeably using callstack and location

@Ethan-000
Copy link
Contributor

should "unoptimized" be added to the spell checker. couple of places seems to be using this word

kevaundray
kevaundray previously approved these changes Aug 15, 2023
@kevaundray kevaundray added this pull request to the merge queue Aug 15, 2023
Merged via the queue into master with commit 9004181 Aug 15, 2023
15 checks passed
@kevaundray kevaundray deleted the jf/callstacks branch August 15, 2023 21:16
TomAFrench added a commit that referenced this pull request Aug 15, 2023
* master:
  feat(ssa): Perform dead instruction elimination on intrinsic functions (#2276)
  feat: Add full call stacks to runtime errors (#2310)
  chore(ci): fix mismatched input name to publish workflow (#2327)
TomAFrench added a commit that referenced this pull request Aug 16, 2023
* master: (34 commits)
  chore: move orphaned integration tests to new directory (#2331)
  chore(noir): Release 0.10.1 (#2328)
  feat(ssa): Switch mem2reg pass to be per function rather than per block (#2243)
  feat(ssa): Perform dead instruction elimination on intrinsic functions (#2276)
  feat: Add full call stacks to runtime errors (#2310)
  chore(ci): fix mismatched input name to publish workflow (#2327)
  chore: add README for integration test structure (#2277)
  feat: Improved error message for unexpected return type (#2302)
  feat(stdlib): Implement `str` `as_bytes` and `into_bytes` function (#2298)
  chore(ci): automatically convert changelog entries to sentence case (#2325)
  chore(noir): Release 0.10.0 (#2039)
  fix(lsp): Ensure lsp does not crawl past the root specified (#2322)
  fix: Prevent panic when passing relative paths to `--program-dir` (#2324)
  fix: Overflowing assignment will result in an error (#2321)
  chore: clippy fixes (#2320)
  chore: Parameterize the build mode for noir-wasm (#2317)
  chore: Make `wasm` tests pull from `result` directory (#2319)
  chore: Fix typo (#2315)
  chore: Reuse workspace target directory in wasm build script (#2312)
  feat(nargo): Add `--workspace` flag to run commands in every package (#2313)
  ...
TomAFrench added a commit that referenced this pull request Aug 17, 2023
* master: (25 commits)
  chore: update noir-source-resolver from `1.1.2` to `^1.1.3` (#2349)
  chore(ci): Avoid writing to cache in workflows triggered by the merge queue (#2341)
  chore(noir): Release 0.10.3 (#2344)
  feat(lsp): Add `Execute` code lens for `main` functions (#2330)
  feat(lsp): Add `Compile` code lens for `main` function and contracts (#2309)
  feat: Allow calling higher-order functions with closures (#2335)
  fix: Display warning if last expression of block is unused (#2314)
  chore(noir): Release 0.10.2 (#2343)
  fix: Prevent dead instruction elimination of brillig functions which may contain side-effects (#2340)
  chore: Separate integration tests for contracts and programs (#2339)
  chore: move orphaned integration tests to new directory (#2331)
  chore(noir): Release 0.10.1 (#2328)
  feat(ssa): Switch mem2reg pass to be per function rather than per block (#2243)
  feat(ssa): Perform dead instruction elimination on intrinsic functions (#2276)
  feat: Add full call stacks to runtime errors (#2310)
  chore(ci): fix mismatched input name to publish workflow (#2327)
  chore: add README for integration test structure (#2277)
  feat: Improved error message for unexpected return type (#2302)
  feat(stdlib): Implement `str` `as_bytes` and `into_bytes` function (#2298)
  chore(ci): automatically convert changelog entries to sentence case (#2325)
  ...
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 full call stack to the Location information in SSA
3 participants