fix(migrate): properly handle rule removal and insertion

crates/biome_cli/src/commands/migrate.rs

@@ -8,7 +8,7 @@ use biome_service::workspace::RegisterProjectFolderParams;
 
 use super::{check_fix_incompatible_arguments, FixFileModeOptions, MigrateSubCommand};
 
-/// Handler for the "check" command of the Biome CLI
+/// Handler for the "migrate" command of the Biome CLI
 pub(crate) fn migrate(
     session: CliSession,
     cli_options: CliOptions,

crates/biome_migrate/src/analyzers/nursery_rules.rs

@@ -6,11 +6,13 @@ use biome_diagnostics::category;
 use biome_json_factory::make::{
     json_member, json_member_list, json_member_name, json_object_value, json_string_literal, token,
 };
-use biome_json_syntax::{AnyJsonValue, JsonMember, JsonMemberList, JsonRoot, T};
+use biome_json_syntax::{AnyJsonValue, JsonLanguage, JsonMember, JsonMemberList, JsonRoot, T};
 use biome_rowan::{
-    AstNode, AstNodeExt, AstSeparatedList, BatchMutationExt, TextRange, TriviaPieceKind, WalkEvent,
+    AstNode, AstNodeExt, AstSeparatedList, BatchMutationExt, SyntaxToken, TextRange,
+    TriviaPieceKind, WalkEvent,
 };
 use rustc_hash::FxHashMap;
+use std::iter::repeat;
 
 declare_migration! {
     pub(crate) NurseryRules {
@@ -23,9 +25,11 @@ declare_migration! {
 #[derive(Debug)]
 pub(crate) struct MigrateRuleState {
     /// The member of the new rule
-    nursery_rule_member: JsonMember,
+    nursery_rule: JsonMember,
     /// The member of the group where the new rule should be moved
     nursery_group: JsonMember,
+    /// The comma separator to be deleted
+    optional_separator: Option<SyntaxToken<JsonLanguage>>,
     /// The name of the new rule
     new_rule_name: &'static str,
     /// The new group name
@@ -34,7 +38,12 @@ pub(crate) struct MigrateRuleState {
 
 impl MigrateRuleState {
     fn as_rule_name_range(&self) -> TextRange {
-        self.nursery_rule_member.range()
+        let range = self.nursery_rule.range();
+        if let Some(separator) = &self.optional_separator {
+            TextRange::cover(separator.text_range(), range)
+        } else {
+            range
+        }
     }
 }
 
@@ -131,36 +140,55 @@ impl Rule for NurseryRules {
         let node = ctx.query();
         let mut rules_to_migrate = vec![];
 
-        let nursery_group = find_group_by_name(node, "nursery");
-
-        if let Some(nursery_member) = nursery_group {
-            let mut rules = FxHashMap::default();
-            for (group, (new_group, new_name)) in RULES_TO_MIGRATE {
-                rules.insert(*group, (*new_group, *new_name));
+        if let Some(nursery_group) = find_group_by_name(node, "nursery") {
+            let mut rules_should_be_migrated = FxHashMap::default();
+            for (group, (new_group_name, new_rule_name)) in RULES_TO_MIGRATE {
+                rules_should_be_migrated.insert(*group, (*new_group_name, *new_rule_name));
             }
-            let object_value = nursery_member
+            let Some(nursery_group_object) = nursery_group
                 .value()
                 .ok()
-                .and_then(|node| node.as_json_object_value().cloned());
-
-            let Some(object_value) = object_value else {
+                .and_then(|node| node.as_json_object_value().cloned())
+            else {
                 return rules_to_migrate;
             };
 
-            for group_member in object_value.json_member_list().iter().flatten() {
-                let Ok(member_name_text) = group_member
+            let mut separator_iterator = nursery_group_object
+                .json_member_list()
+                .separators()
+                .flatten()
+                .enumerate()
+                // Repeat the first separator,
+                // so when the rule to be deleted is the first rule,
+                // its trailing comma is also deleted:
+                // {
+                //    "ruleA": "error",
+                //    "ruleB": "error",
+                //    "ruleC": "error"
+                // }
+                .flat_map(|(i, s)| repeat(s).take(if i == 0 { 2 } else { 1 }));
+
+            for nursery_rule in nursery_group_object.json_member_list().iter().flatten() {
+                let optional_separator = separator_iterator.next();
+
+                let Ok(nursery_rule_name) = nursery_rule
                     .name()
                     .and_then(|node| node.inner_string_text())
                 else {
                     continue;
                 };
-                let new_rule = rules.get(member_name_text.text()).copied();
-                if let Some((new_group, new_rule)) = new_rule {
+
+                let new_rule = rules_should_be_migrated
+                    .get(nursery_rule_name.text())
+                    .copied();
+
+                if let Some((new_group_name, new_rule_name)) = new_rule {
                     rules_to_migrate.push(MigrateRuleState {
-                        nursery_rule_member: group_member.clone(),
-                        nursery_group: nursery_member.clone(),
-                        new_rule_name: new_rule,
-                        new_group_name: new_group,
+                        nursery_rule: nursery_rule.clone(),
+                        nursery_group: nursery_group.clone(),
+                        optional_separator,
+                        new_rule_name,
+                        new_group_name,
                     })
                 }
             }
@@ -188,33 +216,51 @@ impl Rule for NurseryRules {
         let MigrateRuleState {
             new_group_name,
             new_rule_name,
+            optional_separator,
             nursery_group,
-            nursery_rule_member: nursery_rule,
+            nursery_rule,
         } = state;
         let mut mutation = ctx.root().begin();
-
-        let new_group = find_group_by_name(node, new_group_name);
+        let mut rule_existed = false;
 
         // If the group exists, then we just need to update that group by adding a new member
         // with the name of rule we are migrating
-        if let Some(group) = new_group {
-            let value = group.value().ok()?;
-            let value = value.as_json_object_value()?;
+        if let Some(group) = find_group_by_name(node, new_group_name) {
+            let group_value = group.value().ok()?;
+            let group_value_object = group_value.as_json_object_value()?;
 
-            let mut separators = vec![];
-            let mut new_list = vec![];
+            let old_group_rules = group_value_object.json_member_list();
+            let old_group_rules_count = old_group_rules.len();
 
-            let old_list_node = value.json_member_list();
-            let new_rule_member =
-                make_new_rule_name_member(new_rule_name, &nursery_rule.clone().detach())?;
+            let mut separators = Vec::with_capacity(old_group_rules_count + 1);
+            let mut new_group_rules = Vec::with_capacity(old_group_rules_count + 1);
 
-            for member in old_list_node.iter() {
-                let member = member.ok()?;
-                new_list.push(member.clone());
+            for old_rule in old_group_rules.iter() {
+                let old_rule = old_rule.ok()?;
+                if old_rule
+                    .name()
+                    .and_then(|node| node.inner_string_text())
+                    .is_ok_and(|text| text.text() == *new_rule_name)
+                {
+                    rule_existed = true;
+                    break;
+                }
+                new_group_rules.push(old_rule.clone());
                 separators.push(token(T![,]));
             }
-            new_list.push(new_rule_member);
-            mutation.replace_node(old_list_node, json_member_list(new_list, separators));
+
+            if !rule_existed {
+                let new_rule_member =
+                    make_new_rule_name_member(new_rule_name, &nursery_rule.clone().detach())?;
+                new_group_rules.push(new_rule_member);
+                mutation.replace_node(
+                    old_group_rules,
+                    json_member_list(new_group_rules, separators),
+                );
+            }
+            if let Some(separator) = optional_separator {
+                mutation.remove_token(separator.clone());
+            }
             mutation.remove_node(nursery_rule.clone());
         }
         // If we don't have a group, we have to create one. To avoid possible side effects with our mutation logic
@@ -311,8 +357,14 @@ impl Rule for NurseryRules {
         Some(MigrationAction::new(
             ActionCategory::QuickFix,
             ctx.metadata().applicability(),
-            markup! {
-                "Move the rule to the new stable group."
+            if rule_existed {
+                markup! {
+                    "Remove the stale rule from the nursery group."
+                }
+            } else {
+                markup! {
+                    "Move the rule to the new stable group."
+                }
             }
             .to_owned(),
             mutation,

crates/biome_migrate/tests/specs/migrations/nurseryRules/existingGroup.json → crates/biome_migrate/tests/specs/migrations/nurseryRules/existingGroupWithExistingRule.json

@@ -2,7 +2,7 @@
   "linter": {
     "rules": {
       "nursery": {
-        "noExcessiveNestedTestSuites": "error"
+        "noExcessiveNestedTestSuites": "warn"
       },
       "complexity": {
         "noExcessiveNestedTestSuites": "error"

crates/biome_migrate/tests/specs/migrations/nurseryRules/existingGroupWithExistingRule.json.snap

@@ -0,0 +1,44 @@
+---
+source: crates/biome_migrate/tests/spec_tests.rs
+expression: existingGroupWithExistingRule.json
+---
+# Input
+```json
+{
+  "linter": {
+    "rules": {
+      "nursery": {
+        "noExcessiveNestedTestSuites": "warn"
+      },
+      "complexity": {
+        "noExcessiveNestedTestSuites": "error"
+      }
+    }
+  }
+}
+
+```
+
+# Diagnostics
+```
+existingGroupWithExistingRule.json:5:9 migrate  FIXABLE  ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
+
+  ! This rule has been promoted to complexity/noExcessiveNestedTestSuites.
+  
+    3 │     "rules": {
+    4 │       "nursery": {
+  > 5 │         "noExcessiveNestedTestSuites": "warn"
+      │         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+    6 │       },
+    7 │       "complexity": {
+  
+  i Unsafe fix: Remove the stale rule from the nursery group.
+  
+     3  3 │       "rules": {
+     4  4 │         "nursery": {
+     5    │ - ········"noExcessiveNestedTestSuites":·"warn"
+     6  5 │         },
+     7  6 │         "complexity": {
+  
+
+```

crates/biome_migrate/tests/specs/migrations/nurseryRules/firstToExistingGroup.json

@@ -0,0 +1,11 @@
+{
+  "linter": {
+    "rules": {
+      "nursery": {
+        "noExcessiveNestedTestSuites": "error",
+        "nuseryRuleAlways": "error"
+      },
+      "complexity": {}
+    }
+  }
+}

crates/biome_migrate/tests/specs/migrations/nurseryRules/firstToExistingGroup.json.snap

@@ -0,0 +1,47 @@
+---
+source: crates/biome_migrate/tests/spec_tests.rs
+expression: firstToExistingGroup.json
+---
+# Input
+```json
+{
+  "linter": {
+    "rules": {
+      "nursery": {
+        "noExcessiveNestedTestSuites": "error",
+        "nuseryRuleAlways": "error"
+      },
+      "complexity": {}
+    }
+  }
+}
+
+```
+
+# Diagnostics
+```
+firstToExistingGroup.json:5:9 migrate  FIXABLE  ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
+
+  ! This rule has been promoted to complexity/noExcessiveNestedTestSuites.
+  
+    3 │     "rules": {
+    4 │       "nursery": {
+  > 5 │         "noExcessiveNestedTestSuites": "error",
+      │         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+    6 │         "nuseryRuleAlways": "error"
+    7 │       },
+  
+  i Unsafe fix: Move the rule to the new stable group.
+  
+     3  3 │       "rules": {
+     4  4 │         "nursery": {
+     5    │ - ········"noExcessiveNestedTestSuites":·"error",
+     6  5 │           "nuseryRuleAlways": "error"
+     7  6 │         },
+     8    │ - ······"complexity":·{}
+        7 │ + ······"complexity":·{"noExcessiveNestedTestSuites":·"error"}
+     9  8 │       }
+    10  9 │     }
+  
+
+```

crates/biome_migrate/tests/specs/migrations/nurseryRules/lastToExistingGroup.json

@@ -0,0 +1,11 @@
+{
+  "linter": {
+    "rules": {
+      "nursery": {
+        "nuseryRuleAlways": "error",
+        "noExcessiveNestedTestSuites": "error"
+      },
+      "complexity": {}
+    }
+  }
+}

crates/biome_migrate/tests/specs/migrations/nurseryRules/lastToExistingGroup.json.snap

@@ -0,0 +1,50 @@
+---
+source: crates/biome_migrate/tests/spec_tests.rs
+expression: lastToExistingGroup.json
+---
+# Input
+```json
+{
+  "linter": {
+    "rules": {
+      "nursery": {
+        "nuseryRuleAlways": "error",
+        "noExcessiveNestedTestSuites": "error"
+      },
+      "complexity": {}
+    }
+  }
+}
+
+```
+
+# Diagnostics
+```
+lastToExistingGroup.json:5:36 migrate  FIXABLE  ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
+
+  ! This rule has been promoted to complexity/noExcessiveNestedTestSuites.
+  
+    3 │     "rules": {
+    4 │       "nursery": {
+  > 5 │         "nuseryRuleAlways": "error",
+      │                                    ^
+  > 6 │         "noExcessiveNestedTestSuites": "error"
+      │         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+    7 │       },
+    8 │       "complexity": {}
+  
+  i Unsafe fix: Move the rule to the new stable group.
+  
+     3  3 │       "rules": {
+     4  4 │         "nursery": {
+     5    │ - ········"nuseryRuleAlways":·"error",
+     6    │ - ········"noExcessiveNestedTestSuites":·"error"
+        5 │ + ········"nuseryRuleAlways":·"error"
+     7  6 │         },
+     8    │ - ······"complexity":·{}
+        7 │ + ······"complexity":·{"noExcessiveNestedTestSuites":·"error"}
+     9  8 │       }
+    10  9 │     }
+  
+
+```