Revert attempting to format any code that contains any parse errors,

introduce formatter errors and return code enums

Close #323

Author: NathanLovato

CHANGELOG.md

@@ -12,6 +12,7 @@ This file documents the changes made to the formatter with each release.
 
 - Removed space between lambda function name and parameter list
 - Force @export and @onready annotations to stay on the same line as a variable but keep other annotations separate
+- Stop trying to format any code containing parse errors. Until now we tried to still format definitions around the code with errors, but this can lead to cases where the formatter produces invalid code
 
 ### Fixed
 

src/bin/benchmark.rs

@@ -19,7 +19,9 @@
 //! cargo run --bin benchmark --release >> benchmark_results.txt
 //! git checkout -
 //! ```
-use gdscript_formatter::{FormatterConfiguration, RenderElement, format_gdscript_with_buffers};
+use gdscript_formatter::{
+    FormatErrors, FormatterConfiguration, RenderElement, format_gdscript_with_buffers,
+};
 use std::{
     env, fs,
     hint::black_box,
@@ -51,7 +53,11 @@ impl BenchmarkRunner {
         }
     }
 
-    fn format(&mut self, source: &str, config: &FormatterConfiguration) -> Result<(), String> {
+    fn format(
+        &mut self,
+        source: &str,
+        config: &FormatterConfiguration,
+    ) -> Result<(), FormatErrors> {
         format_gdscript_with_buffers(
             black_box(source),
             black_box(config),
@@ -66,7 +72,7 @@ impl BenchmarkRunner {
         &mut self,
         source: &str,
         config: &FormatterConfiguration,
-    ) -> Result<BenchmarkMeasurement, String> {
+    ) -> Result<BenchmarkMeasurement, FormatErrors> {
         let warmup_start = Instant::now();
         while warmup_start.elapsed() < WARMUP_DURATION {
             self.format(source, config)?;

src/formatter.rs

@@ -980,7 +980,7 @@ fn process_source(
     // pull code into or out of a # fmt: off disabled region. For now we
     // skip reordering for disabled regions, but in the future we may want
     // to reorder code around disabled regions as well?
-    if input.reorder_code && !input.has_parse_errors {
+    if input.reorder_code {
         if input.disabled_regions.is_empty() {
             process_source_reorder(input, node, render_elements);
             return;
@@ -1010,12 +1010,6 @@ fn process_source(
         };
 
         let kind = GDScriptNodeKind::get_kind_from_ast_node(child);
-        // Tree-sitter can recover from syntax errors by wrapping a declaration
-        // in an ERROR node. Formatting inside that subtree would rely on AST
-        // relationships that may no longer describe the source. Keep the
-        // malformed declaration intact while formatting its valid siblings.
-        let contains_parse_error = input.has_parse_errors && child.has_error();
-
         // This code is similar to the one in process_body(). See comments
         // there for some explanation of what this does and why it's needed.
         match classify_disabled_region_overlap(input, node, child, current_index) {
@@ -1047,16 +1041,7 @@ fn process_source(
                 continue;
             }
             DisabledRegionOverlapKind::PartiallyCovered => {
-                if contains_parse_error {
-                    render_elements.push(RenderElement::UnformattedSource {
-                        range: RangeSourceBytes {
-                            start_byte: child.start_byte(),
-                            end_byte: child.end_byte(),
-                        },
-                    });
-                } else {
-                    process_node(input, child, render_elements);
-                }
+                process_node(input, child, render_elements);
                 spacing_context.last_output_end = Some(child.end_byte());
                 spacing_context.last_declaration_end = Some(child.end_byte());
                 spacing_context.last_declaration_kind = Some(kind);
@@ -1098,16 +1083,7 @@ fn process_source(
             &spacing_context,
             child,
         );
-        if contains_parse_error {
-            render_elements.push(RenderElement::UnformattedSource {
-                range: RangeSourceBytes {
-                    start_byte: child.start_byte(),
-                    end_byte: child.end_byte(),
-                },
-            });
-        } else {
-            process_node(input, child, render_elements);
-        }
+        process_node(input, child, render_elements);
         spacing_context.last_output_end = Some(child.end_byte());
         spacing_context.last_declaration_end = Some(child.end_byte());
         spacing_context.last_declaration_kind = Some(kind);

src/lib.rs

@@ -25,6 +25,34 @@ pub mod verify_structure;
 
 pub use renderer::{PrinterConfiguration, RenderElement};
 
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub enum FormatErrors {
+    FailedToParseInput,
+    ParseErrors,
+    FailedToParseFormattedOutput,
+    StructureChanged,
+}
+
+impl std::fmt::Display for FormatErrors {
+    fn fmt(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        let message = match self {
+            Self::FailedToParseInput => {
+                "We failed to parse input, the parser did not produce a valid AST"
+            }
+            Self::ParseErrors => "The input GDScript code contains parse errors",
+            Self::FailedToParseFormattedOutput => {
+                "The parser could not parse the formatted output, it did not produce a valid AST"
+            }
+            Self::StructureChanged => {
+                "The formatted output is structurally different from the input. Please report this to the bug tracker with a copy of the input code: https://github.com/GDQuest/GDScript-formatter/issues/"
+            }
+        };
+        formatter.write_str(message)
+    }
+}
+
+impl std::error::Error for FormatErrors {}
+
 /// Selects which delimiters the formatter prefers for string literals.
 #[derive(Clone, Copy, Debug, PartialEq, Eq)]
 pub enum QuoteStyle {
@@ -81,7 +109,10 @@ impl Default for FormatterConfiguration {
 ///
 /// For formatting multiple files, prefer [format_gdscript_with_buffers] to
 /// reuse pre-allocated buffers across multiple calls.
-pub fn format_gdscript(source: &str, config: &FormatterConfiguration) -> Result<String, String> {
+pub fn format_gdscript(
+    source: &str,
+    config: &FormatterConfiguration,
+) -> Result<String, FormatErrors> {
     let mut render_elements = Vec::new();
     let mut output = String::new();
     format_gdscript_with_buffers(source, config, &mut render_elements, &mut output)?;
@@ -96,9 +127,11 @@ pub fn format_gdscript_with_buffers(
     config: &FormatterConfiguration,
     render_elements: &mut Vec<RenderElement>,
     output: &mut String,
-) -> Result<(), String> {
-    let parsed = parser::ParseInput::new(source, config)
-        .ok_or_else(|| "Failed to parse input".to_string())?;
+) -> Result<(), FormatErrors> {
+    let parsed = parser::ParseInput::new(source, config).ok_or(FormatErrors::FailedToParseInput)?;
+    if parsed.has_parse_errors {
+        return Err(FormatErrors::ParseErrors);
+    }
     formatter::build_formatter_intermediate_representation(&parsed, render_elements);
 
     // The renderer clamps every blank-line run to `maximum_blank_lines`. If a
@@ -114,17 +147,13 @@ pub fn format_gdscript_with_buffers(
 
     if config.safe {
         let reparsed = parser::ParseInput::new(output, config)
-            .ok_or_else(|| "Verify structure: formatted output does not parse".to_string())?;
-        if !verify_structure::trees_structurally_equal(
+            .ok_or(FormatErrors::FailedToParseFormattedOutput)?;
+        if !verify_structure::are_syntax_trees_structurally_equal(
             &parsed.tree,
             &reparsed.tree,
             parsed.kind_lookup,
         ) {
-            return Err(
-                "Verify structure: formatted output is structurally different from input. \
-                 Keeping original source."
-                    .to_string(),
-            );
+            return Err(FormatErrors::StructureChanged);
         }
     }
 

src/main.rs

@@ -27,23 +27,31 @@ use gdscript_formatter::linter::rule_config::{
     get_all_rule_names, parse_disabled_rules, validate_rule_names,
 };
 use gdscript_formatter::{
-    FormatterConfiguration, QuoteStyle, RenderElement, format_gdscript,
+    FormatErrors, FormatterConfiguration, QuoteStyle, RenderElement, format_gdscript,
     format_gdscript_with_buffers, linter::LinterConfig,
 };
 use std::collections::HashSet;
 
 use cli::{Command, parse_args};
 
-const ERROR_CODE_NOT_FORMATTED: i32 = 1;
+#[repr(i32)]
+enum FormatterExitCodes {
+    NotFormatted = 1,
+    ParseErrors = 2,
+}
 
-#[derive(Debug, Clone)]
 struct FormatterOutput {
     index: usize,
     file_path: PathBuf,
     formatted_content: String,
     is_formatted: bool,
 }
 
+enum FormatterFileProcessingResult {
+    SkippedParseErrors { index: usize, file_path: PathBuf },
+    Formatted(FormatterOutput),
+}
+
 #[derive(Clone, Copy)]
 struct FormatterConfigOverrides {
     /// Explicitly requested tab or space indentation.
@@ -160,7 +168,16 @@ fn main() -> Result<(), Box<dyn std::error::Error>> {
             &current_directory.join("stdin.gd"),
             config_overrides,
         );
-        let formatted_content = format_gdscript(&input_content, &stdin_config)?;
+        let formatted_content = match format_gdscript(&input_content, &stdin_config) {
+            Ok(formatted_content) => formatted_content,
+            Err(FormatErrors::ParseErrors) => {
+                eprintln!(
+                    "Skipping formatting stdin: the input GDScript code contains parse errors"
+                );
+                std::process::exit(FormatterExitCodes::ParseErrors as i32);
+            }
+            Err(error) => return Err(error.into()),
+        };
 
         if do_check_formatted_only {
             if input_content != formatted_content {
@@ -197,17 +214,21 @@ fn main() -> Result<(), Box<dyn std::error::Error>> {
         let _ = io::stdout().flush();
     }
 
-    let mut sorted_outputs: Vec<Result<FormatterOutput, String>> =
+    let mut sorted_outputs: Vec<Result<FormatterFileProcessingResult, String>> =
         format_files_parallel(&input_gdscript_files, &config, config_overrides);
 
     sorted_outputs.sort_by(compare_output_index);
 
     let mut all_formatted = true;
     let mut modified_file_count = 0;
     let mut unformatted_files = Vec::new();
+    let mut skipped_parse_error_files = Vec::new();
     for output in sorted_outputs {
         match output {
-            Ok(output) => {
+            Ok(FormatterFileProcessingResult::SkippedParseErrors { file_path, .. }) => {
+                skipped_parse_error_files.push(file_path);
+            }
+            Ok(FormatterFileProcessingResult::Formatted(output)) => {
                 if do_check_formatted_only {
                     if use_verbose_output {
                         eprintln!(
@@ -267,6 +288,16 @@ fn main() -> Result<(), Box<dyn std::error::Error>> {
         }
     }
 
+    if !skipped_parse_error_files.is_empty() {
+        for file_path in skipped_parse_error_files {
+            eprintln!(
+                "Skipped formatting file {}: the input GDScript code contains parse errors",
+                file_path.display()
+            );
+        }
+        std::process::exit(FormatterExitCodes::ParseErrors as i32);
+    }
+
     if do_check_formatted_only {
         if !use_verbose_output {
             terminal_clear_line();
@@ -282,7 +313,7 @@ fn main() -> Result<(), Box<dyn std::error::Error>> {
             for file_path in unformatted_files {
                 eprintln!("{}", file_path.display());
             }
-            std::process::exit(ERROR_CODE_NOT_FORMATTED);
+            std::process::exit(FormatterExitCodes::NotFormatted as i32);
         }
     } else if !do_print_to_stdout {
         if !use_verbose_output {
@@ -356,25 +387,39 @@ fn format_one_file(
     config_overrides: FormatterConfigOverrides,
     render_elements: &mut Vec<RenderElement>,
     output: &mut String,
-) -> Result<FormatterOutput, String> {
+) -> Result<FormatterFileProcessingResult, String> {
     let input_content = fs::read_to_string(file_path)
         .map_err(|error| format!("Failed to read file {}: {}", file_path.display(), error))?;
 
     // We need to clone that config because files in nested directories can
     // match different EditorConfig files and rules.
     let mut file_config = config.clone();
     config_apply_editorconfig_then_cli_overrides(&mut file_config, file_path, config_overrides);
-    format_gdscript_with_buffers(&input_content, &file_config, render_elements, output)
-        .map_err(|error| format!("Failed to format file {}: {}", file_path.display(), error))?;
+    match format_gdscript_with_buffers(&input_content, &file_config, render_elements, output) {
+        Ok(()) => {}
+        Err(FormatErrors::ParseErrors) => {
+            return Ok(FormatterFileProcessingResult::SkippedParseErrors {
+                index,
+                file_path: file_path.clone(),
+            });
+        }
+        Err(error) => {
+            return Err(format!(
+                "Failed to format file {}: {}",
+                file_path.display(),
+                error
+            ));
+        }
+    }
 
     let is_formatted = input_content == *output;
 
-    Ok(FormatterOutput {
+    Ok(FormatterFileProcessingResult::Formatted(FormatterOutput {
         index,
         file_path: file_path.clone(),
         formatted_content: output.clone(),
         is_formatted,
-    })
+    }))
 }
 
 /// Applies project editorconfig settings first and CLI settings second.
@@ -415,7 +460,7 @@ fn format_files_parallel(
     files: &[PathBuf],
     config: &FormatterConfiguration,
     config_overrides: FormatterConfigOverrides,
-) -> Vec<Result<FormatterOutput, String>> {
+) -> Vec<Result<FormatterFileProcessingResult, String>> {
     if files.is_empty() {
         return Vec::new();
     }
@@ -450,7 +495,7 @@ fn format_chunk(
     chunk_size: usize,
     config: &FormatterConfiguration,
     config_overrides: FormatterConfigOverrides,
-) -> Vec<Result<FormatterOutput, String>> {
+) -> Vec<Result<FormatterFileProcessingResult, String>> {
     let mut results = Vec::with_capacity(chunk.len());
     let mut render_elements: Vec<RenderElement> = Vec::new();
     let mut output = String::new();
@@ -553,15 +598,17 @@ fn is_path_excluded(path: &Path, excluded_paths: &[PathBuf]) -> bool {
 }
 
 fn compare_output_index(
-    left: &Result<FormatterOutput, String>,
-    right: &Result<FormatterOutput, String>,
+    left: &Result<FormatterFileProcessingResult, String>,
+    right: &Result<FormatterFileProcessingResult, String>,
 ) -> std::cmp::Ordering {
     let left_index = match left {
-        Ok(formatter_output) => formatter_output.index,
+        Ok(FormatterFileProcessingResult::Formatted(formatter_output)) => formatter_output.index,
+        Ok(FormatterFileProcessingResult::SkippedParseErrors { index, .. }) => *index,
         Err(_) => usize::MAX,
     };
     let right_index = match right {
-        Ok(formatter_output) => formatter_output.index,
+        Ok(FormatterFileProcessingResult::Formatted(formatter_output)) => formatter_output.index,
+        Ok(FormatterFileProcessingResult::SkippedParseErrors { index, .. }) => *index,
         Err(_) => usize::MAX,
     };
     left_index.cmp(&right_index)

src/verify_structure.rs

@@ -39,7 +39,7 @@ struct NormalizedNode {
 
 /// Entry point: compare two tree-sitter trees for structural equivalence,
 /// accounting for formatting-induced CST changes.
-pub fn trees_structurally_equal(
+pub fn are_syntax_trees_structurally_equal(
     input_tree: &tree_sitter::Tree,
     output_tree: &tree_sitter::Tree,
     lookup: &[GDScriptNodeKind; 256],
@@ -265,7 +265,7 @@ mod tests {
     fn structurally_equal(a: &str, b: &str) -> bool {
         let ta = parse(a);
         let tb = parse(b);
-        trees_structurally_equal(&ta, &tb, lookup())
+        are_syntax_trees_structurally_equal(&ta, &tb, lookup())
     }
 
     #[test]

tests/expected/parse_error.gd

@@ -1,5 +1,4 @@
-# Valid declarations around declarations with parse errors should still be
-# formatted.
+# If there is any parse error in the file, it should be left untouched.
 var valid = 1
 var name.bla = value
 var also_valid = 2