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: fix false positive/negative of yoda rule (fixes #7676) #7695
Conversation
@mysticatea, thanks for your PR! By analyzing the history of the files in this pull request, we identified @btmills, @vitorbal and @not-an-aardvark to be potential reviewers. |
LGTM |
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.
Could we have a test which checks that 0 <= a.b && a["b"] <= 100
is valid with exceptRange, if one doesn't exist?
LGTM |
@platinumazure Thank you! |
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 the one request for the JSDoc comment, otherwise LGTM. Thanks @mysticatea!
@@ -54,13 +54,15 @@ function looksLikeLiteral(node) { | |||
/** | |||
* Attempts to derive a Literal node from nodes that are treated like literals. | |||
* @param {ASTNode} node Node to normalize. | |||
* @param {number} [defaultValue] The default value to be returned if the node | |||
* is not a Literal. | |||
* @returns {ASTNode} The original node if the node is already a Literal, or a | |||
* normalized Literal node with the negative number as the | |||
* value if the node represents a negative number literal, | |||
* otherwise null if the node cannot be converted to a | |||
* normalized literal. |
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.
Can you update the return value description to note when it will return the default value?
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, I updated the JSDoc comment.
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!
LGTM |
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 @mysticatea!
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!
What is the purpose of this pull request? (put an "X" next to item)
[X] Bug fix (template)
See #7676 for the template.
What changes did you make? (Give an overview)
The
yoda
rule has a false positive and a false negative.This PR fixes those bugs.
c62b847 fixed the false negative about computed properties in the range check.
The
yoda
rule has been handlinga.b
anda[b]
as the same then it determines the condition is a range check wrongly. As the result, it should warn the0 <= a.b
but does not warn it. After this PR is applied, theyoda
rule will warn the0 <= a.b
correctly. (semver-minor)bf9982f fixed the false positive about variables in the range check.
The
yoda
rule has not been handling0 <= index && index < list.length
as a range check wrongly. As the result, it should not warn0 <= index
but warns it. After this PR is applied, theyoda
rule will ignore the0 <= index && index < list.length
correctly. (semver-patch)Is there anything you'd like reviewers to focus on?
Nothing in particular.