diff --git a/crates/rome_cli/src/diagnostics.rs b/crates/rome_cli/src/diagnostics.rs index 8527188653c..b50c7caebdd 100644 --- a/crates/rome_cli/src/diagnostics.rs +++ b/crates/rome_cli/src/diagnostics.rs @@ -1,4 +1,4 @@ -use rome_console::fmt::Formatter; +use rome_console::fmt::{Display, Formatter}; use rome_console::markup; use rome_diagnostics::adapters::{IoError, PicoArgsError}; use rome_diagnostics::{ @@ -36,6 +36,10 @@ pub enum CliDiagnostic { IncompatibleArguments(IncompatibleArguments), /// Returned by a traversal command when error diagnostics were emitted CheckError(CheckError), + /// Emitted when a file is fixed, but it still contains diagnostics. + /// + /// This happens when these diagnostics come from rules that don't have a code action. + FileCheckApply(FileCheckApply), /// When an argument is higher than the expected maximum OverflowNumberArgument(OverflowNumberArgument), /// Wrapper for an underlying `rome_service` error @@ -147,9 +151,57 @@ pub struct IncompatibleArguments { #[diagnostic( category = "internalError/io", severity = Error, - message = "Some errors were emitted while running checks" + message( + description = "{action_kind}", + message({{self.action_kind}}) + ) +)] +pub struct CheckError { + action_kind: CheckActionKind, +} + +#[derive(Debug, Clone, Copy)] +pub enum CheckActionKind { + Check, + Apply, +} + +impl Display for CheckActionKind { + fn fmt(&self, fmt: &mut Formatter) -> std::io::Result<()> { + match self { + CheckActionKind::Check => fmt.write_markup(markup! { + "Some errors were emitted while ""running checks" + }), + CheckActionKind::Apply => fmt.write_markup(markup! { + "Some errors were emitted while ""applying fixes" + }), + } + } +} + +impl std::fmt::Display for CheckActionKind { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + CheckActionKind::Check => { + write!(f, "Some errors were emitted while running checks") + } + CheckActionKind::Apply => { + write!(f, "Some errors were emitted while applying fixes") + } + } + } +} + +#[derive(Debug, Diagnostic)] +#[diagnostic( + category = "internalError/io", + severity = Error, + message = "Fixes applied to the file, but there a still diagnostics to address." )] -pub struct CheckError; +pub struct FileCheckApply { + #[location(resource)] + pub file_path: String, +} #[derive(Debug, Diagnostic)] #[diagnostic( @@ -300,9 +352,25 @@ impl CliDiagnostic { }) } - /// Emitted when errors were emitted while traversing the file system + /// Emitted when errors were emitted while running `check` command pub fn check_error() -> Self { - Self::CheckError(CheckError) + Self::CheckError(CheckError { + action_kind: CheckActionKind::Check, + }) + } + + /// Emitted when errors were emitted while apply code fixes + pub fn apply_error() -> Self { + Self::CheckError(CheckError { + action_kind: CheckActionKind::Apply, + }) + } + + /// Emitted for a file that has code fixes, but still has diagnostics to address + pub fn file_apply_error(file_path: impl Into) -> Self { + Self::FileCheckApply(FileCheckApply { + file_path: file_path.into(), + }) } /// Emitted when the server is not running @@ -353,6 +421,7 @@ impl Diagnostic for CliDiagnostic { CliDiagnostic::ServerNotRunning(diagnostic) => diagnostic.category(), CliDiagnostic::IncompatibleEndConfiguration(diagnostic) => diagnostic.category(), CliDiagnostic::NoFilesWereProcessed(diagnostic) => diagnostic.category(), + CliDiagnostic::FileCheckApply(diagnostic) => diagnostic.category(), } } @@ -372,6 +441,7 @@ impl Diagnostic for CliDiagnostic { CliDiagnostic::ServerNotRunning(diagnostic) => diagnostic.tags(), CliDiagnostic::IncompatibleEndConfiguration(diagnostic) => diagnostic.tags(), CliDiagnostic::NoFilesWereProcessed(diagnostic) => diagnostic.tags(), + CliDiagnostic::FileCheckApply(diagnostic) => diagnostic.tags(), } } @@ -391,6 +461,7 @@ impl Diagnostic for CliDiagnostic { CliDiagnostic::ServerNotRunning(diagnostic) => diagnostic.severity(), CliDiagnostic::IncompatibleEndConfiguration(diagnostic) => diagnostic.severity(), CliDiagnostic::NoFilesWereProcessed(diagnostic) => diagnostic.severity(), + CliDiagnostic::FileCheckApply(diagnostic) => diagnostic.severity(), } } @@ -410,6 +481,7 @@ impl Diagnostic for CliDiagnostic { CliDiagnostic::ServerNotRunning(diagnostic) => diagnostic.location(), CliDiagnostic::IncompatibleEndConfiguration(diagnostic) => diagnostic.location(), CliDiagnostic::NoFilesWereProcessed(diagnostic) => diagnostic.location(), + CliDiagnostic::FileCheckApply(diagnostic) => diagnostic.location(), } } @@ -429,6 +501,7 @@ impl Diagnostic for CliDiagnostic { CliDiagnostic::ServerNotRunning(diagnostic) => diagnostic.message(fmt), CliDiagnostic::IncompatibleEndConfiguration(diagnostic) => diagnostic.message(fmt), CliDiagnostic::NoFilesWereProcessed(diagnostic) => diagnostic.message(fmt), + CliDiagnostic::FileCheckApply(diagnostic) => diagnostic.message(fmt), } } @@ -448,6 +521,7 @@ impl Diagnostic for CliDiagnostic { CliDiagnostic::ServerNotRunning(diagnostic) => diagnostic.description(fmt), CliDiagnostic::IncompatibleEndConfiguration(diagnostic) => diagnostic.description(fmt), CliDiagnostic::NoFilesWereProcessed(diagnostic) => diagnostic.description(fmt), + CliDiagnostic::FileCheckApply(diagnostic) => diagnostic.description(fmt), } } @@ -467,6 +541,7 @@ impl Diagnostic for CliDiagnostic { CliDiagnostic::ServerNotRunning(diagnostic) => diagnostic.advices(visitor), CliDiagnostic::IncompatibleEndConfiguration(diagnostic) => diagnostic.advices(visitor), CliDiagnostic::NoFilesWereProcessed(diagnostic) => diagnostic.advices(visitor), + CliDiagnostic::FileCheckApply(diagnostic) => diagnostic.advices(visitor), } } @@ -490,6 +565,7 @@ impl Diagnostic for CliDiagnostic { diagnostic.verbose_advices(visitor) } CliDiagnostic::NoFilesWereProcessed(diagnostic) => diagnostic.verbose_advices(visitor), + CliDiagnostic::FileCheckApply(diagnostic) => diagnostic.verbose_advices(visitor), } } @@ -509,6 +585,7 @@ impl Diagnostic for CliDiagnostic { CliDiagnostic::ServerNotRunning(diagnostic) => diagnostic.source(), CliDiagnostic::IncompatibleEndConfiguration(diagnostic) => diagnostic.source(), CliDiagnostic::NoFilesWereProcessed(diagnostic) => diagnostic.source(), + CliDiagnostic::FileCheckApply(diagnostic) => diagnostic.source(), } } } diff --git a/crates/rome_cli/src/execute.rs b/crates/rome_cli/src/execute.rs index a5deb08fe27..f55daa30b8a 100644 --- a/crates/rome_cli/src/execute.rs +++ b/crates/rome_cli/src/execute.rs @@ -111,6 +111,13 @@ impl Execution { matches!(self.traversal_mode, TraversalMode::Check { .. }) } + pub(crate) const fn is_check_apply(&self) -> bool { + match self.traversal_mode { + TraversalMode::Check { fix_file_mode } => fix_file_mode.is_some(), + _ => false, + } + } + pub(crate) const fn is_format(&self) -> bool { matches!(self.traversal_mode, TraversalMode::Format { .. }) } diff --git a/crates/rome_cli/src/traversal.rs b/crates/rome_cli/src/traversal.rs index e9b370899ca..327dc8a3bc2 100644 --- a/crates/rome_cli/src/traversal.rs +++ b/crates/rome_cli/src/traversal.rs @@ -208,7 +208,11 @@ pub(crate) fn traverse(execution: Execution, mut session: CliSession) -> Result< if count.saturating_sub(skipped) == 0 { Err(CliDiagnostic::no_files_processed()) } else if errors > 0 { - Err(CliDiagnostic::check_error()) + if execution.is_check_apply() { + Err(CliDiagnostic::apply_error()) + } else { + Err(CliDiagnostic::check_error()) + } } else { Ok(()) } @@ -371,6 +375,25 @@ fn process_messages(options: ProcessMessagesOptions) { total_skipped_suggested_fixes += skipped_suggested_fixes; } + Message::ApplyError(error) => { + *errors += 1; + let should_print = printed_diagnostics < max_diagnostics; + if should_print { + printed_diagnostics += 1; + remaining_diagnostics.store( + max_diagnostics.saturating_sub(printed_diagnostics), + Ordering::Relaxed, + ); + } else { + not_printed_diagnostics += 1; + } + if mode.should_report_to_terminal() && should_print { + console.error(markup! { + {if verbose { PrintDiagnostic::verbose(&error) } else { PrintDiagnostic::simple(&error) }} + }); + } + } + Message::Error(mut err) => { let location = err.location(); if let Some(Resource::File(file_path)) = location.resource.as_ref() { @@ -825,7 +848,13 @@ fn process_file(ctx: &TraversalOptions, path: &Path) -> FileResult { .with_file_path(path.display().to_string())?; } - return Ok(FileStatus::Success); + 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() { @@ -942,6 +971,7 @@ enum Message { /// Suggested fixes skipped during the lint traversal skipped_suggested_fixes: u32, }, + ApplyError(CliDiagnostic), Error(Error), Diagnostics { name: String, diff --git a/crates/rome_cli/tests/commands/check.rs b/crates/rome_cli/tests/commands/check.rs index 9b3685abfd6..c3c430c334e 100644 --- a/crates/rome_cli/tests/commands/check.rs +++ b/crates/rome_cli/tests/commands/check.rs @@ -36,10 +36,10 @@ const NO_DEBUGGER: &str = "debugger;"; const NEW_SYMBOL: &str = "new Symbol(\"\");"; const FIX_BEFORE: &str = " -if(a != -0) {} +(1 >= -0) "; const FIX_AFTER: &str = " -if(a != 0) {} +(1 >= 0) "; const APPLY_SUGGESTED_BEFORE: &str = "let a = 4; @@ -52,19 +52,6 @@ const APPLY_SUGGESTED_AFTER: &str = "const a = 4;\nconsole.log(a);\n"; const NO_DEBUGGER_BEFORE: &str = "debugger;"; const NO_DEBUGGER_AFTER: &str = "debugger;"; -const JS_ERRORS_BEFORE: &str = r#"try { - !a && !b; -} catch (err) { - err = 24; -} -"#; -const JS_ERRORS_AFTER: &str = "try { - !a && !b; -} catch (err) { - err = 24; -} -"; - const UPGRADE_SEVERITY_CODE: &str = r#"if(!cond) { exprA(); } else { exprB() }"#; const NURSERY_UNSTABLE: &str = r#"if(a = b) {}"#; @@ -253,8 +240,6 @@ fn apply_noop() { ]), ); - println!("{console:#?}"); - assert!(result.is_ok(), "run_cli returned {result:?}"); assert_cli_snapshot(SnapshotPayload::new( @@ -333,6 +318,73 @@ fn apply_suggested() { )); } +#[test] +fn apply_suggested_with_error() { + let mut fs = MemoryFileSystem::default(); + let mut console = BufferConsole::default(); + + // last line doesn't have code fix + let source = "let a = 4; +debugger; +console.log(a); +function f() { arguments; } +"; + + let expected = "const a = 4; +console.log(a); +function f() { arguments; } +"; + + let test1 = Path::new("test1.js"); + fs.insert(test1.into(), source.as_bytes()); + + let test2 = Path::new("test2.js"); + fs.insert(test2.into(), source.as_bytes()); + + let result = run_cli( + DynRef::Borrowed(&mut fs), + &mut console, + Arguments::from_vec(vec![ + OsString::from("check"), + OsString::from("--apply-unsafe"), + test1.as_os_str().into(), + test2.as_os_str().into(), + ]), + ); + + assert!(result.is_err(), "run_cli returned {result:?}"); + + let mut file = fs + .open(test1) + .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, expected); + drop(file); + + content.clear(); + + let mut file = fs + .open(test2) + .expect("formatting target file was removed by the CLI"); + + file.read_to_string(&mut content) + .expect("failed to read file from memory FS"); + + drop(file); + + assert_cli_snapshot(SnapshotPayload::new( + module_path!(), + "apply_suggested_with_error", + fs, + console, + result, + )); +} + #[test] fn no_lint_if_linter_is_disabled_when_run_apply() { let mut fs = MemoryFileSystem::default(); @@ -455,7 +507,7 @@ fn should_disable_a_rule_group() { let mut console = BufferConsole::default(); let file_path = Path::new("fix.js"); - fs.insert(file_path.into(), JS_ERRORS_BEFORE.as_bytes()); + fs.insert(file_path.into(), FIX_BEFORE.as_bytes()); let config_path = Path::new("rome.json"); fs.insert( @@ -481,7 +533,7 @@ fn should_disable_a_rule_group() { .read_to_string(&mut buffer) .unwrap(); - assert_eq!(buffer, JS_ERRORS_AFTER); + assert_eq!(buffer, FIX_BEFORE); assert_cli_snapshot(SnapshotPayload::new( module_path!(), @@ -862,15 +914,15 @@ fn fs_error_unknown() { // // ├── rome.json // ├── hidden_nested -// │   └── test -// │   └── symlink_testcase1_2 -> hidden_testcase1 +// │ └── test +// │ └── symlink_testcase1_2 -> hidden_testcase1 // ├── hidden_testcase1 -// │   └── test -// │   └── test.js // ok +// │ └── test +// │ └── test.js // ok // ├── hidden_testcase2 -// │   ├── test1.ts // ignored -// │   ├── test2.ts // ignored -// │   └── test.js // ok +// │ ├── test1.ts // ignored +// │ ├── test2.ts // ignored +// │ └── test.js // ok // └── src // ├── symlink_testcase1_1 -> hidden_nested // └── symlink_testcase2 -> hidden_testcase2 @@ -951,7 +1003,7 @@ fn fs_files_ignore_symlink() { OsString::from("check"), OsString::from("--config-path"), OsString::from(root_path.clone()), - OsString::from("--apply"), + OsString::from("--apply-unsafe"), OsString::from(src_path), ]), ); diff --git a/crates/rome_cli/tests/configs.rs b/crates/rome_cli/tests/configs.rs index 2bfc6981beb..b9af6a8e8fe 100644 --- a/crates/rome_cli/tests/configs.rs +++ b/crates/rome_cli/tests/configs.rs @@ -131,7 +131,7 @@ pub const CONFIG_LINTER_SUPPRESSED_GROUP: &str = r#"{ "linter": { "rules": { "recommended": true, - "correctness": { + "suspicious": { "recommended": false } } diff --git a/crates/rome_cli/tests/snapshots/main_commands_check/all_rules.snap b/crates/rome_cli/tests/snapshots/main_commands_check/all_rules.snap index 4aaf1ce40a9..64677ce321f 100644 --- a/crates/rome_cli/tests/snapshots/main_commands_check/all_rules.snap +++ b/crates/rome_cli/tests/snapshots/main_commands_check/all_rules.snap @@ -16,7 +16,7 @@ expression: content ```js -if(a != -0) {} +(1 >= -0) ``` @@ -34,54 +34,18 @@ internalError/io ━━━━━━━━━━━━━━━━━━━━━ # Emitted Messages ```block -fix.js:2:4 lint/suspicious/noCompareNegZero FIXABLE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ +fix.js:2:2 lint/suspicious/noCompareNegZero FIXABLE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - × Do not use the != operator to compare against -0. + × Do not use the >= operator to compare against -0. - > 2 │ if(a != -0) {} - │ ^^^^^^^ + > 2 │ (1 >= -0) + │ ^^^^^^^ 3 │ i Safe fix: Replace -0 with 0 - 2 │ if(a·!=·-0)·{} - │ - - -``` - -```block -fix.js:2:6 lint/suspicious/noDoubleEquals FIXABLE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - - × Use !== instead of != - - > 2 │ if(a != -0) {} - │ ^^ - 3 │ - - i != is only allowed when comparing against null - - > 2 │ if(a != -0) {} - │ ^^ - 3 │ - - i Using !== may be unsafe if you are relying on type coercion - - i Suggested fix: Use !== - - 2 │ if(a·!==·-0)·{} - │ + - -``` - -```block -fix.js:2:4 lint/correctness/noUndeclaredVariables ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - - ! The a variable is undeclared - - > 2 │ if(a != -0) {} - │ ^ - 3 │ - + 2 │ (1·>=·-0) + │ - ``` diff --git a/crates/rome_cli/tests/snapshots/main_commands_check/apply_noop.snap b/crates/rome_cli/tests/snapshots/main_commands_check/apply_noop.snap index f9ba2ba534a..4a27cf38a56 100644 --- a/crates/rome_cli/tests/snapshots/main_commands_check/apply_noop.snap +++ b/crates/rome_cli/tests/snapshots/main_commands_check/apply_noop.snap @@ -6,18 +6,12 @@ expression: content ```js -if(a != 0) {} +(1 >= 0) ``` # Emitted Messages -```block -Skipped 1 suggested fixes. -If you wish to apply the suggested (unsafe) fixes, use the command rome check --apply-unsafe - -``` - ```block Fixed 1 file(s) in