eval() causing warning with Rollup / ESBuild

[ChrisCrossCrash]

A Rollup package that I'm creating depends on three-stdlib, which depends on chevrotain. When I try to build my project, I get this warning:

PS C:\MyProjects\Project> yarn build
yarn run v1.22.15
$ rollup -c -m

src/index.ts → dist/cjs/index.js, dist/esm/index.js...
(!) Use of eval is strongly discouraged
https://rollupjs.org/guide/en/#avoiding-eval
node_modules\@chevrotain\utils\lib\src\api.js
456:     /* istanbul ignore next */
457:     // tslint:disable-next-line
458:     eval(toBecomeFast);
         ^
459: }
460: exports.toFastProperties = toFastProperties;
created dist/cjs/index.js, dist/esm/index.js in 22.4s

The documentation linked to in the warning offers a couple solutions. Here's one:

Simply 'copying' eval provides you with a function that does exactly the same thing, but which runs in the global scope rather than the local one:

var eval2 = eval;

(function () {
  var foo = 42;
  eval('console.log("with eval:",foo)'); // logs 'with eval: 42'
  eval2('console.log("with eval2:",foo)'); // throws ReferenceError
})();

Is this is something that could be implemented with Cevrotain to prevent this warning and related problems with Rollup?

Thank you for your help on this issue and your work on this package :)

[bd82]

Hello.

The eval call is actually unreachable code used as a kind of voodoo magic to force V8 engine to optimize the base parser prototype. So it won't have any adverse effects during runtime.

• chevrotain/packages/utils/src/to-fast-properties.ts

  Lines 13 to 22 in 2dce611

  	// help V8 understand this is a "real" prototype by actually using
  	// the fake instance.
  	fakeAccess()
  	fakeAccess()
  	return toBecomeFast
  	// Eval prevents optimization of this method (even though this is dead code)
  	/* istanbul ignore next */
  	// tslint:disable-next-line
  	eval(toBecomeFast)

afaik Rollup does not seem to have a way to ignore a specific warning at a specific location, e.g:

• ` /* eslint-ignore-next-line unused-var */

So it seems like you would need to use the onwarn handler to ignore the warning

• https://rollupjs.org/guide/en/#onwarn

I could try to modify the code so it won't trigger a warning, but that is such strange voodoo magic
that I am hesitant to "mess" with it.
Also it seems to me that rollup should have a better way to "ignore" specific issues via code comments...

This discussion also makes me wonder if I should test newer implementations of this "fastProps" magic, e.g:

• https://github.com/sindresorhus/to-fast-properties/blob/main/index.js
• this implementation does not use eval so that may be an advantage.

[ChrisCrossCrash]

Thanks @bd82 for the quick reply and for your care in considering this issue.

I've decided to use onwarn in my rollup.config.js as a workaround:

// rollup.config.js

export default {
  ...
  onwarn(warning, warn) {
    // skip `EVAL``warnings related to Chevrotain
    // https://github.com/Chevrotain/chevrotain/issues/1760
    if (warning.code === 'EVAL' && warning.id?.includes('chevrotain')) return

    // Use default behavior for everything else
    warn(warning)
  },
}

I understood that the code was unreachable, but I was concerned that using eval would cause Rollup to opt-out of minifying the declaration names (variables, functions, etc.). Looking at the minified code in my /dist/, folder, it doesn't seem like that's the case.

Also it seems to me that rollup should have a better way to "ignore" specific issues via code comments...

I agree that would be very handy for this!

This discussion also makes me wonder if I should test newer implementations of this "fastProps" magic

If you think it is better, and it would fix this issue, then I fully support it!

[bd82]

I did a mini prototype with https://github.com/sindresorhus/to-fast-properties/
but it caused issues in the bundling process as sindresorhus's packages are now ESM only.
and potentially a minor performance regression (requires more benchmarking to evaluate).

I will close this issue for now, I believe it should be revisited once Chevrotain itself goes ESM only as well
and the integration of https://github.com/sindresorhus/to-fast-properties/ would be pain-free

[mattbishop]

I’m suggesting copying the code actually.

[bd82]

I’m suggesting copying the code actually.

That is a good idea, but I don't want to mess with copying of the license/copyright notice just for 31 LOC bruto. :(
As otherwise it would be a license violation to copy the code.

And I am not sure what it means if the source code is partially MIT and partially Apache

[arcasoy]

I've recently come across this warning using Rollup as well. For the time being, considering the conversation here that mentions it is unlikely to be a threat since it is unreachable code.

That being said, I recently modified my local version of the to-fast-properties.js file using the suggestions provided here: https://rollupjs.org/troubleshooting/#avoiding-eval, creating a global scoped eval2 from the This removed the warning, but I am unsure if this has other unwanted effects.

Is this a potential change that could remove the warning for others without changing the operation of this package?

[bd82]

Is this a potential change that could remove the warning for others without changing the operation of this package?

Hello @arcasoy

The purpose of this "eval" is as a flag to V8 engine and without it performance is impacted.

So I would recommend testing two scenarios with the local performance benchmark

1. Removing the to-fast-properties completely and evaluating if this flag is still needed to help V8 achieve max performance.
2. Upgrading the to-fast-properties to use var eval2 = eval and evaluating if the performance remains the same (e.g does the flag still work).

I can also think of a third option, The bundling tools are static analysis tools are have limited capability to "understand" complex expressions. so what would happen if we change the code to access the eval function in a more dynamic manner, e.g:

• `this["e" + "val"]
• `this["\u0065" + "val]

Perhaps there is a gap between the "understanding" of V8 and rollup's parser that could be "exploited" to disable the warning while keeping the functionality.
Although the option described in rollup's troubleshooting seems better (if it works).

[bd82]

There is a guide in esbuild docs on avoiding direct eval.
Perhaps it will also solve the problem will rollup.

• https://esbuild.github.io/content-types/#direct-eval

[bd82]

I've applied the pattern suggested by esbuild's docs and it solved the warnning on esbuild.
I assume it will also solve the issue with rollup as it seems to be the same problem.

[bd82]

No performance regressions were measured...