Bug 155545 – [ES6] Class syntax. Access to new.target inside of the eval should not lead to SyntaxError

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

[GSkachkov]

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.

[GSkachkov]

Created attachment 275208 [details]
Patch

[GSkachkov]

Created attachment 275209 [details]
Patch

[GSkachkov]

Created attachment 275212 [details]
Patch

[Saam Barati]

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

[GSkachkov]

Created attachment 275311 [details]
Patch

[Build Bot]

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.

[Build Bot]

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

[GSkachkov]

Created attachment 275411 [details]
Patch

[GSkachkov]

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

[Saam Barati]

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

[GSkachkov]

Created attachment 275472 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 275472 [details]
Patch

Clearing flags on attachment: 275472

Committed r198980: <http://trac.webkit.org/changeset/198980>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.