Bug 209998 - Web Inspector: Debugger: treat comma sub-expressions as separate statements
Summary: Web Inspector: Debugger: treat comma sub-expressions as separate statements
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Local Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on:
Blocks: 210253
  Show dependency treegraph
 
Reported: 2020-04-03 21:32 PDT by Devin Rousso
Modified: 2020-04-21 13:12 PDT (History)
10 users (show)

See Also:


Attachments
Patch (109.58 KB, patch)
2020-04-03 23:05 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (179.77 KB, patch)
2020-04-05 01:59 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (193.99 KB, patch)
2020-04-05 11:39 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (135.07 KB, patch)
2020-04-05 17:33 PDT, Devin Rousso
joepeck: review+
Details | Formatted Diff | Diff
Patch (134.54 KB, patch)
2020-04-08 21:40 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2020-04-03 21:32:47 PDT
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).
Comment 1 Devin Rousso 2020-04-03 23:05:13 PDT
Created attachment 395438 [details]
Patch
Comment 2 Joseph Pecoraro 2020-04-04 11:06:46 PDT
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!
Comment 3 Devin Rousso 2020-04-04 19:39:32 PDT
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 :)
Comment 4 Devin Rousso 2020-04-05 01:59:27 PDT
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.
Comment 5 Joseph Pecoraro 2020-04-05 03:02:41 PDT
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!
Comment 6 Devin Rousso 2020-04-05 11:16:06 PDT
(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 :)
Comment 7 Devin Rousso 2020-04-05 11:39:14 PDT
Created attachment 395519 [details]
Patch
Comment 8 Devin Rousso 2020-04-05 17:33:17 PDT
Created attachment 395537 [details]
Patch

Adjust the tests slightly for a better diff :)
Comment 9 Joseph Pecoraro 2020-04-08 13:47:13 PDT
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.
Comment 10 Devin Rousso 2020-04-08 21:40:41 PDT
Created attachment 395909 [details]
Patch
Comment 11 EWS 2020-04-08 22:10:32 PDT
Committed r259781: <https://trac.webkit.org/changeset/259781>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 395909 [details].
Comment 12 Radar WebKit Bug Importer 2020-04-08 22:11:14 PDT
<rdar://problem/61496849>