From d97d4541e0581a3e70ab88692ebe367451709088 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Sun, 1 Oct 2023 20:02:51 -0400 Subject: [PATCH] Remove LintSource --- crates/ruff_cli/src/commands/add_noqa.rs | 6 +- crates/ruff_cli/src/commands/format.rs | 103 +++++++++----- crates/ruff_cli/src/commands/format_stdin.rs | 37 +++-- crates/ruff_cli/src/diagnostics.rs | 134 ++++++------------- crates/ruff_cli/tests/integration_test.rs | 4 +- crates/ruff_linter/src/source_kind.rs | 44 +++--- 6 files changed, 158 insertions(+), 170 deletions(-) diff --git a/crates/ruff_cli/src/commands/add_noqa.rs b/crates/ruff_cli/src/commands/add_noqa.rs index 513f4ba2487ca..1fd8f5425f979 100644 --- a/crates/ruff_cli/src/commands/add_noqa.rs +++ b/crates/ruff_cli/src/commands/add_noqa.rs @@ -7,12 +7,12 @@ use log::{debug, error}; use rayon::prelude::*; use ruff_linter::linter::add_noqa_to_path; +use ruff_linter::source_kind::SourceKind; use ruff_linter::warn_user_once; use ruff_python_ast::{PySourceType, SourceType}; use ruff_workspace::resolver::{python_files_in_path, PyprojectConfig}; use crate::args::CliOverrides; -use crate::diagnostics::LintSource; /// Add `noqa` directives to a collection of files. pub(crate) fn add_noqa( @@ -57,8 +57,8 @@ pub(crate) fn add_noqa( .and_then(|parent| package_roots.get(parent)) .and_then(|package| *package); let settings = resolver.resolve(path, pyproject_config); - let LintSource(source_kind) = match LintSource::try_from_path(path, source_type) { - Ok(Some(source)) => source, + let source_kind = match SourceKind::from_path(path, source_type) { + Ok(Some(source_kind)) => source_kind, Ok(None) => return None, Err(e) => { error!("Failed to extract source from {}: {e}", path.display()); diff --git a/crates/ruff_cli/src/commands/format.rs b/crates/ruff_cli/src/commands/format.rs index 7d2ad4144f056..42776e8773043 100644 --- a/crates/ruff_cli/src/commands/format.rs +++ b/crates/ruff_cli/src/commands/format.rs @@ -15,7 +15,7 @@ use tracing::debug; use ruff_diagnostics::SourceMap; use ruff_linter::fs; use ruff_linter::logging::LogLevel; -use ruff_linter::source_kind::{SourceKind, SourceReadError, SourceWriteError}; +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}; @@ -147,64 +147,99 @@ fn format_path( mode: FormatMode, ) -> Result { // Extract the sources from the file. - let source_kind = SourceKind::from_path(path, source_type) - .map_err(|err| FormatCommandError::Read(Some(path.to_path_buf()), err))?; + let source_kind = match SourceKind::from_path(path, source_type) { + Ok(Some(source_kind)) => source_kind, + Ok(None) => return Ok(FormatCommandResult::Unchanged), + Err(err) => { + return Err(FormatCommandError::Read(Some(path.to_path_buf()), err)); + } + }; // Format the source. - if let Some(source_kind) = format_source(&source_kind, source_type, Some(path), settings)? { - if mode.is_write() { - let mut writer = File::create(path) - .map_err(|err| FormatCommandError::Write(Some(path.to_path_buf()), err.into()))?; - source_kind - .write(&mut writer) - .map_err(|err| FormatCommandError::Write(Some(path.to_path_buf()), err))?; + match format_source(source_kind, source_type, Some(path), settings)? { + FormattedSource::Formatted(formatted) => { + if mode.is_write() { + let mut writer = File::create(path).map_err(|err| { + FormatCommandError::Write(Some(path.to_path_buf()), err.into()) + })?; + formatted + .write(&mut writer) + .map_err(|err| FormatCommandError::Write(Some(path.to_path_buf()), err))?; + } + Ok(FormatCommandResult::Formatted) + } + FormattedSource::Unchanged(_) => Ok(FormatCommandResult::Unchanged), + } +} + +#[derive(Debug)] +pub(crate) enum FormattedSource { + /// The source was formatted, and the [`SourceKind`] contains the transformed source code. + Formatted(SourceKind), + /// The source was unchanged, and the [`SourceKind`] contains the original source code. + Unchanged(SourceKind), +} + +impl From for FormatCommandResult { + fn from(value: FormattedSource) -> Self { + match value { + FormattedSource::Formatted(_) => FormatCommandResult::Formatted, + FormattedSource::Unchanged(_) => FormatCommandResult::Unchanged, + } + } +} + +impl FormattedSource { + pub(crate) fn source_kind(&self) -> &SourceKind { + match self { + FormattedSource::Formatted(source_kind) => source_kind, + FormattedSource::Unchanged(source_kind) => source_kind, } - Ok(FormatCommandResult::Formatted) - } else { - Ok(FormatCommandResult::Unchanged) } } /// Format a [`SourceKind`], returning the transformed [`SourceKind`], or `None` if the source was /// unchanged. pub(crate) fn format_source( - source_kind: &SourceKind, + source_kind: SourceKind, source_type: PySourceType, path: Option<&Path>, settings: &FormatterSettings, -) -> Result, FormatCommandError> { +) -> Result { match source_kind { SourceKind::Python(unformatted) => { - let options = settings.to_format_options(source_type, unformatted); + let options = settings.to_format_options(source_type, &unformatted); - let formatted = format_module_source(unformatted, options) + let formatted = format_module_source(&unformatted, options) .map_err(|err| FormatCommandError::Format(path.map(Path::to_path_buf), err))?; let formatted = formatted.into_code(); if formatted.len() == unformatted.len() && formatted == *unformatted { - Ok(None) + Ok(FormattedSource::Unchanged(SourceKind::Python(unformatted))) } else { - Ok(Some(SourceKind::Python(formatted))) + Ok(FormattedSource::Formatted(SourceKind::Python(formatted))) } } SourceKind::IpyNotebook(notebook) => { if !notebook.is_python_notebook() { - return Ok(None); + return Ok(FormattedSource::Unchanged(SourceKind::IpyNotebook( + notebook, + ))); } - let mut output = String::with_capacity(notebook.source_code().len()); - let mut source_map = SourceMap::default(); + let options = settings.to_format_options(source_type, notebook.source_code()); + + let mut output: Option = None; let mut last: Option = None; + let mut source_map = SourceMap::default(); // Format each cell individually. for (start, end) in notebook.cell_offsets().iter().tuple_windows::<(_, _)>() { let range = TextRange::new(*start, *end); let unformatted = ¬ebook.source_code()[range]; - let options = settings.to_format_options(source_type, unformatted); - // Format the cell. - let formatted = format_module_source(unformatted, options) + let formatted = format_module_source(unformatted, options.clone()) .map_err(|err| FormatCommandError::Format(path.map(Path::to_path_buf), err))?; // If the cell is unchanged, skip it. @@ -213,6 +248,10 @@ pub(crate) fn format_source( continue; } + // If this is the first newly-formatted cell, initialize the output. + let output = output + .get_or_insert_with(|| String::with_capacity(notebook.source_code().len())); + // Add all contents from `last` to the current cell. let slice = ¬ebook.source_code() [TextRange::new(last.unwrap_or_default(), range.start())]; @@ -232,8 +271,10 @@ pub(crate) fn format_source( } // If the file was unchanged, return `None`. - let Some(last) = last else { - return Ok(None); + let (Some(mut output), Some(last)) = (output, last) else { + return Ok(FormattedSource::Unchanged(SourceKind::IpyNotebook( + notebook, + ))); }; // Add the remaining content. @@ -244,7 +285,9 @@ pub(crate) fn format_source( let mut notebook = notebook.clone(); notebook.update(&source_map, output); - Ok(Some(SourceKind::IpyNotebook(notebook))) + Ok(FormattedSource::Formatted(SourceKind::IpyNotebook( + notebook, + ))) } } } @@ -328,9 +371,9 @@ impl Display for FormatResultSummary { pub(crate) enum FormatCommandError { Ignore(#[from] ignore::Error), Panic(Option, PanicError), - Read(Option, SourceReadError), + Read(Option, SourceError), Format(Option, FormatModuleError), - Write(Option, SourceWriteError), + Write(Option, SourceError), } impl Display for FormatCommandError { diff --git a/crates/ruff_cli/src/commands/format_stdin.rs b/crates/ruff_cli/src/commands/format_stdin.rs index d7c34684be185..a790754d2003b 100644 --- a/crates/ruff_cli/src/commands/format_stdin.rs +++ b/crates/ruff_cli/src/commands/format_stdin.rs @@ -74,28 +74,27 @@ fn format_source_code( ) -> Result { // Read the source from stdin. let source_code = read_from_stdin() - .map_err(|err| FormatCommandError::Read(path.map(Into::into), err.into()))?; - let source_kind = SourceKind::from_source_code(source_code, source_type) - .map_err(|err| FormatCommandError::Read(path.map(Into::into), err))?; + .map_err(|err| FormatCommandError::Read(path.map(Path::to_path_buf), err.into()))?; - // Format the source, and write to stdout regardless of the mode. - if let Some(formatted) = format_source(&source_kind, source_type, path, settings)? { - if mode.is_write() { - let mut writer = stdout().lock(); - formatted - .write(&mut writer) - .map_err(|err| FormatCommandError::Write(path.map(Into::into), err))?; + let source_kind = match SourceKind::from_source_code(source_code, source_type) { + Ok(Some(source_kind)) => source_kind, + Ok(None) => return Ok(FormatCommandResult::Unchanged), + Err(err) => { + return Err(FormatCommandError::Read(path.map(Path::to_path_buf), err)); } + }; - Ok(FormatCommandResult::Formatted) - } else { - if mode.is_write() { - let mut writer = stdout().lock(); - source_kind - .write(&mut writer) - .map_err(|err| FormatCommandError::Write(path.map(Into::into), err))?; - } + // Format the source. + let formatted = format_source(source_kind, source_type, path, settings)?; - Ok(FormatCommandResult::Unchanged) + // Write to stdout regardless of whether the source was formatted. + if mode.is_write() { + let mut writer = stdout().lock(); + formatted + .source_kind() + .write(&mut writer) + .map_err(|err| FormatCommandError::Write(path.map(Path::to_path_buf), err))?; } + + Ok(FormatCommandResult::from(formatted)) } diff --git a/crates/ruff_cli/src/diagnostics.rs b/crates/ruff_cli/src/diagnostics.rs index fc49b0bfe648b..d3dea9f56aeba 100644 --- a/crates/ruff_cli/src/diagnostics.rs +++ b/crates/ruff_cli/src/diagnostics.rs @@ -14,7 +14,6 @@ use filetime::FileTime; use log::{debug, error, warn}; use rustc_hash::FxHashMap; use similar::TextDiff; -use thiserror::Error; use ruff_diagnostics::Diagnostic; use ruff_linter::linter::{lint_fix, lint_only, FixTable, FixerResult, LinterResult}; @@ -23,12 +22,12 @@ use ruff_linter::message::Message; use ruff_linter::pyproject_toml::lint_pyproject_toml; use ruff_linter::registry::AsRule; use ruff_linter::settings::{flags, LinterSettings}; -use ruff_linter::source_kind::SourceKind; +use ruff_linter::source_kind::{SourceError, SourceKind}; use ruff_linter::{fs, IOError, SyntaxError}; use ruff_macros::CacheKey; use ruff_notebook::{Cell, Notebook, NotebookError, NotebookIndex}; use ruff_python_ast::imports::ImportMap; -use ruff_python_ast::{PySourceType, SourceType, TomlSourceType}; +use ruff_python_ast::{SourceType, TomlSourceType}; use ruff_source_file::{LineIndex, SourceCode, SourceFileBuilder}; use ruff_text_size::{TextRange, TextSize}; use ruff_workspace::Settings; @@ -82,15 +81,38 @@ impl Diagnostics { } } - /// Generate [`Diagnostics`] based on a [`SourceExtractionError`]. + /// Generate [`Diagnostics`] based on a [`SourceError`]. pub(crate) fn from_source_error( - err: &SourceExtractionError, + err: &SourceError, path: Option<&Path>, settings: &LinterSettings, ) -> Self { - let diagnostic = Diagnostic::from(err); + let diagnostic = match err { + // IO errors. + SourceError::Io(_) + | SourceError::Notebook(NotebookError::Io(_) | NotebookError::Json(_)) => { + Diagnostic::new( + IOError { + message: err.to_string(), + }, + TextRange::default(), + ) + } + // Syntax errors. + SourceError::Notebook( + NotebookError::InvalidJson(_) + | NotebookError::InvalidSchema(_) + | NotebookError::InvalidFormat(_), + ) => Diagnostic::new( + SyntaxError { + message: err.to_string(), + }, + TextRange::default(), + ), + }; + if settings.rules.enabled(diagnostic.kind.rule()) { - let name = path.map_or_else(|| "-".into(), std::path::Path::to_string_lossy); + let name = path.map_or_else(|| "-".into(), Path::to_string_lossy); let dummy = SourceFileBuilder::new(name, "").finish(); Self::new( vec![Message::from_diagnostic( @@ -183,13 +205,12 @@ pub(crate) fn lint_path( .iter_enabled() .any(|rule_code| rule_code.lint_source().is_pyproject_toml()) { - let contents = - match std::fs::read_to_string(path).map_err(SourceExtractionError::Io) { - Ok(contents) => contents, - Err(err) => { - return Ok(Diagnostics::from_source_error(&err, Some(path), settings)); - } - }; + let contents = match std::fs::read_to_string(path).map_err(SourceError::from) { + Ok(contents) => contents, + Err(err) => { + return Ok(Diagnostics::from_source_error(&err, Some(path), settings)); + } + }; let source_file = SourceFileBuilder::new(path.to_string_lossy(), contents).finish(); lint_pyproject_toml(source_file, settings) } else { @@ -205,8 +226,8 @@ pub(crate) fn lint_path( }; // Extract the sources from the file. - let LintSource(source_kind) = match LintSource::try_from_path(path, source_type) { - Ok(Some(sources)) => sources, + let source_kind = match SourceKind::from_path(path, source_type) { + Ok(Some(source_kind)) => source_kind, Ok(None) => return Ok(Diagnostics::default()), Err(err) => { return Ok(Diagnostics::from_source_error(&err, Some(path), settings)); @@ -371,8 +392,8 @@ pub(crate) fn lint_stdin( }; // Extract the sources from the file. - let LintSource(source_kind) = match LintSource::try_from_source_code(contents, source_type) { - Ok(Some(sources)) => sources, + let source_kind = match SourceKind::from_source_code(contents, source_type) { + Ok(Some(source_kind)) => source_kind, Ok(None) => return Ok(Diagnostics::default()), Err(err) => { return Ok(Diagnostics::from_source_error(&err, path, &settings.linter)); @@ -487,80 +508,3 @@ pub(crate) fn lint_stdin( notebook_indexes, }) } - -#[derive(Debug)] -pub(crate) struct LintSource(pub(crate) SourceKind); - -impl LintSource { - /// Extract the lint [`LintSource`] from the given file path. - pub(crate) fn try_from_path( - path: &Path, - source_type: PySourceType, - ) -> Result, SourceExtractionError> { - if source_type.is_ipynb() { - let notebook = Notebook::from_path(path)?; - Ok(notebook - .is_python_notebook() - .then_some(LintSource(SourceKind::IpyNotebook(notebook)))) - } else { - // This is tested by ruff_cli integration test `unreadable_file` - let contents = std::fs::read_to_string(path)?; - Ok(Some(LintSource(SourceKind::Python(contents)))) - } - } - - /// Extract the lint [`LintSource`] from the raw string contents, optionally accompanied by a - /// file path indicating the path to the file from which the contents were read. If provided, - /// the file path should be used for diagnostics, but not for reading the file from disk. - pub(crate) fn try_from_source_code( - source_code: String, - source_type: PySourceType, - ) -> Result, SourceExtractionError> { - if source_type.is_ipynb() { - let notebook = Notebook::from_source_code(&source_code)?; - Ok(notebook - .is_python_notebook() - .then_some(LintSource(SourceKind::IpyNotebook(notebook)))) - } else { - Ok(Some(LintSource(SourceKind::Python(source_code)))) - } - } -} - -#[derive(Error, Debug)] -pub(crate) enum SourceExtractionError { - /// The extraction failed due to an [`io::Error`]. - #[error(transparent)] - Io(#[from] io::Error), - /// The extraction failed due to a [`NotebookError`]. - #[error(transparent)] - Notebook(#[from] NotebookError), -} - -impl From<&SourceExtractionError> for Diagnostic { - fn from(err: &SourceExtractionError) -> Self { - match err { - // IO errors. - SourceExtractionError::Io(_) - | SourceExtractionError::Notebook(NotebookError::Io(_) | NotebookError::Json(_)) => { - Diagnostic::new( - IOError { - message: err.to_string(), - }, - TextRange::default(), - ) - } - // Syntax errors. - SourceExtractionError::Notebook( - NotebookError::InvalidJson(_) - | NotebookError::InvalidSchema(_) - | NotebookError::InvalidFormat(_), - ) => Diagnostic::new( - SyntaxError { - message: err.to_string(), - }, - TextRange::default(), - ), - } - } -} diff --git a/crates/ruff_cli/tests/integration_test.rs b/crates/ruff_cli/tests/integration_test.rs index 9ddae258c6c61..a47a974d8175b 100644 --- a/crates/ruff_cli/tests/integration_test.rs +++ b/crates/ruff_cli/tests/integration_test.rs @@ -359,7 +359,7 @@ fn stdin_fix_when_no_issues_should_still_print_contents() { #[test] fn stdin_format_jupyter() { - let args = ["format", "--stdin-filename", "Jupyter.ipynb"]; + let args = ["format", "--stdin-filename", "Jupyter.ipynb", "--isolated"]; assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME)) .args(args) .pass_stdin(r#"{ @@ -483,8 +483,6 @@ fn stdin_format_jupyter() { ----- stderr ----- warning: `ruff format` is a work-in-progress, subject to change at any time, and intended only for experimentation. - warning: `one-blank-line-before-class` (D203) and `no-blank-line-before-class` (D211) are incompatible. Ignoring `one-blank-line-before-class`. - warning: `multi-line-summary-first-line` (D212) and `multi-line-summary-second-line` (D213) are incompatible. Ignoring `multi-line-summary-second-line`. "###); } diff --git a/crates/ruff_linter/src/source_kind.rs b/crates/ruff_linter/src/source_kind.rs index 6f41b42038a48..76982451d33bd 100644 --- a/crates/ruff_linter/src/source_kind.rs +++ b/crates/ruff_linter/src/source_kind.rs @@ -38,51 +38,55 @@ impl SourceKind { } } - /// Read the source kind from the given path. - pub fn from_path(path: &Path, source_type: PySourceType) -> Result { + /// Read the [`SourceKind`] from the given path. Returns `None` if the source is not a Python + /// source file. + pub fn from_path(path: &Path, source_type: PySourceType) -> Result, SourceError> { if source_type.is_ipynb() { let notebook = Notebook::from_path(path)?; - Ok(Self::IpyNotebook(notebook)) + Ok(notebook + .is_python_notebook() + .then_some(Self::IpyNotebook(notebook))) } else { let contents = std::fs::read_to_string(path)?; - Ok(Self::Python(contents)) + Ok(Some(Self::Python(contents))) } } - /// Read the source kind from the given source code. + /// Read the [`SourceKind`] from the given source code. Returns `None` if the source is not + /// Python source code. pub fn from_source_code( source_code: String, source_type: PySourceType, - ) -> Result { + ) -> Result, SourceError> { if source_type.is_ipynb() { let notebook = Notebook::from_source_code(&source_code)?; - Ok(Self::IpyNotebook(notebook)) + Ok(notebook + .is_python_notebook() + .then_some(Self::IpyNotebook(notebook))) } else { - Ok(Self::Python(source_code)) + Ok(Some(Self::Python(source_code))) } } /// Write the transformed source file to the given writer. /// /// For Jupyter notebooks, this will write out the notebook as JSON. - pub fn write(&self, writer: &mut dyn Write) -> Result<(), SourceWriteError> { + pub fn write(&self, writer: &mut dyn Write) -> Result<(), SourceError> { match self { - SourceKind::Python(source) => writer.write_all(source.as_bytes()).map_err(Into::into), - SourceKind::IpyNotebook(notebook) => notebook.write(writer).map_err(Into::into), + SourceKind::Python(source) => { + writer.write_all(source.as_bytes())?; + Ok(()) + } + SourceKind::IpyNotebook(notebook) => { + notebook.write(writer)?; + Ok(()) + } } } } #[derive(Error, Debug)] -pub enum SourceReadError { - #[error(transparent)] - Io(#[from] io::Error), - #[error(transparent)] - Notebook(#[from] NotebookError), -} - -#[derive(Error, Debug)] -pub enum SourceWriteError { +pub enum SourceError { #[error(transparent)] Io(#[from] io::Error), #[error(transparent)]