Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

Commit

Permalink
fix rule applicability and added tests
Browse files Browse the repository at this point in the history
  • Loading branch information
ematipico committed Mar 10, 2023
1 parent b6a23c9 commit af964e2
Show file tree
Hide file tree
Showing 16 changed files with 215 additions and 129 deletions.
38 changes: 33 additions & 5 deletions crates/rome_cli/src/diagnostics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -169,7 +173,7 @@ impl Display for CheckActionKind {
"Some errors were emitted while "<Emphasis>"running checks"</Emphasis>
}),
CheckActionKind::Apply => fmt.write_markup(markup! {
<Emphasis>"Fixes applied"</Emphasis>", but there are still diagnostics to be addressed"
"Some errors were emitted while "<Emphasis>"applying fixes"</Emphasis>
}),
}
}
Expand All @@ -182,15 +186,23 @@ impl std::fmt::Display for CheckActionKind {
write!(f, "Some errors were emitted while running checks")
}
CheckActionKind::Apply => {
write!(
f,
"Fixes applied but there are still diagnostics to be addressed"
)
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 FileCheckApply {
#[location(resource)]
pub file_path: String,
}

#[derive(Debug, Diagnostic)]
#[diagnostic(
category = "flags/invalid",
Expand Down Expand Up @@ -354,6 +366,13 @@ impl CliDiagnostic {
})
}

/// 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 {
Self::FileCheckApply(FileCheckApply {
file_path: file_path.into(),
})
}

/// Emitted when the server is not running
pub fn server_not_running() -> Self {
Self::ServerNotRunning(ServerNotRunning)
Expand Down Expand Up @@ -402,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(),
}
}

Expand All @@ -421,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(),
}
}

Expand All @@ -440,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(),
}
}

Expand All @@ -459,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(),
}
}

Expand All @@ -478,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),
}
}

Expand All @@ -497,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),
}
}

Expand All @@ -516,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),
}
}

Expand All @@ -539,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),
}
}

Expand All @@ -558,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(),
}
}
}
Expand Down
7 changes: 7 additions & 0 deletions crates/rome_cli/src/execute.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 { .. })
}
Expand Down
32 changes: 29 additions & 3 deletions crates/rome_cli/src/traversal.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(())
}
Expand Down Expand Up @@ -371,6 +375,27 @@ 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() {
if 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() {
Expand Down Expand Up @@ -828,8 +853,8 @@ fn process_file(ctx: &TraversalOptions, path: &Path) -> FileResult {
return if fixed.errors == 0 {
Ok(FileStatus::Success)
} else {
Ok(FileStatus::Message(Message::Error(
CliDiagnostic::apply_error().into(),
Ok(FileStatus::Message(Message::ApplyError(
CliDiagnostic::file_apply_error(path.display().to_string()).into(),
)))
};
}
Expand Down Expand Up @@ -948,6 +973,7 @@ enum Message {
/// Suggested fixes skipped during the lint traversal
skipped_suggested_fixes: u32,
},
ApplyError(CliDiagnostic),
Error(Error),
Diagnostics {
name: String,
Expand Down
90 changes: 71 additions & 19 deletions crates/rome_cli/tests/commands/check.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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) {}"#;
Expand Down Expand Up @@ -253,8 +240,6 @@ fn apply_noop() {
]),
);

println!("{console:#?}");

assert!(result.is_ok(), "run_cli returned {result:?}");

assert_cli_snapshot(SnapshotPayload::new(
Expand Down Expand Up @@ -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.clone().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();
Expand Down Expand Up @@ -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(
Expand All @@ -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!(),
Expand Down
2 changes: 1 addition & 1 deletion crates/rome_cli/tests/configs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ pub const CONFIG_LINTER_SUPPRESSED_GROUP: &str = r#"{
"linter": {
"rules": {
"recommended": true,
"correctness": {
"suspicious": {
"recommended": false
}
}
Expand Down
Loading

0 comments on commit af964e2

Please sign in to comment.