-
-
Notifications
You must be signed in to change notification settings - Fork 373
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
feat: add noYodaExpression
rule
#2853
feat: add noYodaExpression
rule
#2853
Conversation
CodSpeed Performance ReportMerging #2853 will not alter performanceComparing Summary
|
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 your contribution! This is greatly appreciated ❤️ I didn't review the implementation yet. I just left two preliminary comments.
crates/biome_js_analyze/tests/specs/nursery/noYodaExpression/invalidRange.js.snap
Show resolved
Hide resolved
7c097b5
to
2ea59ef
Compare
2ea59ef
to
838694c
Compare
a065cb6
to
ef3f5e5
Compare
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.
Here are a few things we should do:
- use the try
?
operator to bubble up all thoseOption
types - all functions that need to deal with the
Option
type should returnOption
, e.g.-> bool
to-> Option<bool>
- I think the type
AnyJsLiteralExpression
has a method calledomit_parenthesis
, let's use it. I can't remember exactly the names though. However, we already solved the problem internally is_comparison_operator
should be already solved, I think there's already a function for that. If not, we should move it insteadbiome_js_syntax
let left = node.left().ok(); | ||
let right = node.right().ok(); | ||
let operator = node.operator().ok(); |
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.
Inside rules, if you return an Option
inside the run
function, always use .ok()?
when extracting/querying nodes. There's no need to keep the Option
around unless you're doing an existing check
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 wasn't familiar with the ?
operator until now and... damn, it really makes things a lot easier 👀, I tried to use it on all the possible places
let left = node.left().ok(); | ||
let right = node.right().ok(); | ||
let operator = node.operator().ok(); | ||
let parent_logical_expression = node.parent::<JsLogicalExpression>(); |
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.
Same here. Use the try operator ?
to bubble up the Option
type
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.
done, moved this one to the is_range_assertion
helper
} | ||
|
||
/// Compare literals as number if possible, compare as string otherwise | ||
fn compare_literals(left_string_value: &str, right_string_value: &str) -> bool { |
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.
Nit: compare_string_literals
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.
done!
} | ||
} | ||
|
||
fn is_wrapped_in_parenthesis(logical_expression: &JsLogicalExpression) -> bool { |
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 name of the function isn't aligned with the implementation. The implementation checks more than parenthesis. I suggest
- rename the function
- or document it
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 wasn't aware of .prev_sibling_or_token()
and .next_sibling_or_token()
, so I ended up with this hacky solution, but I just found out about it and now the implementation is more aligned with the name: it is actually looking for (
and )
tokens
} | ||
} | ||
|
||
fn is_comparison_operator(operator: &Option<JsBinaryOperator>) -> bool { |
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.
This function exists on JsBinaryOperator
.
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.
done!
) -> bool { | ||
match (left_expression, right_expression) { | ||
(Some(left), Some(right)) => { | ||
// We can't consider two call expressions equal here because the result of the expression might be different if we call it twice |
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 you add a quick example in the comment?
Do we really need this check?
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've updated the documentation of this method to explain it better why it's needed, but I had to do this check to ensure that this method will return false when there's a call expression inside the expression.
Consider this expression: if (0 <= a[b()] && a[b()] < 1) {}
We can't assume the expressions are equal because the result of the JsCallExpression
might be different in multiple calls:
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 see. I think it is a small thing. It is most likely to cause false positives. I assume you ported it from the ESLint rule? Instead, I could remove this check and just recognize this as a range. What do you think @ematipico ?
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.
Expressions can't be statically evaluated (unless they are literal), so we should skip them.
We do the same in the rule noSelfAssignment
: even though left and right call the same expressions, we can't know their result.
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.
What do you mean by skip them
?
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.
In this case if (0 <= a[b()] && a[b()] < 1) {}
the rule won't recognize the expression as a range, but 0 <= a[b()]
alone still is a yoda expression, so it will trigger the rule anyways, are you ok with that @ematipico?
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 that @ematipico means that every expression should be considered as different. For example 0 < o.p && o.p < 10
would not be recognized as a range because o.p
can give different results (potentially a getter).
Conversely, I think we could recognize all cases, including (0 <= a[b()] && a[b()] < 1)
as ranges.
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.
Conversely, I think we could recognize all cases, including
(0 <= a[b()] && a[b()] < 1)
as ranges.
I'm cool with that too actually, I'm just gonna wait for @ematipico response and I can make the change
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.
@michellocana let's accept (0 <= a[b()] && a[b()] < 1)
and I think we can merge.
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.
done! @Conaclos
577ad16
to
42921ee
Compare
42921ee
to
d79118d
Compare
/// | ||
/// ```js | ||
/// if (0 < value && value < 1) {} | ||
/// ``` |
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.
Let's put the Wikipidia link at the bottom, like this:
biome/crates/biome_js_analyze/src/lint/a11y/no_access_key.rs
Lines 34 to 37 in f685ebc
/// ## Resources | |
/// | |
/// - [WebAIM: Keyboard Accessibility - Accesskey](https://webaim.org/techniques/keyboard/accesskey#spec) | |
/// - [MDN `accesskey` documentation](https://developer.mozilla.org/docs/Web/HTML/Global_attributes/accesskey) |
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.
done!
372e51e
to
49a63d3
Compare
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.
There's some unsafe code to change, mainly the use of unwrap
let new_left = if has_missing_left_trivia { | ||
clone_with_trivia(right, &left_leading_trivia, &left_trailing_trivia) | ||
.with_leading_trivia_pieces(whitespace.clone()) | ||
.unwrap() |
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.
Don't unwrap
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.
done, removed all .unwrap()
calls, but .unwrap_or_default()
is ok, right?
let new_right = if has_missing_right_trivia { | ||
clone_with_trivia(left, &right_leading_trivia, &right_trailing_trivia) | ||
.append_trivia_pieces(whitespace.clone()) | ||
.unwrap() |
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.
Don't unwrap
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.
done
e3dc624
to
f076688
Compare
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 thought I already suggested using the try operator ?
to bubble up Option
. We shouldn't use unwrap_or_default
because it forces the to code still run.
The reason why we turn SyntaxResult
to Option
is because the CST might contain bogus nodes or invalid syntax. If a node has invalid syntax, we just stop the analysis.
&& is_literal_expression(&left).unwrap_or_default() | ||
&& !is_literal_expression(&right).unwrap_or_default() | ||
&& !is_range_assertion(node).unwrap_or_default(); |
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.
&& is_literal_expression(&left).unwrap_or_default() | |
&& !is_literal_expression(&right).unwrap_or_default() | |
&& !is_range_assertion(node).unwrap_or_default(); | |
&& is_literal_expression(&left)? | |
&& !is_literal_expression(&right)? | |
&& !is_range_assertion(node)?; |
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 was able to remove all but this call: !is_range_assertion(node).unwrap_or_default()
, this helper might return None
but the code should keep running in that case, is there another approach I could take here? I can convert it to return bool
instead of Option<bool>
if you think it's better
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.
Yeah, in this case, it makes sense to return bool
. Some documentation could help explain why
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 method is now returning bool
, and also, I did my best to explain in details what this method does, let me know what you think :)
&& (is_inside_range_assertion(operator, &left, &right).unwrap_or_default() | ||
|| is_outside_range_assertion(operator, &left, &right).unwrap_or_default()) |
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.
&& (is_inside_range_assertion(operator, &left, &right).unwrap_or_default() | |
|| is_outside_range_assertion(operator, &left, &right).unwrap_or_default()) | |
&& (is_inside_range_assertion(operator, &left, &right)? | |
|| is_outside_range_assertion(operator, &left, &right)?) |
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.
done!
AnyJsExpression::JsUnaryExpression(unary) => match unary.operator() { | ||
Ok(JsUnaryOperator::Minus) => { | ||
let argument = unary.argument().ok()?.text(); | ||
let is_numeric_literal = unary.is_signed_numeric_literal().unwrap_or_default(); |
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.
let is_numeric_literal = unary.is_signed_numeric_literal().unwrap_or_default(); | |
let is_numeric_literal = unary.is_signed_numeric_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.
done!
Co-authored-by: Emanuele Stoppa <my.burning@gmail.com>
Co-authored-by: Emanuele Stoppa <my.burning@gmail.com>
Co-authored-by: Emanuele Stoppa <my.burning@gmail.com>
Co-authored-by: Emanuele Stoppa <my.burning@gmail.com>
… and make it return bool instead of Option<bool>
b36dc9c
to
d42868e
Compare
Summary
Implementation of the yoda ESLint rule in Biome.
Closes #2743
if ("red" == value) {}
if (value == "red") {}
if (true === value) {}
if (value === true) {}
if (5 != value) {}
if (value != 5) {}
if (5n != value) {}
if (value != 5n) {}
if (null !== value) {}
if (value !== null) {}
if ("red" <= value) {}
if (value >= "red") {}
if (`red` <= value) {}
if (value >= `red`) {}
if (`red` <= `${foo}`) {}
if (`${foo}` >= `red`) {}
if (`red` <= `${"red"}`) {}
if (`${"red"}` >= `red`) {}
if (true >= value) {}
if (value <= true) {}
I'm not entirely sure the error messages are the best it could be, so I'm open to suggestions :)
Test Plan
Added snapshots, for valid and invalid cases according to the yoda ESLint tests.