Add "use strict" directive by MarkusToe · Pull Request #7411 · babel/babel

MarkusToe · 2018-02-22T09:21:40Z

Q	A
Fixed Issues?	Fixes #7349
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass?	Yes
Documentation PR
Any Dependency Changes?
License	MIT

I added the "use strict" directive in the function body BlockStatement.

xtuc · 2018-02-22T10:33:47Z

Thanks for your pr!

Could you please add a test case where the parent already has the "use strict";?

"use strict";

class Foo {
  method(){

  }
}

should be

"use strict";

var Foo = function () {
  function Foo() {
    _classCallCheck(this, Foo);
  }

  _createClass(Foo, [{
    key: "method",
    value: function method() {}
  }]);

  return Foo;
}();

MarkusToe · 2018-02-23T08:43:37Z

@xtuc I guess I would have to check if the program already has a "use strict"; directive? But I'm not sure where to look for it. I was trying to check this.node.directives around L165 but it is undefined. Do you have any tips for where to check if the program already has a "use strict"; directive?

xtuc · 2018-02-23T09:09:48Z

I think we could do it with path.findParent(node => void), my understanding is that you need to check if one of your parent nodes already has a use strict directive (not only in the top-level scope).

xtuc · 2018-03-01T08:21:02Z

packages/babel-plugin-transform-classes/src/vanilla.js

+        closureParams,
+        t.blockStatement(body, [t.directive(t.directiveLiteral("use strict"))]),
+      );
+    }


These two blocks looks really similar, maybe we could add on a condition for the injection of the directive.

Something like:

const directives = []; if (parentHasUseStrict) { directives push t.directive(t.directiveLiteral("use strict")) } t.blockStatement(body, directives)

xtuc · 2018-03-01T08:23:21Z

...abel-plugin-transform-classes/test/fixtures/loose/program-with-use-strict-directive/input.js

+    method() {
+
+    }
+}


Test showing the case where the directive is already preset.+1:

Could you also please add a test where the directive isn't in the program but in a parent node? like that:

function t() { "use strict"; class Foo { method() {} } }

babel-bot · 2018-03-01T09:52:44Z

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/7543/

babel-bot · 2018-03-01T09:52:44Z

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/7029/

xtuc · 2018-03-01T11:16:27Z

packages/babel-plugin-transform-classes/src/vanilla.js

+
+    if (!parentHasUseStrict) {
+      directives.push(t.directive(t.directiveLiteral("use strict")));
+    }


Nit: we are matching two times the use strict string, we could construct the node t.directiveLiteral("use strict") before. Just an idea.

xtuc

Nice job!

Andarist · 2018-03-01T11:30:55Z

packages/babel-plugin-transform-classes/src/vanilla.js

-      closureParams,
-      t.blockStatement(body, [t.directive(t.directiveLiteral("use strict"))]),
-    );
+    const parent = this.path.findParent((path) => path.isProgram() || path.isBlockStatement());


wont it check only the nearest parent? we can have really deep tree and I guess we should check for something like:

const strictParent = this.path.findParent((path) => { if (!path.isProgram() && !path.isBlockStatement()) { return false } return path.node.directives.some(directive => directive.value.value === 'use strict') });

Yes that absolutely right! Thanks @Andarist

Andarist · 2018-03-01T11:32:47Z

packages/babel-plugin-transform-classes/src/vanilla.js

    //
    body.push(t.returnStatement(t.cloneNode(this.classRef)));

+    const parent = this.path.findParent(


ive left comment regarding this here, it got hidden by github though (i think i might have commented on some older commit)

xtuc

Oops @Andarist is right https://github.com/babel/babel/pull/7411/files/d4b987e7f3615b4b745e4aa759a7931b3a8956ea#r171531768

Andarist · 2018-03-01T14:23:32Z

packages/babel-plugin-transform-classes/src/vanilla.js

    //
    body.push(t.returnStatement(t.cloneNode(this.classRef)));

+    const strictParent = this.path.findParent(path => {


looks better now 👍 would be great to add tests to cover this

also would be great to skip adding those directives in modules, but im not entirely sure how to check that, maybe you can check some property on the program or file? there are things like sourceType which could hint you this info, im not sure though where this info resides

I'm not sure this is actually testable. Having the directive in the program or in a parent function produces the same result.

You can test by writing a random non-simple arguments list (destructuring, defaults) in the IIFE the transform generates and seeing if it compiles.

'use strict' inside a function is too late to disable sloppy mode in the arguments list.

Not sure if we have understood each other - I was asking if there is a possibility to skip adding this for such input

import a from './a' class B { constructor() { this.c = a() } }

Modules are already strict be definition and they do not have to add use strict directive explicitly.

Agreed. We'll want to make sure to avoid adding it to module. It should be safe to do

if (path.isProgram() && path.node.sourceType === "module") { return true; }

to this to cover the module case.

xtuc

lgtm with the new changes, thanks!

Good catch @Andarist

nicolo-ribaudo

You also need to consider classes with only the onstructor (for which the iife isn't created):

// Input
class A {
  constructor() {
  }
}

// Current output
var A = function A() {
  _classCallCheck(this, A);
};

// Expected output
var A = function A() {
  "use strict";
  _classCallCheck(this, A);
};

Jessidhia · 2018-03-05T09:05:30Z

We definitely need to always create an IIFE then. Otherwise, it won't be possible to have argument defaults or destructuring in the constructor's arguments.

Andarist · 2018-03-06T20:31:52Z

@Kovensky could u give example of problematic input? I've looked into constructor with default argument case and I'm not sure why it wouldn't be possible to skip the IIFE in such case.

loganfsmyth · 2018-03-06T20:45:03Z

@Andarist For instance if we converted

class Foo {
  constructor(arg = {}) {}
}

to

let Foo = function(arg = {}) {
  "use strict";
};

that in fact throws an error because non-simple parameters need to already be strict by the time the parser reaches them, if you're going to try to make them strict.

We could likely skip the IIFE and keeping the simple function, as long as we special-case the non-simple function params to still create the IIFE.

Andarist · 2018-03-06T20:54:24Z

Ok, that's an interesting case. As you said - we could try to continue optimizing this and de-opt on non-simple params when strict directive has to be added to the class (which in reality should be in minority of cases). I would be for optimizing it as it shouldn't be too complicated code-wise and I'm a little weirdo who likes optimized outputs 😅 dunno how you & other feel about such cases though.

Also I'm wondering why this:

let Foo = function(arg = {}) {};

doesn't throw? 🤔 Why it starts to throw once we add a strict directive in the function's body?

loganfsmyth · 2018-03-06T21:01:37Z

Also I'm wondering why this: ... doesn't throw?

There's nothing wrong with having non-simple parameters when non-strict. The problem is that use strict in the body of a function affects the whole function, including the parameters. But because use strict is in the body of the function, there is a whole ton of work done by the parser before it has reached that point.

The issue essentially is that if your code is non-strict and you're parsing a whole function and then suddenly hit a use strict in the body, you'd have to rewind and redo all of the processing for the function a second time. For example

function fn(
  arg = function() {
    var let;
  }
) {
  "use strict";

}

when the parser encounters var let, should it error, or not? If use strict were allowed in the function body like this, it's impossible to say when the statement was parsed, because even if it isn't strict yet, it would be possible that later on it would suddenly become strict.

Andarist · 2018-03-07T07:54:45Z

@loganfsmyth thanks for clarifying, makes sense 😄

MarkusToe · 2018-03-21T07:59:41Z

@loganfsmyth I am not sure how to handle the constructor only cases - could you please give me some guidance on where to start and what to check for?

So I guess at first we have to add the "use strict" directive to constructor-only function bodies, and then we should check for non-simple arguments and not add the "use strict" directive?

Andarist · 2018-03-21T09:42:48Z

I think there are such cases:

already in strict mode - just create "only constructor"
not in strict, we need to add it
a) simple arguments - add it inside the "only constructor"
b) non-simple arguments - fallback, create IIFE with strict directive and return "only constructor"

loganfsmyth · 2018-04-07T18:42:25Z

How can I help us get this across the finish line? You've done some great work and I feel bad that it keep getting conflicts.

- Add "use strict" directive to transformed class bodies

)

But constructor only that use non-simple parameters must use a strict function wrapper.

jridgewell · 2018-04-10T01:59:27Z

Pushed a rebase, and included the constructor-only and non-simple parameter edge case.

nicolo-ribaudo · 2018-04-10T19:54:38Z

@jridgewell Can you move the strictParent function to be reusable in the other PR? e.g. Maybe path.scope.isStrict().
Also, you should revert the changes to lerna.json

jridgewell · 2018-04-10T20:20:39Z

Can you move the strictParent function to be reusable in the other PR? e.g. Maybe path.scope.isStrict().

Was already planning on it. Would you file an issue?

Also, you should revert the changes to lerna.json

Done.

jridgewell · 2018-04-10T21:34:53Z

Thanks for the core of the work @MarkusToe!

MarkusToe · 2018-04-12T06:22:54Z

@jridgewell Thanks for finishing and I'm sorry that I didn't put more time in lately.

loganfsmyth · 2018-04-12T06:56:59Z

@MarkusToe No problem at all. Thanks for getting the work on this moving in the first place!

xtuc reviewed Mar 1, 2018

View reviewed changes

xtuc approved these changes Mar 1, 2018

View reviewed changes

Andarist reviewed Mar 1, 2018

View reviewed changes

xtuc requested changes Mar 1, 2018

View reviewed changes

Andarist reviewed Mar 1, 2018

View reviewed changes

xtuc approved these changes Mar 2, 2018

View reviewed changes

nicolo-ribaudo requested changes Mar 5, 2018

View reviewed changes

MarkusToe force-pushed the add-use-strict-directive branch 2 times, most recently from 9d3143d to 07b8409 Compare March 14, 2018 07:52

loganfsmyth mentioned this pull request Apr 7, 2018

Get set helpers #7687

Merged

Markus Török added 3 commits April 9, 2018 21:07

babel#7349

b373c3b

- Add "use strict" directive to transformed class bodies

Update tests (babel#7349)

7c38976

Add test for program with use strict directive (babel#7349)

c7ee2f2

Markus Török and others added 8 commits April 9, 2018 21:08

Update tests (babel#7349)

5d19fbc

Update source mapping (babel#7349)

28ec5a8

Add test for parent blockStatement with use strict directive (babel#7349

fc650cd

)

Update tests (babel#7349)

f645d57

Update tests (babel#7349)

bfb8297

Update tests (babel#7349)

4e67d62

Add test for constructor-only class (babel#7349)

880487c

Constructor only classes are strict, too

0ef10c8

But constructor only that use non-simple parameters must use a strict function wrapper.

jridgewell force-pushed the add-use-strict-directive branch from 1462b23 to 0ef10c8 Compare April 10, 2018 01:58

jridgewell added 2 commits April 9, 2018 23:36

Fix test

b348241

Wrapper not needed if class is already strict

e81a033

Revert change to lerna.json

dad92ef

jridgewell closed this Apr 10, 2018

jridgewell reopened this Apr 10, 2018

nicolo-ribaudo approved these changes Apr 10, 2018

View reviewed changes

jridgewell merged commit 6597a47 into babel:master Apr 10, 2018

existentialism added the PR: New Feature 🚀 A type of pull request used for our changelog categories label Apr 12, 2018

lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 5, 2019

lock bot locked as resolved and limited conversation to collaborators Oct 5, 2019

Uh oh!

Conversation

MarkusToe commented Feb 22, 2018

Uh oh!

xtuc commented Feb 22, 2018

Uh oh!

MarkusToe commented Feb 23, 2018

Uh oh!

xtuc commented Feb 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

babel-bot commented Mar 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

babel-bot commented Mar 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xtuc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xtuc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Andarist Mar 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xtuc left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolo-ribaudo left a comment

Choose a reason for hiding this comment

Uh oh!

Jessidhia commented Mar 5, 2018

Uh oh!

Andarist commented Mar 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

loganfsmyth commented Mar 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Andarist commented Mar 6, 2018

Uh oh!

loganfsmyth commented Mar 6, 2018

Uh oh!

Andarist commented Mar 7, 2018

Uh oh!

MarkusToe commented Mar 21, 2018

Uh oh!

Andarist commented Mar 21, 2018

Uh oh!

loganfsmyth commented Apr 7, 2018

Uh oh!

jridgewell commented Apr 10, 2018

Uh oh!

nicolo-ribaudo commented Apr 10, 2018

Uh oh!

jridgewell commented Apr 10, 2018

Uh oh!

jridgewell commented Apr 10, 2018

Uh oh!

babel-bot commented Mar 1, 2018 •

edited

Loading

Andarist Mar 3, 2018 •

edited

Loading

xtuc left a comment •

edited

Loading

Andarist commented Mar 6, 2018 •

edited

Loading

loganfsmyth commented Mar 6, 2018 •

edited

Loading