Bug 163930 – [JSC] forbid lexical redeclaration of generator formal parameters

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Caitlin Potter (:caitp)]

[JSC] forbid lexical redeclaration of generator formal parameters

[Caitlin Potter (:caitp)]

Created attachment 292686 [details]
Patch

[Caitlin Potter (:caitp)]

Created attachment 292687 [details]
Patch

[Caitlin Potter (:caitp)]

couple of test262 failures (for both generators and async functions) fixed here, and some new tests added for previously untested things that were actually working correctly. Good to have coverage :>

[Caitlin Potter (:caitp)]

Created attachment 292963 [details]
Patch

Add comments describing why things are done in isValidStrictMode() and hasDeclaredParameter()

[Yusuke Suzuki]

Comment on attachment 292963 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:8
> +        Also forbids "arguments" and "eval" as generator argument names in strict mode.

Nice!

> Source/JavaScriptCore/parser/Parser.h:1238
>          return m_scopeStack[i].hasDeclaredParameter(ident);

This `hasDeclaredParameter` function is dangerous I think. When performing the syntax checking onto the code, it is OK.
But when actually parsing the generator body function to create the AST, the scope in this line(m_scopeStack[i]) becomes the global scope, right?
So we never call this function during reparsing. It returns the incorrect result.

It is better to rename this function to the other name that represents this function is not valid during the reparsing.

> Source/JavaScriptCore/parser/Parser.h:1403
> +    }

I think this name is also misleading. Can you rename it like, `isValidStatusForStrictMode()`? (And changing m_validStrictMode to the renamed one).

[Caitlin Potter (:caitp)]

Comment on attachment 292963 [details]
Patch

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

>> Source/JavaScriptCore/parser/Parser.h:1238
>>          return m_scopeStack[i].hasDeclaredParameter(ident);
> 
> This `hasDeclaredParameter` function is dangerous I think. When performing the syntax checking onto the code, it is OK.
> But when actually parsing the generator body function to create the AST, the scope in this line(m_scopeStack[i]) becomes the global scope, right?
> So we never call this function during reparsing. It returns the incorrect result.
> 
> It is better to rename this function to the other name that represents this function is not valid during the reparsing.

From local testing, during reparsing, there is no outer scope object (which is why it checks if `i` is non-zero). Otherwise there is an OOB problem.

I could add an assertion that the outer scope is an async function wrapper or generator function wrapper and see if it still passes everything, I guess, but it seems that there is no global scope when reparsing closures.

>> Source/JavaScriptCore/parser/Parser.h:1403
>> +    }
> 
> I think this name is also misleading. Can you rename it like, `isValidStatusForStrictMode()`? (And changing m_validStrictMode to the renamed one).

"isValidStrictMode" seems ok to me, I read it answering the question "does this pass strict mode static restrictions?". Maybe Saam has an opinion?

I'll do it if folks prefer that, but it's sort of outside of the scope of what this patch is meant to fix.

[Caitlin Potter (:caitp)]

Comment on attachment 292963 [details]
Patch

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

>>> Source/JavaScriptCore/parser/Parser.h:1238
>>>          return m_scopeStack[i].hasDeclaredParameter(ident);
>> 
>> This `hasDeclaredParameter` function is dangerous I think. When performing the syntax checking onto the code, it is OK.
>> But when actually parsing the generator body function to create the AST, the scope in this line(m_scopeStack[i]) becomes the global scope, right?
>> So we never call this function during reparsing. It returns the incorrect result.
>> 
>> It is better to rename this function to the other name that represents this function is not valid during the reparsing.
> 
> From local testing, during reparsing, there is no outer scope object (which is why it checks if `i` is non-zero). Otherwise there is an OOB problem.
> 
> I could add an assertion that the outer scope is an async function wrapper or generator function wrapper and see if it still passes everything, I guess, but it seems that there is no global scope when reparsing closures.

Also note, even if it is the global scope during reparsing, it doesn't matter, because `valid-strict-mode-ness` doesn't matter during reparsing (if it's invalid during strict mode, it should be a SyntaxError and never get re-parsed).

[Yusuke Suzuki]

Comment on attachment 292963 [details]
Patch

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

>>>> Source/JavaScriptCore/parser/Parser.h:1238
>>>>          return m_scopeStack[i].hasDeclaredParameter(ident);
>>> 
>>> This `hasDeclaredParameter` function is dangerous I think. When performing the syntax checking onto the code, it is OK.
>>> But when actually parsing the generator body function to create the AST, the scope in this line(m_scopeStack[i]) becomes the global scope, right?
>>> So we never call this function during reparsing. It returns the incorrect result.
>>> 
>>> It is better to rename this function to the other name that represents this function is not valid during the reparsing.
>> 
>> From local testing, during reparsing, there is no outer scope object (which is why it checks if `i` is non-zero). Otherwise there is an OOB problem.
>> 
>> I could add an assertion that the outer scope is an async function wrapper or generator function wrapper and see if it still passes everything, I guess, but it seems that there is no global scope when reparsing closures.
> 
> Also note, even if it is the global scope during reparsing, it doesn't matter, because `valid-strict-mode-ness` doesn't matter during reparsing (if it's invalid during strict mode, it should be a SyntaxError and never get re-parsed).

Oops, I was considering about generator wrapper function case.

Yeah, but if you call `hasDeclaredParameter(...)` in the reparsing phase for the other purpose, we have a bad time (this function returns the incorrect answer).

[Caitlin Potter (:caitp)]

Comment on attachment 292963 [details]
Patch

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

>>>>> Source/JavaScriptCore/parser/Parser.h:1238
>>>>>          return m_scopeStack[i].hasDeclaredParameter(ident);
>>>> 
>>>> This `hasDeclaredParameter` function is dangerous I think. When performing the syntax checking onto the code, it is OK.
>>>> But when actually parsing the generator body function to create the AST, the scope in this line(m_scopeStack[i]) becomes the global scope, right?
>>>> So we never call this function during reparsing. It returns the incorrect result.
>>>> 
>>>> It is better to rename this function to the other name that represents this function is not valid during the reparsing.
>>> 
>>> From local testing, during reparsing, there is no outer scope object (which is why it checks if `i` is non-zero). Otherwise there is an OOB problem.
>>> 
>>> I could add an assertion that the outer scope is an async function wrapper or generator function wrapper and see if it still passes everything, I guess, but it seems that there is no global scope when reparsing closures.
>> 
>> Also note, even if it is the global scope during reparsing, it doesn't matter, because `valid-strict-mode-ness` doesn't matter during reparsing (if it's invalid during strict mode, it should be a SyntaxError and never get re-parsed).
> 
> Oops, I was considering about generator wrapper function case.
> 
> Yeah, but if you call `hasDeclaredParameter(...)` in the reparsing phase for the other purpose, we have a bad time (this function returns the incorrect answer).

Would you prefer a `gif (not a wrapper function) return false;` after decrementing i?

[Yusuke Suzuki]

Comment on attachment 292963 [details]
Patch

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

>>>>>> Source/JavaScriptCore/parser/Parser.h:1238
>>>>>>          return m_scopeStack[i].hasDeclaredParameter(ident);
>>>>> 
>>>>> This `hasDeclaredParameter` function is dangerous I think. When performing the syntax checking onto the code, it is OK.
>>>>> But when actually parsing the generator body function to create the AST, the scope in this line(m_scopeStack[i]) becomes the global scope, right?
>>>>> So we never call this function during reparsing. It returns the incorrect result.
>>>>> 
>>>>> It is better to rename this function to the other name that represents this function is not valid during the reparsing.
>>>> 
>>>> From local testing, during reparsing, there is no outer scope object (which is why it checks if `i` is non-zero). Otherwise there is an OOB problem.
>>>> 
>>>> I could add an assertion that the outer scope is an async function wrapper or generator function wrapper and see if it still passes everything, I guess, but it seems that there is no global scope when reparsing closures.
>>> 
>>> Also note, even if it is the global scope during reparsing, it doesn't matter, because `valid-strict-mode-ness` doesn't matter during reparsing (if it's invalid during strict mode, it should be a SyntaxError and never get re-parsed).
>> 
>> Oops, I was considering about generator wrapper function case.
>> 
>> Yeah, but if you call `hasDeclaredParameter(...)` in the reparsing phase for the other purpose, we have a bad time (this function returns the incorrect answer).
> 
> Would you prefer a `gif (not a wrapper function) return false;` after decrementing i?

Since this function (Parser::hasDeclaredParameter) is only used for this error checking purpose currently, I suggest renaming this function to avoid incorrect use and inserting assertion for isReparsing.

[Caitlin Potter (:caitp)]

Created attachment 292982 [details]
Patch

Rename Parser::hasDeclaredParameter() and Parser::hasDeclaredVariable(), don't call Parser::hasDeclaredParameter() when reparsing, and assert that not reparsing when it is called

[Caitlin Potter (:caitp)]

Created attachment 293070 [details]
Patch

Updated per offline discussions

[Yusuke Suzuki]

Comment on attachment 293070 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 293070 [details]
Patch

Clearing flags on attachment: 293070

Committed r208016: <http://trac.webkit.org/changeset/208016>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Saam Barati]

Comment on attachment 293070 [details]
Patch

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

Bugs like these make me think that it's a bit ineligant that our parser so closely understands the implementation details of generator functions. In some ways, it'd be nicer if the parser didn't know that we lower generators to an outer and an inner function. It could just be worried with parsing generators at a higher level, and we could take the parser's output and generate the outer and the inner function at a later stage. I'm not sure if it's worth doing anything like this now, but the current design makes it harder to reason about properly "syntax checking" generator programs.

> Source/JavaScriptCore/parser/Parser.h:1400
> +        int i = m_scopeStack.size() - 1;

Nit: I'd use size_t here and ASSERT that scopeStack is non-empty