From 28bacd690ed5dcd65d9160017fce2095953a71c2 Mon Sep 17 00:00:00 2001 From: James Casey Date: Sat, 28 Feb 2026 23:45:14 +0000 Subject: [PATCH] feat: warn when non-last module-level expression result is discarded BT-979 - Enable effect-free lint check on module.expressions by default - Add skip_module_expression_lint flag to CompilerOptions for bootstrap-test opt-out - Include Severity::Lint in REPL compiler port warnings so users see the hint - Add tests for module-level expression linting (default-on and opt-out) Co-Authored-By: Claude Sonnet 4.6 --- .../beamtalk-cli/src/commands/test_stdlib.rs | 4 ++ crates/beamtalk-cli/src/main.rs | 1 + crates/beamtalk-compiler-port/src/main.rs | 3 + crates/beamtalk-core/src/lib.rs | 8 +++ .../src/lint/effect_free_statement.rs | 40 +++++++++++- .../src/semantic_analysis/mod.rs | 28 ++++++-- .../src/semantic_analysis/validators.rs | 65 +++++++++++++------ 7 files changed, 121 insertions(+), 28 deletions(-) diff --git a/crates/beamtalk-cli/src/commands/test_stdlib.rs b/crates/beamtalk-cli/src/commands/test_stdlib.rs index 4fd493b06..d0cea79c0 100644 --- a/crates/beamtalk-cli/src/commands/test_stdlib.rs +++ b/crates/beamtalk-cli/src/commands/test_stdlib.rs @@ -369,6 +369,10 @@ fn compile_fixture( allow_primitives: false, workspace_mode: false, suppress_warnings, + // BT-979: Bootstrap-test fixtures use top-level expressions as test assertions + // paired with `// =>` comments. Skip the module-expression lint to avoid + // false positives on intentional assertion expressions. + skip_module_expression_lint: true, }; crate::beam_compiler::compile_source(fixture_path, &module_name, &core_file, &options) diff --git a/crates/beamtalk-cli/src/main.rs b/crates/beamtalk-cli/src/main.rs index 34e136562..df74031aa 100644 --- a/crates/beamtalk-cli/src/main.rs +++ b/crates/beamtalk-cli/src/main.rs @@ -315,6 +315,7 @@ fn run() -> Result<()> { allow_primitives, workspace_mode: false, suppress_warnings: no_warnings, + ..Default::default() }; commands::build::build(&path, &options) } diff --git a/crates/beamtalk-compiler-port/src/main.rs b/crates/beamtalk-compiler-port/src/main.rs index c72b629e4..2b0071ba5 100644 --- a/crates/beamtalk-compiler-port/src/main.rs +++ b/crates/beamtalk-compiler-port/src/main.rs @@ -382,6 +382,8 @@ fn handle_compile_expression(request: &Map) -> Term { d.severity, beamtalk_core::source_analysis::Severity::Warning | beamtalk_core::source_analysis::Severity::Hint + // BT-979: Include lint warnings so REPL users see effect-free statement hints. + | beamtalk_core::source_analysis::Severity::Lint ) }) .map(|d| d.message.to_string()) @@ -556,6 +558,7 @@ fn handle_compile(request: &Map) -> Term { allow_primitives: false, workspace_mode, suppress_warnings: false, + ..Default::default() }; let primitive_diags = beamtalk_core::semantic_analysis::primitive_validator::validate_primitives( diff --git a/crates/beamtalk-core/src/lib.rs b/crates/beamtalk-core/src/lib.rs index c7d324771..f623837f6 100644 --- a/crates/beamtalk-core/src/lib.rs +++ b/crates/beamtalk-core/src/lib.rs @@ -63,4 +63,12 @@ pub struct CompilerOptions { /// When true, suppress warning diagnostics during compilation. /// Useful for test fixtures that intentionally trigger warnings. pub suppress_warnings: bool, + + /// BT-979: When true, skip the effect-free lint check on `module.expressions`. + /// + /// Set this for bootstrap-test compilation, where top-level expressions are + /// intentional test assertions (paired with `// =>` comments) rather than + /// accidentally discarded values. Defaults to false so the REPL and normal + /// `beamtalk build` / `beamtalk lint` paths all get the check. + pub skip_module_expression_lint: bool, } diff --git a/crates/beamtalk-core/src/lint/effect_free_statement.rs b/crates/beamtalk-core/src/lint/effect_free_statement.rs index a4b06dbcb..1d5e79b9e 100644 --- a/crates/beamtalk-core/src/lint/effect_free_statement.rs +++ b/crates/beamtalk-core/src/lint/effect_free_statement.rs @@ -26,7 +26,12 @@ pub(crate) struct EffectFreeStatementPass; impl LintPass for EffectFreeStatementPass { fn check(&self, module: &Module, diagnostics: &mut Vec) { - crate::semantic_analysis::validators::check_effect_free_statements(module, diagnostics); + // BT-979: lint always checks module.expressions (skip_module_expression_lint = false) + crate::semantic_analysis::validators::check_effect_free_statements( + module, + diagnostics, + false, + ); } } @@ -103,4 +108,37 @@ mod tests { "Expected no effect-free lints, got: {effect_free:?}" ); } + + // BT-979: Module-level expression linting + + #[test] + fn discarded_module_level_map_literal_surfaced() { + // Two module-level expressions: the first (map literal) is discarded. + // This matches the REPL scenario: `#{#x => 1} #{#y => 2}` + let diags = lint("#{#x => 1}\n#{#y => 2}"); + let effect_free: Vec<_> = diags + .iter() + .filter(|d| d.message.contains("no effect")) + .collect(); + assert_eq!( + effect_free.len(), + 1, + "Expected 1 effect-free lint for discarded module-level map literal, got: {effect_free:?}" + ); + assert!(effect_free[0].message.contains("map literal")); + } + + #[test] + fn single_module_level_expression_not_flagged() { + // A single module-level expression is the last (and only) expression — not flagged. + let diags = lint("1 + 2"); + let effect_free: Vec<_> = diags + .iter() + .filter(|d| d.message.contains("no effect")) + .collect(); + assert!( + effect_free.is_empty(), + "Single module-level expression should not be flagged: {effect_free:?}" + ); + } } diff --git a/crates/beamtalk-core/src/semantic_analysis/mod.rs b/crates/beamtalk-core/src/semantic_analysis/mod.rs index 18742ebcc..9a0228004 100644 --- a/crates/beamtalk-core/src/semantic_analysis/mod.rs +++ b/crates/beamtalk-core/src/semantic_analysis/mod.rs @@ -183,7 +183,7 @@ pub enum MutationKind { /// assert_eq!(result.diagnostics.len(), 0); /// ``` pub fn analyse(module: &Module) -> AnalysisResult { - analyse_full(module, &[], false) + analyse_full(module, &[], false, false) } /// Analyse a module with pre-defined variables (for REPL context). @@ -197,7 +197,7 @@ pub fn analyse(module: &Module) -> AnalysisResult { /// services. Pre-defining known variables is essential for REPL contexts where /// users build up state incrementally across multiple evaluations. pub fn analyse_with_known_vars(module: &Module, known_vars: &[&str]) -> AnalysisResult { - analyse_full(module, known_vars, false) + analyse_full(module, known_vars, false, false) } /// Analyse a module with compiler options controlling stdlib-specific behaviour. @@ -205,11 +205,21 @@ pub fn analyse_with_known_vars(module: &Module, known_vars: &[&str]) -> Analysis /// When `stdlib_mode` is true, built-in classes are permitted to subclass sealed /// classes (BT-791). This should only be set when compiling stdlib sources. pub fn analyse_with_options(module: &Module, options: &crate::CompilerOptions) -> AnalysisResult { - analyse_full(module, &[], options.stdlib_mode) + analyse_full( + module, + &[], + options.stdlib_mode, + options.skip_module_expression_lint, + ) } /// Internal: full analysis with all knobs. -fn analyse_full(module: &Module, known_vars: &[&str], stdlib_mode: bool) -> AnalysisResult { +fn analyse_full( + module: &Module, + known_vars: &[&str], + stdlib_mode: bool, + skip_module_expression_lint: bool, +) -> AnalysisResult { let mut result = AnalysisResult::new(); // Phase 0: Build Class Hierarchy (ADR 0006 Phase 1a) @@ -280,8 +290,14 @@ fn analyse_full(module: &Module, known_vars: &[&str], stdlib_mode: bool) -> Anal // BT-955: Warn on literal boolean conditions (always true / always false) validators::check_literal_boolean_condition(module, &mut result.diagnostics); - // BT-951: Lint on effect-free statements (suppressed during normal compile) - validators::check_effect_free_statements(module, &mut result.diagnostics); + // BT-951/BT-979: Lint on effect-free statements (suppressed during normal compile). + // Module-level expressions are checked by default; set skip_module_expression_lint + // to opt out (bootstrap-test compilation uses this). + validators::check_effect_free_statements( + module, + &mut result.diagnostics, + skip_module_expression_lint, + ); // Phase 6: Module-level validation (BT-349) let module_diags = module_validator::validate_single_class(module); diff --git a/crates/beamtalk-core/src/semantic_analysis/validators.rs b/crates/beamtalk-core/src/semantic_analysis/validators.rs index ae1542a3f..6d39a2c7f 100644 --- a/crates/beamtalk-core/src/semantic_analysis/validators.rs +++ b/crates/beamtalk-core/src/semantic_analysis/validators.rs @@ -1094,19 +1094,26 @@ fn check_seq_for_effect_free( /// BT-951: Warn (as a lint) when a statement is an effect-free expression /// whose value is silently discarded. /// -/// Checks method bodies and standalone method bodies only. Module-level -/// expressions (`module.expressions`) are intentionally skipped because -/// bootstrap-test files use top-level expressions as test assertions -/// (paired with `// =>` comments) and these are evaluated intentionally. +/// Checks method bodies, standalone method bodies, and (by default) +/// module-level expressions (`module.expressions`). Pass +/// `skip_module_expression_lint = true` to suppress the module-level check — +/// bootstrap-test compilation uses this because those files intentionally use +/// top-level expressions as test assertions paired with `// =>` comments. /// /// Effect-free expressions include literals, variable references, and pure /// binary arithmetic / comparison expressions composed from pure sub-expressions. /// /// Uses `Severity::Lint` so the warning is suppressed during normal compilation -/// and only surfaces when running `beamtalk lint`. -pub(crate) fn check_effect_free_statements(module: &Module, diagnostics: &mut Vec) { - // Skip module.expressions — those are intentional top-level test assertions. - // Only check method bodies and standalone method definitions. +/// and only surfaces when running `beamtalk lint` or in the REPL. +pub(crate) fn check_effect_free_statements( + module: &Module, + diagnostics: &mut Vec, + skip_module_expression_lint: bool, +) { + // BT-979: Check module-level expressions unless the caller opts out. + if !skip_module_expression_lint { + check_seq_for_effect_free(&module.expressions, diagnostics); + } for class in &module.classes { for method in class.methods.iter().chain(class.class_methods.iter()) { check_seq_for_effect_free(&method.body, diagnostics); @@ -1389,7 +1396,7 @@ mod tests { let (module, parse_diags) = parse(tokens); assert!(parse_diags.is_empty(), "Parse failed: {parse_diags:?}"); let mut diagnostics = Vec::new(); - check_effect_free_statements(&module, &mut diagnostics); + check_effect_free_statements(&module, &mut diagnostics, false); assert!( diagnostics.is_empty(), "Expected no lint for single literal return value, got: {diagnostics:?}" @@ -1404,7 +1411,7 @@ mod tests { let (module, parse_diags) = parse(tokens); assert!(parse_diags.is_empty(), "Parse failed: {parse_diags:?}"); let mut diagnostics = Vec::new(); - check_effect_free_statements(&module, &mut diagnostics); + check_effect_free_statements(&module, &mut diagnostics, false); assert_eq!( diagnostics.len(), 1, @@ -1430,7 +1437,7 @@ mod tests { let (module, parse_diags) = parse(tokens); assert!(parse_diags.is_empty(), "Parse failed: {parse_diags:?}"); let mut diagnostics = Vec::new(); - check_effect_free_statements(&module, &mut diagnostics); + check_effect_free_statements(&module, &mut diagnostics, false); assert_eq!( diagnostics.len(), 1, @@ -1447,7 +1454,7 @@ mod tests { let (module, parse_diags) = parse(tokens); assert!(parse_diags.is_empty(), "Parse failed: {parse_diags:?}"); let mut diagnostics = Vec::new(); - check_effect_free_statements(&module, &mut diagnostics); + check_effect_free_statements(&module, &mut diagnostics, false); assert_eq!( diagnostics.len(), 1, @@ -1469,7 +1476,7 @@ mod tests { let (module, parse_diags) = parse(tokens); assert!(parse_diags.is_empty(), "Parse failed: {parse_diags:?}"); let mut diagnostics = Vec::new(); - check_effect_free_statements(&module, &mut diagnostics); + check_effect_free_statements(&module, &mut diagnostics, false); assert!( diagnostics.is_empty(), "Expected no lint for effectful sends, got: {diagnostics:?}" @@ -1484,7 +1491,7 @@ mod tests { let (module, parse_diags) = parse(tokens); assert!(parse_diags.is_empty(), "Parse failed: {parse_diags:?}"); let mut diagnostics = Vec::new(); - check_effect_free_statements(&module, &mut diagnostics); + check_effect_free_statements(&module, &mut diagnostics, false); assert_eq!( diagnostics.len(), 1, @@ -1501,7 +1508,7 @@ mod tests { let (module, parse_diags) = parse(tokens); assert!(parse_diags.is_empty(), "Parse failed: {parse_diags:?}"); let mut diagnostics = Vec::new(); - check_effect_free_statements(&module, &mut diagnostics); + check_effect_free_statements(&module, &mut diagnostics, false); assert_eq!( diagnostics.len(), 2, @@ -1509,19 +1516,35 @@ mod tests { ); } - /// Module-level expressions: NOT linted, because bootstrap-test files use - /// top-level expressions as intentional test assertions (paired with `// =>`). + /// BT-979: Module-level expressions ARE linted by default. + #[test] + fn module_level_effect_free_linted_by_default() { + let src = "42.\nself doSomething"; + let tokens = lex_with_eof(src); + let (module, parse_diags) = parse(tokens); + assert!(parse_diags.is_empty(), "Parse failed: {parse_diags:?}"); + let mut diagnostics = Vec::new(); + check_effect_free_statements(&module, &mut diagnostics, false); + assert_eq!( + diagnostics.len(), + 1, + "Expected 1 lint for discarded module-level literal, got: {diagnostics:?}" + ); + assert_eq!(diagnostics[0].severity, Severity::Lint); + } + + /// BT-979: Module-level expressions NOT linted when opt-out flag is set. #[test] - fn module_level_effect_free_not_linted() { + fn module_level_effect_free_skipped_with_flag() { let src = "42.\nself doSomething"; let tokens = lex_with_eof(src); let (module, parse_diags) = parse(tokens); assert!(parse_diags.is_empty(), "Parse failed: {parse_diags:?}"); let mut diagnostics = Vec::new(); - check_effect_free_statements(&module, &mut diagnostics); + check_effect_free_statements(&module, &mut diagnostics, true); assert!( diagnostics.is_empty(), - "Expected no lint for module-level expressions (they are intentional test assertions), got: {diagnostics:?}" + "Expected no lint when skip_module_expression_lint is true, got: {diagnostics:?}" ); } @@ -1538,7 +1561,7 @@ mod tests { "Expected 1 standalone method" ); let mut diagnostics = Vec::new(); - check_effect_free_statements(&module, &mut diagnostics); + check_effect_free_statements(&module, &mut diagnostics, false); assert_eq!( diagnostics.len(), 1,