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/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/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/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:?}"); + } + } } 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..3332c353f --- /dev/null +++ b/projects/vb6parse/src/lint.rs @@ -0,0 +1,468 @@ +//! 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 + .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 + // 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()); + } + + /// 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"; + + 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();