From da8a3af52409a5cd5034060a482ae93ac6abc27f Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Sun, 31 Dec 2023 11:46:29 -0400 Subject: [PATCH] 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 | 6 +- crates/ruff_cli/src/commands/format.rs | 53 +++++----- crates/ruff_cli/src/diagnostics.rs | 17 +-- crates/ruff_linter/src/linter.rs | 7 +- crates/ruff_linter/src/logging.rs | 140 ++++++++++++++++--------- 5 files changed, 132 insertions(+), 91 deletions(-) diff --git a/crates/ruff_cli/Cargo.toml b/crates/ruff_cli/Cargo.toml index 17f71251f37ca..ff3efae87f846 100644 --- a/crates/ruff_cli/Cargo.toml +++ b/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" } diff --git a/crates/ruff_cli/src/commands/format.rs b/crates/ruff_cli/src/commands/format.rs index 8299c05f6bf8b..096cfeb43760c 100644 --- a/crates/ruff_cli/src/commands/format.rs +++ b/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( - 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) + } })?; // 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, PanicError), Read(Option, SourceError), - Format(Option, SourceKind, FormatModuleError), + Format(Option, FormatModuleError), Write(Option, SourceError), Diff(Option, 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)) => { - 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) => { + Self::Format(path, err) => { if let Some(path) = path { write!( f, diff --git a/crates/ruff_cli/src/diagnostics.rs b/crates/ruff_cli/src/diagnostics.rs index a1bbf5c171829..3acf37b5ccc51 100644 --- a/crates/ruff_cli/src/diagnostics.rs +++ b/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, @@ -356,18 +357,10 @@ pub(crate) fn lint_path( } } - if let Some(err) = parse_error { + if let Some(error) = parse_error { error!( "{}", - DisplayParseError::new( - &err, - &SourceCode::new( - source_kind.source_code(), - &LineIndex::from_source_text(source_kind.source_code()) - ), - &source_kind, - Some(path), - ) + DisplayParseError::from_source_kind(error, Some(path.to_path_buf()), &source_kind,) ); } diff --git a/crates/ruff_linter/src/linter.rs b/crates/ruff_linter/src/linter.rs index ba08ef2399991..9ce637f3c4835 100644 --- a/crates/ruff_linter/src/linter.rs +++ b/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, + ) ); } diff --git a/crates/ruff_linter/src/logging.rs b/crates/ruff_linter/src/logging.rs index 46b8485332744..0e0db735c921c 100644 --- a/crates/ruff_linter/src/logging.rs +++ b/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> { - error: &'a ParseError, - source_code: &'a SourceCode<'a, 'a>, - source_kind: &'a SourceKind, - path: Option<&'a Path>, +#[derive(Debug)] +pub struct DisplayParseError { + error: ParseError, + path: Option, + location: ErrorLocation, } -impl<'a> DisplayParseError<'a> { - pub fn new( - error: &'a ParseError, - source_code: &'a SourceCode<'a, 'a>, - source_kind: &'a SourceKind, - path: Option<&'a Path>, +impl DisplayParseError { + /// Create a [`DisplayParseError`] from a [`ParseError`] and a [`SourceKind`]. + pub fn from_source_kind( + error: ParseError, + path: Option, + source_kind: &SourceKind, ) -> Self { - Self { + Self::from_source_code( error, - source_code, + 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, + 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, 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<'_> { - fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { - if let Some(path) = self.path { +impl std::error::Error for DisplayParseError {} + +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,41 +223,29 @@ impl Display for DisplayParseError<'_> { colon = ":".cyan(), )?; } - - let source_location = self.source_code.source_location(self.error.offset); - - // 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) { + match &self.location { + ErrorLocation::File(location) => { write!( f, - "cell {cell}{colon}", - cell = jupyter_index - .cell(source_location.row) - .unwrap_or(OneIndexed::MIN), + "{row}{colon}{column}{colon} {inner}", + row = location.row, + column = location.column, 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, - column = error_location.column, - colon = ":".cyan(), - inner = &DisplayParseErrorType(&self.error.error) - ) + 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) + ) + } + } } } @@ -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);