Update: consider env in no-implied-eval (fixes #12733)

[yeonjuan]

What is the purpose of this pull request? (put an "X" next to item)

[ ] Documentation update
[ x ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)

Is there anything you'd like reviewers to focus on?
fix #12733
For it, I have referred to the way no-eval does. And this change will make globalThis support easier.

[aladdin-add]

LGTM, thanks! 💯

[kaicataldo]

Thanks for working on this! Just a few comments/suggestions. Thanks for adding in all these tests!

[kaicataldo]

I wonder about some other cases that can result in string coercion like:

String(val);
val.toString();
JSON.stringify(val);

Is there a way to reliably enforce these cases? We should be able to check if they're the globals for String and JSON, but I don't know if X.prototype.toString() can be reliably checked (though it might be a fair assumption to assume it returns a string, given the name).

[yeonjuan]

@kaicataldo
I'll try but I'm not sure to do that. Can I work on that in another PR?

[kaicataldo]

I think it would be fine to do that, since this does already improve the rule. Let's see what others have to say.

[mdjermanovic]

I'd also agree that this could be a nice future improvement.

Maybe we could also use getstaticvalue to catch code like:

var s = "alert('Hi!');";
setTimeout(s, 100);

And perhaps add self to the list of global objects.

[kaicataldo]

Thanks for working on this!

Now that I've thought about this some more, I actually question whether this rule really belongs in core or not. We have decided against many other proposals because they're not truly enforceable without type information, and this is no different. Circumventing the rule is as simple assigning the string to be evaled to a variable first.

That being said, I do think we should land this since it improves the existing rule 👍

[mdjermanovic]

The path for globals should check somewhere if it's really a CallExpression.

When it isn't, the rule crashes at the end on destructuring undefined arguments, e.g.:

/*eslint no-implied-eval: "error"*/
/* eslint-env browser */

foo = window.setTimeout;

[yeonjuan]

The path for globals should check somewhere if it's really a CallExpression. When it isn't, the rule crashes at the end on destructuring undefined arguments, e.g.:

Confirmed! I really thank you for the review. I changed it and added some tests for ensuring it.

Non-blocker for this PR, could this be simplified with: so function isSpecifiedConstant wouldn't be necessary.

It looks better. Thanks! I changed it

[mdjermanovic]

Some thoughts:

• After this PR, the rule will also check global.* (e.g., global.setTimeout("alert('Hi!');", 100);), which wasn't doing before? If those are new warnings, perhaps this should be labeled at least as a semver-minor Update: (I guess it isn't a breaking change, like globalThis?).
• If we are already adding global objects, maybe we could add self as well?
• The rule should probably check whether the global object is "redeclared" in the global scope:

/*eslint no-implied-eval: "error"*/
/* eslint-env browser */

let window;

window.setTimeout("alert('Hi!');", 100); // reports error in this PR

This would be no error for almost all other rules that track window.* (#12774): namely no-alert and all rules that use ReferenceTracker. Only no-eval reports error in such cases, I think it should be fixed there as well (I can work on that). (edit: no-eval is intentionally very different from other rules).

• The rule should probably account for shadowing setTimeout and others:

/*eslint no-implied-eval: "error"*/

function f(setTimeout) {
  setTimeout("alert('Hi!');", 100); // reports error in this PR
}

This could be also done with global references, but it would stop reporting disabled globals, so maybe it's okay for the start to just check whether it is a reference to a declared variable (rules are inconsistent on this matter).

[mdjermanovic]

I think we should fix just this in this PR, I believe it's related to the subject of the PR:

/*eslint no-implied-eval: "error"*/
/* eslint-env browser */

let window;

window.setTimeout("alert('Hi!');", 100); // reports error in this PR

[mdjermanovic]

LGTM, thanks!

[yeonjuan]

@kaicataldo
I added checking static string value. 8776e59 :)
friendly ping for the review :)

[mdjermanovic]

LGTM, thanks!

[btmills]

Just a quick typo suggestion, then this LGTM! Thank you for going through all the iteration on this PR.

[btmills]

LGTM, thanks!

[kaicataldo]

Thank you for working on this!