check: report every file, fix the control-array false positive, exit non-zero, and gain selectable rules - #6
Open
MarioRial22 wants to merge 6 commits into
Open
Conversation
added 5 commits
August 13, 2026 03:49
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.
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.
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.
…essed 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.
…ns, 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.
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.
Author
|
Correction to what I wrote above about the eight failing The cause is line endings. The So: those eight are green in your CI, they are unrelated to this PR, and there is nothing here for you to fix. I should have looked at the diff before explaining it rather than after. The one thing that might be worth having is a note for contributors on Windows to set |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replaces #4, and implements what #3 proposes — see the note there about why the rules belong in
checkrather than in a command of their own.Stacks on #2; GitHub shows those four commits here too. The last commit is what this PR is about. Happy to rebase once #2 lands.
Measured on a production VB6 code base: 11 projects, ~660 source files, written in Spanish.
1. It stopped at the first error
analyze_projectwalked the project's files with?, so the first file it could not analyze ended the run.0.11s for 213 forms is not a clean bill of health, it is a run that stopped on the first one. Collecting the failure and carrying on, the same project takes 15s and reports what is actually there.
2. Control arrays were reported as redefinitions
That first run, once it kept going, produced 432 errors. 406 of them were one bug. A VB6 control array is several controls sharing a name, told apart by
Index, with a single handler takingIndex As Integer—Btn,Ch,Lbl, a row of buttons, a set of menu items. Every element after the first was a duplicate symbol.The element's own
Indexcannot be used to detect this: index 0 is an ordinary array element, and is also what a control with noIndexproperty reads as. What identifies it is that a control of that name is already in the current scope.3. It always exited zero
Printing
1 errors foundand exiting 0 makescheckuseless as a CI gate — it passes in exactly the case where it should fail. Now exits 1 when it found something, following ruff's convention.4. Selectable rules
With the above out of the way there is somewhere to put per-file rules.
Each rule has a stable code, a fixability (
Safe,Unsafe,None) and a default; selection is by code or code prefix through--select/--ignoreor a[lint]section of the.aspen.tomlalready read for[fmt].The registry lives in
vb6parserather thanvb6format, so the analyzer does not depend on the formatter and semantic rules can later be registered fromvb6semanticthrough the same types.Contract change, worth calling out
analyze_projectno longer returnsErrwhen a file cannot be read: it reports it and keeps going.analyze_project_resolves_paths_against_base_diris updated to expect that — it is the one test whose expectations this PR deliberately changes.Result
checkon a 213-form projectKnown limitation
A file shared by several projects — one module here is referenced by eight — is analyzed once per project, so its findings are counted once per project in the total. The per-project report is right; the grand total over-counts. Deduplicating by canonical path is the obvious follow-up, left out to keep this reviewable.
Tests
Two in
vb6semantic: a second control of the same name is not a redefinition, and a name already taken by a variable still is.cargo test --workspace --locked: 5769 pass, with the same eight pre-existingclass_loadsnapshot failures as onmaster(no committed.snapbaseline).