Skip to content

Migrate strict mode check #2684

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 24 commits into from
Apr 12, 2015
Merged

Migrate strict mode check #2684

merged 24 commits into from
Apr 12, 2015

Conversation

yuit
Copy link
Contributor

@yuit yuit commented Apr 9, 2015

Move strict mode check into type checker so we can give better error messages. This will be useful particularly when class declaration and ES6 module become strict mode code. One thing worth pointing out is that strict mode is a grammar check which means that if there is any parsing error, strict mode error will not get reported.

// class C {
// foo(x: public){} // Error.
// }
if (node.typeName.kind === SyntaxKind.Identifier && (<Identifier>node.typeName).strictModeKind) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this reads oddly. Perhaps if this was .isKeywordInStrictMode instead?

}

function checkGrammarDeclarationNameInStrictMode(node: Declaration): boolean {
let name = node.name;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't work for modules. For example, if you have:

"use strict";
module public.whatever {
}

(make sure to add a test for this).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also won't work for a variable with a binding pattern. For example:

"use strict";
var { public, private };

@@ -8069,6 +8070,8 @@ module ts {
// DECLARATION AND STATEMENT TYPE CHECKING

function checkTypeParameter(node: TypeParameterDeclaration) {
checkGrammarDeclarationNameInStrictMode(node);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch remembering to check this. Is there a test for this as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep

// GRAMMAR CHECKING
function isReservedwordInStrictMode(node: Identifier): boolean {
// Check that originalStrictModeSyntaxKind is less than LastFurtureReservedWord to see if an Identifier is a strict-mode reserved word
if ((node.parserContextFlags & ParserContextFlags.StrictMode) && node.originalStrictModeSyntaxKind <= SyntaxKind.LastFutureReservedWord) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you simplify this to "return (node.parseContextFlags & ParserContextFlags.StrictMode) !== 0 && ..."

let name = element.name;
if (name.originalStrictModeSyntaxKind) {
let nameText = declarationNameToString(name);
reportError = grammarErrorOnNode(name, Diagnostics.Identifier_expected_0_is_a_reserved_word_in_strict_mode, nameText);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should probably be |=

yuit added a commit that referenced this pull request Apr 12, 2015
@yuit yuit merged commit 96995e7 into master Apr 12, 2015
@yuit yuit deleted the migrateStrictModeCheck branch April 12, 2015 04:59
@Arnavion
Copy link
Contributor

@CyrusNajmabadi @yuit

With this change, import * as set from "set"; no longer compiles - error TS1212: Identifier expected. 'set' is a reserved word in strict mode (because name.originalKeywordKind is set to SetKeyword). Is this intentional? Or should the checks for originalKeywordKind check that it's between First and LastKeyword/ReservedWord/FutureReservedWord before generating the diagnostic?

Such code compiles with 1.5 alpha with no issue.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants