Debugger API improvements #1382

Jither · 2022-12-17T17:15:13Z

Added Skip event to DebugHandler (see https://github.com/Jither/Jint.DebuggerExample/blob/main/NOTES.md#Events for rationale)
Added Parsed event to Engine for giving e.g. debuggers access to the AST as parsed by the engine, when passing code rather than AST to Execute/Evaluate.
BindingNames on scopes are now lazily evaluated. Because we still want to leave out scopes without any bindings from scope chains (for now), a HasBindings() method has been added to environment records.

Breaking changes:

Global and Local properties on CallFrame have been removed. Because Jint's scope system is now more complex (and mostly aligned with other Javascript debuggers), Global and Local are very arbitrary scopes to make special cases for. Either can, of course, still be retrieved through the ScopeChain property on CallFrame.
Shadowed bindings are no longer removed from outer scopes - they're inspectable like any other binding.
When a breakpoint placed at a debugger statement is hit, only one Break event will be triggered (with PauseType = Break), rather than two (one with PauseType = Break, the other with PauseType = DebuggerStatement). This aligns the behavior with other debuggers - e.g. Chromium dev tools and Visual Studio's debugger - generally, only one event is triggered per execution point, with the priority being: Step > Break > DebuggerStatement > Skip.

These changes would bring https://github.com/Jither/Jint.DebugAdapter back to a place where it can be built against official Jint (although it still has some major issues, and module support in particular hasn't been tested at all yet).

No more removal of shadowed bindings in scopes

Do not trigger debugger statement when that statement also has a breakpoint Fixed some incorrect null-forgiving operators in DebugHandler

lahma · 2022-12-18T14:27:17Z

Jint/Engine.cs

+        /// <summary>
+        /// The Parsed event is triggered whenever a script has been parsed as a result of execution or evaluation.
+        /// </summary>
+        public event ParsedHandler? Parsed;


nit: kind of more like ScriptParsed as doesn't support module code

should these be in options? usually anything configurable is found there

nit: kind of more like ScriptParsed as doesn't support module code

I was actually writing this while you replied:

Rethinking the Parsed event - and the recent Loaded event on DefaultModuleLoader. Ideally (especially for the VS Code DebugAdapter), those two should be combined into just Parsed on Engine. A debugger would need at least the source (ID) of the parsed script - to use as for breakpoints and as a general key - as well as the AST. But of course, after parsing, the source is already available on the AST's root node.

The DebugAdapter protocol needs the Parsed event (or possibly more accurately a "script ready to execute" event) in order to wait for the debugging client (e.g. VS Code) to set e.g. breakpoint positions (after which VS Code sends configurationDone and the script can start evaluation). In order to set breakpoints, of course, the AST needs to be available.

So, a more general approach would be to remove Loaded from DefaultModuleLoader again, and instead let the engine handle it for all module loaders, by triggering the same Parsed event as a "classic" script Execute (with arguments object sender and Program ast) here:

https://github.com/sebastienros/jint/blob/main/Jint/Engine.Modules.cs#L44

(or does it make more sense to add it in ImportModule?)

... and, for "classic" scripts, somewhere... inhere:

jint/Jint/Engine.cs

Line 295 in 4b6f445

var globalEnv = Realm.GlobalEnv;

... which would "unify" Parsed for the two methods of loading scripts. For the case of modules, e.g. a DebugAdapter would only listen for the first time the event is triggered.

Any idea at which points the event would make most sense to add? Considering the Parsed event will, in the case of e.g. a DebugAdapter, be used to pause execution of the Engine thread - e.g. (in the VS Code example) until the debugging client is done setting up breakpoints etc.

should these be in options? usually anything configurable is found there

In the original draft from last year (which I ended up deleting), they were actually calls to DebugHandler.OnScriptReady, and the ScriptReady event was on DebugHandler - along with Step/Break/Skip. Not sure about that, since I guess they may be useful outside of debugging too. How would we handle events with the Options fluent API?

I'm good if you're good with the solution 😉

I wouldn't mind if fluent API isn't there for more special options. It's Options patterns more now with fluent available for common cases IMO. There are already many switches not visible on fluent side of I remember right.

Problem is, if we start introducing C# events to Options, that would require Options to also include logic for dispatching those events, or at the very least have an internal OnParsed on Options. Not just from a pattern perspective - that's enforced by the compiler. I.e., it will force Options to have more responsibilities than just collecting options.

ETA: The good news about the Parsed event is that, no matter where it ends up being placed, the changes described above do bring Jint.DebugAdapter back to a working, almost-full-featured state, including debugging modules (albeit just as unstable as it always was). 😋

Moved Parsed to DebugHandler for now, pending this discussion - just to implement the event for modules too - and test it with the DebugAdapter as shown above.

Removed DefaultModuleLoader.Loaded event - a less general version of DebugHandler.Parsed. Moved Parsed event to DebugHandler (for now)

lahma · 2022-12-19T06:09:12Z

Jint/Engine.cs

@@ -292,6 +292,8 @@ public Engine Execute(Script script)
        /// </summary>
        private Engine ScriptEvaluation(ScriptRecord scriptRecord)
        {
+            DebugHandler.OnParsed(scriptRecord.EcmaScriptCode);


nit: script might be existing Script instance and thus not parsed in literal sense

Yeah - the comment for it is also semi-deliberately phrased as "The Parsed event is triggered after the engine retrieves a parsed AST of a script or module." Might be even more accurate to say "receives". Not making it specific to the places where Jint parses the script itself, because a user may want those cases included - and can easily ignore them, if they prepared the Script themselves. Any ideas for a better name? I guess its point is really BeforeExecute or BeforeEvaluate. ScriptReady or similar seems too vague.

At some point, it might also make sense, for completion, to add a trigger to the evil sides of script parsing - the debugger would then easily be able to step through eval'ed scripts too, much like Chrome devtools (Jint.DebugAdapter already has support for debugging scripts that aren't part of the file system, but where the source is delivered by the host).

Well it's internal so it's all about splitting hairs here. Before/After is good pairing, maybe even OnEvaluating/OnEvaluated, but all good as it's again, internal 🙂

Well, the event that it ends up triggering - Parsed - isn't internal. 🙂

Ah true. We're still in beta, we can break things even after this 😉

But the Debugger API has been so constantly breaking that I'd like for once to make a nice choice. 😐 Ended up with BeforeEvaluate (never liked -ing in event names, because it can both indicate "before" and "during").

And, more importantly, moved it into EvaluateModule rather than LoadModule, which is also used for loading modules for other purposes than evaluation. ~~If that looks OK, it's good to go, I think.~~ Argh, or not - looks like I need another test case...

Yeah, because now it doesn't take LinkModule into account, only EvaluateModule - and LinkModule is actually the main reason it's needed (scripts you don't pass to the engine yourself). 😋 But LinkModule doesn't do evaluation, so the event name, once again, becomes meaningless... 🤨

So there... The name stays for now, although it's now moved back to LoadModule 😋 Added a better test case (for two levels of import), and also tested again with Jint.ExampleDebugger and Jint.DebugAdapter.

lahma

LGTM, we can merge when you say so!

More comprehensive test of BeforeEvaluate behavior for modules

lahma · 2022-12-20T05:56:44Z

Thanks once again!

Jither · 2022-12-20T08:34:01Z

Thanks once again!

My pleasure - I've updated Jint.DebugAdapter and Jint.DebuggerExample (as well as NOTES.md in the latter) to reflect the changes. 🙂

Jither added 5 commits December 12, 2022 23:27

Removed obsolete Global and Local scope properties

7646c42

No more removal of shadowed bindings in scopes

Skip event

a54181a

Do not trigger debugger statement when that statement also has a breakpoint Fixed some incorrect null-forgiving operators in DebugHandler

Parsed event

ca60751

Small code style fix

306bb0f

Comment typo fix

ea4cc36

lahma reviewed Dec 18, 2022

View reviewed changes

Parsed event for both scripts and modules

7d770cc

Removed DefaultModuleLoader.Loaded event - a less general version of DebugHandler.Parsed. Moved Parsed event to DebugHandler (for now)

lahma reviewed Dec 19, 2022

View reviewed changes

lahma approved these changes Dec 19, 2022

View reviewed changes

Jither added 3 commits December 19, 2022 20:14

Moved and renamed OnParsed -> OnBeforeEvaluate

b4e42a0

Moved OnBeforeEvaluate back to LoadModule

04f05ff

More comprehensive test of BeforeEvaluate behavior for modules

Merge branch 'main' into debugger-api-improvements

2117116

lahma merged commit 3c45b8c into sebastienros:main Dec 20, 2022

Jither deleted the debugger-api-improvements branch December 20, 2022 08:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Debugger API improvements #1382

Debugger API improvements #1382

Jither commented Dec 17, 2022

lahma Dec 18, 2022

lahma Dec 18, 2022

Jither Dec 18, 2022 •

edited

Jither Dec 18, 2022

lahma Dec 18, 2022

Jither Dec 18, 2022 •

edited

Jither Dec 18, 2022

lahma Dec 19, 2022

Jither Dec 19, 2022

lahma Dec 19, 2022

Jither Dec 19, 2022

lahma Dec 19, 2022

Jither Dec 19, 2022 •

edited

Jither Dec 19, 2022

Jither Dec 19, 2022

lahma left a comment

lahma commented Dec 20, 2022

Jither commented Dec 20, 2022

Debugger API improvements #1382

Debugger API improvements #1382

Conversation

Jither commented Dec 17, 2022

Breaking changes:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jither Dec 18, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jither Dec 18, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jither Dec 19, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lahma left a comment

Choose a reason for hiding this comment

lahma commented Dec 20, 2022

Jither commented Dec 20, 2022

Jither Dec 18, 2022 •

edited

Jither Dec 18, 2022 •

edited

Jither Dec 19, 2022 •

edited