Closed Bug 1232816 Opened 8 years ago Closed 8 years ago

[jsdbg2] Add name, isTopLevel, and isEnclosingTopLevel properties to Debugger.Script

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED WONTFIX

People

(Reporter: jlong, Assigned: jlong)

References

Details

Attachments

(1 file)

This property is analogous to the `Debugger.Object.prototype.name` property. The D.Script already has `displayName`, but I want to know the pure name of the named function, and it should be `null` otherwise. The `displayName` is the inferred name the SpiderMonkey gives it.

I need this in the debugger for a new algorithm to determine when we should do sliding breakpoints.
Assignee: nobody → jlong
Summary: [jsdbg2] Add Debugger.Script.prototype.name → [jsdbg2] Add name, isTopLevel, and isEnclosingTopLevel properties to Debugger.Script
I need two more properties too, they are easy to add and super helpful for analyzing functions. One is `isTopLevel`, and this is a boolean that indicates that the code is running at "top-level" scope. In a normal JS file, this is global scope. In eval-ed code, it's... well the outermost level. In modules, it's the module scope. Top-level is a good name for all these things: it's the outermost scope of the Debugger.Source instance.

The other property if `isEnclosingTopLevel`. I'm open to other names. This is a boolean that indicates if the "enclosing scope" is top-level or not. It's useful for detecting top-level functions, which is exactly what I use it for.
Attached patch 1232816.patchSplinter Review
Attachment #8698813 - Flags: review?(shu)
I'd like to be clear about what "top-level" means and I think it should only mean two things: global scope or module scope. Any eval sources should not be considered top-level.

I tested the current patch with eval sources and am a little confused. If the scope is a `StaticEvalObject` it should return true, so I was expecting `isTopLevel` to be true for the outermost scope of an eval source.

For example, take this code:

1 eval(`
2   function bizzle() {
3     // Comment
4     return 10;
5   }
6
7   // foo bar baz
8   var x = 10;
9 `);

I would expect `isTopLevel` and `isEnclosingTopLevel` on like 7 to be true. I would expect only `isEnclosingTopLevel` to be true on line 4. But with this patch, `isTopLevel` is false for both and `isEnclosingTopLevel` is only true for line 8.

And none of that seems to change where I call `eval`, whether it's global scope or inside a function.

I'd like to actually remove the check for `StaticEvalObject` and make sure `isTopLevel` is always false for evals. I suppose `isEnclosingTopLevel` could be true for lines 7-8 if `eval` is called from global scope, but I don't think static scopes know anything about where it's called from.
I changed my mind; I think top-level should consistently mean "the static outermost scope of this Debugger.Source object". And that means even for eval sources, they would have a static "top-level" which is just the outermost scope, irregardless of where they were eval-ed.

That means in the above code, `isTopLevel` and `isEnclosingTopLevel` should be true for lines 7 and 8, and `isEnclosingTopLevel` should be true for lines 2-4.
(In reply to James Long (:jlongster) from comment #4)
> I changed my mind; I think top-level should consistently mean "the static
> outermost scope of this Debugger.Source object". And that means even for
> eval sources, they would have a static "top-level" which is just the
> outermost scope, irregardless of where they were eval-ed.
> 
> That means in the above code, `isTopLevel` and `isEnclosingTopLevel` should
> be true for lines 7 and 8, and `isEnclosingTopLevel` should be true for
> lines 2-4.

This really depends on what you want to do with eval scripts. All this makes me think that we might want to expose the notion of 'scopes', i.e., the static nesting structure + binding names, in addition to 'environments', i.e., their dynamic instances.
Comment on attachment 8698813 [details] [diff] [review]
1232816.patch

Review of attachment 8698813 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not sure what's meant by isEnclosingTopLevel.

Also, this would need tests in jit-test/tests/debug and documentation doc/Debugger/Debugger.Script.md.

::: js/src/vm/Debugger.cpp
@@ +4714,5 @@
> +    if (!dbg->wrapDebuggeeValue(cx, &namev))
> +        return false;
> +    args.rval().set(namev);
> +    return true;
> +}

This is the same implementation as getDisplayName, except calling atom() instead of displayAtom(). Could you refactor?

@@ +4845,5 @@
> +    THIS_DEBUGSCRIPT_SCRIPT(cx, argc, vp, "(get isTopLevel)", args, obj, script);
> +    JSObject *scope = script->innermostStaticScope();
> +
> +    if(IsStaticGlobalLexicalScope(scope) ||
> +       scope->is<StaticEvalObject>() ||

A script's innermostStaticScope() will never be a StaticEvalObject. In ES6 all eval code get an invisible lexical scope around them, so 'let' and 'const' decls don't escape. So, the innermostStaticScope of eval scripts is always a Block scope.

@@ +4849,5 @@
> +       scope->is<StaticEvalObject>() ||
> +       scope->is<ModuleObject>()) {
> +        args.rval().setBoolean(true);
> +    }
> +    else {

Nit: SpiderMonkey style for ifs:

if (multilineCondition1 ||
    multilineCondition2)
{
   ...
} else {
   ...
}

That said, this is clearer written as |args.rval().setBoolean(IsStaticLexicalScope(scope) || ...)|.

@@ +4856,5 @@
> +    return true;
> +}
> +
> +static bool
> +DebuggerScript_isEnclosingTopLevel(JSContext* cx, unsigned argc, Value* vp)

So I'm not sure what you actually want here. Is this for function scripts defined at the top level? What about direct eval scripts at the top level?

If it's for functions defined at the top level, it should check for that explicitly. What about functions directly under a block on the top level? e.g.,

{
  function f() {}
}
Attachment #8698813 - Flags: review?(shu)
Closing in favor of bug 1233287
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: