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 support for nargo DAP #51

Merged
merged 10 commits into from
Mar 13, 2024
Merged

Conversation

ggiraldez
Copy link
Contributor

@ggiraldez ggiraldez commented Nov 29, 2023

Description

ℹ️ This PR should be applied after noir-lang/noir#4185 since it builds on top of the features implemented there.

Adds support in the extension to debug Noir binary packages using the DAP subcommand in nargo, implemented by noir-lang/noir#3627.

Problem

Part of noir-lang/noir#3015

Summary

This PR adds the configuration necessary in package.json of the extension to allow debugging Noir binary packages from inside VS.Code using the Debug Adapter implemented in the above mentioned noir PR.

Supports settings breakpoints in Noir source code and opening a Disassembly view while debugging to display the compiled ACIR/Brillig opcodes. All stepping commands will produce the same result, ie. stepping to the next "statement" in the Noir program, ie. there is no concept of stepping into or out of a function yet. If using the commands from the disassembly view, the debugger will step to the next opcode instead.

Additional Context

PR Checklist*

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

@ggiraldez ggiraldez changed the title feat: add support nargo DAP feat: add support for nargo DAP Nov 29, 2023
@kobyhallx
Copy link
Contributor

kobyhallx commented Dec 4, 2023

Usability Observation nr 1

No default launch - when I open debug panel and attempt to run noir program nothing will happen, instead I have to create launch config for any debug to take place.

@kobyhallx
Copy link
Contributor

kobyhallx commented Dec 4, 2023

Usability Observation nr 2

Program by default stops at first opcode - while this is desired in REPL style debugger I believe it should just execute to the end when there's no breakpoint on the way.

@kobyhallx
Copy link
Contributor

kobyhallx commented Dec 4, 2023

Usability Observation nr 3

If breakpoint is set in Function and user choses continue, execution does not stop and just continues to the end of program.

@kobyhallx
Copy link
Contributor

kobyhallx commented Dec 4, 2023

Usability Observation nr 4

Step into is stepping over.
(Note, I do realise that PR says otherwise but I believe this is limitation and breaks UX contract leading to users confusion)

@kobyhallx
Copy link
Contributor

kobyhallx commented Dec 4, 2023

Usability Observation nr 5

No variables are being displayed in Variables Panel.

Copy link
Contributor

@kobyhallx kobyhallx left a comment

Choose a reason for hiding this comment

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

With observatioins shared in comments
Usability Observation nr 5 is in my mind minimum to expose this through vscode.

@TomAFrench
Copy link
Member

Looks like there was some formatting changes from prettier which are included in here. I've fixed these issues in master but could you merge master back into this branch to reduce the diff @ggiraldez?

@TomAFrench
Copy link
Member

TomAFrench commented Dec 11, 2023

Actually, give me a couple of moments. I'm bringing across the linting config from the main repository so it can be enforced here.

Good to merge it into this branch now.

@ggiraldez
Copy link
Contributor Author

Usability Observation nr 1

No default launch - when I open debug panel and attempt to run noir program nothing will happen, instead I have to create launch config for any debug to take place.

This is a known issue, which shouldn't be hard to fix. But yes, agreed it makes the DX cumbersome.

@ggiraldez
Copy link
Contributor Author

Usability Observation nr 2

Program by default stops at first opcode - while this is desired in REPL style debugger I believe it should just execute to the end when there's no breakpoint on the way.

That's a good point. What would you expect to be the behavior if there are no breakpoints set? It could get a bit confusing because I guess by default nothing will happen (at least nothing noticeable) when you hit the Run button.

@ggiraldez
Copy link
Contributor Author

Usability Observation nr 3

If breakpoint is set in Function and user choses continue, execution does not stop and just continues to the end of program.

I guess the problem here is that the debugger is not being able to properly set the breakpoint but you're not getting feedback in the UI. I've encountered this issue as well, but haven't found a good solution yet.

@ggiraldez
Copy link
Contributor Author

Usability Observation nr 4

Step into is stepping over. (Note, I do realise that PR says otherwise but I believe this is limitation and breaks UX contract leading to users confusion)

All stepping commands do the same, which is to continue execution until the source location changes (ie. execute as many opcodes as necessary for the the source location from the debug artifact to change). Depending on compiler optimizations and inlining it may be that it looks like a function call is skipped over.

We will try to address this issue by compiling to Brillig only (instead of ACIR/Brillig) when debugging. We expect that to give us a bit more control over stepping and the ability to get proper stacktraces.

@ggiraldez
Copy link
Contributor Author

With observatioins shared in comments Usability Observation nr 5 is in my mind minimum to expose this through vscode.

Agreed. This is being worked on. There is some preliminary work in the https://github.com/manastech/noir/tree/dap-with-vars branch, but it depends on merging the debug variables instrumentation code.

mverzilli and others added 5 commits January 16, 2024 11:32
* Gracefully handle debugger loading/init errors
* Refactor: extract debugger functionality to own file
* Properly handle launch.json if exists
* Add a bit more context on preflight check
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.

Seems like this struggles with monorepos plus we should probably hide the witness map in the panel if we're compiling to brillig but we can merge this as is.

@TomAFrench TomAFrench merged commit 369fa91 into noir-lang:master Mar 13, 2024
4 checks passed
@github-actions github-actions bot mentioned this pull request Mar 13, 2024
@TomAFrench
Copy link
Member

Thank you and sorry for this PR being stuck in purgatory!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants