Bug 146979 - Implement catch scope using lexical scoping constructs introduced with "let" scoping patch
Summary: Implement catch scope using lexical scoping constructs introduced with "let" ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Saam Barati
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-07-15 14:28 PDT by Saam Barati
Modified: 2015-07-28 14:41 PDT (History)
12 users (show)

See Also:


Attachments
patch (21.74 KB, patch)
2015-07-25 13:13 PDT, Saam Barati
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-mavericks (569.00 KB, application/zip)
2015-07-25 13:47 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-mavericks-wk2 (618.67 KB, application/zip)
2015-07-25 13:53 PDT, Build Bot
no flags Details
patch (21.59 KB, patch)
2015-07-25 16:18 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
patch (59.31 KB, patch)
2015-07-27 15:17 PDT, Saam Barati
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Saam Barati 2015-07-15 14:28:21 PDT
Specifically, catch statement shouldn't cause all variables to be captured. We should only
capture those used inside the catch block.

Variable resolution in BytecodeGenerator should resolve variables where resolution passes
through a catch scope.
Comment 1 Saam Barati 2015-07-24 19:53:13 PDT
(In reply to comment #0)
> Specifically, catch statement shouldn't cause all variables to be captured.
> We should only
> capture those used inside the catch block.
This is wrong. We should just use normal capture rules
and treat a catch block like any other lexical scope.
Comment 2 Saam Barati 2015-07-25 13:13:16 PDT
Created attachment 257516 [details]
patch

Still need to write a change log.
Comment 3 WebKit Commit Bot 2015-07-25 13:15:27 PDT
Attachment 257516 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:585:  The parameter name "environment" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Build Bot 2015-07-25 13:47:21 PDT
Comment on attachment 257516 [details]
patch

Attachment 257516 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5313643080056832

New failing tests:
js/dom/eval-cache-scoped-lookup.html
Comment 5 Build Bot 2015-07-25 13:47:25 PDT
Created attachment 257517 [details]
Archive of layout-test-results from ews103 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-mavericks  Platform: Mac OS X 10.9.5
Comment 6 Build Bot 2015-07-25 13:53:11 PDT
Comment on attachment 257516 [details]
patch

Attachment 257516 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4607432344993792

New failing tests:
js/dom/eval-cache-scoped-lookup.html
Comment 7 Build Bot 2015-07-25 13:53:14 PDT
Created attachment 257518 [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 8 Saam Barati 2015-07-25 15:08:18 PDT
Looks like I should claim that the symbol table corresponds to a lexical scope. 
Otherwise, we will unsoundly cache evals in a catch scope.
Comment 9 Saam Barati 2015-07-25 16:18:52 PDT
Created attachment 257521 [details]
patch
Comment 10 WebKit Commit Bot 2015-07-25 16:22:02 PDT
Attachment 257521 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:585:  The parameter name "environment" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 11 Saam Barati 2015-07-27 15:17:39 PDT
Created attachment 257599 [details]
patch
Comment 12 Geoffrey Garen 2015-07-27 15:29:17 PDT
Comment on attachment 257599 [details]
patch

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

r=me

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1437
> +        for (auto entry : environment) {

auto&

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3030
> +    if (m_shouldEmitDebugHooks)
> +        environment.markAllVariablesAsCaptured();

Can we markAllVariablesAsCaptured when constructing the VariableEnvironment instead? Then we couldn't forget to do it.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:1894
> +        RegisterID* result = generator.emitNode(dst, m_right); // Execute side effects first.
>          bool threwException = generator.emitReadOnlyExceptionIfNeeded(var);
>          if (threwException)
> -            return generator.emitNode(dst, m_right);
> +            return result;

Does this have a test? Good thing to test.

> Source/JavaScriptCore/runtime/SymbolTable.h:655
> +    ALWAYS_INLINE bool correspondsToVarScope() const { return m_scopeType == VarScope; }
> +    ALWAYS_INLINE bool correspondsToLexicalScope() const { return m_scopeType == LexicalScope; }
> +    ALWAYS_INLINE bool correspondsToCatchScope() const { return m_scopeType == CatchScope; }
> +    void setDoesCorrespondToLexicalScope() { m_scopeType = LexicalScope; }
> +    void setDoesCorrespondToCatchScope() { m_scopeType = CatchScope; }

Can this just be a ScopeType accessor, rather than one method for each kind of ScopeType?
Comment 13 Saam Barati 2015-07-27 15:41:48 PDT
(In reply to comment #12)
> Comment on attachment 257599 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=257599&action=review
> 
> r=me
> 
> > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1437
> > +        for (auto entry : environment) {
> 
> auto&
> 
> > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3030
> > +    if (m_shouldEmitDebugHooks)
> > +        environment.markAllVariablesAsCaptured();
> 
> Can we markAllVariablesAsCaptured when constructing the VariableEnvironment
> instead? Then we couldn't forget to do it.
This line of code is actually not needed. pushLexicalScopeInternal
has this same line of code. That said, I think the byte code generator
is the best place to handle "mark everything as captured". The other
option would be in the parser, which feels wrong.

> 
> > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:1894
> > +        RegisterID* result = generator.emitNode(dst, m_right); // Execute side effects first.
> >          bool threwException = generator.emitReadOnlyExceptionIfNeeded(var);
> >          if (threwException)
> > -            return generator.emitNode(dst, m_right);
> > +            return result;
> 
> Does this have a test? Good thing to test.
Yeah, we do have tests. I actually broke a test in this patch.
I needed to change this line because the test, before, relied
on us throwing an error when assigning dynamically. Now, because
we resolve try/catch statically, we converted this to throw static
error in byte code. This change revealed this bug (from a previously
written test).

> 
> > Source/JavaScriptCore/runtime/SymbolTable.h:655
> > +    ALWAYS_INLINE bool correspondsToVarScope() const { return m_scopeType == VarScope; }
> > +    ALWAYS_INLINE bool correspondsToLexicalScope() const { return m_scopeType == LexicalScope; }
> > +    ALWAYS_INLINE bool correspondsToCatchScope() const { return m_scopeType == CatchScope; }
> > +    void setDoesCorrespondToLexicalScope() { m_scopeType = LexicalScope; }
> > +    void setDoesCorrespondToCatchScope() { m_scopeType = CatchScope; }
> 
> Can this just be a ScopeType accessor, rather than one method for each kind
> of ScopeType?
Sure.
Comment 14 WebKit Commit Bot 2015-07-27 15:44:56 PDT
Attachment 257599 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:585:  The parameter name "environment" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 15 Saam Barati 2015-07-28 14:41:27 PDT
landed in:
http://trac.webkit.org/changeset/187515