From 8a86fef4a91957e752539bb617edee536f178b33 Mon Sep 17 00:00:00 2001 From: Mario Rial Date: Thu, 13 Aug 2026 03:49:40 +0200 Subject: [PATCH 1/4] Fix tokenizer hanging on multi-byte characters in code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit tokenize() ends its loop with a fallback that consumes one character and reports it as an unknown token, so the loop always makes progress. That fallback used SourceStream::take_count(1), which works in *bytes*: for any multi-byte character it returns None, because a single byte lands in the middle of it. Nothing was consumed, no branch matched, and the loop spun forever at 100% CPU. Any non-ASCII character in code outside of a comment or a string literal reaches that fallback. Comments and string literals are consumed whole by their own branches, which is why this went unnoticed: the character has to sit in code. Accented identifiers do exactly that, and VB6 accepts them, so this is reachable from real source files. Six of the eleven projects in the codebase I tried this on hang; the smallest reproduction is a single line, "x = ñ". Add SourceStream::take_character(), which consumes exactly one char however many bytes it occupies, and use it for the fallback. As a safety net, break out of the loop if the stream reports that it is not empty yet nothing can be taken from it: a tokenizer that cannot advance should stop rather than hang. --- projects/vb6parse/src/io/source_stream.rs | 35 +++++++++++++ projects/vb6parse/src/lexer/mod.rs | 63 ++++++++++++++++++++++- 2 files changed, 97 insertions(+), 1 deletion(-) diff --git a/projects/vb6parse/src/io/source_stream.rs b/projects/vb6parse/src/io/source_stream.rs index 54acd21a2..2bbcc6631 100644 --- a/projects/vb6parse/src/io/source_stream.rs +++ b/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. /// diff --git a/projects/vb6parse/src/lexer/mod.rs b/projects/vb6parse/src/lexer/mod.rs index 07508e334..c1526399b 100644 --- a/projects/vb6parse/src/lexer/mod.rs +++ b/projects/vb6parse/src/lexer/mod.rs @@ -376,14 +376,29 @@ pub fn tokenize<'a>(input: &mut SourceStream<'a>) -> ParseResult<'a, TokenStream continue; } - if let Some(token_text) = input.take_count(1) { + // Nothing above matched at this position. Consume one whole character + // so that the loop always makes progress, and report it as unknown. + // + // This must not use `take_count(1)`: that method works in *bytes* and + // returns `None` for any multi-byte character, because a single byte + // lands in the middle of it. A source file holding a non-ASCII + // character outside of a comment or a string literal -- an accented + // identifier, for instance, which VB6 itself accepts -- would then + // reach this point, consume nothing, and spin here forever. + if let Some(token_text) = input.take_character() { ctx.error( input.span_here(), LexerError::UnknownToken { token: token_text.into(), }, ); + continue; } + + // The stream reports that it is not empty, yet nothing could be taken + // from it. That should not be reachable, but stopping is always better + // than looping forever. + break; } failures.extend(ctx.take_errors()); @@ -2328,4 +2343,50 @@ Attribute VB_Exposed = False assert_eq!(tokens[4], ("1.5E+10", Token::SingleLiteral)); assert_eq!(tokens.len(), 5); } + + /// A multi-byte character in code used to hang the tokenizer: the fallback + /// branch asked for one *byte*, could not take it without splitting the + /// character, consumed nothing, and the loop spun forever. + /// + /// VB6 accepts accented identifiers, so real source files hit this. + #[test] + fn multibyte_character_in_code_terminates() { + // Reaching the end of this loop at all is the assertion: before the + // fix, tokenize() never returned for any of these inputs. + for content in ["x = ñ", "Dim año As Integer", "x = €", "x = 日"] { + let mut input = SourceStream::new("", content); + let (tokens_opt, _failures) = tokenize(&mut input).unpack(); + + assert!(tokens_opt.is_some(), "{content}"); + } + } + + /// The whole multi-byte character is reported, not a fragment of it, and + /// tokenizing carries on afterwards. + #[test] + fn multibyte_character_is_reported_whole() { + let mut input = SourceStream::new("", "x = ñ + 1"); + let (tokens_opt, failures) = tokenize(&mut input).unpack(); + + let tokens = tokens_opt.expect("Expected tokens"); + let last = tokens.len() - 1; + assert_eq!( + tokens[last].0, "1", + "tokenizing should continue past the unknown character" + ); + assert_eq!(failures.len(), 1, "one unknown character, one failure"); + } + + /// Non-ASCII characters inside comments and string literals are consumed by + /// their own branches and must stay untouched by the fallback. + #[test] + fn multibyte_character_in_comment_or_string_is_not_an_error() { + for content in ["' año", "x = \"año\""] { + let mut input = SourceStream::new("", content); + let (tokens_opt, failures) = tokenize(&mut input).unpack(); + + assert!(tokens_opt.is_some(), "{content}"); + assert!(failures.is_empty(), "{content}: {failures:?}"); + } + } } From 5f57d7c5d99a8780fbe0db650f6cb01f811de2c2 Mon Sep 17 00:00:00 2001 From: Mario Rial Date: Thu, 13 Aug 2026 04:45:47 +0200 Subject: [PATCH 2/4] Add encode_windows_1252, the counterpart of SourceFile::decode The library reads VB6 source files as Windows-1252 but has no way to write one back. Anything that reads a file, changes it and saves it therefore has to fall back on writing a Rust string straight out, which silently re-encodes the file as UTF-8; VB6 then reads every accented character incorrectly. Encoding is refused rather than made lossy. encoding_rs maps a character outside Windows-1252 to '?', losing it without telling anyone, so an unrepresentable character is returned as an error with its byte offset. Tested by cycling all 256 byte values through decode and back, which is the strongest available check that writing is lossless: Windows-1252 decodes every byte, so the whole range must survive. --- projects/vb6parse/src/io/mod.rs | 2 +- projects/vb6parse/src/io/source_file.rs | 101 ++++++++++++++++++++++++ 2 files changed, 102 insertions(+), 1 deletion(-) diff --git a/projects/vb6parse/src/io/mod.rs b/projects/vb6parse/src/io/mod.rs index fe436e90c..0212d47dc 100644 --- a/projects/vb6parse/src/io/mod.rs +++ b/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}; diff --git a/projects/vb6parse/src/io/source_file.rs b/projects/vb6parse/src/io/source_file.rs index 07e2692ec..9969be776 100644 --- a/projects/vb6parse/src/io/source_file.rs +++ b/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, 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 = (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"); + } +} From 509cf2dfea4a00fa2fded4396bb995bbaa8264d5 Mon Sep 17 00:00:00 2001 From: Mario Rial Date: Thu, 13 Aug 2026 04:45:49 +0200 Subject: [PATCH 3/4] Leave the designer block of forms and classes alone fmt reformatted the whole of a .frm, including the VERSION header and the Begin ... End block. On a small form here that rewrote VERSION as Version, flattened every control to column zero and dropped the block from 3436 to 2796 bytes. That section is not source code. The VB6 IDE writes it and reads it, and rewrites it in its own layout every time the form is saved, so reformatting it comes straight back as diff noise on the next save. It is also where the control geometry of a form lives, which makes silently rewriting it the riskiest thing a formatter can do to a VB6 project. Copy VersionStatement and PropertiesBlock subtrees through unchanged. The same nodes cover the header of a .cls, so class files are protected too. Formatting of the code that follows the block is unaffected, which is what the third test pins down. --- projects/vb6format/src/cst_formatter.rs | 37 ++++++++++++ projects/vb6format/tests/designer_block.rs | 65 ++++++++++++++++++++++ 2 files changed, 102 insertions(+) create mode 100644 projects/vb6format/tests/designer_block.rs diff --git a/projects/vb6format/src/cst_formatter.rs b/projects/vb6format/src/cst_formatter.rs index 910e57f23..39e4b84a2 100644 --- a/projects/vb6format/src/cst_formatter.rs +++ b/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 + ) } diff --git a/projects/vb6format/tests/designer_block.rs b/projects/vb6format/tests/designer_block.rs new file mode 100644 index 000000000..8baca0afd --- /dev/null +++ b/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", + ), + ); +} From 7d7bfadb6ad5a9be65eff05e986f566ef66436af Mon Sep 17 00:00:00 2001 From: Mario Rial Date: Thu, 13 Aug 2026 04:46:05 +0200 Subject: [PATCH 4/4] fmt: read and write Windows-1252, and fail when a file cannot be processed Two problems that together made fmt unusable on a non-English code base. Reading used std::fs::read_to_string, which requires valid UTF-8. Every VB6 source file holding an accented character was rejected. On the code base I tried this on that is 364 of 375 files, and the summary still printed "11 of 375 files would be reformatted", presenting the other 364 as fine. Read through SourceFile, the way the rest of the tool already does, and write back with encode_windows_1252 so a formatted file does not silently turn into UTF-8. Failures were also folded into "unchanged" and the exit code depended only on the number of files that would be reformatted. A run in which every file failed to read reported the errors and exited zero, so fmt --check passed in exactly the case where it should fail, which is worthless as a CI gate. Failures are now counted separately, summarised on stderr after the file list, and produce a non-zero exit in both check and write mode. --- projects/aspen/src/fmt.rs | 75 +++++++++++++++++++++++++++++---------- 1 file changed, 57 insertions(+), 18 deletions(-) diff --git a/projects/aspen/src/fmt.rs b/projects/aspen/src/fmt.rs index 01d20ec8a..33e44e194 100644 --- a/projects/aspen/src/fmt.rs +++ b/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 = 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,11 +273,25 @@ 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 { - 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 { @@ -267,7 +299,14 @@ fn process_file(path: &Path, fmt_settings: &FmtSettings, check_only: bool) -> Re } 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()))?; }