Bug 146979 – Implement catch scope using lexical scoping constructs introduced with "let" scoping patch

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

[Saam Barati]

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.

[Saam Barati]

(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.

[Saam Barati]

Created attachment 257516 [details]
patch

Still need to write a change log.

[WebKit Commit Bot]

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.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Saam Barati]

Looks like I should claim that the symbol table corresponds to a lexical scope. 
Otherwise, we will unsoundly cache evals in a catch scope.

[Saam Barati]

Created attachment 257521 [details]
patch

[WebKit Commit Bot]

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.

[Saam Barati]

Created attachment 257599 [details]
patch

[Geoffrey Garen]

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?

[Saam Barati]

(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.

[WebKit Commit Bot]

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.

[Saam Barati]

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