feat(lint): implement a noGlobalEval rule

[yo-iwamoto]

Summary

Impletment correctness/noGlobalEval
fixes #1088

Test Plan

All existing tests has passed.
wip: Added snapshot tests has passed.

Problem

In contrast to the Eslint rule, I think we should not warn against local variables named eval.
In other words, teh following code should be valid:

  function f(eval) {
    eval("var a = 0");
  };

Seeing the error output in valid.js.snap, it seems that biome_js_syntax::global_identifier, which was suggested to use for this issue, determines local scope eval variable as a global_identifier. I have no idea why this doesn't work correctly.

I looked into it a bit and it appears that in strict mode JavaScript, eval is treated like a reserved word, and the usage above does not seem to work.
So it might be better to drop the implementation that allows a variable named eval in the local scope. What do you think? If that's ok with you, I will update valid.js and the snapshot.

[netlify]

✅ Deploy Preview for biomejs canceled.

Name	Link
🔨 Latest commit	e57c0ba
🔍 Latest deploy log	https://app.netlify.com/sites/biomejs/deploys/6596ab357256f50008c54620

[yo-iwamoto]

@nissy-dev
Since a week ago, I've spent several days attempting to modify my implementation to pass all ESLint test cases. However, I'm finding the complexity and scope of the required changes to be beyond my current abilities.

For instance, the initial implementation couldn't detect cases where eval was being used with this. or window., and I struggled to come up with an efficient implementation to catch these. Additionally, ensuring that the diagnostic highlights are appropriate depending on the violation type was challenging for me.

I'm concerned that my continued attempts might delay the progress of this project, and I believe it would be more beneficial for someone with more expertise to take over.
I apologize for any inconvenience caused and for potentially delaying the closure of this issue. I’m grateful for the learning opportunity this has provided me. So could I please be unassigned from this issue?😢

[Conaclos]

Hi @you-5805, thanks for letting us know the obstacles you encounter.

Do you think we could ship a minimal and consistent implementation? "Complex" tests could be commented for the time being and addressed in another PR.

By the way, I don't to which tests you refer to. Sometimes we decide to diverge from the ESLint rule when it makes sense.

[yo-iwamoto]

@Conaclos
Thanks for your comment.

By the way, I don't to which tests you refer to. Sometimes we decide to diverge from the ESLint rule when it makes sense.

First, regarding the test cases for the ESLint rule, there are currently two patterns that cannot pass (can't trigger an error) with the current implementation.
One is the access through global as a property, assuming the Node.js environment, for example, global.eval(). The implementation of global_identifier covers access through globalThis and window, but not global. I am undecided whether it is appropriate to simply add global here.https://github.com/biomejs/biome/blob/main/crates/biome_js_syntax/src/expr_ext.rs#L1226

The second pattern is the invocation of eval using the behavior where (0, x) evaluates to x, such as in (0, eval)('foo'). This should match JsCallExpression, but I am still considering how to implement a static determination that the callee is global eval.

Additionally, the format of the diagnostics is not ideal as it is.
Currently, node.range() is always specified as the span for diagnostics, but when eval is passed as an argument, it should be sufficient to highlight the expression of the argument, and in the case of a variable declaration, the right-hand side.
For instance, when eval is passed as an argument, the output would be as follows:

invalid.js:47:1 lint/nursery/noGlobalEval ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

  ! eval() can be harmful.
  
    45 │ })(eval);
    46 │ 
  > 47 │ window.eval("foo");
       │ ^^^^^^^^^^^^^^^^^^
    48 │ 
    49 │ window.window.eval("foo");

And the output for a variable declaration would be:

invalid.js:12:1 lint/nursery/noGlobalEval ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

  ! eval() can be harmful.
  
    10 │ (0, window["eval"])("foo");
    11 │ 
  > 12 │ var EVAL = eval;
       │ ^^^^^^^^^^^^^^^
    13 │ EVAL("foo");
    14 │ 

[Conaclos]

@you-5805
Sorry, I missed your reply.

I think you are setting the expectation too high.
In several rules we implemented we decided to slightly diverge from ESLint and thus to not support some checks.

One is the access through global as a property, assuming the Node.js environment, for example, global.eval(). The implementation of global_identifier covers access through globalThis and window, but not global.

global was the candidate name for globalThis. I think we should not support global. We can comment or remove all tests using global.

The second pattern is the invocation of eval using the behavior where (0, x) evaluates to x, such as in (0, eval)('foo'). This should match JsCallExpression, but I am still considering how to implement a static determination that the callee is global eval.

This is a specific case that we can implement in another PR.

Similarly, eval aliasing (such as const e = eval; e("")) could be left to another PR.

Additionally, the format of the diagnostics is not ideal as it is.

We could only highlight eval.

If you prefer, I can take the PR :)

[yo-iwamoto]

Following four test cases, eval is bound to local scope variables, and the global eval is not being called. Therefore, the error for noGlobalEval should not be displayed. I understand that if eval is bound to a local scope variable, it should be treated as None with the following process, but it seems not to work as expected.

model.binding(&reference).is_none().then_some(())

[Conaclos]

I think the bug is coming from the semantic model. I will investigate the case.

[yo-iwamoto]

@Conaclos
Thanks to your advice, most of the problems have been resolved. There's just one remaining TODO: #1133 (comment).

If possible, I would like to proceed with this as a TODO, and go ahead with the review and merge of this PR.
Is it okay?

[Conaclos]

Thanks to your advice, most of the problems have been resolved. There's just one remaining TODO: #1133 (comment).

If possible, I would like to proceed with this as a TODO, and go ahead with the review and merge of this PR. Is it okay?

Thanks for your contribution! I improved the docs and the diagnostics.
I will investigate the TODO issues. I think this is coming from the semantic model of Biome.

For now, we can ship the rule ❤️

[codspeed-hq]

CodSpeed Performance Report

Merging #1133 will not alter performance

_{⚠️ No base runs were found}

_{Falling back to comparing you-5805:lint/noGlobalEval (e57c0ba) with main (b88f8b7)}

Summary

✅ 93 untouched benchmarks