Make aspen usable on a non-English VB6 code base: tokenizer hang, designer block, Windows-1252 round trip, exit code

projects/aspen/src/fmt.rs

@@ -5,6 +5,8 @@ use rayon::prelude::*;
 
 use walkdir::WalkDir;
 
+use vb6parse::io::{SourceFile, encode_windows_1252};
+
 pub use vb6format::FmtSettings;
 
 pub struct CliSettings {
@@ -134,44 +136,60 @@ pub fn fmt_subcommand(cmd: FmtCommand) -> Result<()> {
         blank_lines_around_top_level: blank_around_top_level,
     };
 
-    let results: Vec<(PathBuf, bool)> = files_to_format
+    let results: Vec<FileOutcome> = files_to_format
         .par_iter()
         .map(|file| {
             let result = process_file(file, &fmt_settings, cmd.cli.check);
             (file.clone(), result)
         })
         .map(|(file, result)| match result {
-            Ok(changed) => {
-                if changed {
-                    if cmd.cli.check {
-                        println!("Would reformat: {}", file.display());
-                    } else {
-                        println!("Formatted: {}", file.display());
-                    }
-                    (file, true)
+            Ok(true) => {
+                if cmd.cli.check {
+                    println!("Would reformat: {}", file.display());
                 } else {
-                    (file, false)
+                    println!("Formatted: {}", file.display());
                 }
+                FileOutcome::Changed
             }
+            Ok(false) => FileOutcome::Unchanged,
             Err(e) => {
                 eprintln!("Error formatting {}: {}", file.display(), e);
-                (file, false)
+                FileOutcome::Failed
             }
         })
         .collect();
 
-    let changed_count = results.iter().filter(|(_, changed)| *changed).count();
+    let changed_count = results
+        .iter()
+        .filter(|outcome| **outcome == FileOutcome::Changed)
+        .count();
+    let failed_count = results
+        .iter()
+        .filter(|outcome| **outcome == FileOutcome::Failed)
+        .count();
     let total = results.len();
 
     if cmd.cli.check {
         println!("{} of {} files would be reformatted.", changed_count, total);
-        if changed_count > 0 {
-            std::process::exit(1);
-        }
     } else {
         println!("Formatted {} of {} files.", changed_count, total);
     }
 
+    // A file that could not be processed is not a file that is fine: reporting
+    // it and then exiting zero makes `fmt --check` pass in exactly the case it
+    // should fail, which is worthless as a CI gate. Say so, and say it loudly
+    // enough to be seen after a long list of file names.
+    if failed_count > 0 {
+        eprintln!(
+            "{} of {} files could not be processed and were not checked.",
+            failed_count, total
+        );
+    }
+
+    if failed_count > 0 || (cmd.cli.check && changed_count > 0) {
+        std::process::exit(1);
+    }
+
     Ok(())
 }
 
@@ -255,19 +273,40 @@ fn join_parent_project_path(parent_project_path: &Path, file_path: &str) -> Path
     }
 }
 
+/// What happened to a single file. An error is deliberately not folded into
+/// "unchanged": the two need to reach the exit code differently.
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+enum FileOutcome {
+    Changed,
+    Unchanged,
+    Failed,
+}
+
 fn process_file(path: &Path, fmt_settings: &FmtSettings, check_only: bool) -> Result<bool> {
-    let source = std::fs::read_to_string(path)
+    // VB6 source files are Windows-1252, not UTF-8. `read_to_string` rejects
+    // every file holding an accented character, which in a non-English code
+    // base is most of them, so decode the way the rest of the library does.
+    let source = SourceFile::from_file(path)
+        .map_err(|e| anyhow::anyhow!("{}", e))
         .with_context(|| format!("Failed to read {}", path.display()))?;
+    let source: &str = source.as_ref();
 
-    let formatted = vb6format::fmt_source(&source, fmt_settings)
+    let formatted = vb6format::fmt_source(source, fmt_settings)
         .with_context(|| format!("Failed to format {}", path.display()))?;
 
     if formatted == source {
         return Ok(false);
     }
 
     if !check_only {
-        std::fs::write(path, &formatted)
+        // And write it back as Windows-1252 too. Writing the Rust string
+        // straight out would re-encode the file as UTF-8, which VB6 would then
+        // read incorrectly -- a silent corruption of every accented character
+        // in the project.
+        let bytes = encode_windows_1252(&formatted)
+            .with_context(|| format!("Failed to encode {}", path.display()))?;
+
+        std::fs::write(path, bytes)
             .with_context(|| format!("Failed to write {}", path.display()))?;
     }
 

projects/vb6format/src/cst_formatter.rs

@@ -37,6 +37,11 @@ impl<'a> CstFormatter<'a> {
             return;
         }
 
+        if is_designer_node(node.kind()) {
+            self.emit_verbatim(node);
+            return;
+        }
+
         self.passes.on_node_enter(node, &mut self.context);
 
         match node.kind() {
@@ -64,4 +69,36 @@ impl<'a> CstFormatter<'a> {
             self.output.push_str(&buffer.text);
         }
     }
+
+    /// Copies a subtree to the output exactly as it was read, without running
+    /// any pass over it.
+    fn emit_verbatim(&mut self, node: &CstNode) {
+        let text = node.text();
+        let ends_line = text.ends_with('\n');
+
+        self.output.push_str(text);
+
+        // Leave the layout state as if the copied text had been emitted token
+        // by token, so that whatever follows the block is still formatted.
+        self.context.pending_indent = ends_line;
+        self.context.line_has_content = !ends_line;
+        self.context.last_was_blank = false;
+    }
+}
+
+/// The designer section of a `.frm`, `.ctl` or `.cls` file: the `VERSION`
+/// header and the `Begin ... End` block that the VB6 IDE itself writes and
+/// reads.
+///
+/// That section is not source code. Reformatting it fights the IDE, which
+/// rewrites the block in its own layout every time the form is saved, so any
+/// change here comes straight back as diff noise. It is also where a form's
+/// control geometry lives, which makes silently rewriting it the riskiest
+/// thing a formatter could do to a VB6 project. It is copied through
+/// unchanged.
+fn is_designer_node(kind: SyntaxKind) -> bool {
+    matches!(
+        kind,
+        SyntaxKind::VersionStatement | SyntaxKind::PropertiesBlock
+    )
 }

projects/vb6format/tests/designer_block.rs

@@ -0,0 +1,65 @@
+//! The designer section of a form or class -- the `VERSION` header and the
+//! `Begin ... End` block -- is written and read by the VB6 IDE itself, and
+//! holds the control geometry. The formatter must copy it through untouched.
+
+mod common;
+
+#[test]
+fn form_designer_block_is_left_alone() {
+    common::assert_stable(concat!(
+        "VERSION 5.00\r\n",
+        "Begin VB.Form FrmSave \r\n",
+        "   BorderStyle     =   4  'Fixed ToolWindow\r\n",
+        "   Caption         =   \"Save\"\r\n",
+        "   ClientHeight    =   1695\r\n",
+        "   Begin VB.CommandButton Btn \r\n",
+        "      Height          =   360\r\n",
+        "      TabIndex        =   0\r\n",
+        "   End\r\n",
+        "End\r\n",
+        "Attribute VB_Name = \"FrmSave\"\r\n",
+    ));
+}
+
+#[test]
+fn version_header_keeps_its_casing() {
+    // `VERSION` is upper case in every file the IDE writes; the keyword pass
+    // must not turn it into `Version`.
+    let source = concat!(
+        "VERSION 1.0 CLASS\r\n",
+        "BEGIN\r\n",
+        "  MultiUse = -1  'True\r\n",
+        "END\r\n",
+        "Attribute VB_Name = \"CFoo\"\r\n",
+    );
+
+    common::assert_stable(source);
+}
+
+#[test]
+fn code_after_the_designer_block_is_still_formatted() {
+    // Skipping the block must not switch the formatter off for the rest of the
+    // file.
+    common::assert_fmt(
+        concat!(
+            "VERSION 5.00\r\n",
+            "Begin VB.Form FrmSave \r\n",
+            "   ClientHeight    =   1695\r\n",
+            "End\r\n",
+            "Attribute VB_Name = \"FrmSave\"\r\n",
+            "Private Sub Btn_Click()\r\n",
+            "Dim x As Integer\r\n",
+            "End Sub\r\n",
+        ),
+        concat!(
+            "VERSION 5.00\r\n",
+            "Begin VB.Form FrmSave \r\n",
+            "   ClientHeight    =   1695\r\n",
+            "End\r\n",
+            "Attribute VB_Name = \"FrmSave\"\r\n",
+            "Private Sub Btn_Click()\r\n",
+            "    Dim x As Integer\r\n",
+            "End Sub\r\n",
+        ),
+    );
+}

projects/vb6parse/src/io/mod.rs

@@ -25,5 +25,5 @@
 pub mod source_file;
 pub mod source_stream;
 
-pub use source_file::SourceFile;
+pub use source_file::{SourceFile, UnrepresentableCharacter, encode_windows_1252};
 pub use source_stream::{Comparator, SourceStream};

projects/vb6parse/src/io/source_file.rs

@@ -369,3 +369,104 @@ Currently, only latin-1 source code is supported."
         SourceStream::new(self.context.file_name(), self.context.content())
     }
 }
+
+/// Encodes text back into the Windows-1252 bytes that VB6 expects on disk.
+///
+/// This is the counterpart of [`SourceFile::decode`]: anything that reads a
+/// source file, changes it and writes it back needs it, or the file silently
+/// becomes UTF-8 and VB6 stops reading its accented characters correctly.
+///
+/// Encoding is refused rather than made lossy. `encoding_rs` maps any
+/// character outside Windows-1252 to `?`, which loses it without telling
+/// anyone, so an unrepresentable character is reported as an error instead.
+///
+/// # Errors
+///
+/// Returns the first character that Windows-1252 cannot represent, together
+/// with its byte offset in `content`.
+///
+/// # Example
+///
+/// ```rust
+/// use vb6parse::io::{encode_windows_1252, SourceFile};
+///
+/// let bytes = b"' a\xF1o";
+/// let source = SourceFile::decode_with_replacement("m.bas", bytes).expect("decodes");
+///
+/// assert_eq!(encode_windows_1252(source.as_ref()).unwrap(), bytes);
+/// ```
+pub fn encode_windows_1252(content: &str) -> Result<Vec<u8>, UnrepresentableCharacter> {
+    let (bytes, _, had_errors) = WINDOWS_1252.encode(content);
+
+    if had_errors {
+        let (offset, character) = content
+            .char_indices()
+            .find(|(_, character)| {
+                let mut buffer = [0u8; 4];
+                let (_, _, unmappable) = WINDOWS_1252.encode(character.encode_utf8(&mut buffer));
+                unmappable
+            })
+            .expect("encode reported an unmappable character");
+
+        return Err(UnrepresentableCharacter { character, offset });
+    }
+
+    Ok(bytes.into_owned())
+}
+
+/// A character that [`encode_windows_1252`] could not represent.
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub struct UnrepresentableCharacter {
+    /// The offending character.
+    pub character: char,
+    /// Its byte offset in the text that was being encoded.
+    pub offset: usize,
+}
+
+impl Display for UnrepresentableCharacter {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        write!(
+            f,
+            "character {:?} (U+{:04X}) at byte {} cannot be represented in Windows-1252",
+            self.character, self.character as u32, self.offset
+        )
+    }
+}
+
+impl std::error::Error for UnrepresentableCharacter {}
+
+#[cfg(test)]
+mod encode_tests {
+    use super::*;
+
+    #[test]
+    fn round_trips_accented_text() {
+        let bytes = b"Public Function A\xF1adir() ' a\xF1o \xE1rea";
+        let source = SourceFile::decode_with_replacement("m.bas", bytes).expect("decodes");
+
+        let encoded = encode_windows_1252(source.as_ref()).expect("encodes");
+
+        assert_eq!(encoded, bytes, "the file must survive a decode/encode cycle");
+    }
+
+    #[test]
+    fn every_byte_round_trips() {
+        // Windows-1252 decodes all 256 byte values, so cycling the whole range
+        // is the strongest check that writing back is lossless.
+        let bytes: Vec<u8> = (0u8..=255).collect();
+        let source = SourceFile::decode_with_replacement("m.bas", &bytes).expect("decodes");
+
+        assert_eq!(
+            encode_windows_1252(source.as_ref()).expect("encodes"),
+            bytes
+        );
+    }
+
+    #[test]
+    fn refuses_characters_outside_windows_1252() {
+        let error = encode_windows_1252("año 日").expect_err("must not silently write '?'");
+
+        assert_eq!(error.character, '日');
+        assert_eq!(error.offset, 5, "offset is in bytes, and 'ñ' takes two");
+    }
+}

projects/vb6parse/src/io/source_stream.rs

@@ -285,6 +285,41 @@ impl<'a> SourceStream<'a> {
         }
     }
 
+    /// Takes the next whole character from the stream and advances the offset
+    /// past it.
+    ///
+    /// Unlike [`SourceStream::take_count`], which works in *bytes*, this method
+    /// always consumes exactly one `char`, however many bytes that character
+    /// occupies. This makes it safe to use as the "consume something and keep
+    /// going" fallback of a tokenizer loop: `take_count(1)` returns `None` for
+    /// any multi-byte character, because a single byte lands in the middle of
+    /// it, which would leave such a loop unable to make progress.
+    ///
+    /// Returns `None` only when the stream is empty.
+    ///
+    /// # Example
+    ///
+    /// ```rust
+    /// use vb6parse::io::SourceStream;
+    ///
+    /// // 'ñ' is two bytes in UTF-8.
+    /// let mut stream = SourceStream::new("test.bas", "ñx");
+    ///
+    /// assert_eq!(stream.take_count(1), None);
+    /// assert_eq!(stream.take_character(), Some("ñ"));
+    /// assert_eq!(stream.take_character(), Some("x"));
+    /// assert_eq!(stream.take_character(), None);
+    /// ```
+    #[must_use]
+    pub fn take_character(&mut self) -> Option<&'a str> {
+        let remaining = &self.contents[self.offset..];
+        let character = remaining.chars().next()?;
+        let end_offset = self.offset + character.len_utf8();
+        let result = &self.contents[self.offset..end_offset];
+        self.offset = end_offset;
+        Some(result)
+    }
+
     /// Takes characters from the stream until a character that matches the
     /// compare `str` is encountered or the end of the stream is reached.
     ///