feat: add `noYodaExpression` rule #2853

michellocana · 2024-05-14T13:48:08Z

Summary

Implementation of the yoda ESLint rule in Biome.
Closes #2743

Invalid case	Fix suggestion
`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.

codspeed-hq · 2024-05-14T14:26:32Z

CodSpeed Performance Report

Merging #2853 will not alter performance

_{Comparing michellocana:feat/no-yoda-expression (8e462f2) with michellocana:feat/no-yoda-expression (e6f9787)}

Summary

✅ 92 untouched benchmarks

Conaclos

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/src/lint/nursery/no_yoda_expression.rs

crates/biome_js_analyze/tests/specs/nursery/noYodaExpression/invalidRange.js.snap

crates/biome_js_analyze/src/lint/nursery/no_yoda_expression.rs

ematipico

Here are a few things we should do:

use the try ? operator to bubble up all those Option types
all functions that need to deal with the Option type should return Option
, e.g. -> bool to -> Option<bool>
I think the type AnyJsLiteralExpression has a method called omit_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 instead biome_js_syntax

ematipico · 2024-05-18T05:32:52Z

crates/biome_js_analyze/src/lint/nursery/no_yoda_expression.rs

+        let left = node.left().ok();
+        let right = node.right().ok();
+        let operator = node.operator().ok();


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

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

ematipico · 2024-05-18T05:34:21Z

crates/biome_js_analyze/src/lint/nursery/no_yoda_expression.rs

+        let left = node.left().ok();
+        let right = node.right().ok();
+        let operator = node.operator().ok();
+        let parent_logical_expression = node.parent::<JsLogicalExpression>();


Same here. Use the try operator ? to bubble up the Option type

done, moved this one to the is_range_assertion helper

ematipico · 2024-05-18T05:37:51Z

crates/biome_js_analyze/src/lint/nursery/no_yoda_expression.rs

+}
+
+/// Compare literals as number if possible, compare as string otherwise
+fn compare_literals(left_string_value: &str, right_string_value: &str) -> bool {


Nit: compare_string_literals

ematipico · 2024-05-18T05:41:07Z

crates/biome_js_analyze/src/lint/nursery/no_yoda_expression.rs

+    }
+}
+
+fn is_wrapped_in_parenthesis(logical_expression: &JsLogicalExpression) -> bool {


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

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

crates/biome_js_analyze/src/lint/nursery/no_yoda_expression.rs

Conaclos · 2024-05-18T11:54:11Z

crates/biome_js_analyze/src/lint/nursery/no_yoda_expression.rs

+    }
+}
+
+fn is_comparison_operator(operator: &Option<JsBinaryOperator>) -> bool {


This function exists on JsBinaryOperator.

Conaclos · 2024-05-18T11:59:24Z

crates/biome_js_analyze/src/lint/nursery/no_yoda_expression.rs

+) -> 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


Could you add a quick example in the comment?

Do we really need this check?

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:

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 ?

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.

What do you mean by skip them?

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?

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.

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

@michellocana let's accept (0 <= a[b()] && a[b()] < 1) and I think we can merge.

done! @Conaclos

CHANGELOG.md

crates/biome_js_analyze/src/lint/nursery/no_yoda_expression.rs

ematipico · 2024-05-20T11:28:20Z

crates/biome_js_analyze/src/lint/nursery/no_yoda_expression.rs

+    ///
+    /// ```js
+    /// if (0 < value && value < 1) {}
+    /// ```


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)

ematipico

There's some unsafe code to change, mainly the use of unwrap

ematipico · 2024-05-21T11:41:56Z

crates/biome_js_analyze/src/lint/nursery/no_yoda_expression.rs

+        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()


Don't unwrap

done, removed all .unwrap() calls, but .unwrap_or_default() is ok, right?

ematipico · 2024-05-21T11:42:05Z

crates/biome_js_analyze/src/lint/nursery/no_yoda_expression.rs

+        let new_right = if has_missing_right_trivia {
+            clone_with_trivia(left, &right_leading_trivia, &right_trailing_trivia)
+                .append_trivia_pieces(whitespace.clone())
+                .unwrap()


Don't unwrap

ematipico

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.

ematipico · 2024-05-21T12:52:02Z

crates/biome_js_analyze/src/lint/nursery/no_yoda_expression.rs

+            && is_literal_expression(&left).unwrap_or_default()
+            && !is_literal_expression(&right).unwrap_or_default()
+            && !is_range_assertion(node).unwrap_or_default();


Suggested change

&& 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)?;

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

Yeah, in this case, it makes sense to return bool. Some documentation could help explain why

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 :)

ematipico · 2024-05-21T12:52:47Z

crates/biome_js_analyze/src/lint/nursery/no_yoda_expression.rs

+                && (is_inside_range_assertion(operator, &left, &right).unwrap_or_default()
+                    || is_outside_range_assertion(operator, &left, &right).unwrap_or_default())


Suggested change

&& (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)?)

ematipico · 2024-05-21T12:53:02Z

crates/biome_js_analyze/src/lint/nursery/no_yoda_expression.rs

+        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();


Suggested change

let is_numeric_literal = unary.is_signed_numeric_literal().unwrap_or_default();

let is_numeric_literal = unary.is_signed_numeric_literal()?;

…escription

…thesis helper

…ession

…Expression

…ier helper

Co-authored-by: Emanuele Stoppa <my.burning@gmail.com>

… and make it return bool instead of Option<bool>

github-actions bot added A-CLI Area: CLI A-Project Area: project A-Linter Area: linter L-JavaScript Language: JavaScript and super languages A-Diagnostic Area: diagnostocis A-Changelog Area: changelog labels May 14, 2024

Conaclos reviewed May 14, 2024

View reviewed changes

crates/biome_js_analyze/src/lint/nursery/no_yoda_expression.rs Show resolved Hide resolved

crates/biome_js_analyze/tests/specs/nursery/noYodaExpression/invalidRange.js.snap Show resolved Hide resolved

michellocana force-pushed the feat/no-yoda-expression branch from 7c097b5 to 2ea59ef Compare May 15, 2024 11:59

Conaclos reviewed May 15, 2024

View reviewed changes

crates/biome_js_analyze/src/lint/nursery/no_yoda_expression.rs Show resolved Hide resolved

crates/biome_js_analyze/src/lint/nursery/no_yoda_expression.rs Show resolved Hide resolved

michellocana force-pushed the feat/no-yoda-expression branch from 2ea59ef to 838694c Compare May 15, 2024 14:25

michellocana requested a review from Conaclos May 15, 2024 14:25

michellocana force-pushed the feat/no-yoda-expression branch 2 times, most recently from a065cb6 to ef3f5e5 Compare May 17, 2024 21:56

ematipico requested changes May 18, 2024

View reviewed changes

Conaclos reviewed May 18, 2024

View reviewed changes

michellocana force-pushed the feat/no-yoda-expression branch from 577ad16 to 42921ee Compare May 18, 2024 22:47

michellocana requested review from Conaclos and ematipico May 18, 2024 22:49

michellocana force-pushed the feat/no-yoda-expression branch from 42921ee to d79118d Compare May 20, 2024 11:19

ematipico approved these changes May 20, 2024

View reviewed changes

michellocana force-pushed the feat/no-yoda-expression branch 2 times, most recently from 372e51e to 49a63d3 Compare May 21, 2024 11:32

ematipico requested changes May 21, 2024

View reviewed changes

Conaclos approved these changes May 21, 2024

View reviewed changes

michellocana force-pushed the feat/no-yoda-expression branch 2 times, most recently from e3dc624 to f076688 Compare May 21, 2024 12:22

ematipico reviewed May 21, 2024

View reviewed changes

michellocana added 2 commits May 21, 2024 12:04

feat(lint/noYodaExpression): add basic rule implementation

a2b1d95

feat(lint/noYodaExpression): allow basic range assertions

78918b5

michellocana and others added 25 commits May 21, 2024 12:04

feat(lint/noYodaExpression): add rule sources and fix_kind

f3bee0c

feat(lint/noYodaExpression): improve error message

8c95d95

feat(lint/noYodaExpression): update CHANGELOG.md

d5b6986

feat(lint/noYodaExpression): add while expressions tests

d61e7b8

feat(lint/noYodaExpression): improve rule description

6db03de

feat(lint/noYodaExpression): add range expression exception on rule d…

2d5d90e

…escription

feat(lint/noYodaExpression): fix linter issue

91a2206

feat(lint/noYodaExpression): run gen-lint

ef40b4a

feat(lint/noYodaExpression): refact code to use the ? operator

d5c878f

feat(lint/noYodaExpression): rename method to compare_string_literals

7534e38

feat(lint/noYodaExpression): improve the logic of is_wrapped_in_paren…

6d9f238

…thesis helper

feat(lint/noYodaExpression): improve string extraction from AnyJsExpr…

8723cba

…ession

feat(lint/noYodaExpression): use is_comparison_operator from JsBinary…

0999185

…Expression

feat(lint/noYodaExpression): improve documentation of is_same_identif…

9b98a37

…ier helper

feat(lint/noYodaExpression): fix cargo lint issues

9c1041f

feat(lint/noYodaExpression): update CHANGELOG.md

3f64187

Co-authored-by: Emanuele Stoppa <my.burning@gmail.com>

feat(lint/noYodaExpression): update rule definition

5691ed4

Co-authored-by: Emanuele Stoppa <my.burning@gmail.com>

feat(lint/noYodaExpression): update rule definition

9ba05fc

Co-authored-by: Emanuele Stoppa <my.burning@gmail.com>

feat(lint/noYodaExpression): update rule definition

9ef4091

Co-authored-by: Emanuele Stoppa <my.burning@gmail.com>

feat(lint/noYodaExpression): add wikipedia definition link

42ffa1c

feat(lint/noYodaExpression): accept ranges with call expressions

8f87778

feat(lint/noYodaExpression): remove unsafe unwrap

7f6d445

feat(lint/noYodaExpression): run gen-lint

6f99d1d

feat(lint/noYodaExpression): remove .unwrap_or_default() calls

65e68d8

feat(lint/noYodaExpression): improve is_range_assertion documentation…

d42868e

… and make it return bool instead of Option<bool>

michellocana force-pushed the feat/no-yoda-expression branch from b36dc9c to d42868e Compare May 21, 2024 15:05

ematipico approved these changes May 21, 2024

View reviewed changes

ematipico added 2 commits May 22, 2024 09:21

Merge remote-tracking branch 'origin/main' into feat/no-yoda-expression

e6f9787

re run codegen

8e462f2

ematipico merged commit a51fa60 into biomejs:main May 22, 2024
3 checks passed

	/// ## 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)

		&& (is_inside_range_assertion(operator, &left, &right).unwrap_or_default()
		\|\| is_outside_range_assertion(operator, &left, &right).unwrap_or_default())

	let is_numeric_literal = unary.is_signed_numeric_literal().unwrap_or_default();
	let is_numeric_literal = unary.is_signed_numeric_literal()?;

feat: add noYodaExpression rule #2853

feat: add noYodaExpression rule #2853

Conversation

michellocana commented May 14, 2024

Summary

Test Plan

codspeed-hq bot commented May 14, 2024 • edited

Merging #2853 will not alter performance

Summary

Conaclos left a comment • edited

Choose a reason for hiding this comment

ematipico left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Conaclos May 20, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ematipico left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ematipico left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feat: add `noYodaExpression` rule #2853

feat: add `noYodaExpression` rule #2853

codspeed-hq bot commented May 14, 2024 •

edited

Conaclos left a comment •

edited

Conaclos May 20, 2024 •

edited