From 0350b823a860cf9096fc6693ce07a0fed5ad3eee Mon Sep 17 00:00:00 2001 From: Pepe Navarro Date: Wed, 3 Jun 2026 21:56:38 +0200 Subject: [PATCH 01/27] chore(diag): renumber E0020-E0022 to free slots for ownership pass MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The ownership pass (M8.1b) reserves E0020/E0021/E0022 per spec. Move ImmutableAssign/DuplicateDeclaration/UndefinedAssignTarget to E0028/E0029/E0030. Pure label change — pre-alpha, no external consumers of the codes yet. --- src/pipeline.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/pipeline.rs b/src/pipeline.rs index 110f692..361dd25 100644 --- a/src/pipeline.rs +++ b/src/pipeline.rs @@ -220,9 +220,9 @@ fn diag_code_str(code: DiagCode) -> &'static str { DiagCode::UnsupportedOperator => "E0015", DiagCode::VoidValueInExpression => "E0017", DiagCode::ConditionNotBool => "E0018", - DiagCode::ImmutableAssign => "E0020", - DiagCode::DuplicateDeclaration => "E0021", - DiagCode::UndefinedAssignTarget => "E0022", + DiagCode::ImmutableAssign => "E0028", + DiagCode::DuplicateDeclaration => "E0029", + DiagCode::UndefinedAssignTarget => "E0030", DiagCode::FloatModulo => "E0023", DiagCode::BreakOutsideLoop => "E0024", DiagCode::ContinueOutsideLoop => "E0025", From 4c8ad8ed5aacb1c8fd2aa361b85bdc6c2ce05bb7 Mon Sep 17 00:00:00 2001 From: Pepe Navarro Date: Wed, 3 Jun 2026 22:11:39 +0200 Subject: [PATCH 02/27] feat(lexer): add 'move' keyword token First token reserved for ownership annotations on parameters. Used in M8.1b to mark parameters as taking ownership. --- src/lexer.rs | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/lexer.rs b/src/lexer.rs index c2ea72f..f82608f 100644 --- a/src/lexer.rs +++ b/src/lexer.rs @@ -46,6 +46,7 @@ pub enum Token { Else, Return, Mut, + Move, Struct, Enum, Match, @@ -116,6 +117,7 @@ impl fmt::Display for Token { Self::Else => write!(f, "else"), Self::Return => write!(f, "return"), Self::Mut => write!(f, "mut"), + Self::Move => write!(f, "move"), Self::Struct => write!(f, "struct"), Self::Enum => write!(f, "enum"), Self::Match => write!(f, "match"), @@ -191,6 +193,8 @@ pub(crate) enum RawToken<'a> { Return, #[token("mut")] Mut, + #[token("move")] + Move, #[token("struct")] Struct, #[token("enum")] @@ -423,6 +427,7 @@ fn intern_token(raw: RawToken<'_>, span: Span, pool: &mut InternPool) -> Result< RawToken::Else => Token::Else, RawToken::Return => Token::Return, RawToken::Mut => Token::Mut, + RawToken::Move => Token::Move, RawToken::Struct => Token::Struct, RawToken::Enum => Token::Enum, RawToken::Match => Token::Match, @@ -514,6 +519,13 @@ mod tests { assert_eq!(toks[7], Token::Match); } + #[test] + fn lex_move_keyword() { + let (toks, _) = lex_strings("move"); + assert_eq!(toks.len(), 1); + assert!(matches!(toks[0], Token::Move)); + } + #[test] fn lex_simple_identifier() { let (toks, pool) = lex_strings("foo"); From 62160b4c1cbdaedfb3ae6d815c90bbb875da4af7 Mon Sep 17 00:00:00 2001 From: Pepe Navarro Date: Wed, 3 Jun 2026 22:20:07 +0200 Subject: [PATCH 03/27] feat(ast): add is_move flag to Param Defaults to false at every construction site. Parser will emit true when 'move' precedes the parameter name (next task). --- src/ast.rs | 5 ++++- src/parser.rs | 1 + 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/src/ast.rs b/src/ast.rs index f9f2c62..e45bfe7 100644 --- a/src/ast.rs +++ b/src/ast.rs @@ -77,9 +77,11 @@ impl Statement { println!("{}FunctionDef: {}", prefix, pool.str(func.name.name)); let inner = format!("{} ", prefix); for param in &func.params { + let move_prefix = if param.is_move { "move " } else { "" }; println!( - "{}├── param: {}: {}", + "{}├── param: {}{}: {}", inner, + move_prefix, pool.str(param.name.name), pool.str(param.type_annotation.name), ); @@ -255,6 +257,7 @@ pub struct FunctionDef { pub struct Param { pub name: Ident, pub type_annotation: TypeExpr, + pub is_move: bool, pub span: SimpleSpan, } diff --git a/src/parser.rs b/src/parser.rs index 08bb6ec..00cb519 100644 --- a/src/parser.rs +++ b/src/parser.rs @@ -319,6 +319,7 @@ where .map_with(|(name, type_annotation), e| Param { name, type_annotation, + is_move: false, span: e.span(), }); From 148283f922f3b070efaf721dec1a50903fa5d543 Mon Sep 17 00:00:00 2001 From: Pepe Navarro Date: Wed, 3 Jun 2026 22:32:12 +0200 Subject: [PATCH 04/27] feat(parser): accept 'move' before parameter name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit fn consume(move s: str): ... — sets is_move=true on the Param. Bare params remain is_move=false (the default-borrow case). --- src/parser.rs | 39 +++++++++++++++++++++++++++++++++++---- 1 file changed, 35 insertions(+), 4 deletions(-) diff --git a/src/parser.rs b/src/parser.rs index 00cb519..3426afd 100644 --- a/src/parser.rs +++ b/src/parser.rs @@ -312,14 +312,16 @@ where let type_expr = select! { Token::Ident(name) => name }.map_with(|name, e| TypeExpr::new(name, e.span())); - let param = select! { Token::Ident(name) => name } - .map_with(|name, e| Ident::new(name, e.span())) + let param = just(Token::Move) + .or_not() + .map(|m| m.is_some()) + .then(select! { Token::Ident(name) => name }.map_with(|name, e| Ident::new(name, e.span()))) .then_ignore(just(Token::Colon)) .then(type_expr) - .map_with(|(name, type_annotation), e| Param { + .map_with(|((is_move, name), type_annotation), e| Param { name, type_annotation, - is_move: false, + is_move, span: e.span(), }); @@ -1418,4 +1420,33 @@ mod tests { let result = lex_and_parse(code); assert!(result.is_err(), "range() should fail with arity error"); } + + #[test] + fn parse_move_parameter() { + let (program, pool) = lex_and_parse("fn consume(move s: str):\n\tprint(s)\n").unwrap(); + let func = match &program.statements[0].kind { + StmtKind::FunctionDef(f) => f, + other => panic!("expected FunctionDef, got {:?}", other), + }; + assert_eq!(func.params.len(), 1); + assert!(func.params[0].is_move, "param `s` should be marked move"); + assert_eq!(pool.str(func.params[0].name.name), "s"); + assert_eq!(pool.str(func.params[0].type_annotation.name), "str"); + } + + #[test] + fn parse_default_parameter_is_not_move() { + let (program, pool) = lex_and_parse("fn read(s: str):\n\tprint(s)\n").unwrap(); + let func = match &program.statements[0].kind { + StmtKind::FunctionDef(f) => f, + other => panic!("expected FunctionDef, got {:?}", other), + }; + assert_eq!(func.params.len(), 1); + assert!( + !func.params[0].is_move, + "bare param `s` should default to is_move=false" + ); + assert_eq!(pool.str(func.params[0].name.name), "s"); + assert_eq!(pool.str(func.params[0].type_annotation.name), "str"); + } } From 42090de3cd4dbc7601eb542c89fa031913b549b2 Mon Sep 17 00:00:00 2001 From: Pepe Navarro Date: Wed, 3 Jun 2026 22:38:36 +0200 Subject: [PATCH 05/27] feat(uir): thread is_move through UirParam AstGen propagates the AST flag into UIR. Sema and the ownership pass will read it next. --- src/astgen.rs | 1 + src/uir.rs | 1 + 2 files changed, 2 insertions(+) diff --git a/src/astgen.rs b/src/astgen.rs index 81d2c48..bad3435 100644 --- a/src/astgen.rs +++ b/src/astgen.rs @@ -190,6 +190,7 @@ fn gen_function_def( pool, sink, ), + is_move: p.is_move, span: p.name.span, }) .collect(); diff --git a/src/uir.rs b/src/uir.rs index 5664eec..67a8f15 100644 --- a/src/uir.rs +++ b/src/uir.rs @@ -249,6 +249,7 @@ pub struct Inst { pub struct UirParam { pub name: StringId, pub ty: TypeId, + pub is_move: bool, pub span: Span, } From 7b4a31804d1c5e56d2374fcc89d916eea4c16cd8 Mon Sep 17 00:00:00 2001 From: Pepe Navarro Date: Wed, 3 Jun 2026 22:41:53 +0200 Subject: [PATCH 06/27] feat(tir): thread is_move through TirParam Sema propagates the UIR flag into TIR. The ownership pass will read it to distinguish move-consumption from borrow. --- src/sema.rs | 1 + src/tir.rs | 4 ++++ 2 files changed, 5 insertions(+) diff --git a/src/sema.rs b/src/sema.rs index 6ad4edc..5a19497 100644 --- a/src/sema.rs +++ b/src/sema.rs @@ -363,6 +363,7 @@ fn analyze_function(sema: &mut Sema<'_>, body: &FuncBody) -> Tir { .map(|p| TirParam { name: p.name, ty: p.ty, + is_move: p.is_move, span: p.span, }) .collect(); diff --git a/src/tir.rs b/src/tir.rs index e7e4696..c6089ff 100644 --- a/src/tir.rs +++ b/src/tir.rs @@ -246,6 +246,7 @@ pub struct TypedInst { pub struct TirParam { pub name: StringId, pub ty: TypeId, + pub is_move: bool, pub span: Span, } @@ -963,6 +964,9 @@ impl<'a> fmt::Display for TirDump<'a> { if i > 0 { write!(f, ", ")?; } + if p.is_move { + write!(f, "move ")?; + } write!(f, "{}: {}", self.pool.str(p.name), self.pool.display(p.ty))?; } writeln!(f, ") -> {}", self.pool.display(tir.return_type))?; From 89eb33b693b9452660504e78a6bd35c93ce3a511 Mon Sep 17 00:00:00 2001 From: Pepe Navarro Date: Wed, 3 Jun 2026 22:50:49 +0200 Subject: [PATCH 07/27] feat(sema): warn on 'move' annotation for Copy-typed parameters W0002 RedundantMove fires when 'move' precedes a parameter of Copy type (int, float, bool). Move on str remains silent. Warnings now render on the success path of lower_and_analyze. --- src/diag.rs | 15 ++++++++++++++- src/pipeline.rs | 8 ++++++++ src/sema.rs | 31 +++++++++++++++++++++++++++++++ tests/integration_tests.rs | 34 ++++++++++++++++++++++++++++++++++ 4 files changed, 87 insertions(+), 1 deletion(-) diff --git a/src/diag.rs b/src/diag.rs index 47232c5..69e3ec1 100644 --- a/src/diag.rs +++ b/src/diag.rs @@ -43,7 +43,6 @@ pub struct Diag { #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum Severity { Error, - #[allow(dead_code)] Warning, #[allow(dead_code)] Note, @@ -97,6 +96,10 @@ pub enum DiagCode { RangeArgType, /// User attempted to declare a function or variable with a reserved builtin name. ReservedBuiltinName, + /// `move` annotation on a Copy-typed parameter (int, float, bool). + /// Accepted, but redundant — Copy types are duplicated on + /// assignment regardless. W-prefixed because it's a warning. + RedundantMove, /// A declaration's resolution requires its own resolution to be /// already complete — e.g. a chain of decls whose types depend @@ -140,6 +143,16 @@ impl Diag { } } + pub fn warning(span: Span, code: DiagCode, message: impl Into) -> Self { + Diag { + span, + severity: Severity::Warning, + code, + message: message.into(), + notes: Vec::new(), + } + } + #[allow(dead_code)] pub fn with_note(mut self, span: Option, message: impl Into) -> Self { self.notes.push(DiagNote { diff --git a/src/pipeline.rs b/src/pipeline.rs index 361dd25..ad14413 100644 --- a/src/pipeline.rs +++ b/src/pipeline.rs @@ -228,6 +228,7 @@ fn diag_code_str(code: DiagCode) -> &'static str { DiagCode::ContinueOutsideLoop => "E0025", DiagCode::RangeArgType => "E0026", DiagCode::ReservedBuiltinName => "E0027", + DiagCode::RedundantMove => "W0002", DiagCode::CycleInResolution => "E0016", DiagCode::ParseError => "E0100", DiagCode::TooManyDiagnostics => "E0101", @@ -394,6 +395,13 @@ fn lower_and_analyze( render_diags(&diags, input, source_name); return Err(CompilerError::Diagnostics(diags)); } + // No errors — but warnings/notes may still be queued. Render + // them so the user sees `move`-on-Copy lints (W0002) and any + // future lints on otherwise-successful builds. + let diags = sink.into_diags(); + if !diags.is_empty() { + render_diags(&diags, input, source_name); + } Ok(tirs) } diff --git a/src/sema.rs b/src/sema.rs index 5a19497..9dbdff6 100644 --- a/src/sema.rs +++ b/src/sema.rs @@ -54,6 +54,17 @@ use crate::uir::{CallView, FuncBody, InstData, InstRef, InstTag, Span, Uir, VarD use std::collections::{HashMap, VecDeque}; use std::path::Path; +// ---------- Copy-type classification ---------- + +/// True for types whose values are duplicated on read (no destructor, +/// no aliasing concern). Mirrors Mojo's `Copyable` trait for the +/// scalar primitives Ryo currently has. Used by sema to flag +/// redundant `move` annotations and (later) by the ownership pass +/// to short-circuit liveness on these instructions. +fn is_copy_type_kind(kind: TypeKind) -> bool { + matches!(kind, TypeKind::Int | TypeKind::Float | TypeKind::Bool) +} + // ---------- Decl table ---------- /// Index into `uir.func_bodies`. One [`DeclId`] per function the @@ -357,6 +368,26 @@ fn analyze_function(sema: &mut Sema<'_>, body: &FuncBody) -> Tir { scope.insert_binding(param.name, param.ty, false); } + // W0002: warn on `move` annotations applied to Copy-typed + // parameters. Copy types (int, float, bool) are duplicated on + // every read regardless of the annotation, so `move` is + // redundant noise. `move` on `str` (and other heap types) stays + // silent — that's the whole reason the keyword exists. + for param in &body.params { + if param.is_move && is_copy_type_kind(sema.pool.kind(param.ty)) { + let name = sema.pool.str(param.name).to_string(); + let ty_str = sema.pool.display(param.ty).to_string(); + sema.sink.emit(Diag::warning( + param.span, + DiagCode::RedundantMove, + format!( + "redundant 'move' on Copy-typed parameter '{}': {} values are copied on every read", + name, ty_str, + ), + )); + } + } + let params: Vec = body .params .iter() diff --git a/tests/integration_tests.rs b/tests/integration_tests.rs index 07cf35f..f088eee 100644 --- a/tests/integration_tests.rs +++ b/tests/integration_tests.rs @@ -2279,3 +2279,37 @@ fn test_str_shadowed_by_int_assignment_does_not_panic() { stdout ); } + +#[test] +fn test_redundant_move_on_int_warns() { + let temp_dir = TempDir::new().expect("temp dir"); + let code = "fn f(move x: int):\n\tprint(int_to_str(x))\n\nf(42)"; + let test_file = create_test_file(temp_dir.path(), "redundant_move.ryo", code); + let output = run_ryo_command(&["run", "redundant_move.ryo"], &test_file).expect("run"); + assert!( + output.status.success(), + "STDERR: {}", + String::from_utf8_lossy(&output.stderr) + ); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + stderr.contains("W0002"), + "expected W0002 in stderr: {}", + stderr + ); +} + +#[test] +fn test_move_on_str_no_warning() { + let temp_dir = TempDir::new().expect("temp dir"); + let code = "fn f(move s: str):\n\tprint(s)\n\nf(\"hi\")"; + let test_file = create_test_file(temp_dir.path(), "move_on_str.ryo", code); + let output = run_ryo_command(&["run", "move_on_str.ryo"], &test_file).expect("run"); + assert!( + output.status.success(), + "STDERR: {}", + String::from_utf8_lossy(&output.stderr) + ); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!(!stderr.contains("W0002"), "unexpected W0002: {}", stderr); +} From 7a1f127ff1bce39ef3d2779571cf9cc08fef5f22 Mon Sep 17 00:00:00 2001 From: Pepe Navarro Date: Wed, 3 Jun 2026 22:58:16 +0200 Subject: [PATCH 08/27] feat(ownership): add ownership module skeleton + pipeline wiring Module is a no-op for now. Sits between sema and codegen, consumes &[Tir] and a DiagSink, mutates nothing. Subsequent tasks fill in the lattice and forward walk. --- src/main.rs | 1 + src/ownership.rs | 39 +++++++++++++++++++++++++++++++++++++++ src/pipeline.rs | 2 ++ 3 files changed, 42 insertions(+) create mode 100644 src/ownership.rs diff --git a/src/main.rs b/src/main.rs index 5e1a413..cea180a 100644 --- a/src/main.rs +++ b/src/main.rs @@ -10,6 +10,7 @@ mod errors; mod indent; mod lexer; mod linker; +mod ownership; mod parser; mod pipeline; #[allow(dead_code)] diff --git a/src/ownership.rs b/src/ownership.rs new file mode 100644 index 0000000..740eb84 --- /dev/null +++ b/src/ownership.rs @@ -0,0 +1,39 @@ +//! Ownership pass — validates move safety on per-`TirRef` lattice. +//! +//! Runs between sema and codegen. Walks each `Tir` forward, tracking +//! ownership state for every Move-typed value. Catches use-after-move, +//! moves out of borrowed parameters, and returns of borrowed values. +//! Emits diagnostics into the shared `DiagSink` — does not mutate TIR +//! and does not insert Free instructions (that lands in M8.1c). +//! +//! ## State lattice +//! +//! Per-TirRef state, not per-binding. A binding name resolves through +//! a shadow `current_owner: HashMap` to whichever +//! SSA value currently owns the underlying allocation. Anonymous owned +//! temporaries (concat results, formatter outputs) live in the same +//! `states` map with no shadow entry. +//! +//! See `docs/superpowers/specs/2026-05-20-milestone-8.1-heap-str-and-move-semantics-design.md` +//! sub-milestone 8.1b for the full algorithm. +//! +//! ## Mojo reference +//! +//! See `docs/dev/mojo_reference.md`. + +use crate::diag::DiagSink; +use crate::tir::Tir; +use crate::types::InternPool; + +/// Validate move safety for every function body. Emits diagnostics +/// into `sink`. Returns nothing — codegen continues to consume the +/// unchanged `&[Tir]`. +pub fn check(tirs: &[Tir], pool: &InternPool, sink: &mut DiagSink) { + for tir in tirs { + analyze_function(tir, pool, sink); + } +} + +fn analyze_function(_tir: &Tir, _pool: &InternPool, _sink: &mut DiagSink) { + // No-op for now; lattice + walk land in subsequent tasks. +} diff --git a/src/pipeline.rs b/src/pipeline.rs index ad14413..25ec9d2 100644 --- a/src/pipeline.rs +++ b/src/pipeline.rs @@ -294,6 +294,7 @@ pub(crate) fn ir_command(file: &Path, emit: &[EmitKind]) -> Result<(), CompilerE // slots), and `--emit=tir` deliberately prints that partial // TIR — the whole point of the flag is debugging sema. let tirs = sema::analyze(&uir, &mut pool, &mut sink, &input, file); + crate::ownership::check(&tirs, &pool, &mut sink); if want.tir { display_tir(&tirs, &pool); @@ -390,6 +391,7 @@ fn lower_and_analyze( // keeps cascades in check, and surfacing every problem in one // run is the whole point of the structured-diagnostics phase. let tirs = sema::analyze(&uir, pool, &mut sink, input, file_path); + crate::ownership::check(&tirs, pool, &mut sink); if sink.has_errors() { let diags = sink.into_diags(); render_diags(&diags, input, source_name); From 8a1bab653bce081c966610ac498cf77cf8cf3be3 Mon Sep 17 00:00:00 2001 From: Pepe Navarro Date: Wed, 3 Jun 2026 23:06:41 +0200 Subject: [PATCH 09/27] feat(ownership): Copy/Move classification + state lattice types MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds is_copy_type/is_move_type/needs_tracking, the OwnerState enum (NotTracked/Valid/Borrowed/Moved), MoveKind, and the per-function Ownership state container. No walk yet — lattice plumbing only. --- src/ownership.rs | 107 ++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 105 insertions(+), 2 deletions(-) diff --git a/src/ownership.rs b/src/ownership.rs index 740eb84..0c7465d 100644 --- a/src/ownership.rs +++ b/src/ownership.rs @@ -22,8 +22,82 @@ //! See `docs/dev/mojo_reference.md`. use crate::diag::DiagSink; -use crate::tir::Tir; -use crate::types::InternPool; +use crate::tir::{Span, Tir, TirRef}; +use crate::types::{InternPool, StringId, TypeId, TypeKind}; +use std::collections::HashMap; + +// ---------- Classification ---------- + +/// True for types whose values can be freely duplicated by `=` +/// without invalidating the source. Mirrors Mojo's `Copyable` trait +/// for the scalar primitives Ryo currently has. The ownership walk +/// short-circuits on these — they never enter the lattice. +#[allow(dead_code)] // wired into the walk in Task 10 +pub(crate) fn is_copy_type(ty: TypeId, pool: &InternPool) -> bool { + matches!( + pool.kind(ty), + TypeKind::Int | TypeKind::Float | TypeKind::Bool + ) +} + +/// True for types whose values transfer ownership on `=` and must be +/// tracked through the function body. Today: `str` only. Future heap +/// types (`List[T]`, `Dict[K, V]`) will join this set. +#[allow(dead_code)] // wired into the walk in Task 10 +pub(crate) fn is_move_type(ty: TypeId, pool: &InternPool) -> bool { + matches!(pool.kind(ty), TypeKind::Str) +} + +/// Predicate the ownership walk uses to decide whether a `TirRef` +/// needs a lattice slot. Currently identical to `is_move_type`, but +/// kept as its own name so the walk reads correctly when borrows +/// land and the answer becomes "move OR borrowed-of-move". +#[allow(dead_code)] // wired into the walk in Task 10 +pub(crate) fn needs_tracking(ty: TypeId, pool: &InternPool) -> bool { + is_move_type(ty, pool) +} + +// ---------- Lattice ---------- + +/// Per-`TirRef` ownership state. Anything Copy-typed lives in +/// `NotTracked` for its whole lifetime (the walk skips it). Move- +/// typed values start at `Valid` on definition, transition to +/// `Borrowed` while a borrow is live, and to `Moved` once consumed. +#[derive(Clone, Debug, PartialEq, Eq)] +#[allow(dead_code)] +pub(crate) enum OwnerState { + NotTracked, + Valid, + Borrowed, + Moved { moved_at: Span, kind: MoveKind }, +} + +/// Why a value transitioned to `Moved`. Drives diagnostic phrasing +/// at the use-after-move site ("moved into return", "moved into +/// `foo`", etc.). +#[derive(Clone, Debug, PartialEq, Eq)] +#[allow(dead_code)] +pub(crate) enum MoveKind { + Assignment, + Return, + MoveParam { callee: StringId }, +} + +/// Per-function ownership state. `states` is the lattice itself, +/// keyed by the `TirRef` that produced the value. `current_owner` +/// is a shadow map from binding name to whichever SSA value +/// currently owns the underlying allocation (so reassignment +/// reseats ownership without disturbing the producing SSA value). +/// `origin` records, for each tracked `TirRef`, the upstream value +/// it derives from (or `None` for fresh allocations) — used to walk +/// back to the root owner when diagnosing a use-after-move. +#[derive(Default)] +#[allow(dead_code)] +pub(crate) struct Ownership { + pub states: HashMap, + pub current_owner: HashMap, + pub origin: HashMap>, +} /// Validate move safety for every function body. Emits diagnostics /// into `sink`. Returns nothing — codegen continues to consume the @@ -37,3 +111,32 @@ pub fn check(tirs: &[Tir], pool: &InternPool, sink: &mut DiagSink) { fn analyze_function(_tir: &Tir, _pool: &InternPool, _sink: &mut DiagSink) { // No-op for now; lattice + walk land in subsequent tasks. } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn copy_types_classified() { + let pool = InternPool::new(); + assert!(is_copy_type(pool.int(), &pool)); + assert!(is_copy_type(pool.float(), &pool)); + assert!(is_copy_type(pool.bool_(), &pool)); + assert!(!is_copy_type(pool.str_(), &pool)); + } + + #[test] + fn move_types_classified() { + let pool = InternPool::new(); + assert!(is_move_type(pool.str_(), &pool)); + assert!(!is_move_type(pool.int(), &pool)); + assert!(!is_move_type(pool.bool_(), &pool)); + } + + #[test] + fn needs_tracking_matches_move() { + let pool = InternPool::new(); + assert!(needs_tracking(pool.str_(), &pool)); + assert!(!needs_tracking(pool.int(), &pool)); + } +} From 18d0d03ae51cdb5a66c8b8044583b707cc057582 Mon Sep 17 00:00:00 2001 From: Pepe Navarro Date: Wed, 3 Jun 2026 23:23:29 +0200 Subject: [PATCH 10/27] =?UTF-8?q?feat(ownership):=20forward=20walk=20?= =?UTF-8?q?=E2=80=94=20producers=20+=20aliasing=20reads?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Walks each TIR statement-by-statement. Allocating instructions (StrConst, StrConcat, str-returning Call) populate `states` with Valid; Var reads populate `origin` with the current owner. Parameter refs use a synthetic high-end TirRef encoding so parameters live in the same lattice as instructions. Consumption logic (VarDecl, Assign, Return, move-Call) lands in subsequent tasks. --- src/ownership.rs | 153 +++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 147 insertions(+), 6 deletions(-) diff --git a/src/ownership.rs b/src/ownership.rs index 0c7465d..925ec9c 100644 --- a/src/ownership.rs +++ b/src/ownership.rs @@ -22,7 +22,7 @@ //! See `docs/dev/mojo_reference.md`. use crate::diag::DiagSink; -use crate::tir::{Span, Tir, TirRef}; +use crate::tir::{Span, Tir, TirData, TirRef, TirTag}; use crate::types::{InternPool, StringId, TypeId, TypeKind}; use std::collections::HashMap; @@ -32,7 +32,7 @@ use std::collections::HashMap; /// without invalidating the source. Mirrors Mojo's `Copyable` trait /// for the scalar primitives Ryo currently has. The ownership walk /// short-circuits on these — they never enter the lattice. -#[allow(dead_code)] // wired into the walk in Task 10 +#[allow(dead_code)] // wired into consumption logic in later tasks pub(crate) fn is_copy_type(ty: TypeId, pool: &InternPool) -> bool { matches!( pool.kind(ty), @@ -43,7 +43,6 @@ pub(crate) fn is_copy_type(ty: TypeId, pool: &InternPool) -> bool { /// True for types whose values transfer ownership on `=` and must be /// tracked through the function body. Today: `str` only. Future heap /// types (`List[T]`, `Dict[K, V]`) will join this set. -#[allow(dead_code)] // wired into the walk in Task 10 pub(crate) fn is_move_type(ty: TypeId, pool: &InternPool) -> bool { matches!(pool.kind(ty), TypeKind::Str) } @@ -52,7 +51,6 @@ pub(crate) fn is_move_type(ty: TypeId, pool: &InternPool) -> bool { /// needs a lattice slot. Currently identical to `is_move_type`, but /// kept as its own name so the walk reads correctly when borrows /// land and the answer becomes "move OR borrowed-of-move". -#[allow(dead_code)] // wired into the walk in Task 10 pub(crate) fn needs_tracking(ty: TypeId, pool: &InternPool) -> bool { is_move_type(ty, pool) } @@ -108,8 +106,128 @@ pub fn check(tirs: &[Tir], pool: &InternPool, sink: &mut DiagSink) { } } -fn analyze_function(_tir: &Tir, _pool: &InternPool, _sink: &mut DiagSink) { - // No-op for now; lattice + walk land in subsequent tasks. +fn analyze_function(tir: &Tir, pool: &InternPool, _sink: &mut DiagSink) { + let mut own = Ownership::default(); + + // Initialise per-parameter state. Move-typed params start at + // `Valid` (the callee owns them); borrowed params start at + // `Borrowed`. Copy-typed params skip the lattice entirely. + for param in &tir.params { + if !needs_tracking(param.ty, pool) { + continue; + } + let synthetic = synthetic_param_ref(param.name); + let state = if param.is_move { + OwnerState::Valid + } else { + OwnerState::Borrowed + }; + own.states.insert(synthetic, state); + own.current_owner.insert(param.name, synthetic); + } + + for stmt in tir.body_stmts() { + analyze_stmt(tir, pool, &mut own, stmt); + } +} + +/// Build a stable synthetic [`TirRef`] for a parameter so it can +/// share the `states` map with real instruction refs. Encoded near +/// `u32::MAX` to keep it well clear of real per-function indices +/// (which start at 1 and grow with body size). `u32::MAX - name.raw()` +/// stays non-zero for any plausible interned-string id. +fn synthetic_param_ref(name: StringId) -> TirRef { + let raw = u32::MAX - name.raw(); + TirRef::from_raw(raw) +} + +fn analyze_stmt(tir: &Tir, pool: &InternPool, own: &mut Ownership, stmt: TirRef) { + visit_expr(tir, pool, own, stmt); +} + +fn visit_expr(tir: &Tir, pool: &InternPool, own: &mut Ownership, r: TirRef) { + let inst = *tir.inst(r); + match inst.tag { + // ---- Allocating instructions ---- + // `StrConst` materializes a fresh heap string at runtime; + // `StrConcat` produces a brand-new allocation from its two + // operands. Both enter the lattice as `Valid` with no + // upstream origin. + TirTag::StrConst => { + if needs_tracking(inst.ty, pool) { + own.states.insert(r, OwnerState::Valid); + own.origin.insert(r, None); + } + } + TirTag::StrConcat => { + if needs_tracking(inst.ty, pool) { + own.states.insert(r, OwnerState::Valid); + own.origin.insert(r, None); + } + if let TirData::BinOp { lhs, rhs } = inst.data { + visit_expr(tir, pool, own, lhs); + visit_expr(tir, pool, own, rhs); + } + } + TirTag::Call => { + // A str-returning call (e.g. `int_to_str`) is a producer. + // Argument-side consumption lands in a later task; for + // now just recurse so any nested producers/aliases are + // observed. + if needs_tracking(inst.ty, pool) { + own.states.insert(r, OwnerState::Valid); + own.origin.insert(r, None); + } + let view = tir.call_view(r); + for arg in view.args { + visit_expr(tir, pool, own, arg); + } + } + // ---- Aliasing read ---- + // `Var` is a non-consuming read. Record which SSA value it + // currently aliases so a later use-after-move diagnostic can + // walk back to the root owner. + TirTag::Var => { + let name = match inst.data { + TirData::Var(n) => n, + _ => unreachable!("Var must carry TirData::Var"), + }; + if let Some(&owner) = own.current_owner.get(&name) + && needs_tracking(inst.ty, pool) + { + own.origin.insert(r, Some(owner)); + } + } + // ---- Everything else: recurse on operands so nested + // ---- producers/aliases are still observed. + _ => { + recurse_operands(tir, pool, own, r); + } + } +} + +fn recurse_operands(tir: &Tir, pool: &InternPool, own: &mut Ownership, r: TirRef) { + let inst = *tir.inst(r); + match inst.data { + TirData::UnOp(o) => visit_expr(tir, pool, own, o), + TirData::BinOp { lhs, rhs } => { + visit_expr(tir, pool, own, lhs); + visit_expr(tir, pool, own, rhs); + } + // `Extra`-shaped instructions (VarDecl, Assign, Call, + // IfStmt, WhileLoop, ForRange, CompoundAssign) have + // bespoke decoders. Consumption logic lands in subsequent + // tasks; until then their operands are deliberately not + // descended into here so we avoid double-visits when those + // tasks introduce per-tag handling. + TirData::Extra(_) => {} + TirData::None + | TirData::Int(_) + | TirData::Float(_) + | TirData::Str(_) + | TirData::Bool(_) + | TirData::Var(_) => {} + } } #[cfg(test)] @@ -139,4 +257,27 @@ mod tests { assert!(needs_tracking(pool.str_(), &pool)); assert!(!needs_tracking(pool.int(), &pool)); } + + #[test] + fn str_const_walk_no_panic() { + use crate::tir::TirBuilder; + use chumsky::span::{SimpleSpan, Span as _}; + + let mut pool = InternPool::new(); + let str_ty = pool.str_(); + let void = pool.void(); + let main = pool.intern_str("main"); + let hello = pool.intern_str("hello"); + let span = SimpleSpan::new((), 0..0); + + // fn main() -> void: as expr_stmt + let mut b = TirBuilder::new(main, vec![], void, span); + let s = b.str_const(hello, str_ty, span); + let stmt = b.unary(TirTag::ExprStmt, void, s, span); + let tir = b.finish(&[stmt]); + + let mut sink = DiagSink::new(); + check(std::slice::from_ref(&tir), &pool, &mut sink); + assert!(sink.is_empty()); + } } From 52ede54ba42934e704ac89f1dd70df2298563697 Mon Sep 17 00:00:00 2001 From: Pepe Navarro Date: Wed, 3 Jun 2026 23:30:06 +0200 Subject: [PATCH 11/27] feat(diag): add ownership diagnostic codes E0020 UseAfterMove, E0021 MoveOutOfBorrowedParam, E0022 ReturnBorrowedValue, W0001 DeadStore. Codes are wired through diag_code_str but the ownership pass does not emit them yet. --- src/diag.rs | 10 ++++++++++ src/pipeline.rs | 4 ++++ 2 files changed, 14 insertions(+) diff --git a/src/diag.rs b/src/diag.rs index 69e3ec1..6fbb903 100644 --- a/src/diag.rs +++ b/src/diag.rs @@ -110,6 +110,16 @@ pub enum DiagCode { /// comptime / inferred-return-type work doesn't stack-overflow. CycleInResolution, + // --- ownership (M8.1b) --- + /// Use of a value after it has been moved. + UseAfterMove, + /// Attempted to move out of a borrowed parameter. + MoveOutOfBorrowedParam, + /// Attempted to return a borrowed value (Rule 5). + ReturnBorrowedValue, + /// A Move-typed value is declared (or assigned) but never used. + DeadStore, + // --- parser --- ParseError, diff --git a/src/pipeline.rs b/src/pipeline.rs index 25ec9d2..cb3d962 100644 --- a/src/pipeline.rs +++ b/src/pipeline.rs @@ -229,6 +229,10 @@ fn diag_code_str(code: DiagCode) -> &'static str { DiagCode::RangeArgType => "E0026", DiagCode::ReservedBuiltinName => "E0027", DiagCode::RedundantMove => "W0002", + DiagCode::UseAfterMove => "E0020", + DiagCode::MoveOutOfBorrowedParam => "E0021", + DiagCode::ReturnBorrowedValue => "E0022", + DiagCode::DeadStore => "W0001", DiagCode::CycleInResolution => "E0016", DiagCode::ParseError => "E0100", DiagCode::TooManyDiagnostics => "E0101", From 96791f3abac4d894cbd89813d70ee93ec8481010 Mon Sep 17 00:00:00 2001 From: Pepe Navarro Date: Thu, 4 Jun 2026 00:14:20 +0200 Subject: [PATCH 12/27] feat(ownership): VarDecl/Assign consumption + first diagnostics VarDecl and Assign of Move-typed values consume their initializer. Emits E0020 UseAfterMove on consumption of an already-Moved underlying owner, and E0021 MoveOutOfBorrowedParam on consumption of a Borrowed parameter. Concat reads operands as borrows (no state change), per Rule 2. Var reads now also surface E0020 when the aliased owner is already Moved, matching the spec algorithm at design.md:446. After a consume, the destination binding is reseated to a fresh lattice slot (origin severed, state Valid) so subsequent reads of the new binding are valid while reads of the source binding trip the use-after-move check. --- src/ownership.rs | 174 +++++++++++++++++++++++++++++++++---- tests/integration_tests.rs | 62 +++++++++++++ 2 files changed, 221 insertions(+), 15 deletions(-) diff --git a/src/ownership.rs b/src/ownership.rs index 925ec9c..050a639 100644 --- a/src/ownership.rs +++ b/src/ownership.rs @@ -21,7 +21,7 @@ //! //! See `docs/dev/mojo_reference.md`. -use crate::diag::DiagSink; +use crate::diag::{Diag, DiagCode, DiagSink}; use crate::tir::{Span, Tir, TirData, TirRef, TirTag}; use crate::types::{InternPool, StringId, TypeId, TypeKind}; use std::collections::HashMap; @@ -106,7 +106,7 @@ pub fn check(tirs: &[Tir], pool: &InternPool, sink: &mut DiagSink) { } } -fn analyze_function(tir: &Tir, pool: &InternPool, _sink: &mut DiagSink) { +fn analyze_function(tir: &Tir, pool: &InternPool, sink: &mut DiagSink) { let mut own = Ownership::default(); // Initialise per-parameter state. Move-typed params start at @@ -127,7 +127,7 @@ fn analyze_function(tir: &Tir, pool: &InternPool, _sink: &mut DiagSink) { } for stmt in tir.body_stmts() { - analyze_stmt(tir, pool, &mut own, stmt); + analyze_stmt(tir, pool, &mut own, sink, stmt); } } @@ -141,11 +141,140 @@ fn synthetic_param_ref(name: StringId) -> TirRef { TirRef::from_raw(raw) } -fn analyze_stmt(tir: &Tir, pool: &InternPool, own: &mut Ownership, stmt: TirRef) { - visit_expr(tir, pool, own, stmt); +fn analyze_stmt( + tir: &Tir, + pool: &InternPool, + own: &mut Ownership, + sink: &mut DiagSink, + stmt: TirRef, +) { + let inst = *tir.inst(stmt); + match inst.tag { + TirTag::VarDecl => analyze_var_decl(tir, pool, own, sink, stmt), + TirTag::Assign => analyze_assign(tir, pool, own, sink, stmt), + TirTag::ExprStmt => { + if let TirData::UnOp(o) = inst.data { + visit_expr(tir, pool, own, sink, o); + } + } + _ => { + visit_expr(tir, pool, own, sink, stmt); + } + } +} + +/// Move-typed `VarDecl` is a consumer: the new binding takes +/// ownership of the initializer's underlying value. If the +/// initializer aliases a borrowed parameter, this is the E0021 +/// "move out of borrowed parameter" site; if the underlying owner is +/// already `Moved`, this is the E0020 "use after move" site. +fn analyze_var_decl( + tir: &Tir, + pool: &InternPool, + own: &mut Ownership, + sink: &mut DiagSink, + r: TirRef, +) { + let view = tir.var_decl_view(r); + let init = view.initializer; + let init_ty = tir.inst(init).ty; + visit_expr(tir, pool, own, sink, init); + if needs_tracking(init_ty, pool) { + let span = tir.span(r); + consume_for_assignment(own, sink, init, span, MoveKind::Assignment); + rebind_to_init(own, view.name, init); + } else { + own.current_owner.insert(view.name, init); + } +} + +/// Reassignment to a Move-typed binding. Same consumption rules as +/// `VarDecl`; the existing binding name is reseated to whichever +/// SSA value owns the new underlying allocation. +fn analyze_assign( + tir: &Tir, + pool: &InternPool, + own: &mut Ownership, + sink: &mut DiagSink, + r: TirRef, +) { + let view = tir.assign_view(r); + let value_ty = tir.inst(view.value).ty; + visit_expr(tir, pool, own, sink, view.value); + if needs_tracking(value_ty, pool) { + let span = tir.span(r); + consume_for_assignment(own, sink, view.value, span, MoveKind::Assignment); + rebind_to_init(own, view.name, view.value); + } +} + +/// Reseat `name` to point at `init` as its current owner. After a +/// consume, the source binding's underlying value is `Moved`; the +/// new binding takes a fresh, independent slot in the lattice so +/// subsequent reads of `name` resolve to a `Valid` owner instead of +/// tripping over the just-moved underlying. We do this by severing +/// `init`'s `origin` link (if any) and stamping it `Valid`. +fn rebind_to_init(own: &mut Ownership, name: StringId, init: TirRef) { + own.origin.insert(init, None); + own.states.insert(init, OwnerState::Valid); + own.current_owner.insert(name, init); +} + +/// Walk back from `init` to whichever SSA value currently owns the +/// underlying allocation. `visit_expr` is responsible for populating +/// `origin` for `Var` reads; for fresh producers (`StrConst`, +/// `StrConcat`, `Call`) `init` is itself the owner. +fn underlying_owner(own: &Ownership, init: TirRef) -> TirRef { + match own.origin.get(&init).copied() { + Some(Some(owner)) => owner, + _ => init, + } +} + +/// Apply the consumption transition for a Move-typed initializer. +/// Caller must have already populated origin/state for `init` via +/// `visit_expr`. +fn consume_for_assignment( + own: &mut Ownership, + sink: &mut DiagSink, + init: TirRef, + span: Span, + kind: MoveKind, +) { + let underlying = underlying_owner(own, init); + let state = own + .states + .get(&underlying) + .cloned() + .unwrap_or(OwnerState::NotTracked); + match state { + OwnerState::Valid => { + own.states.insert( + underlying, + OwnerState::Moved { + moved_at: span, + kind, + }, + ); + } + OwnerState::Borrowed => { + sink.emit(Diag::error( + span, + DiagCode::MoveOutOfBorrowedParam, + "cannot move out of borrowed parameter", + )); + } + OwnerState::Moved { moved_at, .. } => { + sink.emit( + Diag::error(span, DiagCode::UseAfterMove, "use of moved value") + .with_note(Some(moved_at), "value was moved here"), + ); + } + OwnerState::NotTracked => {} + } } -fn visit_expr(tir: &Tir, pool: &InternPool, own: &mut Ownership, r: TirRef) { +fn visit_expr(tir: &Tir, pool: &InternPool, own: &mut Ownership, sink: &mut DiagSink, r: TirRef) { let inst = *tir.inst(r); match inst.tag { // ---- Allocating instructions ---- @@ -165,8 +294,8 @@ fn visit_expr(tir: &Tir, pool: &InternPool, own: &mut Ownership, r: TirRef) { own.origin.insert(r, None); } if let TirData::BinOp { lhs, rhs } = inst.data { - visit_expr(tir, pool, own, lhs); - visit_expr(tir, pool, own, rhs); + visit_expr(tir, pool, own, sink, lhs); + visit_expr(tir, pool, own, sink, rhs); } } TirTag::Call => { @@ -180,13 +309,16 @@ fn visit_expr(tir: &Tir, pool: &InternPool, own: &mut Ownership, r: TirRef) { } let view = tir.call_view(r); for arg in view.args { - visit_expr(tir, pool, own, arg); + visit_expr(tir, pool, own, sink, arg); } } // ---- Aliasing read ---- // `Var` is a non-consuming read. Record which SSA value it // currently aliases so a later use-after-move diagnostic can - // walk back to the root owner. + // walk back to the root owner. Per the design spec, an + // aliasing read of an already-`Moved` owner is itself an + // E0020 use-after-move. Reads of `Borrowed` owners are fine + // (Rule 2 — borrowed parameters can be freely read). TirTag::Var => { let name = match inst.data { TirData::Var(n) => n, @@ -196,23 +328,35 @@ fn visit_expr(tir: &Tir, pool: &InternPool, own: &mut Ownership, r: TirRef) { && needs_tracking(inst.ty, pool) { own.origin.insert(r, Some(owner)); + if let Some(OwnerState::Moved { moved_at, .. }) = own.states.get(&owner).cloned() { + sink.emit( + Diag::error(tir.span(r), DiagCode::UseAfterMove, "use of moved value") + .with_note(Some(moved_at), "value was moved here"), + ); + } } } // ---- Everything else: recurse on operands so nested // ---- producers/aliases are still observed. _ => { - recurse_operands(tir, pool, own, r); + recurse_operands(tir, pool, own, sink, r); } } } -fn recurse_operands(tir: &Tir, pool: &InternPool, own: &mut Ownership, r: TirRef) { +fn recurse_operands( + tir: &Tir, + pool: &InternPool, + own: &mut Ownership, + sink: &mut DiagSink, + r: TirRef, +) { let inst = *tir.inst(r); match inst.data { - TirData::UnOp(o) => visit_expr(tir, pool, own, o), + TirData::UnOp(o) => visit_expr(tir, pool, own, sink, o), TirData::BinOp { lhs, rhs } => { - visit_expr(tir, pool, own, lhs); - visit_expr(tir, pool, own, rhs); + visit_expr(tir, pool, own, sink, lhs); + visit_expr(tir, pool, own, sink, rhs); } // `Extra`-shaped instructions (VarDecl, Assign, Call, // IfStmt, WhileLoop, ForRange, CompoundAssign) have diff --git a/tests/integration_tests.rs b/tests/integration_tests.rs index f088eee..faa5da0 100644 --- a/tests/integration_tests.rs +++ b/tests/integration_tests.rs @@ -2313,3 +2313,65 @@ fn test_move_on_str_no_warning() { let stderr = String::from_utf8_lossy(&output.stderr); assert!(!stderr.contains("W0002"), "unexpected W0002: {}", stderr); } + +#[test] +fn test_use_after_move_assignment() { + let temp_dir = TempDir::new().expect("temp"); + let code = "name: str = \"Alice\"\nother: str = name\nprint(name)"; + let test_file = create_test_file(temp_dir.path(), "uam_assign.ryo", code); + let output = run_ryo_command(&["run", "uam_assign.ryo"], &test_file).expect("run"); + assert!(!output.status.success(), "expected compile error"); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + stderr.contains("E0020"), + "expected E0020 in stderr: {}", + stderr + ); +} + +#[test] +fn test_move_out_of_borrowed_parameter() { + let temp_dir = TempDir::new().expect("temp"); + let code = "fn process(data: str):\n\tother: str = data\n\nprocess(\"hi\")"; + let test_file = create_test_file(temp_dir.path(), "move_borrowed.ryo", code); + let output = run_ryo_command(&["run", "move_borrowed.ryo"], &test_file).expect("run"); + assert!(!output.status.success()); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!(stderr.contains("E0021"), "expected E0021: {}", stderr); +} + +#[test] +fn test_move_param_consumed_ok() { + let temp_dir = TempDir::new().expect("temp"); + let code = "fn consume(move s: str):\n\tother: str = s\n\nconsume(\"hi\")"; + let test_file = create_test_file(temp_dir.path(), "move_ok.ryo", code); + let output = run_ryo_command(&["run", "move_ok.ryo"], &test_file).expect("run"); + assert!( + output.status.success(), + "STDERR: {}", + String::from_utf8_lossy(&output.stderr) + ); +} + +#[test] +fn test_str_concat_then_use_ok() { + let temp_dir = TempDir::new().expect("temp"); + let code = "a: str = \"hi\"\nb: str = \"there\"\nc: str = a + b\nprint(c)"; + let test_file = create_test_file(temp_dir.path(), "concat_ok.ryo", code); + let output = run_ryo_command(&["run", "concat_ok.ryo"], &test_file).expect("run"); + assert!(output.status.success()); +} + +#[test] +fn test_str_concat_then_use_after_move_in_concat() { + let temp_dir = TempDir::new().expect("temp"); + // a + b reads a and b as borrows (not moves); a is still valid. + let code = "a: str = \"hi\"\nb: str = \"!\"\nc: str = a + b\nprint(a)"; + let test_file = create_test_file(temp_dir.path(), "concat_borrows.ryo", code); + let output = run_ryo_command(&["run", "concat_borrows.ryo"], &test_file).expect("run"); + assert!( + output.status.success(), + "STDERR: {}", + String::from_utf8_lossy(&output.stderr) + ); +} From 94c34d2f108faf962e980f8d79b27e30687f377a Mon Sep 17 00:00:00 2001 From: Pepe Navarro Date: Thu, 4 Jun 2026 00:32:21 +0200 Subject: [PATCH 13/27] feat(ownership): Return + Call argument consumption Return of Move-typed expr consumes its operand. Borrowed parameters cannot be returned (E0022). Call arguments check the callee's per-parameter is_move flag: move params consume, default params borrow (and verify the underlying owner is not already Moved). Builtins (print, assert, int_to_str) borrow all str args by convention since they have no entry in by_name. --- src/ownership.rs | 162 ++++++++++++++++++++++++++++++++----- tests/integration_tests.rs | 50 ++++++++++++ 2 files changed, 191 insertions(+), 21 deletions(-) diff --git a/src/ownership.rs b/src/ownership.rs index 050a639..d3d58ef 100644 --- a/src/ownership.rs +++ b/src/ownership.rs @@ -101,12 +101,21 @@ pub(crate) struct Ownership { /// into `sink`. Returns nothing — codegen continues to consume the /// unchanged `&[Tir]`. pub fn check(tirs: &[Tir], pool: &InternPool, sink: &mut DiagSink) { + // Name-keyed lookup so call sites can read the callee's per- + // parameter `is_move` flags. Builtins (`print`, `assert`, + // `int_to_str`) are not in this map and default to borrowing. + let by_name: HashMap = tirs.iter().map(|t| (t.name, t)).collect(); for tir in tirs { - analyze_function(tir, pool, sink); + analyze_function(tir, pool, sink, &by_name); } } -fn analyze_function(tir: &Tir, pool: &InternPool, sink: &mut DiagSink) { +fn analyze_function( + tir: &Tir, + pool: &InternPool, + sink: &mut DiagSink, + by_name: &HashMap, +) { let mut own = Ownership::default(); // Initialise per-parameter state. Move-typed params start at @@ -127,7 +136,7 @@ fn analyze_function(tir: &Tir, pool: &InternPool, sink: &mut DiagSink) { } for stmt in tir.body_stmts() { - analyze_stmt(tir, pool, &mut own, sink, stmt); + analyze_stmt(tir, pool, &mut own, sink, by_name, stmt); } } @@ -146,19 +155,22 @@ fn analyze_stmt( pool: &InternPool, own: &mut Ownership, sink: &mut DiagSink, + by_name: &HashMap, stmt: TirRef, ) { let inst = *tir.inst(stmt); match inst.tag { - TirTag::VarDecl => analyze_var_decl(tir, pool, own, sink, stmt), - TirTag::Assign => analyze_assign(tir, pool, own, sink, stmt), + TirTag::VarDecl => analyze_var_decl(tir, pool, own, sink, by_name, stmt), + TirTag::Assign => analyze_assign(tir, pool, own, sink, by_name, stmt), + TirTag::Return => analyze_return(tir, pool, own, sink, by_name, stmt), + TirTag::ReturnVoid => {} TirTag::ExprStmt => { if let TirData::UnOp(o) = inst.data { - visit_expr(tir, pool, own, sink, o); + visit_expr(tir, pool, own, sink, by_name, o); } } _ => { - visit_expr(tir, pool, own, sink, stmt); + visit_expr(tir, pool, own, sink, by_name, stmt); } } } @@ -173,12 +185,13 @@ fn analyze_var_decl( pool: &InternPool, own: &mut Ownership, sink: &mut DiagSink, + by_name: &HashMap, r: TirRef, ) { let view = tir.var_decl_view(r); let init = view.initializer; let init_ty = tir.inst(init).ty; - visit_expr(tir, pool, own, sink, init); + visit_expr(tir, pool, own, sink, by_name, init); if needs_tracking(init_ty, pool) { let span = tir.span(r); consume_for_assignment(own, sink, init, span, MoveKind::Assignment); @@ -196,11 +209,12 @@ fn analyze_assign( pool: &InternPool, own: &mut Ownership, sink: &mut DiagSink, + by_name: &HashMap, r: TirRef, ) { let view = tir.assign_view(r); let value_ty = tir.inst(view.value).ty; - visit_expr(tir, pool, own, sink, view.value); + visit_expr(tir, pool, own, sink, by_name, view.value); if needs_tracking(value_ty, pool) { let span = tir.span(r); consume_for_assignment(own, sink, view.value, span, MoveKind::Assignment); @@ -208,6 +222,62 @@ fn analyze_assign( } } +/// Move-typed `Return` is a consumer: the returned value flows out of +/// the function and the caller takes ownership. Borrowed parameters +/// cannot be returned (E0022). If the underlying owner is already +/// `Moved`, this is the E0020 "use after move" site. +fn analyze_return( + tir: &Tir, + pool: &InternPool, + own: &mut Ownership, + sink: &mut DiagSink, + by_name: &HashMap, + r: TirRef, +) { + let inst = *tir.inst(r); + let operand = match inst.data { + TirData::UnOp(o) => o, + _ => unreachable!("Return must carry TirData::UnOp"), + }; + let ty = tir.inst(operand).ty; + visit_expr(tir, pool, own, sink, by_name, operand); + if !needs_tracking(ty, pool) { + return; + } + let span = tir.span(r); + let underlying = underlying_owner(own, operand); + let state = own + .states + .get(&underlying) + .cloned() + .unwrap_or(OwnerState::NotTracked); + match state { + OwnerState::Valid => { + own.states.insert( + underlying, + OwnerState::Moved { + moved_at: span, + kind: MoveKind::Return, + }, + ); + } + OwnerState::Borrowed => { + sink.emit(Diag::error( + span, + DiagCode::ReturnBorrowedValue, + "cannot return borrowed value", + )); + } + OwnerState::Moved { moved_at, .. } => { + sink.emit( + Diag::error(span, DiagCode::UseAfterMove, "use of moved value in return") + .with_note(Some(moved_at), "value was moved here"), + ); + } + OwnerState::NotTracked => {} + } +} + /// Reseat `name` to point at `init` as its current owner. After a /// consume, the source binding's underlying value is `Moved`; the /// new binding takes a fresh, independent slot in the lattice so @@ -274,7 +344,14 @@ fn consume_for_assignment( } } -fn visit_expr(tir: &Tir, pool: &InternPool, own: &mut Ownership, sink: &mut DiagSink, r: TirRef) { +fn visit_expr( + tir: &Tir, + pool: &InternPool, + own: &mut Ownership, + sink: &mut DiagSink, + by_name: &HashMap, + r: TirRef, +) { let inst = *tir.inst(r); match inst.tag { // ---- Allocating instructions ---- @@ -294,22 +371,64 @@ fn visit_expr(tir: &Tir, pool: &InternPool, own: &mut Ownership, sink: &mut Diag own.origin.insert(r, None); } if let TirData::BinOp { lhs, rhs } = inst.data { - visit_expr(tir, pool, own, sink, lhs); - visit_expr(tir, pool, own, sink, rhs); + visit_expr(tir, pool, own, sink, by_name, lhs); + visit_expr(tir, pool, own, sink, by_name, rhs); } } TirTag::Call => { // A str-returning call (e.g. `int_to_str`) is a producer. - // Argument-side consumption lands in a later task; for - // now just recurse so any nested producers/aliases are - // observed. if needs_tracking(inst.ty, pool) { own.states.insert(r, OwnerState::Valid); own.origin.insert(r, None); } let view = tir.call_view(r); - for arg in view.args { - visit_expr(tir, pool, own, sink, arg); + // Look up callee parameter conventions. Builtins + // (`print`, `assert`, `int_to_str`) have no entry and + // default to borrowing all args. + let callee_params = by_name.get(&view.name).map(|t| t.params.as_slice()); + for (i, arg) in view.args.iter().enumerate() { + visit_expr(tir, pool, own, sink, by_name, *arg); + let arg_ty = tir.inst(*arg).ty; + if !needs_tracking(arg_ty, pool) { + continue; + } + let is_move_param = callee_params + .and_then(|ps| ps.get(i)) + .map(|p| p.is_move) + .unwrap_or(false); + if is_move_param { + consume_for_assignment( + own, + sink, + *arg, + tir.span(r), + MoveKind::MoveParam { callee: view.name }, + ); + } else { + // Borrow: just verify the underlying owner is not + // already `Moved`. Var-reads inside `visit_expr` + // already fire E0020 for that case, but we keep a + // call-site check so the diagnostic message can + // be specific to "borrowed argument". + let underlying = underlying_owner(own, *arg); + if let Some(OwnerState::Moved { moved_at, .. }) = + own.states.get(&underlying).cloned() + && underlying != *arg + { + // The Var-read path fires its own E0020 when + // the Var instruction's owner is Moved; only + // emit here when the moved owner is reached + // via aliasing (so we don't double-report). + sink.emit( + Diag::error( + tir.span(r), + DiagCode::UseAfterMove, + "use of moved value as borrowed argument", + ) + .with_note(Some(moved_at), "value was moved here"), + ); + } + } } } // ---- Aliasing read ---- @@ -339,7 +458,7 @@ fn visit_expr(tir: &Tir, pool: &InternPool, own: &mut Ownership, sink: &mut Diag // ---- Everything else: recurse on operands so nested // ---- producers/aliases are still observed. _ => { - recurse_operands(tir, pool, own, sink, r); + recurse_operands(tir, pool, own, sink, by_name, r); } } } @@ -349,14 +468,15 @@ fn recurse_operands( pool: &InternPool, own: &mut Ownership, sink: &mut DiagSink, + by_name: &HashMap, r: TirRef, ) { let inst = *tir.inst(r); match inst.data { - TirData::UnOp(o) => visit_expr(tir, pool, own, sink, o), + TirData::UnOp(o) => visit_expr(tir, pool, own, sink, by_name, o), TirData::BinOp { lhs, rhs } => { - visit_expr(tir, pool, own, sink, lhs); - visit_expr(tir, pool, own, sink, rhs); + visit_expr(tir, pool, own, sink, by_name, lhs); + visit_expr(tir, pool, own, sink, by_name, rhs); } // `Extra`-shaped instructions (VarDecl, Assign, Call, // IfStmt, WhileLoop, ForRange, CompoundAssign) have diff --git a/tests/integration_tests.rs b/tests/integration_tests.rs index faa5da0..ea8bf84 100644 --- a/tests/integration_tests.rs +++ b/tests/integration_tests.rs @@ -2375,3 +2375,53 @@ fn test_str_concat_then_use_after_move_in_concat() { String::from_utf8_lossy(&output.stderr) ); } + +#[test] +fn test_use_after_move_return() { + let temp_dir = TempDir::new().expect("temp"); + let code = "fn make() -> str:\n\ts: str = \"hi\"\n\treturn s\n\nx: str = make()\nprint(x)"; + let test_file = create_test_file(temp_dir.path(), "ret_ok.ryo", code); + let output = run_ryo_command(&["run", "ret_ok.ryo"], &test_file).expect("run"); + assert!( + output.status.success(), + "STDERR: {}", + String::from_utf8_lossy(&output.stderr) + ); +} + +#[test] +fn test_return_borrowed_param() { + let temp_dir = TempDir::new().expect("temp"); + let code = + "fn passthrough(s: str) -> str:\n\treturn s\n\nx: str = passthrough(\"hi\")\nprint(x)"; + let test_file = create_test_file(temp_dir.path(), "ret_borrowed.ryo", code); + let output = run_ryo_command(&["run", "ret_borrowed.ryo"], &test_file).expect("run"); + assert!(!output.status.success(), "expected E0022"); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!(stderr.contains("E0022"), "expected E0022: {}", stderr); +} + +#[test] +fn test_use_after_move_param() { + let temp_dir = TempDir::new().expect("temp"); + let code = + "fn consume(move s: str):\n\tprint(s)\n\nname: str = \"Alice\"\nconsume(name)\nprint(name)"; + let test_file = create_test_file(temp_dir.path(), "uam_param.ryo", code); + let output = run_ryo_command(&["run", "uam_param.ryo"], &test_file).expect("run"); + assert!(!output.status.success()); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!(stderr.contains("E0020"), "expected E0020: {}", stderr); +} + +#[test] +fn test_borrow_param_then_use_ok() { + let temp_dir = TempDir::new().expect("temp"); + let code = "fn print_twice(s: str):\n\tprint(s)\n\tprint(s)\n\nprint_twice(\"hi\")"; + let test_file = create_test_file(temp_dir.path(), "borrow_ok.ryo", code); + let output = run_ryo_command(&["run", "borrow_ok.ryo"], &test_file).expect("run"); + assert!( + output.status.success(), + "STDERR: {}", + String::from_utf8_lossy(&output.stderr) + ); +} From e3562e853cc433fb6441b42a4e8abb3828dd6145 Mon Sep 17 00:00:00 2001 From: Pepe Navarro Date: Thu, 4 Jun 2026 00:57:08 +0200 Subject: [PATCH 14/27] fix(ownership): remove redundant call-site UAM check The borrow-position UAM check in the Call arm fired alongside the Var arm's check, producing two E0020 diagnostics for one fault. The Var arm already covers all current cases (every str arg flows through Var), so drop the call-site duplicate. Tighten test_use_after_move_param to assert exactly one E0020 occurrence so this regression is caught if it returns. --- src/ownership.rs | 31 +++++++------------------------ tests/integration_tests.rs | 6 ++++++ 2 files changed, 13 insertions(+), 24 deletions(-) diff --git a/src/ownership.rs b/src/ownership.rs index d3d58ef..033046b 100644 --- a/src/ownership.rs +++ b/src/ownership.rs @@ -404,31 +404,14 @@ fn visit_expr( tir.span(r), MoveKind::MoveParam { callee: view.name }, ); - } else { - // Borrow: just verify the underlying owner is not - // already `Moved`. Var-reads inside `visit_expr` - // already fire E0020 for that case, but we keep a - // call-site check so the diagnostic message can - // be specific to "borrowed argument". - let underlying = underlying_owner(own, *arg); - if let Some(OwnerState::Moved { moved_at, .. }) = - own.states.get(&underlying).cloned() - && underlying != *arg - { - // The Var-read path fires its own E0020 when - // the Var instruction's owner is Moved; only - // emit here when the moved owner is reached - // via aliasing (so we don't double-report). - sink.emit( - Diag::error( - tir.span(r), - DiagCode::UseAfterMove, - "use of moved value as borrowed argument", - ) - .with_note(Some(moved_at), "value was moved here"), - ); - } } + // Borrow path: no extra check here. With the current + // set of producers (StrConst/StrConcat/Call/Var), every + // tracked str argument flows through `Var`, and the + // `Var` arm already fires E0020 for use-after-move. + // Adding a call-site check would double-report. If a + // future producer pattern bypasses `Var` (e.g. passing + // a moved Call result directly), revisit this. } } // ---- Aliasing read ---- diff --git a/tests/integration_tests.rs b/tests/integration_tests.rs index ea8bf84..0944821 100644 --- a/tests/integration_tests.rs +++ b/tests/integration_tests.rs @@ -2411,6 +2411,12 @@ fn test_use_after_move_param() { assert!(!output.status.success()); let stderr = String::from_utf8_lossy(&output.stderr); assert!(stderr.contains("E0020"), "expected E0020: {}", stderr); + let count = stderr.matches("E0020").count(); + assert_eq!( + count, 1, + "expected exactly one E0020, got {}: {}", + count, stderr + ); } #[test] From e751882b7e81a9eb6bd99928455c86ff7f67a4c7 Mon Sep 17 00:00:00 2001 From: Pepe Navarro Date: Thu, 4 Jun 2026 01:03:26 +0200 Subject: [PATCH 15/27] feat(ownership): if/else CFG joins via snapshot + merge Snapshots state before each branch, walks branches independently, merges Moved conservatively (any branch Moved => merged Moved). Catches conditional use-after-move where one branch consumed and control reconverged before a use. --- src/ownership.rs | 113 ++++++++++++++++++++++++++++++++++++- tests/integration_tests.rs | 66 ++++++++++++++++++++++ 2 files changed, 178 insertions(+), 1 deletion(-) diff --git a/src/ownership.rs b/src/ownership.rs index 033046b..9c1e293 100644 --- a/src/ownership.rs +++ b/src/ownership.rs @@ -89,7 +89,7 @@ pub(crate) enum MoveKind { /// `origin` records, for each tracked `TirRef`, the upstream value /// it derives from (or `None` for fresh allocations) — used to walk /// back to the root owner when diagnosing a use-after-move. -#[derive(Default)] +#[derive(Default, Clone)] #[allow(dead_code)] pub(crate) struct Ownership { pub states: HashMap, @@ -97,6 +97,64 @@ pub(crate) struct Ownership { pub origin: HashMap>, } +impl Ownership { + /// Capture the current lattice for branch analysis. The CFG-join + /// logic in `analyze_if_stmt` snapshots state before each branch, + /// walks branches independently from that snapshot, then merges. + fn clone_snapshot(&self) -> Ownership { + self.clone() + } + + /// Reset the lattice to a previous snapshot. Used to walk each + /// branch of an `if` from the same starting state. + fn restore_from(&mut self, snap: &Ownership) { + self.states = snap.states.clone(); + self.current_owner = snap.current_owner.clone(); + self.origin = snap.origin.clone(); + } + + /// Conservatively merge per-branch lattices into `self`. For each + /// tracked `TirRef`, if any branch left it `Moved` the merged + /// state is `Moved` — that way a value consumed on only one + /// branch is still treated as moved at the join point and a + /// post-`if` use trips E0020. Non-`Moved` states pick the first + /// observed branch state. `current_owner` entries from any branch + /// are preserved so reseats inside a branch survive the join. + fn merge_branches(&mut self, branches: &[Ownership]) { + let mut all_refs: std::collections::HashSet = std::collections::HashSet::new(); + for b in branches { + all_refs.extend(b.states.keys().copied()); + } + for r in all_refs { + let mut merged: Option = None; + for b in branches { + let s = b.states.get(&r).cloned().unwrap_or(OwnerState::NotTracked); + merged = Some(match (merged, s) { + (None, s) => s, + (Some(OwnerState::Moved { moved_at, kind }), _) => { + OwnerState::Moved { moved_at, kind } + } + (_, OwnerState::Moved { moved_at, kind }) => { + OwnerState::Moved { moved_at, kind } + } + (Some(a), _) => a, + }); + } + if let Some(state) = merged { + self.states.insert(r, state); + } + } + for b in branches { + for (name, owner) in &b.current_owner { + self.current_owner.entry(*name).or_insert(*owner); + } + for (k, v) in &b.origin { + self.origin.entry(*k).or_insert(*v); + } + } + } +} + /// Validate move safety for every function body. Emits diagnostics /// into `sink`. Returns nothing — codegen continues to consume the /// unchanged `&[Tir]`. @@ -164,6 +222,7 @@ fn analyze_stmt( TirTag::Assign => analyze_assign(tir, pool, own, sink, by_name, stmt), TirTag::Return => analyze_return(tir, pool, own, sink, by_name, stmt), TirTag::ReturnVoid => {} + TirTag::IfStmt => analyze_if_stmt(tir, pool, own, sink, by_name, stmt), TirTag::ExprStmt => { if let TirData::UnOp(o) = inst.data { visit_expr(tir, pool, own, sink, by_name, o); @@ -344,6 +403,58 @@ fn consume_for_assignment( } } +/// CFG join for `if` / `elif` / `else`. The naïve forward walk +/// would let a move inside a then-branch persist past the merge +/// regardless of whether else also moved — wrong for the spec's +/// guarantee that conditionally-moved values are not safe to use +/// after the join. Snapshot the lattice before each branch, walk +/// each branch from the snapshot independently, then merge. If any +/// branch left a value `Moved`, the post-`if` state is `Moved`; +/// when no `else` is present, the implicit fall-through branch is +/// the pre-`if` snapshot itself, so an unconsumed pre-`if` value +/// stays usable after the join. +fn analyze_if_stmt( + tir: &Tir, + pool: &InternPool, + own: &mut Ownership, + sink: &mut DiagSink, + by_name: &HashMap, + r: TirRef, +) { + let view = tir.if_stmt_view(r); + visit_expr(tir, pool, own, sink, by_name, view.cond); + + let snapshot = own.clone_snapshot(); + + for stmt in &view.then_stmts { + analyze_stmt(tir, pool, own, sink, by_name, *stmt); + } + let then_state = own.clone_snapshot(); + + let mut branch_results = vec![then_state]; + for elif in &view.elif_branches { + own.restore_from(&snapshot); + visit_expr(tir, pool, own, sink, by_name, elif.cond); + for stmt in &elif.body { + analyze_stmt(tir, pool, own, sink, by_name, *stmt); + } + branch_results.push(own.clone_snapshot()); + } + + if let Some(else_stmts) = &view.else_stmts { + own.restore_from(&snapshot); + for stmt in else_stmts { + analyze_stmt(tir, pool, own, sink, by_name, *stmt); + } + branch_results.push(own.clone_snapshot()); + } else { + branch_results.push(snapshot.clone()); + } + + own.restore_from(&snapshot); + own.merge_branches(&branch_results); +} + fn visit_expr( tir: &Tir, pool: &InternPool, diff --git a/tests/integration_tests.rs b/tests/integration_tests.rs index 0944821..e05a11f 100644 --- a/tests/integration_tests.rs +++ b/tests/integration_tests.rs @@ -2431,3 +2431,69 @@ fn test_borrow_param_then_use_ok() { String::from_utf8_lossy(&output.stderr) ); } + +#[test] +fn test_conditional_move_then_use_fails() { + let temp_dir = TempDir::new().expect("temp"); + let code = r#" +fn consume(move s: str): + print(s) + +fn main(): + flag: bool = true + name: str = "Alice" + if flag: + consume(name) + print(name) +"#; + let test_file = create_test_file(temp_dir.path(), "cond_move.ryo", code); + let output = run_ryo_command(&["run", "cond_move.ryo"], &test_file).expect("run"); + assert!( + !output.status.success(), + "expected E0020 — name moved on then-branch" + ); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!(stderr.contains("E0020"), "expected E0020: {}", stderr); +} + +#[test] +fn test_conditional_move_both_branches_fails() { + let temp_dir = TempDir::new().expect("temp"); + let code = r#" +fn consume(move s: str): + print(s) + +fn main(): + flag: bool = true + name: str = "Alice" + if flag: + consume(name) + else: + consume(name) + print(name) +"#; + let test_file = create_test_file(temp_dir.path(), "cond_both.ryo", code); + let output = run_ryo_command(&["run", "cond_both.ryo"], &test_file).expect("run"); + assert!(!output.status.success()); +} + +#[test] +fn test_conditional_use_inside_branch_ok() { + let temp_dir = TempDir::new().expect("temp"); + let code = r#" +fn main(): + flag: bool = true + name: str = "Alice" + if flag: + print(name) + else: + print(name) +"#; + let test_file = create_test_file(temp_dir.path(), "cond_ok.ryo", code); + let output = run_ryo_command(&["run", "cond_ok.ryo"], &test_file).expect("run"); + assert!( + output.status.success(), + "STDERR: {}", + String::from_utf8_lossy(&output.stderr) + ); +} From 0130bd813618438f3a0160a413a9e590d9244373 Mon Sep 17 00:00:00 2001 From: Pepe Navarro Date: Thu, 4 Jun 2026 08:29:54 +0200 Subject: [PATCH 16/27] feat(ownership): loop fixed-point analysis WhileLoop and ForRange run a 2-iteration fixed point. If any Move-tracked TirRef changed Moved-ness between the entry and post-body state, re-walk the body from the merged state and emit diagnostics on the second pass. Catches use-after-move across the back-edge. Break/continue are no-ops in 8.1b (8.1c attaches Free metadata). --- src/ownership.rs | 127 ++++++++++++++++++++++++++++++++++++- tests/integration_tests.rs | 67 +++++++++++++++++++ 2 files changed, 193 insertions(+), 1 deletion(-) diff --git a/src/ownership.rs b/src/ownership.rs index 9c1e293..f102faa 100644 --- a/src/ownership.rs +++ b/src/ownership.rs @@ -24,7 +24,7 @@ use crate::diag::{Diag, DiagCode, DiagSink}; use crate::tir::{Span, Tir, TirData, TirRef, TirTag}; use crate::types::{InternPool, StringId, TypeId, TypeKind}; -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; // ---------- Classification ---------- @@ -223,6 +223,11 @@ fn analyze_stmt( TirTag::Return => analyze_return(tir, pool, own, sink, by_name, stmt), TirTag::ReturnVoid => {} TirTag::IfStmt => analyze_if_stmt(tir, pool, own, sink, by_name, stmt), + TirTag::WhileLoop => analyze_while_loop(tir, pool, own, sink, by_name, stmt), + TirTag::ForRange => analyze_for_range(tir, pool, own, sink, by_name, stmt), + TirTag::Break | TirTag::Continue => { + // 8.1c attaches Free metadata here; 8.1b is a no-op. + } TirTag::ExprStmt => { if let TirData::UnOp(o) = inst.data { visit_expr(tir, pool, own, sink, by_name, o); @@ -455,6 +460,126 @@ fn analyze_if_stmt( own.merge_branches(&branch_results); } +/// Fixed-point ownership analysis for `while`. Walks the body once +/// from the entry state into a scratch sink; if no tracked TirRef +/// changed Moved-ness across the back-edge, the body is loop-invariant +/// for ownership purposes and the scratch diagnostics are flushed. +/// Otherwise we re-walk from the merged (entry ⊔ post-body) state and +/// emit diagnostics on the second pass — a binding moved inside the +/// body without rebinding before the back-edge surfaces as E0020 on +/// iteration two. Converges in at most 2 iterations for the M8.1 +/// pattern set (per the design doc). +fn analyze_while_loop( + tir: &Tir, + pool: &InternPool, + own: &mut Ownership, + sink: &mut DiagSink, + by_name: &HashMap, + r: TirRef, +) { + let view = tir.while_loop_view(r); + let entry = own.clone_snapshot(); + + // First pass into a scratch sink — diagnostics are kept only if + // a single iteration suffices. If the lattice changes we re-walk + // and the scratch diags are dropped (the second walk re-emits + // anything that's still wrong). + let mut scratch_sink = DiagSink::new(); + visit_expr(tir, pool, own, &mut scratch_sink, by_name, view.cond); + for stmt in &view.body { + analyze_stmt(tir, pool, own, &mut scratch_sink, by_name, *stmt); + } + let after_first = own.clone_snapshot(); + + if states_differ(&entry, &after_first) { + let merged = merge_two(&entry, &after_first); + own.restore_from(&merged); + visit_expr(tir, pool, own, sink, by_name, view.cond); + for stmt in &view.body { + analyze_stmt(tir, pool, own, sink, by_name, *stmt); + } + let after_second = own.clone_snapshot(); + let final_merged = merge_two(&entry, &after_second); + own.restore_from(&final_merged); + } else { + for d in scratch_sink.into_diags() { + sink.emit(d); + } + let final_merged = merge_two(&entry, &after_first); + own.restore_from(&final_merged); + } +} + +/// `for i in range(start, end)` loop var is `int` (Copy), so the +/// induction variable never enters the lattice. The body runs the +/// same fixed-point as `while`. +fn analyze_for_range( + tir: &Tir, + pool: &InternPool, + own: &mut Ownership, + sink: &mut DiagSink, + by_name: &HashMap, + r: TirRef, +) { + let view = tir.for_range_view(r); + // Start/end are visited unconditionally — they're plain `int` + // exprs, so they don't move anything, but they may contain nested + // reads we want to record. + visit_expr(tir, pool, own, sink, by_name, view.start); + visit_expr(tir, pool, own, sink, by_name, view.end); + + let entry = own.clone_snapshot(); + + let mut scratch_sink = DiagSink::new(); + for stmt in &view.body { + analyze_stmt(tir, pool, own, &mut scratch_sink, by_name, *stmt); + } + let after_first = own.clone_snapshot(); + + if states_differ(&entry, &after_first) { + let merged = merge_two(&entry, &after_first); + own.restore_from(&merged); + for stmt in &view.body { + analyze_stmt(tir, pool, own, sink, by_name, *stmt); + } + let after_second = own.clone_snapshot(); + let final_merged = merge_two(&entry, &after_second); + own.restore_from(&final_merged); + } else { + for d in scratch_sink.into_diags() { + sink.emit(d); + } + let final_merged = merge_two(&entry, &after_first); + own.restore_from(&final_merged); + } +} + +/// Compare two lattices on the Moved-ness of every tracked `TirRef`. +/// Per the M8.1b plan, the only transition that forces a re-walk is a +/// `Valid`/`Borrowed` → `Moved` flip across the back-edge — non-Moved +/// state changes (e.g. fresh definitions added inside the body) are +/// loop-invariant for the use-after-move check. +fn states_differ(a: &Ownership, b: &Ownership) -> bool { + let keys: HashSet = a.states.keys().chain(b.states.keys()).copied().collect(); + for k in keys { + let av = a.states.get(&k).cloned().unwrap_or(OwnerState::NotTracked); + let bv = b.states.get(&k).cloned().unwrap_or(OwnerState::NotTracked); + if matches!(av, OwnerState::Moved { .. }) != matches!(bv, OwnerState::Moved { .. }) { + return true; + } + } + false +} + +/// Merge two lattices by reusing the existing branch-merge join: if +/// either input has a TirRef `Moved`, the result is `Moved`. Used to +/// seed the second pass and to compute the post-loop state. +fn merge_two(a: &Ownership, b: &Ownership) -> Ownership { + let mut merged = a.clone_snapshot(); + merged.merge_branches(&[a.clone_snapshot(), b.clone_snapshot()]); + merged +} + fn visit_expr( tir: &Tir, pool: &InternPool, diff --git a/tests/integration_tests.rs b/tests/integration_tests.rs index e05a11f..8fd1250 100644 --- a/tests/integration_tests.rs +++ b/tests/integration_tests.rs @@ -2497,3 +2497,70 @@ fn main(): String::from_utf8_lossy(&output.stderr) ); } + +#[test] +fn test_move_in_while_without_rebind_fails() { + let temp_dir = TempDir::new().expect("temp"); + let code = r#" +fn consume(move s: str): + print(s) + +fn main(): + mut name: str = "Alice" + mut i: int = 0 + while i < 3: + consume(name) + i = i + 1 +"#; + let test_file = create_test_file(temp_dir.path(), "loop_move.ryo", code); + let output = run_ryo_command(&["run", "loop_move.ryo"], &test_file).expect("run"); + assert!( + !output.status.success(), + "expected E0020 — name moved without rebind" + ); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!(stderr.contains("E0020"), "expected E0020: {}", stderr); +} + +#[test] +fn test_borrow_in_while_ok() { + let temp_dir = TempDir::new().expect("temp"); + let code = r#" +fn main(): + name: str = "Alice" + mut i: int = 0 + while i < 3: + print(name) + i = i + 1 +"#; + let test_file = create_test_file(temp_dir.path(), "loop_borrow.ryo", code); + let output = run_ryo_command(&["run", "loop_borrow.ryo"], &test_file).expect("run"); + assert!( + output.status.success(), + "STDERR: {}", + String::from_utf8_lossy(&output.stderr) + ); +} + +#[test] +fn test_move_then_rebind_in_loop_ok() { + let temp_dir = TempDir::new().expect("temp"); + let code = r#" +fn consume(move s: str): + print(s) + +fn main(): + mut i: int = 0 + while i < 3: + mut name: str = "Alice" + consume(name) + i = i + 1 +"#; + let test_file = create_test_file(temp_dir.path(), "loop_rebind.ryo", code); + let output = run_ryo_command(&["run", "loop_rebind.ryo"], &test_file).expect("run"); + assert!( + output.status.success(), + "STDERR: {}", + String::from_utf8_lossy(&output.stderr) + ); +} From e0ca17dbf12a48c9bc934d8f17730763c01762aa Mon Sep 17 00:00:00 2001 From: Pepe Navarro Date: Thu, 4 Jun 2026 11:21:37 +0200 Subject: [PATCH 17/27] feat(ownership): W0001 dead-store warning for unused Move values Emits W0001 when a Move-typed binding is declared but never read or consumed. Falls out of the forward walk: pending declarations are cleared on Var reads and on consumption; whatever remains at function end is dead. --- src/ownership.rs | 72 ++++++++++++++++++++++++++++++++++++++ tests/integration_tests.rs | 31 ++++++++++++++++ 2 files changed, 103 insertions(+) diff --git a/src/ownership.rs b/src/ownership.rs index f102faa..107a50d 100644 --- a/src/ownership.rs +++ b/src/ownership.rs @@ -95,6 +95,11 @@ pub(crate) struct Ownership { pub states: HashMap, pub current_owner: HashMap, pub origin: HashMap>, + /// VarDecls of Move-typed values, keyed by the underlying owner + /// `TirRef`. Cleared when the binding is read (`Var`) or consumed + /// (move/return). Whatever remains at function end is a dead + /// store, surfaced as W0001. + pub pending_dead_store: HashMap, } impl Ownership { @@ -111,6 +116,7 @@ impl Ownership { self.states = snap.states.clone(); self.current_owner = snap.current_owner.clone(); self.origin = snap.origin.clone(); + self.pending_dead_store = snap.pending_dead_store.clone(); } /// Conservatively merge per-branch lattices into `self`. For each @@ -152,6 +158,50 @@ impl Ownership { self.origin.entry(*k).or_insert(*v); } } + + // Pending dead-store entries: a key falls into one of two + // buckets relative to the pre-branch snapshot in `self`. + // + // (1) Pre-branch entries (already in `self.pending_dead_store` + // before the branches walked): every branch started with + // the entry. If any branch cleared it, the value was + // used somewhere — drop it. So the rule is "intersect + // across branches". + // + // (2) Branch-local entries (introduced inside a branch by a + // VarDecl): only the introducing branch has the key. If + // that branch ended with the entry still pending, the + // binding was declared and never used inside the branch + // and W0001 should still fire after the join. So the + // rule is "union across branches". + // + // Distinguishing (1) vs (2) is just: was the key in + // `self.pending_dead_store` before the merge? Walk both + // groups separately so each gets its correct rule. + let mut merged_pending: HashMap = HashMap::new(); + + // (1) Pre-branch entries — keep iff still pending in *every* + // branch. + for (k, v) in &self.pending_dead_store { + if branches + .iter() + .all(|b| b.pending_dead_store.contains_key(k)) + { + merged_pending.insert(*k, *v); + } + } + + // (2) Branch-local entries — anything in a branch's pending + // that wasn't pre-existing. + for b in branches { + for (k, v) in &b.pending_dead_store { + if !self.pending_dead_store.contains_key(k) { + merged_pending.insert(*k, *v); + } + } + } + + self.pending_dead_store = merged_pending; } } @@ -196,6 +246,16 @@ fn analyze_function( for stmt in tir.body_stmts() { analyze_stmt(tir, pool, &mut own, sink, by_name, stmt); } + + // Anything left pending was declared but never read or + // consumed — emit W0001 once per surviving binding. + for (_owner, (name, span)) in own.pending_dead_store.drain() { + sink.emit(Diag::warning( + span, + DiagCode::DeadStore, + format!("value `{}` is declared but never used", pool.str(name)), + )); + } } /// Build a stable synthetic [`TirRef`] for a parameter so it can @@ -260,6 +320,11 @@ fn analyze_var_decl( let span = tir.span(r); consume_for_assignment(own, sink, init, span, MoveKind::Assignment); rebind_to_init(own, view.name, init); + // Register the new binding as pending dead-store. The walk + // clears this entry on any later read or consumption; a + // surviving entry at function end fires W0001. Keyed by + // `init`, the same TirRef `rebind_to_init` stamped Valid. + own.pending_dead_store.insert(init, (view.name, span)); } else { own.current_owner.insert(view.name, init); } @@ -317,6 +382,7 @@ fn analyze_return( .unwrap_or(OwnerState::NotTracked); match state { OwnerState::Valid => { + own.pending_dead_store.remove(&underlying); own.states.insert( underlying, OwnerState::Moved { @@ -383,6 +449,7 @@ fn consume_for_assignment( .unwrap_or(OwnerState::NotTracked); match state { OwnerState::Valid => { + own.pending_dead_store.remove(&underlying); own.states.insert( underlying, OwnerState::Moved { @@ -665,6 +732,11 @@ fn visit_expr( if let Some(&owner) = own.current_owner.get(&name) && needs_tracking(inst.ty, pool) { + // Any read counts as "used" for dead-store purposes, + // even if it ultimately fires E0020 — once the + // programmer's code looked at the value, they + // didn't ignore it. + own.pending_dead_store.remove(&owner); own.origin.insert(r, Some(owner)); if let Some(OwnerState::Moved { moved_at, .. }) = own.states.get(&owner).cloned() { sink.emit( diff --git a/tests/integration_tests.rs b/tests/integration_tests.rs index 8fd1250..86d142b 100644 --- a/tests/integration_tests.rs +++ b/tests/integration_tests.rs @@ -2564,3 +2564,34 @@ fn main(): String::from_utf8_lossy(&output.stderr) ); } + +#[test] +fn test_dead_store_warning() { + let temp_dir = TempDir::new().expect("temp"); + let code = "name: str = \"Alice\"\nprint(\"hello\")"; + let test_file = create_test_file(temp_dir.path(), "dead_store.ryo", code); + let output = run_ryo_command(&["run", "dead_store.ryo"], &test_file).expect("run"); + // Warning, not error — exit success. + assert!( + output.status.success(), + "STDERR: {}", + String::from_utf8_lossy(&output.stderr) + ); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + stderr.contains("W0001"), + "expected W0001 in stderr: {}", + stderr + ); +} + +#[test] +fn test_no_dead_store_when_used() { + let temp_dir = TempDir::new().expect("temp"); + let code = "name: str = \"Alice\"\nprint(name)"; + let test_file = create_test_file(temp_dir.path(), "live_store.ryo", code); + let output = run_ryo_command(&["run", "live_store.ryo"], &test_file).expect("run"); + assert!(output.status.success()); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!(!stderr.contains("W0001"), "unexpected W0001: {}", stderr); +} From f673cceb57f119401d7ac515b229e71e1c9e7a7a Mon Sep 17 00:00:00 2001 From: Pepe Navarro Date: Thu, 4 Jun 2026 11:31:45 +0200 Subject: [PATCH 18/27] feat(ownership): multi-label diagnostics with binding names + helps E0020/E0021/E0022 messages now name the involved binding, include a secondary label at the move site (Diag::with_note), and a help line suggesting the fix (move keyword for E0021, borrow-instead for E0020, locally-allocated value for E0022). --- src/ownership.rs | 126 +++++++++++++++++++++++++++++++------ tests/integration_tests.rs | 28 +++++++++ 2 files changed, 136 insertions(+), 18 deletions(-) diff --git a/src/ownership.rs b/src/ownership.rs index 107a50d..3d4fe0f 100644 --- a/src/ownership.rs +++ b/src/ownership.rs @@ -318,7 +318,16 @@ fn analyze_var_decl( visit_expr(tir, pool, own, sink, by_name, init); if needs_tracking(init_ty, pool) { let span = tir.span(r); - consume_for_assignment(own, sink, init, span, MoveKind::Assignment); + let consumed_name = consumed_binding_name(tir, init); + consume_for_assignment( + pool, + own, + sink, + init, + span, + MoveKind::Assignment, + consumed_name, + ); rebind_to_init(own, view.name, init); // Register the new binding as pending dead-store. The walk // clears this entry on any later read or consumption; a @@ -346,7 +355,16 @@ fn analyze_assign( visit_expr(tir, pool, own, sink, by_name, view.value); if needs_tracking(value_ty, pool) { let span = tir.span(r); - consume_for_assignment(own, sink, view.value, span, MoveKind::Assignment); + let consumed_name = consumed_binding_name(tir, view.value); + consume_for_assignment( + pool, + own, + sink, + view.value, + span, + MoveKind::Assignment, + consumed_name, + ); rebind_to_init(own, view.name, view.value); } } @@ -374,6 +392,7 @@ fn analyze_return( return; } let span = tir.span(r); + let consumed_name = consumed_binding_name(tir, operand); let underlying = underlying_owner(own, operand); let state = own .states @@ -392,16 +411,36 @@ fn analyze_return( ); } OwnerState::Borrowed => { - sink.emit(Diag::error( - span, - DiagCode::ReturnBorrowedValue, - "cannot return borrowed value", - )); + sink.emit( + Diag::error( + span, + DiagCode::ReturnBorrowedValue, + format!( + "cannot return borrowed value {} (Rule 5)", + format_binding(consumed_name, pool), + ), + ) + .with_note( + None, + "help: return a locally-allocated value, or accept the parameter as `move`", + ), + ); } OwnerState::Moved { moved_at, .. } => { sink.emit( - Diag::error(span, DiagCode::UseAfterMove, "use of moved value in return") - .with_note(Some(moved_at), "value was moved here"), + Diag::error( + span, + DiagCode::UseAfterMove, + format!( + "use of moved value {}", + format_binding(consumed_name, pool), + ), + ) + .with_note(Some(moved_at), "value moved here") + .with_note( + None, + "help: consider using the value before the move, or pass by default (borrow) instead of `move`", + ), ); } OwnerState::NotTracked => {} @@ -435,11 +474,13 @@ fn underlying_owner(own: &Ownership, init: TirRef) -> TirRef { /// Caller must have already populated origin/state for `init` via /// `visit_expr`. fn consume_for_assignment( + pool: &InternPool, own: &mut Ownership, sink: &mut DiagSink, init: TirRef, span: Span, kind: MoveKind, + name: Option, ) { let underlying = underlying_owner(own, init); let state = own @@ -459,22 +500,60 @@ fn consume_for_assignment( ); } OwnerState::Borrowed => { - sink.emit(Diag::error( - span, - DiagCode::MoveOutOfBorrowedParam, - "cannot move out of borrowed parameter", - )); + sink.emit( + Diag::error( + span, + DiagCode::MoveOutOfBorrowedParam, + format!( + "cannot move out of borrowed parameter {}", + format_binding(name, pool), + ), + ) + .with_note( + None, + "help: add `move` to the parameter declaration if you need ownership", + ), + ); } OwnerState::Moved { moved_at, .. } => { sink.emit( - Diag::error(span, DiagCode::UseAfterMove, "use of moved value") - .with_note(Some(moved_at), "value was moved here"), + Diag::error( + span, + DiagCode::UseAfterMove, + format!("use of moved value {}", format_binding(name, pool)), + ) + .with_note(Some(moved_at), "value moved here") + .with_note( + None, + "help: consider using the value before the move, or pass by default (borrow) instead of `move`", + ), ); } OwnerState::NotTracked => {} } } +/// Render a binding name for inclusion in a diagnostic message. +/// Returns `` `name` `` for known bindings and `value` for anonymous +/// temporaries (concat results, fresh allocations, etc.). +fn format_binding(name: Option, pool: &InternPool) -> String { + match name { + Some(n) => format!("`{}`", pool.str(n)), + None => "value".to_string(), + } +} + +/// If `r` is a direct `Var` read, return the binding name it aliases. +/// Used at consume sites to thread the source binding name into +/// E0020/E0021/E0022 messages. Returns `None` for fresh producers +/// (StrConst, StrConcat, Call), where there's no source binding. +fn consumed_binding_name(tir: &Tir, r: TirRef) -> Option { + match tir.inst(r).data { + TirData::Var(n) => Some(n), + _ => None, + } +} + /// CFG join for `if` / `elif` / `else`. The naïve forward walk /// would let a move inside a then-branch persist past the merge /// regardless of whether else also moved — wrong for the spec's @@ -700,12 +779,15 @@ fn visit_expr( .map(|p| p.is_move) .unwrap_or(false); if is_move_param { + let consumed_name = consumed_binding_name(tir, *arg); consume_for_assignment( + pool, own, sink, *arg, tir.span(r), MoveKind::MoveParam { callee: view.name }, + consumed_name, ); } // Borrow path: no extra check here. With the current @@ -740,8 +822,16 @@ fn visit_expr( own.origin.insert(r, Some(owner)); if let Some(OwnerState::Moved { moved_at, .. }) = own.states.get(&owner).cloned() { sink.emit( - Diag::error(tir.span(r), DiagCode::UseAfterMove, "use of moved value") - .with_note(Some(moved_at), "value was moved here"), + Diag::error( + tir.span(r), + DiagCode::UseAfterMove, + format!("use of moved value `{}`", pool.str(name)), + ) + .with_note(Some(moved_at), "value moved here") + .with_note( + None, + "help: consider using the value before the move, or pass by default (borrow) instead of `move`", + ), ); } } diff --git a/tests/integration_tests.rs b/tests/integration_tests.rs index 86d142b..7a3cd10 100644 --- a/tests/integration_tests.rs +++ b/tests/integration_tests.rs @@ -2419,6 +2419,34 @@ fn test_use_after_move_param() { ); } +#[test] +fn test_e0020_message_includes_binding_name() { + let temp_dir = TempDir::new().expect("temp"); + let code = r#" +fn consume(move s: str): + print(s) + +fn main(): + name: str = "Alice" + consume(name) + print(name) +"#; + let test_file = create_test_file(temp_dir.path(), "e0020_label.ryo", code); + let output = run_ryo_command(&["run", "e0020_label.ryo"], &test_file).expect("run"); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!(stderr.contains("E0020"), "stderr: {}", stderr); + assert!( + stderr.contains("name"), + "expected binding name in message: {}", + stderr + ); + assert!( + stderr.contains("moved here") || stderr.contains("moved into"), + "expected move-site note: {}", + stderr + ); +} + #[test] fn test_borrow_param_then_use_ok() { let temp_dir = TempDir::new().expect("temp"); From 2c5985d595c2ef23ca8316a0d73ffd7fcfe58404 Mon Sep 17 00:00:00 2001 From: Pepe Navarro Date: Thu, 4 Jun 2026 12:11:41 +0200 Subject: [PATCH 19/27] fix(ownership): drop redundant E0020 at consume sites MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Var arm already fires E0020 when reading a moved owner. The consume sites (VarDecl/Assign/Return) emitting again produced double diagnostics for one fault — same class as the Call-arm bug fixed in e3562e8. Also tighten MoveKind: MoveParam no longer carries an unused callee StringId payload, and the doc-comment now reflects that no diagnostic actually reads back the variant today. --- src/ownership.rs | 58 ++++++++++++++++---------------------- tests/integration_tests.rs | 49 ++++++++++++++++++++++++++++++++ 2 files changed, 73 insertions(+), 34 deletions(-) diff --git a/src/ownership.rs b/src/ownership.rs index 3d4fe0f..9d36dbc 100644 --- a/src/ownership.rs +++ b/src/ownership.rs @@ -70,15 +70,15 @@ pub(crate) enum OwnerState { Moved { moved_at: Span, kind: MoveKind }, } -/// Why a value transitioned to `Moved`. Drives diagnostic phrasing -/// at the use-after-move site ("moved into return", "moved into -/// `foo`", etc.). +/// Why a value transitioned to `Moved`. Currently only used to track +/// the kind for future per-kind diagnostic phrasing — see ISSUES.md +/// or future tasks. No diagnostic reads back the variant today. #[derive(Clone, Debug, PartialEq, Eq)] #[allow(dead_code)] pub(crate) enum MoveKind { Assignment, Return, - MoveParam { callee: StringId }, + MoveParam, } /// Per-function ownership state. `states` is the lattice itself, @@ -426,22 +426,11 @@ fn analyze_return( ), ); } - OwnerState::Moved { moved_at, .. } => { - sink.emit( - Diag::error( - span, - DiagCode::UseAfterMove, - format!( - "use of moved value {}", - format_binding(consumed_name, pool), - ), - ) - .with_note(Some(moved_at), "value moved here") - .with_note( - None, - "help: consider using the value before the move, or pass by default (borrow) instead of `move`", - ), - ); + OwnerState::Moved { .. } => { + // No diagnostic here — the `Var` arm in `visit_expr` + // already emits E0020 when `return name` reads a moved + // `name`. See the same rationale on the `Moved` arm in + // `consume_for_assignment`. } OwnerState::NotTracked => {} } @@ -515,19 +504,20 @@ fn consume_for_assignment( ), ); } - OwnerState::Moved { moved_at, .. } => { - sink.emit( - Diag::error( - span, - DiagCode::UseAfterMove, - format!("use of moved value {}", format_binding(name, pool)), - ) - .with_note(Some(moved_at), "value moved here") - .with_note( - None, - "help: consider using the value before the move, or pass by default (borrow) instead of `move`", - ), - ); + OwnerState::Moved { .. } => { + // No diagnostic here — the `Var` arm in `visit_expr` + // already emits E0020 for any aliasing read of a moved + // owner, which covers every path that lands here today + // (the only operands that reach a consume site with + // `Moved` underlying state arrive via `Var`). Direct + // producers (StrConst/StrConcat/Call) are stamped + // `Valid` in `visit_expr` on a freshly-walked branch and + // cannot be `Moved` here. Emitting again would + // double-report a single fault. + // + // If a future producer pattern bypasses `Var` and lands + // here in `Moved` state, prefer no diagnostic over a + // duplicate; revisit then. } OwnerState::NotTracked => {} } @@ -786,7 +776,7 @@ fn visit_expr( sink, *arg, tir.span(r), - MoveKind::MoveParam { callee: view.name }, + MoveKind::MoveParam, consumed_name, ); } diff --git a/tests/integration_tests.rs b/tests/integration_tests.rs index 7a3cd10..4bbe886 100644 --- a/tests/integration_tests.rs +++ b/tests/integration_tests.rs @@ -2419,6 +2419,55 @@ fn test_use_after_move_param() { ); } +#[test] +fn test_use_after_move_in_vardecl_one_diag() { + let temp_dir = TempDir::new().expect("temp"); + let code = r#" +fn consume(move s: str): + print(s) + +fn main(): + name: str = "Alice" + consume(name) + other: str = name +"#; + let test_file = create_test_file(temp_dir.path(), "uam_vardecl_one.ryo", code); + let output = run_ryo_command(&["run", "uam_vardecl_one.ryo"], &test_file).expect("run"); + let stderr = String::from_utf8_lossy(&output.stderr); + let count = stderr.matches("E0020").count(); + assert_eq!( + count, 1, + "expected exactly one E0020, got {}: {}", + count, stderr + ); +} + +#[test] +fn test_use_after_move_in_return_one_diag() { + let temp_dir = TempDir::new().expect("temp"); + let code = r#" +fn consume(move s: str): + print(s) + +fn forward(move s: str) -> str: + consume(s) + return s + +fn main(): + x: str = forward("hi") + print(x) +"#; + let test_file = create_test_file(temp_dir.path(), "uam_ret_one.ryo", code); + let output = run_ryo_command(&["run", "uam_ret_one.ryo"], &test_file).expect("run"); + let stderr = String::from_utf8_lossy(&output.stderr); + let count = stderr.matches("E0020").count(); + assert_eq!( + count, 1, + "expected exactly one E0020, got {}: {}", + count, stderr + ); +} + #[test] fn test_e0020_message_includes_binding_name() { let temp_dir = TempDir::new().expect("temp"); From c9483005183f370607d0335c2ed6fb22c8437e30 Mon Sep 17 00:00:00 2001 From: Pepe Navarro Date: Thu, 4 Jun 2026 12:38:37 +0200 Subject: [PATCH 20/27] refactor(ownership): apply /simplify cleanups - Drop MoveKind enum and OwnerState::Moved.kind (no diagnostic read it). - Add Diag::with_help, replace inline 'help: ' prefix. - Inline clone_snapshot/restore_from; let merge_branches take &[&Ownership] to remove redundant clones in loop fixed-points. - Replace HashSet allocations in merge_branches and states_differ with single-pass iteration. - Hoist Copy-type classification to InternPool::is_copy; eliminates duplicate between sema and ownership. - Drop stale #[allow(dead_code)] annotations on lattice items now that all variants are live. --- src/diag.rs | 6 ++ src/ownership.rs | 268 ++++++++++++++++------------------------------- src/sema.rs | 13 +-- src/types.rs | 13 +++ 4 files changed, 109 insertions(+), 191 deletions(-) diff --git a/src/diag.rs b/src/diag.rs index 6fbb903..04a40f9 100644 --- a/src/diag.rs +++ b/src/diag.rs @@ -171,6 +171,12 @@ impl Diag { }); self } + + /// Top-level help note (no span). Renders with a "help: " prefix + /// so callers don't need to encode the convention. + pub fn with_help(self, message: impl Into) -> Self { + self.with_note(None, format!("help: {}", message.into())) + } } /// Accumulator for diagnostics emitted by a single compilation pass. diff --git a/src/ownership.rs b/src/ownership.rs index 9d36dbc..e6a716c 100644 --- a/src/ownership.rs +++ b/src/ownership.rs @@ -28,18 +28,6 @@ use std::collections::{HashMap, HashSet}; // ---------- Classification ---------- -/// True for types whose values can be freely duplicated by `=` -/// without invalidating the source. Mirrors Mojo's `Copyable` trait -/// for the scalar primitives Ryo currently has. The ownership walk -/// short-circuits on these — they never enter the lattice. -#[allow(dead_code)] // wired into consumption logic in later tasks -pub(crate) fn is_copy_type(ty: TypeId, pool: &InternPool) -> bool { - matches!( - pool.kind(ty), - TypeKind::Int | TypeKind::Float | TypeKind::Bool - ) -} - /// True for types whose values transfer ownership on `=` and must be /// tracked through the function body. Today: `str` only. Future heap /// types (`List[T]`, `Dict[K, V]`) will join this set. @@ -62,23 +50,11 @@ pub(crate) fn needs_tracking(ty: TypeId, pool: &InternPool) -> bool { /// typed values start at `Valid` on definition, transition to /// `Borrowed` while a borrow is live, and to `Moved` once consumed. #[derive(Clone, Debug, PartialEq, Eq)] -#[allow(dead_code)] pub(crate) enum OwnerState { NotTracked, Valid, Borrowed, - Moved { moved_at: Span, kind: MoveKind }, -} - -/// Why a value transitioned to `Moved`. Currently only used to track -/// the kind for future per-kind diagnostic phrasing — see ISSUES.md -/// or future tasks. No diagnostic reads back the variant today. -#[derive(Clone, Debug, PartialEq, Eq)] -#[allow(dead_code)] -pub(crate) enum MoveKind { - Assignment, - Return, - MoveParam, + Moved { moved_at: Span }, } /// Per-function ownership state. `states` is the lattice itself, @@ -90,7 +66,6 @@ pub(crate) enum MoveKind { /// it derives from (or `None` for fresh allocations) — used to walk /// back to the root owner when diagnosing a use-after-move. #[derive(Default, Clone)] -#[allow(dead_code)] pub(crate) struct Ownership { pub states: HashMap, pub current_owner: HashMap, @@ -103,22 +78,6 @@ pub(crate) struct Ownership { } impl Ownership { - /// Capture the current lattice for branch analysis. The CFG-join - /// logic in `analyze_if_stmt` snapshots state before each branch, - /// walks branches independently from that snapshot, then merges. - fn clone_snapshot(&self) -> Ownership { - self.clone() - } - - /// Reset the lattice to a previous snapshot. Used to walk each - /// branch of an `if` from the same starting state. - fn restore_from(&mut self, snap: &Ownership) { - self.states = snap.states.clone(); - self.current_owner = snap.current_owner.clone(); - self.origin = snap.origin.clone(); - self.pending_dead_store = snap.pending_dead_store.clone(); - } - /// Conservatively merge per-branch lattices into `self`. For each /// tracked `TirRef`, if any branch left it `Moved` the merged /// state is `Moved` — that way a value consumed on only one @@ -126,28 +85,22 @@ impl Ownership { /// post-`if` use trips E0020. Non-`Moved` states pick the first /// observed branch state. `current_owner` entries from any branch /// are preserved so reseats inside a branch survive the join. - fn merge_branches(&mut self, branches: &[Ownership]) { - let mut all_refs: std::collections::HashSet = std::collections::HashSet::new(); + fn merge_branches(&mut self, branches: &[&Ownership]) { + // Rule: any branch Moved → Moved; otherwise first observed + // (across branches) wins. Walk each branch once and merge + // directly into `self.states` — no intermediate set of keys. for b in branches { - all_refs.extend(b.states.keys().copied()); - } - for r in all_refs { - let mut merged: Option = None; - for b in branches { - let s = b.states.get(&r).cloned().unwrap_or(OwnerState::NotTracked); - merged = Some(match (merged, s) { - (None, s) => s, - (Some(OwnerState::Moved { moved_at, kind }), _) => { - OwnerState::Moved { moved_at, kind } - } - (_, OwnerState::Moved { moved_at, kind }) => { - OwnerState::Moved { moved_at, kind } - } - (Some(a), _) => a, - }); - } - if let Some(state) = merged { - self.states.insert(r, state); + for (r, s) in &b.states { + self.states + .entry(*r) + .and_modify(|cur| { + if !matches!(cur, OwnerState::Moved { .. }) + && matches!(s, OwnerState::Moved { .. }) + { + *cur = s.clone(); + } + }) + .or_insert_with(|| s.clone()); } } for b in branches { @@ -165,43 +118,32 @@ impl Ownership { // (1) Pre-branch entries (already in `self.pending_dead_store` // before the branches walked): every branch started with // the entry. If any branch cleared it, the value was - // used somewhere — drop it. So the rule is "intersect - // across branches". + // used somewhere — drop it. Rule: intersect across + // branches. // // (2) Branch-local entries (introduced inside a branch by a // VarDecl): only the introducing branch has the key. If - // that branch ended with the entry still pending, the - // binding was declared and never used inside the branch - // and W0001 should still fire after the join. So the - // rule is "union across branches". + // that branch ended with the entry still pending, W0001 + // should still fire after the join. Rule: union across + // branches (skipping keys that were pre-branch — those + // are governed by rule (1)). // - // Distinguishing (1) vs (2) is just: was the key in - // `self.pending_dead_store` before the merge? Walk both - // groups separately so each gets its correct rule. - let mut merged_pending: HashMap = HashMap::new(); - - // (1) Pre-branch entries — keep iff still pending in *every* - // branch. - for (k, v) in &self.pending_dead_store { - if branches + // Snapshot the pre-branch key set so the union step can + // distinguish branch-local keys from pre-branch keys that + // (1) may have just dropped. + let pre_branch_keys: HashSet = self.pending_dead_store.keys().copied().collect(); + self.pending_dead_store.retain(|k, _| { + branches .iter() .all(|b| b.pending_dead_store.contains_key(k)) - { - merged_pending.insert(*k, *v); - } - } - - // (2) Branch-local entries — anything in a branch's pending - // that wasn't pre-existing. + }); for b in branches { for (k, v) in &b.pending_dead_store { - if !self.pending_dead_store.contains_key(k) { - merged_pending.insert(*k, *v); + if !pre_branch_keys.contains(k) { + self.pending_dead_store.insert(*k, *v); } } } - - self.pending_dead_store = merged_pending; } } @@ -319,15 +261,7 @@ fn analyze_var_decl( if needs_tracking(init_ty, pool) { let span = tir.span(r); let consumed_name = consumed_binding_name(tir, init); - consume_for_assignment( - pool, - own, - sink, - init, - span, - MoveKind::Assignment, - consumed_name, - ); + consume_for_assignment(pool, own, sink, init, span, consumed_name); rebind_to_init(own, view.name, init); // Register the new binding as pending dead-store. The walk // clears this entry on any later read or consumption; a @@ -356,15 +290,7 @@ fn analyze_assign( if needs_tracking(value_ty, pool) { let span = tir.span(r); let consumed_name = consumed_binding_name(tir, view.value); - consume_for_assignment( - pool, - own, - sink, - view.value, - span, - MoveKind::Assignment, - consumed_name, - ); + consume_for_assignment(pool, own, sink, view.value, span, consumed_name); rebind_to_init(own, view.name, view.value); } } @@ -402,13 +328,8 @@ fn analyze_return( match state { OwnerState::Valid => { own.pending_dead_store.remove(&underlying); - own.states.insert( - underlying, - OwnerState::Moved { - moved_at: span, - kind: MoveKind::Return, - }, - ); + own.states + .insert(underlying, OwnerState::Moved { moved_at: span }); } OwnerState::Borrowed => { sink.emit( @@ -420,10 +341,7 @@ fn analyze_return( format_binding(consumed_name, pool), ), ) - .with_note( - None, - "help: return a locally-allocated value, or accept the parameter as `move`", - ), + .with_help("return a locally-allocated value, or accept the parameter as `move`"), ); } OwnerState::Moved { .. } => { @@ -468,7 +386,6 @@ fn consume_for_assignment( sink: &mut DiagSink, init: TirRef, span: Span, - kind: MoveKind, name: Option, ) { let underlying = underlying_owner(own, init); @@ -480,13 +397,8 @@ fn consume_for_assignment( match state { OwnerState::Valid => { own.pending_dead_store.remove(&underlying); - own.states.insert( - underlying, - OwnerState::Moved { - moved_at: span, - kind, - }, - ); + own.states + .insert(underlying, OwnerState::Moved { moved_at: span }); } OwnerState::Borrowed => { sink.emit( @@ -498,10 +410,7 @@ fn consume_for_assignment( format_binding(name, pool), ), ) - .with_note( - None, - "help: add `move` to the parameter declaration if you need ownership", - ), + .with_help("add `move` to the parameter declaration if you need ownership"), ); } OwnerState::Moved { .. } => { @@ -565,35 +474,38 @@ fn analyze_if_stmt( let view = tir.if_stmt_view(r); visit_expr(tir, pool, own, sink, by_name, view.cond); - let snapshot = own.clone_snapshot(); + let snapshot = own.clone(); for stmt in &view.then_stmts { analyze_stmt(tir, pool, own, sink, by_name, *stmt); } - let then_state = own.clone_snapshot(); + let then_state = own.clone(); let mut branch_results = vec![then_state]; for elif in &view.elif_branches { - own.restore_from(&snapshot); + *own = snapshot.clone(); visit_expr(tir, pool, own, sink, by_name, elif.cond); for stmt in &elif.body { analyze_stmt(tir, pool, own, sink, by_name, *stmt); } - branch_results.push(own.clone_snapshot()); + branch_results.push(own.clone()); } if let Some(else_stmts) = &view.else_stmts { - own.restore_from(&snapshot); + *own = snapshot.clone(); for stmt in else_stmts { analyze_stmt(tir, pool, own, sink, by_name, *stmt); } - branch_results.push(own.clone_snapshot()); + branch_results.push(own.clone()); } else { branch_results.push(snapshot.clone()); } - own.restore_from(&snapshot); - own.merge_branches(&branch_results); + // Final restore: `snapshot` has no further uses, so move instead + // of clone. + *own = snapshot; + let refs: Vec<&Ownership> = branch_results.iter().collect(); + own.merge_branches(&refs); } /// Fixed-point ownership analysis for `while`. Walks the body once @@ -614,7 +526,7 @@ fn analyze_while_loop( r: TirRef, ) { let view = tir.while_loop_view(r); - let entry = own.clone_snapshot(); + let entry = own.clone(); // First pass into a scratch sink — diagnostics are kept only if // a single iteration suffices. If the lattice changes we re-walk @@ -625,24 +537,21 @@ fn analyze_while_loop( for stmt in &view.body { analyze_stmt(tir, pool, own, &mut scratch_sink, by_name, *stmt); } - let after_first = own.clone_snapshot(); + let after_first = own.clone(); if states_differ(&entry, &after_first) { - let merged = merge_two(&entry, &after_first); - own.restore_from(&merged); + *own = merge_two(&entry, &after_first); visit_expr(tir, pool, own, sink, by_name, view.cond); for stmt in &view.body { analyze_stmt(tir, pool, own, sink, by_name, *stmt); } - let after_second = own.clone_snapshot(); - let final_merged = merge_two(&entry, &after_second); - own.restore_from(&final_merged); + let after_second = own.clone(); + *own = merge_two(&entry, &after_second); } else { for d in scratch_sink.into_diags() { sink.emit(d); } - let final_merged = merge_two(&entry, &after_first); - own.restore_from(&final_merged); + *own = merge_two(&entry, &after_first); } } @@ -664,29 +573,26 @@ fn analyze_for_range( visit_expr(tir, pool, own, sink, by_name, view.start); visit_expr(tir, pool, own, sink, by_name, view.end); - let entry = own.clone_snapshot(); + let entry = own.clone(); let mut scratch_sink = DiagSink::new(); for stmt in &view.body { analyze_stmt(tir, pool, own, &mut scratch_sink, by_name, *stmt); } - let after_first = own.clone_snapshot(); + let after_first = own.clone(); if states_differ(&entry, &after_first) { - let merged = merge_two(&entry, &after_first); - own.restore_from(&merged); + *own = merge_two(&entry, &after_first); for stmt in &view.body { analyze_stmt(tir, pool, own, sink, by_name, *stmt); } - let after_second = own.clone_snapshot(); - let final_merged = merge_two(&entry, &after_second); - own.restore_from(&final_merged); + let after_second = own.clone(); + *own = merge_two(&entry, &after_second); } else { for d in scratch_sink.into_diags() { sink.emit(d); } - let final_merged = merge_two(&entry, &after_first); - own.restore_from(&final_merged); + *own = merge_two(&entry, &after_first); } } @@ -696,11 +602,24 @@ fn analyze_for_range( /// state changes (e.g. fresh definitions added inside the body) are /// loop-invariant for the use-after-move check. fn states_differ(a: &Ownership, b: &Ownership) -> bool { - let keys: HashSet = a.states.keys().chain(b.states.keys()).copied().collect(); - for k in keys { - let av = a.states.get(&k).cloned().unwrap_or(OwnerState::NotTracked); - let bv = b.states.get(&k).cloned().unwrap_or(OwnerState::NotTracked); - if matches!(av, OwnerState::Moved { .. }) != matches!(bv, OwnerState::Moved { .. }) { + // Diverge iff some TirRef is Moved in one and not the other. + // Walk each side once and check the other's matching entry — + // missing entries default to NotTracked, so a Moved with no + // counterpart counts as a divergence on its own. + for (k, av) in &a.states { + let a_moved = matches!(av, OwnerState::Moved { .. }); + let b_moved = matches!(b.states.get(k), Some(OwnerState::Moved { .. })); + if a_moved != b_moved { + return true; + } + } + for (k, bv) in &b.states { + if !matches!(bv, OwnerState::Moved { .. }) { + continue; + } + // Only need to catch keys exclusive to b that are Moved — + // keys present in a were handled in the first loop. + if !a.states.contains_key(k) { return true; } } @@ -711,8 +630,8 @@ fn states_differ(a: &Ownership, b: &Ownership) -> bool { /// either input has a TirRef `Moved`, the result is `Moved`. Used to /// seed the second pass and to compute the post-loop state. fn merge_two(a: &Ownership, b: &Ownership) -> Ownership { - let mut merged = a.clone_snapshot(); - merged.merge_branches(&[a.clone_snapshot(), b.clone_snapshot()]); + let mut merged = a.clone(); + merged.merge_branches(&[a, b]); merged } @@ -770,15 +689,7 @@ fn visit_expr( .unwrap_or(false); if is_move_param { let consumed_name = consumed_binding_name(tir, *arg); - consume_for_assignment( - pool, - own, - sink, - *arg, - tir.span(r), - MoveKind::MoveParam, - consumed_name, - ); + consume_for_assignment(pool, own, sink, *arg, tir.span(r), consumed_name); } // Borrow path: no extra check here. With the current // set of producers (StrConst/StrConcat/Call/Var), every @@ -818,9 +729,8 @@ fn visit_expr( format!("use of moved value `{}`", pool.str(name)), ) .with_note(Some(moved_at), "value moved here") - .with_note( - None, - "help: consider using the value before the move, or pass by default (borrow) instead of `move`", + .with_help( + "consider using the value before the move, or pass by default (borrow) instead of `move`", ), ); } @@ -872,10 +782,10 @@ mod tests { #[test] fn copy_types_classified() { let pool = InternPool::new(); - assert!(is_copy_type(pool.int(), &pool)); - assert!(is_copy_type(pool.float(), &pool)); - assert!(is_copy_type(pool.bool_(), &pool)); - assert!(!is_copy_type(pool.str_(), &pool)); + assert!(pool.is_copy(pool.int())); + assert!(pool.is_copy(pool.float())); + assert!(pool.is_copy(pool.bool_())); + assert!(!pool.is_copy(pool.str_())); } #[test] diff --git a/src/sema.rs b/src/sema.rs index 9dbdff6..6e743b7 100644 --- a/src/sema.rs +++ b/src/sema.rs @@ -54,17 +54,6 @@ use crate::uir::{CallView, FuncBody, InstData, InstRef, InstTag, Span, Uir, VarD use std::collections::{HashMap, VecDeque}; use std::path::Path; -// ---------- Copy-type classification ---------- - -/// True for types whose values are duplicated on read (no destructor, -/// no aliasing concern). Mirrors Mojo's `Copyable` trait for the -/// scalar primitives Ryo currently has. Used by sema to flag -/// redundant `move` annotations and (later) by the ownership pass -/// to short-circuit liveness on these instructions. -fn is_copy_type_kind(kind: TypeKind) -> bool { - matches!(kind, TypeKind::Int | TypeKind::Float | TypeKind::Bool) -} - // ---------- Decl table ---------- /// Index into `uir.func_bodies`. One [`DeclId`] per function the @@ -374,7 +363,7 @@ fn analyze_function(sema: &mut Sema<'_>, body: &FuncBody) -> Tir { // redundant noise. `move` on `str` (and other heap types) stays // silent — that's the whole reason the keyword exists. for param in &body.params { - if param.is_move && is_copy_type_kind(sema.pool.kind(param.ty)) { + if param.is_move && sema.pool.is_copy(param.ty) { let name = sema.pool.str(param.name).to_string(); let ty_str = sema.pool.display(param.ty).to_string(); sema.sink.emit(Diag::warning( diff --git a/src/types.rs b/src/types.rs index 868ed20..7840296 100644 --- a/src/types.rs +++ b/src/types.rs @@ -347,6 +347,19 @@ impl InternPool { id.0 == ID_NEVER } + /// True for types whose values are duplicated on `=` without + /// invalidating the source. Mirrors Mojo's `Copyable` trait for + /// the scalar primitives Ryo currently has: `int`, `float`, + /// `bool`. Used by sema (to flag redundant `move` annotations) + /// and by the ownership pass (to short-circuit liveness on + /// these values — they never enter the lattice). + pub fn is_copy(&self, ty: TypeId) -> bool { + matches!( + self.kind(ty), + TypeKind::Int | TypeKind::Float | TypeKind::Bool + ) + } + /// Compatibility predicate that absorbs the `Error` sentinel. /// Used anywhere sema would otherwise emit a type-mismatch. pub fn compatible(&self, a: TypeId, b: TypeId) -> bool { From eccccff255e3f366f88f96cf13aeb7e4ed6a55d6 Mon Sep 17 00:00:00 2001 From: Pepe Navarro Date: Thu, 4 Jun 2026 12:53:47 +0200 Subject: [PATCH 21/27] fix(ownership): merge current_owner reassignments per binding merge_branches kept pre-branch current_owner mappings untouched via entry().or_insert(), so post-merge reads of a binding resolved to the pre-branch owner regardless of any branch reseats. That produced one false negative (then-branch consumes a reseat, else-branch leaves the original valid -> use after merge missed the conditional move) and two false positives (loop and full conditional reassignments where every path ends Valid still resolved to a Moved pre-branch owner). Fix: snapshot pre-branch (name, owner_pre) pairs before the merge, then for each pair compute the merged state via each branch's end-of-branch owner (b.current_owner[name]) and write it back onto owner_pre in self.states. Branch-local introductions (names declared inside a branch) keep merging via the existing entry().or_insert() path. --- src/ownership.rs | 42 ++++++++++++++++++++ tests/integration_tests.rs | 79 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 121 insertions(+) diff --git a/src/ownership.rs b/src/ownership.rs index e6a716c..2610d44 100644 --- a/src/ownership.rs +++ b/src/ownership.rs @@ -86,6 +86,18 @@ impl Ownership { /// observed branch state. `current_owner` entries from any branch /// are preserved so reseats inside a branch survive the join. fn merge_branches(&mut self, branches: &[&Ownership]) { + // Snapshot pre-branch (name, owner) pairs before we start + // touching `self.states`. After the per-TirRef merge below + // we revisit each pre-branch binding and recompute its state + // through whichever owner each branch ended on, so a branch + // that reseats `current_owner[name]` to a fresh TirRef still + // contributes its end-of-branch state to the merged binding. + // Without this, post-`if` reads of `name` resolve through the + // pre-branch owner whose state reflects only what happened to + // *that* TirRef, missing reseats inside branches. + let pre_branch_owners: Vec<(StringId, TirRef)> = + self.current_owner.iter().map(|(n, t)| (*n, *t)).collect(); + // Rule: any branch Moved → Moved; otherwise first observed // (across branches) wins. Walk each branch once and merge // directly into `self.states` — no intermediate set of keys. @@ -112,6 +124,36 @@ impl Ownership { } } + // Binding-aware override: for each name that existed before + // the branches walked, look up where each branch left that + // binding (b.current_owner[name]), read that owner's state in + // the branch, and merge across branches with the same any- + // Moved-wins rule. Write the merged state back onto the + // pre-branch owner in `self.states` so post-merge reads of + // `name` (which still resolve via `self.current_owner[name]` + // = owner_pre) see the union of what each branch did to its + // respective end-of-branch owner. + for (name, owner_pre) in &pre_branch_owners { + let mut merged: Option = None; + for b in branches { + let owner_b = b.current_owner.get(name).copied().unwrap_or(*owner_pre); + let state_b = b + .states + .get(&owner_b) + .cloned() + .unwrap_or(OwnerState::NotTracked); + merged = Some(match (&merged, &state_b) { + (None, _) => state_b, + (Some(OwnerState::Moved { .. }), _) => merged.clone().unwrap(), + (_, OwnerState::Moved { .. }) => state_b, + (Some(_), _) => merged.clone().unwrap(), + }); + } + if let Some(state) = merged { + self.states.insert(*owner_pre, state); + } + } + // Pending dead-store entries: a key falls into one of two // buckets relative to the pre-branch snapshot in `self`. // diff --git a/tests/integration_tests.rs b/tests/integration_tests.rs index 4bbe886..7c84ecb 100644 --- a/tests/integration_tests.rs +++ b/tests/integration_tests.rs @@ -2642,6 +2642,85 @@ fn main(): ); } +#[test] +fn test_conditional_partial_rebind_then_use_fails() { + let temp_dir = TempDir::new().expect("temp"); + let code = r#" +fn consume(move s: str): + print(s) + +fn main(): + mut s: str = "hello" + flag: bool = true + if flag: + s = "world" + consume(s) + else: + print("else") + print(s) +"#; + let test_file = create_test_file(temp_dir.path(), "cond_partial_rebind.ryo", code); + let output = run_ryo_command(&["run", "cond_partial_rebind.ryo"], &test_file).expect("run"); + assert!( + !output.status.success(), + "expected E0020: STDERR: {}", + String::from_utf8_lossy(&output.stderr) + ); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!(stderr.contains("E0020"), "expected E0020: {}", stderr); +} + +#[test] +fn test_loop_consume_then_rebind_ok() { + let temp_dir = TempDir::new().expect("temp"); + let code = r#" +fn consume(move s: str): + print(s) + +fn main(): + mut name: str = "Alice" + mut i: int = 0 + while i < 3: + consume(name) + name = "Bob" + i = i + 1 +"#; + let test_file = create_test_file(temp_dir.path(), "loop_consume_rebind.ryo", code); + let output = run_ryo_command(&["run", "loop_consume_rebind.ryo"], &test_file).expect("run"); + assert!( + output.status.success(), + "STDERR: {}", + String::from_utf8_lossy(&output.stderr) + ); +} + +#[test] +fn test_both_branches_consume_and_rebind_ok() { + let temp_dir = TempDir::new().expect("temp"); + let code = r#" +fn consume(move s: str): + print(s) + +fn main(): + mut name: str = "Alice" + flag: bool = true + if flag: + consume(name) + name = "Bob" + else: + consume(name) + name = "Charlie" + print(name) +"#; + let test_file = create_test_file(temp_dir.path(), "both_branches_rebind.ryo", code); + let output = run_ryo_command(&["run", "both_branches_rebind.ryo"], &test_file).expect("run"); + assert!( + output.status.success(), + "STDERR: {}", + String::from_utf8_lossy(&output.stderr) + ); +} + #[test] fn test_dead_store_warning() { let temp_dir = TempDir::new().expect("temp"); From 20cdd020a9c896285fe92a913097e1e198c03eb2 Mon Sep 17 00:00:00 2001 From: Pepe Navarro Date: Thu, 4 Jun 2026 13:08:05 +0200 Subject: [PATCH 22/27] fix(pipeline): consolidate diagnostic render path across run/build/ir lower_and_analyze had duplicated render blocks for the success and error paths after the M8.1b warning-on-success work; ir_command silently dropped warnings on the success path. Refactor both to a single tail block via finalize_diags: drain the sink, render whatever is there, return Err iff any errors are present. Surfaces W0001/W0002 on `ryo ir` runs the same way they appear on `ryo run`. Resolves ISSUES.md I-044. --- src/ownership.rs | 8 ++++++ src/pipeline.rs | 56 +++++++++++++++++++++++--------------- tests/integration_tests.rs | 26 ++++++++++++++++++ 3 files changed, 68 insertions(+), 22 deletions(-) diff --git a/src/ownership.rs b/src/ownership.rs index 2610d44..37b3bbf 100644 --- a/src/ownership.rs +++ b/src/ownership.rs @@ -272,6 +272,14 @@ fn analyze_stmt( TirTag::Break | TirTag::Continue => { // 8.1c attaches Free metadata here; 8.1b is a no-op. } + TirTag::CompoundAssign => { + // Compound-assign on Move-typed values is rejected by sema today + // (str doesn't support `+=`/`-=`/etc.), so this is unreachable for + // tracked types. Explicit no-op arm so the next reader doesn't + // wonder whether the fall-through to visit_expr is correct. + // Revisit when compound-assign on Move-typed values becomes + // representable. See ISSUES.md I-046. + } TirTag::ExprStmt => { if let TirData::UnOp(o) = inst.data { visit_expr(tir, pool, own, sink, by_name, o); diff --git a/src/pipeline.rs b/src/pipeline.rs index cb3d962..f96af64 100644 --- a/src/pipeline.rs +++ b/src/pipeline.rs @@ -290,7 +290,7 @@ pub(crate) fn ir_command(file: &Path, emit: &[EmitKind]) -> Result<(), CompilerE if !(want.tir || want.clif) { // UIR-only run. Surface astgen diagnostics now, with a // non-zero exit if anything fired. - return finish_with_diags(sink, &input, &name); + return finalize_diags(sink, &input, &name); } // For TIR / CLIF we also run sema. Per the §4.5 design, sema @@ -310,12 +310,16 @@ pub(crate) fn ir_command(file: &Path, emit: &[EmitKind]) -> Result<(), CompilerE // failed, surface the diagnostics and abort — we cannot // produce a meaningful CLIF dump from a broken TIR. if sink.has_errors() { - return finish_with_diags(sink, &input, &name); + return finalize_diags(sink, &input, &name); } generate_and_display_ir(&tirs, &pool)?; } - finish_with_diags(sink, &input, &name) + // Tail block: drains the sink whether sema/ownership were + // clean or only emitted warnings. Without this `ryo ir` would + // silently swallow W0001/W0002 on success — the bug I-044 + // tracked. + finalize_diags(sink, &input, &name) } /// Resolve `--emit` flag values into a normalized set. Membership @@ -354,14 +358,28 @@ impl EmitSet { } } -/// Render any pending diagnostics and translate them into a -/// `CompilerError::Diagnostics` (or `Ok(())` if the sink is -/// clean). Centralized so every `ryo ir` exit path uses the same -/// rendering plumbing. -fn finish_with_diags(sink: DiagSink, input: &str, source_name: &str) -> Result<(), CompilerError> { - if sink.has_errors() { - let diags = sink.into_diags(); +/// Drain `sink`, render whatever is queued, and translate into a +/// terminal pipeline result. +/// +/// Single tail-block used by every front-end driver (`ryo run`, +/// `ryo build`, `ryo ir`) so that: +/// +/// * warnings (`W0001` DeadStore, `W0002` RedundantMove, …) reach +/// the user on otherwise-successful runs, and +/// * the success and error paths render the *same* diagnostics +/// exactly once — never via two separate `render_diags` calls +/// that could drift out of sync. +/// +/// Returns `Err(CompilerError::Diagnostics(_))` iff at least one +/// diagnostic has `Severity::Error`; warnings/notes alone do not +/// fail the build. +fn finalize_diags(sink: DiagSink, input: &str, source_name: &str) -> Result<(), CompilerError> { + let diags = sink.into_diags(); + let has_errors = diags.iter().any(|d| d.severity == Severity::Error); + if !diags.is_empty() { render_diags(&diags, input, source_name); + } + if has_errors { Err(CompilerError::Diagnostics(diags)) } else { Ok(()) @@ -396,18 +414,12 @@ fn lower_and_analyze( // run is the whole point of the structured-diagnostics phase. let tirs = sema::analyze(&uir, pool, &mut sink, input, file_path); crate::ownership::check(&tirs, pool, &mut sink); - if sink.has_errors() { - let diags = sink.into_diags(); - render_diags(&diags, input, source_name); - return Err(CompilerError::Diagnostics(diags)); - } - // No errors — but warnings/notes may still be queued. Render - // them so the user sees `move`-on-Copy lints (W0002) and any - // future lints on otherwise-successful builds. - let diags = sink.into_diags(); - if !diags.is_empty() { - render_diags(&diags, input, source_name); - } + // Single tail block: render-if-non-empty, Err iff any errors. + // Same shape as `ir_command` so warnings (`W0001` DeadStore, + // `W0002` RedundantMove, …) surface on the success path + // without a separate render block that could drift from the + // error path. + finalize_diags(sink, input, source_name)?; Ok(tirs) } diff --git a/tests/integration_tests.rs b/tests/integration_tests.rs index 7c84ecb..c9ec723 100644 --- a/tests/integration_tests.rs +++ b/tests/integration_tests.rs @@ -957,6 +957,32 @@ fn ir_emit_tir_prints_partial_tir_with_unreachable_on_sema_error() { ); } +#[test] +fn test_ryo_ir_surfaces_warnings_on_success() { + // I-044 regression: `ryo ir` used to call sema + ownership + // against a sink and only render on the error path, so any + // warnings (W0001 DeadStore, W0002 RedundantMove) emitted on + // a successful run were silently dropped. After the + // single-tail-block refactor, `ryo ir` should surface the same + // diagnostics on stderr that `ryo run` / `ryo build` already + // do (cf. test_redundant_move_on_int_warns). + let temp_dir = TempDir::new().expect("temp"); + let code = "fn f(move x: int):\n\tprint(int_to_str(x))\n\nf(42)"; + let test_file = create_test_file(temp_dir.path(), "ir_w0002.ryo", code); + let output = run_ryo_command(&["ir", "--emit=tir", "ir_w0002.ryo"], &test_file).expect("run"); + assert!( + output.status.success(), + "STDERR: {}", + String::from_utf8_lossy(&output.stderr) + ); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + stderr.contains("W0002"), + "expected W0002 from ryo ir on stderr: {}", + stderr + ); +} + // ============================================================================ // Milestone 8b: Conditionals & Logical Operators // ============================================================================ From 5f04567f66d1d9eeca411ec6a0701d1d4fa17456 Mon Sep 17 00:00:00 2001 From: Pepe Navarro Date: Thu, 4 Jun 2026 13:37:46 +0200 Subject: [PATCH 23/27] refactor(ownership): unify consume + return state-match via BorrowedAction MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit consume_for_assignment and analyze_return ran the same four-arm state match (Valid/Borrowed/Moved/NotTracked) with only the Borrowed-arm diagnostic differing (E0021 vs E0022). The recent fix in 2c5985d (drop redundant E0020 at consume sites) had to be applied in both places — exactly the divergence risk dedup prevents. Extract consume_underlying parameterized by a BorrowedAction enum; analyze_return delegates instead of duplicating. Diagnostic wording, help text, and binding-name rendering are preserved verbatim. Resolves ISSUES.md I-052. --- src/ownership.rs | 108 +++++++++++++++++++++++++++++------------------ 1 file changed, 68 insertions(+), 40 deletions(-) diff --git a/src/ownership.rs b/src/ownership.rs index 37b3bbf..30fa4c6 100644 --- a/src/ownership.rs +++ b/src/ownership.rs @@ -369,39 +369,15 @@ fn analyze_return( } let span = tir.span(r); let consumed_name = consumed_binding_name(tir, operand); - let underlying = underlying_owner(own, operand); - let state = own - .states - .get(&underlying) - .cloned() - .unwrap_or(OwnerState::NotTracked); - match state { - OwnerState::Valid => { - own.pending_dead_store.remove(&underlying); - own.states - .insert(underlying, OwnerState::Moved { moved_at: span }); - } - OwnerState::Borrowed => { - sink.emit( - Diag::error( - span, - DiagCode::ReturnBorrowedValue, - format!( - "cannot return borrowed value {} (Rule 5)", - format_binding(consumed_name, pool), - ), - ) - .with_help("return a locally-allocated value, or accept the parameter as `move`"), - ); - } - OwnerState::Moved { .. } => { - // No diagnostic here — the `Var` arm in `visit_expr` - // already emits E0020 when `return name` reads a moved - // `name`. See the same rationale on the `Moved` arm in - // `consume_for_assignment`. - } - OwnerState::NotTracked => {} - } + consume_underlying( + pool, + own, + sink, + operand, + span, + consumed_name, + BorrowedAction::ReturnBorrowed, + ); } /// Reseat `name` to point at `init` as its current owner. After a @@ -427,6 +403,19 @@ fn underlying_owner(own: &Ownership, init: TirRef) -> TirRef { } } +/// Which diagnostic the shared `consume_underlying` helper should +/// emit on the `Borrowed` arm. `consume_for_assignment` and +/// `analyze_return` run identical state transitions otherwise — the +/// only thing that diverges is the error code / wording / help when +/// the underlying owner is borrowed. +enum BorrowedAction { + /// E0021 — `consume_for_assignment` site (VarDecl / Assign / + /// move-typed Call argument). + MoveOutOfParam, + /// E0022 — `analyze_return` site. + ReturnBorrowed, +} + /// Apply the consumption transition for a Move-typed initializer. /// Caller must have already populated origin/state for `init` via /// `visit_expr`. @@ -438,7 +427,38 @@ fn consume_for_assignment( span: Span, name: Option, ) { - let underlying = underlying_owner(own, init); + consume_underlying( + pool, + own, + sink, + init, + span, + name, + BorrowedAction::MoveOutOfParam, + ); +} + +/// Shared transition for every consume site (assignment, return, +/// move-typed Call argument). Walks back to the underlying owner, +/// reads its state, and either: +/// +/// * `Valid` → stamp `Moved { moved_at: span }` and clear any pending +/// dead-store entry, +/// * `Borrowed` → emit E0021 or E0022 per `on_borrowed`, +/// * `Moved` → silent (the `Var` arm of `visit_expr` already emitted +/// E0020 for the aliasing read that brought us here; emitting again +/// would double-report a single fault — see commit 2c5985d), +/// * `NotTracked` → no-op. +fn consume_underlying( + pool: &InternPool, + own: &mut Ownership, + sink: &mut DiagSink, + operand: TirRef, + span: Span, + name: Option, + on_borrowed: BorrowedAction, +) { + let underlying = underlying_owner(own, operand); let state = own .states .get(&underlying) @@ -451,17 +471,25 @@ fn consume_for_assignment( .insert(underlying, OwnerState::Moved { moved_at: span }); } OwnerState::Borrowed => { - sink.emit( - Diag::error( - span, + let (code, msg, help) = match on_borrowed { + BorrowedAction::MoveOutOfParam => ( DiagCode::MoveOutOfBorrowedParam, format!( "cannot move out of borrowed parameter {}", format_binding(name, pool), ), - ) - .with_help("add `move` to the parameter declaration if you need ownership"), - ); + "add `move` to the parameter declaration if you need ownership", + ), + BorrowedAction::ReturnBorrowed => ( + DiagCode::ReturnBorrowedValue, + format!( + "cannot return borrowed value {} (Rule 5)", + format_binding(name, pool), + ), + "return a locally-allocated value, or accept the parameter as `move`", + ), + }; + sink.emit(Diag::error(span, code, msg).with_help(help)); } OwnerState::Moved { .. } => { // No diagnostic here — the `Var` arm in `visit_expr` From 1c58b472496bd92929faf9955b6efd37e3d638c6 Mon Sep 17 00:00:00 2001 From: Pepe Navarro Date: Thu, 4 Jun 2026 14:16:11 +0200 Subject: [PATCH 24/27] docs(issues): record M8.1b deferred follow-ups Tracks the follow-up items surfaced by the four-angle /simplify review of the ownership pass: scratch-sink loop shape (I-045), UIR is_move pass-through (I-047), Call arg_modes in TIR (I-048), Owner enum to replace synthetic_param_ref (I-049), UAM altitude (I-050), loop-helper duplication (I-051), and the parameter-only Borrowed state (I-053). I-044 (ir_command warning render) and I-046 (CompoundAssign no-op arm) and I-052 (consume/return state-match dedup) were resolved in 20cdd02 and 5f04567 and are not added here per the repo convention of removing resolved entries. --- ISSUES.md | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/ISSUES.md b/ISSUES.md index 86f0bd3..1fb0bd3 100644 --- a/ISSUES.md +++ b/ISSUES.md @@ -211,6 +211,41 @@ Resolved entries are removed (not kept around as a changelog). Look at `git log` **Summary:** The runtime staticlib only uses `std::alloc`, `std::process::abort()`, and `eprintln!`, yet linking against precompiled `std` bundles objects with `_Unwind_*` symbol references. This forces the linker to pass `-lunwind` on Linux (workaround in `src/linker.rs`). Migrating to `#![no_std]` with `extern crate alloc` eliminates the dependency entirely. **Resolution:** Replace `std::alloc` with `alloc::alloc` (identical API). Replace `eprintln!` + `process::abort()` with `extern "C" { fn abort() -> !; }`. Add `#[panic_handler]` that aborts. Keep the `rlib` crate-type for `cargo test` via a `#[cfg(test)]` std gate. `ryu` already supports `no_std`. Benefits: smaller archive, faster link times, no hidden unwind dependency, simpler cross-compilation. +### I-045 — Loop fixed-point uses a scratch sink and re-walks the body to suppress duplicates +**Files:** `src/ownership.rs` (`analyze_while_loop`, `analyze_for_range`) +**Summary:** Both loop helpers walk the body once into a throwaway `DiagSink`, compare entry vs. post-body Moved-ness via `states_differ`, and then either replay the scratch diagnostics (converged) or re-walk the body from the merged state against the real sink (didn't converge). Diagnostic output is implicitly a function of *which iteration emitted it*, not of the converged lattice. Today this happens to work because the M8.1 patterns converge in ≤2 iterations, but the body-twice-with-discard shape couples diagnostic emission to control-flow analysis instead of running checks against a fixed-point. +**Resolution:** Refactor to a propagate-only first pass (state mutations only, no diagnostics), iterate to fixed-point, then a single check pass at the converged lattice that emits diagnostics. Removes the scratch sink entirely and makes the loop helpers symmetric with `analyze_if_stmt`. + +### I-047 — UIR `is_move` field is a pass-through +**Files:** `src/uir.rs` (`UirParam`), `src/astgen.rs`, `src/sema.rs` +**Summary:** `is_move` is threaded lexer → parser → AST → UIR → TIR. The UIR copy is never read: astgen propagates the AST flag in, sema reads it back out into `TirParam`, and no UIR pass inspects it. UIR is structural lowering with no semantic meaning, so `UirParam::is_move` is dead weight that exists only to bridge two layers it shouldn't. +**Resolution:** Drop `UirParam::is_move`. Sema can read the flag straight from the AST `FuncBody` (or via a side-channel keyed by FuncBody) when it constructs `TirParam`. Wait until any other UIR-level pass needs the flag before re-introducing it. + +### I-048 — Ownership pass looks up call conventions by name at every Call site +**Files:** `src/ownership.rs` (`visit_expr` Call arm), `src/sema.rs` +**Summary:** For every `Call` instruction, the ownership walker rebuilds a `by_name: HashMap` over all functions and indexes `params[i].is_move` to decide whether each arg should consume or borrow. The map is threaded through nine functions, and builtins need a special "no entry → all borrow" branch. Sema already knows the callee signature when it lowers the call; encoding the per-arg convention into TIR there would let ownership read it directly. +**Resolution:** Add an `arg_modes: ExtraRange` (or a per-arg `ParamMode` enum) alongside `args` in the TIR `Call` view, populated by sema. Ownership then reads `tir.call_view(r).arg_modes[i]` and the `by_name` plumbing disappears. Builtins become uniform (sema stamps `Borrow` for them). Also gives a place to put future indirect-call / fn-pointer conventions. + +### I-049 — `synthetic_param_ref` encoding is informal; model `Owner` as an enum +**Files:** `src/ownership.rs` (`synthetic_param_ref`, `Ownership::states`/`origin`) +**Summary:** Parameters live in the same `states: HashMap` as instruction refs by encoding their key as `u32::MAX - name.raw()`. Correctness depends on real per-function `TirRef`s never approaching `u32::MAX/2` and on `name.raw()` never being zero — neither asserted, both relying on convention. A future change to either `TirRef` numbering or `StringId` interning silently breaks the assumption. Cleaner shape: model owners as an explicit `enum Owner { Param(StringId), Inst(TirRef) }` and key the lattice maps on `Owner`. +**Resolution:** Introduce `Owner` and migrate `Ownership::states`/`origin`/`pending_dead_store` and `current_owner` value types over to it. Drops the `synthetic_param_ref` helper, the implicit numbering coupling, and gives a clean place to add `Owner::Borrow(...)` once real borrow expressions land. + +### I-050 — Var-arm holds UAM detection; consume sites are silent by policy +**Files:** `src/ownership.rs` (`visit_expr` Var arm, `consume_for_assignment`, `analyze_return`, Call arm) +**Summary:** Use-after-move detection only fires in the `Var` arm of `visit_expr`. The four consume sites (VarDecl, Assign, Return, move-Call) all carry comments explaining why they *don't* re-emit, and the policy works only because every consumable operand currently flows through `Var`. Three reviewers (and one bug fix during M8.1b) flagged this as the wrong altitude: any future producer pattern that bypasses `Var` (e.g., a directly-passed `Call` result that was already moved upstream) silently sidesteps the check. +**Resolution:** Invert responsibility. The consume helper becomes the single authority on UAM (it already inspects `underlying_owner` + state); the `Var` arm restricts itself to bookkeeping (origin link + dead-store clear). Pair the change with regression coverage that exercises a non-Var operand path so the new authority is observably tested. + +### I-051 — `analyze_while_loop` / `analyze_for_range` are 90% identical +**Files:** `src/ownership.rs` +**Summary:** After their distinct preludes (visit `cond` vs visit `start` + `end`), the bodies are byte-for-byte the same — entry snapshot, scratch-sink walk, `states_differ` check, optional re-walk, final `merge_two`. Two near-clones of a non-trivial fixed-point loop is exactly where divergence creeps in (only one gets a fix when behavior needs to change). +**Resolution:** Extract `analyze_loop_body(tir, pool, own, sink, by_name, body: &[TirRef])` after the caller has visited the loop's prelude. Folds with I-045 (propagate-only + check pass) — both refactors touch the same bodies. + +### I-053 — `OwnerState::Borrowed` is currently parameter-only +**Files:** `src/ownership.rs` (`OwnerState`) +**Summary:** `OwnerState::Borrowed` is set only at parameter init; no expression produces it. The two sites that read it (E0021, E0022) could equivalently look up `tir.params` for the underlying owner's source param and check `is_move`. The state is anticipating real borrow expressions (`&x`) which the spec migrated to in commit 2ccf6b6 but the compiler doesn't lower yet. +**Resolution:** Document the invariant inline ("only ever set at param init in M8.1b; transitions arrive when `&x` borrow expressions land in a future milestone"). No code change today. + --- ## Cross-References From e9b7e27b9532be964ca4a6667d24fe665a14293d Mon Sep 17 00:00:00 2001 From: Pepe Navarro Date: Thu, 4 Jun 2026 14:25:09 +0200 Subject: [PATCH 25/27] fix(ownership): emit W0001 dead-store warning on unused reassignments Ensure variable reassignments (via Assign) are registered in the pending_dead_store map so that we correctly report dead store warnings (W0001) for Move-typed values that are overwritten or never consumed. --- src/ownership.rs | 1 + tests/integration_tests.rs | 19 +++++++++++++++++++ 2 files changed, 20 insertions(+) diff --git a/src/ownership.rs b/src/ownership.rs index 30fa4c6..7736c4a 100644 --- a/src/ownership.rs +++ b/src/ownership.rs @@ -342,6 +342,7 @@ fn analyze_assign( let consumed_name = consumed_binding_name(tir, view.value); consume_for_assignment(pool, own, sink, view.value, span, consumed_name); rebind_to_init(own, view.name, view.value); + own.pending_dead_store.insert(view.value, (view.name, span)); } } diff --git a/tests/integration_tests.rs b/tests/integration_tests.rs index c9ec723..45c7fa9 100644 --- a/tests/integration_tests.rs +++ b/tests/integration_tests.rs @@ -2777,3 +2777,22 @@ fn test_no_dead_store_when_used() { let stderr = String::from_utf8_lossy(&output.stderr); assert!(!stderr.contains("W0001"), "unexpected W0001: {}", stderr); } + +#[test] +fn test_dead_store_warning_reassignment() { + let temp_dir = TempDir::new().expect("temp"); + let code = "fn main():\n\tmut name: str = \"Alice\"\n\tprint(name)\n\tname = \"Bob\"\n\tprint(\"done\")\n"; + let test_file = create_test_file(temp_dir.path(), "reassign_dead.ryo", code); + let output = run_ryo_command(&["run", "reassign_dead.ryo"], &test_file).expect("run"); + assert!( + output.status.success(), + "STDERR: {}", + String::from_utf8_lossy(&output.stderr) + ); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + stderr.contains("W0001"), + "expected W0001 in stderr: {}", + stderr + ); +} From 12f15836c2d28a4f2c5fae5774716381a97d81e3 Mon Sep 17 00:00:00 2001 From: Pepe Navarro Date: Thu, 4 Jun 2026 17:07:09 +0200 Subject: [PATCH 26/27] refactor(ownership,pipeline): apply /simplify F1 + S3 F1: finalize_diags now reads sink.has_errors() before draining instead of re-scanning the returned Vec. The sink already maintains an O(1) error counter; the post-drain scan was a redundant linear pass. S3: Replace the documentation-only CompoundAssign no-op arm with an enforcing debug_assert! over the operand type. The previous arm pattern-matched the tag and did nothing; if a future sema relaxation lets a Move-typed compound-assign reach the ownership pass, the assertion fires in debug builds instead of silently falling through to no analysis. File new follow-ups in ISSUES.md: I-054 (parse_source + lex paths still hand-roll the render+wrap pattern that finalize_diags centralizes), I-055 (pending_dead_store.insert duplicated between analyze_var_decl and analyze_assign). --- ISSUES.md | 10 ++++++++++ src/ownership.rs | 18 ++++++++++++------ src/pipeline.rs | 2 +- 3 files changed, 23 insertions(+), 7 deletions(-) diff --git a/ISSUES.md b/ISSUES.md index 1fb0bd3..26f1411 100644 --- a/ISSUES.md +++ b/ISSUES.md @@ -241,6 +241,16 @@ Resolved entries are removed (not kept around as a changelog). Look at `git log` **Summary:** After their distinct preludes (visit `cond` vs visit `start` + `end`), the bodies are byte-for-byte the same — entry snapshot, scratch-sink walk, `states_differ` check, optional re-walk, final `merge_two`. Two near-clones of a non-trivial fixed-point loop is exactly where divergence creeps in (only one gets a fix when behavior needs to change). **Resolution:** Extract `analyze_loop_body(tir, pool, own, sink, by_name, body: &[TirRef])` after the caller has visited the loop's prelude. Folds with I-045 (propagate-only + check pass) — both refactors touch the same bodies. +### I-054 — `parse_source` and lex error paths bypass `finalize_diags` +**Files:** `src/pipeline.rs` (`parse_source`, `display_tokens`) +**Summary:** `finalize_diags` consolidates the drain + render + `Err(CompilerError::Diagnostics(_))` shape for the sink-using stages (`lower_and_analyze`, `ir_command`). The lex error paths and `parse_source`'s parse-error branch still hand-roll the same pattern over a `Vec` they build directly (no `DiagSink`). Drift risk is real: parse/lex paths skip the `Severity::Error` filter (they assume every diag they emit is an error, which holds today but isn't enforced), and any future change to the rendering convention has to be applied in three places. +**Resolution:** Generalize `finalize_diags` to take `Vec` (or `impl IntoIterator`); have `DiagSink::into_diags()` feed the new entry point. Then the three lex/parse error paths become `finalize_diags(vec![diag], input, &name)` and the render+wrap pattern lives in exactly one place. Folds naturally with I-014's lexer-DiagSink migration. + +### I-055 — `pending_dead_store.insert` duplicated between VarDecl and Assign +**Files:** `src/ownership.rs` (`analyze_var_decl`, `analyze_assign`) +**Summary:** Both helpers register a Move-typed binding into `own.pending_dead_store` with `(name, span)` after `rebind_to_init`. The pattern was a single site before commit 20cdd02 added W0001 reassignment coverage; it is now a 2-site duplication. If W0001 ever needs different policy at one site (e.g., distinguishing fresh declaration from reassignment), the duplication will silently allow it. +**Resolution:** Extract `register_pending_dead_store(own, owner, name, span)`, or fold the registration into `rebind_to_init` (which already takes the binding) and have it accept the span/name pair. Either is a 3-line change. + ### I-053 — `OwnerState::Borrowed` is currently parameter-only **Files:** `src/ownership.rs` (`OwnerState`) **Summary:** `OwnerState::Borrowed` is set only at parameter init; no expression produces it. The two sites that read it (E0021, E0022) could equivalently look up `tir.params` for the underlying owner's source param and check `is_move`. The state is anticipating real borrow expressions (`&x`) which the spec migrated to in commit 2ccf6b6 but the compiler doesn't lower yet. diff --git a/src/ownership.rs b/src/ownership.rs index 7736c4a..f3d8f54 100644 --- a/src/ownership.rs +++ b/src/ownership.rs @@ -273,12 +273,18 @@ fn analyze_stmt( // 8.1c attaches Free metadata here; 8.1b is a no-op. } TirTag::CompoundAssign => { - // Compound-assign on Move-typed values is rejected by sema today - // (str doesn't support `+=`/`-=`/etc.), so this is unreachable for - // tracked types. Explicit no-op arm so the next reader doesn't - // wonder whether the fall-through to visit_expr is correct. - // Revisit when compound-assign on Move-typed values becomes - // representable. See ISSUES.md I-046. + // Sema rejects compound-assign on Move-typed values today + // (str doesn't support `+=`/`-=`/etc.). Enforce the invariant + // here so a future sema relaxation that reaches the ownership + // pass without ownership-aware handling trips a debug build + // instead of silently falling through to no analysis. + // See ISSUES.md I-046. + let view = tir.compound_assign_view(stmt); + debug_assert!( + !needs_tracking(tir.inst(view.value).ty, pool), + "compound-assign on Move-typed value reached ownership pass; \ + sema should have rejected — see ISSUES.md I-046", + ); } TirTag::ExprStmt => { if let TirData::UnOp(o) = inst.data { diff --git a/src/pipeline.rs b/src/pipeline.rs index f96af64..b937e28 100644 --- a/src/pipeline.rs +++ b/src/pipeline.rs @@ -374,8 +374,8 @@ impl EmitSet { /// diagnostic has `Severity::Error`; warnings/notes alone do not /// fail the build. fn finalize_diags(sink: DiagSink, input: &str, source_name: &str) -> Result<(), CompilerError> { + let has_errors = sink.has_errors(); let diags = sink.into_diags(); - let has_errors = diags.iter().any(|d| d.severity == Severity::Error); if !diags.is_empty() { render_diags(&diags, input, source_name); } From 269fb8e949af6e4e7d07f927012b8575dcd12f8f Mon Sep 17 00:00:00 2001 From: Pepe Navarro Date: Thu, 4 Jun 2026 19:42:55 +0200 Subject: [PATCH 27/27] fix(ownership,astgen): address PR review findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - ISSUES.md: reorder so I-053 lands before I-054/I-055; the prior placement broke the monotonic-id convention. No external references to update. - astgen.rs: UirParam.span now uses p.span (the full parsed parameter span including a `move` annotation) instead of p.name.span, which dropped the `move` keyword from downstream diagnostics. - ownership.rs: synthetic_param_ref guards against the edge case name.raw() == u32::MAX, which would have produced a zero raw and panicked NonZeroU32. Switch to saturating_sub + .max(1). - ownership.rs: rephrase the analyze_while_loop comment from "converges in at most 2 iterations" (misleading — the impl is a 2-pass approximation, not a true fixed-point loop) to spell out *why* two passes suffice for the M8.1 pattern set (monotonic Moved-ness sub-lattice, no iteration-count-dependent control flow) and what would force a revert to a real fixed-point loop. - integration_tests.rs: test_use_after_move_in_vardecl_one_diag and test_use_after_move_in_return_one_diag now also assert output.status.success() == false; the previous assertions passed even if E0020 had become a warning. --- ISSUES.md | 10 ++++----- src/astgen.rs | 2 +- src/ownership.rs | 43 +++++++++++++++++++++++++++----------- tests/integration_tests.rs | 10 +++++++++ 4 files changed, 47 insertions(+), 18 deletions(-) diff --git a/ISSUES.md b/ISSUES.md index 26f1411..caf3228 100644 --- a/ISSUES.md +++ b/ISSUES.md @@ -241,6 +241,11 @@ Resolved entries are removed (not kept around as a changelog). Look at `git log` **Summary:** After their distinct preludes (visit `cond` vs visit `start` + `end`), the bodies are byte-for-byte the same — entry snapshot, scratch-sink walk, `states_differ` check, optional re-walk, final `merge_two`. Two near-clones of a non-trivial fixed-point loop is exactly where divergence creeps in (only one gets a fix when behavior needs to change). **Resolution:** Extract `analyze_loop_body(tir, pool, own, sink, by_name, body: &[TirRef])` after the caller has visited the loop's prelude. Folds with I-045 (propagate-only + check pass) — both refactors touch the same bodies. +### I-053 — `OwnerState::Borrowed` is currently parameter-only +**Files:** `src/ownership.rs` (`OwnerState`) +**Summary:** `OwnerState::Borrowed` is set only at parameter init; no expression produces it. The two sites that read it (E0021, E0022) could equivalently look up `tir.params` for the underlying owner's source param and check `is_move`. The state is anticipating real borrow expressions (`&x`) which the spec migrated to in commit 2ccf6b6 but the compiler doesn't lower yet. +**Resolution:** Document the invariant inline ("only ever set at param init in M8.1b; transitions arrive when `&x` borrow expressions land in a future milestone"). No code change today. + ### I-054 — `parse_source` and lex error paths bypass `finalize_diags` **Files:** `src/pipeline.rs` (`parse_source`, `display_tokens`) **Summary:** `finalize_diags` consolidates the drain + render + `Err(CompilerError::Diagnostics(_))` shape for the sink-using stages (`lower_and_analyze`, `ir_command`). The lex error paths and `parse_source`'s parse-error branch still hand-roll the same pattern over a `Vec` they build directly (no `DiagSink`). Drift risk is real: parse/lex paths skip the `Severity::Error` filter (they assume every diag they emit is an error, which holds today but isn't enforced), and any future change to the rendering convention has to be applied in three places. @@ -251,11 +256,6 @@ Resolved entries are removed (not kept around as a changelog). Look at `git log` **Summary:** Both helpers register a Move-typed binding into `own.pending_dead_store` with `(name, span)` after `rebind_to_init`. The pattern was a single site before commit 20cdd02 added W0001 reassignment coverage; it is now a 2-site duplication. If W0001 ever needs different policy at one site (e.g., distinguishing fresh declaration from reassignment), the duplication will silently allow it. **Resolution:** Extract `register_pending_dead_store(own, owner, name, span)`, or fold the registration into `rebind_to_init` (which already takes the binding) and have it accept the span/name pair. Either is a 3-line change. -### I-053 — `OwnerState::Borrowed` is currently parameter-only -**Files:** `src/ownership.rs` (`OwnerState`) -**Summary:** `OwnerState::Borrowed` is set only at parameter init; no expression produces it. The two sites that read it (E0021, E0022) could equivalently look up `tir.params` for the underlying owner's source param and check `is_move`. The state is anticipating real borrow expressions (`&x`) which the spec migrated to in commit 2ccf6b6 but the compiler doesn't lower yet. -**Resolution:** Document the invariant inline ("only ever set at param init in M8.1b; transitions arrive when `&x` borrow expressions land in a future milestone"). No code change today. - --- ## Cross-References diff --git a/src/astgen.rs b/src/astgen.rs index bad3435..cf81441 100644 --- a/src/astgen.rs +++ b/src/astgen.rs @@ -191,7 +191,7 @@ fn gen_function_def( sink, ), is_move: p.is_move, - span: p.name.span, + span: p.span, }) .collect(); diff --git a/src/ownership.rs b/src/ownership.rs index f3d8f54..1fab524 100644 --- a/src/ownership.rs +++ b/src/ownership.rs @@ -245,10 +245,14 @@ fn analyze_function( /// Build a stable synthetic [`TirRef`] for a parameter so it can /// share the `states` map with real instruction refs. Encoded near /// `u32::MAX` to keep it well clear of real per-function indices -/// (which start at 1 and grow with body size). `u32::MAX - name.raw()` -/// stays non-zero for any plausible interned-string id. +/// (which start at 1 and grow with body size). `TirRef` wraps +/// `NonZeroU32`, so we clamp the lower bound to 1 to defend against +/// the edge case `name.raw() == u32::MAX` (which would otherwise +/// panic at `from_raw`). The collision-free property relies on real +/// instruction refs never reaching `u32::MAX - max_string_id`, which +/// is structurally impossible for any realistic program. fn synthetic_param_ref(name: StringId) -> TirRef { - let raw = u32::MAX - name.raw(); + let raw = u32::MAX.saturating_sub(name.raw()).max(1); TirRef::from_raw(raw) } @@ -593,15 +597,30 @@ fn analyze_if_stmt( own.merge_branches(&refs); } -/// Fixed-point ownership analysis for `while`. Walks the body once -/// from the entry state into a scratch sink; if no tracked TirRef -/// changed Moved-ness across the back-edge, the body is loop-invariant -/// for ownership purposes and the scratch diagnostics are flushed. -/// Otherwise we re-walk from the merged (entry ⊔ post-body) state and -/// emit diagnostics on the second pass — a binding moved inside the -/// body without rebinding before the back-edge surfaces as E0020 on -/// iteration two. Converges in at most 2 iterations for the M8.1 -/// pattern set (per the design doc). +/// 2-pass approximation of a fixed-point ownership analysis for +/// `while`. Walks the body once from the entry state into a scratch +/// sink; if no tracked TirRef changed Moved-ness across the back-edge, +/// the body is loop-invariant for ownership purposes and the scratch +/// diagnostics are flushed. Otherwise we re-walk from the merged +/// (entry ⊔ post-body) state and emit diagnostics on the second pass +/// — a binding moved inside the body without rebinding before the +/// back-edge surfaces as E0020 on iteration two. +/// +/// Why two passes suffice for the M8.1 pattern set (instead of a +/// general fixed-point loop): the merge is monotonic over the +/// Moved-ness sub-lattice (`Valid → Moved` is the only transition, +/// and merge takes "any branch Moved → Moved"), and there are no +/// iteration-count-dependent conditionals — so a TirRef that flips +/// from `Valid` to `Moved` in pass one stays `Moved` after the +/// (entry ⊔ post-body) merge and a third walk would observe nothing +/// new. Original phrasing for traceability: +/// "Converges in at most 2 iterations for the M8.1 pattern set". +/// +/// **Maintainer note.** If a future change introduces non-monotonic +/// merges (e.g., `Moved → Valid` re-entry as a real lattice transition +/// rather than via rebinding) or iteration-count-dependent control +/// flow inside the body, revert this to a true fixed-point loop +/// (iterate until `states_differ` returns false). fn analyze_while_loop( tir: &Tir, pool: &InternPool, diff --git a/tests/integration_tests.rs b/tests/integration_tests.rs index 45c7fa9..2feb505 100644 --- a/tests/integration_tests.rs +++ b/tests/integration_tests.rs @@ -2459,6 +2459,11 @@ fn main(): "#; let test_file = create_test_file(temp_dir.path(), "uam_vardecl_one.ryo", code); let output = run_ryo_command(&["run", "uam_vardecl_one.ryo"], &test_file).expect("run"); + assert!( + !output.status.success(), + "expected compile error, STDERR: {}", + String::from_utf8_lossy(&output.stderr) + ); let stderr = String::from_utf8_lossy(&output.stderr); let count = stderr.matches("E0020").count(); assert_eq!( @@ -2485,6 +2490,11 @@ fn main(): "#; let test_file = create_test_file(temp_dir.path(), "uam_ret_one.ryo", code); let output = run_ryo_command(&["run", "uam_ret_one.ryo"], &test_file).expect("run"); + assert!( + !output.status.success(), + "expected compile error, STDERR: {}", + String::from_utf8_lossy(&output.stderr) + ); let stderr = String::from_utf8_lossy(&output.stderr); let count = stderr.matches("E0020").count(); assert_eq!(