Make aspen usable on a non-English VB6 code base: tokenizer hang, designer block, Windows-1252 round trip, exit code - #2
Conversation
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.
|
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 |
Four fixes found while putting
aspento work on a production VB6 code base: 11 compilable projects, ~660 source files, written in Spanish, so accented characters everywhere. They are separate commits and can be reviewed one at a time, but they are one PR because each one only becomes visible once the previous is out of the way — the hang hides the encoding problem, which hides the exit code problem.Before:
aspen checkhung on 6 of the 11 projects, andaspen fmtsilently processed 11 of 375 files.After:
checkcompletes on all 11 in 4.0s, andfmtreads all 375.1.
tokenize()hangs on a multi-byte character in codeThe tokenizer loop ends with a fallback that consumes one character and reports it as unknown, so the loop always makes progress:
SourceStream::take_countworks in bytes and returnsNonewhen the count would not land on a UTF-8 character boundary.ñis two bytes, so nothing is consumed, no branch matches on the next pass either, and the loop spins forever.Non-ASCII inside comments and string literals is fine —
take_line_commentandtake_string_literalconsume those wholesale, so the character never reaches the fallback. It has to sit in code, which an accented identifier does, and VB6 accepts those. A one-byte unknown character in the same position, such as a backtick, exits normally in 0.17s; the difference is purely the boundary check.Fix:
SourceStream::take_character(), which consumes exactly onecharhowever many bytes it occupies.take_countis left alone — its byte semantics are what every other caller wants. The loop also now breaks if the stream reports it is not empty yet nothing can be taken: a tokenizer that cannot advance should stop rather than hang.2.
fmtrewrites the designer block of forms and classesOn a small form here,
aspen fmtturnedVERSION 5.00intoVersion 5.00, flattened every control to column zero and took the file from 3436 to 2796 bytes.That block is not source code. The VB6 IDE writes it, reads it, and rewrites it in its own layout every time the form is saved, so reformatting it 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 can do to a VB6 project.
Fix:
VersionStatementandPropertiesBlocksubtrees are copied through unchanged. The same nodes cover a.clsheader, so class files are protected too. Code after the block is still formatted, which the third test pins down. On the same form the change is now 40 bytes instead of 640, all of it in the code section.3.
fmtcannot read Windows-1252, and would corrupt it on writeprocess_fileread withstd::fs::read_to_string, which requires valid UTF-8, so every source file holding an accented character was rejected — 364 of 375 files here. The summary still printed11 of 375 files would be reformatted, presenting the other 364 as fine.Reading is only half of it:
std::fs::writeemits the Rust string as UTF-8. Fixing the read alone would have turned "cannot read 364 files" into "silently transcodes 364 files to UTF-8", which VB6 then reads incorrectly — a worse failure, because it is invisible.Fix: read through
SourceFile, the wayvb6semanticalready does, and addvb6parse::io::encode_windows_1252to write back. Encoding is refused rather than made lossy:encoding_rsmaps a character outside Windows-1252 to?, losing it silently, so an unrepresentable character is returned as an error with its byte offset. Verified by cycling all 256 byte values through decode and back — Windows-1252 decodes every byte, so the whole range must survive, and it does.4.
fmtexits 0 when files failFailures were folded into "unchanged" and the exit code depended only on how many files would be reformatted:
A run where every file failed to read printed its errors and exited 0.
fmt --checktherefore passed in exactly the case where it should fail, which makes it worthless as a CI gate. Write mode never exited non-zero at all.Fix: failures are counted separately, summarised on stderr after the file list, and produce a non-zero exit in both modes.
Tests
Nine new tests. The three tokenizer ones hang rather than fail without the fix, which is the point of them.
lexer::tests::multibyte_character_*—ñ,año,€,日, covering 2, 3 and 4-byte characters; the whole character is reported as one unknown token and tokenizing continues past it; no regression for the comment and string-literal paths.io::source_file::encode_tests::*— accented round trip, all 256 byte values, and refusal to write?for an unrepresentable character.vb6format/tests/designer_block.rs— form block untouched,VERSIONcasing kept, code after the block still formatted.cargo test --workspace --lockedon Windows: 5761 pass. Eightclass_loadsnapshot tests fail (fiveaudiostation, threecdiu_beat_up_editor) because there is no committed.snapbaseline for them, so they store a.snap.newand fail. Those eight fail identically on an unmodifiedmasterhere — unrelated to this PR.Not addressed here
Two more things surfaced while testing, left out to keep this reviewable:
SourceFile::decode(the strict variant) rejects a plain Windows-1252 byte such as0xF1with "may not use latin-1 (Windows-1252) code page", which reports the opposite of what happened. The tests here usedecode_with_replacement, the path the tools actually take.On Error GotoforOn Error GoTo, and indents line labels that VB6 convention puts at column zero.Happy to split any of the four out if you would rather take them separately.