From 4ea7ff56c31ff5b7ee61143c3820229b5b7e7932 Mon Sep 17 00:00:00 2001 From: Dhruv Manilawala Date: Mon, 17 Jun 2024 17:26:08 +0530 Subject: [PATCH] Simplify `LinterResult`, avoid cloning `ParseError` --- crates/ruff/src/diagnostics.rs | 8 +- crates/ruff_benchmark/benches/linter.rs | 2 +- crates/ruff_linter/src/linter.rs | 74 +++++++++---------- crates/ruff_linter/src/rules/pyflakes/mod.rs | 7 +- crates/ruff_linter/src/test.rs | 18 ++--- crates/ruff_server/src/fix.rs | 8 +- crates/ruff_server/src/lint.rs | 31 ++++---- .../ruff_server/src/server/api/diagnostics.rs | 6 +- crates/ruff_wasm/src/lib.rs | 6 +- fuzz/fuzz_targets/ruff_formatter_validity.rs | 16 ++-- 10 files changed, 80 insertions(+), 96 deletions(-) diff --git a/crates/ruff/src/diagnostics.rs b/crates/ruff/src/diagnostics.rs index 66f551a286a55..3d4f8b9df88e9 100644 --- a/crates/ruff/src/diagnostics.rs +++ b/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, diff --git a/crates/ruff_benchmark/benches/linter.rs b/crates/ruff_benchmark/benches/linter.rs index 1301d9e7cc179..e3e70ed423be0 100644 --- a/crates/ruff_benchmark/benches/linter.rs +++ b/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, ); diff --git a/crates/ruff_linter/src/linter.rs b/crates/ruff_linter/src/linter.rs index 6c1f555c1bf3b..d5ee5febccabc 100644 --- a/crates/ruff_linter/src/linter.rs +++ b/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 { - pub data: T, - pub error: Option, -} - -impl LinterResult { - const fn new(data: T, error: Option) -> Self { - Self { data, error } - } - - fn map U>(self, f: F) -> LinterResult { - LinterResult::new(f(self.data), self.error) - } +pub struct LinterResult { + /// A collection of diagnostic messages generated by the linter. + pub messages: Vec, + /// 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; pub struct FixerResult<'a> { /// The result returned by the linter, after applying any fixes. - pub result: LinterResult>, + 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, -) -> LinterResult> { +) -> Vec { // 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> { +) -> 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, }); diff --git a/crates/ruff_linter/src/rules/pyflakes/mod.rs b/crates/ruff_linter/src/rules/pyflakes/mod.rs index f9c2d331f60f5..887926720b713 100644 --- a/crates/ruff_linter/src/rules/pyflakes/mod.rs +++ b/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(""), None, &locator, diff --git a/crates/ruff_linter/src/test.rs b/crates/ruff_linter/src/test.rs index 31fae2bdaa887..1f8803f7b7712 100644 --- a/crates/ruff_linter/src/test.rs +++ b/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!( diff --git a/crates/ruff_server/src/fix.rs b/crates/ruff_server/src/fix.rs index 03dcc2980c52d..6992f692bc705 100644 --- a/crates/ruff_server/src/fix.rs +++ b/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 diff --git a/crates/ruff_server/src/lint.rs b/crates/ruff_server/src/lint.rs index 294ded142e4ed..8984d6e2c7309 100644 --- a/crates/ruff_server/src/lint.rs +++ b/crates/ruff_server/src/lint.rs @@ -7,7 +7,7 @@ use ruff_diagnostics::{Applicability, Diagnostic, DiagnosticKind, Edit, Fix}; use ruff_linter::{ directives::{extract_directives, Flags}, generate_noqa_edits, - linter::{check_path, LinterResult}, + linter::check_path, packaging::detect_package_root, registry::AsRule, settings::flags, @@ -57,9 +57,9 @@ pub(crate) struct DiagnosticFix { } /// A series of diagnostics across a single text document or an arbitrary number of notebook cells. -pub(crate) type Diagnostics = FxHashMap>; +pub(crate) type DiagnosticsMap = FxHashMap>; -pub(crate) fn check(query: &DocumentQuery, encoding: PositionEncoding) -> Diagnostics { +pub(crate) fn check(query: &DocumentQuery, encoding: PositionEncoding) -> DiagnosticsMap { let source_kind = query.make_source_kind(); let file_resolver_settings = query.settings().file_resolver(); let linter_settings = query.settings().linter(); @@ -79,7 +79,7 @@ pub(crate) fn check(query: &DocumentQuery, encoding: PositionEncoding) -> Diagno exclusion, document_path.display() ); - return Diagnostics::default(); + return DiagnosticsMap::default(); } detect_package_root( @@ -112,7 +112,7 @@ pub(crate) fn check(query: &DocumentQuery, encoding: PositionEncoding) -> Diagno let directives = extract_directives(parsed.tokens(), Flags::all(), &locator, &indexer); // Generate checks. - let LinterResult { data, .. } = check_path( + let diagnostics = check_path( &query.virtual_file_path(), package, &locator, @@ -128,7 +128,7 @@ pub(crate) fn check(query: &DocumentQuery, encoding: PositionEncoding) -> Diagno let noqa_edits = generate_noqa_edits( &query.virtual_file_path(), - data.as_slice(), + &diagnostics, &locator, indexer.comment_ranges(), &linter_settings.external, @@ -136,19 +136,21 @@ pub(crate) fn check(query: &DocumentQuery, encoding: PositionEncoding) -> Diagno stylist.line_ending(), ); - let mut diagnostics = Diagnostics::default(); + let mut diagnostics_map = DiagnosticsMap::default(); // Populates all relevant URLs with an empty diagnostic list. // This ensures that documents without diagnostics still get updated. if let Some(notebook) = query.as_notebook() { for url in notebook.urls() { - diagnostics.entry(url.clone()).or_default(); + diagnostics_map.entry(url.clone()).or_default(); } } else { - diagnostics.entry(query.make_key().into_url()).or_default(); + diagnostics_map + .entry(query.make_key().into_url()) + .or_default(); } - let lsp_diagnostics = data + let lsp_diagnostics = diagnostics .into_iter() .zip(noqa_edits) .map(|(diagnostic, noqa_edit)| { @@ -161,18 +163,21 @@ pub(crate) fn check(query: &DocumentQuery, encoding: PositionEncoding) -> Diagno tracing::warn!("Unable to find notebook cell at index {index}."); continue; }; - diagnostics.entry(uri.clone()).or_default().push(diagnostic); + diagnostics_map + .entry(uri.clone()) + .or_default() + .push(diagnostic); } } else { for (_, diagnostic) in lsp_diagnostics { - diagnostics + diagnostics_map .entry(query.make_key().into_url()) .or_default() .push(diagnostic); } } - diagnostics + diagnostics_map } /// Converts LSP diagnostics to a list of `DiagnosticFix`es by deserializing associated data on each diagnostic. diff --git a/crates/ruff_server/src/server/api/diagnostics.rs b/crates/ruff_server/src/server/api/diagnostics.rs index a9bb509f3a159..950827ca14126 100644 --- a/crates/ruff_server/src/server/api/diagnostics.rs +++ b/crates/ruff_server/src/server/api/diagnostics.rs @@ -1,17 +1,17 @@ use crate::{ - lint::Diagnostics, + lint::DiagnosticsMap, server::client::Notifier, session::{DocumentQuery, DocumentSnapshot}, }; use super::LSPResult; -pub(super) fn generate_diagnostics(snapshot: &DocumentSnapshot) -> Diagnostics { +pub(super) fn generate_diagnostics(snapshot: &DocumentSnapshot) -> DiagnosticsMap { if snapshot.client_settings().lint() { let document = snapshot.query(); crate::lint::check(document, snapshot.encoding()) } else { - Diagnostics::default() + DiagnosticsMap::default() } } diff --git a/crates/ruff_wasm/src/lib.rs b/crates/ruff_wasm/src/lib.rs index e2b86508a205f..7d151b20e1958 100644 --- a/crates/ruff_wasm/src/lib.rs +++ b/crates/ruff_wasm/src/lib.rs @@ -9,7 +9,7 @@ use ruff_formatter::printer::SourceMapGeneration; use ruff_formatter::{FormatResult, Formatted, IndentStyle}; use ruff_linter::directives; use ruff_linter::line_width::{IndentWidth, LineLength}; -use ruff_linter::linter::{check_path, LinterResult}; +use ruff_linter::linter::check_path; use ruff_linter::registry::AsRule; use ruff_linter::settings::types::PythonVersion; use ruff_linter::settings::{flags, DEFAULT_SELECTORS, DUMMY_VARIABLE_RGX}; @@ -181,9 +181,7 @@ impl Workspace { ); // Generate checks. - let LinterResult { - data: diagnostics, .. - } = check_path( + let diagnostics = check_path( Path::new(""), None, &locator, diff --git a/fuzz/fuzz_targets/ruff_formatter_validity.rs b/fuzz/fuzz_targets/ruff_formatter_validity.rs index 3f8f7d886d4c1..e50d4b3b01f86 100644 --- a/fuzz/fuzz_targets/ruff_formatter_validity.rs +++ b/fuzz/fuzz_targets/ruff_formatter_validity.rs @@ -27,23 +27,23 @@ fn do_fuzz(case: &[u8]) -> Corpus { let linter_settings = SETTINGS.get_or_init(LinterSettings::default); let format_options = PyFormatOptions::default(); - let linter_results = ruff_linter::linter::lint_only( + let linter_result = ruff_linter::linter::lint_only( "fuzzed-source.py".as_ref(), None, - &linter_settings, + linter_settings, Noqa::Enabled, &SourceKind::Python(code.to_string()), PySourceType::Python, ParseSource::None, ); - if linter_results.error.is_some() { + if linter_result.has_error { return Corpus::Keep; // keep, but don't continue } let mut warnings = HashMap::new(); - for msg in linter_results.data { + for msg in linter_result.messages { let count: &mut usize = warnings.entry(msg.kind.name).or_default(); *count += 1; } @@ -55,7 +55,7 @@ fn do_fuzz(case: &[u8]) -> Corpus { let linter_results = ruff_linter::linter::lint_only( "fuzzed-source.py".as_ref(), None, - &linter_settings, + linter_settings, Noqa::Enabled, &SourceKind::Python(formatted.clone()), PySourceType::Python, @@ -63,11 +63,11 @@ fn do_fuzz(case: &[u8]) -> Corpus { ); assert!( - linter_results.error.is_none(), + !linter_results.has_error, "formatter introduced a parse error" ); - for msg in linter_results.data { + for msg in linter_results.messages { if let Some(count) = warnings.get_mut(&msg.kind.name) { if let Some(decremented) = count.checked_sub(1) { *count = decremented; @@ -77,7 +77,6 @@ fn do_fuzz(case: &[u8]) -> Corpus { TextDiff::from_lines(code, &formatted) .unified_diff() .header("Unformatted", "Formatted") - .to_string() ); } } else { @@ -86,7 +85,6 @@ fn do_fuzz(case: &[u8]) -> Corpus { TextDiff::from_lines(code, &formatted) .unified_diff() .header("Unformatted", "Formatted") - .to_string() ); } }