Replace `x != undefined` with `x != null` by j-f1 · Pull Request #572 · babel/minify

j-f1 · 2017-06-08T10:20:17Z

~~Bonus: replace undefined == null and null != null with !0 and !1, respectively.~~

Tests included.

vigneshshanmugam · 2017-06-08T11:02:54Z

packages/babel-plugin-transform-simplify-comparison-operators/__tests__/eqnull-test.js

+
+const babel = require("babel-core");
+const plugin = require("../src/index");
+const unpad = require("../../../utils/unpad");


can be removed

vigneshshanmugam · 2017-06-08T11:03:47Z

packages/babel-plugin-transform-simplify-comparison-operators/src/index.js

        const op = node.operator;
+        const negated = node.operator.startsWith("!");
+
+        if (["!=", "=="].includes(node.operator)) {


use indexOf, we still support node 4

codecov · 2017-06-08T11:54:19Z

Codecov Report

Merging #572 into master will increase coverage by 0.44%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #572      +/-   ##
==========================================
+ Coverage   83.02%   83.46%   +0.44%     
==========================================
  Files          41       42       +1     
  Lines        2763     2867     +104     
  Branches      967     1008      +41     
==========================================
+ Hits         2294     2393      +99     
- Misses        282      283       +1     
- Partials      187      191       +4

Impacted Files	Coverage Δ
...ansform-simplify-comparison-operators/src/index.js	`100% <100%> (ø)`	⬆️
...plugin-minify-constant-folding/src/replacements.js	`96.82% <0%> (ø)`
.../babel-plugin-minify-constant-folding/src/index.js	`85.56% <0%> (+3.02%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cb993ed...fc230db. Read the comment docs.

boopathi · 2017-06-08T13:14:11Z

packages/babel-plugin-transform-simplify-comparison-operators/__tests__/eqnull-test.js

+  });
+
+  it("should shorten `undefined == null`", () => {
+    const source = "undefined == null";


this should be already taken care of by constant-folding plugin. This plugin should not worry about these folding constants.

It doesn’t appear to handle this. Are you saying that I should move it there?

That can be fixed in path.evaluate of babel if that doesn't handle it. Or it's an ordering issue of who runs first - #61 #422.

I could add void 0 to isNull.

How does that sound @boopathi?

I meant this entire replacement shouldn't be a part of this plugin. It should be handled in path.evaluate of babel.

@boopathi done.

xtuc · 2017-06-12T07:56:47Z

I'm wondering if we can do this, isn't it unsafe? null and undefined are not equal.

j-f1 · 2017-06-12T09:35:21Z

@xtuc null and undefined are double-equals equal, not triple-equals equal.

boopathi · 2017-06-13T10:22:44Z

packages/babel-plugin-transform-simplify-comparison-operators/src/index.js

        const op = node.operator;
+
+        if (["!=", "=="].indexOf(node.operator) !== -1) {
+          if (t.isIdentifier(node.right, { name: "undefined" })) {


void 0 will not be transformed.

boopathi · 2017-06-13T10:22:58Z

Can you add some tests for this ?

j-f1 · 2017-06-13T10:39:25Z

@boopathi Are you assuming in babili that people won’t do void someFunctionWithSideEffects()?

j-f1 · 2017-06-13T10:51:19Z

@boopathi Tests restored.

boopathi · 2017-06-13T12:55:23Z

Are you assuming in babili that people won’t do void someFunctionWithSideEffects()?

Constant-folding plugin should take care of folding constants. void foo() should be a deopt anyway here. It should check only for void x where pathOfX.isPure() is true.

boopathi · 2017-08-14T22:37:21Z

Just a few thoughts on this -

Should it go into comparison-operators?
Since this is an unsafe transformation, it's better to put this entire transformation behind an option - undefinedToNull or voidToNull or anything else relevant.
Update tests to use thePlugin
t.isIdentifier("undefined") check should also check for bindings in the scope path.scope.getBinding("undefined") there is a defined undefined.

j-f1 · 2017-08-14T23:23:22Z

@boopathi

👍
I don’t think it’s unsafe. Why do you think it’s unsafe?
👍
👍

xtuc · 2017-08-17T07:38:09Z

it's safe since Replace x != undefined with x != null #572 (comment) (see Equality_comparisons_and_sameness)

j-f1 · 2017-08-17T11:51:49Z

@boopathi

It’s already in comparison-operators.
❓
✅
✅

boopathi · 2017-08-17T12:00:56Z

My question is whether it should go into comparison operators, or should it go into simplify. IMO, Comparison operators simplifies the operator used without changing the operands.

vigneshshanmugam · 2017-08-17T12:35:28Z

packages/babel-plugin-transform-simplify-comparison-operators/src/index.js

+  function undefinedToNull(path) {
+    if (isRealUndefined(path) || isPureVoid(path)) {
+      path.replaceWith(t.nullLiteral());
+      return true;


is this return true / false required? I don't see it used anywhere for conditional check.

Removed 🔥

vigneshshanmugam · 2017-08-17T12:38:33Z

packages/babel-plugin-transform-simplify-comparison-operators/src/index.js

+  function isRealUndefined(path) {
+    return (
+      path.isIdentifier({ name: "undefined" }) &&
+      !path.scope.getBinding("undefined")


why is this check required?. Even if someone does

var undefined = 'blah';

Its a read only property and does not impact our null conversion.

@boopathi said:

t.isIdentifier("undefined") check should also check for bindings in the scope path.scope.getBinding("undefined") there is a defined undefined.

/cc @boopathi Any reason why the check is needed? Am i missing some edge case?

function foo(undefined) { return undefined == void 0 } foo("bar"); // true or false ?

true even if undefined or both are converted to null

> function foo(undefined) { return undefined == void 0 } > foo("bar"); // true or false ? // false

okay got it 👍

boopathi · 2017-08-18T22:14:56Z

It's not immediately obvious that this goes into comparison-operators plugin. This question still applies -

My question is whether it should go into comparison operators, or should it go into simplify. IMO, Comparison operators simplifies the operator used without changing the operands

j-f1 · 2017-08-26T01:02:59Z

@boopathi If you’d like me to move it to simplify, I’ll gladly do so. Should I?

j-f1 · 2017-11-18T22:35:32Z

Sorry for the delay on this @boopathi 😢

I’ve moved the changes to babel-plugin-minify-simplify.

j-f1 · 2017-11-19T00:05:19Z

Looks like the Travis failure is unrelated. Closing and reopening to restart the build.

j-f1 · 2017-12-10T19:50:05Z

Ping @boopathi and @vigneshshanmugam for 👀

boopathi · 2017-12-11T21:17:02Z

@j-f1 Hi, Thanks for the PR. Right now let's concentrate on fixing the existing bugs, babel-7, improving the speed of transformations and fixing the memory issue to get it to 1.0. New set of features may delay that. I'll leave it open and postpone it to sometime later.

j-f1 added 7 commits June 7, 2017 14:45

Replace x != undefined with x != null

175aa88

Create eqnull-test.js

9801c15

Minify null != null, undefined == null, etc.

f2b350b

Test undefined == undefined

e9d86bc

Include babel-types

4d27ae8

Fix a typo

fa02b57

Semicolons in tests

6e5adfd

vigneshshanmugam reviewed Jun 8, 2017

View reviewed changes

Remove unused require

e9a6a72

j-f1 added 2 commits June 8, 2017 07:56

Node 4 support

e0bc92b

npm run format

387de4f

j-f1 force-pushed the patch-1 branch from 82bf3e9 to 387de4f Compare June 8, 2017 12:44

boopathi reviewed Jun 8, 2017

View reviewed changes

j-f1 added 2 commits June 10, 2017 18:44

Remove unwanted transform

df499a3

🔥 unused variable

0f6b6d8

boopathi reviewed Jun 13, 2017

View reviewed changes

j-f1 added 2 commits June 13, 2017 06:50

void <something> is undefined too

9a92d0e

Restore eqnull-test.js

a6a37ba

j-f1 force-pushed the patch-1 branch from 48ad6aa to a6a37ba Compare June 13, 2017 10:50

j-f1 added 3 commits June 13, 2017 10:57

Only include pure void expressions

7be00f7

Fix scope

4599d9d

npm run format

fc230db

boopathi added the Tag: New Feature Pull Request adding a new feature label Aug 7, 2017

j-f1 added 5 commits August 17, 2017 07:44

Merge remote-tracking branch 'babel/master' into patch-1

7c4ea21

thePlugin

5389834

Check if there’s a variable named undefined before transforming

fa150db

t.isFoo(path.node) → path.isFoo()

cb20df5

npm run format

4abca15

vigneshshanmugam reviewed Aug 17, 2017

View reviewed changes

🔥 undefinedToNull returns

92de46d

vigneshshanmugam approved these changes Aug 17, 2017

View reviewed changes

j-f1 added 3 commits November 18, 2017 16:55

Merge remote-tracking branch 'upstream/master' into patch-1

d61ccaa

Move the feature to simplify

6b921fe

Use test fixtures

68e21f9

j-f1 closed this Nov 19, 2017

j-f1 reopened this Nov 19, 2017

Merge branch 'master' into patch-1

4e96f58

boopathi approved these changes May 14, 2018

View reviewed changes

boopathi merged commit 540d75e into babel:master May 14, 2018

j-f1 deleted the patch-1 branch May 14, 2018 13:37

Uh oh!

Conversation

j-f1 commented Jun 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vigneshshanmugam Jun 8, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Jun 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

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!

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!

j-f1 Jun 10, 2017 • 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!

xtuc commented Jun 12, 2017

Uh oh!

j-f1 commented Jun 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

boopathi commented Jun 13, 2017

Uh oh!

j-f1 commented Jun 13, 2017

Uh oh!

j-f1 commented Jun 13, 2017

Uh oh!

boopathi commented Jun 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

boopathi commented Aug 14, 2017

Uh oh!

j-f1 commented Aug 14, 2017

Uh oh!

xtuc commented Aug 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

j-f1 commented Aug 17, 2017

Uh oh!

boopathi commented Aug 17, 2017

Uh oh!

vigneshshanmugam Aug 17, 2017 • 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

j-f1 commented Jun 8, 2017 •

edited

Loading

vigneshshanmugam Jun 8, 2017 •

edited

Loading

codecov bot commented Jun 8, 2017 •

edited

Loading

j-f1 Jun 10, 2017 •

edited

Loading

boopathi commented Jun 13, 2017 •

edited

Loading

xtuc commented Aug 17, 2017 •

edited

Loading

vigneshshanmugam Aug 17, 2017 •

edited

Loading