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()))?; } 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", + ), + ); +} 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"); + } +} 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:?}"); + } + } }