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.

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

Implement try-statement-deoptimization for feature detection, tree-shake unused arguments #2892

Merged
merged 8 commits into from Jun 5, 2019

Conversation

lukastaegert
Copy link
Member

@lukastaegert lukastaegert commented Jun 4, 2019

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

List any relevant issue numbers:
resolves #2891
resolves #2869
resolves #2540
resolves #2273
resolves #1771

Description

This PR adds a heuristic around try-catch blocks that allows for feature detection workflows, or workflows that depend on uncommonly thrown errors such as checking if class test extends someVar {} throws an error to see if someVar is a valid constructor.

I checked that with this modification, all feature detections of core-js should work as intended. Besides the try-catch deoptimization, this PR will also tree-shake unused function arguments.

Try-statement deoptimization

This PR adds a heuristic based on the idea that in order to detect an uncommonly thrown error, you will probably put it into the try-statement of a try-catch construct (alternatively promises would work as well but the heuristic will not cover those).

Therefore:

  • Tree-shaking will be disabled inside try-statements, i.e. code directly nested inside a try-statement will be retained.

  • If a variable is

    • called from inside try-statement
    • called directly, i.e. myVar() and not e.g. (myVar || otherVar)().
    • can be uniquely resolved to a function declaration/expression (i.e. variable is never reassigned)

    then tree-shaking will also be disabled in that function; however, other variables called from such a function will NOT be deoptimized to contain the infectious nature of this feature.

  • If a parameter of a function is called inside a try-statement, this parameter is marked in a special way. In all places this function is called, the argument provided for this parameter will be treated the same way as if it was called from within the try-statement. This is to allow core-js style feature detection:

    function fails(exec) {
      try {
        return !!exec();
      } catch (e) {
        return true;
      }
    }
    
    var MAX_UINT32 = 0xffffffff;
    var SUPPORTS_Y = !fails(function() {
    	RegExp(MAX_UINT32, 'y'); // this will now be retained by Rollup
    });

Unused argument tree-shaking

To use some of the logic built for this feature for other cool stuff, this PR will not also tree-shake unused arguments from function calls. To that end, each time the call target of a call can be uniquely resolved, unused arguments will be removed from the call. More precisely:

  • A parameter that is either

    • not defined or
    • an identifier (i.e. the parameter is not a pattern or the paramter has a default value) and defined but not used by a function

    is "unused" unless arguments is accessed inside that function.

  • Call arguments corresponding to "unused" parameters will be removed if

    • they have no side effects (e.g. they are not the result of a side-effect-ful function call)
    • there are no other arguments after that call that are included (to maintain the correspondence between arguments and parameters).
  • Parameters themselves will not be removed even if they are unused to avoid difficult edge-cases when some logic relies on functionName.length to determine the number of parameters for e.g. currying.

This enables tree-shaking in scenarios such as

// input
const options = {
  prefix: 'cool '
}

const useOptions = false;

function doIt(value, options) {
  if (useOptions) console.log(options.prefix + value);
  else console.log(value);
}

doIt('feature', options);

// result
function doIt(value, options) {
  console.log(value);
}

doIt('feature');

Copy link
Contributor

@guybedford guybedford left a comment

Choose a reason for hiding this comment

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

Very pleased to see work in these directions, and agreed its about matching the common feature detection forms here.

It would be good to see some tests on try-catch dynamic require / non-analyzed dynamic import cases that are passed through to the built environment, although in theory that would be no different right?

@lukastaegert
Copy link
Member Author

At the moment it would not make a difference for dynamic imports. Except to make sure that dynamic imports inside try-blocks are always included if the try-block is included.

@ljharb
Copy link

ljharb commented Jun 4, 2019

It’d be great to test this on es5-shim, es6-shim, and all of airbnb-browser-shims, since that would catch the bug referenced in #2540.

@lukastaegert
Copy link
Member Author

You can try it out by installing rollup via npm install rollup/rollup#try-block-deoptimization.

@lukastaegert
Copy link
Member Author

at least es6-shim should be fine, throwsError is exactly something that should be caught by the heuristic.

@lukastaegert
Copy link
Member Author

Any help here is welcome, a simple way to test it could be to write an entry point that contains the shim, e.g. main.js, and then run

npx rollup/rollup#try-block-deoptimization main.js --format esm > output.js
npx rollup/rollup#try-block-deoptimization main.js --format esm --no-treeshake > full-output.js

and diff the two files output.js and full-output.js with your favourite diffing tool to see if something vital is missing in output.js.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants