Skip to content

Fix #10758 Add compiler option to parse in strict mode #11473

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 3 commits into from
Oct 14, 2016

Conversation

slawomir
Copy link
Contributor

@slawomir slawomir commented Oct 9, 2016

Fixes #10758

  • add compiler option alwaysStrict
  • compile in strict mode when option is set
  • emit "use strict"

 * add compiler option alwaysStrict
 * compile in strict mode when option is set
 * emit "use strict"
@msftclas
Copy link

msftclas commented Oct 9, 2016

Hi @slawomir, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!


It looks like you're a Microsoft contributor (Slawomir Sadziak). If you're full-time, we DON'T require a Contribution License Agreement. If you are a vendor, please DO sign the electronic Contribution License Agreement. It will take 2 minutes and there's no faxing! https://cla.microsoft.com.

TTYL, MSBOT;

}

//// [alwaysStrictNoImplicitUseStrict.js]
"use strict";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

alwaysStrict overrides noImplicitUseStrict

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 add a check in program.ts::verifyCompilerOptions to flag teh use of --noImplicitUseStrict and --alwaysStrict as invalid combination.. so somthing like

if (options.noImplicitUseStrict && options.alwaysStrict) {
    programDiagnostics.add(createCompilerDiagnostic(Diagnostics.Option_0_cannot_be_specified_with_option_1, "noImplicitUseStrict ", "alwaysStrict"));
}

 * add unit test to ensure "use strict" is not added twice
 * fix code
Copy link
Member

@RyanCavanaugh RyanCavanaugh left a comment

Choose a reason for hiding this comment

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

Looking good overall, just a few minor changes.

let foundUseStrict = false;
let statementOffset = 0;
const numStatements = node.statements.length;
while (statementOffset < numStatements) {
Copy link
Member

Choose a reason for hiding this comment

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

Why isn't this a for or for / of loop?

Copy link
Member

Choose a reason for hiding this comment

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

Seems like you were planning to use statementOffset later and then didn't

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's now updated in the next commit

@@ -436,6 +436,11 @@ namespace ts {
function visitSourceFile(node: SourceFile) {
currentSourceFile = node;

// ensure "use strict"" is emitted in all scenarios in alwaysStrict mode
Copy link
Member

Choose a reason for hiding this comment

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

nit: extra quote

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@slawomir
Copy link
Contributor Author

@RyanCavanaugh thanks for analysing. I will send next iteration.

@mhegazy
Copy link
Contributor

mhegazy commented Oct 13, 2016

Looks good. one comment for flagging invalid combinations.

@mhegazy
Copy link
Contributor

mhegazy commented Oct 13, 2016

also can you add a test for --outFile concatenating multiple files.

@mhegazy mhegazy merged commit 8210634 into microsoft:master Oct 14, 2016
@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
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.

4 participants