Skip to content

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

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.

Sign up for GitHub

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

Merged
graphite-app merged 1 commit into from
Feb 16, 2026
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
128 changes: 91 additions & 37 deletions apps/oxfmt/src/core/oxfmtrc.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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;

Expand Down Expand Up @@ -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;
Expand All @@ -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`"
Copy link

@Jungzl Jungzl Feb 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just updated to the latest oxfmt and got this error message. After checking the documentation, I found it still uses kebab case notation, which confused me. I eventually found my way here.
I think adding , use xx_xx instead. might make it clearer.

Copy link
Member Author

@leaysgur leaysgur Feb 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, docs are outdated, I will update today.

));
};
entries.push(entry);
}
groups.push(entries);
}
}
}
Expand All @@ -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 {
Expand Down Expand Up @@ -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.
Expand All @@ -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.
Expand Down Expand Up @@ -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>>,
}
Expand Down Expand Up @@ -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(
Expand All @@ -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);
Expand Down
65 changes: 36 additions & 29 deletions crates/oxc_formatter/src/ir_transform/sort_imports/group_config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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)]
Expand All @@ -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 })
}
Expand Down Expand Up @@ -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),
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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),
Expand All @@ -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",
Expand Down
29 changes: 15 additions & 14 deletions crates/oxc_formatter/src/ir_transform/sort_imports/group_matcher.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -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));
}
}
Expand Down
Loading
Loading