Add strict mode flag to Context

[raskad]

This Pull Request fixes/closes #1504.

It changes the following:

• Add strict mode flag to Context
• Add strict mode detection to StatementList parsing
• Add strict mode handling of delete

[raskad]

Test262 conformance changes:

Test result	master count	PR count	difference
Total	80,685	80,685	0
Passed	32,838	32,845	+7
Ignored	15,818	15,818	0
Failed	32,029	32,022	-7
Panics	0	0	0
Conformance	40.70%	40.71%	+0.01%

Fixed tests (7):

test/built-ins/Boolean/prototype/S15.6.3.1_A3.js [strict mode] (previously Failed)
test/language/expressions/delete/11.4.1-4-a-1-s.js [strict mode] (previously Failed)
test/language/expressions/delete/11.4.1-4.a-8-s.js [strict mode] (previously Failed)
test/language/expressions/delete/11.4.4-4.a-3-s.js [strict mode] (previously Failed)
test/language/expressions/delete/11.4.1-4.a-9-s.js [strict mode] (previously Failed)
test/language/expressions/delete/11.4.1-4.a-3-s.js [strict mode] (previously Failed)
test/language/expressions/delete/11.4.1-4-a-2-s.js [strict mode] (previously Failed)

[jedel1043]

Great work!
Could you add a test to check if functions are strict in global strict contexts? E.g.

'use strict'
function a(){
    delete Object.prototype;
}
a();

should throw

[jedel1043]

I think we should lift strict from StatementList to Function. Even though 'use strict' is inside the function, it throws a syntax error if we define a function with duplicate parameters, and seeing we would need to access that field on parsing, it would be better if parsing returned a strict bool and a StatementList instead of returning a StatementList with a strict field. Also, the spec specifies strict should be on Function objects, so that's another point to take into account.

[Razican]

I think we should lift strict from StatementList to Function. Even though 'use strict' is inside the function, it throws a syntax error if we define a function with duplicate parameters, and seeing we would need to access that field on parsing, it would be better if parsing returned a strict bool and a StatementList instead of returning a StatementList with a strict field. Also, the spec specifies strict should be on Function objects, so that's another point to take into account.

I see why this would be desirable, but the probably better situation would be to have the context as part of the parser, and for the parser to return a JsResult. But maybe this is a too big of a change.

[raskad]

I think we should lift strict from StatementList to Function. Even though 'use strict' is inside the function, it throws a syntax error if we define a function with duplicate parameters, and seeing we would need to access that field on parsing, it would be better if parsing returned a strict bool and a StatementList instead of returning a StatementList with a strict field. Also, the spec specifies strict should be on Function objects, so that's another point to take into account.

Lifting strict to Function would imply passing it trough all of the different function declarations in the AST (e.g. arrow, async, normal...), adding it to Function::Ordinary and handling it in call_construct right? I tried that, but that seems to be a lot of overhead, no? Is there a benefit of having the strict field on Function::Ordinary instead on having it on the StatementList it contains? And we would still need the strict field on the StatementList anyway for the global strict mode (I guess we could lift that somewhere else too).
I think we can do the strict handling concerning the duplicate function parameters without any changes. We should be able to just check the parameters in the parser after parsing the function body.

I see why this would be desirable, but the probably better situation would be to have the context as part of the parser, and for the parser to return a JsResult. But maybe this is a too big of a change.

My first instinct would be not to include the context is the parser, because the context "belongs" the the execution phase. I think the strict on the context should carry different meaning than the strict on the Cursor. But that is only my impression.

[Razican]

My first instinct would be not to include the context is the parser, because the context "belongs" the the execution phase. I think the strict on the context should carry different meaning than the strict on the Cursor. But that is only my impression.

Let's try it like this, we can always change it later if needed.

[jedel1043]

I think we should lift strict from StatementList to Function. Even though 'use strict' is inside the function, it throws a syntax error if we define a function with duplicate parameters, and seeing we would need to access that field on parsing, it would be better if parsing returned a strict bool and a StatementList instead of returning a StatementList with a strict field. Also, the spec specifies strict should be on Function objects, so that's another point to take into account.

Lifting strict to Function would imply passing it trough all of the different function declarations in the AST (e.g. arrow, async, normal...), adding it to Function::Ordinary and handling it in call_construct right? I tried that, but that seems to be a lot of overhead, no? Is there a benefit of having the strict field on Function::Ordinary instead on having it on the StatementList it contains? And we would still need the strict field on the StatementList anyway for the global strict mode (I guess we could lift that somewhere else too).
I think we can do the strict handling concerning the duplicate function parameters without any changes. We should be able to just check the parameters in the parser after parsing the function body.

It's just to follow the spec as closely as possible; maybe there are some corner cases we're not seeing right now. But let's just leave it like this and refactor later if we find some strange corner case that we didn't consider.

Also, can you add a test for a strict function calling a non-strict function? Like

function a(){
    delete Object.prototype;
}

function b(){
    'use strict';
    a();
}

b();

shouldn't throw

Sorry if I'm being a bit pedantic, but we should try to test all posible interactions between non-strict and strict code to ensure nothing will break in the future :)

[raskad]

Sorry if I'm being a bit pedantic, but we should try to test all posible interactions between non-strict and strict code to ensure nothing will break in the future :)

No worries :)
That test actually failed with the previous version, so really good that you found that. I had to implement some further logic to account for that.

[jedel1043]

I think we covered the most important tests. We can make more specific tests while we implement more features dependant on strict. Good work!

[raskad]

Rebased

[jedel1043]

Oops, found another one.

function strict(){
    'use strict';

    return function() {
        delete Object.prototype;
    }
}

strict()();

Should throw but it apparently doesn't.

Ah, now I get why the spec saves the strictness of a function on the function. It's to account for this exact case.

[raskad]

Ah, now I get why the spec saves the strictness of a function on the function. It's to account for this exact case.

Got the test to work without lifting. If you look at the change in my last commit, do you see any way how that could lead to an unexpected strict marking? I thought about calling functions from different files, but all module code is strict code, so that should not be an issue right?

[jedel1043]

Ah, now I get why the spec saves the strictness of a function on the function. It's to account for this exact case.

Got the test to work without lifting. If you look at the change in my last commit, do you see any way how that could lead to an unexpected strict marking? I thought about calling functions from different files, but all module code is strict code, so that should not be an issue right?

Hm... I don't know how we would check on parsing that the function has duplicate argument names if the strict check occurs on the ast execution. Is there a way to make the check from the parser?

Edit: Maybe using cursor.strict_mode() we could pass the strict state to check the arguments? But now we're checking two times the strictness of a function. Or maybe set the strictness of a function expression's StatementList depending on whether cursor.strict_mode() is true?

[raskad]

Hm... I don't know how we would check on parsing that the function has duplicate argument names if the strict check occurs on the ast execution. Is there a way to make the check from the parser?

Edit: Maybe using cursor.strict_mode() we could pass the strict state to check the arguments? But now we're checking two times the strictness of a function. Or maybe set the strictness of a function expression's StatementList depending on whether cursor.strict_mode() is true?

Yep with cursor.strict_mode() and body.strict() in the parser of the function. We have some more early errors for the parameters that we do not check at the moment. I think there is some work needed to provide some helper methods on [FormalParameter] for those errors. See this commit for an idea how this could work (not complete or good, but works for the duplicate check): raskad@b133f55

[jedel1043]

Then I don't see any other problems with the implementation :)
I already approved it, but let's wait for another review to merge it.

[Razican]

We seem we are already taking into account strict parsing, but it's always a global strict. Maybe we should change how that strict parsing works, but it could be part of a new PR

[raskad]

Yes, I will look into that. I currently have one PR with strict mode operations on environments in the pipeline and am working on some strict mode errors that should be thrown in the parser. While working in generator parsing, I noticed we still have to do some work on early errors.

[Razican]

I think this is a great improvement overall, but as we talked, strict mode needs work. Let's merge it as is, and more changes will come in the future :)