Bug 147813 – Block scoped variables should be visible across scripts

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

[Saam Barati]

I've previously thought that "let"/"const" variables declared
at the top level of a script are only visible within that script.
After closer inspection of the spec, I've found this to be wrong. 
They should be visible across all scripts in a global lexical environment.
I need to fix our implementation to account for this.

[Saam Barati]

Created attachment 260201 [details]
WIP

I think this is about 98% of the implementation.
Still need to dot my i's and cross my t's and write tests.

[Saam Barati]

Created attachment 260229 [details]
WIP

just about done. I need to write tests still, and solve one more problem:

<script>
    let b = false;
    function foo() {
        if (b) return x;
    }
    foo(); // causes "x" to get linked as GlobalProperty.
</script>
<script>
     // foo();
     // You can also consider the problem here, if we call "foo" before instantiating "x", how do we implement the TDZ.
     let x = 50;
     foo();  // Throws because "x" isn't a global property!
</script>

I don't have a great solution for this yet.

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/bytecode/EvalCodeCache.h:36:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/runtime/JSGlobalLexicalEnvironment.h:52:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSGlobalLexicalEnvironment.h:52:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSGlobalLexicalEnvironment.h:52:  The parameter name "propertyName" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSGlobalLexicalEnvironment.h:52:  The parameter name "slot" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSGlobalLexicalEnvironment.h:53:  The parameter name "cell" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSGlobalLexicalEnvironment.h:53:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSGlobalLexicalEnvironment.h:53:  The parameter name "propertyName" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSGlobalLexicalEnvironment.h:53:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSGlobalLexicalEnvironment.h:53:  The parameter name "slot" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/Executable.cpp:493:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/runtime/Executable.cpp:498:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:75:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/jit/JITPropertyAccess.cpp:882:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/jit/JITPropertyAccess.cpp:883:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 15 in 38 files


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

[Saam Barati]

Created attachment 260388 [details]
WIP

This is almost done.
Still need to write 32-bit portion, still need to write tests.

[Saam Barati]

Created attachment 260403 [details]
patch

some tests.
Still no 32-bit.

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/runtime/JSGlobalLexicalEnvironment.h:52:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSGlobalLexicalEnvironment.h:52:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSGlobalLexicalEnvironment.h:52:  The parameter name "propertyName" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSGlobalLexicalEnvironment.h:52:  The parameter name "slot" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSGlobalLexicalEnvironment.h:53:  The parameter name "cell" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSGlobalLexicalEnvironment.h:53:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSGlobalLexicalEnvironment.h:53:  The parameter name "propertyName" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSGlobalLexicalEnvironment.h:53:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSGlobalLexicalEnvironment.h:53:  The parameter name "slot" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:667:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:674:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:675:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:676:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:677:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:678:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:681:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:682:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:683:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:684:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:686:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/runtime/Executable.cpp:555:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/runtime/Executable.cpp:560:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/CMakeLists.txt:529:  Alphabetical sorting problem. "runtime/JSGlobalLexicalEnvironment.cpp" should be before "runtime/JSGlobalObject.cpp".  [list/order] [5]
ERROR: Source/JavaScriptCore/llint/LLIntData.cpp:156:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:75:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:2366:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/jit/JITPropertyAccess.cpp:37:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/jit/JITPropertyAccess.cpp:928:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/jit/JITPropertyAccess.cpp:929:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/runtime/GetPutInfo.h:66:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 30 in 63 files


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

[Build Bot]

Comment on attachment 260403 [details]
patch

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

New failing tests:
js/dom/const.html

[Build Bot]

Created attachment 260404 [details]
Archive of layout-test-results from ews104 for mac-mavericks-wk2

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

[Build Bot]

Comment on attachment 260403 [details]
patch

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

New failing tests:
js/dom/const.html

[Build Bot]

Created attachment 260405 [details]
Archive of layout-test-results from ews101 for mac-mavericks

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

[Saam Barati]

Created attachment 260429 [details]
patch

still needs a changelog,

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/llint/LLIntData.cpp:156:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/jit/JITPropertyAccess.cpp:900:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/jit/JITPropertyAccess.cpp:901:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/runtime/GetPutInfo.h:66:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 4 in 80 files


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

[Build Bot]

Comment on attachment 260429 [details]
patch

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

New failing tests:
js/dom/const.html

[Build Bot]

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

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

[Build Bot]

Comment on attachment 260429 [details]
patch

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

New failing tests:
js/dom/const.html

[Build Bot]

Created attachment 260435 [details]
Archive of layout-test-results from ews101 for mac-mavericks

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

[Saam Barati]

Created attachment 260438 [details]
patch

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/llint/LLIntData.cpp:156:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/jit/JITPropertyAccess.cpp:900:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/jit/JITPropertyAccess.cpp:901:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/runtime/GetPutInfo.h:66:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 4 in 82 files


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

[Saam Barati]

Created attachment 260440 [details]
patch

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/llint/LLIntData.cpp:156:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/jit/JITPropertyAccess.cpp:900:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/jit/JITPropertyAccess.cpp:901:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/runtime/GetPutInfo.h:66:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 4 in 81 files


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

[Saam Barati]

Created attachment 260447 [details]
patch

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/llint/LLIntData.cpp:156:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/jit/JITPropertyAccess.cpp:900:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/jit/JITPropertyAccess.cpp:901:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/runtime/GetPutInfo.h:66:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 4 in 81 files


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

[Filip Pizlo]

Comment on attachment 260440 [details]
patch

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

r=me with comment

> Source/JavaScriptCore/dfg/DFGAbstractValue.h:78
> +
> +    void makeHeapTopOrEmpty()
> +    {
> +        makeTop(SpecHeapTop | SpecEmpty);
> +    }

Hmmm, I think that this is just called BytecodeTop.  We should clarify in SpeculatedType.h that "bytecode top" just means "heap top plus empty".  And you should remove this method.

[Saam Barati]

(In reply to comment #23)
> Comment on attachment 260440 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=260440&action=review
> 
> r=me with comment
> 
> > Source/JavaScriptCore/dfg/DFGAbstractValue.h:78
> > +
> > +    void makeHeapTopOrEmpty()
> > +    {
> > +        makeTop(SpecHeapTop | SpecEmpty);
> > +    }
> 
> Hmmm, I think that this is just called BytecodeTop.  We should clarify in
> SpeculatedType.h that "bytecode top" just means "heap top plus empty".  And
> you should remove this method.

Okay. Thanks for the review. I've added a comment.

[Saam Barati]

landed in:
http://trac.webkit.org/changeset/189279

[Filip Pizlo]

This regressed SunSpider by close to 1%.