Make DisplayParseError an error type (#9325)

## Summary

This is a non-behavior-changing refactor to follow-up
https://github.com/astral-sh/ruff/pull/9321 by modifying
`DisplayParseError` to use owned data and make it useable as a
standalone error type (rather than using references and implementing
`Display`). I don't feel very strongly either way. I thought it was
awkward that the `FormatCommandError` had two branches in the display
path, and wanted to represent the `Parse` vs. other cases as a separate
enum, so here we are.

crates/ruff_cli/Cargo.toml

@@ -15,16 +15,16 @@ readme = "../../README.md"
 name = "ruff"
 
 [dependencies]
-ruff_linter = { path = "../ruff_linter", features = ["clap"] }
 ruff_cache = { path = "../ruff_cache" }
 ruff_diagnostics = { path = "../ruff_diagnostics" }
-ruff_notebook = { path = "../ruff_notebook" }
+ruff_linter = { path = "../ruff_linter", features = ["clap"] }
 ruff_macros = { path = "../ruff_macros" }
+ruff_notebook = { path = "../ruff_notebook" }
 ruff_python_ast = { path = "../ruff_python_ast" }
 ruff_python_formatter = { path = "../ruff_python_formatter" }
 ruff_source_file = { path = "../ruff_source_file" }
-ruff_workspace = { path = "../ruff_workspace" }
 ruff_text_size = { path = "../ruff_text_size" }
+ruff_workspace = { path = "../ruff_workspace" }
 
 anyhow = { workspace = true }
 argfile = { version = "0.1.6" }

crates/ruff_cli/src/commands/format.rs

@@ -24,7 +24,6 @@ use ruff_linter::source_kind::{SourceError, SourceKind};
 use ruff_linter::warn_user_once;
 use ruff_python_ast::{PySourceType, SourceType};
 use ruff_python_formatter::{format_module_source, FormatModuleError, QuoteStyle};
-use ruff_source_file::{LineIndex, SourceCode};
 use ruff_text_size::{TextLen, TextRange, TextSize};
 use ruff_workspace::resolver::{
     match_exclusion, python_files_in_path, PyprojectConfig, ResolvedFile, Resolver,
@@ -327,7 +326,16 @@ pub(crate) fn format_source(
             let options = settings.to_format_options(source_type, unformatted);
 
             let formatted = format_module_source(unformatted, options).map_err(|err| {
-                FormatCommandError::Format(path.map(Path::to_path_buf), source_kind.clone(), err)
+                if let FormatModuleError::ParseError(err) = err {
+                    DisplayParseError::from_source_kind(
+                        err,
+                        path.map(Path::to_path_buf),
+                        source_kind,
+                    )
+                    .into()
+                } else {
+                    FormatCommandError::Format(path.map(Path::to_path_buf), err)
+                }
             })?;
 
             let formatted = formatted.into_code();
@@ -356,11 +364,16 @@ pub(crate) fn format_source(
                 // Format the cell.
                 let formatted =
                     format_module_source(unformatted, options.clone()).map_err(|err| {
-                        FormatCommandError::Format(
+                        if let FormatModuleError::ParseError(err) = err {
-                            path.map(Path::to_path_buf),
+                            DisplayParseError::from_source_kind(
-                            source_kind.clone(),
                                 err,
+                                path.map(Path::to_path_buf),
+                                source_kind,
                             )
+                            .into()
+                        } else {
+                            FormatCommandError::Format(path.map(Path::to_path_buf), err)
+                        }
                     })?;
 
                 // If the cell is unchanged, skip it.
@@ -571,9 +584,10 @@ impl<'a> FormatResults<'a> {
 #[derive(Error, Debug)]
 pub(crate) enum FormatCommandError {
     Ignore(#[from] ignore::Error),
+    Parse(#[from] DisplayParseError),
     Panic(Option<PathBuf>, PanicError),
     Read(Option<PathBuf>, SourceError),
-    Format(Option<PathBuf>, SourceKind, FormatModuleError),
+    Format(Option<PathBuf>, FormatModuleError),
     Write(Option<PathBuf>, SourceError),
     Diff(Option<PathBuf>, io::Error),
 }
@@ -588,9 +602,10 @@ impl FormatCommandError {
                     None
                 }
             }
+            Self::Parse(err) => err.path(),
             Self::Panic(path, _)
             | Self::Read(path, _)
-            | Self::Format(path, _, _)
+            | Self::Format(path, _)
             | Self::Write(path, _)
             | Self::Diff(path, _) => path.as_deref(),
         }
@@ -621,6 +636,9 @@ impl Display for FormatCommandError {
                     )
                 }
             }
+            Self::Parse(err) => {
+                write!(f, "{err}")
+            }
             Self::Read(path, err) => {
                 if let Some(path) = path {
                     write!(
@@ -647,22 +665,7 @@ impl Display for FormatCommandError {
                     write!(f, "{}{} {err}", "Failed to write".bold(), ":".bold())
                 }
             }
-            Self::Format(path, source_kind, FormatModuleError::ParseError(err)) => {
+            Self::Format(path, err) => {
-                write!(
-                    f,
-                    "{}",
-                    DisplayParseError::new(
-                        err,
-                        &SourceCode::new(
-                            source_kind.source_code(),
-                            &LineIndex::from_source_text(source_kind.source_code()),
-                        ),
-                        source_kind,
-                        path.as_deref(),
-                    )
-                )
-            }
-            Self::Format(path, _source_kind, err) => {
                 if let Some(path) = path {
                     write!(
                         f,

crates/ruff_cli/src/diagnostics.rs

@@ -10,7 +10,6 @@ use colored::Colorize;
 use log::{debug, error, warn};
 use rustc_hash::FxHashMap;
 
-use crate::cache::{Cache, FileCacheKey, LintCacheData};
 use ruff_diagnostics::Diagnostic;
 use ruff_linter::linter::{lint_fix, lint_only, FixTable, FixerResult, LinterResult, ParseSource};
 use ruff_linter::logging::DisplayParseError;
@@ -24,10 +23,12 @@ use ruff_linter::{fs, IOError, SyntaxError};
 use ruff_notebook::{Notebook, NotebookError, NotebookIndex};
 use ruff_python_ast::imports::ImportMap;
 use ruff_python_ast::{PySourceType, SourceType, TomlSourceType};
-use ruff_source_file::{LineIndex, SourceCode, SourceFileBuilder};
+use ruff_source_file::SourceFileBuilder;
 use ruff_text_size::{TextRange, TextSize};
 use ruff_workspace::Settings;
 
+use crate::cache::{Cache, FileCacheKey, LintCacheData};
+
 #[derive(Debug, Default, PartialEq)]
 pub(crate) struct Diagnostics {
     pub(crate) messages: Vec<Message>,
@@ -356,18 +357,10 @@ pub(crate) fn lint_path(
         }
     }
 
-    if let Some(err) = parse_error {
+    if let Some(error) = parse_error {
         error!(
             "{}",
-            DisplayParseError::new(
+            DisplayParseError::from_source_kind(error, Some(path.to_path_buf()), &source_kind,)
-                &err,
-                &SourceCode::new(
-                    source_kind.source_code(),
-                    &LineIndex::from_source_text(source_kind.source_code())
-                ),
-                &source_kind,
-                Some(path),
-            )
         );
     }
 

crates/ruff_linter/src/linter.rs

@@ -339,7 +339,12 @@ pub fn add_noqa_to_path(
     if let Some(error) = error {
         error!(
             "{}",
-            DisplayParseError::new(&error, &locator.to_source_code(), source_kind, Some(path))
+            DisplayParseError::from_source_code(
+                error,
+                Some(path.to_path_buf()),
+                &locator.to_source_code(),
+                source_kind,
+            )
         );
     }
 

crates/ruff_linter/src/logging.rs

@@ -1,5 +1,5 @@
 use std::fmt::{Display, Formatter, Write};
-use std::path::Path;
+use std::path::{Path, PathBuf};
 use std::sync::Mutex;
 
 use anyhow::Result;
@@ -9,7 +9,7 @@ use log::Level;
 use once_cell::sync::Lazy;
 use ruff_python_parser::{ParseError, ParseErrorType};
 
-use ruff_source_file::{OneIndexed, SourceCode, SourceLocation};
+use ruff_source_file::{LineIndex, OneIndexed, SourceCode, SourceLocation};
 
 use crate::fs;
 use crate::source_kind::SourceKind;
@@ -138,32 +138,76 @@ pub fn set_up_logging(level: &LogLevel) -> Result<()> {
 
 /// A wrapper around [`ParseError`] to translate byte offsets to user-facing
 /// source code locations (typically, line and column numbers).
-pub struct DisplayParseError<'a> {
+#[derive(Debug)]
-    error: &'a ParseError,
+pub struct DisplayParseError {
-    source_code: &'a SourceCode<'a, 'a>,
+    error: ParseError,
-    source_kind: &'a SourceKind,
+    path: Option<PathBuf>,
-    path: Option<&'a Path>,
+    location: ErrorLocation,
 }
 
-impl<'a> DisplayParseError<'a> {
+impl DisplayParseError {
-    pub fn new(
+    /// Create a [`DisplayParseError`] from a [`ParseError`] and a [`SourceKind`].
-        error: &'a ParseError,
+    pub fn from_source_kind(
-        source_code: &'a SourceCode<'a, 'a>,
+        error: ParseError,
-        source_kind: &'a SourceKind,
+        path: Option<PathBuf>,
-        path: Option<&'a Path>,
+        source_kind: &SourceKind,
     ) -> Self {
+        Self::from_source_code(
+            error,
+            path,
+            &SourceCode::new(
+                source_kind.source_code(),
+                &LineIndex::from_source_text(source_kind.source_code()),
+            ),
+            source_kind,
+        )
+    }
+
+    /// Create a [`DisplayParseError`] from a [`ParseError`] and a [`SourceCode`].
+    pub fn from_source_code(
+        error: ParseError,
+        path: Option<PathBuf>,
+        source_code: &SourceCode,
+        source_kind: &SourceKind,
+    ) -> Self {
+        // Translate the byte offset to a location in the originating source.
+        let location =
+            if let Some(jupyter_index) = source_kind.as_ipy_notebook().map(Notebook::index) {
+                let source_location = source_code.source_location(error.offset);
+
+                ErrorLocation::Cell(
+                    jupyter_index
+                        .cell(source_location.row)
+                        .unwrap_or(OneIndexed::MIN),
+                    SourceLocation {
+                        row: jupyter_index
+                            .cell_row(source_location.row)
+                            .unwrap_or(OneIndexed::MIN),
+                        column: source_location.column,
+                    },
+                )
+            } else {
+                ErrorLocation::File(source_code.source_location(error.offset))
+            };
+
         Self {
             error,
-            source_code,
-            source_kind,
             path,
+            location,
         }
     }
+
+    /// Return the path of the file in which the error occurred.
+    pub fn path(&self) -> Option<&Path> {
+        self.path.as_deref()
+    }
 }
 
-impl Display for DisplayParseError<'_> {
+impl std::error::Error for DisplayParseError {}
-    fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
+
-        if let Some(path) = self.path {
+impl Display for DisplayParseError {
+    fn fmt(&self, f: &mut Formatter) -> std::fmt::Result {
+        if let Some(path) = self.path.as_ref() {
             write!(
                 f,
                 "{header} {path}{colon}",
@@ -179,42 +223,30 @@ impl Display for DisplayParseError<'_> {
                 colon = ":".cyan(),
             )?;
         }
-
+        match &self.location {
-        let source_location = self.source_code.source_location(self.error.offset);
+            ErrorLocation::File(location) => {
-
-        // If we're working on a Jupyter notebook, translate the positions
-        // with respect to the cell and row in the cell. This is the same
-        // format as the `TextEmitter`.
-        let error_location =
-            if let Some(jupyter_index) = self.source_kind.as_ipy_notebook().map(Notebook::index) {
-                write!(
-                    f,
-                    "cell {cell}{colon}",
-                    cell = jupyter_index
-                        .cell(source_location.row)
-                        .unwrap_or(OneIndexed::MIN),
-                    colon = ":".cyan(),
-                )?;
-
-                SourceLocation {
-                    row: jupyter_index
-                        .cell_row(source_location.row)
-                        .unwrap_or(OneIndexed::MIN),
-                    column: source_location.column,
-                }
-            } else {
-                source_location
-            };
-
                 write!(
                     f,
                     "{row}{colon}{column}{colon} {inner}",
-            row = error_location.row,
+                    row = location.row,
-            column = error_location.column,
+                    column = location.column,
                     colon = ":".cyan(),
                     inner = &DisplayParseErrorType(&self.error.error)
                 )
             }
+            ErrorLocation::Cell(cell, location) => {
+                write!(
+                    f,
+                    "{cell}{colon}{row}{colon}{column}{colon} {inner}",
+                    cell = cell,
+                    row = location.row,
+                    column = location.column,
+                    colon = ":".cyan(),
+                    inner = &DisplayParseErrorType(&self.error.error)
+                )
+            }
+        }
+    }
 }
 
 pub(crate) struct DisplayParseErrorType<'a>(&'a ParseErrorType);
@@ -251,6 +283,14 @@ impl Display for DisplayParseErrorType<'_> {
     }
 }
 
+#[derive(Debug)]
+enum ErrorLocation {
+    /// The error occurred in a Python file.
+    File(SourceLocation),
+    /// The error occurred in a Jupyter cell.
+    Cell(OneIndexed, SourceLocation),
+}
+
 /// Truncates the display text before the first newline character to avoid line breaks.
 struct TruncateAtNewline<'a>(&'a dyn Display);