From 33b5ce603d902502ee0498f1237b1d3d83bd9a30 Mon Sep 17 00:00:00 2001 From: l3ops Date: Tue, 15 Nov 2022 09:02:42 +0100 Subject: [PATCH] feat(rome_analyze): extend ActionCategory to allow expressing more code action kinds (#3627) * feat(rome_analyze): extent the ActionCategory enum to allow expressing more code action categories * run codegen * add documentation --- crates/rome_analyze/src/categories.rs | 135 ++++++- crates/rome_analyze/src/lib.rs | 4 +- .../src/assists/correctness/flip_bin_exp.rs | 4 +- .../assists/correctness/inline_variable.rs | 4 +- .../nursery/use_camel_case.rs | 2 +- .../style/no_shouty_constants.rs | 2 +- crates/rome_lsp/src/handlers/analysis.rs | 45 ++- crates/rome_lsp/src/utils.rs | 26 +- crates/rome_lsp/tests/server.rs | 332 ++++++++++++++---- .../src/file_handlers/javascript.rs | 4 +- crates/rome_service/src/workspace.rs | 2 + npm/backend-jsonrpc/src/workspace.ts | 30 +- 12 files changed, 479 insertions(+), 111 deletions(-) diff --git a/crates/rome_analyze/src/categories.rs b/crates/rome_analyze/src/categories.rs index c3bd80e97a1..a1f7fc997d7 100644 --- a/crates/rome_analyze/src/categories.rs +++ b/crates/rome_analyze/src/categories.rs @@ -1,3 +1,5 @@ +use std::borrow::Cow; + use bitflags::bitflags; #[derive(Copy, Clone, Debug)] @@ -18,16 +20,145 @@ pub enum RuleCategory { Action, } -#[derive(Copy, Clone, Debug, PartialEq, Eq)] +/// The category of a code action, this type maps directly to the +/// [CodeActionKind] type in the Language Server Protocol specification +/// +/// [CodeActionKind]: https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#codeActionKind +#[derive(Clone, Debug, PartialEq, Eq)] #[cfg_attr( feature = "serde", derive(serde::Serialize, serde::Deserialize, schemars::JsonSchema) )] pub enum ActionCategory { + /// Base kind for quickfix actions: 'quickfix'. + /// /// This action provides a fix to the diagnostic emitted by the same signal QuickFix, + /// Base kind for refactoring actions: 'refactor'. + /// /// This action provides an optional refactor opportunity - Refactor, + Refactor(RefactorKind), + /// Base kind for source actions: `source`. + /// + /// Source code actions apply to the entire file. + Source(SourceActionKind), + /// This action is using a base kind not covered by any of the previous + /// variants + Other(Cow<'static, str>), +} + +impl ActionCategory { + /// Returns true if this category matches the provided filter + /// + /// ## Examples + /// + /// ``` + /// use rome_analyze::{ActionCategory, RefactorKind}; + /// + /// assert!(ActionCategory::QuickFix.matches("quickfix")); + /// assert!(!ActionCategory::QuickFix.matches("refactor")); + /// + /// assert!(ActionCategory::Refactor(RefactorKind::None).matches("refactor")); + /// assert!(!ActionCategory::Refactor(RefactorKind::None).matches("refactor.extract")); + /// + /// assert!(ActionCategory::Refactor(RefactorKind::Extract).matches("refactor")); + /// assert!(ActionCategory::Refactor(RefactorKind::Extract).matches("refactor.extract")); + /// ``` + pub fn matches(&self, filter: &str) -> bool { + self.to_str().starts_with(filter) + } + + /// Returns the representation of this [ActionCategory] as a `CodeActionKind` string + pub fn to_str(&self) -> Cow<'static, str> { + match self { + ActionCategory::QuickFix => Cow::Borrowed("quickfix.rome"), + + ActionCategory::Refactor(RefactorKind::None) => Cow::Borrowed("refactor.rome"), + ActionCategory::Refactor(RefactorKind::Extract) => { + Cow::Borrowed("refactor.extract.rome") + } + ActionCategory::Refactor(RefactorKind::Inline) => Cow::Borrowed("refactor.inline.rome"), + ActionCategory::Refactor(RefactorKind::Rewrite) => { + Cow::Borrowed("refactor.rewrite.rome") + } + ActionCategory::Refactor(RefactorKind::Other(tag)) => { + Cow::Owned(format!("refactor.{tag}.rome")) + } + + ActionCategory::Source(SourceActionKind::None) => Cow::Borrowed("source.rome"), + ActionCategory::Source(SourceActionKind::FixAll) => Cow::Borrowed("source.fixAll.rome"), + ActionCategory::Source(SourceActionKind::OrganizeImports) => { + Cow::Borrowed("source.organizeImports.rome") + } + ActionCategory::Source(SourceActionKind::Other(tag)) => { + Cow::Owned(format!("source.{tag}.rome")) + } + + ActionCategory::Other(tag) => Cow::Owned(format!("{tag}.rome")), + } + } +} + +/// The sub-category of a refactor code action +#[derive(Clone, Debug, PartialEq, Eq)] +#[cfg_attr( + feature = "serde", + derive(serde::Serialize, serde::Deserialize, schemars::JsonSchema) +)] +pub enum RefactorKind { + /// This action describes a refactor with no particular sub-category + None, + /// Base kind for refactoring extraction actions: 'refactor.extract'. + /// + /// Example extract actions: + /// - Extract method + /// - Extract function + /// - Extract variable + /// - Extract interface from class + Extract, + /// Base kind for refactoring inline actions: 'refactor.inline'. + /// + /// Example inline actions: + /// - Inline function + /// - Inline variable + /// - Inline constant + /// - ... + Inline, + /// Base kind for refactoring rewrite actions: 'refactor.rewrite'. + /// + /// Example rewrite actions: + /// - Convert JavaScript function to class + /// - Add or remove parameter + /// - Encapsulate field + /// - Make method static + /// - Move method to base class + /// - ... + Rewrite, + /// This action is using a refactor kind not covered by any of the previous + /// variants + Other(Cow<'static, str>), +} + +/// The sub-category of a source code action +#[derive(Clone, Debug, PartialEq, Eq)] +#[cfg_attr( + feature = "serde", + derive(serde::Serialize, serde::Deserialize, schemars::JsonSchema) +)] +pub enum SourceActionKind { + /// This action describes a source action with no particular sub-category + None, + // Base kind for a 'fix all' source action: `source.fixAll`. + // + // 'Fix all' actions automatically fix errors that have a clear fix that + // do not require user input. They should not suppress errors or perform + // unsafe fixes such as generating new types or classes. + FixAll, + /// Base kind for an organize imports source action: `source.organizeImports`. + OrganizeImports, + /// This action is using a source action kind not covered by any of the + /// previous variants + Other(Cow<'static, str>), } bitflags! { diff --git a/crates/rome_analyze/src/lib.rs b/crates/rome_analyze/src/lib.rs index 65e58864063..981caa3d480 100644 --- a/crates/rome_analyze/src/lib.rs +++ b/crates/rome_analyze/src/lib.rs @@ -21,7 +21,9 @@ mod visitor; // Re-exported for use in the `declare_group` macro pub use rome_diagnostics::v2::category_concat; -pub use crate::categories::{ActionCategory, RuleCategories, RuleCategory}; +pub use crate::categories::{ + ActionCategory, RefactorKind, RuleCategories, RuleCategory, SourceActionKind, +}; pub use crate::matcher::{InspectMatcher, MatchQueryParams, QueryMatcher, RuleKey, SignalEntry}; pub use crate::options::{AnalyzerConfiguration, AnalyzerOptions, AnalyzerRules}; pub use crate::query::{Ast, QueryKey, QueryMatch, Queryable}; diff --git a/crates/rome_js_analyze/src/assists/correctness/flip_bin_exp.rs b/crates/rome_js_analyze/src/assists/correctness/flip_bin_exp.rs index f359c848c62..e0bf2fe772b 100644 --- a/crates/rome_js_analyze/src/assists/correctness/flip_bin_exp.rs +++ b/crates/rome_js_analyze/src/assists/correctness/flip_bin_exp.rs @@ -1,4 +1,4 @@ -use rome_analyze::{context::RuleContext, declare_rule, ActionCategory, Ast, Rule}; +use rome_analyze::{context::RuleContext, declare_rule, ActionCategory, Ast, RefactorKind, Rule}; use rome_console::markup; use rome_diagnostics::Applicability; use rome_js_factory::make; @@ -63,7 +63,7 @@ impl Rule for FlipBinExp { mutation.replace_node(prev_right, new_right); Some(JsRuleAction { - category: ActionCategory::Refactor, + category: ActionCategory::Refactor(RefactorKind::None), applicability: Applicability::Always, message: markup! { "Flip Binary Expression" }.to_owned(), mutation, diff --git a/crates/rome_js_analyze/src/assists/correctness/inline_variable.rs b/crates/rome_js_analyze/src/assists/correctness/inline_variable.rs index f8a761c412a..c7767af6a31 100644 --- a/crates/rome_js_analyze/src/assists/correctness/inline_variable.rs +++ b/crates/rome_js_analyze/src/assists/correctness/inline_variable.rs @@ -1,4 +1,4 @@ -use rome_analyze::{context::RuleContext, declare_rule, ActionCategory, Rule}; +use rome_analyze::{context::RuleContext, declare_rule, ActionCategory, RefactorKind, Rule}; use rome_console::markup; use rome_diagnostics::Applicability; use rome_js_semantic::{AllReferencesExtensions, Reference}; @@ -94,7 +94,7 @@ impl Rule for InlineVariable { } Some(JsRuleAction { - category: ActionCategory::Refactor, + category: ActionCategory::Refactor(RefactorKind::Inline), applicability: Applicability::Always, message: markup! { "Inline variable" }.to_owned(), mutation, diff --git a/crates/rome_js_analyze/src/semantic_analyzers/nursery/use_camel_case.rs b/crates/rome_js_analyze/src/semantic_analyzers/nursery/use_camel_case.rs index 5a50d2805ac..66b48da8176 100644 --- a/crates/rome_js_analyze/src/semantic_analyzers/nursery/use_camel_case.rs +++ b/crates/rome_js_analyze/src/semantic_analyzers/nursery/use_camel_case.rs @@ -197,7 +197,7 @@ impl Rule for UseCamelCase { batch.rename_node_declaration_with_retry(model, binding.clone(), candidates); if renamed { Some(JsRuleAction { - category: ActionCategory::Refactor, + category: ActionCategory::QuickFix, applicability: Applicability::Always, message: markup! { "Rename this symbol to camel case" }.to_owned(), mutation: batch, diff --git a/crates/rome_js_analyze/src/semantic_analyzers/style/no_shouty_constants.rs b/crates/rome_js_analyze/src/semantic_analyzers/style/no_shouty_constants.rs index 43442f28074..80284c8cdcb 100644 --- a/crates/rome_js_analyze/src/semantic_analyzers/style/no_shouty_constants.rs +++ b/crates/rome_js_analyze/src/semantic_analyzers/style/no_shouty_constants.rs @@ -157,7 +157,7 @@ impl Rule for NoShoutyConstants { } Some(JsRuleAction { - category: ActionCategory::Refactor, + category: ActionCategory::QuickFix, applicability: Applicability::MaybeIncorrect, message: markup! { "Use the constant value directly" }.to_owned(), mutation: batch, diff --git a/crates/rome_lsp/src/handlers/analysis.rs b/crates/rome_lsp/src/handlers/analysis.rs index 22ebcaee64a..0c8ea56b764 100644 --- a/crates/rome_lsp/src/handlers/analysis.rs +++ b/crates/rome_lsp/src/handlers/analysis.rs @@ -1,7 +1,8 @@ +use std::borrow::Cow; use std::collections::HashMap; use anyhow::{Context, Result}; -use rome_analyze::ActionCategory; +use rome_analyze::{ActionCategory, SourceActionKind}; use rome_fs::RomePath; use rome_service::workspace::{ FeatureName, FixFileMode, FixFileParams, PullActionsParams, SupportsFeatureParams, @@ -15,7 +16,14 @@ use crate::line_index::LineIndex; use crate::session::Session; use crate::utils; -const FIX_ALL: CodeActionKind = CodeActionKind::new("source.fixAll"); +const FIX_ALL_CATEGORY: ActionCategory = ActionCategory::Source(SourceActionKind::FixAll); + +fn fix_all_kind() -> CodeActionKind { + match FIX_ALL_CATEGORY.to_str() { + Cow::Borrowed(kind) => CodeActionKind::from(kind), + Cow::Owned(kind) => CodeActionKind::from(kind), + } +} /// Queries the [`AnalysisServer`] for code actions of the file matching [FileId] /// @@ -39,17 +47,14 @@ pub(crate) fn code_actions( let mut has_fix_all = false; let mut filters = Vec::new(); - if let Some(filter) = params.context.only { + if let Some(filter) = ¶ms.context.only { for kind in filter { - if kind == CodeActionKind::QUICKFIX { - filters.push(ActionCategory::QuickFix); - } else if kind == CodeActionKind::REFACTOR { - filters.push(ActionCategory::Refactor); - } else if kind == FIX_ALL { + let kind = kind.as_str(); + if FIX_ALL_CATEGORY.matches(kind) { has_fix_all = true; - } else { - tracing::warn!("unknown code action kind {kind:?} requested"); } + + filters.push(kind); } } @@ -86,7 +91,8 @@ pub(crate) fn code_actions( .filter_map(|action| { // Remove actions that do not match the categories requested by the // language client - if !filters.is_empty() && !filters.contains(&action.category) { + let matches_filters = filters.iter().any(|filter| action.category.matches(filter)); + if !filters.is_empty() && !matches_filters { return None; } @@ -103,7 +109,7 @@ pub(crate) fn code_actions( if has_fixes { actions.retain(|action| { if let CodeActionOrCommand::CodeAction(action) = action { - action.kind == Some(FIX_ALL) || action.diagnostics.is_some() + action.kind.as_ref() == Some(&fix_all_kind()) || action.diagnostics.is_some() } else { true } @@ -134,13 +140,18 @@ fn fix_all( let diagnostics = diagnostics .iter() .filter_map(|d| { - let code = d.code.as_ref().and_then(|code| match code { - lsp::NumberOrString::String(code) => Some(code.as_str()), - lsp::NumberOrString::Number(_) => None, - })?; + let code = d.code.as_ref()?; + let code = match code { + lsp::NumberOrString::String(code) => code.as_str(), + lsp::NumberOrString::Number(_) => return None, + }; + + let code = code.strip_prefix("lint/")?; let diag_range = utils::text_range(line_index, d.range).ok()?; let has_matching_rule = fixed.actions.iter().any(|action| { + let Some(code) = code.strip_prefix(action.group_name.as_ref()) else { return false }; + let Some(code) = code.strip_prefix('/') else { return false }; code == action.rule_name && action.range.intersect(diag_range).is_some() }); @@ -175,7 +186,7 @@ fn fix_all( Ok(Some(CodeActionOrCommand::CodeAction(lsp::CodeAction { title: String::from("Fix all auto-fixable issues"), - kind: Some(FIX_ALL), + kind: Some(fix_all_kind()), diagnostics: Some(diagnostics), edit: Some(edit), command: None, diff --git a/crates/rome_lsp/src/utils.rs b/crates/rome_lsp/src/utils.rs index c1768a369da..72e2b632bb1 100644 --- a/crates/rome_lsp/src/utils.rs +++ b/crates/rome_lsp/src/utils.rs @@ -116,10 +116,15 @@ pub(crate) fn code_fix_to_lsp( diagnostics .iter() .filter_map(|d| { - let code = d.code.as_ref().and_then(|code| match code { - lsp::NumberOrString::String(code) => Some(code.as_str()), - lsp::NumberOrString::Number(_) => None, - })?; + let code = d.code.as_ref()?; + let code = match code { + lsp::NumberOrString::String(code) => code.as_str(), + lsp::NumberOrString::Number(_) => return None, + }; + + let code = code.strip_prefix("lint/")?; + let code = code.strip_prefix(action.group_name.as_ref())?; + let code = code.strip_prefix('/')?; if code == action.rule_name { Some(d.clone()) @@ -132,10 +137,13 @@ pub(crate) fn code_fix_to_lsp( Vec::new() }; - let kind = match action.category { - ActionCategory::QuickFix => Some(lsp::CodeActionKind::QUICKFIX), - ActionCategory::Refactor => Some(lsp::CodeActionKind::REFACTOR), - }; + let kind = action.category.to_str(); + let mut kind = kind.into_owned(); + + kind.push('.'); + kind.push_str(action.group_name.as_ref()); + kind.push('.'); + kind.push_str(action.rule_name.as_ref()); let suggestion = action.suggestion; @@ -152,7 +160,7 @@ pub(crate) fn code_fix_to_lsp( lsp::CodeAction { title: print_markup(&suggestion.msg), - kind, + kind: Some(lsp::CodeActionKind::from(kind)), diagnostics: if !diagnostics.is_empty() { Some(diagnostics) } else { diff --git a/crates/rome_lsp/tests/server.rs b/crates/rome_lsp/tests/server.rs index b40e8137dc7..7ffd30576e1 100644 --- a/crates/rome_lsp/tests/server.rs +++ b/crates/rome_lsp/tests/server.rs @@ -27,15 +27,8 @@ use tower::timeout::Timeout; use tower::{Service, ServiceExt}; use tower_lsp::jsonrpc; use tower_lsp::jsonrpc::Response; +use tower_lsp::lsp_types as lsp; use tower_lsp::lsp_types::ClientCapabilities; -use tower_lsp::lsp_types::CodeActionContext; -use tower_lsp::lsp_types::CodeActionKind; -use tower_lsp::lsp_types::CodeActionOrCommand; -use tower_lsp::lsp_types::CodeActionParams; -use tower_lsp::lsp_types::CodeActionResponse; -use tower_lsp::lsp_types::Diagnostic; -use tower_lsp::lsp_types::DiagnosticRelatedInformation; -use tower_lsp::lsp_types::DiagnosticSeverity; use tower_lsp::lsp_types::DidChangeTextDocumentParams; use tower_lsp::lsp_types::DidCloseTextDocumentParams; use tower_lsp::lsp_types::DidOpenTextDocumentParams; @@ -43,9 +36,6 @@ use tower_lsp::lsp_types::DocumentFormattingParams; use tower_lsp::lsp_types::FormattingOptions; use tower_lsp::lsp_types::InitializeResult; use tower_lsp::lsp_types::InitializedParams; -use tower_lsp::lsp_types::Location; -use tower_lsp::lsp_types::NumberOrString; -use tower_lsp::lsp_types::PartialResultParams; use tower_lsp::lsp_types::Position; use tower_lsp::lsp_types::PublishDiagnosticsParams; use tower_lsp::lsp_types::Range; @@ -546,44 +536,44 @@ async fn pull_diagnostics() -> Result<()> { PublishDiagnosticsParams { uri: Url::parse("test://workspace/document.js")?, version: Some(0), - diagnostics: vec![Diagnostic { - range: Range { - start: Position { + diagnostics: vec![lsp::Diagnostic { + range: lsp::Range { + start: lsp::Position { line: 0, - character: 5 + character: 5, }, - end: Position { + end: lsp::Position { line: 0, - character: 7 - } + character: 7, + }, }, - severity: Some(DiagnosticSeverity::ERROR), - code: Some(NumberOrString::String(String::from( - "lint/correctness/noDoubleEquals" + severity: Some(lsp::DiagnosticSeverity::ERROR), + code: Some(lsp::NumberOrString::String(String::from( + "lint/correctness/noDoubleEquals", ))), code_description: None, source: Some(String::from("rome")), message: String::from( - "Use === instead of ==.\n== is only allowed when comparing against `null`" + "Use === instead of ==.\n== is only allowed when comparing against `null`", ), - related_information: Some(vec![DiagnosticRelatedInformation { - location: Location { - uri: Url::parse("test://workspace/document.js")?, - range: Range { - start: Position { + related_information: Some(vec![lsp::DiagnosticRelatedInformation { + location: lsp::Location { + uri: lsp::Url::parse("test://workspace/document.js")?, + range: lsp::Range { + start: lsp::Position { line: 0, - character: 5 + character: 5, }, - end: Position { + end: lsp::Position { line: 0, - character: 7 - } - } + character: 7, + }, + }, }, - message: String::new() + message: String::new(), }]), tags: None, - data: None + data: None, }], } )) @@ -597,6 +587,28 @@ async fn pull_diagnostics() -> Result<()> { Ok(()) } +fn fixable_diagnostic(line: u32) -> Result { + Ok(lsp::Diagnostic { + range: lsp::Range { + start: lsp::Position { line, character: 3 }, + end: lsp::Position { + line, + character: 11, + }, + }, + severity: Some(lsp::DiagnosticSeverity::ERROR), + code: Some(lsp::NumberOrString::String(String::from( + "lint/correctness/noCompareNegZero", + ))), + code_description: None, + source: Some(String::from("rome")), + message: String::from("Do not use the === operator to compare against -0."), + related_information: None, + tags: None, + data: None, + }) +} + #[tokio::test] async fn pull_quick_fixes() -> Result<()> { let factory = ServerFactory::default(); @@ -610,34 +622,34 @@ async fn pull_quick_fixes() -> Result<()> { server.initialize().await?; server.initialized().await?; - server.open_document("if(a == b) {}").await?; + server.open_document("if(a === -0) {}").await?; - let res: CodeActionResponse = server + let res: lsp::CodeActionResponse = server .request( "textDocument/codeAction", "pull_code_actions", - CodeActionParams { - text_document: TextDocumentIdentifier { - uri: Url::parse("test://workspace/document.js")?, + lsp::CodeActionParams { + text_document: lsp::TextDocumentIdentifier { + uri: lsp::Url::parse("test://workspace/document.js")?, }, - range: Range { - start: Position { + range: lsp::Range { + start: lsp::Position { line: 0, character: 6, }, - end: Position { + end: lsp::Position { line: 0, character: 6, }, }, - context: CodeActionContext { - diagnostics: vec![], - only: Some(vec![CodeActionKind::QUICKFIX]), + context: lsp::CodeActionContext { + diagnostics: vec![fixable_diagnostic(0)?], + only: Some(vec![lsp::CodeActionKind::QUICKFIX]), }, - work_done_progress_params: WorkDoneProgressParams { + work_done_progress_params: lsp::WorkDoneProgressParams { work_done_token: None, }, - partial_result_params: PartialResultParams { + partial_result_params: lsp::PartialResultParams { partial_result_token: None, }, }, @@ -645,15 +657,42 @@ async fn pull_quick_fixes() -> Result<()> { .await? .context("codeAction returned None")?; - let code_actions: Vec<_> = res - .iter() - .map(|action| match action { - CodeActionOrCommand::Command(_) => panic!("unexpected command"), - CodeActionOrCommand::CodeAction(action) => &action.title, - }) - .collect(); + let mut changes = HashMap::default(); + changes.insert( + lsp::Url::parse("test://workspace/document.js")?, + vec![lsp::TextEdit { + range: lsp::Range { + start: lsp::Position { + line: 0, + character: 9, + }, + end: lsp::Position { + line: 0, + character: 10, + }, + }, + new_text: String::new(), + }], + ); - assert_eq!(code_actions.as_slice(), &["Use ==="]); + let expected_code_action = lsp::CodeActionOrCommand::CodeAction(lsp::CodeAction { + title: String::from("Replace -0 with 0"), + kind: Some(lsp::CodeActionKind::new( + "quickfix.rome.correctness.noCompareNegZero", + )), + diagnostics: Some(vec![fixable_diagnostic(0)?]), + edit: Some(lsp::WorkspaceEdit { + changes: Some(changes), + document_changes: None, + change_annotations: None, + }), + command: None, + is_preferred: Some(true), + disabled: None, + data: None, + }); + + assert_eq!(res, vec![expected_code_action]); server.close_document().await?; @@ -680,32 +719,32 @@ async fn pull_refactors() -> Result<()> { .open_document("let variable = \"value\"; func(variable);") .await?; - let res: CodeActionResponse = server + let res: lsp::CodeActionResponse = server .request( "textDocument/codeAction", "pull_code_actions", - CodeActionParams { - text_document: TextDocumentIdentifier { - uri: Url::parse("test://workspace/document.js")?, + lsp::CodeActionParams { + text_document: lsp::TextDocumentIdentifier { + uri: lsp::Url::parse("test://workspace/document.js")?, }, - range: Range { - start: Position { + range: lsp::Range { + start: lsp::Position { line: 0, character: 7, }, - end: Position { + end: lsp::Position { line: 0, character: 7, }, }, - context: CodeActionContext { + context: lsp::CodeActionContext { diagnostics: vec![], - only: Some(vec![CodeActionKind::REFACTOR]), + only: Some(vec![lsp::CodeActionKind::REFACTOR]), }, - work_done_progress_params: WorkDoneProgressParams { + work_done_progress_params: lsp::WorkDoneProgressParams { work_done_token: None, }, - partial_result_params: PartialResultParams { + partial_result_params: lsp::PartialResultParams { partial_result_token: None, }, }, @@ -713,15 +752,160 @@ async fn pull_refactors() -> Result<()> { .await? .context("codeAction returned None")?; - let code_actions: Vec<_> = res - .iter() - .map(|action| match action { - CodeActionOrCommand::Command(_) => panic!("unexpected command"), - CodeActionOrCommand::CodeAction(action) => &action.title, - }) - .collect(); + let mut changes = HashMap::default(); + + changes.insert( + lsp::Url::parse("test://workspace/document.js")?, + vec![ + lsp::TextEdit { + range: lsp::Range { + start: lsp::Position { + line: 0, + character: 0, + }, + end: lsp::Position { + line: 0, + character: 15, + }, + }, + new_text: String::from("func("), + }, + lsp::TextEdit { + range: lsp::Range { + start: lsp::Position { + line: 0, + character: 22, + }, + end: lsp::Position { + line: 0, + character: 37, + }, + }, + new_text: String::new(), + }, + ], + ); + + let expected_action = lsp::CodeActionOrCommand::CodeAction(lsp::CodeAction { + title: String::from("Inline variable"), + kind: Some(lsp::CodeActionKind::new( + "refactor.inline.rome.correctness.inlineVariable", + )), + diagnostics: None, + edit: Some(lsp::WorkspaceEdit { + changes: Some(changes), + document_changes: None, + change_annotations: None, + }), + command: None, + is_preferred: Some(true), + disabled: None, + data: None, + }); + + assert_eq!(res, vec![expected_action]); + + server.close_document().await?; + + server.shutdown().await?; + reader.abort(); + + Ok(()) +} + +#[tokio::test] +async fn pull_fix_all() -> Result<()> { + let factory = ServerFactory::default(); + let (service, client) = factory.create().into_inner(); + let (stream, sink) = client.split(); + let mut server = Server::new(service); + + let (sender, _) = channel(CHANNEL_BUFFER_SIZE); + let reader = tokio::spawn(client_handler(stream, sink, sender)); + + server.initialize().await?; + server.initialized().await?; + + server + .open_document("if(a === -0) {}\nif(a === -0) {}\nif(a === -0) {}") + .await?; + + let res: lsp::CodeActionResponse = server + .request( + "textDocument/codeAction", + "pull_code_actions", + lsp::CodeActionParams { + text_document: lsp::TextDocumentIdentifier { + uri: lsp::Url::parse("test://workspace/document.js")?, + }, + range: lsp::Range { + start: lsp::Position { + line: 0, + character: 7, + }, + end: lsp::Position { + line: 0, + character: 7, + }, + }, + context: lsp::CodeActionContext { + diagnostics: vec![ + fixable_diagnostic(0)?, + fixable_diagnostic(1)?, + fixable_diagnostic(2)?, + ], + only: Some(vec![lsp::CodeActionKind::new("source.fixAll")]), + }, + work_done_progress_params: lsp::WorkDoneProgressParams { + work_done_token: None, + }, + partial_result_params: lsp::PartialResultParams { + partial_result_token: None, + }, + }, + ) + .await? + .context("codeAction returned None")?; + + let mut changes = HashMap::default(); + + changes.insert( + lsp::Url::parse("test://workspace/document.js")?, + vec![lsp::TextEdit { + range: lsp::Range { + start: lsp::Position { + line: 0, + character: 0, + }, + end: lsp::Position { + line: 3, + character: 0, + }, + }, + new_text: String::from("if(a === 0) {}\nif(a === 0) {}\nif(a === 0) {}"), + }], + ); - assert_eq!(code_actions.as_slice(), &["Inline variable"]); + let expected_action = lsp::CodeActionOrCommand::CodeAction(lsp::CodeAction { + title: String::from("Fix all auto-fixable issues"), + kind: Some(lsp::CodeActionKind::new("source.fixAll.rome")), + diagnostics: Some(vec![ + fixable_diagnostic(0)?, + fixable_diagnostic(1)?, + fixable_diagnostic(2)?, + ]), + edit: Some(lsp::WorkspaceEdit { + changes: Some(changes), + document_changes: None, + change_annotations: None, + }), + command: None, + is_preferred: Some(true), + disabled: None, + data: None, + }); + + assert_eq!(res, vec![expected_action]); server.close_document().await?; diff --git a/crates/rome_service/src/file_handlers/javascript.rs b/crates/rome_service/src/file_handlers/javascript.rs index b0876eeea95..06781aac3f4 100644 --- a/crates/rome_service/src/file_handlers/javascript.rs +++ b/crates/rome_service/src/file_handlers/javascript.rs @@ -335,7 +335,8 @@ fn code_actions( analyze(file_id, &tree, filter, &analyzer_options, |signal| { if let Some(action) = signal.action() { actions.push(CodeAction { - category: action.category, + category: action.category.clone(), + group_name: Cow::Borrowed(action.group_name), rule_name: Cow::Borrowed(action.rule_name), suggestion: CodeSuggestion::from(action), }); @@ -418,6 +419,7 @@ fn fix_all(params: FixAllParams) -> Result { } }; actions.push(FixAction { + group_name: Cow::Borrowed(action.group_name), rule_name: Cow::Borrowed(action.rule_name), range, }); diff --git a/crates/rome_service/src/workspace.rs b/crates/rome_service/src/workspace.rs index 7634f6feacc..20fe1aa6910 100644 --- a/crates/rome_service/src/workspace.rs +++ b/crates/rome_service/src/workspace.rs @@ -205,6 +205,7 @@ pub struct PullActionsResult { #[cfg_attr(feature = "schemars", derive(schemars::JsonSchema))] pub struct CodeAction { pub category: ActionCategory, + pub group_name: Cow<'static, str>, pub rule_name: Cow<'static, str>, pub suggestion: CodeSuggestion, } @@ -261,6 +262,7 @@ pub struct FixFileResult { #[derive(Debug, serde::Serialize, serde::Deserialize)] #[cfg_attr(feature = "schemars", derive(schemars::JsonSchema))] pub struct FixAction { + pub group_name: Cow<'static, str>, /// Name of the rule that emitted this code action pub rule_name: Cow<'static, str>, /// Source range at which this action was applied diff --git a/npm/backend-jsonrpc/src/workspace.ts b/npm/backend-jsonrpc/src/workspace.ts index 4cb9437bf0f..375c9ebf3a1 100644 --- a/npm/backend-jsonrpc/src/workspace.ts +++ b/npm/backend-jsonrpc/src/workspace.ts @@ -517,10 +517,20 @@ export interface PullActionsResult { } export interface CodeAction { category: ActionCategory; + group_name: string; rule_name: string; suggestion: CodeSuggestion; } -export type ActionCategory = "QuickFix" | "Refactor"; +/** + * The category of a code action, this type maps directly to the [CodeActionKind] type in the Language Server Protocol specification + +[CodeActionKind]: https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#codeActionKind + */ +export type ActionCategory = + | "QuickFix" + | { Refactor: RefactorKind } + | { Source: SourceActionKind } + | { Other: string }; /** * A Suggestion that is provided by rslint, and can be reported to the user, and can be automatically applied if it has the right [`Applicability`]. */ @@ -534,6 +544,23 @@ export interface CodeSuggestion { span: FileSpan; suggestion: TextEdit; } +/** + * The sub-category of a refactor code action + */ +export type RefactorKind = + | "None" + | "Extract" + | "Inline" + | "Rewrite" + | { Other: string }; +/** + * The sub-category of a source code action + */ +export type SourceActionKind = + | "None" + | "FixAll" + | "OrganizeImports" + | { Other: string }; /** * Indicates how a tool should manage this suggestion. */ @@ -598,6 +625,7 @@ export interface FixFileResult { skipped_suggested_fixes: number; } export interface FixAction { + group_name: string; /** * Source range at which this action was applied */