Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
116 changes: 39 additions & 77 deletions crates/beamtalk-core/src/lint/cascade_candidate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@
//! into a single cascade receiver.

use crate::ast::{Block, Expression, Identifier, Module};
use crate::ast_walker::for_each_expr_seq;
use crate::lint::LintPass;
use crate::source_analysis::Diagnostic;

Expand All @@ -32,7 +31,19 @@ pub(crate) struct CascadeCandidatePass;

impl LintPass for CascadeCandidatePass {
fn check(&self, module: &Module, diagnostics: &mut Vec<Diagnostic>) {
for_each_expr_seq(module, |seq| check_sequence(seq, diagnostics));
// Skip module.expressions — top-level sequences are used in bootstrap-test
// files as individual test assertions (each paired with its own `// =>`
// comment). Converting those to a cascade would change the semantics
// because only the last send's result would be asserted. Only check
// method bodies and standalone method definitions.
for class in &module.classes {
for method in class.methods.iter().chain(class.class_methods.iter()) {
check_sequence(&method.body, diagnostics);
}
}
for standalone in &module.method_definitions {
check_sequence(&standalone.method.body, diagnostics);
}
}
}

Expand Down Expand Up @@ -70,7 +81,13 @@ fn check_sequence(exprs: &[Expression], diagnostics: &mut Vec<Diagnostic>) {
i += 1;
}
let run_len = i - run_start;
if run_len >= 3 {
// Skip `self` — consecutive sends to `self` in method bodies are
// typically independent assertions or setup steps (e.g. BUnit's
// `self assert:…`) that should remain as separate statements for
// readability and debuggability. Cascading them would merge
// logically-distinct operations and make test failures harder to
// isolate.
if run_len >= 3 && recv != "self" {
let first_span = exprs[run_start].span();
let last_span = exprs[i - 1].span();
let span = first_span.merge(last_span);
Expand Down Expand Up @@ -212,11 +229,7 @@ mod tests {
#[test]
fn three_consecutive_sends_flagged() {
let diags = lint(
"Object subclass: Foo\n\
add: arr =>\n\
arr add: 1.\n\
arr add: 2.\n\
arr add: 3\n",
"Object subclass: Foo\n add: arr =>\n arr add: 1\n arr add: 2\n arr add: 3\n",
);
assert_eq!(
diags.len(),
Expand All @@ -234,12 +247,7 @@ mod tests {
#[test]
fn four_consecutive_sends_flagged_once() {
let diags = lint(
"Object subclass: Foo\n\
add: arr =>\n\
arr add: 1.\n\
arr add: 2.\n\
arr add: 3.\n\
arr add: 4\n",
"Object subclass: Foo\n add: arr =>\n arr add: 1\n arr add: 2\n arr add: 3\n arr add: 4\n",
);
assert_eq!(
diags.len(),
Expand All @@ -255,12 +263,7 @@ mod tests {

#[test]
fn two_consecutive_sends_not_flagged() {
let diags = lint(
"Object subclass: Foo\n\
add: arr =>\n\
arr add: 1.\n\
arr add: 2\n",
);
let diags = lint("Object subclass: Foo\n add: arr =>\n arr add: 1\n arr add: 2\n");
assert!(
diags.is_empty(),
"two sends should not be flagged, got: {diags:?}"
Expand All @@ -270,12 +273,7 @@ mod tests {
#[test]
fn interleaved_sends_not_flagged() {
let diags = lint(
"Object subclass: Foo\n\
add: arr with: other =>\n\
arr add: 1.\n\
other add: 99.\n\
arr add: 2.\n\
arr add: 3\n",
"Object subclass: Foo\n add: arr with: other =>\n arr add: 1\n other add: 99\n arr add: 2\n arr add: 3\n",
);
assert!(
diags.is_empty(),
Expand All @@ -286,46 +284,30 @@ mod tests {
#[test]
fn lint_has_category_and_hint() {
let diags = lint(
"Object subclass: Foo\n\
add: arr =>\n\
arr add: 1.\n\
arr add: 2.\n\
arr add: 3\n",
"Object subclass: Foo\n add: arr =>\n arr add: 1\n arr add: 2\n arr add: 3\n",
);
assert_eq!(diags.len(), 1);
assert_eq!(diags[0].category, Some(DiagnosticCategory::Lint));
assert!(diags[0].hint.is_some(), "expected a hint");
}

#[test]
fn self_receiver_flagged() {
let diags = lint(
"Object subclass: Foo\n\
run =>\n\
self doA.\n\
self doB.\n\
self doC\n",
);
assert_eq!(
diags.len(),
1,
"self sends should be flagged, got: {diags:?}"
);
fn self_receiver_not_flagged() {
// Consecutive sends to `self` (e.g. BUnit assertions) are intentionally
// exempt from the cascade lint — they represent independent operations
// that should remain as separate statements for debuggability.
let diags =
lint("Object subclass: Foo\n run =>\n self doA\n self doB\n self doC\n");
assert!(
diags[0].message.contains("self"),
"message: {}",
diags[0].message
diags.is_empty(),
"self sends should NOT be flagged, got: {diags:?}"
);
}

#[test]
fn complex_receiver_not_flagged() {
let diags = lint(
"Object subclass: Foo\n\
run: x =>\n\
(x builder) add: 1.\n\
(x builder) add: 2.\n\
(x builder) add: 3\n",
"Object subclass: Foo\n run: x =>\n (x builder) add: 1\n (x builder) add: 2\n (x builder) add: 3\n",
);
assert!(
diags.is_empty(),
Expand All @@ -336,9 +318,7 @@ mod tests {
#[test]
fn cascade_in_block_body_flagged() {
let diags = lint(
"Object subclass: Foo\n\
run: arr =>\n\
[:x | arr add: 1. arr add: 2. arr add: 3]\n",
"Object subclass: Foo\n run: arr =>\n [:x | arr add: 1. arr add: 2. arr add: 3]\n",
);
assert_eq!(
diags.len(),
Expand All @@ -351,11 +331,7 @@ mod tests {
fn mixed_selector_types_flagged() {
// Unary + keyword sends to the same receiver are still a cascade candidate.
let diags = lint(
"Object subclass: Foo\n\
run: arr =>\n\
arr add: 1.\n\
arr sort.\n\
arr add: 2\n",
"Object subclass: Foo\n run: arr =>\n arr add: 1\n arr sort\n arr add: 2\n",
);
assert_eq!(
diags.len(),
Expand All @@ -368,11 +344,7 @@ mod tests {
fn cast_send_breaks_run() {
// Cast sends (`!`) cannot appear in a cascade, so they break the run.
let diags = lint(
"Object subclass: Foo\n\
run: arr =>\n\
arr add: 1.\n\
arr fire!\n\
arr add: 2\n",
"Object subclass: Foo\n run: arr =>\n arr add: 1\n arr fire!\n arr add: 2\n",
);
assert!(
diags.is_empty(),
Expand All @@ -385,12 +357,7 @@ mod tests {
// `@expect` directives are separate expression nodes that break runs.
// This is correct: an @expect-annotated send has different semantics.
let diags = lint(
"Object subclass: Foo\n\
run: arr =>\n\
arr add: 1.\n\
@expect dnu\n\
arr add: 2.\n\
arr add: 3\n",
"Object subclass: Foo\n run: arr =>\n arr add: 1\n @expect dnu\n arr add: 2\n arr add: 3\n",
);
assert!(
diags.is_empty(),
Expand All @@ -401,12 +368,7 @@ mod tests {
#[test]
fn class_method_body_flagged() {
let diags = lint(
"Object subclass: Foo\n\
class\n\
setup: arr =>\n\
arr add: 1.\n\
arr add: 2.\n\
arr add: 3\n",
"Object subclass: Foo\n class\n setup: arr =>\n arr add: 1\n arr add: 2\n arr add: 3\n",
);
assert_eq!(
diags.len(),
Expand Down
32 changes: 22 additions & 10 deletions crates/beamtalk-core/src/semantic_analysis/validators.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
//! - Empty method bodies (BT-631)

use crate::ast::{Block, Expression, Identifier, Module};
use crate::ast_walker::{for_each_expr_seq, walk_expression, walk_module};
use crate::ast_walker::{walk_expression, walk_module};
use crate::semantic_analysis::block_context::{classify_block, is_collection_hof_selector};
use crate::semantic_analysis::{BlockContext, ClassHierarchy};
#[cfg(test)]
Expand Down Expand Up @@ -1077,14 +1077,27 @@ fn check_seq_for_effect_free(exprs: &[Expression], diagnostics: &mut Vec<Diagnos
/// BT-951: Warn (as a lint) when a statement is an effect-free expression
/// whose value is silently discarded.
///
/// Checks method bodies, block bodies, and module-level expression sequences.
/// 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.
///
/// 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<Diagnostic>) {
for_each_expr_seq(module, |seq| check_seq_for_effect_free(seq, diagnostics));
// Skip module.expressions — those are intentional top-level test assertions.
// Only check method bodies and standalone method definitions.
for class in &module.classes {
for method in class.methods.iter().chain(class.class_methods.iter()) {
check_seq_for_effect_free(&method.body, diagnostics);
}
}
for standalone in &module.method_definitions {
check_seq_for_effect_free(&standalone.method.body, diagnostics);
}
}

/// BT-919: Error when `!` (cast) is used on a statically-known value type.
Expand Down Expand Up @@ -1479,21 +1492,20 @@ mod tests {
);
}

/// Module-level expressions: non-last literal triggers lint.
/// Module-level expressions: NOT linted, because bootstrap-test files use
/// top-level expressions as intentional test assertions (paired with `// =>`).
#[test]
fn module_level_effect_free_emits_lint() {
fn module_level_effect_free_not_linted() {
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);
assert_eq!(
diagnostics.len(),
1,
"Expected 1 lint for discarded literal at module level, got: {diagnostics:?}"
assert!(
diagnostics.is_empty(),
"Expected no lint for module-level expressions (they are intentional test assertions), got: {diagnostics:?}"
);
assert_eq!(diagnostics[0].severity, Severity::Lint);
}

/// Standalone method definition: non-last literal triggers lint.
Expand Down
2 changes: 1 addition & 1 deletion stdlib/bootstrap-test/arithmetic.bt
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@
// => 5

// Double negative
0 - (-5)
0 - -5
// => 5

// Zero division operand (numerator)
Expand Down
4 changes: 2 additions & 2 deletions stdlib/bootstrap-test/array_ops.bt
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
// Copyright 2026 James Casey
// SPDX-License-Identifier: Apache-2.0

// Bootstrap-level smoke tests for the Array type (BT-822).
// Tests literal syntax, basic access, and core operations.
// Bootstrap-level smoke tests for the Array type (BT-822)
// Tests literal syntax, basic access, and core operations

// ===========================================================================
// ARRAY LITERALS
Expand Down
4 changes: 2 additions & 2 deletions stdlib/bootstrap-test/equality.bt
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,8 @@ true /= false

// Strict vs loose equality
// Note: 1.0 compiles to integer 1 in Core Erlang (Rust format! truncates .0)
// so 1.0 =:= 1 is true due to compilation artifact.
// Use values like 1.5 to demonstrate real strict equality behavior.
// so 1.0 =:= 1 is true due to compilation artifact
// Use values like 1.5 to demonstrate real strict equality behavior
1.5 =:= 1
// => false

Expand Down
2 changes: 1 addition & 1 deletion stdlib/bootstrap-test/erlang_exceptions.bt
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

// Tests for BEAM interop exception classes (BT-678)
// Verifies BEAMError, ExitError, ThrowError class hierarchy
// and exception catching behavior.
// and exception catching behavior

// ===========================================================================
// BEAM ERROR HIERARCHY — SIGNAL AND CATCH
Expand Down
20 changes: 10 additions & 10 deletions stdlib/bootstrap-test/errors.bt
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,9 @@
// Tests that runtime errors produce correct error kinds
// Validates BT-458: ERROR: assertion support in stdlib test framework
//
// These tests were migrated from E2E where they don't need REPL features.
// These tests were migrated from E2E where they don't need REPL features
// Stdlib tests match #beamtalk_error{kind} structurally (more precise than
// E2E substring matching).
// E2E substring matching)

// ===========================================================================
// ARITHMETIC ERRORS
Expand Down Expand Up @@ -65,8 +65,8 @@ nil nonExistentMethod
// ===========================================================================
// PRIMITIVE TYPE INSTANTIATION ERRORS (BT-422)
// ===========================================================================
// Primitive types are backed by native BEAM values, not maps.
// Calling `new` on them should raise an instantiation_error.
// Primitive types are backed by native BEAM values, not maps
// Calling `new` on them should raise an instantiation_error

Integer new
// => ERROR: instantiation_error
Expand Down Expand Up @@ -96,8 +96,8 @@ Block new
// => ERROR: instantiation_error

// new: on non-instantiable primitives still raises instantiation_error
// because the class can't be constructed at all (BT-473: checked first).
// BT-422: gen_server crash fix ensures class process survives the error.
// because the class can't be constructed at all (BT-473: checked first)
// BT-422: gen_server crash fix ensures class process survives the error

Integer new: 42
// => ERROR: instantiation_error
Expand All @@ -109,7 +109,7 @@ String new: "hello"
// NON-MAP ARGUMENT TO new: (BT-473)
// ===========================================================================
// Passing a non-Dictionary argument to new: should raise type_error,
// not silently fall back to new/0.
// not silently fall back to new/0

Dictionary new: 42
// => ERROR: type_error
Expand All @@ -124,9 +124,9 @@ Dictionary new: true
// FILE I/O ERRORS
// ===========================================================================
// NOTE: File I/O error tests temporarily removed — the underlying File
// operations raise function_clause instead of structured beamtalk_errors.
// See pre-existing failures in file_io.bt E2E tests.
// TODO: Re-add when beamtalk_file.erl properly handles these error paths.
// operations raise function_clause instead of structured beamtalk_errors
// See pre-existing failures in file_io.bt E2E tests
// TODO: Re-add when beamtalk_file.erl properly handles these error paths

// ===========================================================================
// ACTOR INSTANTIATION ERRORS (BT-623)
Expand Down
2 changes: 1 addition & 1 deletion stdlib/bootstrap-test/exceptions.bt
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
// SPDX-License-Identifier: Apache-2.0

// E2E tests for exception handling (BT-338)
// Tests Block on:do:, ensure:, and Exception object inspection.
// Tests Block on:do:, ensure:, and Exception object inspection

// ===========================================================================
// BLOCK on:do: — BASIC EXCEPTION CATCHING
Expand Down
Loading