Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix compress/eval of optional chains #1062

Merged
merged 6 commits into from Sep 19, 2021
Merged

Fix compress/eval of optional chains #1062

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9eac7c2
Fix compress/eval of optional chains
jridgewell Sep 2, 2021
7418170
fix evaluation of chain
jridgewell Sep 2, 2021
e7a4b72
Apply suggestions from code review
jridgewell Sep 13, 2021
f76cdb6
Add test cases for future optional chain dropping
jridgewell Sep 13, 2021
c3d7f15
Only drop expression if optional chaining is used
jridgewell Sep 16, 2021
ad43909
Ensure nullish is never returned from evaluate
jridgewell Sep 19, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
21 changes: 9 additions & 12 deletions lib/compress/drop-side-effect-free.js
View file
Open in desktop
Expand Up @@ -73,13 +73,12 @@ import {
AST_TemplateString,
AST_This,
AST_Unary,
AST_Undefined,
} from "../ast.js";
import { make_node, return_null, return_this } from "../utils/index.js";
import { first_in_statement } from "../utils/first_in_statement.js";

import { pure_prop_access_globals } from "./native-objects.js";
import { lazy_op, unary_side_effects, is_nullish } from "./inference.js";
import { lazy_op, unary_side_effects, is_nullish_shortcircuited } from "./inference.js";
import { WRITE_ONLY, set_flag, clear_flag } from "./compressor-flags.js";
import { make_sequence, is_func_expr, is_iife_call } from "./common.js";

Expand Down Expand Up @@ -121,8 +120,8 @@ def_drop_side_effect_free(AST_Constant, return_null);
def_drop_side_effect_free(AST_This, return_null);

def_drop_side_effect_free(AST_Call, function (compressor, first_in_statement) {
if (this.optional && is_nullish(this.expression, compressor)) {
return make_node(AST_Undefined, this);
if (is_nullish_shortcircuited(this, compressor)) {
return this.expression.drop_side_effect_free(compressor, first_in_statement);
}

if (!this.is_callee_pure(compressor)) {
Expand Down Expand Up @@ -298,21 +297,19 @@ def_drop_side_effect_free(AST_Array, function (compressor, first_in_statement) {
});

def_drop_side_effect_free(AST_Dot, function (compressor, first_in_statement) {
if (this.optional) {
return is_nullish(this.expression, compressor) ? make_node(AST_Undefined, this) : this;
if (is_nullish_shortcircuited(this, compressor)) {
return this.expression.drop_side_effect_free(compressor, first_in_statement);
}
if (this.expression.may_throw_on_access(compressor))
return this;
if (this.expression.may_throw_on_access(compressor)) return this;

return this.expression.drop_side_effect_free(compressor, first_in_statement);
});

def_drop_side_effect_free(AST_Sub, function (compressor, first_in_statement) {
if (this.optional) {
return is_nullish(this.expression, compressor) ? make_node(AST_Undefined, this) : this;
if (is_nullish_shortcircuited(this, compressor)) {
return this.expression.drop_side_effect_free(compressor, first_in_statement);
}
if (this.expression.may_throw_on_access(compressor))
return this;
if (this.expression.may_throw_on_access(compressor)) return this;

var expression = this.expression.drop_side_effect_free(compressor, first_in_statement);
if (!expression)
Expand Down
25 changes: 14 additions & 11 deletions lib/compress/evaluate.js
View file
Open in desktop
Expand Up @@ -82,6 +82,9 @@ function def_eval(node, func) {
node.DEFMETHOD("_eval", func);
}

// Used to propagate a nullish short-circuit signal upwards through the chain.
export const nullish = Symbol("This AST_Chain is nullish");

// If the node has been successfully reduced to a constant,
// then its value is returned; otherwise the element itself
// is returned.
Expand Down Expand Up @@ -338,11 +341,8 @@ const regexp_flags = new Set([
]);

def_eval(AST_PropAccess, function (compressor, depth) {
if (this.optional) {
const obj = this.expression._eval(compressor, depth);
if (obj == null)
return undefined;
}
const obj = this.expression._eval(compressor, depth);
if (obj === nullish || (this.optional && obj == null)) return nullish;
if (compressor.option("unsafe")) {
var key = this.property;
if (key instanceof AST_Node) {
Expand Down Expand Up @@ -397,16 +397,19 @@ def_eval(AST_PropAccess, function (compressor, depth) {

def_eval(AST_Chain, function (compressor, depth) {
const evaluated = this.expression._eval(compressor, depth);
return evaluated === this.expression ? this : evaluated;
return evaluated === nullish
? undefined
: evaluated === this.expression
? this
: evaluated;
});

def_eval(AST_Call, function (compressor, depth) {
var exp = this.expression;
if (this.optional) {
const callee = this.expression._eval(compressor, depth);
if (callee == null)
return undefined;
}

const callee = exp._eval(compressor, depth);
if (callee === nullish || (this.optional && callee == null)) return nullish;

if (compressor.option("unsafe") && exp instanceof AST_PropAccess) {
var key = exp.property;
if (key instanceof AST_Node) {
Expand Down
20 changes: 5 additions & 15 deletions lib/compress/index.js
View file
Open in desktop
Expand Up @@ -177,7 +177,7 @@ import {
} from "../scope.js";
import "../size.js";

import "./evaluate.js";
import { nullish } from "./evaluate.js";
import "./drop-side-effect-free.js";
import "./reduce-vars.js";
import {
Expand Down Expand Up @@ -2012,10 +2012,6 @@ def_optimize(AST_Call, function(self, compressor) {
}
}

if (self.optional && is_nullish(fn, compressor)) {
return make_node(AST_Undefined, self);
}

var is_func = fn instanceof AST_Lambda;

if (is_func && fn.pinned()) return self;
Expand Down Expand Up @@ -2436,7 +2432,7 @@ def_optimize(AST_Call, function(self, compressor) {
}

var ev = self.evaluate(compressor);
if (ev !== self) {
if (ev !== self && ev !== nullish) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can remove these checks, by making evaluate return this when it sees nullish.

https://github.com/terser/terser/pull/1062/files#diff-3e5692bda6529f2cdc21e76c6bc0d34e209304ad796f83acb5905d4b84931d8eR93-R102

There's already a bit of a redirection, so it's probably good to take advantage of it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For some reason, the link doesn't show me where you're pointing. I'm assuming it's the AST_Node.DEFMETHOD("evaluate", …) definition, so I added a check there.

ev = make_node_from_constant(ev, self).optimize(compressor);
return best_of(compressor, ev, self);
}
Expand Down Expand Up @@ -4195,18 +4191,15 @@ def_optimize(AST_Sub, function(self, compressor) {
}
}
var ev = self.evaluate(compressor);
if (ev !== self) {
if (ev !== self && ev !== nullish) {
ev = make_node_from_constant(ev, self).optimize(compressor);
return best_of(compressor, ev, self);
}
if (self.optional && is_nullish(self.expression, compressor)) {
return make_node(AST_Undefined, self);
}
return self;
});

def_optimize(AST_Chain, function (self, compressor) {
self.expression = self.expression.optimize(compressor);
if (is_nullish(self.expression, compressor)) return make_node(AST_Undefined, self);
return self;
});

Expand Down Expand Up @@ -4269,13 +4262,10 @@ def_optimize(AST_Dot, function(self, compressor) {
if (sub) return sub.optimize(compressor);
}
let ev = self.evaluate(compressor);
if (ev !== self) {
if (ev !== self && ev !== nullish) {
ev = make_node_from_constant(ev, self).optimize(compressor);
return best_of(compressor, ev, self);
}
if (self.optional && is_nullish(self.expression, compressor)) {
return make_node(AST_Undefined, self);
}
return self;
});

Expand Down
40 changes: 27 additions & 13 deletions lib/compress/inference.js
View file
Open in desktop
Expand Up @@ -226,9 +226,8 @@ export function is_undefined(node, compressor) {
);
}

// Find out if something is == null
// Used to optimize ?. and ??
export function is_nullish(node, compressor) {
// Is the node explicitly null or undefined.
function is_null_or_undefined(node, compressor) {
let fixed;
return (
node instanceof AST_Null
Expand All @@ -238,13 +237,29 @@ export function is_nullish(node, compressor) {
&& (fixed = node.definition().fixed) instanceof AST_Node
&& is_nullish(fixed, compressor)
)
// Recurse into those optional chains!
|| node instanceof AST_PropAccess && node.optional && is_nullish(node.expression, compressor)
|| node instanceof AST_Call && node.optional && is_nullish(node.expression, compressor)
|| node instanceof AST_Chain && is_nullish(node.expression, compressor)
);
}

// Find out if this expression is optionally chained from a base-point that we
// can statically analyze as null or undefined.
export function is_nullish_shortcircuited(node, compressor) {
if (node instanceof AST_PropAccess || node instanceof AST_Call) {
return (
(node.optional && is_null_or_undefined(node.expression, compressor))
|| is_nullish_shortcircuited(node.expression, compressor)
);
}
if (node instanceof AST_Chain) return is_nullish_shortcircuited(node.expression, compressor);
return false;
}

// Find out if something is == null, or can short circuit into nullish.
// Used to optimize ?. and ??
export function is_nullish(node, compressor) {
if (is_null_or_undefined(node, compressor)) return true;
return is_nullish_shortcircuited(node, compressor);
}

// Determine if expression might cause side effects
// If there's a possibility that a node may change something when it's executed, this returns true
(function(def_has_side_effects) {
Expand Down Expand Up @@ -352,13 +367,12 @@ export function is_nullish(node, compressor) {
return any(this.elements, compressor);
});
def_has_side_effects(AST_Dot, function(compressor) {
if (is_nullish(this, compressor)) return false;
return !this.optional && this.expression.may_throw_on_access(compressor)
|| this.expression.has_side_effects(compressor);
});
def_has_side_effects(AST_Sub, function(compressor) {
if (this.optional && is_nullish(this.expression, compressor)) {
return false;
}
if (is_nullish(this, compressor)) return false;

return !this.optional && this.expression.may_throw_on_access(compressor)
|| this.expression.has_side_effects(compressor)
Expand Down Expand Up @@ -426,7 +440,7 @@ export function is_nullish(node, compressor) {
return any(this.body, compressor);
});
def_may_throw(AST_Call, function(compressor) {
if (this.optional && is_nullish(this.expression, compressor)) return false;
if (is_nullish(this, compressor)) return false;
if (any(this.args, compressor)) return true;
if (this.is_callee_pure(compressor)) return false;
if (this.expression.may_throw(compressor)) return true;
Expand Down Expand Up @@ -485,12 +499,12 @@ export function is_nullish(node, compressor) {
return this.body.may_throw(compressor);
});
def_may_throw(AST_Dot, function(compressor) {
if (is_nullish(this, compressor)) return false;
return !this.optional && this.expression.may_throw_on_access(compressor)
|| this.expression.may_throw(compressor);
});
def_may_throw(AST_Sub, function(compressor) {
if (this.optional && is_nullish(this.expression, compressor)) return false;

if (is_nullish(this, compressor)) return false;
return !this.optional && this.expression.may_throw_on_access(compressor)
|| this.expression.may_throw(compressor)
|| this.property.may_throw(compressor);
Expand Down
111 changes: 109 additions & 2 deletions test/compress/drop-unused.js
View file
Open in desktop
Expand Up @@ -2927,8 +2927,115 @@ unused_null_conditional_chain: {
null?.maybe_call?.(3);
}
expect: {
undefined.i_will_throw;
undefined(1);
}
}

unused_null_conditional_chain_2: {
options = {
toplevel: true,
defaults: true,
passes: 5,
}
input: {
let i = 0;
(i++, null)?.unused;
(i++, null)?.[side_effect()];
(i++, null)?.unused.i_will_throw;
(i++, null)?.call(1);
(i++, null)?.(2);
(i++, null)?.maybe_call?.(3);
}
expect: {
}
}

unused_null_conditional_chain_3: {
options = {
toplevel: true,
defaults: true,
passes: 5,
}
input: {
(side_effect(), null)?.unused;
(side_effect(), null)?.[side_effect()];
(side_effect(), null)?.unused.i_will_throw;
(side_effect(), null)?.call(1);
(side_effect(), null)?.(2);
(side_effect(), null)?.maybe_call?.(3);
}
expect: {
// TODO: Elminate everything after ?.
(side_effect(), null)?.unused,
(side_effect(), null)?.[side_effect()],
(side_effect(), null)?.unused.i_will_throw,
(side_effect(), null)?.call(1),
(side_effect(), null)?.(2),
(side_effect(), null)?.maybe_call?.(3);
}
}

unused_null_conditional_chain_4: {
options = {
toplevel: true,
defaults: true,
passes: 5,
}
input: {
function nullish() {
side_effect();
return null;
}
nullish()?.unused;
nullish()?.[side_effect()];
nullish()?.unused.i_will_throw;
nullish()?.call(1);
nullish()?.(2);
nullish()?.maybe_call?.(3);
}
expect: {
// TODO: Elminate everything after ?.
function nullish() {
return side_effect(), null;
}
nullish()?.unused,
nullish()?.[side_effect()],
nullish()?.unused.i_will_throw,
nullish()?.call(1),
nullish()?.(2),
nullish()?.maybe_call?.(3);
}
}

unused_null_conditional_chain_5: {
options = {
toplevel: true,
defaults: true,
passes: 5,
}
input: {
export const obj = {
side_effect() {
side_effect();
return { null: null };
}
}
obj.side_effect().null?.unused;
obj.side_effect().null?.[side_effect()];
obj.side_effect().null?.unused.i_will_throw;
obj.side_effect().null?.call(1);
obj.side_effect().null?.(2);
obj.side_effect().null?.maybe_call?.(3);
}
expect: {
export const obj = {
side_effect: () => (side_effect(), {null: null})
}
// TODO: Elminate everything after ?.
obj.side_effect().null?.unused,
obj.side_effect().null?.[side_effect()],
obj.side_effect().null?.unused.i_will_throw,
obj.side_effect().null?.call(1),
obj.side_effect().null?.(2),
obj.side_effect().null?.maybe_call?.(3);
}
}