feat: no useless undefined initialization #2591

lutaok · 2024-04-24T21:28:59Z

Summary

Closes #2533.
Implements eslint no-undef-init rule with similar diagnostic and quick action.

EDIT: After some explanation on the quick action behaviour, I decided to make it diverge from Eslint's behaviour.
My implementation removes any inline comments attached to the initialization clause (variable or value)
I added some line of comments to document it and a test case that shows this behaviour.

Test Plan

Added snapshots, valid and invalid cases.

codspeed-hq · 2024-04-24T22:01:42Z

CodSpeed Performance Report

Merging #2591 will not alter performance

_{Comparing lutaok:feature/no-useless-undefined-initialization (0080714) with main (ce4979d)}

Summary

✅ 90 untouched benchmarks

ematipico · 2024-04-25T06:34:02Z

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

+    /// A variable that is declared and not initialized to any value automatically gets the value of undefined.
+    /// It’s considered a best practice to avoid initializing variables to undefined.
+    ///
+    /// Source: https://eslint.org/docs/latest/rules/no-undef-init


This line isn't needed, the code automation takes care of it

Do you mean the whole block you highlighted or just the Source line?

I think Ema means the source line.

Okay thanks, I'll remove it!

ematipico · 2024-04-25T06:34:34Z

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

+use crate::JsRuleAction;
+
+declare_rule! {
+    /// Disallow initializing variables to undefined.


Suggested change

/// Disallow initializing variables to undefined.

/// Disallow initializing variables to `undefined`.

ematipico · 2024-04-25T06:34:49Z

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

+    /// A variable that is declared and not initialized to any value automatically gets the value of undefined.
+    /// It’s considered a best practice to avoid initializing variables to undefined.


Suggested change

/// A variable that is declared and not initialized to any value automatically gets the value of undefined.

/// It’s considered a best practice to avoid initializing variables to undefined.

/// A variable that is declared and not initialized to any value automatically gets the value of `undefined`.

/// It’s considered a best practice to avoid initializing variables to `undefined`.

ematipico · 2024-04-25T06:36:52Z

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

+    /// ```js,expect_diagnostic
+    /// var a = undefined;
+    ///
+    /// let b = undefined, c = 1, d = undefined;
+    ///
+    /// for (let i = 0; i < 100; i++) {
+    /// 	let i = undefined;
+    /// }
+    /// ```


I am not sure if you read the contribution guide. Each snippet must emit only one diagnostic.

Suggested change

/// ```js,expect_diagnostic

/// var a = undefined;

///

/// let b = undefined, c = 1, d = undefined;

///

/// for (let i = 0; i < 100; i++) {

/// let i = undefined;

/// }

/// ```

/// ```js,expect_diagnostic

/// var a = undefined;

/// ```

/// ```js,expect_diagnostic

/// let b = undefined, c = 1, d = undefined;

/// ```

/// ```js,expect_diagnostic

/// for (let i = 0; i < 100; i++) {

/// let i = undefined;

/// }

/// ```

ematipico · 2024-04-25T06:39:43Z

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

+            let decl = match declarator {
+                Ok(item) => item,
+                _ => continue,
+            };
+
+            let initializer = match decl.initializer() {
+                Some(init) => init,
+                _ => continue,
+            };
+
+            let expression = match initializer.expression() {
+                Ok(expr) => expr,
+                _ => continue,
+            };


All of this can be chained using a combination of ok(), .and_then() and .map().
It that's not possible, you can use the let-else syntax:

let Ok(decl) = declarator else { continue }

ematipico · 2024-04-25T06:40:57Z

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

+            rule_category!(),
+            state.1,
+            markup! {
+                "It's not necessary to initialize "<Emphasis>{state.0}</Emphasis>" to undefined"


Suggested change

"It's not necessary to initialize "<Emphasis>{state.0}</Emphasis>" to undefined"

"It's not necessary to initialize "<Emphasis>{state.0}</Emphasis>" to undefined."

ematipico · 2024-04-25T06:48:59Z

crates/biome_js_analyze/tests/specs/nursery/noUselessUndefinedInitialization/invalid.js

I am not sure what's your plan about the rule, but these cases are incomplete. We miss cases with comments attached to the tokens we're removing, e.g.:

let a = undefined/**/ ; let a = /**/undefined/**/ ; let a /**/= /**/undefined/**/ ;

The last declaration is tricky.

We need to decide what to do with these comments and where to place them. We can also choose to remove them. If we decide to remove some of them, we need to document it.

Right, my bad, these are not cases I considered unfortunately.
If I understand correctly if I decide to remove comments I should also change fix_kind into Unsafe and document it.

I read on Eslint's rule tests sources, though, that they don't auto-fix the rule if there are comments attached.
Would you like it to replicate Eslint's behaviour?

EDIT: Removing comments would also require to change the Applicability on the JsRuleAction return right?

We have a different point of view on the code actions. We think code actions should be safe/unsafe regardless of how the code is, because this gives certain and correct expectations to the user.

A code action can start as unsafe and then become safe once we are certain it covers most cases.

Now, this is a nursery rule, so we are allowed to ship a half-baked rule, and then make new PRs to improve it. My suggestion to you, is to cut short the scope of the PR and handle comments in another PR. It will be less overwhelming for now and it gives you time to think about what we want to do.

FYI, we have rules that remove comments and are safe, as long as removing comments makes sense.

EDIT: Removing comments would also require to change the Applicability on the JsRuleAction return right?

Yes, we would need to change the metadata fix_kind and Applicability

Okay I'll take your suggestion and will modify fix_kind and Applicability and I'll also add comments and a single diagnostic on this case to make it clear.
Thank you for the explanation!

ematipico · 2024-04-25T06:50:53Z

One more thing: remember to run just gen-lint to generate some missing code files. It's all written in the code contribution guide.

lutaok · 2024-04-25T07:26:17Z

One more thing: remember to run just gen-lint to generate some missing code files. It's all written in the code contribution guide.

Hi @ematipico thanks for the feedbacks, I read the contribution guide and ran just ready which should have included just gen-lint.
I'm looking forward to change what you point out and I'm sorry if I missed something from the guide.
I'm not familiar with many of these concepts nor with Rust.
Thanks again for the time you put into the review 😃

ematipico · 2024-04-26T05:46:39Z

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

+    /// It’s considered a best practice to avoid initializing variables to `undefined`.
+    ///
+    /// Please note that any inline comments attached to the initialization value or variable will be removed on auto-fix.
+    /// Please be also aware that this differs from Eslint's behaviour and we are still discussing on how to properly handle this case.


No need for this line, the website will take care of it for you

ematipico · 2024-04-26T05:48:25Z

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

+    pub NoUselessUndefinedInitialization {
+        version: "next",
+        name: "noUselessUndefinedInitialization",
+        sources: &[RuleSource::Eslint("no-undef-init")],


Since we decided to diverge, you need to add a new metadata called source_kind and mark it as Inspired. Look how other rules do it

ematipico · 2024-04-26T05:52:52Z

crates/biome_js_analyze/tests/specs/nursery/noUselessUndefinedInitialization/invalid.js.snap

+
+    2 │ 	inline2 = undefined;
+    3 │ let test1,
+  > 4 │ 	test2 = undefined;


The diagnostic is highlighting the whole assignment, but it's incorrect. The diagnostic should only align starting from =.

You can use the state of the rule to save the TextRange of the right hand side of the assignment

I replicated Eslint's behaviour on this matter (Eslint's Playground).

If you think we should diverge on this too, I'll gladly change the logic for the diagnostic!

We don't usually check their diagnostics, our "parity" is around the logic of the rule (what to trigger and what to not trigger).

Code fixes and diagnostics are our own :)

Oh okay, thanks!
I'll change it in order to make the diagnostic start from the = token!

ematipico · 2024-04-26T05:53:52Z

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

+        Some(JsRuleAction {
+            category: ActionCategory::QuickFix,
+            applicability: Applicability::MaybeIncorrect,
+            message: markup! { "Remove undefined initialization" }.to_owned(),


Suggested change

message: markup! { "Remove undefined initialization" }.to_owned(),

message: markup! { "Remove undefined initialization." }.to_owned(),

ematipico · 2024-04-26T05:57:25Z

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

+            .map(|decl| decl.ok())?
+            .and_then(|declarator| declarator.initializer())?;
+
+        let mut mutation = declarators.begin();


You shouldn't create a mutation this way.

let mut mutation = ctx.root().begin();

Can't remember the exact syntax. I suggest you look at other rules

ematipico

Thank you, @lutaok, for your contribution! ❤️

lutaok added 5 commits April 24, 2024 22:01

feat(biome_js_analyze): add no_useless_undefined_initialization rule

4e2eebd

ref: comply with linter

2395972

ref: after format

5a517f6

a8cf833

chore: sort config after format

06d67f4

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 labels Apr 24, 2024

lutaok changed the title ~~Feature/no useless undefined initialization~~ feat: no useless undefined initialization Apr 24, 2024

ematipico requested changes Apr 25, 2024

View reviewed changes

lutaok added 3 commits April 25, 2024 12:54

chore: just gen-lint run

286d9b0

ref(biome_js_analyze): more concise code

24c6a83

docs: add docs on inline comments removal

ac3780e

lutaok requested a review from ematipico April 25, 2024 11:13

Merge branch 'main' into feature/no-useless-undefined-initialization

10cb1ad

ematipico reviewed Apr 26, 2024

View reviewed changes

lutaok added 3 commits April 26, 2024 11:08

docs: add source kind metadata

ee4ab7f

fix: diagnostic start from initializer

fc7d5f9

Merge branch 'main' into feature/no-useless-undefined-initialization

0080714

lutaok requested a review from ematipico April 26, 2024 09:34

ematipico approved these changes Apr 26, 2024

View reviewed changes

ematipico merged commit a13d36d into biomejs:main Apr 26, 2024
12 checks passed

Conaclos added the A-Changelog Area: changelog label Apr 29, 2024

lutaok mentioned this pull request May 16, 2024

📎 Lint Rule noUselessUndefinedInitialization - autofixing behaviour on inline comments #2879

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: no useless undefined initialization #2591

feat: no useless undefined initialization #2591

lutaok commented Apr 24, 2024 •

edited

Loading

codspeed-hq bot commented Apr 24, 2024 •

edited

Loading

ematipico Apr 25, 2024

lutaok Apr 25, 2024

Conaclos Apr 25, 2024

lutaok Apr 25, 2024

ematipico Apr 25, 2024

ematipico Apr 25, 2024

ematipico Apr 25, 2024

ematipico Apr 25, 2024

ematipico Apr 25, 2024

ematipico Apr 25, 2024

lutaok Apr 25, 2024 •

edited

Loading

ematipico Apr 25, 2024

lutaok Apr 25, 2024

ematipico commented Apr 25, 2024

lutaok commented Apr 25, 2024

ematipico Apr 26, 2024

ematipico Apr 26, 2024

ematipico Apr 26, 2024

lutaok Apr 26, 2024

ematipico Apr 26, 2024

lutaok Apr 26, 2024

ematipico Apr 26, 2024

ematipico Apr 26, 2024

ematipico left a comment

	/// Disallow initializing variables to undefined.
	/// Disallow initializing variables to `undefined`.

		/// A variable that is declared and not initialized to any value automatically gets the value of undefined.
		/// It’s considered a best practice to avoid initializing variables to undefined.

	"It's not necessary to initialize "<Emphasis>{state.0}</Emphasis>" to undefined"
	"It's not necessary to initialize "<Emphasis>{state.0}</Emphasis>" to undefined."

	message: markup! { "Remove undefined initialization" }.to_owned(),
	message: markup! { "Remove undefined initialization." }.to_owned(),

feat: no useless undefined initialization #2591

feat: no useless undefined initialization #2591

Conversation

lutaok commented Apr 24, 2024 • edited Loading

Summary

Test Plan

codspeed-hq bot commented Apr 24, 2024 • edited Loading

Merging #2591 will not alter performance

Summary

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

lutaok Apr 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ematipico commented Apr 25, 2024

lutaok commented Apr 25, 2024

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

ematipico left a comment

Choose a reason for hiding this comment

lutaok commented Apr 24, 2024 •

edited

Loading

codspeed-hq bot commented Apr 24, 2024 •

edited

Loading

lutaok Apr 25, 2024 •

edited

Loading