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): support suggestions and dangerous fixes #4223

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
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

36 changes: 35 additions & 1 deletion apps/oxlint/src/command/lint.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use std::{path::PathBuf, str::FromStr};

use bpaf::Bpaf;
use oxc_linter::AllowWarnDeny;
use oxc_linter::{AllowWarnDeny, FixKind};

use super::{
expand_glob,
Expand Down Expand Up @@ -122,6 +122,40 @@ pub struct FixOptions {
/// Fix as many issues as possible. Only unfixed issues are reported in the output
#[bpaf(switch)]
pub fix: bool,
/// Apply auto-fixable suggestions. May change program behavior.
#[bpaf(switch)]
pub fix_suggestions: bool,
DonIsaac marked this conversation as resolved.
Show resolved Hide resolved

/// Apply dangerous fixes and suggestions.
#[bpaf(switch)]
pub fix_dangerously: bool,
}

impl FixOptions {
pub fn fix_kind(&self) -> FixKind {
let mut kind = FixKind::None;

if self.fix {
kind.set(FixKind::SafeFix, true);
}

if self.fix_suggestions {
kind.set(FixKind::Suggestion, true);
}

if self.fix_dangerously {
if kind.is_none() {
kind.set(FixKind::Fix, true);
}
kind.set(FixKind::Dangerous, true);
}

kind
}

pub fn is_enabled(&self) -> bool {
self.fix || self.fix_suggestions || self.fix_dangerously
}
}

/// Handle Warnings
Expand Down
2 changes: 1 addition & 1 deletion apps/oxlint/src/lint/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ impl Runner for LintRunner {
let lint_options = LintOptions::default()
.with_filter(filter)
.with_config_path(basic_options.config)
.with_fix(fix_options.fix)
.with_fix(fix_options.fix_kind())
.with_react_plugin(enable_plugins.react_plugin)
.with_unicorn_plugin(enable_plugins.unicorn_plugin)
.with_typescript_plugin(enable_plugins.typescript_plugin)
Expand Down
4 changes: 2 additions & 2 deletions crates/oxc_language_server/src/linter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ use oxc_linter::{
AstroPartialLoader, JavaScriptSource, SveltePartialLoader, VuePartialLoader,
LINT_PARTIAL_LOADER_EXT,
},
LintContext, Linter,
FixKind, LintContext, Linter,
};
use oxc_parser::Parser;
use oxc_semantic::SemanticBuilder;
Expand Down Expand Up @@ -388,7 +388,7 @@ pub struct ServerLinter {

impl ServerLinter {
pub fn new() -> Self {
let linter = Linter::default().with_fix(true);
let linter = Linter::default().with_fix(FixKind::SafeFix);
Self { linter: Arc::new(linter) }
}

Expand Down
6 changes: 4 additions & 2 deletions crates/oxc_language_server/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use futures::future::join_all;
use globset::Glob;
use ignore::gitignore::Gitignore;
use log::{debug, error, info};
use oxc_linter::{LintOptions, Linter};
use oxc_linter::{FixKind, LintOptions, Linter};
use serde::{Deserialize, Serialize};
use tokio::sync::{Mutex, OnceCell, RwLock, SetError};
use tower_lsp::{
Expand Down Expand Up @@ -345,7 +345,9 @@ impl Backend {
let mut linter = self.server_linter.write().await;
*linter = ServerLinter::new_with_linter(
Linter::from_options(
LintOptions::default().with_fix(true).with_config_path(Some(config_path)),
LintOptions::default()
.with_fix(FixKind::SafeFix)
.with_config_path(Some(config_path)),
)
.expect("should have initialized linter with new options"),
);
Expand Down
1 change: 1 addition & 0 deletions crates/oxc_linter/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ oxc_codegen = { workspace = true }
oxc_resolver = { workspace = true }

rayon = { workspace = true }
bitflags = { workspace = true }
lazy_static = { workspace = true }
serde_json = { workspace = true }
serde = { workspace = true, features = ["derive"] }
Expand Down
115 changes: 103 additions & 12 deletions crates/oxc_linter/src/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ use oxc_syntax::module_record::ModuleRecord;
use crate::{
config::OxlintRules,
disable_directives::{DisableDirectives, DisableDirectivesBuilder},
fixer::{CompositeFix, Message, RuleFixer},
fixer::{FixKind, Message, RuleFix, RuleFixer},
javascript_globals::GLOBALS,
AllowWarnDeny, OxlintConfig, OxlintEnv, OxlintGlobals, OxlintSettings,
};
Expand All @@ -26,10 +26,12 @@ pub struct LintContext<'a> {

disable_directives: Rc<DisableDirectives<'a>>,

/// Whether or not to apply code fixes during linting. Defaults to `false`.
/// Whether or not to apply code fixes during linting. Defaults to
/// [`FixKind::None`] (no fixing).
///
/// Set via the `--fix` CLI flag.
fix: bool,
/// Set via the `--fix`, `--fix-suggestions`, and `--fix-dangerously` CLI
/// flags.
fix: FixKind,

file_path: Rc<Path>,

Expand Down Expand Up @@ -69,7 +71,7 @@ impl<'a> LintContext<'a> {
semantic,
diagnostics: RefCell::new(Vec::with_capacity(DIAGNOSTICS_INITIAL_CAPACITY)),
disable_directives: Rc::new(disable_directives),
fix: false,
fix: FixKind::None,
file_path: file_path.into(),
eslint_config: Arc::new(OxlintConfig::default()),
current_rule_name: "",
Expand All @@ -79,7 +81,7 @@ impl<'a> LintContext<'a> {

/// Enable/disable automatic code fixes.
#[must_use]
pub fn with_fix(mut self, fix: bool) -> Self {
pub fn with_fix(mut self, fix: FixKind) -> Self {
self.fix = fix;
self
}
Expand Down Expand Up @@ -190,25 +192,114 @@ impl<'a> LintContext<'a> {
/// Report a lint rule violation.
///
/// Use [`LintContext::diagnostic_with_fix`] to provide an automatic fix.
#[inline]
pub fn diagnostic(&self, diagnostic: OxcDiagnostic) {
self.add_diagnostic(Message::new(diagnostic, None));
}

/// Report a lint rule violation and provide an automatic fix.
///
/// The second argument is a [closure] that takes a [`RuleFixer`] and
/// returns something that can turn into a [`CompositeFix`].
/// returns something that can turn into a `CompositeFix`.
///
/// Fixes created this way should not create parse errors or change the
/// semantics of the linted code. If your fix may change the code's
/// semantics, use [`LintContext::diagnostic_with_suggestion`] instead. If
/// your fix has the potential to create parse errors, use
/// [`LintContext::diagnostic_with_dangerous_fix`].
///
/// [closure]: <https://doc.rust-lang.org/book/ch13-01-closures.html>
#[inline]
pub fn diagnostic_with_fix<C, F>(&self, diagnostic: OxcDiagnostic, fix: F)
where
C: Into<CompositeFix<'a>>,
C: Into<RuleFix<'a>>,
F: FnOnce(RuleFixer<'_, 'a>) -> C,
{
self.diagnostic_with_fix_of_kind(diagnostic, FixKind::SafeFix, fix);
}

/// Report a lint rule violation and provide a suggestion for fixing it.
///
/// The second argument is a [closure] that takes a [`RuleFixer`] and
/// returns something that can turn into a `CompositeFix`.
///
/// Fixes created this way should not create parse errors, but have the
/// potential to change the code's semantics. If your fix is completely safe
/// and definitely does not change semantics, use [`LintContext::diagnostic_with_fix`].
/// If your fix has the potential to create parse errors, use
/// [`LintContext::diagnostic_with_dangerous_fix`].
///
/// [closure]: <https://doc.rust-lang.org/book/ch13-01-closures.html>
#[inline]
pub fn diagnostic_with_suggestion<C, F>(&self, diagnostic: OxcDiagnostic, fix: F)
where
C: Into<RuleFix<'a>>,
F: FnOnce(RuleFixer<'_, 'a>) -> C,
{
self.diagnostic_with_fix_of_kind(diagnostic, FixKind::Suggestion, fix);
}

/// Report a lint rule violation and provide a potentially dangerous
/// automatic fix for it.
///
/// The second argument is a [closure] that takes a [`RuleFixer`] and
/// returns something that can turn into a `CompositeFix`.
///
/// Dangerous fixes should be avoided and are not applied by default with
/// `--fix`. Use this method if:
/// - Your fix is experimental and you want to test it out in the wild
/// before marking it as safe.
/// - Your fix is extremely aggressive and risky, but you want to provide
/// it as an option to users.
///
/// When possible, prefer [`LintContext::diagnostic_with_fix`]. If the only
/// risk your fix poses is minor(ish) changes to code semantics, use
/// [`LintContext::diagnostic_with_suggestion`] instead.
///
/// [closure]: <https://doc.rust-lang.org/book/ch13-01-closures.html>
///
#[inline]
pub fn diagnostic_with_dangerous_fix<C, F>(&self, diagnostic: OxcDiagnostic, fix: F)
where
C: Into<RuleFix<'a>>,
F: FnOnce(RuleFixer<'_, 'a>) -> C,
{
self.diagnostic_with_fix_of_kind(diagnostic, FixKind::DangerousFix, fix);
}

pub fn diagnostic_with_fix_of_kind<C, F>(
&self,
diagnostic: OxcDiagnostic,
fix_kind: FixKind,
fix: F,
) where
C: Into<RuleFix<'a>>,
F: FnOnce(RuleFixer<'_, 'a>) -> C,
{
if self.fix {
let fixer = RuleFixer::new(self);
let composite_fix: CompositeFix = fix(fixer).into();
let fix = composite_fix.normalize_fixes(self.source_text());
// if let Some(accepted_fix_kind) = self.fix {
// let fixer = RuleFixer::new(fix_kind, self);
// let rule_fix: RuleFix<'a> = fix(fixer).into();
// let diagnostic = match (rule_fix.message(), &diagnostic.help) {
// (Some(message), None) => diagnostic.with_help(message.to_owned()),
// _ => diagnostic,
// };
// if rule_fix.kind() <= accepted_fix_kind {
// let fix = rule_fix.into_fix(self.source_text());
// self.add_diagnostic(Message::new(diagnostic, Some(fix)));
// } else {
// self.diagnostic(diagnostic);
// }
// } else {
// self.diagnostic(diagnostic);
// }
let fixer = RuleFixer::new(fix_kind, self);
let rule_fix: RuleFix<'a> = fix(fixer).into();
let diagnostic = match (rule_fix.message(), &diagnostic.help) {
(Some(message), None) => diagnostic.with_help(message.to_owned()),
_ => diagnostic,
};
if self.fix.can_apply(rule_fix.kind()) {
let fix = rule_fix.into_fix(self.source_text());
self.add_diagnostic(Message::new(diagnostic, Some(fix)));
} else {
self.diagnostic(diagnostic);
Expand Down
Loading
Loading