From 9001234a7bbcd771264492699417e1089f398241 Mon Sep 17 00:00:00 2001 From: camc314 <18101008+camc314@users.noreply.github.com> Date: Wed, 3 Dec 2025 17:04:10 +0000 Subject: [PATCH] feat(linter): add fix support for tsgolint diagnostics (#16344) fixes https://github.com/oxc-project/oxc/issues/16330 --- apps/oxlint/fixtures/tsgolint_fix/fix.ts | 6 + .../fixtures/tsgolint_fix/tsconfig.json | 9 + apps/oxlint/src/lint.rs | 26 ++ apps/oxlint/src/tester.rs | 12 +- crates/oxc_linter/src/lint_runner.rs | 2 +- crates/oxc_linter/src/tsgolint.rs | 350 ++++++++++++------ 6 files changed, 281 insertions(+), 124 deletions(-) create mode 100644 apps/oxlint/fixtures/tsgolint_fix/fix.ts create mode 100644 apps/oxlint/fixtures/tsgolint_fix/tsconfig.json diff --git a/apps/oxlint/fixtures/tsgolint_fix/fix.ts b/apps/oxlint/fixtures/tsgolint_fix/fix.ts new file mode 100644 index 0000000000000..cd1059e4b9761 --- /dev/null +++ b/apps/oxlint/fixtures/tsgolint_fix/fix.ts @@ -0,0 +1,6 @@ +// This file has a fixable tsgolint error: no-unnecessary-type-assertion +// The type assertion `as string` is unnecessary because str is already a string +const str: string = 'hello'; +const redundant = str as string; + +export { redundant }; diff --git a/apps/oxlint/fixtures/tsgolint_fix/tsconfig.json b/apps/oxlint/fixtures/tsgolint_fix/tsconfig.json new file mode 100644 index 0000000000000..392b7a29452d8 --- /dev/null +++ b/apps/oxlint/fixtures/tsgolint_fix/tsconfig.json @@ -0,0 +1,9 @@ +{ + "compilerOptions": { + "strict": true, + "target": "ES2020", + "module": "ESNext", + "moduleResolution": "bundler" + }, + "include": ["*.ts"] +} diff --git a/apps/oxlint/src/lint.rs b/apps/oxlint/src/lint.rs index 09a183d5eca1a..d640460601a88 100644 --- a/apps/oxlint/src/lint.rs +++ b/apps/oxlint/src/lint.rs @@ -1433,4 +1433,30 @@ mod test { let args = &["--type-aware"]; Tester::new().with_cwd("fixtures/tsgolint_rule_options".into()).test_and_snapshot(args); } + + #[test] + #[cfg(all(not(target_os = "windows"), not(target_endian = "big")))] + fn test_tsgolint_fix() { + // Note: tsgolint fixes this lint rule by providing two string manipulations + // the first removing `as` and the second removing `string` This results in the two spaces + // after `str` but before `;`, this is ok, as it's not guaranteed that our fixers are stylistically correct. + Tester::test_fix_with_args( + "fixtures/tsgolint_fix/fix.ts", + "// This file has a fixable tsgolint error: no-unnecessary-type-assertion +// The type assertion `as string` is unnecessary because str is already a string +const str: string = 'hello'; +const redundant = str as string; + +export { redundant }; +", + "// This file has a fixable tsgolint error: no-unnecessary-type-assertion +// The type assertion `as string` is unnecessary because str is already a string +const str: string = 'hello'; +const redundant = str ; + +export { redundant }; +", + &["--type-aware", "-D", "no-unnecessary-type-assertion"], + ); + } } diff --git a/apps/oxlint/src/tester.rs b/apps/oxlint/src/tester.rs index 24b37d0e1449a..666698d09c8ec 100644 --- a/apps/oxlint/src/tester.rs +++ b/apps/oxlint/src/tester.rs @@ -47,18 +47,26 @@ impl Tester { } pub fn test_fix(file: &str, before: &str, after: &str) { + Self::test_fix_with_args(file, before, after, &[]); + } + + /// Test fix with additional CLI arguments (e.g., `--type-aware` for tsgolint) + pub fn test_fix_with_args(file: &str, before: &str, after: &str, extra_args: &[&str]) { use std::fs; #[expect(clippy::disallowed_methods)] let content_original = fs::read_to_string(file).unwrap().replace("\r\n", "\n"); assert_eq!(content_original, before); - Tester::new().test(&["--fix", file]); + let mut args = vec!["--fix"]; + args.extend(extra_args); + args.push(file); + Tester::new().test(&args); #[expect(clippy::disallowed_methods)] let new_content = fs::read_to_string(file).unwrap().replace("\r\n", "\n"); assert_eq!(new_content, after); - Tester::new().test(&["--fix", file]); + Tester::new().test(&args); // File should not be modified if no fix is applied. let modified_before: std::time::SystemTime = diff --git a/crates/oxc_linter/src/lint_runner.rs b/crates/oxc_linter/src/lint_runner.rs index ec518b052c80d..7eee7b218e0d5 100644 --- a/crates/oxc_linter/src/lint_runner.rs +++ b/crates/oxc_linter/src/lint_runner.rs @@ -222,7 +222,7 @@ impl LintRunner { self.lint_service.run(fs, files.to_owned(), &tx_error); if let Some(type_aware_linter) = self.type_aware_linter.take() { - type_aware_linter.lint(files, self.directives_store.map(), tx_error)?; + type_aware_linter.lint(files, self.directives_store.map(), tx_error, fs)?; } else { drop(tx_error); } diff --git a/crates/oxc_linter/src/tsgolint.rs b/crates/oxc_linter/src/tsgolint.rs index 0768a28c66c3d..89a4e4a885b01 100644 --- a/crates/oxc_linter/src/tsgolint.rs +++ b/crates/oxc_linter/src/tsgolint.rs @@ -14,7 +14,7 @@ use oxc_span::{SourceType, Span}; use super::{AllowWarnDeny, ConfigStore, DisableDirectives, ResolvedLinterState, read_to_string}; -use crate::{CompositeFix, FixKind, Message, PossibleFixes}; +use crate::{CompositeFix, FixKind, Fixer, Message, PossibleFixes}; /// State required to initialize the `tsgolint` linter. #[derive(Debug, Clone)] @@ -105,6 +105,7 @@ impl TsGoLintState { paths: &[Arc], disable_directives_map: Arc>>, error_sender: DiagnosticSender, + file_system: &(dyn crate::RuntimeFileSystem + Sync + Send), ) -> Result<(), String> { if paths.is_empty() { return Ok(()); @@ -117,137 +118,91 @@ impl TsGoLintState { return Ok(()); } + let should_fix = self.fix || self.fix_suggestions; + let cwd = self.cwd.clone(); + let sender_for_fixes = error_sender.clone(); + let handler = std::thread::spawn(move || { let mut child = self.spawn_tsgolint(&json_input)?; let stdout = child.stdout.take().expect("Failed to open tsgolint stdout"); // Process stdout stream in a separate thread to send diagnostics as they arrive - let cwd_clone = self.cwd.clone(); - - let stdout_handler = std::thread::spawn(move || -> Result<(), String> { - struct SourceTextCache(FxHashMap); - impl SourceTextCache { - fn get_or_insert(&mut self, path: &Path) -> &str { - self.0 - .entry(path.to_path_buf()) - .or_insert_with(|| read_to_string(path).unwrap_or_default()) - .as_str() - } - } - - let disable_directives_map = - disable_directives_map.lock().expect("disable_directives_map mutex poisoned"); - let msg_iter = TsGoLintMessageStream::new(stdout); - - let mut source_text_map = SourceTextCache(FxHashMap::default()); - - for msg in msg_iter { - match msg { - Ok(TsGoLintMessage::Error(err)) => { - return Err(err.error); - } - Ok(TsGoLintMessage::Diagnostic(tsgolint_diagnostic)) => { - match tsgolint_diagnostic { - TsGoLintDiagnostic::Rule(tsgolint_diagnostic) => { - let path = tsgolint_diagnostic.file_path.clone(); - let severity = resolved_configs - .get(&path) - .or_else(|| { - debug_assert!(false, "resolved_configs should have an entry for every file we linted {tsgolint_diagnostic:?}"); - None - }) - .and_then(|resolved_config| { - resolved_config - .rules - .iter() - .find(|(rule, _)| { - rule.name() == tsgolint_diagnostic.rule - }) - .map(|(_, status)| *status) - }) - .or_else(|| { - debug_assert!(false, "resolved_config should have a matching rule for every diagnostic we received {tsgolint_diagnostic:?}"); - None - }); - - let Some(severity) = severity else { - // If the severity is not found, we should not report the diagnostic - continue; - }; - - if should_skip_diagnostic( - &disable_directives_map, - &path, - &tsgolint_diagnostic, - ) { - continue; - } - - let oxc_diagnostic: OxcDiagnostic = - OxcDiagnostic::from(tsgolint_diagnostic); - - let oxc_diagnostic = oxc_diagnostic.with_severity( - if severity == AllowWarnDeny::Deny { - Severity::Error - } else { - Severity::Warning - }, - ); - - let source_text: &str = if self.silent { - // The source text is not needed in silent mode, the diagnostic isn't printed. - "" - } else { - source_text_map.get_or_insert(&path) - }; - - let diagnostics = DiagnosticService::wrap_diagnostics( - cwd_clone.clone(), - path, - source_text, - vec![oxc_diagnostic], - ); - - error_sender - .send(diagnostics) - .expect("Failed to send diagnostics"); - } - TsGoLintDiagnostic::Internal(e) => { - let oxc_diagnostic: OxcDiagnostic = e.clone().into(); - - let diagnostics = if let Some(file_path) = e.file_path { - let source_text: &str = if self.silent { - // The source text is not needed in silent mode, the diagnostic isn't printed. - "" - } else { - source_text_map.get_or_insert(&file_path) + let stdout_handler = std::thread::spawn( + move || -> Result)>, String> { + let disable_directives_map = disable_directives_map + .lock() + .expect("disable_directives_map mutex poisoned"); + + let mut diagnostic_handler = DiagnosticHandler::new( + self.cwd.clone(), + self.silent, + should_fix, + error_sender, + ); + + let msg_iter = TsGoLintMessageStream::new(stdout); + + for msg in msg_iter { + match msg { + Ok(TsGoLintMessage::Error(err)) => { + return Err(err.error); + } + Ok(TsGoLintMessage::Diagnostic(tsgolint_diagnostic)) => { + match tsgolint_diagnostic { + TsGoLintDiagnostic::Rule(tsgolint_diagnostic) => { + let path = &tsgolint_diagnostic.file_path; + + let severity = resolved_configs + .get(path) + .or_else(|| { + debug_assert!(false, "resolved_configs should have an entry for every file we linted {tsgolint_diagnostic:?}"); + None + }) + .and_then(|resolved_config| { + resolved_config + .rules + .iter() + .find(|(rule, _)| { + rule.name() == tsgolint_diagnostic.rule + }) + .map(|(_, status)| *status) + }) + .or_else(|| { + debug_assert!(false, "resolved_config should have a matching rule for every diagnostic we received {tsgolint_diagnostic:?}"); + None + }); + let Some(severity) = severity else { + // If the severity is not found, we should not report + // the diagnostic + continue; }; - DiagnosticService::wrap_diagnostics( - cwd_clone.clone(), - file_path, - source_text, - vec![oxc_diagnostic], - ) - } else { - vec![oxc_diagnostic.into()] - }; + if should_skip_diagnostic( + &disable_directives_map, + path, + &tsgolint_diagnostic, + ) { + continue; + } - error_sender - .send(diagnostics) - .expect("Failed to send diagnostics"); + diagnostic_handler + .handle_rule_diagnostic(tsgolint_diagnostic, severity); + } + TsGoLintDiagnostic::Internal(e) => { + diagnostic_handler.handle_internal_diagnostic(e); + } } } - } - Err(e) => { - return Err(e); + Err(e) => { + return Err(e); + } } } - } - Ok(()) - }); + Ok(diagnostic_handler.into_messages_requiring_fixes()) + }, + ); // Wait for process to complete and stdout processing to finish let exit_status = child.wait().expect("Failed to wait for tsgolint process"); @@ -264,15 +219,38 @@ impl TsGoLintState { } match stdout_result { - Ok(Ok(())) => Ok(()), + Ok(Ok(messages)) => Ok(messages), Ok(Err(err)) => Err(format!("exit status: {exit_status}, error: {err}")), Err(_) => Err("Failed to join stdout processing thread".to_string()), } }); match handler.join() { - Ok(Ok(())) => { - // Successfully ran tsgolint + Ok(Ok(messages_requiring_fixes)) => { + for (path, source_text, messages) in messages_requiring_fixes { + let source_type = SourceType::from_path(&path) + .ok() + .map(|st| if st.is_javascript() { st.with_jsx(true) } else { st }); + let fix_result = Fixer::new(&source_text, messages, source_type).fix(); + + if fix_result.fixed { + file_system + .write_file(&path, &fix_result.fixed_code) + .expect("Failed to write fixed file"); + } + + if !fix_result.messages.is_empty() { + let source_for_diagnostics: &str = + if fix_result.fixed { &fix_result.fixed_code } else { &source_text }; + let diagnostics = DiagnosticService::wrap_diagnostics( + &cwd, + &path, + source_for_diagnostics, + fix_result.messages.into_iter().map(Into::into).collect(), + ); + sender_for_fixes.send(diagnostics).expect("Failed to send diagnostics"); + } + } Ok(()) } Ok(Err(err)) => Err(format!("Error running tsgolint: {err:?}")), @@ -913,6 +891,136 @@ impl std::fmt::Display for TsGoLintMessageParseError { } } +/// Cache for source text to avoid reading the same file multiple times. +#[derive(Default)] +struct SourceTextCache(FxHashMap); + +impl SourceTextCache { + fn get_or_insert(&mut self, path: &Path) -> &str { + self.0 + .entry(path.to_path_buf()) + .or_insert_with(|| read_to_string(path).unwrap_or_default()) + .as_str() + } +} + +/// Handles streaming and collecting diagnostics from tsgolint. +struct DiagnosticHandler { + cwd: PathBuf, + silent: bool, + should_fix: bool, + source_text_cache: SourceTextCache, + error_sender: DiagnosticSender, + /// Messages requiring fixes, grouped by file path: messages. + messages_requiring_fixes: FxHashMap>, +} + +impl DiagnosticHandler { + fn new(cwd: PathBuf, silent: bool, should_fix: bool, error_sender: DiagnosticSender) -> Self { + Self { + cwd, + silent, + should_fix, + source_text_cache: SourceTextCache::default(), + error_sender, + messages_requiring_fixes: FxHashMap::default(), + } + } + + fn get_source_text(&mut self, path: &Path) -> &str { + if self.silent && !self.should_fix { + // The source text is not needed in silent mode, the diagnostic isn't printed. + "" + } else { + self.source_text_cache.get_or_insert(path) + } + } + + fn handle_rule_diagnostic( + &mut self, + diagnostic: TsGoLintRuleDiagnostic, + severity: AllowWarnDeny, + ) { + let path = diagnostic.file_path.clone(); + let has_fixes = + self.should_fix && (!diagnostic.fixes.is_empty() || !diagnostic.suggestions.is_empty()); + + if has_fixes { + // Collect for later fix application + let mut message = + Message::from_tsgo_lint_diagnostic(diagnostic, self.get_source_text(&path)); + message.error.severity = + if severity == AllowWarnDeny::Deny { Severity::Error } else { Severity::Warning }; + + let entry = self.messages_requiring_fixes.entry(path).or_default(); + + entry.push(message); + } else { + // Stream immediately + self.send_diagnostic(&path, diagnostic.into(), severity); + } + } + + fn handle_internal_diagnostic(&mut self, e: TsGoLintInternalDiagnostic) { + let file_path = e.file_path.clone(); + let oxc_diagnostic: OxcDiagnostic = e.into(); + + let diagnostics = if let Some(ref file_path) = file_path { + let source_text = self.get_source_text(file_path).to_string(); + DiagnosticService::wrap_diagnostics( + &self.cwd, + file_path, + &source_text, + vec![oxc_diagnostic], + ) + } else { + vec![oxc_diagnostic.into()] + }; + + self.error_sender.send(diagnostics).expect("Failed to send diagnostics"); + } + + fn send_diagnostic( + &mut self, + path: &Path, + oxc_diagnostic: OxcDiagnostic, + severity: AllowWarnDeny, + ) { + let source_text = self.get_source_text(path).to_string(); + let oxc_diagnostic = oxc_diagnostic.with_severity(if severity == AllowWarnDeny::Deny { + Severity::Error + } else { + Severity::Warning + }); + let diagnostics = DiagnosticService::wrap_diagnostics( + &self.cwd, + path, + &source_text, + vec![oxc_diagnostic], + ); + self.error_sender.send(diagnostics).expect("Failed to send diagnostics"); + } + + /// Consume the handler and return collected messages requiring fixes. + fn into_messages_requiring_fixes(self) -> Vec<(PathBuf, String, Vec)> { + let Self { messages_requiring_fixes, mut source_text_cache, should_fix, silent, .. } = self; + + messages_requiring_fixes + .into_iter() + .map(|(path, messages)| { + let source_text = source_text_cache.0.remove(&path).unwrap_or_else(|| { + if !silent || should_fix { + read_to_string(&path).unwrap_or_default() + } else { + String::new() + } + }); + (path, source_text, messages) + }) + .collect() + } +} + fn should_skip_diagnostic( disable_directives_map: &FxHashMap, path: &Path,