rollup removes feature detect code from inside function when it should not: broken functionality in generated output

[GerHobbelt]

AFAICT this is closely related to #1946 and #1771.

A stripped-down showcase + work-around is provided below. At the bottom is a suggestion how this might be approached more consistently and without the need to 'discover' this issue ad hoc by everyone anytime because tests start to fail or code inspection turns up 'weird chunks of code' or ... 😉

How to reproduce

Take the code below (which comes from the XRegExp library @ https://github.com/slevithan/xregexp/blob/master/src/xregexp.js#L60) and run it through rollup using the given config file and command line below:

(dummy.js 💥💥💥)

function hasNativeFlag(flag) {
    // Can't check based on the presence of properties/getters since browsers might support such
    // properties even when they don't support the corresponding flag in regex construction (tested
    // in Chrome 48, where `'unicode' in /x/` is true but trying to construct a regex with flag `u`
    // throws an error)
    let isSupported = true;
    try {
        // Can't use regex literals for testing even in a `try` because regex literals with
        // unsupported flags cause a compilation error in IE
        new RegExp('', flag);   // 💥💥💥
    } catch (exception) {
        isSupported = false;
    }
    return isSupported;
}

export default hasNativeFlag;

(rollup.config.js)

// rollup.config.js
import resolve from 'rollup-plugin-node-resolve';

export default {
  input: 'dummy.js',
  output: [
      {
      file: 'dist/xregexp-cjs.js',
      format: 'cjs'
    },
    {
      file: 'dist/xregexp-es6.js',
      format: 'es'
    },
    {
      file: 'dist/xregexp-umd.js',
      name: 'XRegExp',
      format: 'umd'
    }
  ],
  plugins: [
    resolve({
      // use "module" field for ES6 module if possible
      module: true, // Default: true

      // use "main" field or index.js, even if it's not an ES6 module
      // (needs to be converted from CommonJS to ES6
      // – see https://github.com/rollup/rollup-plugin-commonjs
      main: true,  // Default: true

      // not all files you want to resolve are .js files
      extensions: [ '.js' ],  // Default: ['.js']

      // whether to prefer built-in modules (e.g. `fs`, `path`) or
      // local ones with the same names
      preferBuiltins: true,  // Default: true

      // If true, inspect resolved files to check that they are
      // ES2015 modules
      modulesOnly: true, // Default: false
    })
  ]
};

(command line)

rollup -c

Produces this FAULTY output:

(dist/xregexp-cjs.js 💥💥💥 )

'use strict';

function hasNativeFlag(flag) {
    // Can't check based on the presence of properties/getters since browsers might support such
    // properties even when they don't support the corresponding flag in regex construction (tested
    // in Chrome 48, where `'unicode' in /x/` is true but trying to construct a regex with flag `u`
    // throws an error)
    let isSupported = true;
    try {
    } catch (exception) {
        isSupported = false;
    }
    return isSupported;
}

module.exports = hasNativeFlag;

As you can see, the new RegExp(...) statement inside the try/catch block has disappeared, which kills the intended functionality of the given function: feature detection of regex flags cross-platform.

This looks a lot like #1946...

Work-around for today

The idea here is to make it hard for rollup to analyze the code while we still guarantee the proper function result when it executes.

That implies we'll need include the new RegExp(...) into the codeflow towards the return-ed function result somehow; fortunately JavaScript doesn't care about variable type, hence we can construct an expression inside the try/catch which is not immediately obvious constant assuming an expression evaluator inside the rollup 'compiler' which does not does a full constant-folding and type propagation analysis.

Lucky for us, rollup is not that smart yet.

WARNING: this trick may not work in future rollup versions when the code analysis gets smarter in there!

Here's the tweaked version which passes okay:

(dummy.js 👍 )

function hasNativeFlag(flag) {
    // Can't check based on the presence of properties/getters since browsers might support such
    // properties even when they don't support the corresponding flag in regex construction (tested
    // in Chrome 48, where `'unicode' in /x/` is true but trying to construct a regex with flag `u`
    // throws an error)
    let isSupported = true;
    try {
        // Can't use regex literals for testing even in a `try` because regex literals with
        // unsupported flags cause a compilation error in IE
        var x = new RegExp('', flag);
        isSupported = !!x || true;          // thwart ROLLUP
    } catch (exception) {
        isSupported = false;
    }
    return isSupported;
}

(command line)

rollup -c

now produces this CORRECT output:

(dist/xregexp-cjs.js 👌 )

'use strict';

function hasNativeFlag(flag) {
    // Can't check based on the presence of properties/getters since browsers might support such
    // properties even when they don't support the corresponding flag in regex construction (tested
    // in Chrome 48, where `'unicode' in /x/` is true but trying to construct a regex with flag `u`
    // throws an error)
    let isSupported = true;
    try {
        // Can't use regex literals for testing even in a `try` because regex literals with
        // unsupported flags cause a compilation error in IE
        var x = new RegExp('', flag);    // 👌
        isSupported = !!x || true;          // thwart ROLLUP 👌
    } catch (exception) {
        isSupported = false;
    }
    return isSupported;
}

module.exports = hasNativeFlag;

Suggestion how rollup could handle this pattern 'better'

As a lot of feature testing and other non-obvious code with lots of side effects often is wrapped in a try/catch, I'ld suggest to simply kill tree-shaking/code-removal logic for any code chunks which reside inside a try section of a try/catch/finally block.

If there's any 'magic' happening in the catch section, then (at least in our code bases) such 'magic stuff' is itself wrapped in another try/catch block, so the logic rollup might want to follow is then quite simple: no code removal inside any try/catch block.

The suggestion covers only code directly inside the try section; any code that exists inside a try/catch indirectly, i.e. via one or more function invocation levels, is free-for-all AFAIAC; if such code chunks, sometimes observed in (unit/system) test rigs, should be subjected to the same 'no code cleanup in here' rule suggestion, I'd say wrap that chunk in another try/catch block like this:

try {
  bla_bla_boom_side_effect;
} catch (ex) {
  throw ex;   // try thwarts rollup, `throw ex` propagates the exception down the stack
}

[GerHobbelt]

Silly me. Forgot to mention:

$ node_modules/.bin/rollup --version
rollup version 0.58.2

[lukastaegert]

I think it is actually the same situation as #1771. As stated there, I am rather opposed to implementing a heuristic to keep certain statements and would prefer an annotation solution. As you did not tell us about your background, would annotations be a suitable solution for your use case?

[GerHobbelt]

Annotations as an alternative to heuristics is fine. Bottom line for me is the code doesn't get too cluttered, like it is now with smartypants JS expressions to misdirect rollup. 😉
Killing code reduction inside try blocks is also not a 'sure thing' as sometimes one might want code cleanup happening in there, sometimes not, so that would be a source of future issues, while annotations are unambiguous in their purpose.

The drawback of having to inject annotations is that one has to visit/find all the spots where rollup does the wrong thing, which means extra work on our own and third party libs and added risk where test coverage is not 100% cross-platform (this issue was detected via code review, but would otherwise need specific test platforms to trigger it, which isn't always happening), but from a rollup maintenance POV I can understand your preference for annotations.

When we go and use annotations, do we have a Big Switch to turn rollup optimizations off?

Alternatively, can rollup code reduction/tree shaking be disabled globally via its config?

That would significantly help diagnosing issues which crop up due to this issue and #1771 as then we can more quickly reduce the search area while debugging/analyzing! (Same as when you've got trouble in production-stage code and kill the minifier phase to check if the problem is inside the minifier or elsewhere.)

I looked at the rollup documentation but couldn't find such a configuration option. I wonder if I overlooked it?

Thanks for the quick reply! 👍

[guybedford]

My worry with annotations as a fix here is that it sets down a path where the expectation is that if code does something that "isn't sensible" (by whatever definition deemed by Rollup), then annotations are necessary to make it actually work.

Us advanced users will be fine - but it leaves out the beginners who won't know why someone else's code broke their build, which won't be a great user experience.

Perhaps it's time to introduce "optimization levels", where things that can possibly be unsafe can be disabled on the "safe optimization" setting?

[lukastaegert]

When we go and use annotations, do we have a Big Switch to turn rollup optimizations off

The switch exists, it is treeshaking: false in the config.

But it leaves out the beginners who won't know why someone else's code broke their build, which won't be a great user experience

A heuristic is much more unpredictable in its behaviour than an annotation so the result will be that the code will work for some out of shear luck but not for others who tried to be intelligent and wrote their feature detection the wrong way.

As feature detection is a rather advanced topic anyway I would rather hope this is something to be implemented by plugin makers and not by average users.

[lukastaegert]

I guess we should document treeshaking: false a little better

[GerHobbelt]

My mistake! 😊 (https://rollupjs.org/guide/en#big-list-of-options -> Danger Zone -> treeshake)
Thanks!

Tested it and disabling treeshaking all around works fine with treeshake: true in the config file, e.g.

// rollup.config.js
import resolve from 'rollup-plugin-node-resolve';

export default {
  input: 'dummy.js',
  treeshake: false,   // 👌 
  output: [
      {
      file: 'dist/dummy-cjs.js',
      format: 'cjs'
    },
    ...etc...

[GerHobbelt]

Closing this issue as now it's become a duplicate of #1771.