Bug 209998 – Web Inspector: Debugger: treat comma sub-expressions as separate statements

• Component: Web Inspector
• Status: RESOLVED FIXED

[Devin Rousso]

Some minifies use `,` instead of `;` in many situations, and frankly the `,` operator is basically an inline-statement, so we should treat each comma sub-expression as a statement for pausing (including breakpoints).

[Devin Rousso]

Created attachment 395438 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 395438 [details]
Patch

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

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3045
> +    for (; node && node->next(); node = node->next()) {

I wonder if the `node` check is actually necessary here. Given we always fall out of this loop assuming `node` is valid.

> Source/WebInspectorUI/UserInterface/Workers/Formatter/JSFormatter.js:217
> +        switch (node.type) {
> +        case "BlockStatement":
> +        case "ClassDeclaration":
> +        case "DoWhileStatement":
> +        case "ForInStatement":
> +        case "ForOfStatement":
> +        case "ForStatement":
> +        case "FunctionDeclaration":
> +        case "IfStatement":
> +        case "SwitchStatement":
> +        case "TryStatement":
> +        case "WhileStatement":
> +        case "WithStatement":
> +            return true;

Previously this was sorted by frequency of statement, to "optimize" the fewest number of checks. I'm not sure it mattered. Switch is probably fine!

> LayoutTests/inspector/debugger/stepping/stepOver-expected.txt:222
> + ->  40        let a = 1|,
> +     41            b = 2,
> +     42            c = 3;

Wouldn't we rather pause before "b"? This seems poor.

> LayoutTests/inspector/debugger/stepping/stepOver-expected.txt:247
> + ->  43        testAlert("comma log 1"), |testAlert("comma log 2"), testAlert("comma log 3");

This looks correct tho!

> LayoutTests/inspector/formatting/resources/javascript-tests/sample-jquery-expected.js:4165
> -    }), e.jQuery = e.$ = b, "function" == typeof define && define.amd && define.amd.jQuery && define("jquery", [], function() {
> +    }),
> +    e.jQuery = e.$ = b,

Awesome improvement in general!

[Devin Rousso]

Comment on attachment 395438 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Workers/Formatter/JSFormatter.js:217
>> +            return true;
> 
> Previously this was sorted by frequency of statement, to "optimize" the fewest number of checks. I'm not sure it mattered. Switch is probably fine!

I was wondering how this was sorted!  In that case, I would've probably expected DoWhileStatement to be lower down :P

We're running this on a `Worker`, so this is probably fine :)

>> LayoutTests/inspector/debugger/stepping/stepOver-expected.txt:222
>> +     42            c = 3;
> 
> Wouldn't we rather pause before "b"? This seems poor.

I agree, but I'm not really sure what to do about this, especially since it's coming from the same `CommaNode` 🤔

>> LayoutTests/inspector/formatting/resources/javascript-tests/sample-jquery-expected.js:4165
>> +    e.jQuery = e.$ = b,
> 
> Awesome improvement in general!

Yeah!  I think this looks _way_ better :)

[Devin Rousso]

Created attachment 395495 [details]
Patch

Turns out, the only reason I needed to have `m_lastExecutedPosition` was because I was unnecessarily `emitDebugHook` before the first `CommaNode`, which really should already be handled by the container `StatementNode`.In addition to considering each `CommaNode` as a pause location, we should also consider them as a possible breakpoint location.

[Joseph Pecoraro]

Comment on attachment 395495 [details]
Patch

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

Do we have a test for something like:

    for(let x = 1, y = 2; ...; ...) {}

I want to make sure we don't pretty print that poorly, and that we'd pause on `y=2` naturally.

And maybe just sanity check this as well (though I don't expect it to be any different):

    let x;
    if (x = 1, x) {}

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3045
> +    for (; node && node->next(); node = node->next()) {

I still think we can and should drop the `node` at the start of the condition. If it was ever nullptr we'd crash at line 3047.

> LayoutTests/inspector/debugger/breakpoints/resolved-dump-all-pause-locations-expected.txt:1107
>  INSERTING AT: 166:1
> -PAUSES AT: 168:0
> +PAUSES AT: 167:4
>      163    
>      164    var v1 = 1,
>      165        v2 = 1;
>   -> 166    l#et l1 = 2,
> -    167        l2 = 2;
> + => 167        |l2 = 2;
> +    168    const c1 = 3,
> +    169        c2 = 3;
> +    170    

Nice!

[Devin Rousso]

(In reply to Joseph Pecoraro from comment #5)
> Comment on attachment 395495 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=395495&action=review
> 
> Do we have a test for something like:
> 
>     for(let x = 1, y = 2; ...; ...) {}
> 
> I want to make sure we don't pretty print that poorly, and that we'd pause on `y=2` naturally.
We already have pretty printing tests this.  I'll add a stepping test.

> And maybe just sanity check this as well (though I don't expect it to be any different):
> 
>     let x;
>     if (x = 1, x) {}
Good idea :)

[Devin Rousso]

Created attachment 395519 [details]
Patch

[Devin Rousso]

Created attachment 395537 [details]
Patch

Adjust the tests slightly for a better diff :)

[Joseph Pecoraro]

Comment on attachment 395537 [details]
Patch

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

Awesome! r=me

> Source/JavaScriptCore/parser/Parser.cpp:901
> +                    head = tail = context.createCommaExpr(headLocation, head);

Clever. Maybe too clever.

> LayoutTests/inspector/formatting/resources/javascript-tests/for-statements.js:38
>  for(var x=1;x<len;++x)1;
> -for(var x=1,len=10;x<len;++x)1;
> +for(var x=1,len=1;len=10,x<len;len=5,++x)1;

FWIW, if you're purely additive to these tests, the diffs end up easier to read and we hypothetically test more cases.

[Devin Rousso]

Created attachment 395909 [details]
Patch

[EWS]

Committed r259781: <https://trac.webkit.org/changeset/259781>

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

[Radar WebKit Bug Importer]

<rdar://problem/61496849>