Web Inspector: Debugger: Step Over should only step through comma expressions if they are comma statements

• Component: Web Inspector
• Status: RESOLVED FIXED

[Devin Rousso]

Step Over should be able to step through
```
    a(), b(), c()
```
but NOT through
```
    true && (a(), b(), c()) && true
```

[Devin Rousso]

Created attachment 396663 [details]
Patch

[BJ Burg]

Comment on attachment 396663 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=396663&action=review

Looks good overall to me, though it would be good to have Keith, Yusuke, or Saam double check the AST / emitter changes.

> Source/JavaScriptCore/parser/Parser.cpp:3204
> +    TreeExpression expression = parseExpression(context, true);

Ugh, 'true' as an argument. This seems like an ideal case to do something like enum class IsStatement::{Yes,No}.

[Saam Barati]

Comment on attachment 396663 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=396663&action=review

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3277
> +    // If the expression is not a comma chain, pre-prime the `Debugger` to pause on sub-expressions.
> +    // If the expression is a comma chain, each sub-expression is treated as a standalone statement.
> +    if (!m_expr->isCommaNode())
> +        generator.emitDebugHook(WillExecuteExpression, position());

I don't really get why this is needed. Can you explain?
(You say what it's doing, but not why.)

Does the debugger already have a way of stepping through expressions in Inspector?

>> Source/JavaScriptCore/parser/Parser.cpp:3204
>> +    TreeExpression expression = parseExpression(context, true);
> 
> Ugh, 'true' as an argument. This seems like an ideal case to do something like enum class IsStatement::{Yes,No}.

agreed

[Ross Kirsling]

I don't think "statement" is the descriptor you want here -- it's never the case that an expression *is* a statement, and there's no difference between the two examples in comment 0 in terms of their status as expressions.

This is also a property specific to comma expressions, so perhaps you could just add `m_isStepDebuggable`* to CommaNode itself and set it from createCommaExpr?

(* There may be a better name than this, but I think I'd recommend naming it from Web Inspector's perspective because I'm not certain that it can be coherently named from the AST perspective. We can't say m_parentIsStatement unless you want this apply to if/while/return/throw/etc., and I'm not sure that there's anything that *just* ExpressionStatement and DeclarationStatement can be said to have in common.)

[Devin Rousso]

(In reply to Ross Kirsling from comment #4)
> I don't think "statement" is the descriptor you want here -- it's never the case that an expression *is* a statement, and there's no difference between the two examples in comment 0 in terms of their status as expressions.
In the example in comment 0, I'm specifically focusing on the `a(), b(), c()` sub-expression in both cases.  I agree that they're both expressions, but in the eyes of developers there is a distinction: the former expression (`a(), b(), c()`) effectively _is_ the statement, whereas the latter (`a(), b(), c()` inside `true && (a(), b(), c()) && true`) is a sub-expression as part of a larger expression/statement.  Similar to how there is an `ExprStatementNode`, the `m_isStatement` is intended to convey that this `ExpressionNode` is held by an `ExprStatementNode`, and therefore may want different considerations (such as described above).

> This is also a property specific to comma expressions, so perhaps you could just add `m_isStepDebuggable`* to CommaNode itself and set it from createCommaExpr?
For now, yes it's specific to comma expressions.  In the future, however, we may want to do other things.  I try to have things that are added by Web Inspector be non-specific so that if it can be used by something else too, we don't end up duplicating things.

> (* There may be a better name than this, but I think I'd recommend naming it from Web Inspector's perspective because I'm not certain that it can be coherently named from the AST perspective. We can't say m_parentIsStatement unless you want this apply to if/while/return/throw/etc., and I'm not sure that there's anything that *just* ExpressionStatement and DeclarationStatement can be said to have in common.)
I totally forgot about `return` and `throw`!  I think those should also be included.

As far as `if` and `while`, those are complex statements, in that they're not just a prefix identifier (e.g. `let` or `return`) followed by a regular expression.  I don't think developers think of `while (a(), b(), c()) { }` in the same way as either of the examples in comment 0.

[Devin Rousso]

Comment on attachment 396663 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=396663&action=review

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3277
>> +        generator.emitDebugHook(WillExecuteExpression, position());
> 
> I don't really get why this is needed. Can you explain?
> (You say what it's doing, but not why.)
> 
> Does the debugger already have a way of stepping through expressions in Inspector?

Actually, this is probably the wrong approach.  I'm gonna think about this a bit more.

[Ross Kirsling]

(In reply to Devin Rousso from comment #5)
> For now, yes it's specific to comma expressions.  In the future, however, we
> may want to do other things.  I try to have things that are added by Web
> Inspector be non-specific so that if it can be used by something else too,
> we don't end up duplicating things.

It would be easy to broaden it to expressions in general later though. What's dangerous is to pretend something is broad when it isn't -- i.e. to have a m_parentIsStatement flag and then only set it in a small subset of "parent is statement" cases.

> As far as `if` and `while`, those are complex statements, in that they're
> not just a prefix identifier (e.g. `let` or `return`) followed by a regular
> expression.  I don't think developers think of `while (a(), b(), c()) { }`
> in the same way as either of the examples in comment 0.

But developers aren't generally using the comma operator in the first place outside of declaration lists, in which setting they're not thinking of it as an operator at all. :P

[Devin Rousso]

(In reply to Ross Kirsling from comment #7)
> (In reply to Devin Rousso from comment #5)
> > For now, yes it's specific to comma expressions.  In the future, however, we may want to do other things.  I try to have things that are added by Web Inspector be non-specific so that if it can be used by something else too, we don't end up duplicating things.
> 
> It would be easy to broaden it to expressions in general later though. What's dangerous is to pretend something is broad when it isn't -- i.e. to have a m_parentIsStatement flag and then only set it in a small subset of "parent is statement" cases.

I'm working on a patch that adds `return` and `throw` as well.  I don't think I'm missing any other cases, but if I am I think we should add them now as well.

> > As far as `if` and `while`, those are complex statements, in that they're not just a prefix identifier (e.g. `let` or `return`) followed by a regular expression.  I don't think developers think of `while (a(), b(), c()) { }` in the same way as either of the examples in comment 0.
> 
> But developers aren't generally using the comma operator in the first place outside of declaration lists, in which setting they're not thinking of it as an operator at all. :P

Minifiers use commas all the time.  We've gotten a bunch of bugs in the past (and recently) about having to deal with the comma operator when trying to debug minified code.  It may not be common for developers to write this intentionally, but it's seen in a huge portion of code that's actually sent to users.  Frankly, this improvement is likely something that a developer will never encounter _unless_ they are debugging minified code.  This was a big motivator for bug 209998 (r259781) and bug 210324 (r260113),

[Ross Kirsling]

(In reply to Devin Rousso from comment #8)
> I'm working on a patch that adds `return` and `throw` as well.  I don't
> think I'm missing any other cases, but if I am I think we should add them
> now as well.

But you didn't want to include if/while/for, right? So then we can't say anything about StatementNodes as a whole...

> Minifiers use commas all the time.  We've gotten a bunch of bugs in the past
> (and recently) about having to deal with the comma operator when trying to
> debug minified code.  It may not be common for developers to write this
> intentionally, but it's seen in a huge portion of code that's actually sent
> to users.  Frankly, this improvement is likely something that a developer
> will never encounter _unless_ they are debugging minified code.  This was a
> big motivator for bug 209998 (r259781) and bug 210324 (r260113),

Right, that definitely makes sense -- I just mean that folks might think of comma expressions in if/while/for heads the same way if minifiers actually produced that.

[Devin Rousso]

(In reply to Ross Kirsling from comment #9)
> (In reply to Devin Rousso from comment #8)
> > I'm working on a patch that adds `return` and `throw` as well.  I don't think I'm missing any other cases, but if I am I think we should add them now as well.
> 
> But you didn't want to include if/while/for, right? So then we can't say anything about StatementNodes as a whole...

In the case of if/while/for, the comma expression there is sub-expression (emphasis on sub), whereas in the case of `ExprStatement`/declaration/return/throw the expression effectively _is_ the statement.  One way I'm looking at this is how much overlap there is between the parent statement and the child expression.  In the case of `ExprStatement`/declaration/return/throw, the range of the child expression almost entirely overlaps that of the parent statement, with the only difference being a keyword (e.g. `let`, `return`, `throw`, etc.), and in the case of `ExprStatement` the range overlap is 100%.  The idea behind `isStatement` is to indicate that the expression actually _is_ the statement, not that it's _part_ of a parent statement but that it _is_ the parent statement (with the only "allowed" difference being what I described in the last sentence).  if/while/for don't follow that logic, as the child expression not only doesn't overlap in the same way, but is also in an entirely different grammatical context.

> > Minifiers use commas all the time.  We've gotten a bunch of bugs in the past (and recently) about having to deal with the comma operator when trying to debug minified code.  It may not be common for developers to write this intentionally, but it's seen in a huge portion of code that's actually sent to users.  Frankly, this improvement is likely something that a developer will never encounter _unless_ they are debugging minified code.  This was a big motivator for bug 209998 (r259781) and bug 210324 (r260113),
> 
> Right, that definitely makes sense -- I just mean that folks might think of comma expressions in if/while/for heads the same way if minifiers actually produced that.

What do you mean by "if minifiers actually produced that"?

[Ross Kirsling]

(In reply to Devin Rousso from comment #10)
> In the case of if/while/for, the comma expression there is sub-expression
> (emphasis on sub), whereas in the case of
> `ExprStatement`/declaration/return/throw the expression effectively _is_ the
> statement.  One way I'm looking at this is how much overlap there is between
> the parent statement and the child expression.  In the case of
> `ExprStatement`/declaration/return/throw, the range of the child expression
> almost entirely overlaps that of the parent statement, with the only
> difference being a keyword (e.g. `let`, `return`, `throw`, etc.), and in the
> case of `ExprStatement` the range overlap is 100%.  The idea behind
> `isStatement` is to indicate that the expression actually _is_ the
> statement, not that it's _part_ of a parent statement but that it _is_ the
> parent statement (with the only "allowed" difference being what I described
> in the last sentence).  if/while/for don't follow that logic, as the child
> expression not only doesn't overlap in the same way, but is also in an
> entirely different grammatical context.

Yeah, but you see how that's completely arbitrary from the AST perspective, right? Statements are statements and some of them have expressions as children. :) Everything but an ExprStatement itself will add non-trivial logic on top of the expression.

WI should certainly do whatever's most helpful for developers, but somebody working in JSC's parser is not usually thinking about WI. In general, trying to have a flag not be WI-specific is good, but this is only realistic if the flag has a motivation independent of WI. I believe the only motivation here is "what WI wants to be able to step over" so it would be most helpful to be honest about that in the naming.

> > Right, that definitely makes sense -- I just mean that folks might think of comma expressions in if/while/for heads the same way if minifiers actually produced that.
> 
> What do you mean by "if minifiers actually produced that"?

You were saying that developers will encounter `a(), b(), c();` in minified code but not `while (a(), b(), c()) { ... }`. Presumably the former is originally three statements and the return values for the first two are unused. Now, we can't have three statements in a while condition, but we could have `while (c()) { ... a(); b(); }`, so it's still conceivable to commatize that, in which case you might want to step over in the same way.

[Devin Rousso]

(In reply to Ross Kirsling from comment #11)
> (In reply to Devin Rousso from comment #10)
> > In the case of if/while/for, the comma expression there is sub-expression (emphasis on sub), whereas in the case of `ExprStatement`/declaration/return/throw the expression effectively _is_ the statement.  One way I'm looking at this is how much overlap there is between the parent statement and the child expression.  In the case of `ExprStatement`/declaration/return/throw, the range of the child expression almost entirely overlaps that of the parent statement, with the only difference being a keyword (e.g. `let`, `return`, `throw`, etc.), and in the case of `ExprStatement` the range overlap is 100%.  The idea behind `isStatement` is to indicate that the expression actually _is_ the statement, not that it's _part_ of a parent statement but that it _is_ the parent statement (with the only "allowed" difference being what I described in the last sentence).  if/while/for don't follow that logic, as the child expression not only doesn't overlap in the same way, but is also in an entirely different grammatical context.
> 
> Yeah, but you see how that's completely arbitrary from the AST perspective, right? Statements are statements and some of them have expressions as children. :) Everything but an ExprStatement itself will add non-trivial logic on top of the expression.

I think this is codifying that relationship of "statement has an expression child" so that the child expression is able to reason about itself.

> > > Right, that definitely makes sense -- I just mean that folks might think of comma expressions in if/while/for heads the same way if minifiers actually produced that.
> > 
> > What do you mean by "if minifiers actually produced that"?
> 
> You were saying that developers will encounter `a(), b(), c();` in minified code but not `while (a(), b(), c()) { ... }`. Presumably the former is originally three statements and the return values for the first two are unused. Now, we can't have three statements in a while condition, but we could have `while (c()) { ... a(); b(); }`, so it's still conceivable to commatize that, in which case you might want to step over in the same way.

I've seen `while (a(), b(), c()) { ... }` _many_ times.  Even more so with `if`.  Minifiers do super funky things :/

[Devin Rousso]

Created attachment 397131 [details]
Patch

[Ross Kirsling]

Comment on attachment 397131 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=397131&action=review

> Source/JavaScriptCore/parser/NodeConstructors.h:837
> +        m_expr->setIsOnlyChildOfStatement();

You don't need a setter at all if you follow the pattern of something like isLocation(). :D

[Ross Kirsling]

Comment on attachment 397131 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=397131&action=review

>> Source/JavaScriptCore/parser/NodeConstructors.h:837
>> +        m_expr->setIsOnlyChildOfStatement();
> 
> You don't need a setter at all if you follow the pattern of something like isLocation(). :D

Er sorry, I'm being silly, this isn't true. Maybe we could move the logic to ASTBuilder though.

[BJ Burg]

Comment on attachment 397131 [details]
Patch

r=me, nice work!

[EWS]

Committed r260520: <https://trac.webkit.org/changeset/260520>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 397131 [details].

[Radar WebKit Bug Importer]

<rdar://problem/62193674>