From 01e246171830f5c9b5efbd430e43c038f67e28f7 Mon Sep 17 00:00:00 2001 From: Lukas Taegert-Atkinson Date: Fri, 15 Jul 2022 12:20:57 +0200 Subject: [PATCH] Do not include unused return expressions (#4573) * Add test * Do not include unused return expressions * Improve coverage --- src/ast/nodes/CallExpression.ts | 4 - src/ast/nodes/shared/MultiExpression.ts | 13 +- .../builtin-prototypes/literal/_expected.js | 24 +++- .../builtin-prototypes/literal/main.js | 136 ++++++++++-------- .../template-literal/_expected.js | 5 + .../template-literal/main.js | 6 + .../samples/return-after-error/_config.js | 5 + .../samples/return-after-error/_expected.js | 7 + test/form/samples/return-after-error/main.js | 8 ++ .../samples/class-prop-returns/_config.js | 22 +++ .../samples/class-prop-returns/main.js | 11 ++ .../conditionals-deoptimization/_config.js | 11 +- .../conditionals-deoptimization/main.js | 46 ++++-- .../samples/long-path-deopt/_config.js | 3 + test/function/samples/long-path-deopt/main.js | 27 ++++ 15 files changed, 233 insertions(+), 95 deletions(-) create mode 100644 test/form/samples/return-after-error/_config.js create mode 100644 test/form/samples/return-after-error/_expected.js create mode 100644 test/form/samples/return-after-error/main.js create mode 100644 test/function/samples/class-prop-returns/_config.js create mode 100644 test/function/samples/class-prop-returns/main.js create mode 100644 test/function/samples/long-path-deopt/_config.js create mode 100644 test/function/samples/long-path-deopt/main.js diff --git a/src/ast/nodes/CallExpression.ts b/src/ast/nodes/CallExpression.ts index 414904a8608..b4f0b1f45f5 100644 --- a/src/ast/nodes/CallExpression.ts +++ b/src/ast/nodes/CallExpression.ts @@ -99,10 +99,6 @@ export default class CallExpression extends CallExpressionBase implements Deopti this.callee.include(context, false); } this.callee.includeCallArguments(context, this.arguments); - const returnExpression = this.getReturnExpression(); - if (!returnExpression.included) { - returnExpression.include(context, false); - } } render( diff --git a/src/ast/nodes/shared/MultiExpression.ts b/src/ast/nodes/shared/MultiExpression.ts index 7497a8f914e..bddc520cac0 100644 --- a/src/ast/nodes/shared/MultiExpression.ts +++ b/src/ast/nodes/shared/MultiExpression.ts @@ -1,9 +1,8 @@ import type { DeoptimizableEntity } from '../../DeoptimizableEntity'; -import type { HasEffectsContext, InclusionContext } from '../../ExecutionContext'; +import type { HasEffectsContext } from '../../ExecutionContext'; import { NodeInteraction, NodeInteractionCalled } from '../../NodeInteractions'; import type { ObjectPath, PathTracker } from '../../utils/PathTracker'; import { ExpressionEntity } from './Expression'; -import type { IncludeChildren } from './Node'; export class MultiExpression extends ExpressionEntity { included = false; @@ -41,14 +40,4 @@ export class MultiExpression extends ExpressionEntity { } return false; } - - include(context: InclusionContext, includeChildrenRecursively: IncludeChildren): void { - // This is only relevant to include values that do not have an AST representation, - // such as UnknownArrayExpression. Thus we only need to include them once. - for (const expression of this.expressions) { - if (!expression.included) { - expression.include(context, includeChildrenRecursively); - } - } - } } diff --git a/test/form/samples/builtin-prototypes/literal/_expected.js b/test/form/samples/builtin-prototypes/literal/_expected.js index 52445d752cd..fe60d5932c9 100644 --- a/test/form/samples/builtin-prototypes/literal/_expected.js +++ b/test/form/samples/builtin-prototypes/literal/_expected.js @@ -1,15 +1,25 @@ // retained -true.valueOf().unknown.unknown(); true.valueOf()(); -(1).valueOf().unknown.unknown(); -(1).valueOf().unknown(); -(1).valueOf()[globalThis.unknown](); +true.valueOf()[globalThis.unknown](); +true.valueOf().unknown(); +true.valueOf().unknown.unknown(); +true.valueOf().unknown.unknown().unknown; + (1).valueOf()(); -'ab'.charAt(1).unknown.unknown(); +(1).valueOf()[globalThis.unknown](); +(1).valueOf().unknown(); +(1).valueOf().unknown.unknown(); +(1).valueOf().unknown.unknown().unknown; + 'ab'.charAt(1)(); +'ab'.charAt(1)[globalThis.unknown](); +'ab'.charAt(1).unknown(); +'ab'.charAt(1).unknown.unknown(); +'ab'.charAt(1).unknown.unknown().unknown; + null.unknown; -'ab'.replace( 'a', () => console.log( 1 ) || 'b' ); -'ab'.replaceAll( 'a', () => console.log( 1 ) || 'b' ); +'ab'.replace('a', () => console.log(1) || 'b'); +'ab'.replaceAll('a', () => console.log(1) || 'b'); // deep property access is forbidden true.x.y; diff --git a/test/form/samples/builtin-prototypes/literal/main.js b/test/form/samples/builtin-prototypes/literal/main.js index 4e8e734a7ce..d3add5bb105 100644 --- a/test/form/samples/builtin-prototypes/literal/main.js +++ b/test/form/samples/builtin-prototypes/literal/main.js @@ -1,67 +1,90 @@ -const boolean = true; -const valueOf1 = boolean.valueOf(); -const valueOf2 = true.valueOf(); -const valueOf3 = true.valueOf().valueOf(); -const valueOf4 = true.valueOf().valueOf().valueOf(); - // retained -true.valueOf().unknown.unknown(); true.valueOf()(); -(1).valueOf().unknown.unknown(); -(1).valueOf().unknown(); -(1).valueOf()[globalThis.unknown](); +true.valueOf()[globalThis.unknown](); +true.valueOf().unknown(); +true.valueOf().unknown.unknown(); +true.valueOf().unknown.unknown().unknown; + (1).valueOf()(); -'ab'.charAt(1).unknown.unknown(); +(1).valueOf()[globalThis.unknown](); +(1).valueOf().unknown(); +(1).valueOf().unknown.unknown(); +(1).valueOf().unknown.unknown().unknown; + 'ab'.charAt(1)(); +'ab'.charAt(1)[globalThis.unknown](); +'ab'.charAt(1).unknown(); +'ab'.charAt(1).unknown.unknown(); +'ab'.charAt(1).unknown.unknown().unknown; + null.unknown; -// number prototype -const _toExponential = (1).toExponential( 2 ).trim(); -const _toFixed = (1).toFixed( 2 ).trim(); -const _toLocaleString = (1).toLocaleString().trim(); -const _toPrecision = (1).toPrecision( 2 ).trim(); -const _numberValueOf = (1).valueOf().toExponential( 2 ); +// boolean prototype +true.valueOf(); +true.valueOf().valueOf(); +true.valueOf().valueOf().valueOf(); +const _booleanToString = true.toString().trim(); +const _booleanValueOf = true.valueOf().valueOf(); + // inherited -const _numberHasOwnProperty = (1).hasOwnProperty( 'toString' ).valueOf(); -const _numberIsPrototypeOf = (1).isPrototypeOf( 1 ).valueOf(); -const _numberPropertyIsEnumerable = (1).propertyIsEnumerable( 'toString' ).valueOf(); +const _booleanHasOwnProperty = true.hasOwnProperty('toString').valueOf(); +const _booleanIsPrototypeOf = true.isPrototypeOf(true).valueOf(); +const _booleanPropertyIsEnumerable = true.propertyIsEnumerable('toString').valueOf(); +const _booleanToLocaleString = true.toLocaleString().trim(); + +// number prototype +(1).valueOf(); +(1).valueOf().valueOf(); +(1).valueOf().valueOf().valueOf(); +const _numberToExponential = (1).toExponential(2).trim(); +const _numberToFixed = (1).toFixed(2).trim(); const _numberToLocaleString = (1).toLocaleString().trim(); +const _numberToPrecision = (1).toPrecision(2).trim(); const _numberToString = (1).toString().trim(); +const _numberValueOf = (1).valueOf().toExponential(2); + +// inherited +const _numberHasOwnProperty = (1).hasOwnProperty('toString').valueOf(); +const _numberIsPrototypeOf = (1).isPrototypeOf(1).valueOf(); +const _numberPropertyIsEnumerable = (1).propertyIsEnumerable('toString').valueOf(); // string prototype -const _at = 'ab'.at( 1 ) -const _charAt = 'ab'.charAt( 1 ).trim(); -const _charCodeAt = 'ab'.charCodeAt( 1 ).toExponential( 2 ); -const _codePointAt = 'ab'.codePointAt( 1 ); -const _concat = 'ab'.concat( 'c' ).trim(); -const _includes = 'ab'.includes( 'a' ).valueOf(); -const _endsWith = 'ab'.endsWith( 'a' ).valueOf(); -const _indexOf = 'ab'.indexOf( 'a' ).toExponential( 2 ); -const _lastIndexOf = 'ab'.lastIndexOf( 'a' ).toExponential( 2 ); -const _localeCompare = 'ab'.localeCompare( 'a' ).toExponential( 2 ); -const _match = 'ab'.match( /a/ ) -const _matchAll = 'ab'.matchAll( /a/ ) -const _normalize = 'ab'.normalize().trim(); -const _padEnd = 'ab'.padEnd( 4, 'a' ).trim(); -const _padStart = 'ab'.padStart( 4, 'a' ).trim(); -const _repeat = 'ab'.repeat( 2 ).trim(); -const _replace = 'ab'.replace( 'a', () => 'b' ).trim(); -const _replaceEffect = 'ab'.replace( 'a', () => console.log( 1 ) || 'b' ); -const _replaceAll = 'ab'.replaceAll( 'a', () => 'b' ).trim(); -const _replaceAllEffect = 'ab'.replaceAll( 'a', () => console.log( 1 ) || 'b' ); -const _search = 'ab'.search( /a/ ).toExponential( 2 ); -const _slice = 'ab'.slice( 0, 1 ).trim(); -const _split = 'ab'.split( 'a' ); -const _startsWith = 'ab'.startsWith( 'a' ).valueOf(); -const _substring = 'ab'.substring( 0, 1 ).trim(); -const _toLocaleLowerCase = 'ab'.toLocaleLowerCase().trim(); -const _toLocaleUpperCase = 'ab'.toLocaleUpperCase().trim(); -const _toLowerCase = 'ab'.toLowerCase().trim(); -const _toString = 'ab'.trim(); -const _toUpperCase = 'ab'.toUpperCase().trim(); -const _trim = 'ab'.trim().trim(); -const _trimEnd = 'ab'.trimEnd().trim(); -const _trimStart = 'ab'.trimStart().trim(); +'ab'.valueOf(); +'ab'.valueOf().valueOf(); +'ab'.valueOf().valueOf().valueOf(); +const _stringAt = 'ab'.at(1); +const _stringCharAt = 'ab'.charAt(1).trim(); +const _stringCharCodeAt = 'ab'.charCodeAt(1).toExponential(2); +const _stringCodePointAt = 'ab'.codePointAt(1); +const _stringConcat = 'ab'.concat('c').trim(); +const _stringEndsWith = 'ab'.endsWith('a').valueOf(); +const _stringIncludes = 'ab'.includes('a').valueOf(); +const _stringIndexOf = 'ab'.indexOf('a').toExponential(2); +const _stringLastIndexOf = 'ab'.lastIndexOf('a').toExponential(2); +const _stringLocaleCompare = 'ab'.localeCompare('a').toExponential(2); +const _stringMatch = 'ab'.match(/a/); +const _stringMatchAll = 'ab'.matchAll(/a/); +const _stringNormalize = 'ab'.normalize().trim(); +const _stringPadEnd = 'ab'.padEnd(4, 'a').trim(); +const _stringPadStart = 'ab'.padStart(4, 'a').trim(); +const _stringRepeat = 'ab'.repeat(2).trim(); +const _stringReplace = 'ab'.replace('a', () => 'b').trim(); +const _stringReplaceEffect = 'ab'.replace('a', () => console.log(1) || 'b'); +const _stringReplaceAll = 'ab'.replaceAll('a', () => 'b').trim(); +const _stringReplaceAllEffect = 'ab'.replaceAll('a', () => console.log(1) || 'b'); +const _stringSearch = 'ab'.search(/a/).toExponential(2); +const _stringSlice = 'ab'.slice(0, 1).trim(); +const _stringSplit = 'ab'.split('a'); +const _stringStartsWith = 'ab'.startsWith('a').valueOf(); +const _stringSubstring = 'ab'.substring(0, 1).trim(); +const _stringToLocaleLowerCase = 'ab'.toLocaleLowerCase().trim(); +const _stringToLocaleUpperCase = 'ab'.toLocaleUpperCase().trim(); +const _stringToLowerCase = 'ab'.toLowerCase().trim(); +const _stringToString = 'ab'.trim(); +const _stringToUpperCase = 'ab'.toUpperCase().trim(); +const _stringTrim = 'ab'.trim().trim(); +const _stringTrimEnd = 'ab'.trimEnd().trim(); +const _stringTrimStart = 'ab'.trimStart().trim(); const _stringValueOf = 'ab'.valueOf().trim(); // DEPRECATED prototype methods @@ -83,11 +106,10 @@ const _trimLeft = 'ab'.trimLeft().trim(); const _trimRight = 'ab'.trimRight().trim(); // inherited -const _stringHasOwnProperty = 'ab'.hasOwnProperty( 'toString' ).valueOf(); -const _stringIsPrototypeOf = 'ab'.isPrototypeOf( '' ).valueOf(); -const _stringPropertyIsEnumerable = 'ab'.propertyIsEnumerable( 'toString' ).valueOf(); +const _stringHasOwnProperty = 'ab'.hasOwnProperty('toString').valueOf(); +const _stringIsPrototypeOf = 'ab'.isPrototypeOf('').valueOf(); +const _stringPropertyIsEnumerable = 'ab'.propertyIsEnumerable('toString').valueOf(); const _stringToLocaleString = 'ab'.toLocaleString().trim(); -const _stringToString = 'ab'.toString().trim(); // property access is allowed const accessBoolean = true.x; diff --git a/test/form/samples/builtin-prototypes/template-literal/_expected.js b/test/form/samples/builtin-prototypes/template-literal/_expected.js index 693f9e21560..fecddc5bb9b 100644 --- a/test/form/samples/builtin-prototypes/template-literal/_expected.js +++ b/test/form/samples/builtin-prototypes/template-literal/_expected.js @@ -1,3 +1,8 @@ +// retained +`ab`(); +`ab`.unknown.unknown(); +`ab`.unknown.unknown().unknown; + // deep property access is forbidden `ab`.x.y; diff --git a/test/form/samples/builtin-prototypes/template-literal/main.js b/test/form/samples/builtin-prototypes/template-literal/main.js index 4e7a67f32fa..eeb37ccacb1 100644 --- a/test/form/samples/builtin-prototypes/template-literal/main.js +++ b/test/form/samples/builtin-prototypes/template-literal/main.js @@ -1,3 +1,9 @@ +// retained +`ab`(); +`ab`.unknown.unknown(); +`ab`.unknown.unknown().unknown; + +// removed `ab`.trim(); `ab`.trim().trim(); `ab`.toString().trim(); diff --git a/test/form/samples/return-after-error/_config.js b/test/form/samples/return-after-error/_config.js new file mode 100644 index 00000000000..9574d009200 --- /dev/null +++ b/test/form/samples/return-after-error/_config.js @@ -0,0 +1,5 @@ +const path = require('path'); + +module.exports = { + description: 'tree-shakes entities referenced in a return statement after an error' +}; diff --git a/test/form/samples/return-after-error/_expected.js b/test/form/samples/return-after-error/_expected.js new file mode 100644 index 00000000000..fdf4b564e60 --- /dev/null +++ b/test/form/samples/return-after-error/_expected.js @@ -0,0 +1,7 @@ +function getInstance() { + throw new Error('error'); +} + +console.log(getInstance()); + +export { getInstance }; diff --git a/test/form/samples/return-after-error/main.js b/test/form/samples/return-after-error/main.js new file mode 100644 index 00000000000..eb43273f068 --- /dev/null +++ b/test/form/samples/return-after-error/main.js @@ -0,0 +1,8 @@ +class Removed {} + +export function getInstance() { + throw new Error('error'); + return new Removed(); +} + +console.log(getInstance()); diff --git a/test/function/samples/class-prop-returns/_config.js b/test/function/samples/class-prop-returns/_config.js new file mode 100644 index 00000000000..bd4c18af8b1 --- /dev/null +++ b/test/function/samples/class-prop-returns/_config.js @@ -0,0 +1,22 @@ +const assert = require('assert'); + +module.exports = { + description: 'does not remove calls to props without value', + minNodeVersion: 12, + exports({ callProp, callStaticProp }) { + let hasError = false; + try { + callStaticProp(); + } catch { + hasError = true; + } + assert.ok(hasError); + hasError = false; + try { + callProp(); + } catch { + hasError = true; + } + assert.ok(hasError); + } +}; diff --git a/test/function/samples/class-prop-returns/main.js b/test/function/samples/class-prop-returns/main.js new file mode 100644 index 00000000000..03c01649472 --- /dev/null +++ b/test/function/samples/class-prop-returns/main.js @@ -0,0 +1,11 @@ +class Foo { + static staticProp; + prop; +} + +export const callStaticProp = () => Foo.staticProp() || false; + +export const callProp = () => { + const foo = new Foo(); + return foo.prop() || false; +}; diff --git a/test/function/samples/conditionals-deoptimization/_config.js b/test/function/samples/conditionals-deoptimization/_config.js index 48ed684f81d..755ed4b5d3e 100644 --- a/test/function/samples/conditionals-deoptimization/_config.js +++ b/test/function/samples/conditionals-deoptimization/_config.js @@ -3,6 +3,15 @@ const assert = require('assert'); module.exports = { description: 'handles deoptimization of conditionals', exports(exports) { - assert.deepStrictEqual(exports, { first: true, second: true, third: true, fourth: true }); + assert.deepStrictEqual(exports, { + cond1a: true, + cond1b: true, + cond2a: true, + cond2b: true, + log1a: true, + log1b: true, + log2a: true, + log2b: true + }); } }; diff --git a/test/function/samples/conditionals-deoptimization/main.js b/test/function/samples/conditionals-deoptimization/main.js index 324e03b344e..667d597cdca 100644 --- a/test/function/samples/conditionals-deoptimization/main.js +++ b/test/function/samples/conditionals-deoptimization/main.js @@ -1,21 +1,39 @@ -export let first = false; -export let second = false; -export let third = false; -export let fourth = false; +export let cond1a = false; +export let cond1b = false; +export let cond2a = false; +export let cond2b = false; +export let log1a = false; +export let log1b = false; +export let log2a = false; +export let log2b = false; let flag = false; -checkConditional(); -checkLogical(); +checkConditional1(); +checkConditional2(); +checkLogical1(); +checkLogical2(); flag = true; -checkConditional(); -checkLogical(); +checkConditional1(); +checkConditional2(); +checkLogical1(); +checkLogical2(); -function checkConditional() { - if (flag ? true : false) first = true; - else second = true; +function checkConditional1() { + if (flag ? true : false) cond1a = true; + else cond1b = true; } -function checkLogical() { - if (flag && true) third = true; - else fourth = true; +function checkConditional2() { + if (!flag ? true : false) cond2a = true; + else cond2b = true; +} + +function checkLogical1() { + if (flag && true) log1a = true; + else log1b = true; +} + +function checkLogical2() { + if (!flag && true) log2a = true; + else log2b = true; } diff --git a/test/function/samples/long-path-deopt/_config.js b/test/function/samples/long-path-deopt/_config.js new file mode 100644 index 00000000000..37d8fd1f405 --- /dev/null +++ b/test/function/samples/long-path-deopt/_config.js @@ -0,0 +1,3 @@ +module.exports = { + description: 'deoptimizes getting return expression for long property paths' +}; diff --git a/test/function/samples/long-path-deopt/main.js b/test/function/samples/long-path-deopt/main.js new file mode 100644 index 00000000000..8a00a8a5a0f --- /dev/null +++ b/test/function/samples/long-path-deopt/main.js @@ -0,0 +1,27 @@ +let effect = false; +const obj = { + a: { + b: { + c: { + d: { + e: { + f: { + g: { + h: { + i: { + j: { + k: () => ({ effect: () => (effect = true) }) + } + } + } + } + } + } + } + } + } + } +}; + +obj.a.b.c.d.e.f.g.h.i.j.k().effect(); +assert.ok(effect);