Skip to content

Commit

Permalink
feat(linter): report unmatched rules with error exit code
Browse files Browse the repository at this point in the history
  • Loading branch information
camchenry committed Oct 30, 2024
1 parent 76947e2 commit 30660e0
Show file tree
Hide file tree
Showing 8 changed files with 192 additions and 68 deletions.
53 changes: 40 additions & 13 deletions apps/oxlint/src/lint.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
use std::{env, io::BufWriter, time::Instant};

use ignore::gitignore::Gitignore;
use oxc_diagnostics::{DiagnosticService, GraphicalReportHandler};
use oxc_diagnostics::{DiagnosticService, Error, GraphicalReportHandler, OxcDiagnostic};
use oxc_linter::{
loader::LINT_PARTIAL_LOADER_EXT, AllowWarnDeny, InvalidFilterKind, LintFilter, LintService,
LintServiceOptions, Linter, LinterBuilder, Oxlintrc,
LintServiceOptions, Linter, LinterBuilder, LinterBuilderError, Oxlintrc,
};
use oxc_span::VALID_EXTENSIONS;

Expand Down Expand Up @@ -119,9 +119,24 @@ impl Runner for LintRunner {

let oxlintrc_for_print =
if misc_options.print_config { Some(oxlintrc.clone()) } else { None };
let builder = LinterBuilder::from_oxlintrc(false, oxlintrc)
.with_filters(filter)
.with_fix(fix_options.fix_kind());
let builder = LinterBuilder::from_oxlintrc(false, oxlintrc);
// Gracefully report any linter builder errors as CLI errors
let builder = match builder {
Ok(builder) => builder,
Err(err) => match err {
LinterBuilderError::UnknownRules { rules } => {
let rules = rules.iter().map(|r| r.full_name()).collect::<Vec<_>>().join("\n");
let error = Error::from(
OxcDiagnostic::warn(format!(
"The following rules do not match the currently supported rules:\n{rules}"
))
.with_help("Check that the plugin that contains this rule is enabled."),
);
return CliRunResult::LintError { error: format!("{error:?}") };
}
},
};
let builder = builder.with_filters(filter).with_fix(fix_options.fix_kind());

if let Some(basic_config_file) = oxlintrc_for_print {
return CliRunResult::PrintConfigResult {
Expand Down Expand Up @@ -244,6 +259,9 @@ mod test {
let options = lint_command().run_inner(new_args.as_slice()).unwrap();
match LintRunner::new(options).run() {
CliRunResult::LintResult(lint_result) => lint_result,
CliRunResult::LintError { error } => {
panic!("{error}")
}
other => panic!("{other:?}"),
}
}
Expand Down Expand Up @@ -483,18 +501,20 @@ mod test {
assert_eq!(result.number_of_errors, 0);
}

// Previously, this test would pass and the unmatched rule would be ignored, but now we report that
// there was unmatched rule, because the typescript plugin has been disabled and we are trying to configure it.
#[test]
#[should_panic(
expected = "The following rules do not match the currently supported rules:\n | typescript/no-namespace"
)]
fn typescript_eslint_off() {
let args = &[
"-c",
"fixtures/typescript_eslint/eslintrc.json",
"--disable-typescript-plugin",
"fixtures/typescript_eslint/test.ts",
];
let result = test(args);
assert_eq!(result.number_of_files, 1);
assert_eq!(result.number_of_warnings, 2);
assert_eq!(result.number_of_errors, 0);
test(args);
}

#[test]
Expand Down Expand Up @@ -543,17 +563,24 @@ mod test {
.contains("oxc/tsconfig.json\" does not exist, Please provide a valid tsconfig file."));
}

// Previously, we used to not report errors when enabling a rule that did not have the corresponding plugin enabled,
// but now this is reported as an unmatched rule.
#[test]
fn test_enable_vitest_plugin() {
#[should_panic(
// FIXME: We should probably report the original rule name error, not the mapped jest rule name?
expected = "The following rules do not match the currently supported rules:\n | jest/no-disabled-tests\n"
)]
fn test_enable_vitest_rule_without_plugin() {
let args = &[
"-c",
"fixtures/eslintrc_vitest_replace/eslintrc.json",
"fixtures/eslintrc_vitest_replace/foo.test.js",
];
let result = test(args);
assert_eq!(result.number_of_files, 1);
assert_eq!(result.number_of_errors, 0);
test(args);
}

#[test]
fn test_enable_vitest_plugin() {
let args = &[
"--vitest-plugin",
"-c",
Expand Down
37 changes: 32 additions & 5 deletions apps/oxlint/src/result.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,23 +7,46 @@ use std::{
#[derive(Debug)]
pub enum CliRunResult {
None,
InvalidOptions { message: String },
PathNotFound { paths: Vec<PathBuf> },
InvalidOptions {
message: String,
},
PathNotFound {
paths: Vec<PathBuf>,
},
/// Indicates that there was an error trying to run the linter and it was
/// not able to complete linting successfully.
LintError {
error: String,
},
LintResult(LintResult),
FormatResult(FormatResult),
TypeCheckResult { duration: Duration, number_of_diagnostics: usize },
PrintConfigResult { config_file: String },
TypeCheckResult {
duration: Duration,
number_of_diagnostics: usize,
},
PrintConfigResult {
config_file: String,
},
}

/// A summary of a complete linter run.
#[derive(Debug, Default)]
pub struct LintResult {
/// The total time it took to run the linter.
pub duration: Duration,
/// The number of lint rules that were run.
pub number_of_rules: usize,
/// The number of files that were linted.
pub number_of_files: usize,
/// The number of warnings that were found.
pub number_of_warnings: usize,
/// The number of errors that were found.
pub number_of_errors: usize,
/// Whether or not the maximum number of warnings was exceeded.
pub max_warnings_exceeded: bool,
/// Whether or not warnings should be treated as errors (from `--deny-warnings` for example)
pub deny_warnings: bool,
/// Whether or not to print a summary of the results
pub print_summary: bool,
}

Expand All @@ -34,7 +57,7 @@ pub struct FormatResult {
}

impl Termination for CliRunResult {
#[allow(clippy::print_stdout)]
#[allow(clippy::print_stdout, clippy::print_stderr)]
fn report(self) -> ExitCode {
match self {
Self::None => ExitCode::from(0),
Expand All @@ -46,6 +69,10 @@ impl Termination for CliRunResult {
println!("Path {paths:?} does not exist.");
ExitCode::from(1)
}
Self::LintError { error } => {
eprintln!("Error: {error}");
ExitCode::from(1)
}
Self::LintResult(LintResult {
duration,
number_of_rules,
Expand Down
2 changes: 2 additions & 0 deletions crates/oxc_language_server/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -359,6 +359,8 @@ impl Backend {
Oxlintrc::from_file(&config_path)
.expect("should have initialized linter with new options"),
)
// FIXME: Handle this error more gracefully and report it properly
.expect("failed to build linter from oxlint config")
.with_fix(FixKind::SafeFix)
.build(),
);
Expand Down
37 changes: 31 additions & 6 deletions crates/oxc_linter/src/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,13 @@ pub struct LinterBuilder {
cache: RulesCache,
}

/// An error that can occur while building a [`Linter`] from an [`Oxlintrc`].
#[derive(Debug, Clone)]
pub enum LinterBuilderError {
/// There were unknown rules that could not be matched to any known plugins/rules.
UnknownRules { rules: Vec<ESLintRule> },
}

impl Default for LinterBuilder {
fn default() -> Self {
Self { rules: Self::warn_correctness(LintPlugins::default()), ..Self::empty() }
Expand Down Expand Up @@ -74,9 +81,18 @@ impl LinterBuilder {
/// // you can use `From` as a shorthand for `from_oxlintrc(false, oxlintrc)`
/// let linter = LinterBuilder::from(oxlintrc).build();
/// ```
pub fn from_oxlintrc(start_empty: bool, oxlintrc: Oxlintrc) -> Self {
///
/// # Errors
///
/// Will return a [`LinterBuilderError::UnknownRules`] if there are unknown rules in the
/// config. This can happen if the plugin for a rule is not enabled, or the rule name doesn't
/// match any recognized rules.
pub fn from_oxlintrc(
start_empty: bool,
oxlintrc: Oxlintrc,
) -> Result<Self, LinterBuilderError> {
// TODO: monorepo config merging, plugin-based extends, etc.
let Oxlintrc { plugins, settings, env, globals, categories, rules: oxlintrc_rules } =
let Oxlintrc { plugins, settings, env, globals, categories, rules: mut oxlintrc_rules } =
oxlintrc;

let config = LintConfig { plugins, settings, env, globals };
Expand All @@ -95,7 +111,13 @@ impl LinterBuilder {
oxlintrc_rules.override_rules(&mut builder.rules, all_rules.as_slice());
}

builder
if !oxlintrc_rules.unknown_rules.is_empty() {
return Err(LinterBuilderError::UnknownRules {
rules: std::mem::take(&mut oxlintrc_rules.unknown_rules),
});
}

Ok(builder)
}

#[inline]
Expand Down Expand Up @@ -258,6 +280,7 @@ impl LinterBuilder {
let previous_rules = std::mem::take(&mut oxlintrc.rules);

let rule_name_to_rule = previous_rules
.rules
.into_iter()
.map(|r| (get_name(&r.plugin_name, &r.rule_name), r))
.collect::<rustc_hash::FxHashMap<_, _>>();
Expand Down Expand Up @@ -290,9 +313,11 @@ fn get_name(plugin_name: &str, rule_name: &str) -> CompactStr {
}
}

impl From<Oxlintrc> for LinterBuilder {
impl TryFrom<Oxlintrc> for LinterBuilder {
type Error = LinterBuilderError;

#[inline]
fn from(oxlintrc: Oxlintrc) -> Self {
fn try_from(oxlintrc: Oxlintrc) -> Result<Self, Self::Error> {
Self::from_oxlintrc(false, oxlintrc)
}
}
Expand Down Expand Up @@ -602,7 +627,7 @@ mod test {
"#,
)
.unwrap();
let builder = LinterBuilder::from_oxlintrc(false, oxlintrc);
let builder = LinterBuilder::from_oxlintrc(false, oxlintrc).unwrap();
for rule in &builder.rules {
let name = rule.name();
let plugin = rule.plugin_name();
Expand Down
2 changes: 1 addition & 1 deletion crates/oxc_linter/src/config/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ mod test {
fn test_vitest_rule_replace() {
let fixture_path: std::path::PathBuf =
env::current_dir().unwrap().join("fixtures/eslint_config_vitest_replace.json");
let config = Oxlintrc::from_file(&fixture_path).unwrap();
let mut config = Oxlintrc::from_file(&fixture_path).unwrap();
let mut set = FxHashSet::default();
config.rules.override_rules(&mut set, &RULES);

Expand Down
Loading

0 comments on commit 30660e0

Please sign in to comment.