Bug 152361 – Provide a way to distinguish a nested lexical block from a function's lexical block

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Matt Baker]

* SUMMARY
Add a BlockScope to SymbolTable::ScopeType. This will allow the inspector to show more detailed information in the Scope Chain sidebar while debugging.

[Joseph Pecoraro]

We may not want to add a new "BlockType" but just an extra bit that says if a "LexicalScopeType" is an "inner block" or not.

[Joseph Pecoraro]

The inspector would like to be able to distinguish a nested lexical block from the function's lexical block.

For example:

    function foo() {
        var local = 1;
        let bar = 2;
        if (true) {
            let bar = 3;
        }
    }

In this situation there are two LexicalScope's each containing an identifier "bar". The Inspector would like to better display the scope chain. Informing the user of the local variables owned everywhere within "foo" (the first `let bar`) and the lexically scoped variables inside of a nested block (the second `let bar`).

Possible UI, when paused inside the if block:

    Block Variables:
      bar: 3

    Local Variables (foo):
      bar: 2
      local: 1

    Lexical Global Variables:
      ...

    Global Variables:
      ...

[Joseph Pecoraro]

Created attachment 267529 [details]
[PATCH] Proposed Fix

Next steps would be:
1. Eliminating "local" scope from the protocol, as the backend no longer sends it.
2. Better associate CallFrames with the ScopeChain so that the frontend can merge a Function's 1 or 2 base Closure scopes (for `vars` and `lets`) into a single "Local Variables" section. The backend should send enough data to make this possible on the frontend.

[WebKit Commit Bot]

Attachment 267529 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/inspector/JSJavaScriptCallFrame.h:81:  NESTED_LEXICAL_SCOPE is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Joseph Pecoraro]

Created attachment 267530 [details]
[IMAGE] After - Nested Block Scopes different from Function Closure Scopes

[Saam Barati]

Comment on attachment 267529 [details]
[PATCH] Proposed Fix

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

> Source/JavaScriptCore/runtime/SymbolTable.h:692
> +    bool m_nestedLexicalScope : 1; // Non-function LexicalScope.

Nit: this comment is a bit confusing to me. Maybe: "Lexical scope that is not at the base-level of a function"?

[Build Bot]

Comment on attachment 267529 [details]
[PATCH] Proposed Fix

Attachment 267529 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/569931

New failing tests:
inspector/debugger/breakpoint-scope.html

[Build Bot]

Created attachment 267536 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 267529 [details]
[PATCH] Proposed Fix

Attachment 267529 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/569934

New failing tests:
inspector/debugger/breakpoint-scope.html

[Build Bot]

Created attachment 267537 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 267529 [details]
[PATCH] Proposed Fix

Attachment 267529 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/569949

New failing tests:
inspector/debugger/breakpoint-scope.html

[Build Bot]

Created attachment 267538 [details]
Archive of layout-test-results from ews117 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Joseph Pecoraro]

Created attachment 267571 [details]
[PATCH] Proposed Fix

[WebKit Commit Bot]

Attachment 267571 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/inspector/JSJavaScriptCallFrame.h:81:  NESTED_LEXICAL_SCOPE is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Comment on attachment 267571 [details]
[PATCH] Proposed Fix

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

looks good to me, I'll let other people critique

> LayoutTests/ChangeLog:3
> +        Provide a way to distinguish a nested lexical block from a function's lexical block

I like these tests.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3586
> +    pushLexicalScopeInternal(environment, true, false, nullptr, TDZRequirement::NotUnderTDZ, ScopeType::CatchScope, ScopeRegisterType::Block);

We don't want this to be true?

[Joseph Pecoraro]

Comment on attachment 267571 [details]
[PATCH] Proposed Fix

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

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3586
>> +    pushLexicalScopeInternal(environment, true, false, nullptr, TDZRequirement::NotUnderTDZ, ScopeType::CatchScope, ScopeRegisterType::Block);
> 
> We don't want this to be true?

Correct. For now, this CatchScope only covers the single identifier in the "catch (e)" portion. Much like WithBlock only covers the single object in with (...)". The braces following it open a new lexical scope, which will correctly be identifier as a Block scope. We may want to clean this up later but that can be done in the frontend. CatchScope / WithBlock will both always be followed by a BlockScope (I think). I doubt "catch(e);" or "with(e);" are legal JS.

[Saam Barati]

Comment on attachment 267571 [details]
[PATCH] Proposed Fix

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

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3586
>> +    pushLexicalScopeInternal(environment, true, false, nullptr, TDZRequirement::NotUnderTDZ, ScopeType::CatchScope, ScopeRegisterType::Block);
> 
> We don't want this to be true?

Never mind, we already do something better.

[Saam Barati]

(In reply to comment #16)
> Comment on attachment 267571 [details]
> [PATCH] Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=267571&action=review
> 
> >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3586
> >> +    pushLexicalScopeInternal(environment, true, false, nullptr, TDZRequirement::NotUnderTDZ, ScopeType::CatchScope, ScopeRegisterType::Block);
> > 
> > We don't want this to be true?
> 
> Correct. For now, this CatchScope only covers the single identifier in the
> "catch (e)" portion. Much like WithBlock only covers the single object in
> with (...)". The braces following it open a new lexical scope, which will
> correctly be identifier as a Block scope. We may want to clean this up later
> but that can be done in the frontend. CatchScope / WithBlock will both
> always be followed by a BlockScope (I think). I doubt "catch(e);" or
> "with(e);" are legal JS.
It looks like "catch" must be followed by a block and "with" must be followed by any statement.

[Joseph Pecoraro]

<http://trac.webkit.org/changeset/194251>

[Geoffrey Garen]

Comment on attachment 267571 [details]
[PATCH] Proposed Fix

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

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:812
> +        pushLexicalScopeInternal(environment, true, false, nullptr, TDZRequirement::UnderTDZ, ScopeType::LetConstScope, ScopeRegisterType::Block);

It would be more readable to give these booleans names.

[Saam Barati]

(In reply to comment #20)
> Comment on attachment 267571 [details]
> [PATCH] Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=267571&action=review
> 
> > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:812
> > +        pushLexicalScopeInternal(environment, true, false, nullptr, TDZRequirement::UnderTDZ, ScopeType::LetConstScope, ScopeRegisterType::Block);
> 
> It would be more readable to give these booleans names.

I think we should just enumify all the boolean arguments to this function.