Simplify LinterResult, avoid cloning ParseError

Author: dhruvmanila

crates/ruff/src/diagnostics.rs

@@ -260,8 +260,8 @@ pub(crate) fn lint_path(
     // Lint the file.
     let (
         LinterResult {
-            data: messages,
-            error: parse_error,
+            messages,
+            has_error,
         },
         transformed,
         fixed,
@@ -330,7 +330,7 @@ pub(crate) fn lint_path(
 
     if let Some((cache, relative_path, key)) = caching {
         // We don't cache parsing errors.
-        if parse_error.is_none() {
+        if !has_error {
             // `FixMode::Apply` and `FixMode::Diff` rely on side-effects (writing to disk,
             // and writing the diff to stdout, respectively). If a file has diagnostics, we
             // need to avoid reading from and writing to the cache in these modes.
@@ -396,7 +396,7 @@ pub(crate) fn lint_stdin(
     };
 
     // Lint the inputs.
-    let (LinterResult { data: messages, .. }, transformed, fixed) =
+    let (LinterResult { messages, .. }, transformed, fixed) =
         if matches!(fix_mode, flags::FixMode::Apply | flags::FixMode::Diff) {
             if let Ok(FixerResult {
                 result,

crates/ruff_benchmark/benches/linter.rs

@@ -73,7 +73,7 @@ fn benchmark_linter(mut group: BenchmarkGroup, settings: &LinterSettings) {
                         );
 
                         // Assert that file contains no parse errors
-                        assert_eq!(result.error, None);
+                        assert!(!result.has_error);
                     },
                     criterion::BatchSize::SmallInput,
                 );

crates/ruff_linter/src/linter.rs

@@ -36,29 +36,19 @@ use crate::settings::{flags, LinterSettings};
 use crate::source_kind::SourceKind;
 use crate::{directives, fs};
 
-/// A [`Result`]-like type that returns both data and an error. Used to return
-/// diagnostics even in the face of parse errors, since many diagnostics can be
-/// generated without a full AST.
-pub struct LinterResult<T> {
-    pub data: T,
-    pub error: Option<ParseError>,
-}
-
-impl<T> LinterResult<T> {
-    const fn new(data: T, error: Option<ParseError>) -> Self {
-        Self { data, error }
-    }
-
-    fn map<U, F: FnOnce(T) -> U>(self, f: F) -> LinterResult<U> {
-        LinterResult::new(f(self.data), self.error)
-    }
+pub struct LinterResult {
+    /// A collection of diagnostic messages generated by the linter.
+    pub messages: Vec<Message>,
+    /// A flag indicating the presence of syntax errors in the source file.
+    /// If `true`, at least one syntax error was detected in the source file.
+    pub has_error: bool,
 }
 
 pub type FixTable = FxHashMap<Rule, usize>;
 
 pub struct FixerResult<'a> {
     /// The result returned by the linter, after applying any fixes.
-    pub result: LinterResult<Vec<Message>>,
+    pub result: LinterResult,
     /// The resulting source code, after applying any fixes.
     pub transformed: Cow<'a, SourceKind>,
     /// The number of fixes applied for each [`Rule`].
@@ -80,7 +70,7 @@ pub fn check_path(
     source_kind: &SourceKind,
     source_type: PySourceType,
     parsed: &Parsed<ModModule>,
-) -> LinterResult<Vec<Diagnostic>> {
+) -> Vec<Diagnostic> {
     // Aggregate all diagnostics.
     let mut diagnostics = vec![];
 
@@ -328,7 +318,7 @@ pub fn check_path(
         }
     }
 
-    LinterResult::new(diagnostics, parsed.errors().iter().next().cloned())
+    diagnostics
 }
 
 const MAX_ITERATIONS: usize = 100;
@@ -362,9 +352,7 @@ pub fn add_noqa_to_path(
     );
 
     // Generate diagnostics, ignoring any existing `noqa` directives.
-    let LinterResult {
-        data: diagnostics, ..
-    } = check_path(
+    let diagnostics = check_path(
         path,
         package,
         &locator,
@@ -401,7 +389,7 @@ pub fn lint_only(
     source_kind: &SourceKind,
     source_type: PySourceType,
     source: ParseSource,
-) -> LinterResult<Vec<Message>> {
+) -> LinterResult {
     let parsed = source.into_parsed(source_kind, source_type);
 
     // Map row and column locations to byte slices (lazily).
@@ -422,7 +410,7 @@ pub fn lint_only(
     );
 
     // Generate diagnostics.
-    let result = check_path(
+    let diagnostics = check_path(
         path,
         package,
         &locator,
@@ -436,7 +424,10 @@ pub fn lint_only(
         &parsed,
     );
 
-    result.map(|diagnostics| diagnostics_to_messages(diagnostics, path, &locator, &directives))
+    LinterResult {
+        messages: diagnostics_to_messages(diagnostics, path, &locator, &directives),
+        has_error: !parsed.is_valid(),
+    }
 }
 
 /// Convert from diagnostics to messages.
@@ -513,7 +504,7 @@ pub fn lint_fix<'a>(
         );
 
         // Generate diagnostics.
-        let result = check_path(
+        let diagnostics = check_path(
             path,
             package,
             &locator,
@@ -528,19 +519,21 @@ pub fn lint_fix<'a>(
         );
 
         if iterations == 0 {
-            parseable = result.error.is_none();
+            parseable = parsed.is_valid();
         } else {
             // If the source code was parseable on the first pass, but is no
             // longer parseable on a subsequent pass, then we've introduced a
             // syntax error. Return the original code.
-            if parseable && result.error.is_some() {
-                report_fix_syntax_error(
-                    path,
-                    transformed.source_code(),
-                    &result.error.unwrap(),
-                    fixed.keys().copied(),
-                );
-                return Err(anyhow!("Fix introduced a syntax error"));
+            if parseable {
+                if let [error, ..] = parsed.errors() {
+                    report_fix_syntax_error(
+                        path,
+                        transformed.source_code(),
+                        error,
+                        fixed.keys().copied(),
+                    );
+                    return Err(anyhow!("Fix introduced a syntax error"));
+                }
             }
         }
 
@@ -549,7 +542,7 @@ pub fn lint_fix<'a>(
             code: fixed_contents,
             fixes: applied,
             source_map,
-        }) = fix_file(&result.data, &locator, unsafe_fixes)
+        }) = fix_file(&diagnostics, &locator, unsafe_fixes)
         {
             if iterations < MAX_ITERATIONS {
                 // Count the number of fixed errors.
@@ -566,13 +559,14 @@ pub fn lint_fix<'a>(
                 continue;
             }
 
-            report_failed_to_converge_error(path, transformed.source_code(), &result.data);
+            report_failed_to_converge_error(path, transformed.source_code(), &diagnostics);
         }
 
         return Ok(FixerResult {
-            result: result.map(|diagnostics| {
-                diagnostics_to_messages(diagnostics, path, &locator, &directives)
-            }),
+            result: LinterResult {
+                messages: diagnostics_to_messages(diagnostics, path, &locator, &directives),
+                has_error: !parseable,
+            },
             transformed,
             fixed,
         });

crates/ruff_linter/src/rules/pyflakes/mod.rs

@@ -22,7 +22,7 @@ mod tests {
     use ruff_source_file::Locator;
     use ruff_text_size::Ranged;
 
-    use crate::linter::{check_path, LinterResult};
+    use crate::linter::check_path;
     use crate::registry::{AsRule, Linter, Rule};
     use crate::rules::pyflakes;
     use crate::settings::types::PreviewMode;
@@ -649,10 +649,7 @@ mod tests {
             &locator,
             &indexer,
         );
-        let LinterResult {
-            data: mut diagnostics,
-            ..
-        } = check_path(
+        let mut diagnostics = check_path(
             Path::new("<filename>"),
             None,
             &locator,

crates/ruff_linter/src/test.rs

@@ -22,7 +22,7 @@ use ruff_text_size::Ranged;
 
 use crate::directives;
 use crate::fix::{fix_file, FixResult};
-use crate::linter::{check_path, LinterResult};
+use crate::linter::check_path;
 use crate::message::{Emitter, EmitterContext, Message, TextEmitter};
 use crate::packaging::detect_package_root;
 use crate::registry::AsRule;
@@ -119,10 +119,7 @@ pub(crate) fn test_contents<'a>(
         &locator,
         &indexer,
     );
-    let LinterResult {
-        data: diagnostics,
-        error,
-    } = check_path(
+    let diagnostics = check_path(
         path,
         path.parent()
             .and_then(|parent| detect_package_root(parent, &settings.namespace_packages)),
@@ -137,7 +134,7 @@ pub(crate) fn test_contents<'a>(
         &parsed,
     );
 
-    let source_has_errors = error.is_some();
+    let source_has_errors = !parsed.is_valid();
 
     // Detect fixes that don't converge after multiple iterations.
     let mut iterations = 0;
@@ -186,10 +183,7 @@ pub(crate) fn test_contents<'a>(
                 &indexer,
             );
 
-            let LinterResult {
-                data: fixed_diagnostics,
-                error: fixed_error,
-            } = check_path(
+            let fixed_diagnostics = check_path(
                 path,
                 None,
                 &locator,
@@ -203,13 +197,13 @@ pub(crate) fn test_contents<'a>(
                 &parsed,
             );
 
-            if let Some(fixed_error) = fixed_error {
+            if let [fixed_error, ..] = parsed.errors() {
                 if !source_has_errors {
                     // Previous fix introduced a syntax error, abort
                     let fixes = print_diagnostics(diagnostics, path, source_kind);
 
                     let mut syntax_diagnostics = Vec::new();
-                    syntax_error(&mut syntax_diagnostics, &fixed_error, &locator);
+                    syntax_error(&mut syntax_diagnostics, fixed_error, &locator);
                     let syntax_errors = print_diagnostics(syntax_diagnostics, path, &transformed);
 
                     panic!(

crates/ruff_server/src/fix.rs

@@ -68,7 +68,7 @@ pub(crate) fn fix_all(
     // which is inconsistent with how `ruff check --fix` works.
     let FixerResult {
         transformed,
-        result: LinterResult { error, .. },
+        result: LinterResult { has_error, .. },
         ..
     } = ruff_linter::linter::lint_fix(
         &query.virtual_file_path(),
@@ -80,11 +80,9 @@ pub(crate) fn fix_all(
         source_type,
     )?;
 
-    if let Some(error) = error {
+    if has_error {
         // abort early if a parsing error occurred
-        return Err(anyhow::anyhow!(
-            "A parsing error occurred during `fix_all`: {error}"
-        ));
+        return Err(anyhow::anyhow!("A parsing error occurred during `fix_all`"));
     }
 
     // fast path: if `transformed` is still borrowed, no changes were made and we can return early