feat(formatter/sort_imports)!: Report invalid group name with renaming side-effect > side_effect (#19416)

- Breaking: Rename ambiguous modifier/selector names
  - `side-effect` → `side_effect`
  - `side-effect-style` → `side_effect_style`
  - Now `-` is used only for joiner (`modifier-modifier-selector`)
    - before: `value-side-effect-style` 🆚 after: `value-side_effect_style`
- Breaking: Add config-time validation for invalid group names
  - previously silently ignored...
  - or wrongly applied (and user didn't notice)

Author: leaysgur

apps/oxfmt/src/core/oxfmtrc.rs

@@ -1,14 +1,15 @@
 use std::path::Path;
 
+use rustc_hash::FxHashSet;
 use schemars::JsonSchema;
 use serde::{Deserialize, Serialize};
 use serde_json::Value;
 
 use oxc_formatter::{
     ArrowParentheses, AttributePosition, BracketSameLine, BracketSpacing, CustomGroupDefinition,
-    EmbeddedLanguageFormatting, Expand, FormatOptions, ImportModifier, ImportSelector, IndentStyle,
-    IndentWidth, LineEnding, LineWidth, QuoteProperties, QuoteStyle, Semicolons,
-    SortImportsOptions, SortOrder, TailwindcssOptions, TrailingCommas,
+    EmbeddedLanguageFormatting, Expand, FormatOptions, GroupEntry, GroupName, ImportModifier,
+    ImportSelector, IndentStyle, IndentWidth, LineEnding, LineWidth, QuoteProperties, QuoteStyle,
+    Semicolons, SortImportsOptions, SortOrder, TailwindcssOptions, TrailingCommas,
 };
 use oxc_toml::Options as TomlFormatterOptions;
 
@@ -419,7 +420,46 @@ impl FormatConfig {
             if let Some(v) = config.internal_pattern {
                 sort_imports.internal_pattern = v;
             }
+            // Validate and parse `customGroups` first, since `groups` may refer to custom group names.
+            if let Some(v) = config.custom_groups {
+                let mut custom_groups = Vec::with_capacity(v.len());
+                for cg in v {
+                    let CustomGroupItemConfig { group_name, element_name_pattern, .. } = cg;
+                    let selector = match cg.selector.as_deref() {
+                        Some(s) => match ImportSelector::parse(s) {
+                            Some(parsed) => Some(parsed),
+                            None => {
+                                return Err(format!(
+                                    "Invalid `sortImports` configuration: unknown selector: `{s}` in customGroups: `{group_name}`"
+                                ));
+                            }
+                        },
+                        None => None,
+                    };
+                    let raw_modifiers = cg.modifiers.unwrap_or_default();
+                    let mut modifiers = Vec::with_capacity(raw_modifiers.len());
+                    for m in &raw_modifiers {
+                        match ImportModifier::parse(m) {
+                            Some(parsed) => modifiers.push(parsed),
+                            None => {
+                                return Err(format!(
+                                    "Invalid `sortImports` configuration: unknown modifier: `{m}` in customGroups: `{group_name}`"
+                                ));
+                            }
+                        }
+                    }
+                    custom_groups.push(CustomGroupDefinition {
+                        group_name,
+                        element_name_pattern,
+                        selector,
+                        modifiers,
+                    });
+                }
+                sort_imports.custom_groups = custom_groups;
+            }
             if let Some(v) = config.groups {
+                let custom_group_names: FxHashSet<&str> =
+                    sort_imports.custom_groups.iter().map(|g| g.group_name.as_str()).collect();
                 let mut groups = Vec::new();
                 let mut newline_boundary_overrides: Vec<Option<bool>> = Vec::new();
                 let mut pending_override: Option<bool> = None;
@@ -437,15 +477,24 @@ impl FormatConfig {
                         }
                         other => {
                             if !groups.is_empty() {
-                                // Record the boundary between the previous group and this one.
-                                // `pending_override` is
-                                // - `Some(bool)` if a marker preceded this group
-                                // - or `None` (= use global `newlines_between`) otherwise
-                                // For the very first group (`groups.is_empty()`),
-                                // there is no preceding boundary, so we skip this entirely.
                                 newline_boundary_overrides.push(pending_override.take());
                             }
-                            groups.push(other.into_vec());
+                            let mut entries = Vec::new();
+                            for name in other.into_vec() {
+                                let entry = if name == "unknown" {
+                                    GroupEntry::Unknown
+                                } else if custom_group_names.contains(name.as_str()) {
+                                    GroupEntry::Custom(name)
+                                } else if let Some(group_name) = GroupName::parse(&name) {
+                                    GroupEntry::Predefined(group_name)
+                                } else {
+                                    return Err(format!(
+                                        "Invalid `sortImports` configuration: unknown group name `{name}` in `groups`"
+                                    ));
+                                };
+                                entries.push(entry);
+                            }
+                            groups.push(entries);
                         }
                     }
                 }
@@ -463,22 +512,6 @@ impl FormatConfig {
             {
                 return Err("Invalid `sortImports` configuration: `partitionByNewline` and per-group `{ \"newlinesBetween\" }` markers cannot be used together".to_string());
             }
-            if let Some(v) = config.custom_groups {
-                sort_imports.custom_groups = v
-                    .into_iter()
-                    .map(|c| CustomGroupDefinition {
-                        group_name: c.group_name,
-                        element_name_pattern: c.element_name_pattern,
-                        selector: c.selector.as_deref().and_then(ImportSelector::parse),
-                        modifiers: c
-                            .modifiers
-                            .unwrap_or_default()
-                            .iter()
-                            .filter_map(|s| ImportModifier::parse(s))
-                            .collect(),
-                    })
-                    .collect();
-            }
 
             // `partition_by_newline: true` and `newlines_between: true` cannot be used together
             if sort_imports.partition_by_newline && sort_imports.newlines_between {
@@ -679,8 +712,8 @@ pub struct SortImportsConfig {
     ///
     /// The list of selectors is sorted from most to least important:
     /// - `type` — TypeScript type imports.
-    /// - `side-effect-style` — Side effect style imports.
-    /// - `side-effect` — Side effect imports.
+    /// - `side_effect_style` — Side effect style imports.
+    /// - `side_effect` — Side effect imports.
     /// - `style` — Style imports.
     /// - `index` — Main file from the current directory.
     /// - `sibling` — Modules from the same directory.
@@ -692,7 +725,7 @@ pub struct SortImportsConfig {
     /// - `import` — Any import.
     ///
     /// The list of modifiers is sorted from most to least important:
-    /// - `side-effect` — Side effect imports.
+    /// - `side_effect` — Side effect imports.
     /// - `type` — TypeScript type imports.
     /// - `value` — Value imports.
     /// - `default` — Imports containing the default specifier.
@@ -776,14 +809,14 @@ pub struct CustomGroupItemConfig {
     pub element_name_pattern: Vec<String>,
     /// Selector to match the import kind.
     ///
-    /// Possible values: `"type"`, `"side-effect-style"`, `"side-effect"`, `"style"`, `"index"`,
+    /// Possible values: `"type"`, `"side_effect_style"`, `"side_effect"`, `"style"`, `"index"`,
     /// `"sibling"`, `"parent"`, `"subpath"`, `"internal"`, `"builtin"`, `"external"`, `"import"`
     #[serde(skip_serializing_if = "Option::is_none")]
     pub selector: Option<String>,
     /// Modifiers to match the import characteristics.
     /// All specified modifiers must be present (AND logic).
     ///
-    /// Possible values: `"side-effect"`, `"type"`, `"value"`, `"default"`, `"wildcard"`, `"named"`
+    /// Possible values: `"side_effect"`, `"type"`, `"value"`, `"default"`, `"wildcard"`, `"named"`
     #[serde(skip_serializing_if = "Option::is_none")]
     pub modifiers: Option<Vec<String>>,
 }
@@ -1259,9 +1292,21 @@ mod tests {
         let oxfmt_options = config.into_oxfmt_options().unwrap();
         let sort_imports = oxfmt_options.format_options.experimental_sort_imports.unwrap();
         assert_eq!(sort_imports.groups.len(), 5);
-        assert_eq!(sort_imports.groups[0], vec!["builtin".to_string()]);
-        assert_eq!(sort_imports.groups[1], vec!["external".to_string(), "internal".to_string()]);
-        assert_eq!(sort_imports.groups[4], vec!["index".to_string()]);
+        assert_eq!(
+            sort_imports.groups[0],
+            vec![GroupEntry::Predefined(GroupName::parse("builtin").unwrap())]
+        );
+        assert_eq!(
+            sort_imports.groups[1],
+            vec![
+                GroupEntry::Predefined(GroupName::parse("external").unwrap()),
+                GroupEntry::Predefined(GroupName::parse("internal").unwrap())
+            ]
+        );
+        assert_eq!(
+            sort_imports.groups[4],
+            vec![GroupEntry::Predefined(GroupName::parse("index").unwrap())]
+        );
 
         // Test groups with newlinesBetween overrides
         let config: FormatConfig = serde_json::from_str(
@@ -1280,9 +1325,18 @@ mod tests {
         let oxfmt_options = config.into_oxfmt_options().unwrap();
         let sort_imports = oxfmt_options.format_options.experimental_sort_imports.unwrap();
         assert_eq!(sort_imports.groups.len(), 3);
-        assert_eq!(sort_imports.groups[0], vec!["builtin".to_string()]);
-        assert_eq!(sort_imports.groups[1], vec!["external".to_string()]);
-        assert_eq!(sort_imports.groups[2], vec!["parent".to_string()]);
+        assert_eq!(
+            sort_imports.groups[0],
+            vec![GroupEntry::Predefined(GroupName::parse("builtin").unwrap())]
+        );
+        assert_eq!(
+            sort_imports.groups[1],
+            vec![GroupEntry::Predefined(GroupName::parse("external").unwrap())]
+        );
+        assert_eq!(
+            sort_imports.groups[2],
+            vec![GroupEntry::Predefined(GroupName::parse("parent").unwrap())]
+        );
         assert_eq!(sort_imports.newline_boundary_overrides.len(), 2);
         assert_eq!(sort_imports.newline_boundary_overrides[0], Some(false));
         assert_eq!(sort_imports.newline_boundary_overrides[1], None);

crates/oxc_formatter/src/ir_transform/sort_imports/group_config.rs

@@ -1,5 +1,16 @@
 use std::cmp::Ordering;
 
+/// A parsed entry in a group configuration.
+#[derive(Debug, Clone, PartialEq, Eq)]
+pub enum GroupEntry {
+    /// A predefined group name (e.g. "type-external", "value-builtin").
+    Predefined(GroupName),
+    /// The special "unknown" catch-all group.
+    Unknown,
+    /// A reference to a user-defined custom group by name.
+    Custom(String),
+}
+
 /// Represents a group name pattern for matching imports.
 /// A group name consists of 1 selector and N modifiers.
 #[derive(Debug, Clone, PartialEq, Eq, Hash)]
@@ -17,38 +28,34 @@ impl GroupName {
     /// Parse a group name string into a GroupName.
     ///
     /// Format: `(modifier-)*selector`
+    ///
+    /// Since no selector or modifier name contains `-`,
+    /// we can simply split by `-`: the last element is the selector,
+    /// and all preceding elements are modifiers.
+    ///
     /// Examples:
     /// - "external" -> modifiers: (empty), selector: External
     /// - "type-external" -> modifiers: Type, selector: External
     /// - "value-builtin" -> modifiers: Value, selector: Builtin
-    /// - "side-effect-import" -> modifiers: SideEffect, selector: Import
-    /// - "side-effect-type-external" -> modifiers: SideEffect, Type, selector: External
-    /// - "named-side-effect-type-builtin" -> modifiers: SideEffect, Type, Named, selector: Builtin
+    /// - "side_effect-import" -> modifiers: SideEffect, selector: Import
+    /// - "side_effect-type-external" -> modifiers: SideEffect, Type, selector: External
     pub fn parse(s: &str) -> Option<Self> {
-        // Try to parse as a selector without modifiers first
-        if let Some(selector) = ImportSelector::parse(s) {
+        let parts: Vec<&str> = s.split('-').collect();
+        let selector = ImportSelector::parse(parts.last()?)?;
+
+        if parts.len() == 1 {
             return Some(Self { modifiers: vec![], selector });
         }
 
-        // Last part should be the selector
-        let selector =
-            *ImportSelector::ALL_SELECTORS.iter().find(|selector| s.ends_with(selector.name()))?;
-
-        // The remaining part represents a sequence of modifiers joined by "-".
-        // Since modifiers themselves may contain "-", splitting by "-" would be ambiguous.
-        // Instead, we iterate over modifiers in a predefined order and check
-        // whether they appear in the remaining string.
-        // This guarantees the extracted modifiers are already ordered
-        // and no additional sorting is required.
-        //
-        // The trade-off is that this approach may tolerate invalid input,
-        // as unmatched or malformed segments are not strictly rejected.
-        let mut modifiers = Vec::with_capacity(ImportModifier::ALL_MODIFIERS.len());
-        for m in ImportModifier::ALL_MODIFIERS {
-            if s.contains(m.name()) {
-                modifiers.push(*m);
-            }
+        let mut modifiers = Vec::with_capacity(parts.len() - 1);
+        for part in &parts[..parts.len() - 1] {
+            modifiers.push(ImportModifier::parse(part)?);
         }
+        // Normalize modifier order so that
+        // "type-value-external" and "value-type-external" are treated as the same.
+        // Also deduplicate in case the user wrote "type-type-external".
+        modifiers.sort_unstable();
+        modifiers.dedup();
 
         Some(Self { selector, modifiers })
     }
@@ -121,8 +128,8 @@ impl ImportSelector {
     pub fn parse(s: &str) -> Option<Self> {
         match s {
             "type" => Some(Self::Type),
-            "side-effect-style" => Some(Self::SideEffectStyle),
-            "side-effect" => Some(Self::SideEffect),
+            "side_effect_style" => Some(Self::SideEffectStyle),
+            "side_effect" => Some(Self::SideEffect),
             "style" => Some(Self::Style),
             "index" => Some(Self::Index),
             "sibling" => Some(Self::Sibling),
@@ -154,8 +161,8 @@ impl ImportSelector {
     pub fn name(&self) -> &str {
         match self {
             ImportSelector::Type => "type",
-            ImportSelector::SideEffectStyle => "side-effect-style",
-            ImportSelector::SideEffect => "side-effect",
+            ImportSelector::SideEffectStyle => "side_effect_style",
+            ImportSelector::SideEffect => "side_effect",
             ImportSelector::Style => "style",
             ImportSelector::Index => "index",
             ImportSelector::Sibling => "sibling",
@@ -200,7 +207,7 @@ impl ImportModifier {
     /// Parse a string into an ImportModifier.
     pub fn parse(s: &str) -> Option<Self> {
         match s {
-            "side-effect" => Some(Self::SideEffect),
+            "side_effect" => Some(Self::SideEffect),
             "type" => Some(Self::Type),
             "value" => Some(Self::Value),
             "default" => Some(Self::Default),
@@ -212,7 +219,7 @@ impl ImportModifier {
 
     pub fn name(&self) -> &str {
         match self {
-            ImportModifier::SideEffect => "side-effect",
+            ImportModifier::SideEffect => "side_effect",
             ImportModifier::Type => "type",
             ImportModifier::Value => "value",
             ImportModifier::Default => "default",

crates/oxc_formatter/src/ir_transform/sort_imports/group_matcher.rs

@@ -1,6 +1,6 @@
-use rustc_hash::{FxHashMap, FxHashSet};
+use rustc_hash::FxHashMap;
 
-use super::group_config::{GroupName, ImportModifier, ImportSelector};
+use super::group_config::{GroupEntry, GroupName, ImportModifier, ImportSelector};
 use super::options::CustomGroupDefinition;
 
 // Intermediate import metadata that is used for group matching
@@ -21,30 +21,31 @@ pub struct GroupMatcher {
 }
 
 impl GroupMatcher {
-    pub fn new(groups: &[Vec<String>], custom_groups: &[CustomGroupDefinition]) -> Self {
-        let custom_group_name_set =
-            custom_groups.iter().map(|g| g.group_name.clone()).collect::<FxHashSet<_>>();
-
+    pub fn new(groups: &[Vec<GroupEntry>], custom_groups: &[CustomGroupDefinition]) -> Self {
         let mut unknown_group_index: Option<usize> = None;
 
         let mut used_custom_group_index_map = FxHashMap::default();
         let mut predefined_groups = Vec::new();
         for (index, group_union) in groups.iter().enumerate() {
-            for group in group_union {
-                if group == "unknown" {
-                    unknown_group_index = Some(index);
-                } else if custom_group_name_set.contains(group) {
-                    used_custom_group_index_map.insert(group.to_owned(), index);
-                } else if let Some(group_name) = GroupName::parse(group) {
-                    predefined_groups.push((group_name, index));
+            for entry in group_union {
+                match entry {
+                    GroupEntry::Unknown => {
+                        unknown_group_index = Some(index);
+                    }
+                    GroupEntry::Custom(name) => {
+                        used_custom_group_index_map.insert(name.as_str(), index);
+                    }
+                    GroupEntry::Predefined(group_name) => {
+                        predefined_groups.push((group_name.clone(), index));
+                    }
                 }
             }
         }
 
         let mut used_custom_groups: Vec<(CustomGroupDefinition, usize)> =
             Vec::with_capacity(used_custom_group_index_map.len());
         for custom_group in custom_groups {
-            if let Some(index) = used_custom_group_index_map.get(&custom_group.group_name) {
+            if let Some(index) = used_custom_group_index_map.get(custom_group.group_name.as_str()) {
                 used_custom_groups.push((custom_group.clone(), *index));
             }
         }