Skip to content

Commit

Permalink
Fix compress/eval of optional chains (#1062)
Browse files Browse the repository at this point in the history 
* Fix compress/eval of optional chains

This implements the recursion necessary to determine if a property access/call is part of an optional chain that has short-circuited.

* fix evaluation of chain

* Apply suggestions from code review

Co-authored-by: F谩bio Santos <fabiosantosart@gmail.com>

* Add test cases for future optional chain dropping

* Only drop expression if optional chaining is used

* Ensure nullish is never returned from evaluate

Co-authored-by: F谩bio Santos <fabiosantosart@gmail.com>
  • Loading branch information
@jridgewell @fabiosantoscode
jridgewell and fabiosantoscode committed Sep 19, 2021
1 parent 38bef40 commit 8dea529
Show file tree
Hide file tree
Showing 6 changed files with 184 additions and 52 deletions.
21 changes: 9 additions & 12 deletions lib/compress/drop-side-effect-free.js
View file
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
27 changes: 15 additions & 12 deletions lib/compress/evaluate.js
View file
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 All @@ -93,7 +96,7 @@ AST_Node.DEFMETHOD("evaluate", function (compressor) {
var val = this._eval(compressor, 1);
if (!val || val instanceof RegExp)
return val;
if (typeof val == "function" || typeof val == "object")
if (typeof val == "function" || typeof val == "object" || val == nullish)
return this;
return val;
});
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
12 changes: 1 addition & 11 deletions lib/compress/index.js
View file
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 @@ -4200,14 +4196,11 @@ def_optimize(AST_Sub, function(self, compressor) {
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 @@ -4274,9 +4267,6 @@ def_optimize(AST_Dot, function(self, compressor) {
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
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
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);
}
}
25 changes: 23 additions & 2 deletions test/compress/evaluate.js
View file
Expand Up @@ -1783,8 +1783,29 @@ null_conditional_chain_eval: {
null?.maybe_call?.(3)
}
expect: {
(void 0).but_might_throw;
(void 0)(1);
}
}

null_conditional_chain_eval_2: {
options = {
evaluate: true,
side_effects: true
}
input: {
null.deep?.unused
null.deep?.[side_effect()]
null.deep?.unused.but_might_throw
null.deep?.call(1)
null.deep?.(2)
null.deep?.maybe_call?.(3)
}
expect: {
null.deep?.unused
null.deep?.[side_effect()]
null.deep?.unused.but_might_throw
null.deep?.call(1)
null.deep?.(2)
null.deep?.maybe_call?.(3)
}
}

Expand Down

0 comments on commit 8dea529

Please sign in to comment.