feat(rome_cli): show diagnostics even when --apply-unsafe is applied (#…

…4668)

* feat(rome_cli): show diagnostics even when --apply-unsafe is applied

* fix regression

.editorconfig

@@ -10,9 +10,8 @@ indent_size = 2
 
 # YAML doesn't support hard tabs 🙃
 # Templates that will be weird with hard tabs in the website editor
-[{**.yml, **.md, **.rs, **.mdx, justfile}]
+[{**.yml,**.md,**.rs,**.mdx,justfile}]
 indent_style = space
 
 [*.rs]
-indent_style = space
 indent_size = 4

CHANGELOG.md

@@ -78,6 +78,12 @@ multiple files:
   The resolution of the files is file system based, Rome doesn't know how to
   resolve dependencies yet.
 
+- The commands `rome check` and `rome lint` now show the remaining diagnostics even when
+	`--apply-safe` or `--apply-unsafe` are passed.
+
+- Fix the commands `rome check` and `rome lint`, they won't exit with an error code
+if no error diagnostics are emitted.
+
 ### Editors
 
 #### Other changes
@@ -262,6 +268,29 @@ multiple files:
 
 ### Parser
 
+- Add support for decorators in class method parameters, example:
+
+	```js
+	class AppController {
+ 		get(@Param() id) {}
+ 		//	^^^^^^^^ new supported syntax
+ 	}
+	```
+
+	This syntax is only supported via configuration, because it's a non-standard
+	syntax.
+
+	```json
+	{
+		"//": "rome.json file",
+ 		"javascript": {
+ 			"parser": {
+				"unsafeParameterDecoratorsEnabled": true
+ 			}
+ 		}
+ 	}
+	```
+
 ### VSCode
 
 ### JavaScript APIs

crates/rome_cli/src/diagnostics.rs

@@ -152,7 +152,6 @@ pub struct IncompatibleArguments {
 
 #[derive(Debug, Diagnostic)]
 #[diagnostic(
-    category = "internalError/io",
     severity = Error,
     message(
         description = "{action_kind}",
@@ -161,6 +160,9 @@ pub struct IncompatibleArguments {
 )]
 pub struct CheckError {
     action_kind: CheckActionKind,
+
+    #[category]
+    category: &'static Category,
 }
 
 #[derive(Clone, Copy)]
@@ -203,13 +205,15 @@ impl std::fmt::Display for CheckActionKind {
 
 #[derive(Debug, Diagnostic)]
 #[diagnostic(
-    category = "internalError/io",
     severity = Error,
     message = "Fixes applied to the file, but there are still diagnostics to address."
 )]
 pub struct FileCheckApply {
     #[location(resource)]
     pub file_path: String,
+
+    #[category]
+    pub category: &'static Category,
 }
 
 #[derive(Debug, Diagnostic)]
@@ -400,23 +404,26 @@ impl CliDiagnostic {
     }
 
     /// Emitted when errors were emitted while running `check` command
-    pub fn check_error() -> Self {
+    pub fn check_error(category: &'static Category) -> Self {
         Self::CheckError(CheckError {
             action_kind: CheckActionKind::Check,
+            category,
         })
     }
 
     /// Emitted when errors were emitted while apply code fixes
-    pub fn apply_error() -> Self {
+    pub fn apply_error(category: &'static Category) -> Self {
         Self::CheckError(CheckError {
             action_kind: CheckActionKind::Apply,
+            category,
         })
     }
 
     /// Emitted for a file that has code fixes, but still has diagnostics to address
-    pub fn file_apply_error(file_path: impl Into<String>) -> Self {
+    pub fn file_apply_error(file_path: impl Into<String>, category: &'static Category) -> Self {
         Self::FileCheckApply(FileCheckApply {
             file_path: file_path.into(),
+            category,
         })
     }
 

crates/rome_cli/src/execute/migrate.rs

@@ -1,7 +1,7 @@
 use crate::execute::diagnostics::{ContentDiffAdvice, MigrateDiffDiagnostic};
 use crate::{CliDiagnostic, CliSession};
 use rome_console::{markup, ConsoleExt};
-use rome_diagnostics::PrintDiagnostic;
+use rome_diagnostics::{category, PrintDiagnostic};
 use rome_fs::OpenOptions;
 use rome_json_syntax::JsonRoot;
 use rome_migrate::{migrate_configuration, ControlFlow};
@@ -54,7 +54,7 @@ pub(crate) fn run(
                 if let Some((range, _)) = action.mutation.as_text_edits() {
                     tree = match JsonRoot::cast(action.mutation.commit()) {
                         Some(tree) => tree,
-                        None => return Err(CliDiagnostic::check_error()),
+                        None => return Err(CliDiagnostic::check_error(category!("migrate"))),
                     };
                     actions.push(FixAction {
                         rule_name: action

crates/rome_cli/src/execute/process_file.rs

@@ -221,8 +221,10 @@ pub(crate) fn process_file(ctx: &TraversalOptions, path: &Path) -> FileResult {
                 // the unsupported case should be handled already at this point
                 format(shared_context, path)
             }
-            TraversalMode::Check { .. } => check_file(shared_context, path, &file_features),
-            TraversalMode::CI => check_file(shared_context, path, &file_features),
+            TraversalMode::Check { .. } => {
+                check_file(shared_context, path, &file_features, category!("check"))
+            }
+            TraversalMode::CI => check_file(shared_context, path, &file_features, category!("ci")),
             TraversalMode::Migrate { .. } => {
                 unreachable!("The migration should not be called for this file")
             }

crates/rome_cli/src/execute/process_file/check.rs

@@ -4,13 +4,15 @@ use crate::execute::process_file::organize_imports::organize_imports_with_guard;
 use crate::execute::process_file::workspace_file::WorkspaceFile;
 use crate::execute::process_file::{FileResult, FileStatus, Message, SharedTraversalOptions};
 use crate::CliDiagnostic;
+use rome_diagnostics::Category;
 use rome_service::workspace::{FeatureName, FileFeaturesResult};
 use std::path::Path;
 
 pub(crate) fn check_file<'ctx>(
     ctx: &'ctx SharedTraversalOptions<'ctx, '_>,
     path: &Path,
     file_features: &'ctx FileFeaturesResult,
+    category: &'static Category,
 ) -> FileResult {
     let mut has_errors = false;
     let mut workspace_file = WorkspaceFile::new(ctx, path)?;
@@ -71,11 +73,11 @@ pub(crate) fn check_file<'ctx>(
     if has_errors {
         if ctx.execution.is_check_apply() || ctx.execution.is_check_apply_unsafe() {
             Ok(FileStatus::Message(Message::ApplyError(
-                CliDiagnostic::file_apply_error(path.display().to_string()),
+                CliDiagnostic::file_apply_error(path.display().to_string(), category),
             )))
         } else {
             Ok(FileStatus::Message(Message::ApplyError(
-                CliDiagnostic::check_error(),
+                CliDiagnostic::check_error(category),
             )))
         }
     } else {

crates/rome_cli/src/execute/process_file/format.rs

@@ -5,7 +5,7 @@ use crate::execute::process_file::{
 };
 use crate::execute::TraversalMode;
 use crate::FormatterReportFileDetail;
-use rome_diagnostics::{category, DiagnosticExt, Error};
+use rome_diagnostics::{category, DiagnosticExt};
 use rome_service::workspace::RuleCategories;
 use std::path::Path;
 use std::sync::atomic::Ordering;
@@ -42,23 +42,10 @@ pub(crate) fn format_with_guard<'ctx>(
         ),
     };
 
-    if diagnostics_result.errors > 0 {
-        return Err(if ignore_errors {
-            Message::from(
-                SkippedDiagnostic.with_file_path(workspace_file.path.display().to_string()),
-            )
-        } else {
-            Message::Diagnostics {
-                name: workspace_file.path.display().to_string(),
-                content: input,
-                diagnostics: diagnostics_result
-                    .diagnostics
-                    .into_iter()
-                    .map(Error::from)
-                    .collect(),
-                skipped_diagnostics: diagnostics_result.skipped_diagnostics,
-            }
-        });
+    if diagnostics_result.errors > 0 && ignore_errors {
+        return Err(Message::from(
+            SkippedDiagnostic.with_file_path(workspace_file.path.display().to_string()),
+        ));
     }
 
     let printed = workspace_file

crates/rome_cli/src/execute/process_file/lint.rs

@@ -18,7 +18,7 @@ pub(crate) fn lint_with_guard<'ctx>(
     workspace_file: &mut WorkspaceFile,
 ) -> FileResult {
     let mut errors = 0;
-    let input = workspace_file.input()?;
+    let mut input = workspace_file.input()?;
 
     if let Some(fix_mode) = ctx.execution.as_fix_file_mode() {
         let fixed = workspace_file
@@ -35,33 +35,42 @@ pub(crate) fn lint_with_guard<'ctx>(
 
         if fixed.code != input {
             workspace_file.update_file(fixed.code)?;
+            input = workspace_file.input()?;
         }
         errors = fixed.errors;
     }
 
     let max_diagnostics = ctx.remaining_diagnostics.load(Ordering::Relaxed);
-    let result = workspace_file
+    let pull_diagnostics_result = workspace_file
         .guard()
         .pull_diagnostics(RuleCategories::LINT, max_diagnostics.into())
         .with_file_path_and_code(workspace_file.path.display().to_string(), category!("lint"))?;
 
-    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 {
+    let no_diagnostics = pull_diagnostics_result.diagnostics.is_empty()
+        && pull_diagnostics_result.skipped_diagnostics == 0;
+    errors += pull_diagnostics_result.errors;
+
+    if !no_diagnostics {
+        ctx.push_message(Message::Diagnostics {
             name: workspace_file.path.display().to_string(),
             content: input,
-            diagnostics: result.diagnostics.into_iter().map(Error::from).collect(),
-            skipped_diagnostics: result.skipped_diagnostics,
-        })
-    };
+            diagnostics: pull_diagnostics_result
+                .diagnostics
+                .into_iter()
+                .map(Error::from)
+                .collect(),
+            skipped_diagnostics: pull_diagnostics_result.skipped_diagnostics,
+        });
+    }
 
     if errors > 0 {
-        return Ok(FileStatus::Message(Message::ApplyError(
-            CliDiagnostic::file_apply_error(workspace_file.path.display().to_string()),
-        )));
+        Ok(FileStatus::Message(Message::ApplyError(
+            CliDiagnostic::file_apply_error(
+                workspace_file.path.display().to_string(),
+                category!("lint"),
+            ),
+        )))
     } else {
-        Ok(result)
+        Ok(FileStatus::Success)
     }
 }

crates/rome_cli/src/execute/traverse.rs

@@ -205,10 +205,15 @@ pub(crate) fn traverse(
     if count.saturating_sub(skipped) == 0 && !cli_options.no_errors_on_unmatched {
         Err(CliDiagnostic::no_files_processed())
     } else if errors > 0 {
+        let category = if execution.is_ci() {
+            category!("ci")
+        } else {
+            category!("check")
+        };
         if execution.is_check_apply() {
-            Err(CliDiagnostic::apply_error())
+            Err(CliDiagnostic::apply_error(category))
         } else {
-            Err(CliDiagnostic::check_error())
+            Err(CliDiagnostic::check_error(category))
         }
     } else {
         Ok(())

crates/rome_cli/tests/commands/check.rs

@@ -191,10 +191,9 @@ fn maximum_diagnostics() {
         .filter(|m| m.level == LogLevel::Log)
         .any(|m| {
             let content = format!("{:?}", m.content);
-            dbg!(&content);
             content.contains("The number of diagnostics exceeds the number allowed by Rome")
                 && content.contains("Diagnostics not shown")
-                && content.contains("78")
+                && content.contains("79")
         }));
 
     assert_cli_snapshot(SnapshotPayload::new(
@@ -608,8 +607,6 @@ fn downgrade_severity() {
         Args::from([("check"), file_path.as_os_str().to_str().unwrap()].as_slice()),
     );
 
-    println!("{console:?}");
-
     assert!(result.is_err(), "run_cli returned {result:?}");
 
     let messages = &console.out_buffer;
@@ -1220,7 +1217,8 @@ fn max_diagnostics_default() {
                 || node.content.contains("useBlockStatements")
                 || node.content.contains("noConstantCondition")
                 || node.content.contains("format")
-                || node.content.contains("internalError/io")
+                || node.content.contains("lint")
+                || node.content.contains("check")
         });
 
         if is_diagnostic {
@@ -1278,6 +1276,7 @@ fn max_diagnostics() {
                 || node.content.contains("useBlockStatements")
                 || node.content.contains("noConstantCondition")
                 || node.content.contains("format")
+                || node.content.contains("lint")
                 || node.content.contains("Some errors were emitted while")
         });
 
@@ -2552,3 +2551,52 @@ fn doesnt_error_if_no_files_were_processed() {
         result,
     ));
 }
+
+#[test]
+fn should_pass_if_there_are_only_warnings() {
+    let mut console = BufferConsole::default();
+    let mut fs = MemoryFileSystem::default();
+
+    let file_path = Path::new("rome.json");
+    fs.insert(
+        file_path.into(),
+        r#"
+{
+  "linter": {
+    "rules": {
+        "recommended": true,
+        "suspicious": {
+            "noClassAssign": "warn"
+        }
+    }
+  }
+}
+        "#
+        .as_bytes(),
+    );
+
+    let file_path = Path::new("file.js");
+    fs.insert(
+        file_path.into(),
+        r#"class A {};
+A = 0;
+"#
+        .as_bytes(),
+    );
+
+    let result = run_cli(
+        DynRef::Borrowed(&mut fs),
+        &mut console,
+        Args::from([("check"), "--apply-unsafe", ("file.js")].as_slice()),
+    );
+
+    assert!(result.is_ok(), "run_cli returned {result:?}");
+
+    assert_cli_snapshot(SnapshotPayload::new(
+        module_path!(),
+        "should_pass_if_there_are_only_warnings",
+        fs,
+        console,
+        result,
+    ));
+}