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
Update: consider env in no-implied-eval (fixes #12733) #12757
Conversation
…al-env # Conflicts: # lib/rules/no-implied-eval.js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks! 💯
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this! Just a few comments/suggestions. Thanks for adding in all these tests!
) { | ||
return true; | ||
} | ||
if (node.type === "BinaryExpression" && node.operator === "+") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kaicataldo
I'll try but I'm not sure to do that. Can I work on that in another PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be fine to do that, since this does already improve the rule. Let's see what others have to say.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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 That being said, I do think we should land this since it improves the existing rule 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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;
Confirmed! I really thank you for the review. I changed it and added some tests for ensuring it.
It looks better. Thanks! I changed it |
Some thoughts:
/*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
/*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). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
@kaicataldo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a quick typo suggestion, then this LGTM! Thank you for going through all the iteration on this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for working on this!
…12757) * Fix: consider env in no-implied-eval (fixes eslint#12733) * fix typo, refactor to chaining * change to check only CallExpression, refactoring * check defs * checks callee * check static string value * check falsy - first argument * check empty arg * fix typo
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 makeglobalThis
support easier.