Debugger improvements 4 #1018
Debugger improvements 4 #1018
Conversation
BindingObject property on Global and With scopes (mainly for devtools for now, but may be useful for VSCode DebugAdapter)
Only allows a single breakpoint at a given location (source/line/column) - hence, API is now "Set" instead of "Add". Requires the breakpoint to be set at *exactly* the correct line/column (this allows faster matching, column breakpoints, and more). Fixed tests broken by addition of Script scope and new breakpoint semantics. Breakpoints still need work on the API (and probably implementation) side. Additional BreakPoint tests
| /// BreakLocation is a combination of an Esprima position (line and column) and a source (path or identifier of script). | ||
| /// Like Esprima, first column is 0 and first line is 1. | ||
| /// </summary> | ||
| public sealed record BreakLocation |
There was a problem hiding this comment.
btw, if you want really make the code short, this should also work:
public sealed record BreakLocation(string Source, int Line, int Column)
{
public BreakLocation(int line, int column) : this(null, line, column)
{
}
public BreakLocation(string source, Esprima.Position position) : this(source, position.Line, position.Column)
{
}
}These are of course personal preferences and not important for this PR, I've just really liked the expressiveness they have 🙂
There was a problem hiding this comment.
Not a fan (yet) of positional records, when the properties aren't self-evidently positional (emphasized here by the multiple constructors). But that's also just philosophy. May change it anyway. 😉
| public sealed class BreakPoint | ||
| // BreakPoint is not sealed. It's useful to be able to add additional properties on a derived BreakPoint class (e.g. a breakpoint ID | ||
| // or breakpoint type) but still let it be managed by Jint's breakpoint collection. | ||
| public class BreakPoint |
There was a problem hiding this comment.
For the same reason as above, not sure record adds something to BreakPoint - value equality, yes, but not entirely sure if value equality is even appropriate for a breakpoint. 🙂 (But that's just my impaired brain speaking)
Jint/SourceParsedEventArgs.cs
Outdated
|
|
||
| namespace Jint | ||
| { | ||
| public class SourceParsedEventArgs |
There was a problem hiding this comment.
should be sealed if not an extension point, maybe even a record 😉
There was a problem hiding this comment.
Sealed, absolutely. But philosophy-wise, here I'm quite sure record doesn't add much, though - can't remember ever needing to compare two eventargs. 🙂 (and since some EventArgs may oftentimes need to be mutable, I'll generalize/take a stance (for now) and say no EventArgs should be records, even when they're immutable 😉).
Co-authored-by: Sébastien Ros <sebastienros@gmail.com>
Co-authored-by: lph <philipp.luethi@sva-ag.ch> Co-authored-by: Marko Lahma <marko.lahma@gmail.com> Co-authored-by: Sébastien Ros <sebastienros@gmail.com>
Issues: * function-valued properties should be excluded entirely from JSON, not serialized as objects * proxies to functions are not handled correctly Changes: * Remove extraneous early exit case (JsonSerializer line 175) which diverges from the algorithm in the spec (https://tc39.es/ecma262/#sec-serializejsonproperty). This appears to correct the serialization of function-valued properties, but unmasks another issue where ProxyInstance is mistakenly treated as callable even when target is not * Change "value is ICallbable" test to "value.IsCallable" so that serialization of ProxyInstance is handled correctly (according to the spec proxies are callable only if the target is callable - https://tc39.es/ecma262/#sec-proxycreate) * Add test to verify serialization of function-valued properties
Co-authored-by: Jonathan Resnick <jonathan.resnick@synaptivemedical.com>
* fix dictionary missing key property access * fix destructuring with function arg in strict mode * fix passing functions to cs side * fix Array.sort crashing script in debug mode * fix indexer property being enumerated
* script functions should resolve to same reference * fix construct crashing without context
BindingObject property on Global and With scopes (mainly for devtools for now, but may be useful for VSCode DebugAdapter)
Only allows a single breakpoint at a given location (source/line/column) - hence, API is now "Set" instead of "Add". Requires the breakpoint to be set at *exactly* the correct line/column (this allows faster matching, column breakpoints, and more). Fixed tests broken by addition of Script scope and new breakpoint semantics. Breakpoints still need work on the API (and probably implementation) side. Additional BreakPoint tests
…bugger-improvements-4
|
Hmmm... Judging from that list of commits, I clearly messed up when fetching the upstream. I'll probably manually rebase the changes onto the latest dev, when they're done - since so far, they don't interfere at all with the rest of the engine. |
Test case for failing breakpoint conditions (currently they, and other evaluations during paused state) reset the call stack (and probably other things). Documentation of SourceParsedEventArgs
Renamed DebugHandler OnBreak event to clarify that it only handles debugger statements. Added PauseType (Break/Step/DebuggerStatement) to DebugInformation to allow debuggers to distinguish between breaks and debugger statements.
|
Messed up history - will open a new clean slate, based on latest main |
As discussed, and referenced in #1016
Minor compared to previous debugger-improvements PR's, but a relatively important step for Chrome devtools protocol and/or Debug Adapter Protocol (VSCode).
Summary of changes
bold areas = breaking changes (breaking for debugger implementations, that is)
Parsed) on engine for notification of when scripts have been through Esprima. This avoids the need for double parsing, because debugger implementations are likely to need the AST too (for e.g. determining proper locations for breakpoints).BreakLocationclass.Breakpoints still need work on the API (and probably implementation) side.