From 4ae568e8c9ca22ae378624669c365ca47fcd1d55 Mon Sep 17 00:00:00 2001 From: "Alexander S." Date: Thu, 23 Jan 2025 03:09:51 +0100 Subject: [PATCH] feat(linter): add DiagnosticResult to the Reporters for receiving a sub part result (#8666) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We are currently outputting only for the default-outputter some extra information: https://github.com/oxc-project/oxc/blob/3be03926e8a5097e5c1fe249b8bff0009ec4468d/apps/oxlint/src/result.rs#L61-L87 My goal is that all information will be passed to our new DiagnosticReporter / OutputFormatter. This will break the output format in the next PR. **Merging this PR is the OK for me to make this change** ⚠️ The only breaking point: `"Found {number_of_warnings} warning{} and {number_of_errors} error{}."` will still be outputted when `max_warnings_exceeded` is true. Because this is something the `DiagnosticReporter` should do and not the `OutputFormatter`. The end goal is: - no `println!`, our `OutputFormatter` and his `DiagnosticReporter` will return `Option` and we output it the our `stdout` - `LintResult` will only handle `ExitCode` result and nothing more - `stdout` can be changed from outside (for the next part) - add snapshots with a custom `stdout` I do not know if all goals can be done easily. Last two parts should be a bit tricky for me, never did test setups in rust. But we do never stop to learn ;) --- apps/oxlint/src/lint.rs | 9 +-- .../oxlint/src/output_formatter/checkstyle.rs | 11 ++-- apps/oxlint/src/output_formatter/default.rs | 14 ++-- apps/oxlint/src/output_formatter/github.rs | 11 ++-- apps/oxlint/src/output_formatter/json.rs | 10 ++- apps/oxlint/src/output_formatter/stylish.rs | 8 +-- apps/oxlint/src/output_formatter/unix.rs | 13 ++-- crates/oxc_diagnostics/src/reporter.rs | 38 ++++++++++- crates/oxc_diagnostics/src/service.rs | 66 ++++++++----------- 9 files changed, 114 insertions(+), 66 deletions(-) diff --git a/apps/oxlint/src/lint.rs b/apps/oxlint/src/lint.rs index 8b1c29621ef07..5773c6876240e 100644 --- a/apps/oxlint/src/lint.rs +++ b/apps/oxlint/src/lint.rs @@ -208,15 +208,16 @@ impl Runner for LintRunner { lint_service.run(&tx_error); } }); - diagnostic_service.run(&mut stdout); + + let diagnostic_result = diagnostic_service.run(&mut stdout); CliRunResult::LintResult(LintResult { duration: now.elapsed(), number_of_rules: lint_service.linter().number_of_rules(), number_of_files, - number_of_warnings: diagnostic_service.warnings_count(), - number_of_errors: diagnostic_service.errors_count(), - max_warnings_exceeded: diagnostic_service.max_warnings_exceeded(), + number_of_warnings: diagnostic_result.warnings_count(), + number_of_errors: diagnostic_result.errors_count(), + max_warnings_exceeded: diagnostic_result.max_warnings_exceeded(), deny_warnings: warning_options.deny_warnings, print_summary: matches!(output_options.format, OutputFormat::Default), }) diff --git a/apps/oxlint/src/output_formatter/checkstyle.rs b/apps/oxlint/src/output_formatter/checkstyle.rs index adcbd4eed824a..ac5bfd1e32e3d 100644 --- a/apps/oxlint/src/output_formatter/checkstyle.rs +++ b/apps/oxlint/src/output_formatter/checkstyle.rs @@ -3,7 +3,7 @@ use std::{borrow::Cow, io::Write}; use rustc_hash::FxHashMap; use oxc_diagnostics::{ - reporter::{DiagnosticReporter, Info}, + reporter::{DiagnosticReporter, DiagnosticResult, Info}, Error, Severity, }; @@ -31,7 +31,7 @@ struct CheckstyleReporter { } impl DiagnosticReporter for CheckstyleReporter { - fn finish(&mut self) -> Option { + fn finish(&mut self, _: &DiagnosticResult) -> Option { Some(format_checkstyle(&self.diagnostics)) } @@ -123,7 +123,10 @@ fn xml_escape_impl bool>(raw: &str, escape_chars: F) -> Cow { #[cfg(test)] mod test { - use oxc_diagnostics::{reporter::DiagnosticReporter, NamedSource, OxcDiagnostic}; + use oxc_diagnostics::{ + reporter::{DiagnosticReporter, DiagnosticResult}, + NamedSource, OxcDiagnostic, + }; use oxc_span::Span; use super::CheckstyleReporter; @@ -142,7 +145,7 @@ mod test { assert!(first_result.is_none()); // report not gives us all diagnostics at ones - let second_result = reporter.finish(); + let second_result = reporter.finish(&DiagnosticResult::default()); assert!(second_result.is_some()); assert_eq!(second_result.unwrap(), ""); diff --git a/apps/oxlint/src/output_formatter/default.rs b/apps/oxlint/src/output_formatter/default.rs index 838bf7720243d..d2704837d4097 100644 --- a/apps/oxlint/src/output_formatter/default.rs +++ b/apps/oxlint/src/output_formatter/default.rs @@ -1,6 +1,9 @@ use std::io::Write; -use oxc_diagnostics::{reporter::DiagnosticReporter, Error, GraphicalReportHandler}; +use oxc_diagnostics::{ + reporter::{DiagnosticReporter, DiagnosticResult}, + Error, GraphicalReportHandler, +}; use oxc_linter::table::RuleTable; use crate::output_formatter::InternalFormatter; @@ -37,7 +40,7 @@ impl Default for GraphicalReporter { } impl DiagnosticReporter for GraphicalReporter { - fn finish(&mut self) -> Option { + fn finish(&mut self, _: &DiagnosticResult) -> Option { None } @@ -62,7 +65,10 @@ mod test { InternalFormatter, }; use miette::{GraphicalReportHandler, GraphicalTheme, NamedSource}; - use oxc_diagnostics::{reporter::DiagnosticReporter, OxcDiagnostic}; + use oxc_diagnostics::{ + reporter::{DiagnosticReporter, DiagnosticResult}, + OxcDiagnostic, + }; use oxc_span::Span; #[test] @@ -78,7 +84,7 @@ mod test { fn reporter_finish() { let mut reporter = GraphicalReporter::default(); - let result = reporter.finish(); + let result = reporter.finish(&DiagnosticResult::default()); assert!(result.is_none()); } diff --git a/apps/oxlint/src/output_formatter/github.rs b/apps/oxlint/src/output_formatter/github.rs index f5739366e0641..897bffcb43685 100644 --- a/apps/oxlint/src/output_formatter/github.rs +++ b/apps/oxlint/src/output_formatter/github.rs @@ -1,7 +1,7 @@ use std::{borrow::Cow, io::Write}; use oxc_diagnostics::{ - reporter::{DiagnosticReporter, Info}, + reporter::{DiagnosticReporter, DiagnosticResult, Info}, Error, Severity, }; @@ -25,7 +25,7 @@ impl InternalFormatter for GithubOutputFormatter { struct GithubReporter; impl DiagnosticReporter for GithubReporter { - fn finish(&mut self) -> Option { + fn finish(&mut self, _: &DiagnosticResult) -> Option { None } @@ -88,7 +88,10 @@ fn escape_property(value: &str) -> String { #[cfg(test)] mod test { - use oxc_diagnostics::{reporter::DiagnosticReporter, NamedSource, OxcDiagnostic}; + use oxc_diagnostics::{ + reporter::{DiagnosticReporter, DiagnosticResult}, + NamedSource, OxcDiagnostic, + }; use oxc_span::Span; use super::GithubReporter; @@ -97,7 +100,7 @@ mod test { fn reporter_finish() { let mut reporter = GithubReporter; - let result = reporter.finish(); + let result = reporter.finish(&DiagnosticResult::default()); assert!(result.is_none()); } diff --git a/apps/oxlint/src/output_formatter/json.rs b/apps/oxlint/src/output_formatter/json.rs index 7e1cdd946f192..3dd6637aaa780 100644 --- a/apps/oxlint/src/output_formatter/json.rs +++ b/apps/oxlint/src/output_formatter/json.rs @@ -1,5 +1,6 @@ use std::io::Write; +use oxc_diagnostics::reporter::DiagnosticResult; use oxc_diagnostics::{reporter::DiagnosticReporter, Error}; use oxc_linter::rules::RULES; use oxc_linter::RuleCategory; @@ -52,7 +53,7 @@ struct JsonReporter { impl DiagnosticReporter for JsonReporter { // NOTE: this output does not conform to eslint json format yet // https://eslint.org/docs/latest/use/formatters/#json - fn finish(&mut self) -> Option { + fn finish(&mut self, _: &DiagnosticResult) -> Option { Some(format_json(&mut self.diagnostics)) } @@ -80,7 +81,10 @@ fn format_json(diagnostics: &mut Vec) -> String { #[cfg(test)] mod test { - use oxc_diagnostics::{reporter::DiagnosticReporter, NamedSource, OxcDiagnostic}; + use oxc_diagnostics::{ + reporter::{DiagnosticReporter, DiagnosticResult}, + NamedSource, OxcDiagnostic, + }; use oxc_span::Span; use super::JsonReporter; @@ -99,7 +103,7 @@ mod test { assert!(first_result.is_none()); // report not gives us all diagnostics at ones - let second_result = reporter.finish(); + let second_result = reporter.finish(&DiagnosticResult::default()); assert!(second_result.is_some()); assert_eq!( diff --git a/apps/oxlint/src/output_formatter/stylish.rs b/apps/oxlint/src/output_formatter/stylish.rs index 7f9e4e2e17bb9..c046fae484685 100644 --- a/apps/oxlint/src/output_formatter/stylish.rs +++ b/apps/oxlint/src/output_formatter/stylish.rs @@ -1,7 +1,7 @@ use std::io::Write; use oxc_diagnostics::{ - reporter::{DiagnosticReporter, Info}, + reporter::{DiagnosticReporter, DiagnosticResult, Info}, Error, Severity, }; use rustc_hash::FxHashMap; @@ -27,7 +27,7 @@ struct StylishReporter { } impl DiagnosticReporter for StylishReporter { - fn finish(&mut self) -> Option { + fn finish(&mut self, _: &DiagnosticResult) -> Option { Some(format_stylish(&self.diagnostics)) } @@ -116,7 +116,7 @@ fn format_stylish(diagnostics: &[Error]) -> String { #[cfg(test)] mod test { use super::*; - use oxc_diagnostics::{NamedSource, OxcDiagnostic}; + use oxc_diagnostics::{reporter::DiagnosticResult, NamedSource, OxcDiagnostic}; use oxc_span::Span; #[test] @@ -134,7 +134,7 @@ mod test { reporter.render_error(error); reporter.render_error(warning); - let output = reporter.finish().unwrap(); + let output = reporter.finish(&DiagnosticResult::default()).unwrap(); assert!(output.contains("error message"), "Output should contain 'error message'"); assert!(output.contains("warning message"), "Output should contain 'warning message'"); diff --git a/apps/oxlint/src/output_formatter/unix.rs b/apps/oxlint/src/output_formatter/unix.rs index 12cfa99699602..e1f0607c6db18 100644 --- a/apps/oxlint/src/output_formatter/unix.rs +++ b/apps/oxlint/src/output_formatter/unix.rs @@ -1,7 +1,7 @@ use std::{borrow::Cow, io::Write}; use oxc_diagnostics::{ - reporter::{DiagnosticReporter, Info}, + reporter::{DiagnosticReporter, DiagnosticResult, Info}, Error, Severity, }; @@ -28,7 +28,7 @@ struct UnixReporter { } impl DiagnosticReporter for UnixReporter { - fn finish(&mut self) -> Option { + fn finish(&mut self, _: &DiagnosticResult) -> Option { let total = self.total; if total > 0 { return Some(format!("\n{total} problem{}\n", if total > 1 { "s" } else { "" })); @@ -57,7 +57,10 @@ fn format_unix(diagnostic: &Error) -> String { #[cfg(test)] mod test { - use oxc_diagnostics::{reporter::DiagnosticReporter, NamedSource, OxcDiagnostic}; + use oxc_diagnostics::{ + reporter::{DiagnosticReporter, DiagnosticResult}, + NamedSource, OxcDiagnostic, + }; use oxc_span::Span; use super::UnixReporter; @@ -66,7 +69,7 @@ mod test { fn reporter_finish_empty() { let mut reporter = UnixReporter::default(); - let result = reporter.finish(); + let result = reporter.finish(&DiagnosticResult::default()); assert!(result.is_none()); } @@ -80,7 +83,7 @@ mod test { .with_source_code(NamedSource::new("file://test.ts", "debugger;")); let _ = reporter.render_error(error); - let result = reporter.finish(); + let result = reporter.finish(&DiagnosticResult::default()); assert!(result.is_some()); assert_eq!(result.unwrap(), "\n1 problem\n"); diff --git a/crates/oxc_diagnostics/src/reporter.rs b/crates/oxc_diagnostics/src/reporter.rs index ffe44986c4586..0bdd7ace8fa1d 100644 --- a/crates/oxc_diagnostics/src/reporter.rs +++ b/crates/oxc_diagnostics/src/reporter.rs @@ -45,7 +45,7 @@ pub trait DiagnosticReporter { /// /// While this method _should_ only ever be called a single time, this is not a guarantee /// upheld in Oxc's API. Do not rely on this behavior. - fn finish(&mut self) -> Option; + fn finish(&mut self, result: &DiagnosticResult) -> Option; /// Render a diagnostic into this reporter's desired format. For example, a JSONLinesReporter /// might return a stringified JSON object on a single line. Returns [`None`] to skip reporting @@ -55,6 +55,42 @@ pub trait DiagnosticReporter { fn render_error(&mut self, error: Error) -> Option; } +/// DiagnosticResult will be submitted to the Reporter when the [`DiagnosticService`](crate::service::DiagnosticService) +/// is finished receiving all files +#[derive(Default)] +pub struct DiagnosticResult { + /// Total number of warnings received + warnings_count: usize, + + /// Total number of errors received + errors_count: usize, + + /// Did the threshold for warnings exceeded the max_warnings? + /// ToDo: We giving the input from outside, let the owner calculate the result + max_warnings_exceeded: bool, +} + +impl DiagnosticResult { + pub fn new(warnings_count: usize, errors_count: usize, max_warnings_exceeded: bool) -> Self { + Self { warnings_count, errors_count, max_warnings_exceeded } + } + + /// Get the number of warning-level diagnostics received. + pub fn warnings_count(&self) -> usize { + self.warnings_count + } + + /// Get the number of error-level diagnostics received. + pub fn errors_count(&self) -> usize { + self.errors_count + } + + /// Did the threshold for warnings exceeded the max_warnings? + pub fn max_warnings_exceeded(&self) -> bool { + self.max_warnings_exceeded + } +} + pub struct Info { pub start: InfoPosition, pub end: InfoPosition, diff --git a/crates/oxc_diagnostics/src/service.rs b/crates/oxc_diagnostics/src/service.rs index b46c2678f63d3..16ec5a098a234 100644 --- a/crates/oxc_diagnostics/src/service.rs +++ b/crates/oxc_diagnostics/src/service.rs @@ -1,11 +1,13 @@ use std::{ - cell::Cell, io::{ErrorKind, Write}, path::{Path, PathBuf}, sync::{mpsc, Arc}, }; -use crate::{reporter::DiagnosticReporter, Error, NamedSource, OxcDiagnostic, Severity}; +use crate::{ + reporter::{DiagnosticReporter, DiagnosticResult}, + Error, NamedSource, OxcDiagnostic, Severity, +}; pub type DiagnosticTuple = (PathBuf, Vec); pub type DiagnosticSender = mpsc::Sender>; @@ -56,12 +58,6 @@ pub struct DiagnosticService { /// which can be used to force exit with an error status if there are too many warning-level rule violations in your project max_warnings: Option, - /// Total number of warnings received - warnings_count: Cell, - - /// Total number of errors received - errors_count: Cell, - sender: DiagnosticSender, receiver: DiagnosticReceiver, } @@ -71,16 +67,7 @@ impl DiagnosticService { /// provided [`DiagnosticReporter`]. pub fn new(reporter: Box) -> Self { let (sender, receiver) = mpsc::channel(); - Self { - reporter, - quiet: false, - silent: false, - max_warnings: None, - warnings_count: Cell::new(0), - errors_count: Cell::new(0), - sender, - receiver, - } + Self { reporter, quiet: false, silent: false, max_warnings: None, sender, receiver } } /// Set to `true` to only report errors and ignore warnings. @@ -109,7 +96,7 @@ impl DiagnosticService { /// are too many warning-level rule violations in your project. Errors do not count towards the /// warning limit. /// - /// Use [`max_warnings_exceeded`](DiagnosticService::max_warnings_exceeded) to check if too + /// Use [`DiagnosticResult`](DiagnosticResult::max_warnings_exceeded) to check if too /// many warnings have been received. /// /// Default: [`None`] @@ -129,20 +116,10 @@ impl DiagnosticService { &self.sender } - /// Get the number of warning-level diagnostics received. - pub fn warnings_count(&self) -> usize { - self.warnings_count.get() - } - - /// Get the number of error-level diagnostics received. - pub fn errors_count(&self) -> usize { - self.errors_count.get() - } - /// Check if the max warning threshold, as set by /// [`with_max_warnings`](DiagnosticService::with_max_warnings), has been exceeded. - pub fn max_warnings_exceeded(&self) -> bool { - self.max_warnings.is_some_and(|max_warnings| self.warnings_count.get() > max_warnings) + fn max_warnings_exceeded(&self, warnings_count: usize) -> bool { + self.max_warnings.is_some_and(|max_warnings| warnings_count > max_warnings) } /// Wrap [diagnostics] with the source code and path, converting them into [Error]s. @@ -165,7 +142,16 @@ impl DiagnosticService { /// # Panics /// /// * When the writer fails to write - pub fn run(&mut self, writer: &mut dyn Write) { + /// + /// ToDo: + /// We are passing [`DiagnosticResult`] to the [`DiagnosticReporter`] already + /// currently for the GraphicalReporter there is another extra output, + /// which does some more things. This is the reason why we are returning it. + /// Let's check at first it we can easily change for the default output before removing this return. + pub fn run(&mut self, writer: &mut dyn Write) -> DiagnosticResult { + let mut warnings_count: usize = 0; + let mut errors_count: usize = 0; + while let Ok(Some((path, diagnostics))) = self.receiver.recv() { for diagnostic in diagnostics { let severity = diagnostic.severity(); @@ -173,12 +159,10 @@ impl DiagnosticService { let is_error = severity == Some(Severity::Error) || severity.is_none(); if is_warning || is_error { if is_warning { - let warnings_count = self.warnings_count() + 1; - self.warnings_count.set(warnings_count); + warnings_count += 1; } if is_error { - let errors_count = self.errors_count() + 1; - self.errors_count.set(errors_count); + errors_count += 1; } // The --quiet flag follows ESLint's --quiet behavior as documented here: https://eslint.org/docs/latest/use/command-line-interface#--quiet // Note that it does not disable ALL diagnostics, only Warning diagnostics @@ -217,7 +201,13 @@ impl DiagnosticService { } } - if let Some(finish_output) = self.reporter.finish() { + let result = DiagnosticResult::new( + warnings_count, + errors_count, + self.max_warnings_exceeded(warnings_count), + ); + + if let Some(finish_output) = self.reporter.finish(&result) { writer .write_all(finish_output.as_bytes()) .or_else(Self::check_for_writer_error) @@ -225,6 +215,8 @@ impl DiagnosticService { } writer.flush().or_else(Self::check_for_writer_error).unwrap(); + + result } fn check_for_writer_error(error: std::io::Error) -> Result<(), std::io::Error> {