Bug 155545 - [ES6] Class syntax. Access to new.target inside of the eval should not lead to SyntaxError
Summary: [ES6] Class syntax. Access to new.target inside of the eval should not lead t...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 140491
  Show dependency treegraph
 
Reported: 2016-03-16 10:08 PDT by GSkachkov
Modified: 2016-04-03 00:59 PDT (History)
7 users (show)

See Also:


Attachments
Patch (42.78 KB, patch)
2016-03-30 12:11 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (42.75 KB, patch)
2016-03-30 12:27 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (42.96 KB, patch)
2016-03-30 12:50 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (43.68 KB, patch)
2016-03-31 12:44 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews114 for mac-yosemite (986.57 KB, application/zip)
2016-03-31 13:41 PDT, Build Bot
no flags Details
Patch (44.49 KB, patch)
2016-04-01 09:56 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (44.16 KB, patch)
2016-04-02 10:13 PDT, GSkachkov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description GSkachkov 2016-03-16 10:08:23 PDT
In current version of ES6 class we can't access to the new.target in eval:

class A { 
    constructor() { 
        eval('debug(new.target)');
    }
}

new A();//SyntaxError: new.target is only valid inside functions.
Comment 1 GSkachkov 2016-03-30 12:11:17 PDT
Created attachment 275208 [details]
Patch
Comment 2 GSkachkov 2016-03-30 12:27:10 PDT
Created attachment 275209 [details]
Patch
Comment 3 GSkachkov 2016-03-30 12:50:25 PDT
Created attachment 275212 [details]
Patch
Comment 4 Saam Barati 2016-03-30 14:49:36 PDT
Comment on attachment 275212 [details]
Patch

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

> Source/JavaScriptCore/runtime/CodeCache.cpp:86
> +UnlinkedCodeBlockType* CodeCache::getGlobalCodeBlock(VM& vm, ExecutableType* executable, const SourceCode& source, JSParserBuiltinMode builtinMode, JSParserStrictMode strictMode, ThisTDZMode thisTDZMode, bool, DebuggerMode debuggerMode, ProfilerMode profilerMode, ParserError& error, bool isParantProgramParseMode, const VariableEnvironment* variablesUnderTDZ)

Is this truly just the parent? Or is this parent or self is program?
i.e, should a Program node be true or false for this?

> Source/JavaScriptCore/runtime/CodeCache.cpp:147
> +    return getGlobalCodeBlock<UnlinkedModuleProgramCodeBlock>(vm, executable, source, builtinMode, JSParserStrictMode::Strict, ThisTDZMode::CheckIfNeeded, false, debuggerMode, profilerMode, error, false, &emptyParentTDZVariables);

It doesn't make sense that this is different from UnlinkedProgramCodeBlock
Comment 5 GSkachkov 2016-03-31 12:44:09 PDT
Created attachment 275311 [details]
Patch
Comment 6 Build Bot 2016-03-31 13:41:34 PDT
Comment on attachment 275311 [details]
Patch

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

Number of test failures exceeded the failure limit.
Comment 7 Build Bot 2016-03-31 13:41:37 PDT
Created attachment 275321 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 8 GSkachkov 2016-04-01 09:56:48 PDT
Created attachment 275411 [details]
Patch
Comment 9 GSkachkov 2016-04-01 10:39:36 PDT
Comment on attachment 275212 [details]
Patch

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

>> Source/JavaScriptCore/runtime/CodeCache.cpp:86
>> +UnlinkedCodeBlockType* CodeCache::getGlobalCodeBlock(VM& vm, ExecutableType* executable, const SourceCode& source, JSParserBuiltinMode builtinMode, JSParserStrictMode strictMode, ThisTDZMode thisTDZMode, bool, DebuggerMode debuggerMode, ProfilerMode profilerMode, ParserError& error, bool isParantProgramParseMode, const VariableEnvironment* variablesUnderTDZ)
> 
> Is this truly just the parent? Or is this parent or self is program?
> i.e, should a Program node be true or false for this?

In general for Program node it should be does not matter. I've made refactoring in  next patch, so it reflect that this value is matter only in eval context.

>> Source/JavaScriptCore/runtime/CodeCache.cpp:147
>> +    return getGlobalCodeBlock<UnlinkedModuleProgramCodeBlock>(vm, executable, source, builtinMode, JSParserStrictMode::Strict, ThisTDZMode::CheckIfNeeded, false, debuggerMode, profilerMode, error, false, &emptyParentTDZVariables);
> 
> It doesn't make sense that this is different from UnlinkedProgramCodeBlock

Fixed
Comment 10 Saam Barati 2016-04-01 17:30:32 PDT
Comment on attachment 275411 [details]
Patch

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

> Source/JavaScriptCore/bytecode/ExecutableInfo.h:34
> +enum class EvalContextType    : uint8_t { None, ProgramEvalContextType, FunctionEvalContextType };

It seems like we don't need ProgramEvalContextType. Wy not just use "None" and "FunctionEvalContext"?

Style: Also, these name don't need to end with "Type". It can just be "FunctionEvalContext" since this is an "enum class"

> Source/JavaScriptCore/parser/Parser.cpp:3866
> +                semanticFailIfFalse(currentScope()->isFunction() || (closestParentOrdinaryFunctionNonLexicalScope()->isEvalContext() && closestParentOrdinaryFunctionNonLexicalScope()->evalContextType() == EvalContextType::FunctionEvalContextType), "new.target is only valid inside functions");

Do we really need to check both "isEvalContext() and evalContextType() == ..."?
It seems based on your assertion in the constructor it's sufficient to just check that "evalContextType() == FunctionEvalContext"

> Source/JavaScriptCore/parser/Parser.h:732
> +    unsigned m_evalContextType;

This should have type EvalContextType, not unsigned.
(The other fields that  are unsigned and do similar things with casting should also have their respective types instead of unsigned. We can fix that in a separate patch. I introduced this ugliness in.)
Comment 11 GSkachkov 2016-04-02 10:13:10 PDT
Created attachment 275472 [details]
Patch
Comment 12 WebKit Commit Bot 2016-04-03 00:59:16 PDT
Comment on attachment 275472 [details]
Patch

Clearing flags on attachment: 275472

Committed r198980: <http://trac.webkit.org/changeset/198980>
Comment 13 WebKit Commit Bot 2016-04-03 00:59:20 PDT
All reviewed patches have been landed.  Closing bug.