From e8e0c32a1602c63703a6b6652a15665814ab2092 Mon Sep 17 00:00:00 2001 From: Guillaume Piedigrossi Date: Sun, 15 Dec 2024 00:04:54 +0100 Subject: [PATCH 1/8] feat(linter): create the import/order rule all cases are not covered bu the architecture is setup --- crates/oxc_linter/src/rules.rs | 2 + crates/oxc_linter/src/rules/eslint/order.rs | 599 ++++++++++++++++++ .../src/snapshots/eslint_order.snap | 27 + 3 files changed, 628 insertions(+) create mode 100644 crates/oxc_linter/src/rules/eslint/order.rs create mode 100644 crates/oxc_linter/src/snapshots/eslint_order.snap diff --git a/crates/oxc_linter/src/rules.rs b/crates/oxc_linter/src/rules.rs index 8a1c0686db7b7..41a6ed4cf6b31 100644 --- a/crates/oxc_linter/src/rules.rs +++ b/crates/oxc_linter/src/rules.rs @@ -135,6 +135,7 @@ mod eslint { pub mod no_var; pub mod no_void; pub mod no_with; + pub mod order; pub mod prefer_exponentiation_operator; pub mod prefer_numeric_literals; pub mod prefer_object_has_own; @@ -625,6 +626,7 @@ oxc_macros::declare_all_lint_rules! { eslint::no_var, eslint::no_void, eslint::no_with, + eslint::order, eslint::prefer_exponentiation_operator, eslint::prefer_numeric_literals, eslint::prefer_object_has_own, diff --git a/crates/oxc_linter/src/rules/eslint/order.rs b/crates/oxc_linter/src/rules/eslint/order.rs new file mode 100644 index 0000000000000..b374a91a597d0 --- /dev/null +++ b/crates/oxc_linter/src/rules/eslint/order.rs @@ -0,0 +1,599 @@ +use oxc_diagnostics::OxcDiagnostic; +use oxc_macros::declare_oxc_lint; +use oxc_span::{CompactStr, Span}; +use rustc_hash::FxHashMap; +use serde::Deserialize; + +use crate::{ + context::LintContext, + module_record::{ImportEntry, ImportImportName}, + rule::Rule, +}; + +#[derive(Debug, Clone, Deserialize)] +struct OrderConfig { + groups: Option>, + #[serde(rename = "pathGroups")] + path_groups: Option>, + #[serde(rename = "pathGroupsExcludedImportTypes")] + path_groups_excluded_import_types: Option>, + #[serde(rename = "newlines-between")] + newlines_between: Option, + named: Option, + alphabetize: Option, + #[serde(rename = "warnOnUnassignedImports")] + warn_on_unassigned_imports: Option, +} + +#[derive(Debug, Clone, Deserialize)] +struct PathGroup { + pattern: String, + #[serde(rename = "patternOptions")] + pattern_options: Option, + group: String, + position: Option, +} + +#[derive(Debug, Clone, Deserialize)] +struct PatternOptions { + #[serde(rename = "nocomment")] + no_comment: Option, +} + +#[derive(Debug, Clone, Deserialize)] +struct NamedOrder { + enabled: Option, + import: Option, + export: Option, + require: Option, + #[serde(rename = "cjsExports")] + cjs_exports: Option, + types: Option, +} + +#[derive(Debug, Clone, Deserialize)] +struct Alphabetize { + order: Option, + #[serde(rename = "orderImportKind")] + order_import_kind: Option, + #[serde(rename = "caseInsensitive")] + case_insensitive: Option, +} + +#[derive(Debug, Default, Clone)] +pub struct Order { + config: Option, +} + +#[derive(Debug)] +struct ImportInfo { + source: CompactStr, + span: Span, + kind: ImportKind, + group: String, + rank: usize, + specifiers: Vec, +} + +#[derive(Debug)] +struct ImportSpecifier { + name: CompactStr, + span: Span, + is_type: bool, +} + +#[derive(Debug, PartialEq)] +enum ImportKind { + Import, + Require, + ExportFrom, +} + +declare_oxc_lint!( + /// ### What it does + /// Enforces a convention in module import order. + /// + /// ### Why is this bad? + /// Having a consistent order in imports helps readability and maintainability. + /// + /// ### Examples + /// + /// Examples of **incorrect** code for this rule: + /// ```js + /// import _ from 'lodash'; + /// import path from 'path'; // `path` import should occur before import of `lodash` + /// + /// // ----- + /// + /// var _ = require('lodash'); + /// var path = require('path'); // `path` import should occur before import of `lodash` + /// + /// // ----- + /// + /// var path = require('path'); + /// import foo from './foo'; // `import` statements must be before `require` statement + /// ``` + /// + /// Examples of **correct** code for this rule: + /// ```js + /// import path from 'path'; + /// import _ from 'lodash'; + /// + /// // ----- + /// + /// var path = require('path'); + /// var _ = require('lodash'); + /// + /// // ----- + /// + /// // Allowed as ̀`babel-register` is not assigned. + /// require('babel-register'); + /// var path = require('path'); + /// + /// // ----- + /// + /// // Allowed as `import` must be before `require` + /// import foo from './foo'; + /// var path = require('path'); + /// ``` + Order, + style, + pending // TODO: describe fix capabilities. Remove if no fix can be done, + // keep at 'pending' if you think one could be added but don't know how. + // Options are 'fix', 'fix_dangerous', 'suggestion', and 'conditional_fix_suggestion' +); + +impl Rule for Order { + fn from_configuration(value: serde_json::Value) -> Self { + Self { config: serde_json::from_value(value).ok() } + } + fn run_once(&self, ctx: &LintContext) { + if let Some(config) = &self.config { + let mut imports = self.collect_imports(ctx); + self.check_imports_order(ctx, &mut imports, config); + } + } +} + +impl Order { + fn collect_imports(&self, ctx: &LintContext) -> Vec { + let mut imports = Vec::new(); + let module_record = ctx.module_record(); + + // Collect import declarations + for entry in &module_record.import_entries { + let source = entry.module_request.name(); + let span = entry.module_request.span(); + + imports.push(ImportInfo { + source: CompactStr::new(source), + span, + kind: ImportKind::Import, + group: self.get_import_group(source), + rank: 0, + specifiers: self.collect_import_specifiers(entry), + }); + } + + // Collect export from declarations + for entry in &module_record.indirect_export_entries { + if let Some(module_request) = &entry.module_request { + let source = module_request.name(); + imports.push(ImportInfo { + source: CompactStr::new(source), + span: entry.span, + kind: ImportKind::ExportFrom, + group: self.get_import_group(source), + rank: 0, + specifiers: vec![], + }); + } + } + + imports + } + + fn get_import_group(&self, source: &str) -> String { + // Default groups: builtin, external, parent, sibling, index + if source.starts_with('.') { + if source == "." || source == ".." { + "parent".into() + } else if source.starts_with("./") { + "sibling".into() + } else { + "parent".into() + } + } else if self.is_builtin_module(source) { + "builtin".into() + } else { + "external".into() + } + } + + fn is_builtin_module(&self, source: &str) -> bool { + // List of Node.js builtin modules + static BUILTIN_MODULES: &[&str] = &[ + "assert", + "buffer", + "child_process", + "cluster", + "crypto", + "dgram", + "dns", + "domain", + "events", + "fs", + "http", + "https", + "net", + "os", + "path", + "punycode", + "querystring", + "readline", + "stream", + "string_decoder", + "tls", + "tty", + "url", + "util", + "v8", + "vm", + "zlib", + ]; + BUILTIN_MODULES.contains(&source) + } + + fn check_imports_order( + &self, + ctx: &LintContext, + imports: &mut [ImportInfo], + config: &OrderConfig, + ) { + // Assign ranks based on groups + self.assign_ranks(imports, config); + + // Check alphabetical order if configured + if let Some(alphabetize) = &config.alphabetize { + self.check_alphabetical_order(ctx, imports, alphabetize); + } + + // Check newlines between imports if configured + if let Some(newlines_between) = &config.newlines_between { + self.check_newlines_between(ctx, imports, newlines_between); + } + + // Check for out of order imports + self.check_order_violations(ctx, imports); + } + + fn check_order_violations(&self, ctx: &LintContext, imports: &[ImportInfo]) { + for i in 1..imports.len() { + let prev = &imports[i - 1]; + let curr = &imports[i]; + + if curr.rank < prev.rank { + ctx.diagnostic( + OxcDiagnostic::warn(format!( + "Import from '{}' should occur before import from '{}'", + curr.source, prev.source + )) + .with_label(curr.span), + ); + } + } + } + + // Helper methods + // 1. Helper functions for import collection and processing + fn collect_import_specifiers(&self, entry: &ImportEntry) -> Vec { + let mut specifiers = Vec::new(); + + match &entry.import_name { + ImportImportName::Name(import) => { + specifiers.push(ImportSpecifier { + name: CompactStr::new(import.name()), + span: import.span(), + is_type: entry.is_type, + }); + } + ImportImportName::Default(span) => { + specifiers.push(ImportSpecifier { + name: CompactStr::new(entry.local_name.name()), + span: *span, + is_type: entry.is_type, + }); + } + ImportImportName::NamespaceObject => { + // Handle namespace imports (* as name) + specifiers.push(ImportSpecifier { + name: CompactStr::new(entry.local_name.name()), + span: entry.local_name.span(), + is_type: entry.is_type, + }); + } + } + + specifiers + } + + // 3. Functions for ranking and group assignment + fn assign_ranks(&self, imports: &mut [ImportInfo], config: &OrderConfig) { + let group_ranks = self.get_group_ranks(config); + + for import in imports.iter_mut() { + import.rank = self.calculate_rank(&import.group, &group_ranks); + + // Apply path group rankings if configured + if let Some(path_groups) = &config.path_groups { + if let Some(path_group_rank) = self.get_path_group_rank(&import.source, path_groups) + { + import.rank = path_group_rank; + } + } + } + } + + fn get_group_ranks(&self, config: &OrderConfig) -> FxHashMap { + let default_groups = vec![ + "builtin".to_string(), + "external".to_string(), + "parent".to_string(), + "sibling".to_string(), + "index".to_string(), + ]; + + let groups = config.groups.as_ref().unwrap_or(&default_groups); + let mut ranks = FxHashMap::default(); + + for (index, group) in groups.iter().enumerate() { + ranks.insert(group.clone(), index); + } + + ranks + } + + fn calculate_rank(&self, group: &str, group_ranks: &FxHashMap) -> usize { + *group_ranks.get(group).unwrap_or(&usize::MAX) + } + + // 4. Functions for path group handling + fn get_path_group_rank(&self, source: &str, path_groups: &[PathGroup]) -> Option { + for (index, path_group) in path_groups.iter().enumerate() { + if self.matches_pattern(source, &path_group.pattern) { + let base_rank = index * 100; // Use multiplier to leave room for position adjustments + + // Adjust rank based on position if specified + match path_group.position.as_deref() { + Some("before") => return Some(base_rank.saturating_sub(50)), + Some("after") => return Some(base_rank + 50), + _ => return Some(base_rank), + } + } + } + None + } + + fn matches_pattern(&self, source: &str, pattern: &str) -> bool { + // Simple pattern matching implementation + // Could be enhanced with proper glob matching + if pattern.contains('*') { + let pattern = pattern.replace('*', ".*"); + regex::Regex::new(&pattern).map(|re| re.is_match(source)).unwrap_or(false) + } else { + source == pattern + } + } + + // 5. Functions for alphabetical ordering + fn check_alphabetical_order( + &self, + ctx: &LintContext, + imports: &[ImportInfo], + alphabetize: &Alphabetize, + ) { + let case_insensitive = alphabetize.case_insensitive.unwrap_or(false); + let order = alphabetize.order.as_deref().unwrap_or("ignore"); + + if order == "ignore" { + return; + } + + for window in imports.windows(2) { + let prev = &window[0]; + let curr = &window[1]; + + // Only compare imports within the same group + if prev.rank != curr.rank { + continue; + } + + let prev_source = + if case_insensitive { prev.source.to_lowercase() } else { prev.source.to_string() }; + + let curr_source = + if case_insensitive { curr.source.to_lowercase() } else { curr.source.to_string() }; + + let is_wrong_order = match order { + "asc" => prev_source > curr_source, + "desc" => prev_source < curr_source, + _ => false, + }; + + if is_wrong_order { + ctx.diagnostic( + OxcDiagnostic::warn(format!( + "Imports must be sorted in {} order. '{}' should be before '{}'.", + order, curr.source, prev.source + )) + .with_label(curr.span), + ); + } + } + } + + // 6. Functions for newlines checking + fn check_newlines_between( + &self, + ctx: &LintContext, + imports: &[ImportInfo], + newlines_setting: &str, + ) { + if newlines_setting == "ignore" { + return; + } + + let source_code = ctx.source_text(); + + for window in imports.windows(2) { + let prev = &window[0]; + let curr = &window[1]; + + let lines_between = self.count_newlines_between( + source_code, + prev.span.end.try_into().unwrap(), + curr.span.start.try_into().unwrap(), + ); + + // Only check for newlines when transitioning between different groups + let is_different_group = self.is_different_group(prev, curr); + + match newlines_setting { + "always" => { + if is_different_group && lines_between == 0 { + ctx.diagnostic( + OxcDiagnostic::warn( + "There should be at least one empty line between import groups", + ) + .with_label(curr.span), + ); + } + } + "never" => { + if lines_between > 0 { + ctx.diagnostic( + OxcDiagnostic::warn("There should be no empty lines between imports") + .with_label(curr.span), + ); + } + } + "always-and-inside-groups" => { + if lines_between == 0 { + ctx.diagnostic( + OxcDiagnostic::warn( + "There should be at least one empty line between imports", + ) + .with_label(curr.span), + ); + } + } + _ => {} + } + } + } + + fn is_different_group(&self, prev: &ImportInfo, curr: &ImportInfo) -> bool { + // Compare the base group names rather than ranks + // This ensures we only require newlines between actual different groups + prev.group != curr.group + } + + fn count_newlines_between(&self, source: &str, start: usize, end: usize) -> usize { + source[start..end].chars().filter(|&c| c == '\n').count().saturating_sub(1) + // Subtract 1 because we don't count the line with the import + } +} + +#[test] +fn test() { + use crate::tester::Tester; + + let pass = vec![ + // Basic sorting + ( + r#" + import fs from 'fs'; + import path from 'path'; + + import _ from 'lodash'; + import chalk from 'chalk'; + + import foo from '../foo'; + + import bar from './bar'; + "#, + Some(serde_json::json!({ + "groups": ["builtin", "external", "parent", "sibling", "index"], + "newlines-between": "always" + })), + ), + // Alphabetical order + ( + r#" + import a from 'a'; + import b from 'b'; + import c from 'c'; + "#, + Some(serde_json::json!({ + "alphabetize": { + "order": "asc", + "caseInsensitive": true + } + })), + ), + // Mixed groups with correct newlines + ( + r#" + import path from 'path'; + import fs from 'fs'; + + import _ from 'lodash'; + + import foo from '../foo'; + import bar from './bar'; + "#, + Some(serde_json::json!({ + "groups": ["builtin", "external", ["parent", "sibling"]], + "newlines-between": "always" + })), + ), + ]; + + let fail = vec![ + // Wrong order + ( + r#" + import _ from 'lodash'; + import fs from 'fs'; + "#, + Some(serde_json::json!({ + "groups": ["builtin", "external"] + })), + ), + // Missing newline between groups + ( + r#" + import fs from 'fs'; + import _ from 'lodash'; // Should have newline before this + "#, + Some(serde_json::json!({ + "groups": ["builtin", "external"], + "newlines-between": "always" + })), + ), + // Wrong alphabetical order + ( + r#" + import b from 'b'; + import a from 'a'; + "#, + Some(serde_json::json!({ + "alphabetize": { + "order": "asc" + } + })), + ), + ]; + + Tester::new(Order::NAME, Order::CATEGORY, pass, fail).test_and_snapshot(); +} diff --git a/crates/oxc_linter/src/snapshots/eslint_order.snap b/crates/oxc_linter/src/snapshots/eslint_order.snap new file mode 100644 index 0000000000000..6bd2109432d16 --- /dev/null +++ b/crates/oxc_linter/src/snapshots/eslint_order.snap @@ -0,0 +1,27 @@ +--- +source: crates/oxc_linter/src/tester.rs +snapshot_kind: text +--- + ⚠ eslint(order): Import from 'fs' should occur before import from 'lodash' + ╭─[order.tsx:3:28] + 2 │ import _ from 'lodash'; + 3 │ import fs from 'fs'; + · ──── + 4 │ + ╰──── + + ⚠ eslint(order): There should be at least one empty line between import groups + ╭─[order.tsx:3:27] + 2 │ import fs from 'fs'; + 3 │ import _ from 'lodash'; // Should have newline before this + · ──────── + 4 │ + ╰──── + + ⚠ eslint(order): Imports must be sorted in asc order. 'a' should be before 'b'. + ╭─[order.tsx:3:27] + 2 │ import b from 'b'; + 3 │ import a from 'a'; + · ─── + 4 │ + ╰──── From 6043dbe817f996f088e54fa246cbc650c7a23219 Mon Sep 17 00:00:00 2001 From: Guillaume Piedigrossi Date: Mon, 16 Dec 2024 22:12:42 +0100 Subject: [PATCH 2/8] perf(linter/order): improve performances by using CompactStr instead of String --- crates/oxc_linter/src/rules/eslint/order.rs | 43 +++++++++++---------- 1 file changed, 23 insertions(+), 20 deletions(-) diff --git a/crates/oxc_linter/src/rules/eslint/order.rs b/crates/oxc_linter/src/rules/eslint/order.rs index b374a91a597d0..72bc817e422fb 100644 --- a/crates/oxc_linter/src/rules/eslint/order.rs +++ b/crates/oxc_linter/src/rules/eslint/order.rs @@ -12,13 +12,13 @@ use crate::{ #[derive(Debug, Clone, Deserialize)] struct OrderConfig { - groups: Option>, + groups: Option>, #[serde(rename = "pathGroups")] path_groups: Option>, #[serde(rename = "pathGroupsExcludedImportTypes")] - path_groups_excluded_import_types: Option>, + path_groups_excluded_import_types: Option>, #[serde(rename = "newlines-between")] - newlines_between: Option, + newlines_between: Option, named: Option, alphabetize: Option, #[serde(rename = "warnOnUnassignedImports")] @@ -31,7 +31,7 @@ struct PathGroup { #[serde(rename = "patternOptions")] pattern_options: Option, group: String, - position: Option, + position: Option, } #[derive(Debug, Clone, Deserialize)] @@ -48,14 +48,14 @@ struct NamedOrder { require: Option, #[serde(rename = "cjsExports")] cjs_exports: Option, - types: Option, + types: Option, } #[derive(Debug, Clone, Deserialize)] struct Alphabetize { - order: Option, + order: Option, #[serde(rename = "orderImportKind")] - order_import_kind: Option, + order_import_kind: Option, #[serde(rename = "caseInsensitive")] case_insensitive: Option, } @@ -70,7 +70,7 @@ struct ImportInfo { source: CompactStr, span: Span, kind: ImportKind, - group: String, + group: CompactStr, rank: usize, specifiers: Vec, } @@ -169,7 +169,7 @@ impl Order { source: CompactStr::new(source), span, kind: ImportKind::Import, - group: self.get_import_group(source), + group: CompactStr::new(&self.get_import_group(source).as_str()), rank: 0, specifiers: self.collect_import_specifiers(entry), }); @@ -183,7 +183,7 @@ impl Order { source: CompactStr::new(source), span: entry.span, kind: ImportKind::ExportFrom, - group: self.get_import_group(source), + group: CompactStr::new(&self.get_import_group(source).as_str()), rank: 0, specifiers: vec![], }); @@ -334,16 +334,19 @@ impl Order { } } - fn get_group_ranks(&self, config: &OrderConfig) -> FxHashMap { - let default_groups = vec![ - "builtin".to_string(), - "external".to_string(), - "parent".to_string(), - "sibling".to_string(), - "index".to_string(), - ]; + fn get_group_ranks(&self, config: &OrderConfig) -> FxHashMap { + let mut default_groups = FxHashMap::default(); + default_groups.insert(CompactStr::new("builtin"), 0); + default_groups.insert(CompactStr::new("external"), 1); + default_groups.insert(CompactStr::new("parent"), 2); + default_groups.insert(CompactStr::new("sibling"), 3); + default_groups.insert(CompactStr::new("index"), 4); + + if config.groups.is_none() { + return default_groups; + } - let groups = config.groups.as_ref().unwrap_or(&default_groups); + let groups = config.groups.as_ref().unwrap(); let mut ranks = FxHashMap::default(); for (index, group) in groups.iter().enumerate() { @@ -353,7 +356,7 @@ impl Order { ranks } - fn calculate_rank(&self, group: &str, group_ranks: &FxHashMap) -> usize { + fn calculate_rank(&self, group: &str, group_ranks: &FxHashMap) -> usize { *group_ranks.get(group).unwrap_or(&usize::MAX) } From 351bed6e739c07bc69b3d04372f84c1aed20819d Mon Sep 17 00:00:00 2001 From: Guillaume Piedigrossi Date: Mon, 16 Dec 2024 22:35:34 +0100 Subject: [PATCH 3/8] perf(linter/order): improve performances by reducing allocations --- crates/oxc_linter/src/rules/eslint/order.rs | 90 ++++++++++++--------- 1 file changed, 50 insertions(+), 40 deletions(-) diff --git a/crates/oxc_linter/src/rules/eslint/order.rs b/crates/oxc_linter/src/rules/eslint/order.rs index 72bc817e422fb..f5120e8b48562 100644 --- a/crates/oxc_linter/src/rules/eslint/order.rs +++ b/crates/oxc_linter/src/rules/eslint/order.rs @@ -1,3 +1,5 @@ +use std::sync::LazyLock; + use oxc_diagnostics::OxcDiagnostic; use oxc_macros::declare_oxc_lint; use oxc_span::{CompactStr, Span}; @@ -27,7 +29,7 @@ struct OrderConfig { #[derive(Debug, Clone, Deserialize)] struct PathGroup { - pattern: String, + pattern: CompactStr, #[serde(rename = "patternOptions")] pattern_options: Option, group: String, @@ -85,10 +87,43 @@ struct ImportSpecifier { #[derive(Debug, PartialEq)] enum ImportKind { Import, - Require, ExportFrom, } +static BUILTIN_MODULES: LazyLock> = LazyLock::new(|| { + let mut set = rustc_hash::FxHashSet::default(); + set.extend([ + "assert", + "buffer", + "child_process", + "cluster", + "crypto", + "dgram", + "dns", + "domain", + "events", + "fs", + "http", + "https", + "net", + "os", + "path", + "punycode", + "querystring", + "readline", + "stream", + "string_decoder", + "tls", + "tty", + "url", + "util", + "v8", + "vm", + "zlib", + ]); + set +}); + declare_oxc_lint!( /// ### What it does /// Enforces a convention in module import order. @@ -155,6 +190,16 @@ impl Rule for Order { } } +fn compare_sources(a: &str, b: &str, case_insensitive: bool) -> std::cmp::Ordering { + if case_insensitive { + let a_chars = a.chars().map(|c| c.to_ascii_lowercase()); + let b_chars = b.chars().map(|c| c.to_ascii_lowercase()); + a_chars.cmp(b_chars) + } else { + a.cmp(b) + } +} + impl Order { fn collect_imports(&self, ctx: &LintContext) -> Vec { let mut imports = Vec::new(); @@ -211,36 +256,6 @@ impl Order { } fn is_builtin_module(&self, source: &str) -> bool { - // List of Node.js builtin modules - static BUILTIN_MODULES: &[&str] = &[ - "assert", - "buffer", - "child_process", - "cluster", - "crypto", - "dgram", - "dns", - "domain", - "events", - "fs", - "http", - "https", - "net", - "os", - "path", - "punycode", - "querystring", - "readline", - "stream", - "string_decoder", - "tls", - "tty", - "url", - "util", - "v8", - "vm", - "zlib", - ]; BUILTIN_MODULES.contains(&source) } @@ -305,7 +320,6 @@ impl Order { }); } ImportImportName::NamespaceObject => { - // Handle namespace imports (* as name) specifiers.push(ImportSpecifier { name: CompactStr::new(entry.local_name.name()), span: entry.local_name.span(), @@ -411,15 +425,11 @@ impl Order { continue; } - let prev_source = - if case_insensitive { prev.source.to_lowercase() } else { prev.source.to_string() }; - - let curr_source = - if case_insensitive { curr.source.to_lowercase() } else { curr.source.to_string() }; + let ordering = compare_sources(&prev.source, &curr.source, case_insensitive); let is_wrong_order = match order { - "asc" => prev_source > curr_source, - "desc" => prev_source < curr_source, + "asc" => ordering == std::cmp::Ordering::Greater, + "desc" => ordering == std::cmp::Ordering::Less, _ => false, }; From 71e2a33a749233927a4722c9ec64fdf69d63a987 Mon Sep 17 00:00:00 2001 From: Guillaume Piedigrossi Date: Mon, 16 Dec 2024 22:41:38 +0100 Subject: [PATCH 4/8] perf(linter/order): removed unused variables which cost performances --- crates/oxc_linter/src/rules/eslint/order.rs | 61 +-------------------- 1 file changed, 3 insertions(+), 58 deletions(-) diff --git a/crates/oxc_linter/src/rules/eslint/order.rs b/crates/oxc_linter/src/rules/eslint/order.rs index f5120e8b48562..6a488a699e8c4 100644 --- a/crates/oxc_linter/src/rules/eslint/order.rs +++ b/crates/oxc_linter/src/rules/eslint/order.rs @@ -6,11 +6,7 @@ use oxc_span::{CompactStr, Span}; use rustc_hash::FxHashMap; use serde::Deserialize; -use crate::{ - context::LintContext, - module_record::{ImportEntry, ImportImportName}, - rule::Rule, -}; +use crate::{context::LintContext, rule::Rule}; #[derive(Debug, Clone, Deserialize)] struct OrderConfig { @@ -71,23 +67,8 @@ pub struct Order { struct ImportInfo { source: CompactStr, span: Span, - kind: ImportKind, group: CompactStr, rank: usize, - specifiers: Vec, -} - -#[derive(Debug)] -struct ImportSpecifier { - name: CompactStr, - span: Span, - is_type: bool, -} - -#[derive(Debug, PartialEq)] -enum ImportKind { - Import, - ExportFrom, } static BUILTIN_MODULES: LazyLock> = LazyLock::new(|| { @@ -213,10 +194,8 @@ impl Order { imports.push(ImportInfo { source: CompactStr::new(source), span, - kind: ImportKind::Import, - group: CompactStr::new(&self.get_import_group(source).as_str()), + group: CompactStr::new(self.get_import_group(source).as_str()), rank: 0, - specifiers: self.collect_import_specifiers(entry), }); } @@ -227,10 +206,8 @@ impl Order { imports.push(ImportInfo { source: CompactStr::new(source), span: entry.span, - kind: ImportKind::ExportFrom, - group: CompactStr::new(&self.get_import_group(source).as_str()), + group: CompactStr::new(self.get_import_group(source).as_str()), rank: 0, - specifiers: vec![], }); } } @@ -299,38 +276,6 @@ impl Order { } } - // Helper methods - // 1. Helper functions for import collection and processing - fn collect_import_specifiers(&self, entry: &ImportEntry) -> Vec { - let mut specifiers = Vec::new(); - - match &entry.import_name { - ImportImportName::Name(import) => { - specifiers.push(ImportSpecifier { - name: CompactStr::new(import.name()), - span: import.span(), - is_type: entry.is_type, - }); - } - ImportImportName::Default(span) => { - specifiers.push(ImportSpecifier { - name: CompactStr::new(entry.local_name.name()), - span: *span, - is_type: entry.is_type, - }); - } - ImportImportName::NamespaceObject => { - specifiers.push(ImportSpecifier { - name: CompactStr::new(entry.local_name.name()), - span: entry.local_name.span(), - is_type: entry.is_type, - }); - } - } - - specifiers - } - // 3. Functions for ranking and group assignment fn assign_ranks(&self, imports: &mut [ImportInfo], config: &OrderConfig) { let group_ranks = self.get_group_ranks(config); From f382f9b28d0c21c0fc6ef0c0491939639e839138 Mon Sep 17 00:00:00 2001 From: Guillaume Piedigrossi Date: Mon, 16 Dec 2024 22:47:09 +0100 Subject: [PATCH 5/8] refactor(linter/order): move from eslint to the import folder --- crates/oxc_linter/src/rules.rs | 4 +- .../src/rules/{eslint => import}/order.rs | 39 +++++++------------ .../src/snapshots/import_order.snap | 27 +++++++++++++ 3 files changed, 42 insertions(+), 28 deletions(-) rename crates/oxc_linter/src/rules/{eslint => import}/order.rs (94%) create mode 100644 crates/oxc_linter/src/snapshots/import_order.snap diff --git a/crates/oxc_linter/src/rules.rs b/crates/oxc_linter/src/rules.rs index 41a6ed4cf6b31..7746c7de16350 100644 --- a/crates/oxc_linter/src/rules.rs +++ b/crates/oxc_linter/src/rules.rs @@ -26,6 +26,7 @@ mod import { pub mod no_namespace; pub mod no_self_import; pub mod no_webpack_loader_syntax; + pub mod order; pub mod unambiguous; } @@ -135,7 +136,6 @@ mod eslint { pub mod no_var; pub mod no_void; pub mod no_with; - pub mod order; pub mod prefer_exponentiation_operator; pub mod prefer_numeric_literals; pub mod prefer_object_has_own; @@ -626,7 +626,6 @@ oxc_macros::declare_all_lint_rules! { eslint::no_var, eslint::no_void, eslint::no_with, - eslint::order, eslint::prefer_exponentiation_operator, eslint::prefer_numeric_literals, eslint::prefer_object_has_own, @@ -659,6 +658,7 @@ oxc_macros::declare_all_lint_rules! { import::no_named_as_default_member, import::no_self_import, import::no_webpack_loader_syntax, + import::order, import::unambiguous, jest::consistent_test_it, jest::expect_expect, diff --git a/crates/oxc_linter/src/rules/eslint/order.rs b/crates/oxc_linter/src/rules/import/order.rs similarity index 94% rename from crates/oxc_linter/src/rules/eslint/order.rs rename to crates/oxc_linter/src/rules/import/order.rs index 6a488a699e8c4..c113b76f7b6fd 100644 --- a/crates/oxc_linter/src/rules/eslint/order.rs +++ b/crates/oxc_linter/src/rules/import/order.rs @@ -186,7 +186,6 @@ impl Order { let mut imports = Vec::new(); let module_record = ctx.module_record(); - // Collect import declarations for entry in &module_record.import_entries { let source = entry.module_request.name(); let span = entry.module_request.span(); @@ -199,7 +198,6 @@ impl Order { }); } - // Collect export from declarations for entry in &module_record.indirect_export_entries { if let Some(module_request) = &entry.module_request { let source = module_request.name(); @@ -216,7 +214,6 @@ impl Order { } fn get_import_group(&self, source: &str) -> String { - // Default groups: builtin, external, parent, sibling, index if source.starts_with('.') { if source == "." || source == ".." { "parent".into() @@ -242,20 +239,16 @@ impl Order { imports: &mut [ImportInfo], config: &OrderConfig, ) { - // Assign ranks based on groups self.assign_ranks(imports, config); - // Check alphabetical order if configured if let Some(alphabetize) = &config.alphabetize { self.check_alphabetical_order(ctx, imports, alphabetize); } - // Check newlines between imports if configured if let Some(newlines_between) = &config.newlines_between { self.check_newlines_between(ctx, imports, newlines_between); } - // Check for out of order imports self.check_order_violations(ctx, imports); } @@ -276,14 +269,12 @@ impl Order { } } - // 3. Functions for ranking and group assignment fn assign_ranks(&self, imports: &mut [ImportInfo], config: &OrderConfig) { let group_ranks = self.get_group_ranks(config); for import in imports.iter_mut() { import.rank = self.calculate_rank(&import.group, &group_ranks); - // Apply path group rankings if configured if let Some(path_groups) = &config.path_groups { if let Some(path_group_rank) = self.get_path_group_rank(&import.source, path_groups) { @@ -319,13 +310,11 @@ impl Order { *group_ranks.get(group).unwrap_or(&usize::MAX) } - // 4. Functions for path group handling fn get_path_group_rank(&self, source: &str, path_groups: &[PathGroup]) -> Option { for (index, path_group) in path_groups.iter().enumerate() { if self.matches_pattern(source, &path_group.pattern) { - let base_rank = index * 100; // Use multiplier to leave room for position adjustments + let base_rank = index * 100; - // Adjust rank based on position if specified match path_group.position.as_deref() { Some("before") => return Some(base_rank.saturating_sub(50)), Some("after") => return Some(base_rank + 50), @@ -347,7 +336,6 @@ impl Order { } } - // 5. Functions for alphabetical ordering fn check_alphabetical_order( &self, ctx: &LintContext, @@ -390,7 +378,6 @@ impl Order { } } - // 6. Functions for newlines checking fn check_newlines_between( &self, ctx: &LintContext, @@ -469,7 +456,7 @@ fn test() { let pass = vec![ // Basic sorting ( - r#" + r" import fs from 'fs'; import path from 'path'; @@ -479,7 +466,7 @@ fn test() { import foo from '../foo'; import bar from './bar'; - "#, + ", Some(serde_json::json!({ "groups": ["builtin", "external", "parent", "sibling", "index"], "newlines-between": "always" @@ -487,11 +474,11 @@ fn test() { ), // Alphabetical order ( - r#" + r" import a from 'a'; import b from 'b'; import c from 'c'; - "#, + ", Some(serde_json::json!({ "alphabetize": { "order": "asc", @@ -501,7 +488,7 @@ fn test() { ), // Mixed groups with correct newlines ( - r#" + r" import path from 'path'; import fs from 'fs'; @@ -509,7 +496,7 @@ fn test() { import foo from '../foo'; import bar from './bar'; - "#, + ", Some(serde_json::json!({ "groups": ["builtin", "external", ["parent", "sibling"]], "newlines-between": "always" @@ -520,20 +507,20 @@ fn test() { let fail = vec![ // Wrong order ( - r#" + r" import _ from 'lodash'; import fs from 'fs'; - "#, + ", Some(serde_json::json!({ "groups": ["builtin", "external"] })), ), // Missing newline between groups ( - r#" + r" import fs from 'fs'; import _ from 'lodash'; // Should have newline before this - "#, + ", Some(serde_json::json!({ "groups": ["builtin", "external"], "newlines-between": "always" @@ -541,10 +528,10 @@ fn test() { ), // Wrong alphabetical order ( - r#" + r" import b from 'b'; import a from 'a'; - "#, + ", Some(serde_json::json!({ "alphabetize": { "order": "asc" diff --git a/crates/oxc_linter/src/snapshots/import_order.snap b/crates/oxc_linter/src/snapshots/import_order.snap new file mode 100644 index 0000000000000..5e2011687e274 --- /dev/null +++ b/crates/oxc_linter/src/snapshots/import_order.snap @@ -0,0 +1,27 @@ +--- +source: crates/oxc_linter/src/tester.rs +snapshot_kind: text +--- + ⚠ eslint-plugin-import(order): Import from 'fs' should occur before import from 'lodash' + ╭─[order.tsx:3:28] + 2 │ import _ from 'lodash'; + 3 │ import fs from 'fs'; + · ──── + 4 │ + ╰──── + + ⚠ eslint-plugin-import(order): There should be at least one empty line between import groups + ╭─[order.tsx:3:27] + 2 │ import fs from 'fs'; + 3 │ import _ from 'lodash'; // Should have newline before this + · ──────── + 4 │ + ╰──── + + ⚠ eslint-plugin-import(order): Imports must be sorted in asc order. 'a' should be before 'b'. + ╭─[order.tsx:3:27] + 2 │ import b from 'b'; + 3 │ import a from 'a'; + · ─── + 4 │ + ╰──── From 793dde2f4c4744fc494053c0863bb7b8b7aafb4d Mon Sep 17 00:00:00 2001 From: Guillaume Piedigrossi Date: Mon, 16 Dec 2024 23:54:13 +0100 Subject: [PATCH 6/8] feat(linter/order): handle group path patterns with regex --- crates/oxc_linter/src/rules/import/order.rs | 357 ++++++++++-------- .../src/snapshots/import_order.snap | 8 + 2 files changed, 207 insertions(+), 158 deletions(-) diff --git a/crates/oxc_linter/src/rules/import/order.rs b/crates/oxc_linter/src/rules/import/order.rs index c113b76f7b6fd..2edff95536c23 100644 --- a/crates/oxc_linter/src/rules/import/order.rs +++ b/crates/oxc_linter/src/rules/import/order.rs @@ -13,47 +13,34 @@ struct OrderConfig { groups: Option>, #[serde(rename = "pathGroups")] path_groups: Option>, - #[serde(rename = "pathGroupsExcludedImportTypes")] - path_groups_excluded_import_types: Option>, #[serde(rename = "newlines-between")] newlines_between: Option, - named: Option, alphabetize: Option, - #[serde(rename = "warnOnUnassignedImports")] - warn_on_unassigned_imports: Option, } #[derive(Debug, Clone, Deserialize)] -struct PathGroup { - pattern: CompactStr, - #[serde(rename = "patternOptions")] - pattern_options: Option, - group: String, - position: Option, -} - -#[derive(Debug, Clone, Deserialize)] -struct PatternOptions { - #[serde(rename = "nocomment")] - no_comment: Option, +#[serde(rename_all = "lowercase")] +enum PredefinedGroup { + Builtin, + External, + Internal, + Parent, + Sibling, + Index, + Object, } #[derive(Debug, Clone, Deserialize)] -struct NamedOrder { - enabled: Option, - import: Option, - export: Option, - require: Option, - #[serde(rename = "cjsExports")] - cjs_exports: Option, - types: Option, +struct PathGroup { + pattern: CompactStr, + #[serde(rename = "group")] + group: PredefinedGroup, + position: Option, } #[derive(Debug, Clone, Deserialize)] struct Alphabetize { order: Option, - #[serde(rename = "orderImportKind")] - order_import_kind: Option, #[serde(rename = "caseInsensitive")] case_insensitive: Option, } @@ -240,31 +227,116 @@ impl Order { config: &OrderConfig, ) { self.assign_ranks(imports, config); + self.check_all_rules(ctx, imports, config); + } - if let Some(alphabetize) = &config.alphabetize { - self.check_alphabetical_order(ctx, imports, alphabetize); - } - - if let Some(newlines_between) = &config.newlines_between { - self.check_newlines_between(ctx, imports, newlines_between); + fn check_all_rules(&self, ctx: &LintContext, imports: &[ImportInfo], config: &OrderConfig) { + if imports.len() <= 1 { + return; } - self.check_order_violations(ctx, imports); - } + let source_code = ctx.source_text(); + let alphabetize = &config.alphabetize; + let newlines_setting = config.newlines_between.as_deref(); + + // Get alphabetization settings if enabled + let (check_alpha, alpha_case_insensitive, alpha_order) = if let Some(alpha) = alphabetize { + ( + alpha.order.as_deref() != Some("ignore"), + alpha.case_insensitive.unwrap_or(false), + alpha.order.as_deref().unwrap_or("ignore"), + ) + } else { + (false, false, "ignore") + }; - fn check_order_violations(&self, ctx: &LintContext, imports: &[ImportInfo]) { + // Single pass through imports checking all rules for i in 1..imports.len() { let prev = &imports[i - 1]; let curr = &imports[i]; + // Check order violations if curr.rank < prev.rank { - ctx.diagnostic( - OxcDiagnostic::warn(format!( + let message = if curr.rank % 100 != 0 { + format!( + "Import from '{}' should occur {} import from '{}'", + curr.source, + if curr.rank % 100 == 50 { "after" } else { "before" }, + prev.source + ) + } else { + format!( "Import from '{}' should occur before import from '{}'", curr.source, prev.source - )) - .with_label(curr.span), - ); + ) + }; + ctx.diagnostic(OxcDiagnostic::warn(message).with_label(curr.span)); + } + + // Check alphabetical order within same group + if check_alpha && prev.rank == curr.rank { + let ordering = compare_sources(&prev.source, &curr.source, alpha_case_insensitive); + let is_wrong_order = match alpha_order { + "asc" => ordering == std::cmp::Ordering::Greater, + "desc" => ordering == std::cmp::Ordering::Less, + _ => false, + }; + + if is_wrong_order { + ctx.diagnostic( + OxcDiagnostic::warn(format!( + "Imports must be sorted in {} order. '{}' should be before '{}'.", + alpha_order, curr.source, prev.source + )) + .with_label(curr.span), + ); + } + } + + // Check newlines between imports + if let Some(newlines_setting) = newlines_setting { + if newlines_setting != "ignore" { + let lines_between = self.count_newlines_between( + source_code, + prev.span.end.try_into().unwrap(), + curr.span.start.try_into().unwrap(), + ); + let is_different_group = prev.group != curr.group; + + match newlines_setting { + "always" => { + if is_different_group && lines_between == 0 { + ctx.diagnostic( + OxcDiagnostic::warn( + "There should be at least one empty line between import groups", + ) + .with_label(curr.span), + ); + } + } + "never" => { + if lines_between > 0 { + ctx.diagnostic( + OxcDiagnostic::warn( + "There should be no empty lines between imports", + ) + .with_label(curr.span), + ); + } + } + "always-and-inside-groups" => { + if lines_between == 0 { + ctx.diagnostic( + OxcDiagnostic::warn( + "There should be at least one empty line between imports", + ) + .with_label(curr.span), + ); + } + } + _ => {} + } + } } } } @@ -274,7 +346,6 @@ impl Order { for import in imports.iter_mut() { import.rank = self.calculate_rank(&import.group, &group_ranks); - if let Some(path_groups) = &config.path_groups { if let Some(path_group_rank) = self.get_path_group_rank(&import.source, path_groups) { @@ -307,17 +378,26 @@ impl Order { } fn calculate_rank(&self, group: &str, group_ranks: &FxHashMap) -> usize { - *group_ranks.get(group).unwrap_or(&usize::MAX) + match group { + "builtin" => 0 * 100, + "external" => 1 * 100, + "internal" => 2 * 100, + "parent" => 3 * 100, + "sibling" => 4 * 100, + "index" => 5 * 100, + _ => *group_ranks.get(group).unwrap_or(&(usize::MAX / 100)) * 100, + } } fn get_path_group_rank(&self, source: &str, path_groups: &[PathGroup]) -> Option { - for (index, path_group) in path_groups.iter().enumerate() { + for (_, path_group) in path_groups.iter().enumerate() { if self.matches_pattern(source, &path_group.pattern) { - let base_rank = index * 100; + let target_group_rank = self.get_predefined_group_rank(&path_group.group); + let base_rank = target_group_rank * 100; // Multiply by 100 to leave space for positioning match path_group.position.as_deref() { - Some("before") => return Some(base_rank.saturating_sub(50)), - Some("after") => return Some(base_rank + 50), + Some("before") => return Some(base_rank - 10), + Some("after") => return Some(base_rank + 110), // Add more than 100 to ensure it's after the next group _ => return Some(base_rank), } } @@ -325,122 +405,33 @@ impl Order { None } - fn matches_pattern(&self, source: &str, pattern: &str) -> bool { - // Simple pattern matching implementation - // Could be enhanced with proper glob matching - if pattern.contains('*') { - let pattern = pattern.replace('*', ".*"); - regex::Regex::new(&pattern).map(|re| re.is_match(source)).unwrap_or(false) - } else { - source == pattern - } - } - - fn check_alphabetical_order( - &self, - ctx: &LintContext, - imports: &[ImportInfo], - alphabetize: &Alphabetize, - ) { - let case_insensitive = alphabetize.case_insensitive.unwrap_or(false); - let order = alphabetize.order.as_deref().unwrap_or("ignore"); - - if order == "ignore" { - return; - } - - for window in imports.windows(2) { - let prev = &window[0]; - let curr = &window[1]; - - // Only compare imports within the same group - if prev.rank != curr.rank { - continue; - } - - let ordering = compare_sources(&prev.source, &curr.source, case_insensitive); - - let is_wrong_order = match order { - "asc" => ordering == std::cmp::Ordering::Greater, - "desc" => ordering == std::cmp::Ordering::Less, - _ => false, - }; - - if is_wrong_order { - ctx.diagnostic( - OxcDiagnostic::warn(format!( - "Imports must be sorted in {} order. '{}' should be before '{}'.", - order, curr.source, prev.source - )) - .with_label(curr.span), - ); - } + fn get_predefined_group_rank(&self, group: &PredefinedGroup) -> usize { + match group { + PredefinedGroup::Builtin => 0, + PredefinedGroup::External => 1, + PredefinedGroup::Internal => 2, + PredefinedGroup::Parent => 3, + PredefinedGroup::Sibling => 4, + PredefinedGroup::Index => 5, + PredefinedGroup::Object => 6, } } - fn check_newlines_between( - &self, - ctx: &LintContext, - imports: &[ImportInfo], - newlines_setting: &str, - ) { - if newlines_setting == "ignore" { - return; + fn matches_pattern(&self, source: &str, pattern: &str) -> bool { + // Handle regular glob patterns + if pattern.contains('*') { + let regex_pattern = pattern + .replace(".", "\\.") // Escape dots + .replace("**", "__DOUBLE_STAR__") // Preserve ** pattern + .replace('*', "[^/]*") // Single * doesn't match across directories + .replace("__DOUBLE_STAR__", ".*"); // ** matches across directories + return regex::Regex::new(&format!("^{}$", regex_pattern)) + .map(|re| re.is_match(source)) + .unwrap_or(false); } - let source_code = ctx.source_text(); - - for window in imports.windows(2) { - let prev = &window[0]; - let curr = &window[1]; - - let lines_between = self.count_newlines_between( - source_code, - prev.span.end.try_into().unwrap(), - curr.span.start.try_into().unwrap(), - ); - - // Only check for newlines when transitioning between different groups - let is_different_group = self.is_different_group(prev, curr); - - match newlines_setting { - "always" => { - if is_different_group && lines_between == 0 { - ctx.diagnostic( - OxcDiagnostic::warn( - "There should be at least one empty line between import groups", - ) - .with_label(curr.span), - ); - } - } - "never" => { - if lines_between > 0 { - ctx.diagnostic( - OxcDiagnostic::warn("There should be no empty lines between imports") - .with_label(curr.span), - ); - } - } - "always-and-inside-groups" => { - if lines_between == 0 { - ctx.diagnostic( - OxcDiagnostic::warn( - "There should be at least one empty line between imports", - ) - .with_label(curr.span), - ); - } - } - _ => {} - } - } - } - - fn is_different_group(&self, prev: &ImportInfo, curr: &ImportInfo) -> bool { - // Compare the base group names rather than ranks - // This ensures we only require newlines between actual different groups - prev.group != curr.group + // Exact match + source == pattern } fn count_newlines_between(&self, source: &str, start: usize, end: usize) -> usize { @@ -502,6 +493,23 @@ fn test() { "newlines-between": "always" })), ), + // Test with pathGroups + ( + r" + import fs from 'fs'; + import _ from 'lodash'; + import MyComponent from '~/components/MyComponent'; + import utils from './utils'; + ", + Some(serde_json::json!({ + "groups": ["builtin", "external", "internal", "parent", "sibling", "index"], + "pathGroups": [{ + "pattern": "~/components/**", + "group": "internal", + "position": "after" + }] + })), + ), ]; let fail = vec![ @@ -538,7 +546,40 @@ fn test() { } })), ), + ( + r" + import MyComponent from '~/components/MyComponent'; + import _ from 'lodash'; + ", + Some(serde_json::json!({ + "groups": ["builtin", "external", "internal"], + "pathGroups": [{ + "pattern": "~/components/**", + "group": "internal", + "position": "after" + }] + })), + ), ]; Tester::new(Order::NAME, Order::CATEGORY, pass, fail).test_and_snapshot(); } + +#[test] +fn test_matches_pattern() { + let order = Order::default(); + + // Root-relative paths + assert!(order.matches_pattern("~/components/Button", "~/components/**")); + assert!(order.matches_pattern("~/components/forms/Input", "~/components/**")); + assert!(!order.matches_pattern("other/Button", "~/components/**")); + + // Regular glob patterns + assert!(order.matches_pattern("@org/utils", "@org/*")); + assert!(order.matches_pattern("@org/deep/nested/util", "@org/**")); + assert!(!order.matches_pattern("@org/deep/util", "@org/*")); + + // Exact matches + assert!(order.matches_pattern("exact-match", "exact-match")); + assert!(!order.matches_pattern("not-exact", "exact-match")); +} diff --git a/crates/oxc_linter/src/snapshots/import_order.snap b/crates/oxc_linter/src/snapshots/import_order.snap index 5e2011687e274..a4be7491a6e4d 100644 --- a/crates/oxc_linter/src/snapshots/import_order.snap +++ b/crates/oxc_linter/src/snapshots/import_order.snap @@ -25,3 +25,11 @@ snapshot_kind: text · ─── 4 │ ╰──── + + ⚠ eslint-plugin-import(order): Import from 'lodash' should occur before import from '~/components/MyComponent' + ╭─[order.tsx:3:27] + 2 │ import MyComponent from '~/components/MyComponent'; + 3 │ import _ from 'lodash'; + · ──────── + 4 │ + ╰──── From 2eaa197a2891031a74f0562e32dce359855fa950 Mon Sep 17 00:00:00 2001 From: Guillaume Piedigrossi Date: Tue, 17 Dec 2024 00:03:54 +0100 Subject: [PATCH 7/8] style(linter/order): fix clippy warnings --- crates/oxc_linter/src/rules/import/order.rs | 527 ++++++++++---------- 1 file changed, 256 insertions(+), 271 deletions(-) diff --git a/crates/oxc_linter/src/rules/import/order.rs b/crates/oxc_linter/src/rules/import/order.rs index 2edff95536c23..2fb405f3db9fb 100644 --- a/crates/oxc_linter/src/rules/import/order.rs +++ b/crates/oxc_linter/src/rules/import/order.rs @@ -1,5 +1,4 @@ -use std::sync::LazyLock; - +use cow_utils::CowUtils; use oxc_diagnostics::OxcDiagnostic; use oxc_macros::declare_oxc_lint; use oxc_span::{CompactStr, Span}; @@ -58,40 +57,6 @@ struct ImportInfo { rank: usize, } -static BUILTIN_MODULES: LazyLock> = LazyLock::new(|| { - let mut set = rustc_hash::FxHashSet::default(); - set.extend([ - "assert", - "buffer", - "child_process", - "cluster", - "crypto", - "dgram", - "dns", - "domain", - "events", - "fs", - "http", - "https", - "net", - "os", - "path", - "punycode", - "querystring", - "readline", - "stream", - "string_decoder", - "tls", - "tty", - "url", - "util", - "v8", - "vm", - "zlib", - ]); - set -}); - declare_oxc_lint!( /// ### What it does /// Enforces a convention in module import order. @@ -152,8 +117,8 @@ impl Rule for Order { } fn run_once(&self, ctx: &LintContext) { if let Some(config) = &self.config { - let mut imports = self.collect_imports(ctx); - self.check_imports_order(ctx, &mut imports, config); + let mut imports = collect_imports(ctx); + check_imports_order(ctx, &mut imports, config); } } } @@ -168,276 +133,298 @@ fn compare_sources(a: &str, b: &str, case_insensitive: bool) -> std::cmp::Orderi } } -impl Order { - fn collect_imports(&self, ctx: &LintContext) -> Vec { - let mut imports = Vec::new(); - let module_record = ctx.module_record(); +fn collect_imports(ctx: &LintContext) -> Vec { + let mut imports = Vec::new(); + let module_record = ctx.module_record(); + + for entry in &module_record.import_entries { + let source = entry.module_request.name(); + let span = entry.module_request.span(); - for entry in &module_record.import_entries { - let source = entry.module_request.name(); - let span = entry.module_request.span(); + imports.push(ImportInfo { + source: CompactStr::new(source), + span, + group: CompactStr::new(get_import_group(source).as_str()), + rank: 0, + }); + } + for entry in &module_record.indirect_export_entries { + if let Some(module_request) = &entry.module_request { + let source = module_request.name(); imports.push(ImportInfo { source: CompactStr::new(source), - span, - group: CompactStr::new(self.get_import_group(source).as_str()), + span: entry.span, + group: CompactStr::new(get_import_group(source).as_str()), rank: 0, }); } - - for entry in &module_record.indirect_export_entries { - if let Some(module_request) = &entry.module_request { - let source = module_request.name(); - imports.push(ImportInfo { - source: CompactStr::new(source), - span: entry.span, - group: CompactStr::new(self.get_import_group(source).as_str()), - rank: 0, - }); - } - } - - imports } - fn get_import_group(&self, source: &str) -> String { - if source.starts_with('.') { - if source == "." || source == ".." { - "parent".into() - } else if source.starts_with("./") { - "sibling".into() - } else { - "parent".into() - } - } else if self.is_builtin_module(source) { - "builtin".into() - } else { - "external".into() - } - } + imports +} - fn is_builtin_module(&self, source: &str) -> bool { - BUILTIN_MODULES.contains(&source) - } +fn check_imports_order(ctx: &LintContext, imports: &mut [ImportInfo], config: &OrderConfig) { + assign_ranks(imports, config); + check_all_rules(ctx, imports, config); +} - fn check_imports_order( - &self, - ctx: &LintContext, - imports: &mut [ImportInfo], - config: &OrderConfig, - ) { - self.assign_ranks(imports, config); - self.check_all_rules(ctx, imports, config); +fn check_all_rules(ctx: &LintContext, imports: &[ImportInfo], config: &OrderConfig) { + if imports.len() <= 1 { + return; } - fn check_all_rules(&self, ctx: &LintContext, imports: &[ImportInfo], config: &OrderConfig) { - if imports.len() <= 1 { - return; - } + let source_code = ctx.source_text(); + let alphabetize = &config.alphabetize; + let newlines_setting = config.newlines_between.as_deref(); - let source_code = ctx.source_text(); - let alphabetize = &config.alphabetize; - let newlines_setting = config.newlines_between.as_deref(); - - // Get alphabetization settings if enabled - let (check_alpha, alpha_case_insensitive, alpha_order) = if let Some(alpha) = alphabetize { - ( - alpha.order.as_deref() != Some("ignore"), - alpha.case_insensitive.unwrap_or(false), - alpha.order.as_deref().unwrap_or("ignore"), - ) - } else { - (false, false, "ignore") - }; - - // Single pass through imports checking all rules - for i in 1..imports.len() { - let prev = &imports[i - 1]; - let curr = &imports[i]; - - // Check order violations - if curr.rank < prev.rank { - let message = if curr.rank % 100 != 0 { - format!( - "Import from '{}' should occur {} import from '{}'", - curr.source, - if curr.rank % 100 == 50 { "after" } else { "before" }, - prev.source - ) - } else { - format!( - "Import from '{}' should occur before import from '{}'", - curr.source, prev.source - ) - }; - ctx.diagnostic(OxcDiagnostic::warn(message).with_label(curr.span)); - } + // Get alphabetization settings if enabled + let (check_alpha, alpha_case_insensitive, alpha_order) = if let Some(alpha) = alphabetize { + ( + alpha.order.as_deref() != Some("ignore"), + alpha.case_insensitive.unwrap_or(false), + alpha.order.as_deref().unwrap_or("ignore"), + ) + } else { + (false, false, "ignore") + }; + + // Single pass through imports checking all rules + for i in 1..imports.len() { + let prev = &imports[i - 1]; + let curr = &imports[i]; + + // Check order violations + if curr.rank < prev.rank { + let message = if curr.rank % 100 != 0 { + format!( + "Import from '{}' should occur {} import from '{}'", + curr.source, + if curr.rank % 100 == 50 { "after" } else { "before" }, + prev.source + ) + } else { + format!( + "Import from '{}' should occur before import from '{}'", + curr.source, prev.source + ) + }; + ctx.diagnostic(OxcDiagnostic::warn(message).with_label(curr.span)); + } - // Check alphabetical order within same group - if check_alpha && prev.rank == curr.rank { - let ordering = compare_sources(&prev.source, &curr.source, alpha_case_insensitive); - let is_wrong_order = match alpha_order { - "asc" => ordering == std::cmp::Ordering::Greater, - "desc" => ordering == std::cmp::Ordering::Less, - _ => false, - }; - - if is_wrong_order { - ctx.diagnostic( - OxcDiagnostic::warn(format!( - "Imports must be sorted in {} order. '{}' should be before '{}'.", - alpha_order, curr.source, prev.source - )) - .with_label(curr.span), - ); - } + // Check alphabetical order within same group + if check_alpha && prev.rank == curr.rank { + let ordering = compare_sources(&prev.source, &curr.source, alpha_case_insensitive); + let is_wrong_order = match alpha_order { + "asc" => ordering == std::cmp::Ordering::Greater, + "desc" => ordering == std::cmp::Ordering::Less, + _ => false, + }; + + if is_wrong_order { + ctx.diagnostic( + OxcDiagnostic::warn(format!( + "Imports must be sorted in {} order. '{}' should be before '{}'.", + alpha_order, curr.source, prev.source + )) + .with_label(curr.span), + ); } + } - // Check newlines between imports - if let Some(newlines_setting) = newlines_setting { - if newlines_setting != "ignore" { - let lines_between = self.count_newlines_between( - source_code, - prev.span.end.try_into().unwrap(), - curr.span.start.try_into().unwrap(), - ); - let is_different_group = prev.group != curr.group; - - match newlines_setting { - "always" => { - if is_different_group && lines_between == 0 { - ctx.diagnostic( - OxcDiagnostic::warn( - "There should be at least one empty line between import groups", - ) - .with_label(curr.span), - ); - } + // Check newlines between imports + if let Some(newlines_setting) = newlines_setting { + if newlines_setting != "ignore" { + let lines_between = count_newlines_between( + source_code, + prev.span.end.try_into().unwrap(), + curr.span.start.try_into().unwrap(), + ); + let is_different_group = prev.group != curr.group; + + match newlines_setting { + "always" => { + if is_different_group && lines_between == 0 { + ctx.diagnostic( + OxcDiagnostic::warn( + "There should be at least one empty line between import groups", + ) + .with_label(curr.span), + ); } - "never" => { - if lines_between > 0 { - ctx.diagnostic( - OxcDiagnostic::warn( - "There should be no empty lines between imports", - ) - .with_label(curr.span), - ); - } + } + "never" => { + if lines_between > 0 { + ctx.diagnostic( + OxcDiagnostic::warn( + "There should be no empty lines between imports", + ) + .with_label(curr.span), + ); } - "always-and-inside-groups" => { - if lines_between == 0 { - ctx.diagnostic( - OxcDiagnostic::warn( - "There should be at least one empty line between imports", - ) - .with_label(curr.span), - ); - } + } + "always-and-inside-groups" => { + if lines_between == 0 { + ctx.diagnostic( + OxcDiagnostic::warn( + "There should be at least one empty line between imports", + ) + .with_label(curr.span), + ); } - _ => {} } + _ => {} } } } } +} - fn assign_ranks(&self, imports: &mut [ImportInfo], config: &OrderConfig) { - let group_ranks = self.get_group_ranks(config); - - for import in imports.iter_mut() { - import.rank = self.calculate_rank(&import.group, &group_ranks); - if let Some(path_groups) = &config.path_groups { - if let Some(path_group_rank) = self.get_path_group_rank(&import.source, path_groups) - { - import.rank = path_group_rank; - } - } +fn get_import_group(source: &str) -> String { + if source.starts_with('.') { + if source == "." || source == ".." { + "parent".into() + } else if source.starts_with("./") { + "sibling".into() + } else { + "parent".into() } + } else if is_builtin_module(source) { + "builtin".into() + } else { + "external".into() } +} + +fn is_builtin_module(source: &str) -> bool { + let mut builtin_modules = rustc_hash::FxHashSet::default(); + builtin_modules.extend([ + "assert", + "buffer", + "child_process", + "cluster", + "crypto", + "dgram", + "dns", + "domain", + "events", + "fs", + "http", + "https", + "net", + "os", + "path", + "punycode", + "querystring", + "readline", + "stream", + "string_decoder", + "tls", + "tty", + "url", + "util", + "v8", + "vm", + "zlib", + ]); + + builtin_modules.contains(&source) +} - fn get_group_ranks(&self, config: &OrderConfig) -> FxHashMap { - let mut default_groups = FxHashMap::default(); - default_groups.insert(CompactStr::new("builtin"), 0); - default_groups.insert(CompactStr::new("external"), 1); - default_groups.insert(CompactStr::new("parent"), 2); - default_groups.insert(CompactStr::new("sibling"), 3); - default_groups.insert(CompactStr::new("index"), 4); +fn assign_ranks(imports: &mut [ImportInfo], config: &OrderConfig) { + let group_ranks = get_group_ranks(config); - if config.groups.is_none() { - return default_groups; + for import in imports.iter_mut() { + import.rank = calculate_rank(&import.group, &group_ranks); + if let Some(path_groups) = &config.path_groups { + if let Some(path_group_rank) = get_path_group_rank(&import.source, path_groups) { + import.rank = path_group_rank; + } } + } +} - let groups = config.groups.as_ref().unwrap(); - let mut ranks = FxHashMap::default(); +fn get_group_ranks(config: &OrderConfig) -> FxHashMap { + let mut default_groups = FxHashMap::default(); + default_groups.insert(CompactStr::new("builtin"), 0); + default_groups.insert(CompactStr::new("external"), 1); + default_groups.insert(CompactStr::new("parent"), 2); + default_groups.insert(CompactStr::new("sibling"), 3); + default_groups.insert(CompactStr::new("index"), 4); - for (index, group) in groups.iter().enumerate() { - ranks.insert(group.clone(), index); - } + if config.groups.is_none() { + return default_groups; + } + + let groups = config.groups.as_ref().unwrap(); + let mut ranks = FxHashMap::default(); - ranks + for (index, group) in groups.iter().enumerate() { + ranks.insert(group.clone(), index); } - fn calculate_rank(&self, group: &str, group_ranks: &FxHashMap) -> usize { - match group { - "builtin" => 0 * 100, - "external" => 1 * 100, - "internal" => 2 * 100, - "parent" => 3 * 100, - "sibling" => 4 * 100, - "index" => 5 * 100, - _ => *group_ranks.get(group).unwrap_or(&(usize::MAX / 100)) * 100, - } + ranks +} + +fn calculate_rank(group: &str, group_ranks: &FxHashMap) -> usize { + match group { + "builtin" => 0, + "external" => 100, + "internal" => 200, + "parent" => 300, + "sibling" => 400, + "index" => 500, + _ => *group_ranks.get(group).unwrap_or(&(usize::MAX / 100)) * 100, } +} - fn get_path_group_rank(&self, source: &str, path_groups: &[PathGroup]) -> Option { - for (_, path_group) in path_groups.iter().enumerate() { - if self.matches_pattern(source, &path_group.pattern) { - let target_group_rank = self.get_predefined_group_rank(&path_group.group); - let base_rank = target_group_rank * 100; // Multiply by 100 to leave space for positioning +fn get_path_group_rank(source: &str, path_groups: &[PathGroup]) -> Option { + for path_group in path_groups { + if matches_pattern(source, &path_group.pattern) { + let target_group_rank = get_predefined_group_rank(&path_group.group); + let base_rank = target_group_rank * 100; // Multiply by 100 to leave space for positioning - match path_group.position.as_deref() { - Some("before") => return Some(base_rank - 10), - Some("after") => return Some(base_rank + 110), // Add more than 100 to ensure it's after the next group - _ => return Some(base_rank), - } + match path_group.position.as_deref() { + Some("before") => return Some(base_rank - 10), + Some("after") => return Some(base_rank + 110), // Add more than 100 to ensure it's after the next group + _ => return Some(base_rank), } } - None } + None +} - fn get_predefined_group_rank(&self, group: &PredefinedGroup) -> usize { - match group { - PredefinedGroup::Builtin => 0, - PredefinedGroup::External => 1, - PredefinedGroup::Internal => 2, - PredefinedGroup::Parent => 3, - PredefinedGroup::Sibling => 4, - PredefinedGroup::Index => 5, - PredefinedGroup::Object => 6, - } +fn get_predefined_group_rank(group: &PredefinedGroup) -> usize { + match group { + PredefinedGroup::Builtin => 0, + PredefinedGroup::External => 1, + PredefinedGroup::Internal => 2, + PredefinedGroup::Parent => 3, + PredefinedGroup::Sibling => 4, + PredefinedGroup::Index => 5, + PredefinedGroup::Object => 6, } +} - fn matches_pattern(&self, source: &str, pattern: &str) -> bool { - // Handle regular glob patterns - if pattern.contains('*') { - let regex_pattern = pattern - .replace(".", "\\.") // Escape dots - .replace("**", "__DOUBLE_STAR__") // Preserve ** pattern - .replace('*', "[^/]*") // Single * doesn't match across directories - .replace("__DOUBLE_STAR__", ".*"); // ** matches across directories - return regex::Regex::new(&format!("^{}$", regex_pattern)) - .map(|re| re.is_match(source)) - .unwrap_or(false); - } - - // Exact match - source == pattern +fn matches_pattern(source: &str, pattern: &str) -> bool { + // Handle regular glob patterns + if pattern.contains('*') { + let escaped = pattern.cow_replace('.', "\\."); + let with_temp_stars = escaped.cow_replace("**", "__DOUBLE_STAR__"); + let with_single_stars = with_temp_stars.cow_replace('*', "[^/]*"); + let regex_pattern = with_single_stars.cow_replace("__DOUBLE_STAR__", ".*"); + return regex::Regex::new(&format!("^{regex_pattern}$")) + .map(|re| re.is_match(source)) + .unwrap_or(false); } - fn count_newlines_between(&self, source: &str, start: usize, end: usize) -> usize { - source[start..end].chars().filter(|&c| c == '\n').count().saturating_sub(1) - // Subtract 1 because we don't count the line with the import - } + // Exact match + source == pattern +} + +fn count_newlines_between(source: &str, start: usize, end: usize) -> usize { + source[start..end].chars().filter(|&c| c == '\n').count().saturating_sub(1) + // Subtract 1 because we don't count the line with the import } #[test] @@ -567,19 +554,17 @@ fn test() { #[test] fn test_matches_pattern() { - let order = Order::default(); - // Root-relative paths - assert!(order.matches_pattern("~/components/Button", "~/components/**")); - assert!(order.matches_pattern("~/components/forms/Input", "~/components/**")); - assert!(!order.matches_pattern("other/Button", "~/components/**")); + assert!(matches_pattern("~/components/Button", "~/components/**")); + assert!(matches_pattern("~/components/forms/Input", "~/components/**")); + assert!(!matches_pattern("other/Button", "~/components/**")); // Regular glob patterns - assert!(order.matches_pattern("@org/utils", "@org/*")); - assert!(order.matches_pattern("@org/deep/nested/util", "@org/**")); - assert!(!order.matches_pattern("@org/deep/util", "@org/*")); + assert!(matches_pattern("@org/utils", "@org/*")); + assert!(matches_pattern("@org/deep/nested/util", "@org/**")); + assert!(!matches_pattern("@org/deep/util", "@org/*")); // Exact matches - assert!(order.matches_pattern("exact-match", "exact-match")); - assert!(!order.matches_pattern("not-exact", "exact-match")); + assert!(matches_pattern("exact-match", "exact-match")); + assert!(!matches_pattern("not-exact", "exact-match")); } From 30d87abfb5eddd1e2ae3fcf3318c3b78447b2797 Mon Sep 17 00:00:00 2001 From: Guillaume Piedigrossi Date: Tue, 24 Dec 2024 15:34:09 +0100 Subject: [PATCH 8/8] perf(import/order): fix performances issues where the config has a size over 16bits --- crates/oxc_linter/src/rules/import/order.rs | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/crates/oxc_linter/src/rules/import/order.rs b/crates/oxc_linter/src/rules/import/order.rs index 2fb405f3db9fb..7e2b5778646c2 100644 --- a/crates/oxc_linter/src/rules/import/order.rs +++ b/crates/oxc_linter/src/rules/import/order.rs @@ -46,7 +46,7 @@ struct Alphabetize { #[derive(Debug, Default, Clone)] pub struct Order { - config: Option, + config: Option>, } #[derive(Debug)] @@ -105,15 +105,13 @@ declare_oxc_lint!( /// var path = require('path'); /// ``` Order, - style, - pending // TODO: describe fix capabilities. Remove if no fix can be done, - // keep at 'pending' if you think one could be added but don't know how. - // Options are 'fix', 'fix_dangerous', 'suggestion', and 'conditional_fix_suggestion' + nursery, + pending ); impl Rule for Order { fn from_configuration(value: serde_json::Value) -> Self { - Self { config: serde_json::from_value(value).ok() } + Self { config: serde_json::from_value(value).ok().map(Box::new) } } fn run_once(&self, ctx: &LintContext) { if let Some(config) = &self.config {