From c93e59336e9c4d5c4a2b06b559d89a59f3ae3bbc Mon Sep 17 00:00:00 2001 From: l3ops Date: Tue, 15 Nov 2022 10:49:04 +0100 Subject: [PATCH 1/2] feat(rome_js_analyze): implement the organizeImports code action --- .../src/assists/correctness.rs | 3 +- .../assists/correctness/organize_imports.rs | 331 ++++++++++++++++++ .../correctness/organizeImports/comments.js | 18 + .../organizeImports/comments.js.snap | 60 ++++ .../correctness/organizeImports/directives.js | 7 + .../organizeImports/directives.js.snap | 32 ++ .../correctness/organizeImports/duplicate.js | 3 + .../organizeImports/duplicate.js.snap | 23 ++ .../correctness/organizeImports/empty-line.js | 5 + .../organizeImports/empty-line.js.snap | 28 ++ .../organizeImports/interpreter.js | 6 + .../organizeImports/interpreter.js.snap | 30 ++ .../organizeImports/non-import-newline.js | 7 + .../non-import-newline.js.snap | 32 ++ .../correctness/organizeImports/non-import.js | 5 + .../organizeImports/non-import.js.snap | 28 ++ .../organizeImports/remaining-content.js | 7 + .../organizeImports/remaining-content.js.snap | 32 ++ .../correctness/organizeImports/sorted.js | 5 + .../organizeImports/sorted.js.snap | 15 + 20 files changed, 676 insertions(+), 1 deletion(-) create mode 100644 crates/rome_js_analyze/src/assists/correctness/organize_imports.rs create mode 100644 crates/rome_js_analyze/tests/specs/correctness/organizeImports/comments.js create mode 100644 crates/rome_js_analyze/tests/specs/correctness/organizeImports/comments.js.snap create mode 100644 crates/rome_js_analyze/tests/specs/correctness/organizeImports/directives.js create mode 100644 crates/rome_js_analyze/tests/specs/correctness/organizeImports/directives.js.snap create mode 100644 crates/rome_js_analyze/tests/specs/correctness/organizeImports/duplicate.js create mode 100644 crates/rome_js_analyze/tests/specs/correctness/organizeImports/duplicate.js.snap create mode 100644 crates/rome_js_analyze/tests/specs/correctness/organizeImports/empty-line.js create mode 100644 crates/rome_js_analyze/tests/specs/correctness/organizeImports/empty-line.js.snap create mode 100644 crates/rome_js_analyze/tests/specs/correctness/organizeImports/interpreter.js create mode 100644 crates/rome_js_analyze/tests/specs/correctness/organizeImports/interpreter.js.snap create mode 100644 crates/rome_js_analyze/tests/specs/correctness/organizeImports/non-import-newline.js create mode 100644 crates/rome_js_analyze/tests/specs/correctness/organizeImports/non-import-newline.js.snap create mode 100644 crates/rome_js_analyze/tests/specs/correctness/organizeImports/non-import.js create mode 100644 crates/rome_js_analyze/tests/specs/correctness/organizeImports/non-import.js.snap create mode 100644 crates/rome_js_analyze/tests/specs/correctness/organizeImports/remaining-content.js create mode 100644 crates/rome_js_analyze/tests/specs/correctness/organizeImports/remaining-content.js.snap create mode 100644 crates/rome_js_analyze/tests/specs/correctness/organizeImports/sorted.js create mode 100644 crates/rome_js_analyze/tests/specs/correctness/organizeImports/sorted.js.snap diff --git a/crates/rome_js_analyze/src/assists/correctness.rs b/crates/rome_js_analyze/src/assists/correctness.rs index 9d369197d73..37dd41f8b65 100644 --- a/crates/rome_js_analyze/src/assists/correctness.rs +++ b/crates/rome_js_analyze/src/assists/correctness.rs @@ -3,4 +3,5 @@ use rome_analyze::declare_group; mod flip_bin_exp; mod inline_variable; -declare_group! { pub (crate) Correctness { name : "correctness" , rules : [self :: flip_bin_exp :: FlipBinExp , self :: inline_variable :: InlineVariable ,] } } +mod organize_imports; +declare_group! { pub (crate) Correctness { name : "correctness" , rules : [self :: flip_bin_exp :: FlipBinExp , self :: inline_variable :: InlineVariable , self :: organize_imports :: OrganizeImports ,] } } diff --git a/crates/rome_js_analyze/src/assists/correctness/organize_imports.rs b/crates/rome_js_analyze/src/assists/correctness/organize_imports.rs new file mode 100644 index 00000000000..4eec40dee45 --- /dev/null +++ b/crates/rome_js_analyze/src/assists/correctness/organize_imports.rs @@ -0,0 +1,331 @@ +use std::{ + cmp::Ordering, + collections::{btree_map::Entry, BTreeMap}, + mem::take, +}; + +use rome_analyze::{ + context::RuleContext, declare_rule, ActionCategory, Ast, Rule, SourceActionKind, +}; +use rome_console::markup; +use rome_diagnostics::Applicability; +use rome_js_factory::make; +use rome_js_syntax::{ + JsAnyImportClause, JsAnyModuleItem, JsImport, JsModule, JsSyntaxToken, TriviaPieceKind, +}; +use rome_rowan::{AstNode, AstNodeExt, AstNodeList, BatchMutationExt, SyntaxTokenText}; + +use crate::JsRuleAction; + +declare_rule! { + /// Provides a whole-source code action to sort the imports in the file + /// + /// ## Examples + /// + /// ```js + /// import React, { + /// FC, + /// useEffect, + /// useRef, + /// ChangeEvent, + /// KeyboardEvent, + /// } from 'react'; + /// import { logger } from '@core/logger'; + /// import { reduce, debounce } from 'lodash'; + /// import { Message } from '../Message'; + /// import { createServer } from '@server/node'; + /// import { Alert } from '@ui/Alert'; + /// import { repeat, filter, add } from '../utils'; + /// import { initializeApp } from '@core/app'; + /// import { Popup } from '@ui/Popup'; + /// import { createConnection } from '@server/database'; + /// ``` + pub(crate) OrganizeImports { + version: "11.0.0", + name: "organizeImports", + recommended: false, + } +} + +impl Rule for OrganizeImports { + type Query = Ast; + type State = ImportGroups; + type Signals = Option; + type Options = (); + + fn run(ctx: &RuleContext) -> Option { + let root = ctx.query(); + + let mut groups = Vec::new(); + + let mut first_slot = 0; + let mut group_leading_newlines = 0; + let mut nodes = BTreeMap::new(); + + for item in root.items() { + let import = match item { + JsAnyModuleItem::JsImport(import) => import, + JsAnyModuleItem::JsAnyStatement(_) | JsAnyModuleItem::JsExport(_) => { + // If we have pending nodes and encounter a non-import node, append the nodes to a new group + if !nodes.is_empty() { + groups.push(ImportGroup { + first_slot, + leading_newlines: group_leading_newlines, + nodes: take(&mut nodes), + }); + } + continue; + } + }; + + let first_token = import.import_token().ok()?; + let leading_newlines = count_leading_newlines(&first_token); + + if !nodes.is_empty() { + // If this is not the first import in the group, check for a group break + if leading_newlines >= 2 { + groups.push(ImportGroup { + first_slot, + leading_newlines: group_leading_newlines, + nodes: take(&mut nodes), + }); + + first_slot = import.syntax().index(); + group_leading_newlines = 2; + } + } else { + // If this is the first import in the group save the number of leading newlines + first_slot = import.syntax().index(); + group_leading_newlines = leading_newlines.min(2); + } + + let source = match import.import_clause().ok()? { + JsAnyImportClause::JsImportBareClause(clause) => clause.source().ok()?, + JsAnyImportClause::JsImportDefaultClause(clause) => clause.source().ok()?, + JsAnyImportClause::JsImportNamedClause(clause) => clause.source().ok()?, + JsAnyImportClause::JsImportNamespaceClause(clause) => clause.source().ok()?, + }; + + let key = source.inner_string_text().ok()?; + match nodes.entry(ImportKey(key)) { + Entry::Vacant(entry) => { + entry.insert(vec![import]); + } + Entry::Occupied(mut entry) => { + entry.get_mut().push(import); + } + } + } + + // Flush the remaining nodes + if !nodes.is_empty() { + groups.push(ImportGroup { + first_slot, + leading_newlines: group_leading_newlines, + nodes, + }); + } + + groups + .iter() + .any(|group| !group.is_sorted()) + .then_some(ImportGroups { groups }) + } + + fn action(ctx: &RuleContext, groups: &Self::State) -> Option { + let mut groups_iter = groups.groups.iter(); + let mut next_group = groups_iter.next().expect("state is empty"); + + let old_list = ctx.query().items(); + let mut new_list = Vec::new(); + + let mut items_iter = old_list.iter(); + let mut iter = (&mut items_iter).enumerate(); + + // Iterate other the nodes of the old list + while let Some((item_slot, item)) = iter.next() { + // If the current position in the old list is lower than the start + // of the new group, append the old node to the new list + if item_slot < next_group.first_slot { + new_list.push(item); + continue; + } + + let nodes_iter = next_group + .nodes + .values() + // TODO: Try to merge nodes from the same source + .flat_map(|nodes| nodes.iter()) + .enumerate(); + + for (node_index, node) in nodes_iter { + // For each node in the group, pop an item from the old list + // iterator (ignoring `item` itself) and discard it + if node_index > 0 { + iter.next() + .unwrap_or_else(|| panic!("mising node {item_slot} {node_index}")); + } + + // Preserve the leading newlines count for the first import in + // the group, and normalize it to 1 for the rest of the nodes + let expected_leading_newlines = if node_index == 0 { + next_group.leading_newlines + } else { + 1 + }; + + let node = normalize_leading_newlines(node, expected_leading_newlines)?; + new_list.push(JsAnyModuleItem::JsImport(node)); + } + + // Load the next group before moving on to the next item in the old + // list, breaking the loop if there a no remaining groups to insert + next_group = match groups_iter.next() { + Some(entry) => entry, + None => break, + }; + } + + // Append all remaining nodes to the new list if the loop performed an + // early exit after reaching the last group + new_list.extend(items_iter); + + let new_list = make::js_module_item_list(new_list); + + let mut mutation = ctx.root().begin(); + mutation.replace_node_discard_trivia(old_list, new_list); + + Some(JsRuleAction { + category: ActionCategory::Source(SourceActionKind::OrganizeImports), + applicability: Applicability::MaybeIncorrect, + message: markup! { "Organize Imports" }.to_owned(), + mutation, + }) + } +} + +#[derive(Debug)] +pub(crate) struct ImportGroups { + /// The list of all the import groups in the file + groups: Vec, +} + +#[derive(Debug)] +struct ImportGroup { + /// The starting index of this group in the module's item list + first_slot: usize, + /// The number of leading newlines the first import in the group should have + leading_newlines: usize, + /// Multimap storing all the imports for each import source in the group, + /// sorted in alphabetical order + nodes: BTreeMap>, +} + +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 alphabetically) is higher or equal to the previous item in + // the sequence + let mut iter = self + .nodes + .values() + .flat_map(|nodes| nodes.iter()) + .map(|node| node.syntax().text_range().start()); + + let mut last = match iter.next() { + Some(last) => last, + None => return true, + }; + + iter.all(|value| { + let result = value >= last; + last = value; + result + }) + } +} + +#[derive(Debug)] +struct ImportKey(SyntaxTokenText); + +impl Ord for ImportKey { + fn cmp(&self, other: &Self) -> Ordering { + // Sort imports alphabetically by defering to the string ordering logic + // of the standard library + (*self.0).cmp(&*other.0) + } +} + +impl PartialOrd for ImportKey { + fn partial_cmp(&self, other: &Self) -> Option { + Some(self.cmp(other)) + } +} + +impl Eq for ImportKey {} + +impl PartialEq for ImportKey { + fn eq(&self, other: &Self) -> bool { + self.0 == other.0 + } +} + +/// Return a copy of `node` with `expected_leading_newlines` newline trivia +/// pieces at the start of its leading trivia +fn normalize_leading_newlines( + node: &JsImport, + expected_leading_newlines: usize, +) -> Option { + let first_token = node.import_token().ok()?; + let actual_leading_newlines = count_leading_newlines(&first_token); + + if actual_leading_newlines != expected_leading_newlines { + let mut first_token = first_token.detach(); + + first_token = if actual_leading_newlines > expected_leading_newlines { + // The node has more leading newlines than necessary: skip the + // excess trivia pieces when reconstructing the leading trivia + let diff = actual_leading_newlines - expected_leading_newlines; + let leading_trivia: Vec<_> = first_token.leading_trivia().pieces().skip(diff).collect(); + + first_token.with_leading_trivia( + leading_trivia + .iter() + .map(|piece| (piece.kind(), piece.text())), + ) + } else { + // The node has less leading newlines than necessary: prepend the + // reconstructed leading trivia with additional newline pieces + let leading_trivia: Vec<_> = first_token.leading_trivia().pieces().collect(); + + let diff = expected_leading_newlines - actual_leading_newlines; + let total_len = leading_trivia.len() + diff; + + first_token.with_leading_trivia((0..total_len).map(|index| { + if let Some(index) = index.checked_sub(diff) { + let piece = &leading_trivia[index]; + (piece.kind(), piece.text()) + } else { + // TODO: Use "\r\n" if the file is using CRLF line terminators + (TriviaPieceKind::Newline, "\n") + } + })) + }; + + Some(node.clone().detach().with_import_token(first_token)) + } else { + Some(node.clone()) + } +} + +/// Return the number of newline trivia pieces contained in the leading trivia +/// of `token` before the first comment or skipped trivia +fn count_leading_newlines(token: &JsSyntaxToken) -> usize { + token + .leading_trivia() + .pieces() + .take_while(|piece| !piece.is_comments() && !piece.is_skipped()) + .filter(|piece| piece.is_newline()) + .count() +} diff --git a/crates/rome_js_analyze/tests/specs/correctness/organizeImports/comments.js b/crates/rome_js_analyze/tests/specs/correctness/organizeImports/comments.js new file mode 100644 index 00000000000..f3b545d1986 --- /dev/null +++ b/crates/rome_js_analyze/tests/specs/correctness/organizeImports/comments.js @@ -0,0 +1,18 @@ +// leading b +import b, { + B, // dangling b +} from 'b'; // trailing b +// leading a +import a, { + A, // dangling a +} from 'a'; // trailing a + +// leading d +import d, { + D, // dangling d +} from 'd'; // trailing d +// leading c +import c, { + C, // dangling c +} from 'c'; // trailing c +// leading eof diff --git a/crates/rome_js_analyze/tests/specs/correctness/organizeImports/comments.js.snap b/crates/rome_js_analyze/tests/specs/correctness/organizeImports/comments.js.snap new file mode 100644 index 00000000000..aecfa378531 --- /dev/null +++ b/crates/rome_js_analyze/tests/specs/correctness/organizeImports/comments.js.snap @@ -0,0 +1,60 @@ +--- +source: crates/rome_js_analyze/tests/spec_tests.rs +expression: comments.js +--- +# Input +```js +// leading b +import b, { + B, // dangling b +} from 'b'; // trailing b +// leading a +import a, { + A, // dangling a +} from 'a'; // trailing a + +// leading d +import d, { + D, // dangling d +} from 'd'; // trailing d +// leading c +import c, { + C, // dangling c +} from 'c'; // trailing c +// leading eof + +``` + +# Actions +```diff +@@ -1,18 +1,18 @@ ++// leading a ++import a, { ++ A, // dangling a ++} from 'a'; // trailing a + // leading b + import b, { + B, // dangling b + } from 'b'; // trailing b +-// leading a +-import a, { +- A, // dangling a +-} from 'a'; // trailing a + ++// leading c ++import c, { ++ C, // dangling c ++} from 'c'; // trailing c + // leading d + import d, { + D, // dangling d + } from 'd'; // trailing d +-// leading c +-import c, { +- C, // dangling c +-} from 'c'; // trailing c + // leading eof + +``` + + diff --git a/crates/rome_js_analyze/tests/specs/correctness/organizeImports/directives.js b/crates/rome_js_analyze/tests/specs/correctness/organizeImports/directives.js new file mode 100644 index 00000000000..9ad762f2cd4 --- /dev/null +++ b/crates/rome_js_analyze/tests/specs/correctness/organizeImports/directives.js @@ -0,0 +1,7 @@ +"use strict"; + +import d from 'd'; +import c from 'c'; + +import b from 'b'; +import a from 'a'; diff --git a/crates/rome_js_analyze/tests/specs/correctness/organizeImports/directives.js.snap b/crates/rome_js_analyze/tests/specs/correctness/organizeImports/directives.js.snap new file mode 100644 index 00000000000..3e25948ec66 --- /dev/null +++ b/crates/rome_js_analyze/tests/specs/correctness/organizeImports/directives.js.snap @@ -0,0 +1,32 @@ +--- +source: crates/rome_js_analyze/tests/spec_tests.rs +expression: directives.js +--- +# Input +```js +"use strict"; + +import d from 'd'; +import c from 'c'; + +import b from 'b'; +import a from 'a'; + +``` + +# Actions +```diff +@@ -1,7 +1,7 @@ + "use strict"; + ++import c from 'c'; + import d from 'd'; +-import c from 'c'; + ++import a from 'a'; + import b from 'b'; +-import a from 'a'; + +``` + + diff --git a/crates/rome_js_analyze/tests/specs/correctness/organizeImports/duplicate.js b/crates/rome_js_analyze/tests/specs/correctness/organizeImports/duplicate.js new file mode 100644 index 00000000000..b17e9c63b96 --- /dev/null +++ b/crates/rome_js_analyze/tests/specs/correctness/organizeImports/duplicate.js @@ -0,0 +1,3 @@ +import b from 'b'; +import a from 'a'; +import { A } from 'a'; diff --git a/crates/rome_js_analyze/tests/specs/correctness/organizeImports/duplicate.js.snap b/crates/rome_js_analyze/tests/specs/correctness/organizeImports/duplicate.js.snap new file mode 100644 index 00000000000..1b336f980d0 --- /dev/null +++ b/crates/rome_js_analyze/tests/specs/correctness/organizeImports/duplicate.js.snap @@ -0,0 +1,23 @@ +--- +source: crates/rome_js_analyze/tests/spec_tests.rs +expression: duplicate.js +--- +# Input +```js +import b from 'b'; +import a from 'a'; +import { A } from 'a'; + +``` + +# Actions +```diff +@@ -1,3 +1,3 @@ +-import b from 'b'; + import a from 'a'; + import { A } from 'a'; ++import b from 'b'; + +``` + + diff --git a/crates/rome_js_analyze/tests/specs/correctness/organizeImports/empty-line.js b/crates/rome_js_analyze/tests/specs/correctness/organizeImports/empty-line.js new file mode 100644 index 00000000000..8828ee713f7 --- /dev/null +++ b/crates/rome_js_analyze/tests/specs/correctness/organizeImports/empty-line.js @@ -0,0 +1,5 @@ +import d from 'd'; +import c from 'c'; + +import b from 'b'; +import a from 'a'; diff --git a/crates/rome_js_analyze/tests/specs/correctness/organizeImports/empty-line.js.snap b/crates/rome_js_analyze/tests/specs/correctness/organizeImports/empty-line.js.snap new file mode 100644 index 00000000000..36b97c05882 --- /dev/null +++ b/crates/rome_js_analyze/tests/specs/correctness/organizeImports/empty-line.js.snap @@ -0,0 +1,28 @@ +--- +source: crates/rome_js_analyze/tests/spec_tests.rs +expression: empty-line.js +--- +# Input +```js +import d from 'd'; +import c from 'c'; + +import b from 'b'; +import a from 'a'; + +``` + +# Actions +```diff +@@ -1,5 +1,5 @@ ++import c from 'c'; + import d from 'd'; +-import c from 'c'; + ++import a from 'a'; + import b from 'b'; +-import a from 'a'; + +``` + + diff --git a/crates/rome_js_analyze/tests/specs/correctness/organizeImports/interpreter.js b/crates/rome_js_analyze/tests/specs/correctness/organizeImports/interpreter.js new file mode 100644 index 00000000000..db2a88cec3f --- /dev/null +++ b/crates/rome_js_analyze/tests/specs/correctness/organizeImports/interpreter.js @@ -0,0 +1,6 @@ +#!/usr/bin/env node +import d from 'd'; +import c from 'c'; + +import b from 'b'; +import a from 'a'; diff --git a/crates/rome_js_analyze/tests/specs/correctness/organizeImports/interpreter.js.snap b/crates/rome_js_analyze/tests/specs/correctness/organizeImports/interpreter.js.snap new file mode 100644 index 00000000000..d419011559d --- /dev/null +++ b/crates/rome_js_analyze/tests/specs/correctness/organizeImports/interpreter.js.snap @@ -0,0 +1,30 @@ +--- +source: crates/rome_js_analyze/tests/spec_tests.rs +expression: interpreter.js +--- +# Input +```js +#!/usr/bin/env node +import d from 'd'; +import c from 'c'; + +import b from 'b'; +import a from 'a'; + +``` + +# Actions +```diff +@@ -1,6 +1,6 @@ + #!/usr/bin/env node ++import c from 'c'; + import d from 'd'; +-import c from 'c'; + ++import a from 'a'; + import b from 'b'; +-import a from 'a'; + +``` + + diff --git a/crates/rome_js_analyze/tests/specs/correctness/organizeImports/non-import-newline.js b/crates/rome_js_analyze/tests/specs/correctness/organizeImports/non-import-newline.js new file mode 100644 index 00000000000..71ee8d885e1 --- /dev/null +++ b/crates/rome_js_analyze/tests/specs/correctness/organizeImports/non-import-newline.js @@ -0,0 +1,7 @@ +import d from 'd'; +import c from 'c'; + +export { d }; + +import b from 'b'; +import a from 'a'; diff --git a/crates/rome_js_analyze/tests/specs/correctness/organizeImports/non-import-newline.js.snap b/crates/rome_js_analyze/tests/specs/correctness/organizeImports/non-import-newline.js.snap new file mode 100644 index 00000000000..84ac3f413db --- /dev/null +++ b/crates/rome_js_analyze/tests/specs/correctness/organizeImports/non-import-newline.js.snap @@ -0,0 +1,32 @@ +--- +source: crates/rome_js_analyze/tests/spec_tests.rs +expression: non-import-newline.js +--- +# Input +```js +import d from 'd'; +import c from 'c'; + +export { d }; + +import b from 'b'; +import a from 'a'; + +``` + +# Actions +```diff +@@ -1,7 +1,7 @@ ++import c from 'c'; + import d from 'd'; +-import c from 'c'; + + export { d }; + ++import a from 'a'; + import b from 'b'; +-import a from 'a'; + +``` + + diff --git a/crates/rome_js_analyze/tests/specs/correctness/organizeImports/non-import.js b/crates/rome_js_analyze/tests/specs/correctness/organizeImports/non-import.js new file mode 100644 index 00000000000..76fec386d41 --- /dev/null +++ b/crates/rome_js_analyze/tests/specs/correctness/organizeImports/non-import.js @@ -0,0 +1,5 @@ +import d from 'd'; +import c from 'c'; +export { d }; +import b from 'b'; +import a from 'a'; diff --git a/crates/rome_js_analyze/tests/specs/correctness/organizeImports/non-import.js.snap b/crates/rome_js_analyze/tests/specs/correctness/organizeImports/non-import.js.snap new file mode 100644 index 00000000000..846a9d51d42 --- /dev/null +++ b/crates/rome_js_analyze/tests/specs/correctness/organizeImports/non-import.js.snap @@ -0,0 +1,28 @@ +--- +source: crates/rome_js_analyze/tests/spec_tests.rs +expression: non-import.js +--- +# Input +```js +import d from 'd'; +import c from 'c'; +export { d }; +import b from 'b'; +import a from 'a'; + +``` + +# Actions +```diff +@@ -1,5 +1,5 @@ ++import c from 'c'; + import d from 'd'; +-import c from 'c'; + export { d }; ++import a from 'a'; + import b from 'b'; +-import a from 'a'; + +``` + + diff --git a/crates/rome_js_analyze/tests/specs/correctness/organizeImports/remaining-content.js b/crates/rome_js_analyze/tests/specs/correctness/organizeImports/remaining-content.js new file mode 100644 index 00000000000..2263e6a081f --- /dev/null +++ b/crates/rome_js_analyze/tests/specs/correctness/organizeImports/remaining-content.js @@ -0,0 +1,7 @@ +import d from 'd'; +import c from 'c'; + +import b from 'b'; +import a from 'a'; + +function test() {} diff --git a/crates/rome_js_analyze/tests/specs/correctness/organizeImports/remaining-content.js.snap b/crates/rome_js_analyze/tests/specs/correctness/organizeImports/remaining-content.js.snap new file mode 100644 index 00000000000..a2bd4653429 --- /dev/null +++ b/crates/rome_js_analyze/tests/specs/correctness/organizeImports/remaining-content.js.snap @@ -0,0 +1,32 @@ +--- +source: crates/rome_js_analyze/tests/spec_tests.rs +expression: remaining-content.js +--- +# Input +```js +import d from 'd'; +import c from 'c'; + +import b from 'b'; +import a from 'a'; + +function test() {} + +``` + +# Actions +```diff +@@ -1,7 +1,7 @@ ++import c from 'c'; + import d from 'd'; +-import c from 'c'; + ++import a from 'a'; + import b from 'b'; +-import a from 'a'; + + function test() {} + +``` + + diff --git a/crates/rome_js_analyze/tests/specs/correctness/organizeImports/sorted.js b/crates/rome_js_analyze/tests/specs/correctness/organizeImports/sorted.js new file mode 100644 index 00000000000..288f412170e --- /dev/null +++ b/crates/rome_js_analyze/tests/specs/correctness/organizeImports/sorted.js @@ -0,0 +1,5 @@ +import c from 'c'; +import d from 'd'; + +import a from 'a'; +import b from 'b'; diff --git a/crates/rome_js_analyze/tests/specs/correctness/organizeImports/sorted.js.snap b/crates/rome_js_analyze/tests/specs/correctness/organizeImports/sorted.js.snap new file mode 100644 index 00000000000..5941f820ded --- /dev/null +++ b/crates/rome_js_analyze/tests/specs/correctness/organizeImports/sorted.js.snap @@ -0,0 +1,15 @@ +--- +source: crates/rome_js_analyze/tests/spec_tests.rs +expression: sorted.js +--- +# Input +```js +import c from 'c'; +import d from 'd'; + +import a from 'a'; +import b from 'b'; + +``` + + From c47907e7e2099b994381be33befd944447123670 Mon Sep 17 00:00:00 2001 From: l3ops Date: Tue, 22 Nov 2022 11:37:59 +0100 Subject: [PATCH 2/2] improve the handling of comments and whitespace --- .../assists/correctness/organize_imports.rs | 219 ++++++++++-------- .../organizeImports/empty-line-whitespace.js | 6 + .../empty-line-whitespace.js.snap | 28 +++ .../organizeImports/group-comment.js | 13 ++ .../organizeImports/group-comment.js.snap | 46 ++++ 5 files changed, 217 insertions(+), 95 deletions(-) create mode 100644 crates/rome_js_analyze/tests/specs/correctness/organizeImports/empty-line-whitespace.js create mode 100644 crates/rome_js_analyze/tests/specs/correctness/organizeImports/empty-line-whitespace.js.snap create mode 100644 crates/rome_js_analyze/tests/specs/correctness/organizeImports/group-comment.js create mode 100644 crates/rome_js_analyze/tests/specs/correctness/organizeImports/group-comment.js.snap diff --git a/crates/rome_js_analyze/src/assists/correctness/organize_imports.rs b/crates/rome_js_analyze/src/assists/correctness/organize_imports.rs index 4eec40dee45..792ef3fb75e 100644 --- a/crates/rome_js_analyze/src/assists/correctness/organize_imports.rs +++ b/crates/rome_js_analyze/src/assists/correctness/organize_imports.rs @@ -10,15 +10,16 @@ use rome_analyze::{ use rome_console::markup; use rome_diagnostics::Applicability; use rome_js_factory::make; -use rome_js_syntax::{ - JsAnyImportClause, JsAnyModuleItem, JsImport, JsModule, JsSyntaxToken, TriviaPieceKind, +use rome_js_syntax::{JsAnyImportClause, JsAnyModuleItem, JsImport, JsLanguage, JsModule}; +use rome_rowan::{ + syntax::SyntaxTrivia, AstNode, AstNodeExt, AstNodeList, BatchMutationExt, SyntaxTokenText, + SyntaxTriviaPiece, }; -use rome_rowan::{AstNode, AstNodeExt, AstNodeList, BatchMutationExt, SyntaxTokenText}; use crate::JsRuleAction; declare_rule! { - /// Provides a whole-source code action to sort the imports in the file + /// Provides a whole-source code action to sort the imports in the file alphabetically /// /// ## Examples /// @@ -58,8 +59,7 @@ impl Rule for OrganizeImports { let mut groups = Vec::new(); - let mut first_slot = 0; - let mut group_leading_newlines = 0; + let mut first_node = None; let mut nodes = BTreeMap::new(); for item in root.items() { @@ -67,10 +67,9 @@ impl Rule for OrganizeImports { JsAnyModuleItem::JsImport(import) => import, JsAnyModuleItem::JsAnyStatement(_) | JsAnyModuleItem::JsExport(_) => { // If we have pending nodes and encounter a non-import node, append the nodes to a new group - if !nodes.is_empty() { + if let Some(first_node) = first_node.take() { groups.push(ImportGroup { - first_slot, - leading_newlines: group_leading_newlines, + first_node, nodes: take(&mut nodes), }); } @@ -79,24 +78,21 @@ impl Rule for OrganizeImports { }; let first_token = import.import_token().ok()?; - let leading_newlines = count_leading_newlines(&first_token); - if !nodes.is_empty() { - // If this is not the first import in the group, check for a group break - if leading_newlines >= 2 { + // If this is not the first import in the group, check for a group break + if has_empty_line(first_token.leading_trivia()) { + if let Some(first_node) = first_node.take() { groups.push(ImportGroup { - first_slot, - leading_newlines: group_leading_newlines, + first_node, nodes: take(&mut nodes), }); - - first_slot = import.syntax().index(); - group_leading_newlines = 2; } - } else { - // If this is the first import in the group save the number of leading newlines - first_slot = import.syntax().index(); - group_leading_newlines = leading_newlines.min(2); + } + + // 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 source = match import.import_clause().ok()? { @@ -118,12 +114,8 @@ impl Rule for OrganizeImports { } // Flush the remaining nodes - if !nodes.is_empty() { - groups.push(ImportGroup { - first_slot, - leading_newlines: group_leading_newlines, - nodes, - }); + if let Some(first_node) = first_node.take() { + groups.push(ImportGroup { first_node, nodes }); } groups @@ -146,11 +138,52 @@ impl Rule for OrganizeImports { while let Some((item_slot, item)) = iter.next() { // If the current position in the old list is lower than the start // of the new group, append the old node to the new list - if item_slot < next_group.first_slot { + if item_slot < next_group.first_node.syntax().index() { new_list.push(item); continue; } + // Extract the leading trivia for the whole group from the leading + // trivia for the import token of the first node in the group. If + // the trivia contains empty lines the leading trivia for the group + // comprise all trivia pieces coming before the empty line that's + // closest to the token. Otherwise the group leading trivia is + // created from all the newline and whitespace pieces on the first + // token before the first comment or skipped piece. + let group_first_token = next_group.first_node.import_token().ok()?; + let group_leading_trivia = group_first_token.leading_trivia(); + + let mut prev_newline = None; + let mut group_leading_trivia: Vec<_> = group_leading_trivia + .pieces() + .enumerate() + .rev() + .find_map(|(index, piece)| { + if piece.is_whitespace() { + return None; + } + + let is_newline = piece.is_newline(); + if let Some(first_newline) = prev_newline.filter(|_| is_newline) { + return Some(first_newline + 1); + } + + prev_newline = is_newline.then_some(index); + None + }) + .map_or_else( + || { + group_leading_trivia + .pieces() + .take_while(is_ascii_whitespace) + .collect() + }, + |length| group_leading_trivia.pieces().take(length).collect(), + ); + + let mut saved_leading_trivia = Vec::new(); + let group_leading_pieces = group_leading_trivia.len(); + let nodes_iter = next_group .nodes .values() @@ -166,15 +199,40 @@ impl Rule for OrganizeImports { .unwrap_or_else(|| panic!("mising node {item_slot} {node_index}")); } - // Preserve the leading newlines count for the first import in - // the group, and normalize it to 1 for the rest of the nodes - let expected_leading_newlines = if node_index == 0 { - next_group.leading_newlines - } else { - 1 - }; + let first_token = node.import_token().ok()?; + let mut node = node.clone().detach(); + + if node_index == 0 && group_first_token != first_token { + // If this node was not previously in the leading position + // but is being moved there, replace its leading whitespace + // with the group's leading trivia + let group_leading_trivia = group_leading_trivia.drain(..); + let mut token_leading_trivia = first_token.leading_trivia().pieces().peekable(); + + // Save off the leading whitespace of the token to be + // reused by the import take the place of this node in the list + while let Some(piece) = token_leading_trivia.next_if(is_ascii_whitespace) { + saved_leading_trivia.push(piece); + } + + node = node.with_import_token(first_token.with_leading_trivia_pieces( + exact_chain(group_leading_trivia, token_leading_trivia), + )); + } else if node_index > 0 && group_first_token == first_token { + // If this node used to be in the leading position but + // got moved, remove the group leading trivia from its + // first token + let saved_leading_trivia = saved_leading_trivia.drain(..); + let token_leading_trivia = first_token + .leading_trivia() + .pieces() + .skip(group_leading_pieces); + + node = node.with_import_token(first_token.with_leading_trivia_pieces( + exact_chain(saved_leading_trivia, token_leading_trivia), + )); + } - let node = normalize_leading_newlines(node, expected_leading_newlines)?; new_list.push(JsAnyModuleItem::JsImport(node)); } @@ -212,10 +270,8 @@ pub(crate) struct ImportGroups { #[derive(Debug)] struct ImportGroup { - /// The starting index of this group in the module's item list - first_slot: usize, - /// The number of leading newlines the first import in the group should have - leading_newlines: usize, + /// The import that was at the start of the group before sorting + first_node: JsImport, /// Multimap storing all the imports for each import source in the group, /// sorted in alphabetical order nodes: BTreeMap>, @@ -271,61 +327,34 @@ impl PartialEq for ImportKey { } } -/// Return a copy of `node` with `expected_leading_newlines` newline trivia -/// pieces at the start of its leading trivia -fn normalize_leading_newlines( - node: &JsImport, - expected_leading_newlines: usize, -) -> Option { - let first_token = node.import_token().ok()?; - let actual_leading_newlines = count_leading_newlines(&first_token); - - if actual_leading_newlines != expected_leading_newlines { - let mut first_token = first_token.detach(); - - first_token = if actual_leading_newlines > expected_leading_newlines { - // The node has more leading newlines than necessary: skip the - // excess trivia pieces when reconstructing the leading trivia - let diff = actual_leading_newlines - expected_leading_newlines; - let leading_trivia: Vec<_> = first_token.leading_trivia().pieces().skip(diff).collect(); - - first_token.with_leading_trivia( - leading_trivia - .iter() - .map(|piece| (piece.kind(), piece.text())), - ) - } else { - // The node has less leading newlines than necessary: prepend the - // reconstructed leading trivia with additional newline pieces - let leading_trivia: Vec<_> = first_token.leading_trivia().pieces().collect(); - - let diff = expected_leading_newlines - actual_leading_newlines; - let total_len = leading_trivia.len() + diff; - - first_token.with_leading_trivia((0..total_len).map(|index| { - if let Some(index) = index.checked_sub(diff) { - let piece = &leading_trivia[index]; - (piece.kind(), piece.text()) - } else { - // TODO: Use "\r\n" if the file is using CRLF line terminators - (TriviaPieceKind::Newline, "\n") - } - })) - }; - - Some(node.clone().detach().with_import_token(first_token)) - } else { - Some(node.clone()) - } +/// Returns true is this trivia piece is "ASCII whitespace" (newline or whitespace) +fn is_ascii_whitespace(piece: &SyntaxTriviaPiece) -> bool { + piece.is_newline() || piece.is_whitespace() } -/// Return the number of newline trivia pieces contained in the leading trivia -/// of `token` before the first comment or skipped trivia -fn count_leading_newlines(token: &JsSyntaxToken) -> usize { - token - .leading_trivia() +/// Returns true if the provided trivia contains an empty line (two consecutive newline pieces, ignoring whitespace) +fn has_empty_line(trivia: SyntaxTrivia) -> bool { + let mut was_newline = false; + trivia .pieces() - .take_while(|piece| !piece.is_comments() && !piece.is_skipped()) - .filter(|piece| piece.is_newline()) - .count() + .filter(|piece| !piece.is_whitespace()) + .any(|piece| { + let prev_newline = was_newline; + was_newline = piece.is_newline(); + prev_newline && was_newline + }) +} + +/// Returns an iterator yielding the full content of `lhs`, then the full +/// content of `rhs`. This is similar to the `.chain()` method on the +/// [Iterator] trait except the returned iterator implements [ExactSizeIterator] +fn exact_chain<'a, T>( + mut lhs: impl Iterator + ExactSizeIterator + 'a, + mut rhs: impl Iterator + ExactSizeIterator + 'a, +) -> impl Iterator + ExactSizeIterator + 'a { + let total_len = lhs.len() + rhs.len(); + (0..total_len).map(move |_| { + // SAFETY: The above range iterator should have the exact length of lhs + rhs + lhs.next().or_else(|| rhs.next()).unwrap() + }) } diff --git a/crates/rome_js_analyze/tests/specs/correctness/organizeImports/empty-line-whitespace.js b/crates/rome_js_analyze/tests/specs/correctness/organizeImports/empty-line-whitespace.js new file mode 100644 index 00000000000..2f2a93f2c07 --- /dev/null +++ b/crates/rome_js_analyze/tests/specs/correctness/organizeImports/empty-line-whitespace.js @@ -0,0 +1,6 @@ +import d from 'd'; +import c from 'c'; + + +import a from 'a'; +import b from 'b'; diff --git a/crates/rome_js_analyze/tests/specs/correctness/organizeImports/empty-line-whitespace.js.snap b/crates/rome_js_analyze/tests/specs/correctness/organizeImports/empty-line-whitespace.js.snap new file mode 100644 index 00000000000..3104305aec9 --- /dev/null +++ b/crates/rome_js_analyze/tests/specs/correctness/organizeImports/empty-line-whitespace.js.snap @@ -0,0 +1,28 @@ +--- +source: crates/rome_js_analyze/tests/spec_tests.rs +expression: empty-line-whitespace.js +--- +# Input +```js +import d from 'd'; +import c from 'c'; + + +import a from 'a'; +import b from 'b'; + +``` + +# Actions +```diff +@@ -1,5 +1,5 @@ ++import c from 'c'; + import d from 'd'; +-import c from 'c'; + + + import a from 'a'; + +``` + + diff --git a/crates/rome_js_analyze/tests/specs/correctness/organizeImports/group-comment.js b/crates/rome_js_analyze/tests/specs/correctness/organizeImports/group-comment.js new file mode 100644 index 00000000000..9c971340607 --- /dev/null +++ b/crates/rome_js_analyze/tests/specs/correctness/organizeImports/group-comment.js @@ -0,0 +1,13 @@ +// group 1 leading + +// leading d +import d from 'd'; +// leading c +import c from 'c'; + +// group 2 leading + +// leading b +import b from 'b'; +// leading a +import a from 'a'; diff --git a/crates/rome_js_analyze/tests/specs/correctness/organizeImports/group-comment.js.snap b/crates/rome_js_analyze/tests/specs/correctness/organizeImports/group-comment.js.snap new file mode 100644 index 00000000000..fe6f2a88ab4 --- /dev/null +++ b/crates/rome_js_analyze/tests/specs/correctness/organizeImports/group-comment.js.snap @@ -0,0 +1,46 @@ +--- +source: crates/rome_js_analyze/tests/spec_tests.rs +expression: group-comment.js +--- +# Input +```js +// group 1 leading + +// leading d +import d from 'd'; +// leading c +import c from 'c'; + +// group 2 leading + +// leading b +import b from 'b'; +// leading a +import a from 'a'; + +``` + +# Actions +```diff +@@ -1,13 +1,13 @@ + // group 1 leading + ++// leading c ++import c from 'c'; + // leading d + import d from 'd'; +-// leading c +-import c from 'c'; + + // group 2 leading + ++// leading a ++import a from 'a'; + // leading b + import b from 'b'; +-// leading a +-import a from 'a'; + +``` + +