From 8a86fef4a91957e752539bb617edee536f178b33 Mon Sep 17 00:00:00 2001 From: Mario Rial Date: Thu, 13 Aug 2026 03:49:40 +0200 Subject: [PATCH 1/5] 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/5] 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/5] 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/5] 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()))?; } From 6ec86ea2d8acd7c0ff5f6341f6d8d0f27cd9859e Mon Sep 17 00:00:00 2001 From: Mario Rial Date: Thu, 13 Aug 2026 05:21:32 +0200 Subject: [PATCH 5/5] Add a lint layer with selectable rules MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Neither of the two existing commands has anywhere to put a finding that will not be fixed automatically. `vb6format` is fix-only: a `FormatPass` can rewrite tokens and `fmt_source` returns a `String`, so a pass cannot say "this is wrong and I am deliberately not touching it". `check` is project-only: it needs a `.vbp`, runs the whole semantic analysis and stops at the first error. That leaves out the checks that are cheap, per-file and not safely fixable. The one that prompted this is a non-ASCII character in an identifier: VB6 accepts `Public Function Añadir()`, a code base written in Spanish is full of them, almost nothing downstream of VB6 accepts them, and renaming one is a decision about a public API rather than something a formatter should do behind your back. The shape follows ruff, since the vocabulary is already familiar: every rule carries a stable code, a fixability (Safe, Unsafe, None) and a default, and a run selects by code or code prefix through `--select` / `--ignore` or a `[lint]` section in the `.aspen.toml` that is already read for `[fmt]`. Exit codes follow the same convention: 0 clean, 1 findings, 2 the run itself failed, so CI can tell "your code has a problem" from "the tool could not look". Three rules to establish the shape: mixed-line-endings and trailing- whitespace (safe fixes, on by default) and non-ascii-in-code (no fix, off by default). Two things worth knowing about that last rule. It cannot be written as "an Identifier token holding a non-ASCII character", because the lexer takes identifiers with take_ascii_underscore_alphanumerics and stops at the first non-ASCII byte: `Añadir` never becomes one token, and the stray character is recorded as an UnknownToken failure with no token pushed, so it is absent from the CST entirely. The failure list is where it lives. And it skips lines that continue a comment: VB6 continues a logical line with a trailing `_` and that applies to comments, which the lexer does not model, so prose from a continued comment otherwise reaches the token stream and every accent in it looks like an identifier. On the code base I tested this on that was the difference between 2898 findings and 2247, and the 651 it removed were entirely accented vowels from Spanish prose. Builds on the fixes in #2 -- the hang has to go first, or a file with an accented identifier never finishes tokenizing, and the Windows-1252 read has to go first, or the linter cannot open the files this rule is about. Discussed in #3. --- projects/aspen/src/lint.rs | 212 ++++++++++++++++ projects/aspen/src/main.rs | 63 +++++ projects/vb6format/src/lib.rs | 2 + projects/vb6format/src/lint.rs | 451 +++++++++++++++++++++++++++++++++ 4 files changed, 728 insertions(+) create mode 100644 projects/aspen/src/lint.rs create mode 100644 projects/vb6format/src/lint.rs diff --git a/projects/aspen/src/lint.rs b/projects/aspen/src/lint.rs new file mode 100644 index 000000000..f4957e209 --- /dev/null +++ b/projects/aspen/src/lint.rs @@ -0,0 +1,212 @@ +//! `aspen lint`: per-file rules, selected by code. +//! +//! Separate from `check`, which needs a `.vbp` and runs the whole semantic +//! analysis. This walks files, parses each one on its own and applies the +//! selected rules, which keeps it fast enough for a pre-commit hook. + +use std::path::{Path, PathBuf}; + +use anyhow::{Context, Result}; +use rayon::prelude::*; + +use vb6format::lint::{Diagnostic, Fixability, LintSettings, RULES}; +use vb6parse::io::SourceFile; + +pub struct LintCommand { + pub project_path: PathBuf, + pub select: Vec, + pub ignore: Vec, + pub explain: bool, +} + +/// The `[lint]` section of `.aspen.toml`, alongside the `[fmt]` section that +/// is already read from the same file. +#[derive(Debug, Clone, Default, serde::Deserialize)] +pub struct LintConfig { + #[serde(default)] + pub select: Vec, + #[serde(default)] + pub ignore: Vec, +} + +#[derive(Debug, Clone, serde::Deserialize)] +struct AspenConfig { + lint: Option, +} + +/// Reads the `[lint]` section next to the given path, then from the working +/// directory, mirroring how the formatter finds its own settings. +#[must_use] +pub fn load_lint_settings(project_path: &Path) -> LintConfig { + let config_root = if project_path.is_dir() { + project_path.to_path_buf() + } else { + project_path + .parent() + .map_or_else(|| PathBuf::from("."), Path::to_path_buf) + }; + + let candidates = [ + config_root.join(".aspenfmt.toml"), + config_root.join(".aspen.toml"), + PathBuf::from(".aspenfmt.toml"), + PathBuf::from(".aspen.toml"), + ]; + + for path in &candidates { + if !path.exists() { + continue; + } + + let Ok(contents) = std::fs::read_to_string(path) else { + continue; + }; + + match toml::from_str::(&contents) { + Ok(config) => return config.lint.unwrap_or_default(), + Err(e) => { + // A malformed config that is silently ignored looks exactly + // like a config that selected nothing. + eprintln!("Ignoring {}: {}", path.display(), e); + } + } + } + + LintConfig::default() +} + +pub fn lint_subcommand(cmd: LintCommand) -> Result<()> { + if cmd.explain { + println!( + "{:<6} {:<24} {:<8} {:<8} {}", + "CODE", "NAME", "DEFAULT", "FIX", "SUMMARY" + ); + for rule in RULES { + println!( + "{:<6} {:<24} {:<8} {:<8} {}", + rule.code, + rule.name, + if rule.default_on { "on" } else { "off" }, + match rule.fixability { + Fixability::Safe => "safe", + Fixability::Unsafe => "unsafe", + Fixability::None => "none", + }, + rule.summary + ); + } + return Ok(()); + } + + let unknown: Vec<&String> = cmd + .select + .iter() + .chain(cmd.ignore.iter()) + .filter(|code| { + !RULES + .iter() + .any(|rule| rule.code.starts_with(code.as_str())) + }) + .collect(); + + if !unknown.is_empty() { + // A typo in a rule code must not quietly select nothing. + anyhow::bail!( + "unknown rule code(s): {}. `aspen lint --explain` lists them.", + unknown + .iter() + .map(|code| code.as_str()) + .collect::>() + .join(", ") + ); + } + + let settings = LintSettings::from_selection(&cmd.select, &cmd.ignore); + let files = collect_files(&cmd.project_path); + + if files.is_empty() { + println!("No VB6 source files found."); + return Ok(()); + } + + let results: Vec<(PathBuf, Result>)> = files + .par_iter() + .map(|path| (path.clone(), lint_file(path, &settings))) + .collect(); + + let mut found = 0usize; + let mut failed = 0usize; + + for (path, result) in &results { + match result { + Ok(diagnostics) => { + for diagnostic in diagnostics { + println!( + "{}:{}:{}: {} {}", + path.display(), + diagnostic.line, + diagnostic.column, + diagnostic.code, + diagnostic.message + ); + found += 1; + } + } + Err(e) => { + eprintln!("Error linting {}: {}", path.display(), e); + failed += 1; + } + } + } + + println!("{} finding(s) in {} file(s).", found, files.len()); + + if failed > 0 { + eprintln!("{} of {} files could not be read.", failed, files.len()); + } + + // ruff's convention: 1 means the run worked and found something, 2 means + // the run itself failed. Keeping them apart lets CI tell "your code has a + // problem" from "the tool could not look". + if failed > 0 { + std::process::exit(2); + } + + if found > 0 { + std::process::exit(1); + } + + Ok(()) +} + +fn lint_file(path: &Path, settings: &LintSettings) -> Result> { + let source = SourceFile::from_file(path) + .map_err(|e| anyhow::anyhow!("{}", e)) + .with_context(|| format!("Failed to read {}", path.display()))?; + + Ok(vb6format::lint_source(source.as_ref(), settings)) +} + +fn collect_files(path: &Path) -> Vec { + if path.is_file() { + return if is_source_file(path) { + vec![path.to_path_buf()] + } else { + Vec::new() + }; + } + + walkdir::WalkDir::new(path) + .into_iter() + .filter_map(std::result::Result::ok) + .map(|entry| entry.path().to_path_buf()) + .filter(|path| path.is_file() && is_source_file(path)) + .collect() +} + +fn is_source_file(path: &Path) -> bool { + matches!( + path.extension().and_then(|s| s.to_str()), + Some("bas" | "cls" | "frm") + ) +} diff --git a/projects/aspen/src/main.rs b/projects/aspen/src/main.rs index 9d763bad9..ba553fad7 100644 --- a/projects/aspen/src/main.rs +++ b/projects/aspen/src/main.rs @@ -1,8 +1,10 @@ mod check; mod fmt; +mod lint; use check::check_subcommand; use fmt::fmt_subcommand; +use lint::lint_subcommand; use anyhow::Result; @@ -19,6 +21,42 @@ fn main() -> Result<()> { .value_parser(value_parser!(PathBuf)), ), ) + .subcommand( + Command::new("lint") + .about("Check VB6 source files against selectable rules") + .arg( + Arg::new("select") + .long("select") + .required(false) + // One value per flag, repeatable, comma-separated. Not + // `num_args(1..)`: that swallows the positional path. + .num_args(1) + .action(clap::ArgAction::Append) + .value_delimiter(',') + .help("rule codes or code prefixes to run, e.g. N001 or N"), + ) + .arg( + Arg::new("ignore") + .long("ignore") + .required(false) + .num_args(1) + .action(clap::ArgAction::Append) + .value_delimiter(',') + .help("rule codes or code prefixes to skip"), + ) + .arg( + Arg::new("explain") + .long("explain") + .required(false) + .action(clap::ArgAction::SetTrue) + .help("list every rule with its default and fixability"), + ) + .arg( + Arg::new("project path") + .required(false) + .value_parser(value_parser!(PathBuf)), + ), + ) .subcommand( Command::new("fmt") .about("Format VB6 source files") @@ -138,6 +176,31 @@ fn main() -> Result<()> { return Ok(()); } + if let Some(matches) = matches.subcommand_matches("lint") { + let current_dir = current_dir()?; + + let project_path = matches + .get_one::("project path") + .unwrap_or(¤t_dir) + .to_path_buf(); + + let configured = lint::load_lint_settings(&project_path); + let from_cli = |name: &str| -> Option> { + matches + .get_many::(name) + .map(|values| values.cloned().collect()) + }; + + lint_subcommand(lint::LintCommand { + select: from_cli("select").unwrap_or(configured.select), + ignore: from_cli("ignore").unwrap_or(configured.ignore), + explain: matches.get_flag("explain"), + project_path, + })?; + + return Ok(()); + } + println!("Unknown subcommand"); Ok(()) diff --git a/projects/vb6format/src/lib.rs b/projects/vb6format/src/lib.rs index e2c527286..28d63b92a 100644 --- a/projects/vb6format/src/lib.rs +++ b/projects/vb6format/src/lib.rs @@ -1,9 +1,11 @@ pub mod context; mod cst_formatter; +pub mod lint; mod passes; pub mod rewrite; pub mod settings; +pub use lint::{Diagnostic, Fixability, LintSettings, RULES, Rule, lint_source}; pub use settings::FmtSettings; use anyhow::Result; diff --git a/projects/vb6format/src/lint.rs b/projects/vb6format/src/lint.rs new file mode 100644 index 000000000..5c377eb1c --- /dev/null +++ b/projects/vb6format/src/lint.rs @@ -0,0 +1,451 @@ +//! Per-file lint rules over the CST. +//! +//! Formatting answers "is this file laid out canonically?" and always applies +//! its own answer. Linting answers "is there something wrong here?", and some +//! of those answers must not be applied automatically: renaming an identifier +//! is a decision about a public API, not a whitespace change. +//! +//! The shape follows `ruff`: every rule has a stable code, a fixability, and a +//! default, and a run selects rules by code or code prefix. +//! +//! ```rust +//! use vb6format::lint::{lint_source, LintSettings}; +//! +//! let settings = LintSettings::from_selection(&["N001".to_string()], &[]); +//! let found = lint_source("Public Function Añadir()\r\nEnd Function\r\n", &settings); +//! +//! assert_eq!(found[0].code, "N001"); +//! ``` + +use vb6parse::ConcreteSyntaxTree; +use vb6parse::errors::{ErrorKind, LexerError}; + +/// Whether a rule's finding can be corrected mechanically. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum Fixability { + /// The correction cannot change the meaning of the program. + Safe, + /// The correction is mechanical but could change behaviour in edge cases. + Unsafe, + /// The correction needs a judgment call and is left to a person. + None, +} + +/// A lint rule. +#[derive(Debug, Clone, Copy)] +pub struct Rule { + /// Stable identifier, used in configuration and in output. + pub code: &'static str, + /// Short kebab-case name. + pub name: &'static str, + /// One line describing what the rule looks for. + pub summary: &'static str, + /// Whether the finding can be corrected mechanically. + pub fixability: Fixability, + /// Whether the rule runs when nothing is selected explicitly. + pub default_on: bool, +} + +/// Every rule the linter knows about. +pub const RULES: &[Rule] = &[ + Rule { + code: "W001", + name: "mixed-line-endings", + summary: "file mixes CRLF and LF line endings", + fixability: Fixability::Safe, + default_on: true, + }, + Rule { + code: "W002", + name: "trailing-whitespace", + summary: "line ends in whitespace", + fixability: Fixability::Safe, + default_on: true, + }, + Rule { + code: "N001", + name: "non-ascii-in-code", + summary: "identifier contains a character outside ASCII", + fixability: Fixability::None, + default_on: false, + }, +]; + +/// Looks a rule up by its code. +#[must_use] +pub fn rule(code: &str) -> Option<&'static Rule> { + RULES.iter().find(|rule| rule.code == code) +} + +/// Something a rule found, at a place in the file. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct Diagnostic { + /// The code of the rule that produced it. + pub code: &'static str, + /// What was found. + pub message: String, + /// One-based line number. + pub line: usize, + /// One-based column, counted in characters. + pub column: usize, + /// Whether this particular finding could be corrected mechanically. + pub fixability: Fixability, +} + +/// Which rules a run should apply. +/// +/// Selection works by code or by code prefix, so `"N"` takes every rule in the +/// `N` category and `"N001"` takes one. An empty selection means the rules +/// that are on by default. +#[derive(Debug, Clone, Default)] +pub struct LintSettings { + select: Vec, + ignore: Vec, +} + +impl LintSettings { + /// Builds a selection from lists of codes or code prefixes. + #[must_use] + pub fn from_selection(select: &[String], ignore: &[String]) -> Self { + Self { + select: select.to_vec(), + ignore: ignore.to_vec(), + } + } + + /// Whether `code` runs under this selection. + #[must_use] + pub fn is_enabled(&self, code: &str) -> bool { + if self.ignore.iter().any(|prefix| code.starts_with(prefix)) { + return false; + } + + if self.select.is_empty() { + return rule(code).is_some_and(|rule| rule.default_on); + } + + self.select.iter().any(|prefix| code.starts_with(prefix)) + } +} + +/// Runs the selected rules over one file. +#[must_use] +pub fn lint_source(source: &str, settings: &LintSettings) -> Vec { + let mut found = Vec::new(); + + if settings.is_enabled("W001") { + found.extend(mixed_line_endings(source)); + } + + if settings.is_enabled("W002") { + found.extend(trailing_whitespace(source)); + } + + if settings.is_enabled("N001") { + found.extend(non_ascii_in_code(source)); + } + + found.sort_by_key(|diagnostic| (diagnostic.line, diagnostic.column)); + found +} + +/// W001. VB6 writes CRLF; a file holding both usually got there through a tool +/// that did not know that, and the mixture shows up as a whole-file diff the +/// next time anything touches it. +fn mixed_line_endings(source: &str) -> Vec { + let crlf = source.matches("\r\n").count(); + let lf = source.matches('\n').count() - crlf; + + if crlf == 0 || lf == 0 { + return Vec::new(); + } + + // Report the first line that disagrees with whichever ending dominates, + // rather than every one of them: the finding is about the file. + let odd_one_out_is_lf = crlf >= lf; + let mut offset = 0usize; + + for (index, line) in source.split_inclusive('\n').enumerate() { + let is_lf = line.ends_with('\n') && !line.ends_with("\r\n"); + + if is_lf == odd_one_out_is_lf && line.ends_with('\n') { + return vec![Diagnostic { + code: "W001", + message: format!( + "file mixes line endings: {crlf} CRLF and {lf} LF; this line ends in {}", + if is_lf { "LF" } else { "CRLF" } + ), + line: index + 1, + column: line.chars().count(), + fixability: Fixability::Safe, + }]; + } + + offset += line.len(); + } + + let _ = offset; + Vec::new() +} + +/// W002. Trailing whitespace is invisible, survives in the file forever and +/// turns into diff noise the first time a formatter or an editor removes it. +fn trailing_whitespace(source: &str) -> Vec { + source + .split('\n') + .enumerate() + .filter_map(|(index, line)| { + let line = line.strip_suffix('\r').unwrap_or(line); + let trimmed = line.trim_end_matches([' ', '\t']); + + if trimmed.len() == line.len() { + return None; + } + + Some(Diagnostic { + code: "W002", + message: format!( + "line ends in {} whitespace characters", + line.len() - trimmed.len() + ), + line: index + 1, + column: trimmed.chars().count() + 1, + fixability: Fixability::Safe, + }) + }) + .collect() +} + +/// N001. VB6 accepts accented identifiers, and code bases written in Spanish, +/// French or German are full of them. Almost nothing downstream of VB6 does, +/// so they are worth knowing about before a migration. Renaming one changes a +/// public name, so this rule never offers a fix. +/// +/// It cannot be written as "an `Identifier` token holding a non-ASCII +/// character". The lexer takes identifiers with +/// `take_ascii_underscore_alphanumerics` and stops at the first byte outside +/// ASCII, so `Añadir` never becomes one token; the stray character reaches the +/// tokenizer's fallback, which records an `UnknownToken` failure and pushes no +/// token at all. The character is therefore absent from the CST, and the +/// failure list is the only place it appears. +/// +/// That is also what makes the rule precise: comments and string literals are +/// consumed whole by their own branches, so an accent inside product text +/// never reaches the fallback and never shows up here. +fn non_ascii_in_code(source: &str) -> Vec { + let (_cst_opt, failures) = ConcreteSyntaxTree::from_text("lint_input", source).unpack(); + + let line_starts = line_starts(source); + let continued_comment = continued_comment_lines(source); + let mut found: Vec = Vec::new(); + + for failure in failures { + let ErrorKind::Lexer(LexerError::UnknownToken { token }) = failure.kind.as_ref() else { + continue; + }; + + if token.is_ascii() { + continue; + } + + // The failure is recorded after the character has been consumed, so + // step back over it to point at the character itself. + let offset = (failure.error_offset as usize) + .min(source.len()) + .saturating_sub(token.len()); + let (line, column) = position(&line_starts, source, offset); + + if continued_comment.contains(&line) { + continue; + } + + // One accented name produces one failure per character; report the + // name once rather than once per accent. + if found + .last() + .is_some_and(|previous| previous.line == line && column <= previous.column + 2) + { + continue; + } + + found.push(Diagnostic { + code: "N001", + message: format!("identifier contains a character outside ASCII: {token}"), + line, + column, + fixability: Fixability::None, + }); + } + + found +} + +/// One-based numbers of the lines that are the continuation of a comment +/// started on an earlier line. +/// +/// VB6 continues a logical line with a trailing `_`, and that applies to +/// comments: everything after the continuation is still comment text. The +/// lexer does not model this — it starts reading the next line as code — so +/// prose lands in the token stream and any accent in it looks like an +/// identifier. Until the lexer handles it, the rule skips those lines rather +/// than report words from a comment. +fn continued_comment_lines(source: &str) -> std::collections::HashSet { + let mut continued = std::collections::HashSet::new(); + let mut inside_comment = false; + + for (index, line) in source.split('\n').enumerate() { + let line = line.strip_suffix('\r').unwrap_or(line); + let trimmed = line.trim_start(); + + if inside_comment { + continued.insert(index + 1); + } else { + inside_comment = trimmed.starts_with('\'') + || trimmed.len() >= 4 && trimmed[..4].eq_ignore_ascii_case("rem "); + } + + // The run of comment lines ends at the first one without a trailing + // continuation. + inside_comment = inside_comment && line.trim_end().ends_with('_'); + } + + continued +} + +fn line_starts(source: &str) -> Vec { + let mut starts = vec![0usize]; + starts.extend(source.match_indices('\n').map(|(index, _)| index + 1)); + starts +} + +/// Turns a byte offset into a one-based line and character column. +fn position(line_starts: &[usize], source: &str, offset: usize) -> (usize, usize) { + let line_index = line_starts.partition_point(|start| *start <= offset) - 1; + let line_start = line_starts[line_index]; + let column = source[line_start..offset].chars().count() + 1; + + (line_index + 1, column) +} + +#[cfg(test)] +mod tests { + use super::*; + + fn with(codes: &[&str]) -> LintSettings { + let select: Vec = codes.iter().map(|code| (*code).to_string()).collect(); + LintSettings::from_selection(&select, &[]) + } + + #[test] + fn finds_accented_identifiers_but_not_accented_text() { + let source = concat!( + "' un a\u{f1}o cualquiera\r\n", + "Public Function A\u{f1}adir() As String\r\n", + " A\u{f1}adir = \"a\u{f1}o\"\r\n", + "End Function\r\n", + ); + + let found = lint_source(source, &with(&["N001"])); + + assert_eq!( + found.len(), + 2, + "the comment and the string are text: {found:?}" + ); + assert_eq!(found[0].line, 2); + // Column 18 is the 'ñ' itself in `Public Function Añadir()`. + assert_eq!(found[0].column, 18); + assert_eq!(found[0].fixability, Fixability::None); + } + + #[test] + fn ignores_the_continuation_of_a_comment() { + // The second line is still comment text, because the first ends in the + // VB6 line-continuation `_`. The words in it are prose, not code. + let source = concat!( + "' esta linea sigue en la siguiente _\r\n", + " S/N\u{ba}PROYECTO y su a\u{f1}o\r\n", + "Public Sub Main()\r\n", + "End Sub\r\n", + ); + + assert!(lint_source(source, &with(&["N001"])).is_empty()); + } + + #[test] + fn ascii_only_code_is_clean() { + let source = "Public Function Anadir() As String\r\nEnd Function\r\n"; + + assert!(lint_source(source, &with(&["N001"])).is_empty()); + } + + #[test] + fn finds_mixed_line_endings() { + let found = lint_source("Dim a\r\nDim b\nDim c\r\n", &with(&["W001"])); + + assert_eq!( + found.len(), + 1, + "the finding is about the file, not the line" + ); + assert_eq!(found[0].code, "W001"); + assert_eq!(found[0].line, 2); + } + + #[test] + fn consistent_line_endings_are_clean() { + assert!(lint_source("Dim a\r\nDim b\r\n", &with(&["W001"])).is_empty()); + assert!(lint_source("Dim a\nDim b\n", &with(&["W001"])).is_empty()); + } + + #[test] + fn finds_trailing_whitespace() { + let found = lint_source("Dim a \r\nDim b\r\n", &with(&["W002"])); + + assert_eq!(found.len(), 1); + assert_eq!((found[0].line, found[0].column), (1, 6)); + } + + #[test] + fn selection_is_by_code_or_prefix() { + let source = "Public Function A\u{f1}adir() \r\n"; + + // Nothing selected: only the rules that are on by default. + let default_codes: Vec<_> = lint_source(source, &LintSettings::default()) + .iter() + .map(|diagnostic| diagnostic.code) + .collect(); + assert_eq!(default_codes, vec!["W002"], "N001 is off by default"); + + // A whole category by prefix. + assert!( + lint_source(source, &with(&["N"])) + .iter() + .any(|diagnostic| diagnostic.code == "N001") + ); + + // And ignore wins over select. + let settings = LintSettings::from_selection( + &["N".to_string(), "W".to_string()], + &["N001".to_string()], + ); + assert!( + lint_source(source, &settings) + .iter() + .all(|diagnostic| diagnostic.code != "N001") + ); + } + + #[test] + fn every_rule_has_a_unique_code_and_is_reachable() { + for rule in RULES { + assert_eq!( + RULES.iter().filter(|other| other.code == rule.code).count(), + 1, + "duplicate code {}", + rule.code + ); + assert!(super::rule(rule.code).is_some()); + } + } +}