From 05c11dd928884df332b315167e4b9c562e4f12e5 Mon Sep 17 00:00:00 2001 From: Emanuele Stoppa Date: Tue, 21 May 2024 11:34:09 +0100 Subject: [PATCH 1/2] feat(lsp): pull diagnostics for CSS files --- crates/biome_lsp/src/handlers/analysis.rs | 14 +- crates/biome_lsp/tests/server.rs | 132 ++++++++++-- crates/biome_service/src/file_handlers/css.rs | 199 +++++++++++++++++- .../src/file_handlers/javascript.rs | 2 - 4 files changed, 309 insertions(+), 38 deletions(-) diff --git a/crates/biome_lsp/src/handlers/analysis.rs b/crates/biome_lsp/src/handlers/analysis.rs index 5fd96da0ed86..78e311e4d60d 100644 --- a/crates/biome_lsp/src/handlers/analysis.rs +++ b/crates/biome_lsp/src/handlers/analysis.rs @@ -33,7 +33,16 @@ fn fix_all_kind() -> CodeActionKind { /// Queries the [`AnalysisServer`] for code actions of the file matching its path /// /// If the AnalysisServer has no matching file, results in error. -#[tracing::instrument(level = "debug", skip_all, fields(uri = display(& params.text_document.uri), range = debug(params.range), only = debug(& params.context.only), diagnostics = debug(& params.context.diagnostics)), err)] +#[tracing::instrument(level = "debug", + skip_all, + fields( + uri = display(& params.text_document.uri), + range = debug(params.range), + only = debug(& params.context.only), + diagnostics = debug(& params.context.diagnostics) + ), + err +)] pub(crate) fn code_actions( session: &Session, params: CodeActionParams, @@ -48,7 +57,7 @@ pub(crate) fn code_actions( .with_organize_imports() .build(), })?; - + if !file_features.supports_lint() && !file_features.supports_organize_imports() { debug!("Linter and organize imports are both disabled"); return Ok(Some(Vec::new())); @@ -106,7 +115,6 @@ pub(crate) fn code_actions( }; debug!("Cursor range {:?}", &cursor_range); - let result = match session.workspace.pull_actions(PullActionsParams { path: biome_path.clone(), range: cursor_range, diff --git a/crates/biome_lsp/tests/server.rs b/crates/biome_lsp/tests/server.rs index d06768ccc291..02b2b5dbe31a 100644 --- a/crates/biome_lsp/tests/server.rs +++ b/crates/biome_lsp/tests/server.rs @@ -62,6 +62,28 @@ macro_rules! url { }; } +fn fixable_diagnostic(line: u32) -> Result { + Ok(lsp::Diagnostic { + range: Range { + start: Position { line, character: 3 }, + end: Position { + line, + character: 11, + }, + }, + severity: Some(lsp::DiagnosticSeverity::ERROR), + code: Some(lsp::NumberOrString::String(String::from( + "lint/suspicious/noCompareNegZero", + ))), + code_description: None, + source: Some(String::from("biome")), + message: String::from("Do not use the === operator to compare against -0."), + related_information: None, + tags: None, + data: None, + }) +} + struct Server { service: Timeout>, } @@ -150,7 +172,7 @@ impl Server { InitializeParams { process_id: None, root_path: None, - root_uri: Some(url!("/")), + root_uri: Some(url!("")), initialization_options: None, capabilities: ClientCapabilities::default(), trace: None, @@ -277,6 +299,8 @@ impl Server { ) .await } + + /// When calling this function, remember to insert the file inside the memory file system async fn load_configuration(&mut self) -> Result<()> { self.notify( "workspace/didChangeConfiguration", @@ -829,28 +853,6 @@ async fn pull_diagnostics_from_new_file() -> Result<()> { Ok(()) } -fn fixable_diagnostic(line: u32) -> Result { - Ok(lsp::Diagnostic { - range: Range { - start: Position { line, character: 3 }, - end: Position { - line, - character: 11, - }, - }, - severity: Some(lsp::DiagnosticSeverity::ERROR), - code: Some(lsp::NumberOrString::String(String::from( - "lint/suspicious/noCompareNegZero", - ))), - code_description: None, - source: Some(String::from("biome")), - 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(); @@ -1382,6 +1384,90 @@ async fn pull_diagnostics_for_rome_json() -> Result<()> { Ok(()) } +#[tokio::test] +async fn pull_diagnostics_for_css_files() -> Result<()> { + let factory = ServerFactory::default(); + let mut fs = MemoryFileSystem::default(); + let config = r#"{ + "css": { + "linter": { "enabled": true } + }, + "linter": { + "rules": { "nursery": { "noUnknownProperty": "error" } } + } + }"#; + + fs.insert(url!("biome.json").to_file_path().unwrap(), config); + let (service, client) = factory + .create_with_fs(None, DynRef::Owned(Box::new(fs))) + .into_inner(); + + let (stream, sink) = client.split(); + let mut server = Server::new(service); + + let (sender, mut receiver) = channel(CHANNEL_BUFFER_SIZE); + let reader = tokio::spawn(client_handler(stream, sink, sender)); + + server.initialize().await?; + server.initialized().await?; + + server.load_configuration().await?; + + let incorrect_config = r#"a {colr: blue;}"#; + server + .open_named_document(incorrect_config, url!("document.css"), "css") + .await?; + + let notification = tokio::select! { + msg = receiver.next() => msg, + _ = sleep(Duration::from_secs(1)) => { + panic!("timed out waiting for the server to send diagnostics") + } + }; + + assert_eq!( + notification, + Some(ServerNotification::PublishDiagnostics( + PublishDiagnosticsParams { + uri: url!("document.css"), + version: Some(0), + diagnostics: vec![lsp::Diagnostic { + range: Range { + start: Position { + line: 0, + character: 3, + }, + end: Position { + line: 0, + character: 7, + }, + }, + severity: Some(lsp::DiagnosticSeverity::ERROR), + code: Some(lsp::NumberOrString::String(String::from( + "lint/nursery/noUnknownProperty" + ))), + code_description: Some(CodeDescription { + href: Url::parse("https://biomejs.dev/linter/rules/no-unknown-property") + .unwrap() + }), + source: Some(String::from("biome")), + message: String::from("Unknown property is not allowed.",), + related_information: None, + tags: None, + data: None, + }], + } + )) + ); + + server.close_document().await?; + + server.shutdown().await?; + reader.abort(); + + Ok(()) +} + #[tokio::test] async fn no_code_actions_for_ignored_json_files() -> Result<()> { let factory = ServerFactory::default(); diff --git a/crates/biome_service/src/file_handlers/css.rs b/crates/biome_service/src/file_handlers/css.rs index a68785528f23..195c0e25b22e 100644 --- a/crates/biome_service/src/file_handlers/css.rs +++ b/crates/biome_service/src/file_handlers/css.rs @@ -1,4 +1,7 @@ -use super::{ExtensionHandler, LintParams, LintResults, ParseResult}; +use super::{ + is_diagnostic_error, CodeActionsParams, ExtensionHandler, FixAllParams, LintParams, + LintResults, ParseResult, +}; use crate::configuration::to_analyzer_rules; use crate::file_handlers::DebugCapabilities; use crate::file_handlers::{ @@ -8,7 +11,10 @@ use crate::settings::{ FormatSettings, LanguageListSettings, LanguageSettings, LinterSettings, OverrideSettings, ServiceLanguage, Settings, WorkspaceSettingsHandle, }; -use crate::workspace::{DocumentFileSource, GetSyntaxTreeResult, OrganizeImportsResult}; +use crate::workspace::{ + CodeAction, DocumentFileSource, FixAction, FixFileMode, FixFileResult, GetSyntaxTreeResult, + OrganizeImportsResult, PullActionsResult, +}; use crate::WorkspaceError; use biome_analyze::options::PreferredQuote; use biome_analyze::{ @@ -19,15 +25,17 @@ use biome_css_formatter::context::CssFormatOptions; use biome_css_formatter::format_node; use biome_css_parser::CssParserOptions; use biome_css_syntax::{CssLanguage, CssRoot, CssSyntaxNode}; -use biome_diagnostics::{category, Diagnostic, DiagnosticExt, Severity}; +use biome_diagnostics::{category, Applicability, Diagnostic, DiagnosticExt, Severity}; use biome_formatter::{ FormatError, IndentStyle, IndentWidth, LineEnding, LineWidth, Printed, QuoteStyle, }; use biome_fs::BiomePath; +use biome_js_analyze::RuleError; use biome_parser::AnyParse; -use biome_rowan::NodeCache; +use biome_rowan::{AstNode, NodeCache}; use biome_rowan::{TextRange, TextSize, TokenAtOffset}; -use tracing::{debug_span, info}; +use std::borrow::Cow; +use tracing::{debug_span, error, info, trace, trace_span}; #[derive(Debug, Clone, serde::Serialize, serde::Deserialize)] #[cfg_attr(feature = "schema", derive(schemars::JsonSchema))] @@ -173,9 +181,9 @@ impl ExtensionHandler for CssFileHandler { }, analyzer: AnalyzerCapabilities { lint: Some(lint), - code_actions: None, + code_actions: Some(code_actions), rename: None, - fix_all: None, + fix_all: Some(fix_all), organize_imports: Some(organize_imports), }, formatter: FormatterCapabilities { @@ -311,8 +319,8 @@ fn format_on_type( } fn lint(params: LintParams) -> LintResults { - debug_span!("Linting JavaScript file", path =? params.path, language =? params.language) - .in_scope(move || { + debug_span!("Linting CSS file", path =? params.path, language =? params.language).in_scope( + move || { let workspace_settings = ¶ms.settings; let analyzer_options = workspace_settings.analyzer_options::(params.path, ¶ms.language); @@ -399,7 +407,8 @@ fn lint(params: LintParams) -> LintResults { errors, skipped_diagnostics, } - }) + }, + ) } fn organize_imports(parse: AnyParse) -> Result { @@ -407,3 +416,173 @@ fn organize_imports(parse: AnyParse) -> Result().to_string(), }) } + +#[tracing::instrument(level = "debug", skip(params))] +pub(crate) fn code_actions(params: CodeActionsParams) -> PullActionsResult { + let CodeActionsParams { + parse, + range, + workspace, + path, + manifest: _, + language, + } = params; + debug_span!("Code actions JavaScript", range =? range, path =? path).in_scope(move || { + let tree = parse.tree(); + trace_span!("Parsed file", tree =? tree).in_scope(move || { + let settings = workspace.settings(); + let rules = settings.as_rules(params.path.as_path()); + let filter = rules + .as_ref() + .map(|rules| rules.as_enabled_rules()) + .unwrap_or_default() + .into_iter() + .collect::>(); + + let mut filter = AnalysisFilter::from_enabled_rules(Some(filter.as_slice())); + + filter.categories = RuleCategories::SYNTAX | RuleCategories::LINT; + if settings.organize_imports.enabled { + filter.categories |= RuleCategories::ACTION; + } + filter.range = Some(range); + + let analyzer_options = workspace.analyzer_options::(path, &language); + + let Some(_) = language.to_css_file_source() else { + error!("Could not determine the file source of the file"); + return PullActionsResult { actions: vec![] }; + }; + + trace!("CSS runs the analyzer"); + let mut actions = Vec::new(); + + analyze(&tree, filter, &analyzer_options, |signal| { + actions.extend(signal.actions().into_code_action_iter().map(|item| { + CodeAction { + category: item.category.clone(), + rule_name: item + .rule_name + .map(|(group, name)| (Cow::Borrowed(group), Cow::Borrowed(name))), + suggestion: item.suggestion, + } + })); + + ControlFlow::::Continue(()) + }); + + PullActionsResult { actions } + }) + }) +} + +/// If applies all the safe fixes to the given syntax tree. +pub(crate) fn fix_all(params: FixAllParams) -> Result { + let FixAllParams { + parse, + rules, + fix_file_mode, + settings, + should_format, + biome_path, + mut filter, + manifest: _, + document_file_source, + } = params; + + let mut tree: CssRoot = parse.tree(); + let mut actions = Vec::new(); + + filter.categories = RuleCategories::SYNTAX | RuleCategories::LINT; + + let mut skipped_suggested_fixes = 0; + let mut errors: u16 = 0; + let analyzer_options = + settings.analyzer_options::(biome_path, &document_file_source); + loop { + let (action, _) = analyze(&tree, filter, &analyzer_options, |signal| { + let current_diagnostic = signal.diagnostic(); + + if let Some(diagnostic) = current_diagnostic.as_ref() { + if is_diagnostic_error(diagnostic, rules) { + errors += 1; + } + } + + for action in signal.actions() { + // suppression actions should not be part of the fixes (safe or suggested) + if action.is_suppression() { + continue; + } + + match fix_file_mode { + FixFileMode::SafeFixes => { + if action.applicability == Applicability::MaybeIncorrect { + skipped_suggested_fixes += 1; + } + if action.applicability == Applicability::Always { + errors = errors.saturating_sub(1); + return ControlFlow::Break(action); + } + } + FixFileMode::SafeAndUnsafeFixes => { + if matches!( + action.applicability, + Applicability::Always | Applicability::MaybeIncorrect + ) { + errors = errors.saturating_sub(1); + return ControlFlow::Break(action); + } + } + } + } + + ControlFlow::Continue(()) + }); + + match action { + Some(action) => { + if let (root, Some((range, _))) = + action.mutation.commit_with_text_range_and_edit(true) + { + tree = match CssRoot::cast(root) { + Some(tree) => tree, + None => { + return Err(WorkspaceError::RuleError( + RuleError::ReplacedRootWithNonRootError { + rule_name: action.rule_name.map(|(group, rule)| { + (Cow::Borrowed(group), Cow::Borrowed(rule)) + }), + }, + )); + } + }; + actions.push(FixAction { + rule_name: action + .rule_name + .map(|(group, rule)| (Cow::Borrowed(group), Cow::Borrowed(rule))), + range, + }); + } + } + None => { + let code = if should_format { + format_node( + settings.format_options::(biome_path, &document_file_source), + tree.syntax(), + )? + .print()? + .into_code() + } else { + tree.syntax().to_string() + }; + return Ok(FixFileResult { + code, + skipped_suggested_fixes, + actions, + errors: errors.into(), + }); + } + } + } +} diff --git a/crates/biome_service/src/file_handlers/javascript.rs b/crates/biome_service/src/file_handlers/javascript.rs index edf3192c6d68..b5e317d0c968 100644 --- a/crates/biome_service/src/file_handlers/javascript.rs +++ b/crates/biome_service/src/file_handlers/javascript.rs @@ -619,8 +619,6 @@ pub(crate) fn code_actions(params: CodeActionsParams) -> PullActionsResult { } /// If applies all the safe fixes to the given syntax tree. -/// -/// If `indent_style` is [Some], it means that the formatting should be applied at the end pub(crate) fn fix_all(params: FixAllParams) -> Result { let FixAllParams { parse, From cbdfddbff5280b5186ef6244bad905fb164e7327 Mon Sep 17 00:00:00 2001 From: Emanuele Stoppa Date: Tue, 21 May 2024 11:44:58 +0100 Subject: [PATCH 2/2] revert change --- crates/biome_lsp/src/handlers/analysis.rs | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/crates/biome_lsp/src/handlers/analysis.rs b/crates/biome_lsp/src/handlers/analysis.rs index 78e311e4d60d..c214bda3a2b3 100644 --- a/crates/biome_lsp/src/handlers/analysis.rs +++ b/crates/biome_lsp/src/handlers/analysis.rs @@ -33,16 +33,7 @@ fn fix_all_kind() -> CodeActionKind { /// Queries the [`AnalysisServer`] for code actions of the file matching its path /// /// If the AnalysisServer has no matching file, results in error. -#[tracing::instrument(level = "debug", - skip_all, - fields( - uri = display(& params.text_document.uri), - range = debug(params.range), - only = debug(& params.context.only), - diagnostics = debug(& params.context.diagnostics) - ), - err -)] +#[tracing::instrument(level = "debug", skip_all, fields(uri = display(& params.text_document.uri), range = debug(params.range), only = debug(& params.context.only), diagnostics = debug(& params.context.diagnostics)), err)] pub(crate) fn code_actions( session: &Session, params: CodeActionParams, @@ -57,7 +48,7 @@ pub(crate) fn code_actions( .with_organize_imports() .build(), })?; - + if !file_features.supports_lint() && !file_features.supports_organize_imports() { debug!("Linter and organize imports are both disabled"); return Ok(Some(Vec::new()));