Open
Bug 1414683
Opened 7 years ago
Updated 2 years ago
[jsdbg2] Debugger.Environment.prototype.type should return more detailed information
Categories
(Core :: JavaScript Engine, enhancement, P3)
Core
JavaScript Engine
Tracking
()
NEW
Tracking | Status | |
---|---|---|
firefox58 | --- | fix-optional |
People
(Reporter: jimb, Unassigned)
References
(Blocks 2 open bugs)
Details
At the moment, the Debugger.Environment.prototype.type accessor property distinguishes only between "declarative", "with", and "object" environments. This causes clients that want to help users recognize scopes to resort to other means to make finer discriminations, which is often inefficient and inaccurate.
For example, here's some code implementing the Firefox devtools protocol's environment actor:
// What is this environment's type?
if (this.obj.type == "declarative") {
form.type = this.obj.callee ? "function" : "block";
} else {
form.type = this.obj.type;
}
http://searchfox.org/mozilla-central/rev/5a60492a53667fc61a62af1847d005a210b7a4f6/devtools/server/actors/environment.js#45-50
On environments with callees, checking the callee property here causes the allocation of a Debugger.Object which is used as a boolean value and abandoned. Meanwhile, the implementation of D.E.p.type is throwing away exactly the details the environment actor wants:
bool
DebugEnvironmentProxy::isForDeclarative() const
{
EnvironmentObject& e = environment();
return e.is<CallObject>() ||
e.is<VarEnvironmentObject>() ||
e.is<ModuleEnvironmentObject>() ||
e.is<WasmInstanceEnvironmentObject>() ||
e.is<WasmFunctionCallObject>() ||
e.is<LexicalEnvironmentObject>();
}
http://searchfox.org/mozilla-central/rev/5a60492a53667fc61a62af1847d005a210b7a4f6/js/src/vm/EnvironmentObject.cpp#2371-2381
It's reasonable to be concerned about exposing implementation details via the Debugger API, but most of these distinctions look like they can be defined in terms of the source language.
Updated•7 years ago
|
status-firefox58:
--- → fix-optional
Priority: -- → P3
Reporter | ||
Updated•7 years ago
|
Blocks: js-devtools
Comment 1•7 years ago
|
||
Here are some notes from a November call where we discussed this:
https://docs.google.com/document/d/1QZyTzte5UvQsqN3ad38Q2RqIEJgTWsRqZgsYU4VEP5E/edit
Comment 2•7 years ago
|
||
Here is a quick patch based on my memory of our call in November. I did not try to compile this so, it is safe to say that it will likely contain lots of syntax errors. The main thing i'm interested in though is if this captures the primary goals:
1. make environment->type() more fine grained
2. simplify the environment actor
https://github.com/jasonLaster/gecko-dev/compare/bookmarks/central...jasonLaster:js-env-type
Jim, let me know how close this was to what you remember...
Comment 3•6 years ago
|
||
I'm not going to do an implementation of this, but I did look it over to see how complex it would be, and I wanted to share what I could see.
I think that patch is a step in the right direct. My main concern was that it only covers a few of the cases that the debugger's own AST visitor marks, so I was curious to see where things diverged. As you might guess, the main thing is that, from a user standpoint, it would be nice to be able to have more specific information about where a lexical scope is used. Some places in the engine have custom scopes to handle spec edge cases, but there a lots of places where things are generic lexical scopes. While that's fine for the engine, it's not ideal if the goal is for users to easily see what scopes map to.
Here's a list of the scopes we handle in the devtools AST visitor, compared to what I could see in the engine.
* Global
* ScopeKind::Global
* Lexical Global
* Emitted in a standard lexical scope
* Class Inner
* Emitted in a standard lexical scope
* For-loop lexical capture
* Emitted in a standard lexical scope
* Switch
* Emitted in a standard lexical scope
* Block
* Emitted in a standard lexical scope
* Module
* ScopeKind::Module
* Named function expression
* ScopeKind::NamedLambda
* ScopeKind::StrictNamedLambda
* Function
* ScopeKind::Function
* ScopeKind::ParameterExpressionVar
* ScopeKind::FunctionBodyVar
* Lexical Function Body
* ScopeKind::Lexical
* Catch
* ScopeKind::SimpleCatch
* ScopeKind::Catch
* With
* ScopeKind::With
* Class Field Inititializer
* Doesn't exist in engine
The other issue is that not all of these ScopeKind types are exposed in their respective environment types, so that might also need to be passed through somewhere.
Updated•6 years ago
|
Blocks: dbg-scopes
Updated•6 years ago
|
Blocks: dbg-help-wanted
Keywords: helpwanted
Updated•6 years ago
|
Keywords: good-first-bug
Updated•6 years ago
|
Whiteboard: [debugger-mvp]
Updated•5 years ago
|
Whiteboard: [debugger-mvp]
Comment 4•5 years ago
|
||
I'd love to help with this, but it's unclear from the bug exactly what is the goal of this change. Is the goal to simply give additional information on scope by including "function" and "block" to "with" and "object"? Or do something more comprehensive like what Mr. Smyth recommended? Or simply clean up the patch that Mr. Laster drafted? If you could be a little more specific, I'd love to help!
Thanks!
Will
Updated•4 years ago
|
Blocks: js-debugger
Comment 5•4 years ago
|
||
Dropping the good-first-bug and helpwanted tags, given this bug isn't currently mentored.
Keywords: good-first-bug,
helpwanted
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•