From 8a86fef4a91957e752539bb617edee536f178b33 Mon Sep 17 00:00:00 2001 From: Mario Rial Date: Thu, 13 Aug 2026 03:49:40 +0200 Subject: [PATCH 1/6] 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/6] 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/6] 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/6] 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 c5dee9709b01caaf932a5077b3fc8ca4406496d1 Mon Sep 17 00:00:00 2001 From: Mario Rial Date: Thu, 13 Aug 2026 07:56:22 +0200 Subject: [PATCH 5/6] check: report every file, stop counting control arrays as redefinitions, and gain selectable rules `check` could not say anything useful about a large project. Three things were in the way, and they only become visible one after the other. **It stopped at the first error.** `analyze_project` walked the project's files with `?`, so the first file it could not analyze ended the run. On the project I tried this on -- 213 forms -- it reported one error and finished in 0.11s, which reads like a clean bill of health. Collect the failure and carry on: the same project now takes 15s and reports what is actually there. **Control arrays were reported as redefinitions.** A VB6 control array is several controls sharing one name, told apart by `Index`, with a single event handler taking `Index As Integer`. Registering each element as a fresh symbol made every one of them a duplicate: 406 of the 432 errors that first appeared were `Btn`, `Ch`, `Lbl` and other array names. The element's own `Index` cannot be used to detect this, because index 0 is an ordinary array element and is also what a control with no `Index` property reads as; what identifies it is that a control of that name is already in the current scope. **It always exited zero.** Printing "1 errors found" and exiting 0 makes `check` useless as a CI gate: it passes in exactly the case where it should fail. It now exits 1 when it found something, following ruff's convention. With those out of the way there is somewhere to put per-file rules, which is the fourth part of this change. `vb6parse::lint` holds a registry where each rule has a stable code, a fixability (Safe, Unsafe, None) and a default, and `check` gains `--select`, `--ignore` and `--explain`, plus a `[lint]` section in the `.aspen.toml` it already reads for `[fmt]`. 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) for identifiers holding characters that VB6 accepts but almost nothing downstream of it does. Rules live in `vb6parse` rather than `vb6format` so that the analyzer does not have to depend on the formatter, and so that semantic rules can later be registered from `vb6semantic` through the same types. Note the contract change: `analyze_project` no longer returns `Err` when a file cannot be read, it reports it and keeps going, which is what `analyze_project_resolves_paths_against_base_dir` is updated to expect. --- projects/aspen/src/check.rs | 166 ++++++++++ projects/aspen/src/main.rs | 81 ++++- projects/vb6parse/src/lib.rs | 1 + projects/vb6parse/src/lint.rs | 451 +++++++++++++++++++++++++++ projects/vb6semantic/src/analyzer.rs | 153 +++++++-- 5 files changed, 821 insertions(+), 31 deletions(-) create mode 100644 projects/vb6parse/src/lint.rs diff --git a/projects/aspen/src/check.rs b/projects/aspen/src/check.rs index 8817176a8..e39f9c175 100644 --- a/projects/aspen/src/check.rs +++ b/projects/aspen/src/check.rs @@ -4,12 +4,50 @@ use std::path::{Path, PathBuf}; use anyhow::Result; use rayon::prelude::*; use vb6parse::files::project::ProjectReference; +use vb6parse::lint::LintSettings; use vb6parse::{ProjectFile, SourceFile}; use walkdir::WalkDir; pub struct CheckSettings { pub project_path: PathBuf, + pub lint: LintSettings, +} + +/// Runs the selected lint rules over the files a project refers to. +/// +/// The same file can be shared by several projects -- in the code base this +/// was written against one module is referenced by eight of them -- so a +/// finding is reported once per project that includes it. Deduplication is +/// left to the summary, where the whole run is visible. +fn run_lint_rules(paths: &[PathBuf], settings: &LintSettings) -> Vec { + let mut findings: Vec = paths + .par_iter() + .flat_map(|path| { + let Ok(source) = SourceFile::from_file(path) else { + // Unreadable files are already reported as missing or as a + // parse failure; do not say it twice. + return Vec::new(); + }; + + vb6parse::lint::lint_source(source.as_ref(), settings) + .into_iter() + .map(|finding| { + format!( + "{}:{}:{}: {} {}", + path.display(), + finding.line, + finding.column, + finding.code, + finding.message + ) + }) + .collect::>() + }) + .collect(); + + findings.sort(); + findings } pub struct CheckResults { @@ -18,6 +56,8 @@ pub struct CheckResults { pub non_english_files: Vec, pub missing_files: Vec, pub warnings: Vec, + /// Findings from the lint rules, already formatted for display. + pub lint_findings: Vec, } pub fn check_subcommand(check_settings: CheckSettings) -> Result<()> { @@ -31,6 +71,8 @@ pub fn check_subcommand(check_settings: CheckSettings) -> Result<()> { let mut check_summary = Vec::new(); + let lint = check_settings.lint.clone(); + if check_settings.project_path.is_dir() { let search_path = check_settings.project_path.to_str().unwrap(); let walker = WalkDir::new(search_path).into_iter(); @@ -58,6 +100,7 @@ pub fn check_subcommand(check_settings: CheckSettings) -> Result<()> { project_path.as_ref().err().unwrap() )], warnings: Vec::new(), + lint_findings: Vec::new(), }; return check_result; @@ -65,6 +108,7 @@ pub fn check_subcommand(check_settings: CheckSettings) -> Result<()> { let check_settings = CheckSettings { project_path: project_path.as_ref().unwrap().path().to_path_buf(), + lint: lint.clone(), }; match check_project(&check_settings) { @@ -75,6 +119,7 @@ pub fn check_subcommand(check_settings: CheckSettings) -> Result<()> { non_english_files: Vec::new(), missing_files: Vec::new(), warnings: Vec::new(), + lint_findings: Vec::new(), }, } }) @@ -88,6 +133,7 @@ pub fn check_subcommand(check_settings: CheckSettings) -> Result<()> { non_english_files: Vec::new(), missing_files: Vec::new(), warnings: Vec::new(), + lint_findings: Vec::new(), }, }; check_summary.push(check_result); @@ -97,8 +143,22 @@ pub fn check_subcommand(check_settings: CheckSettings) -> Result<()> { report_check(check_result); } + let anything_found = check_summary.iter().any(|result| { + !result.parsing_errors.is_empty() + || !result.missing_files.is_empty() + || !result.non_english_files.is_empty() + || !result.lint_findings.is_empty() + }); + report_check_summary(check_summary); + // Reporting problems and then exiting zero makes `check` useless as a CI + // gate: the gate passes in exactly the case where it should fail. The + // convention is ruff's -- 1 means the run worked and found something. + if anything_found { + std::process::exit(1); + } + Ok(()) } @@ -107,6 +167,7 @@ fn report_check(check_results: &CheckResults) { && check_results.non_english_files.is_empty() && check_results.missing_files.is_empty() && check_results.warnings.is_empty() + && check_results.lint_findings.is_empty() { return; } @@ -136,6 +197,12 @@ fn report_check(check_results: &CheckResults) { println!(" {}", warning); } } + if !check_results.lint_findings.is_empty() { + println!("Lint:"); + for finding in &check_results.lint_findings { + println!(" {}", finding); + } + } } fn report_single_check_summary(summary: &CheckResults) { @@ -156,6 +223,9 @@ fn report_single_check_summary(summary: &CheckResults) { if !summary.warnings.is_empty() { parts.push(format!("{} warnings", summary.warnings.len())); } + if !summary.lint_findings.is_empty() { + parts.push(format!("{} lint findings", summary.lint_findings.len())); + } if parts.is_empty() { println!("No errors found in {}.", summary.project_path); @@ -184,6 +254,8 @@ fn report_check_summary(summary: Vec) { let total_warning_count = summary.iter().fold(0, |acc, x| acc + x.warnings.len()); + let total_lint_count = summary.iter().fold(0, |acc, x| acc + x.lint_findings.len()); + let mut parts = Vec::new(); if total_missed_file_count != 0 { @@ -201,6 +273,9 @@ fn report_check_summary(summary: Vec) { if total_warning_count != 0 { parts.push(format!("{} warnings", total_warning_count)); } + if total_lint_count != 0 { + parts.push(format!("{} lint findings", total_lint_count)); + } if parts.is_empty() { println!("No errors found in {} projects.", project_count); @@ -238,6 +313,7 @@ fn check_project(check_settings: &CheckSettings) -> Result { non_english_files: Vec::new(), missing_files: Vec::new(), warnings: Vec::new(), + lint_findings: Vec::new(), }; let project_contents = std::fs::read(&check_settings.project_path).unwrap(); @@ -296,6 +372,9 @@ fn check_project(check_settings: &CheckSettings) -> Result { } } + // Every source file the project refers to, for the lint rules to run over. + let mut source_paths: Vec = Vec::new(); + for class_reference in project.classes() { let class_path = join_parent_project_path(project_directory, class_reference.path); @@ -304,6 +383,8 @@ fn check_project(check_settings: &CheckSettings) -> Result { check_results .missing_files .push(format!("Class not found: {}", class_path.to_str().unwrap())); + } else { + source_paths.push(class_path); } } @@ -316,6 +397,8 @@ fn check_project(check_settings: &CheckSettings) -> Result { "Module not found: {}", module_path.to_str().unwrap() )); + } else { + source_paths.push(module_path); } } @@ -327,9 +410,13 @@ fn check_project(check_settings: &CheckSettings) -> Result { check_results .missing_files .push(format!("Form not found: {}", form_path.to_str().unwrap())); + } else { + source_paths.push(form_path); } } + check_results.lint_findings = run_lint_rules(&source_paths, &check_settings.lint); + // Analyze the project with vb6semantic. This resolves names, builds symbol // tables, and reports semantic errors and warnings across all of the // project's source files. @@ -361,3 +448,82 @@ fn check_project(check_settings: &CheckSettings) -> Result { Ok(check_results) } + +/// 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, the same way 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() +} + +/// Prints every rule with its default and fixability. +pub fn explain_rules() { + println!( + "{:<6} {:<24} {:<8} {:<8} {}", + "CODE", "NAME", "DEFAULT", "FIX", "SUMMARY" + ); + + for rule in vb6parse::lint::RULES { + println!( + "{:<6} {:<24} {:<8} {:<8} {}", + rule.code, + rule.name, + if rule.default_on { "on" } else { "off" }, + match rule.fixability { + vb6parse::lint::Fixability::Safe => "safe", + vb6parse::lint::Fixability::Unsafe => "unsafe", + vb6parse::lint::Fixability::None => "none", + }, + rule.summary + ); + } +} diff --git a/projects/aspen/src/main.rs b/projects/aspen/src/main.rs index 9d763bad9..ed887ef7d 100644 --- a/projects/aspen/src/main.rs +++ b/projects/aspen/src/main.rs @@ -13,11 +13,40 @@ use clap::{Arg, Command, builder::PossibleValue, command, value_parser}; fn main() -> Result<()> { let matches = command!() .subcommand( - Command::new("check").about("Check the project").arg( - Arg::new("project path") - .required(false) - .value_parser(value_parser!(PathBuf)), - ), + Command::new("check") + .about("Check the project") + .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("lint 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("lint rule codes or code prefixes to skip"), + ) + .arg( + Arg::new("explain") + .long("explain") + .required(false) + .action(clap::ArgAction::SetTrue) + .help("list every lint rule with its default and fixability"), + ) + .arg( + Arg::new("project path") + .required(false) + .value_parser(value_parser!(PathBuf)), + ), ) .subcommand( Command::new("fmt") @@ -97,7 +126,47 @@ fn main() -> Result<()> { .unwrap_or(¤t_dir) .to_path_buf(); - let check_settings = check::CheckSettings { project_path }; + if matches.get_flag("explain") { + check::explain_rules(); + return Ok(()); + } + + let configured = check::load_lint_settings(&project_path); + let from_cli = |name: &str| -> Option> { + matches + .get_many::(name) + .map(|values| values.cloned().collect()) + }; + + let select = from_cli("select").unwrap_or(configured.select); + let ignore = from_cli("ignore").unwrap_or(configured.ignore); + + let unknown: Vec<&String> = select + .iter() + .chain(ignore.iter()) + .filter(|code| { + !vb6parse::lint::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 check --explain` lists them.", + unknown + .iter() + .map(|code| code.as_str()) + .collect::>() + .join(", ") + ); + } + + let check_settings = check::CheckSettings { + project_path, + lint: vb6parse::lint::LintSettings::from_selection(&select, &ignore), + }; check_subcommand(check_settings)?; diff --git a/projects/vb6parse/src/lib.rs b/projects/vb6parse/src/lib.rs index c6d7e9935..a3caf6e05 100644 --- a/projects/vb6parse/src/lib.rs +++ b/projects/vb6parse/src/lib.rs @@ -189,6 +189,7 @@ pub mod files; pub mod io; pub mod language; pub mod lexer; +pub mod lint; pub mod parsers; pub mod syntax; diff --git a/projects/vb6parse/src/lint.rs b/projects/vb6parse/src/lint.rs new file mode 100644 index 000000000..24cc64172 --- /dev/null +++ b/projects/vb6parse/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 vb6parse::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 crate::ConcreteSyntaxTree; +use crate::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()); + } + } +} diff --git a/projects/vb6semantic/src/analyzer.rs b/projects/vb6semantic/src/analyzer.rs index 062f6907a..1ac77f8d2 100644 --- a/projects/vb6semantic/src/analyzer.rs +++ b/projects/vb6semantic/src/analyzer.rs @@ -153,19 +153,28 @@ impl SemanticAnalyzer { // Analyze the source files in `.vbp` line order so that cross-module // name resolution matches the order the IDE sees the files in. for entry in project.file_entries() { - match entry { + let outcome = match entry { vb6parse::files::project::ProjectFileEntry::Module(module_reference) => { - self.analyze_module_reference(module_reference)?; + self.analyze_module_reference(module_reference) } vb6parse::files::project::ProjectFileEntry::Class(class_reference) => { - self.analyze_class_reference(class_reference)?; + self.analyze_class_reference(class_reference) } vb6parse::files::project::ProjectFileEntry::Form(form_file_name) => { - self.analyze_form_path(form_file_name)?; + self.analyze_form_path(form_file_name) } // User controls, user documents, designers, property pages, and // related documents are not analyzed yet. - _ => {} + _ => Ok(()), + }; + + // One file that cannot be analyzed must not hide every file after + // it: record the failure and carry on down the project. Stopping + // at the first problem makes the report say nothing about the + // project's real state, and on a large project the first problem + // is usually in the first form. + if let Err(error) = outcome { + self.errors.push(error); } } @@ -439,6 +448,24 @@ impl SemanticAnalyzer { Ok(()) } + /// Whether a control or menu of this name is already registered in the + /// current scope. + /// + /// In VB6 that is not a redefinition: it is a control array, several + /// elements sharing one name and told apart by `Index`, with a single + /// event handler taking `Index As Integer`. Forms use them heavily -- a + /// row of buttons, a set of menu items -- so reporting every element as a + /// duplicate buries the real findings. + /// + /// The element's own `Index` cannot be used to detect this: index 0 is an + /// ordinary array element, and is also what a control with no `Index` + /// property reads as. + fn is_control_array_element(&self, name: &str) -> bool { + self.scope_manager + .lookup_in_scope(self.scope_manager.current_scope_id(), name) + .is_some_and(|existing| existing.kind == SymbolKind::Control) + } + /// Add a symbol to the current scope pub fn add_symbol(&mut self, symbol: Symbol) -> Result<()> { match self.scope_manager.add_symbol(symbol) { @@ -958,15 +985,20 @@ impl SemanticAnalyzer { if !control.tag().is_empty() { attributes.insert("tag".to_string(), control.tag().to_string()); } - self.add_symbol(Symbol { - name: control.name().to_string(), - kind: SymbolKind::Control, - type_info: TypeInfo::object(), - visibility: Visibility::Public, - location: self.make_location(1, 1), - scope_id: self.scope_manager.current_scope_id(), - attributes, - })?; + + // Further elements of a control array share the name of the first and + // are not a redefinition of it. + if !self.is_control_array_element(control.name()) { + self.add_symbol(Symbol { + name: control.name().to_string(), + kind: SymbolKind::Control, + type_info: TypeInfo::object(), + visibility: Visibility::Public, + location: self.make_location(1, 1), + scope_id: self.scope_manager.current_scope_id(), + attributes, + })?; + } // Recursively register controls nested inside containers match control.kind() { @@ -992,15 +1024,19 @@ impl SemanticAnalyzer { if menu.index() != 0 { attributes.insert("index".to_string(), menu.index().to_string()); } - self.add_symbol(Symbol { - name: menu.name().to_string(), - kind: SymbolKind::Control, - type_info: TypeInfo::object(), - visibility: Visibility::Public, - location: self.make_location(1, 1), - scope_id: self.scope_manager.current_scope_id(), - attributes, - })?; + + // Menus form control arrays too, and are the most common case of it. + if !self.is_control_array_element(menu.name()) { + self.add_symbol(Symbol { + name: menu.name().to_string(), + kind: SymbolKind::Control, + type_info: TypeInfo::object(), + visibility: Visibility::Public, + location: self.make_location(1, 1), + scope_id: self.scope_manager.current_scope_id(), + attributes, + })?; + } for sub in menu.sub_menus() { self.register_menu(sub)?; } @@ -1561,6 +1597,65 @@ mod tests { use std::{collections::HashMap, fs}; use tempfile::tempdir; + /// A control array is several elements sharing one name, told apart by + /// `Index`, with a single event handler. Registering the second element + /// must not be reported as a redefinition of the first. + #[test] + fn control_array_elements_are_not_a_redefinition() { + let mut analyzer = SemanticAnalyzer::new(); + analyzer + .scope_manager + .push_module_scope(ScopeKind::Class, "FrmTest".to_string()); + + let control = Symbol { + name: "Btn".to_string(), + kind: SymbolKind::Control, + type_info: TypeInfo::object(), + visibility: Visibility::Public, + location: analyzer.make_location(1, 1), + scope_id: analyzer.scope_manager.current_scope_id(), + attributes: HashMap::new(), + }; + + analyzer + .add_symbol(control) + .expect("the first element registers"); + + assert!( + analyzer.is_control_array_element("Btn"), + "a control of that name is already in scope" + ); + assert!( + !analyzer.is_control_array_element("Lbl"), + "a name that is not in scope is not an array element" + ); + assert_eq!(analyzer.errors.len(), 0); + } + + /// A name already taken by something that is not a control is still a + /// redefinition. + #[test] + fn a_variable_of_the_same_name_is_not_a_control_array() { + let mut analyzer = SemanticAnalyzer::new(); + analyzer + .scope_manager + .push_module_scope(ScopeKind::Class, "FrmTest".to_string()); + + analyzer + .add_symbol(Symbol { + name: "Total".to_string(), + kind: SymbolKind::Variable, + type_info: TypeInfo::object(), + visibility: Visibility::Public, + location: analyzer.make_location(1, 1), + scope_id: analyzer.scope_manager.current_scope_id(), + attributes: HashMap::new(), + }) + .expect("registers"); + + assert!(!analyzer.is_control_array_element("Total")); + } + #[test] fn analyze_empty_project() { let mut analyzer = SemanticAnalyzer::new(); @@ -2238,9 +2333,17 @@ End Function assert!(failures.is_empty(), "Parse failures: {:?}", failures); let project = project_opt.expect("Project should parse"); - // Without a base dir the bare relative path cannot be found. + // Without a base dir the bare relative path cannot be found. The + // analysis reports that rather than propagating it: one file it cannot + // read must not stop it from looking at the rest of the project. let mut analyzer = SemanticAnalyzer::new(); - assert!(analyzer.analyze_project(&project).is_err()); + let result = analyzer + .analyze_project(&project) + .expect("the run itself succeeds"); + assert!( + !result.is_successful(), + "the unreadable module should be reported" + ); // With the base dir set to the project directory it is found. let mut analyzer = SemanticAnalyzer::new(); From 54e1a639eb5de6b6c8fd7df334414ed64021181d Mon Sep 17 00:00:00 2001 From: Mario Rial Date: Thu, 13 Aug 2026 08:17:33 +0200 Subject: [PATCH 6/6] Do not slice the continuation line at a fixed byte offset The check for a `REM` on a continuation line used `line[..4]`, which panics when that line begins with a multi-byte character -- exactly what a continuation line holding Spanish prose does: end byte index 4 is not a char boundary; it is inside 'a' Found by running the built binary over a real code base rather than only the test suite. `str::get` returns `None` at a non-boundary instead. --- projects/vb6parse/src/lint.rs | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/projects/vb6parse/src/lint.rs b/projects/vb6parse/src/lint.rs index 24cc64172..3332c353f 100644 --- a/projects/vb6parse/src/lint.rs +++ b/projects/vb6parse/src/lint.rs @@ -301,7 +301,9 @@ fn continued_comment_lines(source: &str) -> std::collections::HashSet { continued.insert(index + 1); } else { inside_comment = trimmed.starts_with('\'') - || trimmed.len() >= 4 && trimmed[..4].eq_ignore_ascii_case("rem "); + || trimmed + .get(..4) + .is_some_and(|prefix| prefix.eq_ignore_ascii_case("rem ")); } // The run of comment lines ends at the first one without a trailing @@ -372,6 +374,21 @@ mod tests { assert!(lint_source(source, &with(&["N001"])).is_empty()); } + /// The `REM` check must not slice a line at a fixed byte offset: on a + /// continuation line that starts with an accented character the offset + /// lands inside it, which panics. + #[test] + fn continuation_line_starting_with_a_multibyte_character() { + let source = concat!( + "' primero _\r\n", + "\u{e1}rea de trabajo\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";