Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(linter): do not bail for unmatched rules yet #7093

Merged
merged 1 commit into from
Nov 3, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 5 additions & 28 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, Error, GraphicalReportHandler, OxcDiagnostic};
use oxc_diagnostics::{DiagnosticService, GraphicalReportHandler};
use oxc_linter::{
loader::LINT_PARTIAL_LOADER_EXT, AllowWarnDeny, InvalidFilterKind, LintFilter, LintService,
LintServiceOptions, Linter, LinterBuilder, LinterBuilderError, Oxlintrc,
LintServiceOptions, Linter, LinterBuilder, Oxlintrc,
};
use oxc_span::VALID_EXTENSIONS;

Expand Down Expand Up @@ -119,21 +119,9 @@ 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);
// 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}"
)));
return CliRunResult::LintError { error: format!("{error:?}") };
}
},
};
let builder = builder.with_filters(filter).with_fix(fix_options.fix_kind());
let builder = LinterBuilder::from_oxlintrc(false, oxlintrc)
.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 @@ -498,12 +486,7 @@ 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",
Expand Down Expand Up @@ -560,13 +543,7 @@ 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]
#[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",
Expand Down
2 changes: 0 additions & 2 deletions crates/oxc_language_server/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -359,8 +359,6 @@ 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
26 changes: 16 additions & 10 deletions crates/oxc_linter/src/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ use std::{
fmt,
};

use oxc_diagnostics::{Error, OxcDiagnostic};
use oxc_span::CompactStr;
use rustc_hash::FxHashSet;

Expand Down Expand Up @@ -80,10 +81,7 @@ impl LinterBuilder {
/// 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> {
pub fn from_oxlintrc(start_empty: bool, oxlintrc: Oxlintrc) -> Self {
// TODO: monorepo config merging, plugin-based extends, etc.
let Oxlintrc { plugins, settings, env, globals, categories, rules: mut oxlintrc_rules } =
oxlintrc;
Expand All @@ -104,13 +102,21 @@ impl LinterBuilder {
oxlintrc_rules.override_rules(&mut builder.rules, all_rules.as_slice());
}

#[expect(clippy::print_stderr)]
if !oxlintrc_rules.unknown_rules.is_empty() {
return Err(LinterBuilderError::UnknownRules {
rules: std::mem::take(&mut oxlintrc_rules.unknown_rules),
});
let rules = oxlintrc_rules
.unknown_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}"
)));
eprintln!("{error:?}");
}

Ok(builder)
builder
}

#[inline]
Expand Down Expand Up @@ -311,7 +317,7 @@ impl TryFrom<Oxlintrc> for LinterBuilder {

#[inline]
fn try_from(oxlintrc: Oxlintrc) -> Result<Self, Self::Error> {
Self::from_oxlintrc(false, oxlintrc)
Ok(Self::from_oxlintrc(false, oxlintrc))
}
}

Expand Down Expand Up @@ -643,7 +649,7 @@ mod test {
"#,
)
.unwrap();
let builder = LinterBuilder::from_oxlintrc(false, oxlintrc).unwrap();
let builder = LinterBuilder::from_oxlintrc(false, oxlintrc);
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/tester.rs
Original file line number Diff line number Diff line change
Expand Up @@ -432,7 +432,7 @@ impl Tester {
let linter = eslint_config
.as_ref()
.map_or_else(LinterBuilder::empty, |v| {
LinterBuilder::from_oxlintrc(true, Oxlintrc::deserialize(v).unwrap()).unwrap()
LinterBuilder::from_oxlintrc(true, Oxlintrc::deserialize(v).unwrap())
})
.with_fix(fix.into())
.with_plugins(self.plugins)
Expand Down
Loading