Skip to content

Commit

Permalink
refactor(organize_import): code cleanup
Browse files Browse the repository at this point in the history
  • Loading branch information
@Conaclos
Conaclos committed Oct 9, 2023
1 parent 608e0fa commit e568e46
Show file tree
Hide file tree
Showing 3 changed files with 132 additions and 167 deletions.
194 changes: 55 additions & 139 deletions crates/biome_js_analyze/src/assists/correctness/organize_imports.rs
View file
Original file line number Diff line number Diff line change
@@ -1,10 +1,4 @@
use std::{
cell::Cell,
cmp::Ordering,
collections::{btree_map::Entry, BTreeMap},
iter,
mem::take,
};
use std::{cell::Cell, cmp::Ordering, collections::BTreeMap, iter, mem::take};

use biome_analyze::{
context::RuleContext, declare_rule, ActionCategory, Ast, FixKind, Rule, SourceActionKind,
Expand All @@ -18,7 +12,7 @@ use biome_js_syntax::{
};
use biome_rowan::{
chain_trivia_pieces, syntax::SyntaxTrivia, AstNode, AstNodeExt, AstNodeList, AstSeparatedList,
BatchMutationExt, SyntaxTriviaPiece, TokenText, TriviaPiece,
BatchMutationExt, SyntaxTriviaPiece, TextSize, TokenText, TriviaPiece,
};

use crate::JsRuleAction;
Expand Down Expand Up @@ -63,31 +57,24 @@ impl Rule for OrganizeImports {

fn run(ctx: &RuleContext<Self>) -> Option<Self::State> {
let root = ctx.query();

let mut groups = Vec::new();

let mut first_node = None;
let mut nodes = BTreeMap::new();

for item in root.items() {
let import = match item {
AnyJsModuleItem::JsImport(import) => import,
AnyJsModuleItem::AnyJsStatement(_) | AnyJsModuleItem::JsExport(_) => {
// If we have pending nodes and encounter a non-import node, append the nodes to a new group
if let Some(first_node) = first_node.take() {
groups.push(ImportGroup {
first_node,
nodes: take(&mut nodes),
});
}
continue;
let AnyJsModuleItem::JsImport(import) = item else {
// If we have pending nodes and encounter a non-import node, append the nodes to a new group
if let Some(first_node) = first_node.take() {
groups.push(ImportGroup {
first_node,
nodes: take(&mut nodes),
});
}
continue;
};

let first_token = import.import_token().ok()?;

// If this is not the first import in the group, check for a group break
if has_empty_line(&first_token.leading_trivia()) {
if has_empty_line(&import.import_token().ok()?.leading_trivia()) {
if let Some(first_node) = first_node.take() {
groups.push(ImportGroup {
first_node,
Expand All @@ -96,28 +83,16 @@ impl Rule for OrganizeImports {
}
}

let source = match import.import_clause().ok()? {
AnyJsImportClause::JsImportBareClause(clause) => clause.source().ok()?,
AnyJsImportClause::JsImportDefaultClause(clause) => clause.source().ok()?,
AnyJsImportClause::JsImportNamedClause(clause) => clause.source().ok()?,
AnyJsImportClause::JsImportNamespaceClause(clause) => clause.source().ok()?,
};

// If this is the first import in the group save the leading trivia
// and slot index
if first_node.is_none() {
first_node = Some(import.clone());
}

let key = source.inner_string_text().ok()?;
match nodes.entry(ImportKey(key)) {
Entry::Vacant(entry) => {
entry.insert(vec![ImportNode::from(import)]);
}
Entry::Occupied(mut entry) => {
entry.get_mut().push(ImportNode::from(import));
}
}
nodes
.entry(ImportKey(import.source_text().ok()?))
.or_default()
.push(ImportNode::from(import));
}

// Flush the remaining nodes
Expand Down Expand Up @@ -287,35 +262,16 @@ struct ImportGroup {
impl ImportGroup {
/// Returns true if the nodes in the group are already sorted in the file
fn is_sorted(&self) -> bool {
// The imports are sorted if the start of each node in the `BTreeMap`
// (sorted in natural order) is higher or equal to the previous item in
// The imports are sorted if the text position of each node in the `BTreeMap`
// (sorted in natural order) is higher than the previous item in
// the sequence
let mut iter = self.nodes.values().flat_map(|nodes| nodes.iter());

let Some(import_node) = iter.next() else {
return true;
};

if !import_node.is_sorted() {
return false;
}

let mut last = import_node.node.syntax().text_range().start();
let mut previous = TextSize::from(0);
iter.all(|import_node| {
let start = import_node.node.syntax().text_range().start();
if start < last {
return false;
}

// Only return false if the node has been fully inspected and was
// found to be unsorted, but continue visiting the remaining
// imports if the node was sorted or contained syntax errors
if !import_node.is_sorted() {
return false;
}

last = start;
true
let current = import_node.node.syntax().text_range().end();
let is_sorted = previous < current && import_node.is_sorted();
previous = current;
is_sorted
})
}
}
Expand All @@ -337,11 +293,8 @@ impl From<JsImport> for ImportNode {

let mut separator_count = 0;
let specifiers = import_clause.and_then(|import_clause| {
let import_named_clause = match import_clause {
AnyJsImportClause::JsImportBareClause(_)
| AnyJsImportClause::JsImportDefaultClause(_)
| AnyJsImportClause::JsImportNamespaceClause(_) => return None,
AnyJsImportClause::JsImportNamedClause(import_clause) => import_clause,
let AnyJsImportClause::JsImportNamedClause(import_named_clause) = import_clause else {
return None;
};

let named_import = import_named_clause.named_import().ok()?;
Expand All @@ -354,7 +307,7 @@ impl From<JsImport> for ImportNode {

for element in named_import_specifiers.specifiers().elements() {
let node = element.node.ok()?;
let key = import_specifier_name(&node)?;
let key = node.local_name()?.token_text_trimmed();

let trailing_separator = element.trailing_separator.ok()?;
separator_count += usize::from(trailing_separator.is_some());
Expand All @@ -376,22 +329,12 @@ impl From<JsImport> for ImportNode {
impl ImportNode {
/// Returns `true` if the named import specifiers of this import node are sorted
fn is_sorted(&self) -> bool {
let mut iter = self
.specifiers
.values()
.map(|(node, _)| node.syntax().text_range().start());

let Some(mut last) = iter.next() else {
return true;
};

iter.all(|value| {
if value < last {
return false;
}

last = value;
true
let mut previous = TextSize::from(0);
self.specifiers.values().all(|(node, _)| {
let current = node.syntax().text_range().end();
let is_sorted = previous < current;
previous = current;
is_sorted
})
}

Expand All @@ -400,20 +343,14 @@ impl ImportNode {
let import = self.node.clone().detach();

let import_clause = import.import_clause();
let import_named_clause =
if let Ok(AnyJsImportClause::JsImportNamedClause(node)) = import_clause {
node
} else {
return import;
};
let Ok(AnyJsImportClause::JsImportNamedClause(import_named_clause)) = import_clause else {
return import;
};

let named_import = import_named_clause.named_import();
let old_specifiers =
if let Ok(AnyJsNamedImport::JsNamedImportSpecifiers(node)) = named_import {
node
} else {
return import;
};
let Ok(AnyJsNamedImport::JsNamedImportSpecifiers(old_specifiers)) = named_import else {
return import;
};

let element_count = self.specifiers.len();
let last_element = element_count.saturating_sub(1);
Expand All @@ -428,9 +365,8 @@ impl ImportNode {
let is_last = index == last_element;

let mut node = node.clone().detach();
let prev_token = match node.syntax().last_token() {
Some(last_token) => last_token,
None => return node,
let Some(prev_token) = node.syntax().last_token() else {
return node;
};

if let Some(sep) = sep {
Expand Down Expand Up @@ -546,29 +482,6 @@ impl ImportNode {
}
}

/// Return the name associated with a given named import specifier
///
/// Currently named import specifiers are sorted using their import name,
/// not the local name they get imported as
fn import_specifier_name(import_specifier: &AnyJsNamedImportSpecifier) -> Option<TokenText> {
let token = match import_specifier {
AnyJsNamedImportSpecifier::JsNamedImportSpecifier(import_specifier) => {
import_specifier.name().ok()?.value().ok()?
}
AnyJsNamedImportSpecifier::JsShorthandNamedImportSpecifier(import_specifier) => {
import_specifier
.local_name()
.ok()?
.as_js_identifier_binding()?
.name_token()
.ok()?
}
AnyJsNamedImportSpecifier::JsBogusNamedImportSpecifier(_) => return None,
};

Some(token.token_text_trimmed())
}

/// Return a clone of `prev_token` with a newline trivia piece prepended to its
/// leading trivia if it didn't have one already. This function will try to copy
/// the newline trivia piece from the leading trivia of `newline_source` if its set
Expand Down Expand Up @@ -681,7 +594,6 @@ enum ImportCategory {
/// may (incorrectly) include source imports through custom import mappings
/// as well.
Library,
/// Relative file imports.
Relative,
/// Any unrecognized protocols are grouped here. These may include custom
/// protocols such as supported by bundlers.
Expand Down Expand Up @@ -725,18 +637,22 @@ const NODE_BUILTINS: &[&str] = &[

impl From<&str> for ImportCategory {
fn from(value: &str) -> Self {
if value.starts_with("node:") || NODE_BUILTINS.contains(&value) {
Self::NodeBuiltin
} else if value.starts_with("npm:") {
Self::Npm
} else if value.starts_with("http:") || value.starts_with("https:") {
Self::Url
} else if value.starts_with('.') {
Self::Relative
} else if !value.contains(':') {
Self::Library
} else {
Self::Other
match value.chars().next().unwrap_or_default() {
'.' => Self::Relative,
_ => {
if let Some((protocol, _)) = value.split_once(':') {
match protocol {
"http" | "https" => Self::Url,
"node" => Self::NodeBuiltin,
"npm" => Self::Npm,
_ => Self::Other,
}
} else if NODE_BUILTINS.contains(&value) {
Self::NodeBuiltin
} else {
Self::Library
}
}
}
}
}
Expand Down
7 changes: 3 additions & 4 deletions crates/biome_js_analyze/src/react.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -269,9 +269,8 @@ fn is_react_export(binding: &Binding, lib: ReactLibrary) -> bool {
binding
.syntax()
.ancestors()
.find_map(|ancestor| JsImport::cast_ref(&ancestor))
.and_then(|js_import| js_import.source_is(lib.import_name()).ok())
.unwrap_or(false)
.find_map(|ancestor| JsImport::cast(ancestor)?.source_text().ok())
.is_some_and(|source| source.text() == lib.import_name())
}

fn is_named_react_export(binding: &Binding, lib: ReactLibrary, name: &str) -> Option<bool> {
Expand All @@ -296,5 +295,5 @@ fn is_named_react_export(binding: &Binding, lib: ReactLibrary, name: &str) -> Op
let import_clause = import_specifiers.parent::<JsImportNamedClause>()?;
let import = import_clause.parent::<JsImport>()?;

import.source_is(lib.import_name()).ok()
Some(import.source_text().ok()?.text() == lib.import_name())
}
Loading

0 comments on commit e568e46

Please sign in to comment.