From 6bd0a69f68fd81deb51ad59d875a3c866d92aa4c Mon Sep 17 00:00:00 2001 From: Emanuele Stoppa Date: Fri, 17 Mar 2023 17:05:27 -0300 Subject: [PATCH] fix(rome_ci): apply import sorting (#4305) * fix(rome_ci): apply import sorting * clippy * better tests * check feature inside LSP * fix regression --- crates/rome_cli/src/commands/check.rs | 2 +- crates/rome_cli/src/execute.rs | 28 +- .../src/{traversal.rs => traversal/mod.rs} | 382 +++++------------- crates/rome_cli/src/traversal/process_file.rs | 328 +++++++++++++++ crates/rome_cli/tests/commands/check.rs | 166 +++++++- crates/rome_cli/tests/commands/ci.rs | 59 ++- crates/rome_cli/tests/configs.rs | 4 +- ...rts.snap => applies_organize_imports.snap} | 9 +- ...rror.snap => apply_unsafe_with_error.snap} | 2 +- ...ies_organize_imports_for_ignored_file.snap | 25 ++ .../shows_organize_imports_diff_on_check.snap | 58 +++ ..._organize_imports_diff_on_check_apply.snap | 58 +++ .../ci_formatter_linter_organize_imports.snap | 86 ++++ .../creates_config_file.snap | 2 +- ...en_rome_installed_via_package_manager.snap | 2 +- .../src/categories.rs | 1 + crates/rome_fs/src/fs.rs | 3 + crates/rome_fs/src/fs/memory.rs | 8 + crates/rome_fs/src/fs/os.rs | 8 + crates/rome_fs/src/lib.rs | 2 +- crates/rome_lsp/src/handlers/analysis.rs | 16 +- crates/rome_lsp/src/server.rs | 55 +-- crates/rome_service/src/configuration/mod.rs | 4 +- .../src/configuration/organize_imports.rs | 38 +- .../configuration/parse/json/javascript.rs | 11 +- .../parse/json/organize_imports.rs | 12 +- .../src/file_handlers/javascript.rs | 67 ++- crates/rome_service/src/file_handlers/json.rs | 7 +- crates/rome_service/src/file_handlers/mod.rs | 5 +- crates/rome_service/src/settings.rs | 38 +- crates/rome_service/src/workspace.rs | 49 ++- crates/rome_service/src/workspace/client.rs | 10 +- crates/rome_service/src/workspace/server.rs | 38 +- editors/vscode/configuration_schema.json | 10 +- npm/backend-jsonrpc/src/workspace.ts | 13 +- npm/rome/configuration_schema.json | 10 +- 36 files changed, 1237 insertions(+), 379 deletions(-) rename crates/rome_cli/src/{traversal.rs => traversal/mod.rs} (71%) create mode 100644 crates/rome_cli/src/traversal/process_file.rs rename crates/rome_cli/tests/snapshots/main_commands_check/{organize_imports.snap => applies_organize_imports.snap} (77%) rename crates/rome_cli/tests/snapshots/main_commands_check/{apply_suggested_with_error.snap => apply_unsafe_with_error.snap} (96%) create mode 100644 crates/rome_cli/tests/snapshots/main_commands_check/dont_applies_organize_imports_for_ignored_file.snap create mode 100644 crates/rome_cli/tests/snapshots/main_commands_check/shows_organize_imports_diff_on_check.snap create mode 100644 crates/rome_cli/tests/snapshots/main_commands_check/shows_organize_imports_diff_on_check_apply.snap create mode 100644 crates/rome_cli/tests/snapshots/main_commands_ci/ci_formatter_linter_organize_imports.snap diff --git a/crates/rome_cli/src/commands/check.rs b/crates/rome_cli/src/commands/check.rs index e55d733f0f4..9cffd8f9b96 100644 --- a/crates/rome_cli/src/commands/check.rs +++ b/crates/rome_cli/src/commands/check.rs @@ -41,7 +41,7 @@ pub(crate) fn check(mut session: CliSession) -> Result<(), CliDiagnostic> { } else if apply && !apply_suggested { Some(FixFileMode::SafeFixes) } else { - Some(FixFileMode::SafeAndSuggestedFixes) + Some(FixFileMode::SafeAndUnsafeFixes) }; execute_mode( diff --git a/crates/rome_cli/src/execute.rs b/crates/rome_cli/src/execute.rs index f55daa30b8a..fdf67aea5d2 100644 --- a/crates/rome_cli/src/execute.rs +++ b/crates/rome_cli/src/execute.rs @@ -112,16 +112,36 @@ impl Execution { } pub(crate) const fn is_check_apply(&self) -> bool { - match self.traversal_mode { - TraversalMode::Check { fix_file_mode } => fix_file_mode.is_some(), - _ => false, - } + matches!( + self.traversal_mode, + TraversalMode::Check { + fix_file_mode: Some(FixFileMode::SafeFixes) + } + ) + } + + pub(crate) const fn is_check_apply_unsafe(&self) -> bool { + matches!( + self.traversal_mode, + TraversalMode::Check { + fix_file_mode: Some(FixFileMode::SafeAndUnsafeFixes) + } + ) } pub(crate) const fn is_format(&self) -> bool { matches!(self.traversal_mode, TraversalMode::Format { .. }) } + /// Whether the traversal mode requires write access to files + pub(crate) const fn requires_write_access(&self) -> bool { + match self.traversal_mode { + TraversalMode::Check { fix_file_mode } => fix_file_mode.is_some(), + TraversalMode::CI => false, + TraversalMode::Format { write, .. } => write, + } + } + pub(crate) fn as_stdin_file(&self) -> Option<&(PathBuf, String)> { match &self.traversal_mode { TraversalMode::Format { stdin, .. } => stdin.as_ref(), diff --git a/crates/rome_cli/src/traversal.rs b/crates/rome_cli/src/traversal/mod.rs similarity index 71% rename from crates/rome_cli/src/traversal.rs rename to crates/rome_cli/src/traversal/mod.rs index 327dc8a3bc2..d810abcad59 100644 --- a/crates/rome_cli/src/traversal.rs +++ b/crates/rome_cli/src/traversal/mod.rs @@ -1,3 +1,6 @@ +mod process_file; + +use crate::traversal::process_file::DiffKind; use crate::{ CliDiagnostic, CliSession, Execution, FormatterReportFileDetail, FormatterReportSummary, Report, ReportDiagnostic, ReportDiff, ReportErrorKind, ReportKind, TraversalMode, @@ -6,19 +9,18 @@ use crossbeam::{ channel::{unbounded, Receiver, Sender}, select, }; +use process_file::{process_file, FileStatus, Message}; use rome_console::{fmt, markup, Console, ConsoleExt}; use rome_diagnostics::{ adapters::{IoError, StdError}, category, Advices, Category, Diagnostic, DiagnosticExt, Error, PrintDescription, PrintDiagnostic, Resource, Severity, Visit, }; -use rome_fs::{FileSystem, OpenOptions, PathInterner, RomePath}; +use rome_fs::{FileSystem, PathInterner, RomePath}; use rome_fs::{TraversalContext, TraversalScope}; -use rome_service::workspace::{IsPathIgnoredParams, SupportsFeatureResult, UnsupportedReason}; +use rome_service::workspace::{IsPathIgnoredParams, SupportsFeatureResult}; use rome_service::{ - workspace::{ - FeatureName, FileGuard, Language, OpenFileParams, RuleCategories, SupportsFeatureParams, - }, + workspace::{FeatureName, SupportsFeatureParams}, Workspace, WorkspaceError, }; use rome_text_edit::TextEdit; @@ -276,7 +278,19 @@ struct ProcessMessagesOptions<'ctx> { category = "format", message = "File content differs from formatting output" )] -struct CIDiffDiagnostic<'a> { +struct CIFormatDiffDiagnostic<'a> { + #[location(resource)] + file_name: &'a str, + #[advice] + diff: FormatDiffAdvice<'a>, +} + +#[derive(Debug, Diagnostic)] +#[diagnostic( + category = "organizeImports", + message = "Import statements differs from the output" +)] +struct CIOrganizeImportsDiffDiagnostic<'a> { #[location(resource)] file_name: &'a str, #[advice] @@ -285,8 +299,8 @@ struct CIDiffDiagnostic<'a> { #[derive(Debug, Diagnostic)] #[diagnostic( - severity = Information, category = "format", + severity = Information, message = "Formatter would have printed the following content:" )] struct FormatDiffDiagnostic<'a> { @@ -296,6 +310,19 @@ struct FormatDiffDiagnostic<'a> { diff: FormatDiffAdvice<'a>, } +#[derive(Debug, Diagnostic)] +#[diagnostic( + category = "organizeImports", + severity = Information, + message = "Import statements could be sorted:" +)] +struct OrganizeImportsDiffDiagnostic<'a> { + #[location(resource)] + file_name: &'a str, + #[advice] + diff: FormatDiffAdvice<'a>, +} + #[derive(Debug)] struct FormatDiffAdvice<'a> { old: &'a str, @@ -520,11 +547,11 @@ fn process_messages(options: ProcessMessagesOptions) { } } } - Message::Diff { file_name, old, new, + diff_kind, } => { if mode.is_ci() { // A diff is an error in CI mode @@ -545,29 +572,59 @@ fn process_messages(options: ProcessMessagesOptions) { if mode.should_report_to_terminal() { if should_print { if mode.is_ci() { - let diag = CIDiffDiagnostic { - file_name: &file_name, - diff: FormatDiffAdvice { - old: &old, - new: &new, - }, + match diff_kind { + DiffKind::Format => { + let diag = CIFormatDiffDiagnostic { + file_name: &file_name, + diff: FormatDiffAdvice { + old: &old, + new: &new, + }, + }; + console.error(markup! { + {if verbose { PrintDiagnostic::verbose(&diag) } else { PrintDiagnostic::simple(&diag) }} + }); + } + DiffKind::OrganizeImports => { + let diag = CIOrganizeImportsDiffDiagnostic { + file_name: &file_name, + diff: FormatDiffAdvice { + old: &old, + new: &new, + }, + }; + console.error(markup! { + {if verbose { PrintDiagnostic::verbose(&diag) } else { PrintDiagnostic::simple(&diag) }} + }); + } }; - - console.error(markup! { - {if verbose { PrintDiagnostic::verbose(&diag) } else { PrintDiagnostic::simple(&diag) }} - }); } else { - let diag = FormatDiffDiagnostic { - file_name: &file_name, - diff: FormatDiffAdvice { - old: &old, - new: &new, - }, + match diff_kind { + DiffKind::Format => { + let diag = FormatDiffDiagnostic { + file_name: &file_name, + diff: FormatDiffAdvice { + old: &old, + new: &new, + }, + }; + console.error(markup! { + {if verbose { PrintDiagnostic::verbose(&diag) } else { PrintDiagnostic::simple(&diag) }} + }); + } + DiffKind::OrganizeImports => { + let diag = OrganizeImportsDiffDiagnostic { + file_name: &file_name, + diff: FormatDiffAdvice { + old: &old, + new: &new, + }, + }; + console.error(markup! { + {if verbose { PrintDiagnostic::verbose(&diag) } else { PrintDiagnostic::simple(&diag) }} + }); + } }; - - console.error(markup! { - {if verbose { PrintDiagnostic::verbose(&diag) } else { PrintDiagnostic::simple(&diag) }} - }); } } } else { @@ -600,7 +657,7 @@ fn process_messages(options: ProcessMessagesOptions) { } /// Context object shared between directory traversal tasks -struct TraversalOptions<'ctx, 'app> { +pub(crate) struct TraversalOptions<'ctx, 'app> { /// Shared instance of [FileSystem] fs: &'app dyn FileSystem, /// Instance of [Workspace] used by this instance of the CLI @@ -652,6 +709,16 @@ impl<'ctx, 'app> TraversalOptions<'ctx, 'app> { }) } + fn can_organize_imports( + &self, + rome_path: &RomePath, + ) -> Result { + self.workspace.supports_feature(SupportsFeatureParams { + path: rome_path.clone(), + feature: FeatureName::OrganizeImports, + }) + } + fn miss_handler_err(&self, err: WorkspaceError, rome_path: &RomePath) { self.push_diagnostic( StdError::from(err) @@ -687,6 +754,7 @@ impl<'ctx, 'app> TraversalContext for TraversalOptions<'ctx, 'app> { let can_lint = self.can_lint(rome_path); let can_format = self.can_format(rome_path); + let can_organize_imports = self.can_organize_imports(rome_path); match self.execution.traversal_mode() { TraversalMode::Check { .. } => can_lint @@ -695,14 +763,16 @@ impl<'ctx, 'app> TraversalContext for TraversalOptions<'ctx, 'app> { self.miss_handler_err(err, rome_path); false }), - TraversalMode::CI { .. } => match (can_format, can_lint) { + TraversalMode::CI { .. } => match (can_format, can_lint, can_organize_imports) { // the result of the error is the same, rome can't handle the file - (Err(err), _) | (_, Err(err)) => { + (Err(err), _, _) | (_, Err(err), _) | (_, _, Err(err)) => { self.miss_handler_err(err, rome_path); false } - (Ok(can_format), Ok(can_lint)) => { - can_lint.reason.is_none() || can_format.reason.is_none() + (Ok(can_format), Ok(can_lint), Ok(can_organize_imports)) => { + can_lint.reason.is_none() + || can_format.reason.is_none() + || can_organize_imports.reason.is_none() } }, TraversalMode::Format { .. } => can_format @@ -749,252 +819,6 @@ fn handle_file(ctx: &TraversalOptions, path: &Path) { } } -enum FileStatus { - Success, - Message(Message), - Ignored, -} - -/// The return type for [process_file], with the following semantics: -/// - `Ok(Success)` means the operation was successful (the file is added to -/// the `processed` counter) -/// - `Ok(Message(_))` means the operation was successful but a message still -/// needs to be printed (eg. the diff when not in CI or write mode) -/// - `Ok(Ignored)` means the file was ignored (the file is not added to the -/// `processed` or `skipped` counters) -/// - `Err(_)` means the operation failed and the file should be added to the -/// `skipped` counter -type FileResult = Result; - -/// This function performs the actual processing: it reads the file from disk -/// and parse it; analyze and / or format it; then it either fails if error -/// diagnostics were emitted, or compare the formatted code with the original -/// content of the file and emit a diff or write the new content to the disk if -/// write mode is enabled -fn process_file(ctx: &TraversalOptions, path: &Path) -> FileResult { - tracing::trace_span!("process_file", path = ?path).in_scope(move || { - let rome_path = RomePath::new(path); - - let supported_format = ctx.can_format(&rome_path).with_file_path_and_code( - path.display().to_string(), - category!("files/missingHandler"), - )?; - - let supported_lint = ctx.can_lint(&rome_path).with_file_path_and_code( - path.display().to_string(), - category!("files/missingHandler"), - )?; - - let unsupported_reason = match ctx.execution.traversal_mode() { - TraversalMode::Check { .. } => supported_lint.reason.as_ref(), - TraversalMode::CI { .. } => supported_lint - .reason - .as_ref() - .and(supported_format.reason.as_ref()), - TraversalMode::Format { .. } => supported_format.reason.as_ref(), - }; - - if let Some(reason) = unsupported_reason { - return match reason { - UnsupportedReason::FileNotSupported => Err(Message::from( - UnhandledDiagnostic.with_file_path(path.display().to_string()), - )), - UnsupportedReason::FeatureNotEnabled | UnsupportedReason::Ignored => { - Ok(FileStatus::Ignored) - } - }; - } - - let write_access = matches!( - ctx.execution.traversal_mode(), - TraversalMode::Check { - fix_file_mode: Some(_), - } | TraversalMode::Format { write: true, .. } - ); - - let open_options = OpenOptions::default().read(true).write(write_access); - let mut file = ctx - .fs - .open_with_options(path, open_options) - .with_file_path(path.display().to_string())?; - - let mut input = String::new(); - file.read_to_string(&mut input) - .with_file_path(path.display().to_string())?; - ctx.increment_processed(); - - let file_guard = FileGuard::open( - ctx.workspace, - OpenFileParams { - path: rome_path, - version: 0, - content: input.clone(), - language_hint: Language::default(), - }, - ) - .with_file_path_and_code(path.display().to_string(), category!("internalError/fs"))?; - - if let Some(fix_mode) = ctx.execution.as_fix_file_mode() { - let fixed = file_guard - .fix_file(*fix_mode) - .with_file_path_and_code(path.display().to_string(), category!("lint"))?; - - ctx.push_message(Message::SkippedFixes { - skipped_suggested_fixes: fixed.skipped_suggested_fixes, - }); - - if fixed.code != input { - file.set_content(fixed.code.as_bytes()) - .with_file_path(path.display().to_string())?; - } - - return if fixed.errors == 0 { - Ok(FileStatus::Success) - } else { - Ok(FileStatus::Message(Message::ApplyError( - CliDiagnostic::file_apply_error(path.display().to_string()), - ))) - }; - } - - let categories = if ctx.execution.is_format() || supported_lint.reason.is_some() { - RuleCategories::SYNTAX - } else { - RuleCategories::SYNTAX | RuleCategories::LINT - }; - - let max_diagnostics = ctx.remaining_diagnostics.load(Ordering::Relaxed); - let result = file_guard - .pull_diagnostics(categories, max_diagnostics.into()) - .with_file_path_and_code(path.display().to_string(), category!("lint"))?; - - // In formatting mode, abort immediately if the file has errors - let errors = result.errors; - match ctx.execution.traversal_mode() { - TraversalMode::Format { ignore_errors, .. } if errors > 0 => { - return Err(if *ignore_errors { - Message::from(SkippedDiagnostic.with_file_path(path.display().to_string())) - } else { - Message::Diagnostics { - name: path.display().to_string(), - content: input, - diagnostics: result.diagnostics.into_iter().map(Error::from).collect(), - skipped_diagnostics: result.skipped_diagnostics, - } - }); - } - - _ => {} - } - - // In format mode the diagnostics have already been checked for errors - // at this point, so they can just be dropped now since we don't want - // to print syntax warnings for the format command - let no_diagnostics = result.diagnostics.is_empty() && result.skipped_diagnostics == 0; - let result = if no_diagnostics || ctx.execution.is_format() { - FileStatus::Success - } else { - FileStatus::Message(Message::Diagnostics { - name: path.display().to_string(), - content: input.clone(), - diagnostics: result.diagnostics.into_iter().map(Error::from).collect(), - skipped_diagnostics: result.skipped_diagnostics, - }) - }; - - if errors > 0 { - // Having errors is considered a "success" at this point because - // this is only reachable on the check / CI path (the parser result - // is checked for errors earlier on the format path, and that mode - // doesn't run the analyzer so no new diagnostics could have been - // added), and having errors on these paths still means the file - // was processed (added to the checked files counter) - return Ok(result); - } - - if supported_format.reason.is_none() { - let write = match ctx.execution.traversal_mode() { - // In check mode do not run the formatter and return the result immediately, - // but only if the argument `--apply` is not passed. - TraversalMode::Check { .. } => { - if ctx.execution.as_fix_file_mode().is_some() { - true - } else { - return Ok(result); - } - } - TraversalMode::CI { .. } => false, - TraversalMode::Format { write, .. } => *write, - }; - - let printed = file_guard - .format_file() - .with_file_path_and_code(path.display().to_string(), category!("format"))?; - - let output = printed.into_code(); - if output != input { - if write { - file.set_content(output.as_bytes()) - .with_file_path(path.display().to_string())?; - } else { - if !ctx.execution.should_report_to_terminal() { - ctx.push_format_stat( - path.display().to_string(), - FormatterReportFileDetail { - formatted_content: Some(output.clone()), - }, - ) - } - // Returning the diff message will discard the content of - // diagnostics, meaning those would not be printed so they - // have to be manually sent through the console channel - if let FileStatus::Message(msg) = result { - ctx.messages.send(msg).ok(); - } - - return Ok(FileStatus::Message(Message::Diff { - file_name: path.display().to_string(), - old: input, - new: output, - })); - } - } - } - - Ok(result) - }) -} - -/// Wrapper type for messages that can be printed during the traversal process -enum Message { - SkippedFixes { - /// Suggested fixes skipped during the lint traversal - skipped_suggested_fixes: u32, - }, - ApplyError(CliDiagnostic), - Error(Error), - Diagnostics { - name: String, - content: String, - diagnostics: Vec, - skipped_diagnostics: u64, - }, - Diff { - file_name: String, - old: String, - new: String, - }, -} - -impl From for Message -where - Error: From, -{ - fn from(err: D) -> Self { - Self::Error(Error::from(err)) - } -} - #[derive(Debug, Diagnostic)] #[diagnostic(category = "internalError/panic", tags(INTERNAL))] struct PanicDiagnostic { diff --git a/crates/rome_cli/src/traversal/process_file.rs b/crates/rome_cli/src/traversal/process_file.rs new file mode 100644 index 00000000000..c1b5f41f2bd --- /dev/null +++ b/crates/rome_cli/src/traversal/process_file.rs @@ -0,0 +1,328 @@ +use crate::execute::TraversalMode; +use crate::traversal::{ + ResultExt, ResultIoExt, SkippedDiagnostic, TraversalOptions, UnhandledDiagnostic, +}; +use crate::{CliDiagnostic, FormatterReportFileDetail}; +use rome_diagnostics::{category, DiagnosticExt, Error}; +use rome_fs::{OpenOptions, RomePath}; +use rome_service::workspace::{ + FileGuard, Language, OpenFileParams, RuleCategories, UnsupportedReason, +}; +use std::path::Path; +use std::sync::atomic::Ordering; + +pub(crate) enum FileStatus { + Success, + Message(Message), + Ignored, +} + +/// Wrapper type for messages that can be printed during the traversal process +pub(crate) enum Message { + SkippedFixes { + /// Suggested fixes skipped during the lint traversal + skipped_suggested_fixes: u32, + }, + ApplyError(CliDiagnostic), + Error(Error), + Diagnostics { + name: String, + content: String, + diagnostics: Vec, + skipped_diagnostics: u64, + }, + Diff { + file_name: String, + old: String, + new: String, + diff_kind: DiffKind, + }, +} + +pub(crate) enum DiffKind { + Format, + OrganizeImports, +} + +impl From for Message +where + Error: From, +{ + fn from(err: D) -> Self { + Self::Error(Error::from(err)) + } +} + +/// The return type for [process_file], with the following semantics: +/// - `Ok(Success)` means the operation was successful (the file is added to +/// the `processed` counter) +/// - `Ok(Message(_))` means the operation was successful but a message still +/// needs to be printed (eg. the diff when not in CI or write mode) +/// - `Ok(Ignored)` means the file was ignored (the file is not added to the +/// `processed` or `skipped` counters) +/// - `Err(_)` means the operation failed and the file should be added to the +/// `skipped` counter +pub(crate) type FileResult = Result; + +/// This function performs the actual processing: it reads the file from disk +/// and parse it; analyze and / or format it; then it either fails if error +/// diagnostics were emitted, or compare the formatted code with the original +/// content of the file and emit a diff or write the new content to the disk if +/// write mode is enabled +pub(crate) fn process_file(ctx: &TraversalOptions, path: &Path) -> FileResult { + tracing::trace_span!("process_file", path = ?path).in_scope(move || { + let rome_path = RomePath::new(path); + + let supported_format = ctx.can_format(&rome_path).with_file_path_and_code( + path.display().to_string(), + category!("files/missingHandler"), + )?; + + let supported_lint = ctx.can_lint(&rome_path).with_file_path_and_code( + path.display().to_string(), + category!("files/missingHandler"), + )?; + + let supported_organize_imports = ctx + .can_organize_imports(&rome_path) + .with_file_path_and_code( + path.display().to_string(), + category!("files/missingHandler"), + )?; + + let unsupported_reason = match ctx.execution.traversal_mode() { + TraversalMode::Check { .. } => supported_lint + .reason + .as_ref() + .and(supported_organize_imports.reason.as_ref()), + TraversalMode::CI { .. } => supported_lint + .reason + .as_ref() + .and(supported_format.reason.as_ref()) + .and(supported_organize_imports.reason.as_ref()), + TraversalMode::Format { .. } => supported_format.reason.as_ref(), + }; + + if let Some(reason) = unsupported_reason { + return match reason { + UnsupportedReason::FileNotSupported => Err(Message::from( + UnhandledDiagnostic.with_file_path(path.display().to_string()), + )), + UnsupportedReason::FeatureNotEnabled | UnsupportedReason::Ignored => { + Ok(FileStatus::Ignored) + } + }; + } + + let open_options = OpenOptions::default() + .read(true) + .write(ctx.execution.requires_write_access()); + let mut file = ctx + .fs + .open_with_options(path, open_options) + .with_file_path(path.display().to_string())?; + + let mut input = String::new(); + file.read_to_string(&mut input) + .with_file_path(path.display().to_string())?; + ctx.increment_processed(); + + let file_guard = FileGuard::open( + ctx.workspace, + OpenFileParams { + path: rome_path, + version: 0, + content: input.clone(), + language_hint: Language::default(), + }, + ) + .with_file_path_and_code(path.display().to_string(), category!("internalError/fs"))?; + + let mut errors = 0; + + if let Some(fix_mode) = ctx.execution.as_fix_file_mode() { + let fixed = file_guard + .fix_file(*fix_mode) + .with_file_path_and_code(path.display().to_string(), category!("lint"))?; + + ctx.push_message(Message::SkippedFixes { + skipped_suggested_fixes: fixed.skipped_suggested_fixes, + }); + + if fixed.code != input { + file.set_content(fixed.code.as_bytes()) + .with_file_path(path.display().to_string())?; + file_guard.change_file(file.file_version(), fixed.code)?; + } + errors = fixed.errors; + } + + if supported_organize_imports.is_supported() && ctx.execution.is_check() { + let sorted = file_guard.organize_imports().with_file_path_and_code( + path.display().to_string(), + category!("organizeImports"), + )?; + + if let Some(output) = sorted.code { + if output != input { + if ctx.execution.is_check_apply_unsafe() { + file.set_content(output.as_bytes()) + .with_file_path(path.display().to_string())?; + file_guard.change_file(file.file_version(), output)?; + } else { + errors += 1; + ctx.messages + .send(Message::Diff { + file_name: path.display().to_string(), + old: input.clone(), + new: output, + diff_kind: DiffKind::OrganizeImports, + }) + .ok(); + } + } + } + } + + // If we are here, errors were emitted when applying code actions, so checking only for errors should be safe + if errors > 0 { + return Ok(FileStatus::Message(Message::ApplyError( + CliDiagnostic::file_apply_error(path.display().to_string()), + ))); + } else if ctx.execution.is_check_apply() || ctx.execution.is_check_apply_unsafe() { + return Ok(FileStatus::Success); + } + + let categories = if ctx.execution.is_format() || !supported_lint.is_supported() { + RuleCategories::SYNTAX + } else { + RuleCategories::SYNTAX | RuleCategories::LINT + }; + + let max_diagnostics = ctx.remaining_diagnostics.load(Ordering::Relaxed); + let result = file_guard + .pull_diagnostics(categories, max_diagnostics.into()) + .with_file_path_and_code(path.display().to_string(), category!("lint"))?; + + // In formatting mode, abort immediately if the file has errors + errors = result.errors; + match ctx.execution.traversal_mode() { + TraversalMode::Format { ignore_errors, .. } if errors > 0 => { + return Err(if *ignore_errors { + Message::from(SkippedDiagnostic.with_file_path(path.display().to_string())) + } else { + Message::Diagnostics { + name: path.display().to_string(), + content: input, + diagnostics: result.diagnostics.into_iter().map(Error::from).collect(), + skipped_diagnostics: result.skipped_diagnostics, + } + }); + } + + _ => {} + } + + // In format mode the diagnostics have already been checked for errors + // at this point, so they can just be dropped now since we don't want + // to print syntax warnings for the format command + let no_diagnostics = result.diagnostics.is_empty() && result.skipped_diagnostics == 0; + let result = if no_diagnostics || ctx.execution.is_format() { + FileStatus::Success + } else { + FileStatus::Message(Message::Diagnostics { + name: path.display().to_string(), + content: input.clone(), + diagnostics: result.diagnostics.into_iter().map(Error::from).collect(), + skipped_diagnostics: result.skipped_diagnostics, + }) + }; + + if errors > 0 { + // Having errors is considered a "success" at this point because + // this is only reachable on the check / CI path (the parser result + // is checked for errors earlier on the format path, and that mode + // doesn't run the analyzer so no new diagnostics could have been + // added), and having errors on these paths still means the file + // was processed (added to the checked files counter) + return Ok(result); + } + + if supported_organize_imports.is_supported() + // we want to print a diff only if we are in CI + // or we are running "check" or "check --apply" + && (ctx.execution.is_ci() || !ctx.execution.is_check_apply_unsafe()) + { + let sorted = file_guard.organize_imports().with_file_path_and_code( + path.display().to_string(), + category!("organizeImports"), + )?; + + if let Some(output) = sorted.code { + if output != input { + ctx.messages + .send(Message::Diff { + file_name: path.display().to_string(), + old: input.clone(), + new: output, + diff_kind: DiffKind::OrganizeImports, + }) + .ok(); + } + } + } + + if supported_format.is_supported() { + let should_write = match ctx.execution.traversal_mode() { + // In check mode do not run the formatter and return the result immediately, + // but only if the argument `--apply` is not passed. + TraversalMode::Check { .. } => { + if ctx.execution.as_fix_file_mode().is_some() { + true + } else { + return Ok(result); + } + } + TraversalMode::CI { .. } => false, + TraversalMode::Format { write, .. } => *write, + }; + + let printed = file_guard + .format_file() + .with_file_path_and_code(path.display().to_string(), category!("format"))?; + + let output = printed.into_code(); + if output != input { + if should_write { + file.set_content(output.as_bytes()) + .with_file_path(path.display().to_string())?; + file_guard.change_file(file.file_version(), output)?; + } else { + if !ctx.execution.should_report_to_terminal() { + ctx.push_format_stat( + path.display().to_string(), + FormatterReportFileDetail { + formatted_content: Some(output.clone()), + }, + ) + } + // Returning the diff message will discard the content of + // diagnostics, meaning those would not be printed so they + // have to be manually sent through the console channel + if let FileStatus::Message(msg) = result { + ctx.messages.send(msg).ok(); + } + + return Ok(FileStatus::Message(Message::Diff { + file_name: path.display().to_string(), + old: input, + new: output, + diff_kind: DiffKind::Format, + })); + } + } + } + + Ok(result) + }) +} diff --git a/crates/rome_cli/tests/commands/check.rs b/crates/rome_cli/tests/commands/check.rs index c3c430c334e..a769c0b8a2d 100644 --- a/crates/rome_cli/tests/commands/check.rs +++ b/crates/rome_cli/tests/commands/check.rs @@ -319,7 +319,7 @@ fn apply_suggested() { } #[test] -fn apply_suggested_with_error() { +fn apply_unsafe_with_error() { let mut fs = MemoryFileSystem::default(); let mut console = BufferConsole::default(); @@ -378,7 +378,7 @@ function f() { arguments; } assert_cli_snapshot(SnapshotPayload::new( module_path!(), - "apply_suggested_with_error", + "apply_unsafe_with_error", fs, console, result, @@ -1419,19 +1419,22 @@ fn nursery_unstable() { } #[test] -fn organize_imports() { +fn applies_organize_imports() { let mut fs = MemoryFileSystem::default(); let mut console = BufferConsole::default(); + let rome_json = r#"{ "organizeImports": { "enabled": true } }"#; + + let config_path = Path::new("rome.json"); + fs.insert(config_path.into(), rome_json.as_bytes()); + let file_path = Path::new("check.js"); - let content = r#" -import { lorem, foom, bar } from "foo"; -import * as something from "../something"; - "#; - let expected = r#" + let content = r#"import { lorem, foom, bar } from "foo"; import * as something from "../something"; +"#; + let expected = r#"import * as something from "../something"; import { bar, foom, lorem } from "foo"; - "#; +"#; fs.insert(file_path.into(), content.as_bytes()); let result = run_cli( @@ -1460,7 +1463,150 @@ import { bar, foom, lorem } from "foo"; assert_cli_snapshot(SnapshotPayload::new( module_path!(), - "organize_imports", + "applies_organize_imports", + fs, + console, + result, + )); +} + +#[test] +fn shows_organize_imports_diff_on_check() { + let mut fs = MemoryFileSystem::default(); + let mut console = BufferConsole::default(); + + let rome_json = r#"{ "organizeImports": { "enabled": true } }"#; + + let config_path = Path::new("rome.json"); + fs.insert(config_path.into(), rome_json.as_bytes()); + + let file_path = Path::new("check.js"); + let content = r#"import { lorem, foom, bar } from "foo"; +import * as something from "../something"; +"#; + fs.insert(file_path.into(), content.as_bytes()); + + let result = run_cli( + DynRef::Borrowed(&mut fs), + &mut console, + Arguments::from_vec(vec![OsString::from("check"), file_path.as_os_str().into()]), + ); + + assert!(result.is_err(), "run_cli returned {result:?}"); + + let mut file = fs + .open(file_path) + .expect("formatting target file was removed by the CLI"); + + let mut content = String::new(); + file.read_to_string(&mut content) + .expect("failed to read file from memory FS"); + + assert_eq!(content, content); + + drop(file); + + assert_cli_snapshot(SnapshotPayload::new( + module_path!(), + "shows_organize_imports_diff_on_check", + fs, + console, + result, + )); +} + +#[test] +fn shows_organize_imports_diff_on_check_apply() { + let mut fs = MemoryFileSystem::default(); + let mut console = BufferConsole::default(); + + let rome_json = r#"{ "organizeImports": { "enabled": true } }"#; + + let config_path = Path::new("rome.json"); + fs.insert(config_path.into(), rome_json.as_bytes()); + + let file_path = Path::new("check.js"); + let content = r#"import { lorem, foom, bar } from "foo"; +import * as something from "../something"; +"#; + fs.insert(file_path.into(), content.as_bytes()); + + let result = run_cli( + DynRef::Borrowed(&mut fs), + &mut console, + Arguments::from_vec(vec![ + OsString::from("check"), + OsString::from("--apply"), + file_path.as_os_str().into(), + ]), + ); + + assert!(result.is_err(), "run_cli returned {result:?}"); + + let mut file = fs + .open(file_path) + .expect("formatting target file was removed by the CLI"); + + let mut content = String::new(); + file.read_to_string(&mut content) + .expect("failed to read file from memory FS"); + + assert_eq!(content, content); + + drop(file); + + assert_cli_snapshot(SnapshotPayload::new( + module_path!(), + "shows_organize_imports_diff_on_check_apply", + fs, + console, + result, + )); +} + +#[test] +fn dont_applies_organize_imports_for_ignored_file() { + let mut fs = MemoryFileSystem::default(); + let mut console = BufferConsole::default(); + + let rome_json = r#"{ "organizeImports": { "enabled": true, "ignore": ["check.js"] } }"#; + + let config_path = Path::new("rome.json"); + fs.insert(config_path.into(), rome_json.as_bytes()); + + let file_path = Path::new("check.js"); + let content = r#"import { lorem, foom, bar } from "foo"; +import * as something from "../something"; +"#; + fs.insert(file_path.into(), content.as_bytes()); + + let result = run_cli( + DynRef::Borrowed(&mut fs), + &mut console, + Arguments::from_vec(vec![ + OsString::from("check"), + OsString::from("--apply-unsafe"), + file_path.as_os_str().into(), + ]), + ); + + assert!(result.is_ok(), "run_cli returned {result:?}"); + + let mut file = fs + .open(file_path) + .expect("formatting target file was removed by the CLI"); + + let mut content = String::new(); + file.read_to_string(&mut content) + .expect("failed to read file from memory FS"); + + assert_eq!(content, content); + + drop(file); + + assert_cli_snapshot(SnapshotPayload::new( + module_path!(), + "dont_applies_organize_imports_for_ignored_file", fs, console, result, diff --git a/crates/rome_cli/tests/commands/ci.rs b/crates/rome_cli/tests/commands/ci.rs index e6b8f65af0d..de02ec26810 100644 --- a/crates/rome_cli/tests/commands/ci.rs +++ b/crates/rome_cli/tests/commands/ci.rs @@ -329,7 +329,7 @@ import * as something from "../something"; ]), ); - assert!(result.is_ok(), "run_cli returned {result:?}"); + // assert!(result.is_ok(), "run_cli returned {result:?}"); let mut file = fs .open(file_path) @@ -687,3 +687,60 @@ fn print_verbose() { result, )); } + +#[test] +fn ci_formatter_linter_organize_imports() { + let mut fs = MemoryFileSystem::default(); + let mut console = BufferConsole::default(); + + let rome_json = r#"{ + "linter": { + "enabled": true, + "rules": { + "recommended": true + } + }, + "organizeImports": { + "enabled": true + } +}"#; + + let input = r#" +import { B, C } from "b.js" +import A from "a.js" + + +something( ) + "#; + + let file_path = Path::new("rome.json"); + fs.insert(file_path.into(), rome_json.as_bytes()); + + let file_path = Path::new("file.js"); + fs.insert(file_path.into(), input.as_bytes()); + + let result = run_cli( + DynRef::Borrowed(&mut fs), + &mut console, + Arguments::from_vec(vec![OsString::from("ci"), file_path.as_os_str().into()]), + ); + + assert!(result.is_err(), "run_cli returned {result:?}"); + + let mut file = fs + .open(file_path) + .expect("ci target file was removed by the CLI"); + + let mut content = String::new(); + file.read_to_string(&mut content) + .expect("failed to read file from memory FS"); + + drop(file); + assert_cli_snapshot(SnapshotPayload::new( + module_path!(), + "ci_formatter_linter_organize_imports", + fs, + console, + result, + )); +} diff --git a/crates/rome_cli/tests/configs.rs b/crates/rome_cli/tests/configs.rs index b9af6a8e8fe..def817fd34c 100644 --- a/crates/rome_cli/tests/configs.rs +++ b/crates/rome_cli/tests/configs.rs @@ -10,7 +10,7 @@ pub const CONFIG_FORMAT: &str = r#"{ pub const CONFIG_INIT_DEFAULT: &str = r#"{ "organizeImports": { - "enabled": true + "enabled": false }, "linter": { "enabled": true, @@ -24,7 +24,7 @@ pub const CONFIG_INIT_DEFAULT: &str = r#"{ pub const CONFIG_INIT_DEFAULT_WHEN_INSTALLED: &str = r#"{ "$schema": "./node_modules/rome/configuration_schema.json", "organizeImports": { - "enabled": true + "enabled": false }, "linter": { "enabled": true, diff --git a/crates/rome_cli/tests/snapshots/main_commands_check/organize_imports.snap b/crates/rome_cli/tests/snapshots/main_commands_check/applies_organize_imports.snap similarity index 77% rename from crates/rome_cli/tests/snapshots/main_commands_check/organize_imports.snap rename to crates/rome_cli/tests/snapshots/main_commands_check/applies_organize_imports.snap index ecae1652bbb..86ef8a97f4a 100644 --- a/crates/rome_cli/tests/snapshots/main_commands_check/organize_imports.snap +++ b/crates/rome_cli/tests/snapshots/main_commands_check/applies_organize_imports.snap @@ -2,13 +2,18 @@ source: crates/rome_cli/tests/snap_test.rs expression: content --- +## `rome.json` + +```json +{ "organizeImports": { "enabled": true } } +``` + ## `check.js` ```js - import * as something from "../something"; import { bar, foom, lorem } from "foo"; - + ``` # Emitted Messages diff --git a/crates/rome_cli/tests/snapshots/main_commands_check/apply_suggested_with_error.snap b/crates/rome_cli/tests/snapshots/main_commands_check/apply_unsafe_with_error.snap similarity index 96% rename from crates/rome_cli/tests/snapshots/main_commands_check/apply_suggested_with_error.snap rename to crates/rome_cli/tests/snapshots/main_commands_check/apply_unsafe_with_error.snap index cd11f307b8e..22d2e0be7ea 100644 --- a/crates/rome_cli/tests/snapshots/main_commands_check/apply_suggested_with_error.snap +++ b/crates/rome_cli/tests/snapshots/main_commands_check/apply_unsafe_with_error.snap @@ -25,7 +25,7 @@ function f() { arguments; } ```block internalError/io ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - × Some errors were emitted while applying fixes + × Some errors were emitted while running checks diff --git a/crates/rome_cli/tests/snapshots/main_commands_check/dont_applies_organize_imports_for_ignored_file.snap b/crates/rome_cli/tests/snapshots/main_commands_check/dont_applies_organize_imports_for_ignored_file.snap new file mode 100644 index 00000000000..36706501f7e --- /dev/null +++ b/crates/rome_cli/tests/snapshots/main_commands_check/dont_applies_organize_imports_for_ignored_file.snap @@ -0,0 +1,25 @@ +--- +source: crates/rome_cli/tests/snap_test.rs +expression: content +--- +## `rome.json` + +```json +{ "organizeImports": { "enabled": true, "ignore": ["check.js"] } } +``` + +## `check.js` + +```js +import { lorem, foom, bar } from "foo"; +import * as something from "../something"; + +``` + +# Emitted Messages + +```block +Fixed 1 file(s) in