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

Author: dylwil3

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.

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,
                 );

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<Message>,
-    /// 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.
+    ///
+    /// 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.
     ///
-    /// This includes both [`ParseError`]s and [`UnsupportedSyntaxError`]s.
-    pub has_syntax_error: bool,
+    /// 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<Rule, usize>;
@@ -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,

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());
     }

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"
         );