add use strict and simple parameter check#25633
Conversation
| } | ||
|
|
||
| function checkGrammarForUseStrictSimpleParameterList (node: FunctionLikeDeclaration): boolean { | ||
| const useStrictDirective = node.body && isBlock(node.body) && startsWithUseStrict(node.body.statements) ? first(node.body.statements) : undefined; |
There was a problem hiding this comment.
startsWithUseStrict is actually not correct. It doesn't handle the case where 'use strict' is not the first directive in the prologue:
function foo(param = 1) {
'foo';
'use strict'; // this is also a syntax error
}Also note that startsWithUseStrict is also not correct about what it considers a Use Strict Directive:
'use\u0020strict'; is not a valid Use Strict Directive, but the function thinks it is.
Though that is probably not your responsibility to fix it in this PR.
| } | ||
|
|
||
| function checkGrammarForUseStrictSimpleParameterList (node: FunctionLikeDeclaration): boolean { | ||
| const useStrictDirective = node.body && isBlock(node.body) && startsWithUseStrict(node.body.statements) ? first(node.body.statements) : undefined; |
There was a problem hiding this comment.
Is it necessary to only do this check if the target is ES2016 or higher? These early errors were introduced in ES2016 and are valid in ES2015: https://www.ecma-international.org/ecma-262/6.0/index.html
|
@weswigham can you please review? |
| if (parameters.length === 0) return []; | ||
|
|
||
| const last = lastOrUndefined(parameters); | ||
| if (last && isRestParameter(last)) return [last]; |
There was a problem hiding this comment.
The presence of a rest parameter likely shouldn't preclude other parameters from being non-simple. Since an error is issued (well, marked with related spans) on each non-simple parameter, I think the restiness check aughta get moved into the filter below (this way fixing one error on the rest doesn't just move the error to the initialized parameter before it, for example).
A test of this would be good, too:
function foo(a = 3, ... b) {
"use strict";
}| "category": "Error", | ||
| "code": 1345 | ||
| }, | ||
| "{0} is here.": { |
There was a problem hiding this comment.
I imagine this is a localization hazard. Parameter declared here. and "use strict" directive used here. should be written in full, instead of a "generic" message, so "parameter" and "use strict" directive get localized.
| "category": "Error", | ||
| "code": 1344 | ||
| }, | ||
| "'use strict' directive cannot be used with non simple parameter list.": { |
There was a problem hiding this comment.
nit: non-simple instead of non simple just because non isn't a word.
weswigham
left a comment
There was a problem hiding this comment.
This seems by and large great - just some small changes I think we need to localize correctly and issue better errors.
| !!! related TS1348 tests/cases/conformance/functions/functionWithUseStrictAndSimpleParameterList_es2016.ts:20:5: Use strict directive used here. | ||
| ~~~~~~~ | ||
| !!! error TS1345: This parameter is not allowed with 'use strict' directive. | ||
| !!! related TS1348 tests/cases/conformance/functions/functionWithUseStrictAndSimpleParameterList_es2016.ts:20:5: Use strict directive used here. |
There was a problem hiding this comment.
Nit: Message should be 'use strict' directive and not Use strict directive to match the other diagnostic.
|
@weswigham one more round please |
|
Thanks! |
Fixes #6815, #25616