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.

Sign up for GitHub

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

Jump to bottom

Debugger API improvements #1382

Merged
merged 9 commits into from
Dec 20, 2022
Merged

Debugger API improvements #1382

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
7646c42
Removed obsolete Global and Local scope properties
Jither Dec 12, 2022
a54181a
Skip event
Jither Dec 17, 2022
ca60751
Parsed event
Jither Dec 17, 2022
306bb0f
Small code style fix
Jither Dec 17, 2022
ea4cc36
Comment typo fix
Jither Dec 17, 2022
7d770cc
Parsed event for both scripts and modules
Jither Dec 18, 2022
b4e42a0
Moved and renamed OnParsed -> OnBeforeEvaluate
Jither Dec 19, 2022
04f05ff
Moved OnBeforeEvaluate back to LoadModule
Jither Dec 19, 2022
2117116
Merge branch 'main' into debugger-api-improvements
Jither Dec 19, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
28 changes: 28 additions & 0 deletions Jint.Tests/Runtime/Debugger/BreakPointTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,7 @@ public void DebuggerStatementTriggersBreak()
bool didBreak = false;
engine.DebugHandler.Break += (sender, info) =>
{
Assert.Equal(PauseType.DebuggerStatement, info.PauseType);
didBreak = true;
return StepMode.None;
};
Expand Down Expand Up @@ -232,6 +233,33 @@ public void DebuggerStatementDoesNotTriggerBreakWhenStepping()
Assert.False(didBreak);
}

[Fact]
public void DebuggerStatementDoesNotTriggerBreakWhenAtBreakPoint()
{
string script = @"'dummy';
debugger;
'dummy';";

var engine = new Engine(options => options
.DebugMode()
.DebuggerStatementHandling(DebuggerStatementHandling.Script)
.InitialStepMode(StepMode.None));

int breakCount = 0;

engine.DebugHandler.BreakPoints.Set(new BreakPoint(2, 0));

engine.DebugHandler.Break += (sender, info) =>
{
Assert.Equal(PauseType.Break, info.PauseType);
breakCount++;
return StepMode.None;
};

engine.Execute(script);
Assert.Equal(1, breakCount);
}

[Fact]
public void BreakPointDoesNotTriggerBreakWhenStepping()
{
Expand Down
2 changes: 1 addition & 1 deletion Jint.Tests/Runtime/Debugger/DebugHandlerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ public void AvoidsPauseRecursion()

if (info.ReachedLiteral("target"))
{
var obj = info.CurrentScopeChain.Global.GetBindingValue("obj") as ObjectInstance;
var obj = info.CurrentScopeChain[0].GetBindingValue("obj") as ObjectInstance;
var prop = obj.GetOwnProperty("name");
// This is where reentrance would occur:
var value = prop.Get.Invoke(engine);
Expand Down
7 changes: 4 additions & 3 deletions Jint.Tests/Runtime/Debugger/ScopeTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,7 +299,8 @@ function power(a)
{
Assert.Collection(info.CurrentScopeChain,
scope => AssertScope(scope, DebugScopeType.Local, "arguments", "a"),
scope => AssertScope(scope, DebugScopeType.Closure, "b", "power"), // a, this, arguments shadowed by local
// a, arguments shadowed by local - but still exist in this scope
scope => AssertScope(scope, DebugScopeType.Closure, "a", "arguments", "b", "power"),
scope => AssertScope(scope, DebugScopeType.Script, "x", "y", "z"),
scope => AssertScope(scope, DebugScopeType.Global, "add"));
});
Expand Down Expand Up @@ -327,7 +328,7 @@ function add(a, b)
Assert.Collection(info.CurrentScopeChain,
scope => AssertScope(scope, DebugScopeType.Block, "y"),
scope => AssertScope(scope, DebugScopeType.Local, "arguments", "a", "b"),
scope => AssertScope(scope, DebugScopeType.Script, "x", "z"), // y shadowed
scope => AssertScope(scope, DebugScopeType.Script, "x", "y", "z"), // y is shadowed, but still in the scope
scope => AssertScope(scope, DebugScopeType.Global, "add"));
});
}
Expand Down Expand Up @@ -391,7 +392,7 @@ function add(a, b)
scope => AssertScope(scope, DebugScopeType.Block, "x"),
scope => AssertScope(scope, DebugScopeType.Block, "y"),
scope => AssertScope(scope, DebugScopeType.Local, "arguments", "a", "b"),
scope => AssertScope(scope, DebugScopeType.Script, "z"), // x, y shadowed
scope => AssertScope(scope, DebugScopeType.Script, "x", "y", "z"), // x, y are shadowed, but still in the scope
scope => AssertScope(scope, DebugScopeType.Global, "add"));
});
}
Expand Down
49 changes: 48 additions & 1 deletion Jint.Tests/Runtime/Debugger/StepModeTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -413,7 +413,7 @@ public void StepNotTriggeredWhenRunning()
function test()
{
'dummy';
'øummy';
'dummy';
}";

var engine = new Engine(options => options
Expand All @@ -432,5 +432,52 @@ function test()

Assert.Equal(1, stepCount);
}

[Fact]
public void SkipIsTriggeredWhenRunning()
{
string script = @"
'step';
'skip';
'skip';
debugger;
'step';
'step';
";

var engine = new Engine(options => options
.DebugMode()
.DebuggerStatementHandling(DebuggerStatementHandling.Script)
.InitialStepMode(StepMode.Into));

int stepCount = 0;
int skipCount = 0;

engine.DebugHandler.Step += (sender, info) =>
{
Assert.True(TestHelpers.IsLiteral(info.CurrentNode, "step"));
stepCount++;
// Start running after first step
return stepCount == 1 ? StepMode.None : StepMode.Into;
};

engine.DebugHandler.Skip += (sender, info) =>
{
Assert.True(TestHelpers.IsLiteral(info.CurrentNode, "skip"));
skipCount++;
return StepMode.None;
};

engine.DebugHandler.Break += (sender, info) =>
{
// Back to stepping after debugger statement
return StepMode.Into;
};

engine.Execute(script);

Assert.Equal(2, skipCount);
Assert.Equal(3, stepCount);
}
}
}
112 changes: 108 additions & 4 deletions Jint.Tests/Runtime/EngineTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
using Jint.Native.Object;
using Jint.Runtime;
using Jint.Runtime.Debugger;
using Jint.Tests.Runtime.Debugger;
using Xunit.Abstractions;

#pragma warning disable 618
Expand Down Expand Up @@ -1584,13 +1585,11 @@ private StepMode EngineStepVerifyDebugInfo(object sender, DebugInformation debug
Assert.NotNull(debugInfo.CallStack);
Assert.NotNull(debugInfo.CurrentNode);
Assert.NotNull(debugInfo.CurrentScopeChain);
Assert.NotNull(debugInfo.CurrentScopeChain.Global);
Assert.NotNull(debugInfo.CurrentScopeChain.Local);

Assert.Equal(2, debugInfo.CallStack.Count);
Assert.Equal("func1", debugInfo.CurrentCallFrame.FunctionName);
var globalScope = debugInfo.CurrentScopeChain.Global;
var localScope = debugInfo.CurrentScopeChain.Local;
var globalScope = debugInfo.CurrentScopeChain.Single(s => s.ScopeType == DebugScopeType.Global);
var localScope = debugInfo.CurrentScopeChain.Single(s => s.ScopeType == DebugScopeType.Local);
Assert.Contains("global", globalScope.BindingNames);
Assert.Equal(true, globalScope.GetBindingValue("global").AsBoolean());
Assert.Contains("local", localScope.BindingNames);
Expand Down Expand Up @@ -2882,6 +2881,111 @@ public void ShouldAllowOptionalChainingForMemberCall()
Assert.True(array[1].IsUndefined());
}

[Fact]
public void ExecuteShouldTriggerParsedEvent()
{
TestParsedEvent(
(engine, code) => engine.Execute(code),
expectedSource: "<anonymous>"
);
}

[Fact]
public void ExecuteWithSourceShouldTriggerParsedEvent()
{
TestParsedEvent(
(engine, code) => engine.Execute(code, "mysource"),
expectedSource: "mysource"
);
}

[Fact]
public void ExecuteWithParserOptionsShouldTriggerParsedEvent()
{
TestParsedEvent(
(engine, code) => engine.Execute(code, ParserOptions.Default),
expectedSource: "<anonymous>"
);
}

[Fact]
public void ExecuteWithSourceAndParserOptionsShouldTriggerParsedEvent()
{
TestParsedEvent(
(engine, code) => engine.Execute(code, "mysource", ParserOptions.Default),
expectedSource: "mysource"
);
}

[Fact]
public void EvaluateShouldTriggerParsedEvent()
{
TestParsedEvent(
(engine, code) => engine.Evaluate(code),
expectedSource: "<anonymous>"
);
}

[Fact]
public void EvaluateWithSourceShouldTriggerParsedEvent()
{
TestParsedEvent(
(engine, code) => engine.Evaluate(code, "mysource"),
expectedSource: "mysource"
);
}

[Fact]
public void EvaluateWithParserOptionsShouldTriggerParsedEvent()
{
TestParsedEvent(
(engine, code) => engine.Evaluate(code, ParserOptions.Default),
expectedSource: "<anonymous>"
);
}

[Fact]
public void EvaluateWithSourceAndParserOptionsShouldTriggerParsedEvent()
{
TestParsedEvent(
(engine, code) => engine.Evaluate(code, "mysource", ParserOptions.Default),
expectedSource: "mysource"
);
}

[Fact]
public void ImportModuleShouldTriggerParsedEvent()
{
TestParsedEvent(
(engine, code) =>
{
engine.AddModule("my-module", code);
engine.ImportModule("my-module");
},
expectedSource: "my-module"
);
}

private static void TestParsedEvent(Action<Engine, string> call, string expectedSource)
{
var engine = new Engine();

const string script = "'dummy';";

var parsedTriggered = false;
engine.DebugHandler.Parsed += (sender, ast) =>
{
parsedTriggered = true;
Assert.Equal(engine, sender);
Assert.Equal(expectedSource, ast.Location.Source);
Assert.Collection(ast.Body, node => Assert.True(TestHelpers.IsLiteral(node, "dummy")));
};

call(engine, script);

Assert.True(parsedTriggered);
}

private class Wrapper
{
public Testificate Test { get; set; }
Expand Down
17 changes: 0 additions & 17 deletions Jint.Tests/Runtime/Modules/DefaultModuleLoaderTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,23 +35,6 @@ public void ShouldRejectPathsOutsideOfBasePath(string specifier)
Assert.StartsWith(exc.Specifier, specifier);
}

[Fact]
public void ShouldTriggerLoadedEvent()
{
var loader = new DefaultModuleLoader(ModuleTests.GetBasePath());
bool triggered = false;
loader.Loaded += (sender, source, module) =>
{
Assert.Equal(loader, sender);
Assert.NotNull(source);
Assert.NotNull(module);
triggered = true;
};
var engine = new Engine(options => options.EnableModules(loader));
engine.ImportModule("./modules/format-name.js");
Assert.True(triggered);
}

[Fact]
public void ShouldResolveBareSpecifiers()
{
Expand Down
5 changes: 5 additions & 0 deletions Jint/Engine.Modules.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,11 @@ internal ModuleRecord LoadModule(string? referencingModuleLocation, string speci
module = LoaderFromModuleLoader(moduleResolution);
}

if (module is SourceTextModuleRecord sourceTextModule)
{
DebugHandler.OnParsed(sourceTextModule._source);
}

return module;
}

Expand Down
4 changes: 3 additions & 1 deletion Jint/Engine.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -292,6 +292,8 @@ public Engine Execute(Script script)
/// </summary>
private Engine ScriptEvaluation(ScriptRecord scriptRecord)
{
DebugHandler.OnParsed(scriptRecord.EcmaScriptCode);
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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).

Copy link
Collaborator

Choose a reason for hiding this comment

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

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 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@Jither Jither Dec 19, 2022 •
edited

Choose a reason for hiding this comment

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

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...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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... 🤨

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.


var globalEnv = Realm.GlobalEnv;

var scriptContext = new ExecutionContext(
Expand Down Expand Up @@ -363,7 +365,7 @@ public ManualPromise RegisterPromise()

Action<JsValue> SettleWith(FunctionInstance settle) => value =>
{
settle.Call(JsValue.Undefined, new[] {value});
settle.Call(JsValue.Undefined, new[] { value });
RunAvailableContinuations();
};

Expand Down
2 changes: 1 addition & 1 deletion Jint/Runtime/Debugger/BreakLocation.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ public BreakLocation(int line, int column) : this(null, line, column)

}

public BreakLocation(string source, Esprima.Position position) : this(source, position.Line, position.Column)
public BreakLocation(string? source, Esprima.Position position) : this(source, position.Line, position.Column)
{
}

Expand Down