Bug 147813

Summary: Block scoped variables should be visible across scripts
Product: WebKit Reporter: Saam Barati <saam>
Component: JavaScriptCoreAssignee: Saam Barati <saam>
Status: RESOLVED FIXED    
Severity: Normal CC: basile_clement, benjamin, buildbot, commit-queue, fpizlo, ggaren, keith_miller, mark.lam, mmirman, msaboff, oliver, rniwa, ysuzuki
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=202044
Attachments:
Description Flags
WIP
none
WIP
none
WIP
none
patch
buildbot: commit-queue-
Archive of layout-test-results from ews104 for mac-mavericks-wk2
none
Archive of layout-test-results from ews101 for mac-mavericks
none
patch
buildbot: commit-queue-
Archive of layout-test-results from ews106 for mac-mavericks-wk2
none
Archive of layout-test-results from ews101 for mac-mavericks
none
patch
none
patch
fpizlo: review+
patch fpizlo: review+

Description Saam Barati 2015-08-08 15:24:02 PDT 
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.
Comment 1 Saam Barati 2015-08-28 18:59:52 PDT 
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.
Comment 2 Saam Barati 2015-08-29 17:45:25 PDT 
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.
Comment 3 WebKit Commit Bot 2015-08-29 17:50:30 PDT 
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.
Comment 4 Saam Barati 2015-09-01 15:29:22 PDT 
Created attachment 260388 [details]
WIP

This is almost done.
Still need to write 32-bit portion, still need to write tests.
Comment 5 Saam Barati 2015-09-01 20:48:40 PDT 
Created attachment 260403 [details]
patch

some tests.
Still no 32-bit.
Comment 6 WebKit Commit Bot 2015-09-01 20:50:21 PDT 
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.
Comment 7 Build Bot 2015-09-01 21:38:21 PDT 
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
Comment 8 Build Bot 2015-09-01 21:38:24 PDT 
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
Comment 9 Build Bot 2015-09-01 21:57:09 PDT 
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
Comment 10 Build Bot 2015-09-01 21:57:13 PDT 
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
Comment 11 Saam Barati 2015-09-02 13:00:43 PDT 
Created attachment 260429 [details]
patch

still needs a changelog,
Comment 12 WebKit Commit Bot 2015-09-02 13:04:23 PDT 
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.
Comment 13 Build Bot 2015-09-02 13:50:29 PDT 
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
Comment 14 Build Bot 2015-09-02 13:50:32 PDT 
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
Comment 15 Build Bot 2015-09-02 13:59:46 PDT 
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
Comment 16 Build Bot 2015-09-02 13:59:50 PDT 
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
Comment 17 Saam Barati 2015-09-02 14:21:58 PDT 
Created attachment 260438 [details]
patch
Comment 18 WebKit Commit Bot 2015-09-02 14:23:47 PDT 
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.
Comment 19 Saam Barati 2015-09-02 14:29:36 PDT 
Created attachment 260440 [details]
patch
Comment 20 WebKit Commit Bot 2015-09-02 14:31:41 PDT 
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.
Comment 21 Saam Barati 2015-09-02 15:20:55 PDT 
Created attachment 260447 [details]
patch
Comment 22 WebKit Commit Bot 2015-09-02 15:23:33 PDT 
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.
Comment 23 Filip Pizlo 2015-09-02 19:05:29 PDT 
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.
Comment 24 Saam Barati 2015-09-02 22:24:11 PDT 
(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.
Comment 25 Saam Barati 2015-09-03 12:46:21 PDT 
landed in:
http://trac.webkit.org/changeset/189279
Comment 26 Filip Pizlo 2015-10-04 17:06:45 PDT 
This regressed SunSpider by close to 1%.