From 74f64d3f96d76fc252512acf30cf45b252be58f7 Mon Sep 17 00:00:00 2001 From: Dylan Date: Thu, 20 Mar 2025 05:09:14 -0500 Subject: [PATCH] Server: Allow `FixAll` action in presence of version-specific syntax errors (#16848) The single flag `has_syntax_error` on `LinterResult` is replaced with two (private) flags: `has_valid_syntax` and `has_no_unsupported_syntax_errors`, which record whether there are `ParseError`s or `UnsupportedSyntaxError`s, respectively. Only the former is used to prevent a `FixAll` action. An attempt has been made to make consistent the usage of the phrases "valid syntax" (which seems to be used to refer only to _parser_ errors) and "syntax error" (which refers to both _parser_ errors and version-specific syntax errors). Closes #16841 --- crates/ruff/src/diagnostics.rs | 103 +++++++++---------- crates/ruff_benchmark/benches/linter.rs | 2 +- crates/ruff_linter/src/linter.rs | 67 +++++++++--- crates/ruff_server/src/fix.rs | 8 +- fuzz/fuzz_targets/ruff_formatter_validity.rs | 4 +- 5 files changed, 111 insertions(+), 73 deletions(-) diff --git a/crates/ruff/src/diagnostics.rs b/crates/ruff/src/diagnostics.rs index 620be66454..b61eea5880 100644 --- a/crates/ruff/src/diagnostics.rs +++ b/crates/ruff/src/diagnostics.rs @@ -263,48 +263,56 @@ pub(crate) fn lint_path( }; // Lint the file. - let ( - LinterResult { - messages, - has_syntax_error: has_error, - }, - transformed, - fixed, - ) = if matches!(fix_mode, flags::FixMode::Apply | flags::FixMode::Diff) { - if let Ok(FixerResult { - result, - transformed, - fixed, - }) = lint_fix( - path, - package, - noqa, - unsafe_fixes, - settings, - &source_kind, - source_type, - ) { - if !fixed.is_empty() { - match fix_mode { - flags::FixMode::Apply => transformed.write(&mut File::create(path)?)?, - flags::FixMode::Diff => { - write!( - &mut io::stdout().lock(), - "{}", - source_kind.diff(&transformed, Some(path)).unwrap() - )?; + let (result, transformed, fixed) = + if matches!(fix_mode, flags::FixMode::Apply | flags::FixMode::Diff) { + if let Ok(FixerResult { + result, + transformed, + fixed, + }) = lint_fix( + path, + package, + noqa, + unsafe_fixes, + settings, + &source_kind, + source_type, + ) { + if !fixed.is_empty() { + match fix_mode { + flags::FixMode::Apply => transformed.write(&mut File::create(path)?)?, + flags::FixMode::Diff => { + write!( + &mut io::stdout().lock(), + "{}", + source_kind.diff(&transformed, Some(path)).unwrap() + )?; + } + flags::FixMode::Generate => {} } - flags::FixMode::Generate => {} } - } - let transformed = if let Cow::Owned(transformed) = transformed { - transformed + let transformed = if let Cow::Owned(transformed) = transformed { + transformed + } else { + source_kind + }; + (result, transformed, fixed) } else { - source_kind - }; - (result, transformed, fixed) + // If we fail to fix, lint the original source code. + let result = lint_only( + path, + package, + settings, + noqa, + &source_kind, + source_type, + ParseSource::None, + ); + let transformed = source_kind; + let fixed = FxHashMap::default(); + (result, transformed, fixed) + } } else { - // If we fail to fix, lint the original source code. let result = lint_only( path, package, @@ -317,21 +325,10 @@ pub(crate) fn lint_path( let transformed = source_kind; let fixed = FxHashMap::default(); (result, transformed, fixed) - } - } else { - let result = lint_only( - path, - package, - settings, - noqa, - &source_kind, - source_type, - ParseSource::None, - ); - let transformed = source_kind; - let fixed = FxHashMap::default(); - (result, transformed, fixed) - }; + }; + + let has_error = result.has_syntax_errors(); + let messages = result.messages; if let Some((cache, relative_path, key)) = caching { // We don't cache parsing errors. diff --git a/crates/ruff_benchmark/benches/linter.rs b/crates/ruff_benchmark/benches/linter.rs index 870d945f33..712bd73b6c 100644 --- a/crates/ruff_benchmark/benches/linter.rs +++ b/crates/ruff_benchmark/benches/linter.rs @@ -89,7 +89,7 @@ fn benchmark_linter(mut group: BenchmarkGroup, settings: &LinterSettings) { ); // Assert that file contains no parse errors - assert!(!result.has_syntax_error); + assert!(!result.has_syntax_errors()); }, criterion::BatchSize::SmallInput, ); diff --git a/crates/ruff_linter/src/linter.rs b/crates/ruff_linter/src/linter.rs index cd620b652a..ca365fc578 100644 --- a/crates/ruff_linter/src/linter.rs +++ b/crates/ruff_linter/src/linter.rs @@ -40,11 +40,48 @@ use crate::{directives, fs, Locator}; 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. + /// Flag indicating that the parsed source code does not contain any + /// [`ParseError`]s + has_valid_syntax: bool, + /// Flag indicating that the parsed source code does not contain any + /// [`UnsupportedSyntaxError`]s + has_no_unsupported_syntax_errors: bool, +} + +impl LinterResult { + /// Returns `true` if the parsed source code contains any [`ParseError`]s *or* + /// [`UnsupportedSyntaxError`]s. /// - /// This includes both [`ParseError`]s and [`UnsupportedSyntaxError`]s. - pub has_syntax_error: bool, + /// See [`LinterResult::has_invalid_syntax`] for a version specific to [`ParseError`]s. + pub fn has_syntax_errors(&self) -> bool { + !self.has_no_syntax_errors() + } + + /// Returns `true` if the parsed source code does not contain any [`ParseError`]s *or* + /// [`UnsupportedSyntaxError`]s. + /// + /// See [`LinterResult::has_valid_syntax`] for a version specific to [`ParseError`]s. + pub fn has_no_syntax_errors(&self) -> bool { + self.has_valid_syntax() && self.has_no_unsupported_syntax_errors + } + + /// Returns `true` if the parsed source code is valid i.e., it has no [`ParseError`]s. + /// + /// Note that this does not include version-related [`UnsupportedSyntaxError`]s. + /// + /// See [`LinterResult::has_no_syntax_errors`] for a version that takes these into account. + pub fn has_valid_syntax(&self) -> bool { + self.has_valid_syntax + } + + /// Returns `true` if the parsed source code is invalid i.e., it has [`ParseError`]s. + /// + /// Note that this does not include version-related [`UnsupportedSyntaxError`]s. + /// + /// See [`LinterResult::has_no_syntax_errors`] for a version that takes these into account. + pub fn has_invalid_syntax(&self) -> bool { + !self.has_valid_syntax() + } } pub type FixTable = FxHashMap; @@ -446,7 +483,8 @@ pub fn lint_only( LinterResult { messages, - has_syntax_error: parsed.has_syntax_errors(), + has_valid_syntax: parsed.has_valid_syntax(), + has_no_unsupported_syntax_errors: parsed.unsupported_syntax_errors().is_empty(), } } @@ -503,8 +541,11 @@ pub fn lint_fix<'a>( // As an escape hatch, bail after 100 iterations. let mut iterations = 0; - // Track whether the _initial_ source code is valid syntax. - let mut is_valid_syntax = false; + // Track whether the _initial_ source code has valid syntax. + let mut has_valid_syntax = false; + + // Track whether the _initial_ source code has no unsupported syntax errors. + let mut has_no_unsupported_syntax_errors = false; let target_version = settings.resolve_target_version(path); @@ -547,12 +588,13 @@ pub fn lint_fix<'a>( ); if iterations == 0 { - is_valid_syntax = parsed.has_no_syntax_errors(); + has_valid_syntax = parsed.has_valid_syntax(); + has_no_unsupported_syntax_errors = parsed.unsupported_syntax_errors().is_empty(); } 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 + // If the source code had no syntax errors on the first pass, but + // does on a subsequent pass, then we've introduced a // syntax error. Return the original code. - if is_valid_syntax { + if has_valid_syntax && has_no_unsupported_syntax_errors { if let Some(error) = parsed.errors().first() { report_fix_syntax_error( path, @@ -593,7 +635,8 @@ pub fn lint_fix<'a>( return Ok(FixerResult { result: LinterResult { messages, - has_syntax_error: !is_valid_syntax, + has_valid_syntax, + has_no_unsupported_syntax_errors, }, transformed, fixed, diff --git a/crates/ruff_server/src/fix.rs b/crates/ruff_server/src/fix.rs index 374d3ac25a..99477c6460 100644 --- a/crates/ruff_server/src/fix.rs +++ b/crates/ruff_server/src/fix.rs @@ -10,7 +10,7 @@ use crate::{ }; use ruff_linter::package::PackageRoot; use ruff_linter::{ - linter::{FixerResult, LinterResult}, + linter::FixerResult, packaging::detect_package_root, settings::{flags, LinterSettings}, }; @@ -62,9 +62,7 @@ pub(crate) fn fix_all( // which is inconsistent with how `ruff check --fix` works. let FixerResult { transformed, - result: LinterResult { - has_syntax_error, .. - }, + result, .. } = ruff_linter::linter::lint_fix( &query.virtual_file_path(), @@ -76,7 +74,7 @@ pub(crate) fn fix_all( source_type, )?; - if has_syntax_error { + if result.has_invalid_syntax() { // If there's a syntax error, then there won't be any fixes to apply. return Ok(Fixes::default()); } diff --git a/fuzz/fuzz_targets/ruff_formatter_validity.rs b/fuzz/fuzz_targets/ruff_formatter_validity.rs index a3b9276c43..cd7b39e78e 100644 --- a/fuzz/fuzz_targets/ruff_formatter_validity.rs +++ b/fuzz/fuzz_targets/ruff_formatter_validity.rs @@ -37,7 +37,7 @@ fn do_fuzz(case: &[u8]) -> Corpus { ParseSource::None, ); - if linter_result.has_syntax_error { + if linter_result.has_syntax_errors() { return Corpus::Keep; // keep, but don't continue } @@ -63,7 +63,7 @@ fn do_fuzz(case: &[u8]) -> Corpus { ); assert!( - !linter_result.has_syntax_error, + !linter_result.has_syntax_errors(), "formatter introduced a parse error" );