Add a lint layer with selectable rules, following ruff's model - #4
Closed
MarioRial22 wants to merge 5 commits into
Closed
Add a lint layer with selectable rules, following ruff's model#4MarioRial22 wants to merge 5 commits into
MarioRial22 wants to merge 5 commits into
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.
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 scriptandcompile#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 scriptandcompile#3.
Author
|
Closing in favour of #6, after a rethink prompted by a good question: why a new The honest answer is that I designed around #6 fixes the three limitations and puts the rules in |
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.
Implements what #3 proposes. Stacks on #2 — GitHub shows those four commits here too; the last commit,
Add a lint layer with selectable rules, is what this PR is about. Happy to rebase once #2 lands.What it adds
aspen lint, per-file and parse-only, so it stays fast enough for a pre-commit hook — unlikecheck, which needs a.vbpand runs the whole semantic analysis.Every rule carries a stable code, a fixability and a default; a run selects by code or code prefix.
Selection also comes from a
[lint]section of the.aspen.tomlthat is already read for[fmt]. Exit codes follow ruff:0clean,1findings,2the run itself failed.Two things worth reviewing
N001 reads the failure list, not the CST. It cannot be written as "an
Identifiertoken holding a non-ASCII character": the lexer takes identifiers withtake_ascii_underscore_alphanumericsand stops at the first non-ASCII byte, soAñadirnever becomes one token. The stray character reaches the tokenizer fallback, which records anUnknownTokenfailure and pushes no token, so it is absent from the CST entirely.That is also what makes the rule precise: comments and string literals are consumed whole by their own branches, so an accent in product text never reaches the fallback.
It skips lines that continue a comment. VB6 continues a logical line with a trailing
_, and that applies to comments — the lexer does not model this, so the next line is read as code and every accent in that prose looks like an identifier. On the code base I tested this on the difference was 2898 findings versus 2247, and the 651 removed were entirely accented vowels from Spanish prose. The remaining findings are allñ, which is what Spanish identifiers actually use (Año,Añadir,Diseño).The skip lives in the rule rather than the lexer, because changing how comments tokenize is a much larger change than this PR should carry. It is probably worth doing properly — say the word and I will open it separately.
Tests
Seven tests in
vb6format::lint: each rule's positive and negative case, the comment-continuation skip, selection by code and by prefix withignorewinning overselect, and a registry check that every code is unique and reachable.cargo test -p vb6format: 59 pass. Workspace-wide, the same eight pre-existingclass_loadsnapshot failures as onmaster(no committed.snapbaseline), unrelated to this.