feat(compiler): a release through a borrow is reported

A borrowing function's caller keeps its claim: it may read the storage
afterwards, and something else is still going to release it. A function
that releases such a parameter breaks both, and the symptom is a read of
freed memory or a second release. On a tree the second release walks
memory that has been handed back and recurses on whatever is in it now,
which arrives as a stack overflow nowhere near the cause.

The check reports a parameter that is borrowed and released, whether by
the free intrinsic, by a runtime symbol that allocates, or by being
handed to a callee that takes it owned. It reuses the drop pass's own
walk over the values a pointer reaches, so releasing a cast or a field
pointer of the parameter counts the same, rather than growing a second
copy that would miss a different set of instructions.

Errors reached the developer through the struct's `Debug`, which names
fields and value ids and says nothing about what is wrong or what would
be right. They are sentences now.

`own self` also parses. The word required a type annotation and `self`
is spelled on its own, so the only way to say a method releases its
receiver was to write the receiver's type after it. `mut` gets the same
bare form. Both come after the annotated rules in the ordered choice, or
`own p: T` would match the name and leave the type behind.

Author: darmie

crates/compiler/src/borrow_check.rs

@@ -19,6 +19,7 @@ use crate::CompilerError;
 use crate::CompilerResult;
 use std::collections::{HashMap, HashSet};
 use zyntax_typed_ast::source::Span;
+use zyntax_typed_ast::InternedString;
 
 /// Result of borrow checking
 #[derive(Debug)]
@@ -70,6 +71,63 @@ pub enum BorrowError {
         borrow: HirId,
         location: Option<Span>,
     },
+    /// A parameter the function only borrows is released by it.
+    ///
+    /// Releasing storage ends the claim on it, and a borrowed parameter
+    /// leaves that claim with the caller. So a function that releases
+    /// one has released something it does not own: the caller still
+    /// believes it holds the storage and may read it, and whatever else
+    /// is responsible for releasing it will do so a second time.
+    ReleaseThroughBorrow {
+        function: InternedString,
+        parameter: InternedString,
+        location: Option<Span>,
+    },
+}
+
+/// What a borrow error says to the person who wrote the program.
+///
+/// Rendered rather than dumped: these reached the developer as the
+/// struct's `Debug`, which names fields and value ids and says nothing
+/// about what is wrong or what would be right.
+impl std::fmt::Display for BorrowError {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        match self {
+            BorrowError::UseAfterMove { .. } => {
+                write!(f, "a value is used after it was handed to something else")
+            }
+            BorrowError::MutableBorrowConflict { .. } => write!(
+                f,
+                "a value is borrowed for writing while it is already borrowed"
+            ),
+            BorrowError::ImmutableBorrowConflict { .. } => write!(
+                f,
+                "a value is borrowed for reading while it is borrowed for writing"
+            ),
+            BorrowError::ReferenceOutlivesReferent { .. } => {
+                write!(f, "a reference outlives what it refers to")
+            }
+            BorrowError::MutationThroughImmutableRef { .. } => {
+                write!(f, "a value is written through a reference that only reads")
+            }
+            BorrowError::MovedWhileBorrowed { .. } => {
+                write!(f, "a value is handed away while something still borrows it")
+            }
+            BorrowError::ReleaseThroughBorrow {
+                function,
+                parameter,
+                ..
+            } => write!(
+                f,
+                "`{}` releases `{}`, which it only borrows. The caller still \
+                 holds that storage and may read it, and whatever else is \
+                 responsible for it will release it a second time. A \
+                 parameter that a function releases has to be declared owned.",
+                function.resolve_global().unwrap_or_default(),
+                parameter.resolve_global().unwrap_or_default()
+            ),
+        }
+    }
 }
 
 /// Borrow checking warning
@@ -183,10 +241,95 @@ impl<'a> HirBorrowChecker<'a> {
             }
         }
 
+        self.check_releases_of_borrowed_params(func);
+
         self.function_contexts.insert(func_id, context);
         Ok(())
     }
 
+    /// A parameter the function only borrows must not be released by it.
+    ///
+    /// The caller of a borrowing function keeps its claim, so it may
+    /// read the storage afterwards and something else is still going to
+    /// release it. A function that releases a borrowed parameter breaks
+    /// both of those, and the symptom is a read of freed memory or a
+    /// second release, neither of which says where it came from.
+    ///
+    /// A parameter declared owned is exactly how a function says it
+    /// takes that responsibility, so this reports only the ones that
+    /// have not.
+    fn check_releases_of_borrowed_params(&mut self, func: &HirFunction) {
+        use crate::drop_insert::{derived_values, symbol_role, SymbolRole};
+        use crate::hir::{HirCallable, Intrinsic, ParamOwnership};
+
+        for (index, param) in func.signature.params.iter().enumerate() {
+            if !matches!(
+                param.ownership,
+                ParamOwnership::Borrowed | ParamOwnership::BorrowedMut
+            ) {
+                continue;
+            }
+            // The value the body refers to, which is not `param.id`:
+            // building the SSA form mints a fresh value for each
+            // parameter and the signature keeps its own. Rooting the
+            // walk at the signature's id finds nothing, and a check that
+            // finds nothing reads exactly like a check that passed.
+            let Some(root) = func.values.values().find_map(|v| {
+                matches!(v.kind, HirValueKind::Parameter(i) if i as usize == index).then_some(v.id)
+            }) else {
+                continue;
+            };
+            // Every name the parameter reaches, so releasing a cast or a
+            // field pointer of it counts the same as releasing it.
+            let reached = derived_values(func, root);
+            for block in func.blocks.values() {
+                for inst in &block.instructions {
+                    let HirInstruction::Call { callee, args, .. } = inst else {
+                        continue;
+                    };
+                    if !args.iter().any(|a| reached.contains(a)) {
+                        continue;
+                    }
+                    let releases = match callee {
+                        HirCallable::Intrinsic(Intrinsic::Free) => true,
+                        HirCallable::Symbol(name) => {
+                            matches!(symbol_role(name), Some(SymbolRole::Allocates(_)))
+                        }
+                        // A callee that takes the argument owned is
+                        // being handed the claim, which a borrower has
+                        // not got to give.
+                        HirCallable::Function(id) => self
+                            .module
+                            .functions
+                            .get(id)
+                            .map(|callee| {
+                                callee
+                                    .signature
+                                    .params
+                                    .iter()
+                                    .zip(args.iter())
+                                    .any(|(p, a)| {
+                                        reached.contains(a) && p.ownership == ParamOwnership::Owned
+                                    })
+                            })
+                            .unwrap_or(false),
+                        _ => false,
+                    };
+                    if releases {
+                        self.errors.push(BorrowError::ReleaseThroughBorrow {
+                            function: func.name,
+                            parameter: param.name,
+                            location: None,
+                        });
+                        // One report per parameter. A release inside a
+                        // loop is the same mistake said many times.
+                        break;
+                    }
+                }
+            }
+        }
+    }
+
     /// Get blocks in execution order (basic topological sort)
     fn get_block_order(&self, func: &HirFunction) -> Vec<HirId> {
         let mut order = Vec::new();
@@ -574,7 +717,7 @@ pub fn validate_borrow_check(result: &BorrowCheckResult) -> CompilerResult<()> {
     if result.errors.is_empty() {
         Ok(())
     } else {
-        let error_msgs: Vec<String> = result.errors.iter().map(|e| format!("{:?}", e)).collect();
+        let error_msgs: Vec<String> = result.errors.iter().map(|e| e.to_string()).collect();
         Err(CompilerError::Analysis(format!(
             "Borrow check failed with {} errors:\n{}",
             result.errors.len(),

crates/compiler/src/drop_insert.rs

@@ -487,7 +487,7 @@ fn run_function(func: &mut HirFunction, facts: &ModuleFacts) -> DropStats {
 /// that boxes in a loop allocates once per iteration and releases
 /// nothing.
 #[derive(Debug, Clone, Copy, PartialEq, Eq)]
-enum SymbolRole {
+pub(crate) enum SymbolRole {
     /// Returns storage the caller owns, released by the named symbol.
     Allocates(&'static str),
     /// Reads through a pointer argument without keeping it, so passing
@@ -497,7 +497,7 @@ enum SymbolRole {
 
 /// The role of a runtime symbol, or `None` where the pass knows nothing
 /// about it and must assume the worst.
-fn symbol_role(name: &str) -> Option<SymbolRole> {
+pub(crate) fn symbol_role(name: &str) -> Option<SymbolRole> {
     match name {
         "zyntax_box_bool" | "zyntax_box_f32" | "zyntax_box_f64" | "zyntax_box_i32"
         | "zyntax_box_i64" | "zyntax_box_opaque" => Some(SymbolRole::Allocates("zyntax_box_free")),
@@ -895,7 +895,7 @@ fn analyze_site(func: &HirFunction, site: &MallocSite, facts: &ModuleFacts) -> S
 /// Only the shapes that carry the same pointer are followed: putting it
 /// into an aggregate, taking it back out, and casting it. Following more
 /// would widen the live range without making anything reclaimable.
-fn derived_values(func: &HirFunction, root: HirId) -> std::collections::HashSet<HirId> {
+pub(crate) fn derived_values(func: &HirFunction, root: HirId) -> std::collections::HashSet<HirId> {
     let mut set = std::collections::HashSet::new();
     set.insert(root);
     // Blocks are unordered here, so a single sweep can miss a chain

crates/zynml/ml.zyn

@@ -2139,7 +2139,7 @@ fn_param_comma = { "," ~ param:fn_param }
 // Stating neither leaves the type to decide: a pointer is borrowed, and
 // anything held by value is copied.
 fn_param = {
-    param_owned | param_mut |
+    param_owned | param_owned_bare | param_mut | param_mut_bare |
     param_typed_default | param_default | param_typed | param_simple
 }
 
@@ -2157,6 +2157,24 @@ param_mut = { "mut" ~ name:identifier ~ ":" ~ ty:type_expr }
       ownership: ParamOwnership::BorrowedMut,
   }
 
+// The same two, for a parameter whose type is not written. A method's
+// receiver is the one that matters: `self` is spelled on its own and
+// takes its type from the impl, so requiring an annotation to say `own`
+// meant `own self` did not parse and the only spelling was
+// `own self: TheType`. Both come after the annotated forms, since an
+// ordered choice would otherwise take the name and leave the `:` behind.
+param_owned_bare = { "own" ~ name:identifier }
+  -> TypedParameter {
+      name: intern(name),
+      ownership: ParamOwnership::Owned,
+  }
+
+param_mut_bare = { "mut" ~ name:identifier }
+  -> TypedParameter {
+      name: intern(name),
+      ownership: ParamOwnership::BorrowedMut,
+  }
+
 
 // x: Type = default_value
 param_typed_default = { name:identifier ~ ":" ~ ty:type_expr ~ "=" ~ default:expr }

crates/zynml/tests/a_release_through_a_borrow_is_reported.rs

@@ -0,0 +1,139 @@
+//! Releasing a parameter the function only borrows is an error.
+//!
+//! A borrowing function's caller keeps its claim: it may read the
+//! storage afterwards, and something else is still going to release it.
+//! A function that releases such a parameter breaks both, and the
+//! symptom is a read of freed memory or a second release. Neither says
+//! where it came from, and on a tree the second release walks memory
+//! that has been handed back and recurses on whatever is now in it,
+//! which arrives as a stack overflow nowhere near the cause.
+//!
+//! Declaring the parameter owned is how a function says it takes that
+//! responsibility. This is what makes the difference between the two a
+//! message rather than a crash.
+
+use std::path::Path;
+use zynml::{ZynML, ZynMLConfig};
+
+fn load(src: &str) -> Result<(), String> {
+    let plugins = Path::new(env!("CARGO_MANIFEST_DIR")).join("../../plugins/target/zrtl");
+    let cfg = ZynMLConfig {
+        plugins_dir: plugins.to_string_lossy().to_string(),
+        // The profile that enforces ownership. The tiered profiles do
+        // not, so asking them would prove nothing either way.
+        runtime_profile: zynml::ZynMLRuntimeProfile::Classic,
+        ..ZynMLConfig::default()
+    };
+    let mut z = ZynML::with_config(cfg).map_err(|e| format!("{e:?}"))?;
+    z.load_source(src).map_err(|e| format!("{e:?}"))?;
+    Ok(())
+}
+
+const NODE: &str = r#"
+import prelude
+
+@reference
+struct Node { left: Node, right: Node, item: i64 }
+"#;
+
+/// A method that frees its receiver without saying it consumes it.
+#[test]
+fn a_method_that_frees_a_borrowed_receiver_is_reported() {
+    let err = load(&format!(
+        "{NODE}
+impl Node {{
+    def make(item: i64): Node {{ return Node {{ left: null, right: null, item: item }} }}
+    def dispose(self) {{ free(self as Ptr<i8>) }}
+}}
+
+def main(): i64 {{
+    let n: Node = Node::make(7)
+    n.dispose()
+    return 0
+}}"
+    ))
+    .expect_err("freeing a borrowed receiver should be refused");
+
+    assert!(
+        err.contains("dispose"),
+        "the report should name the function it is in, got: {err}"
+    );
+    assert!(
+        err.contains("only borrows") || err.contains("declared owned"),
+        "the report should say what is wrong and what would be right, got: {err}"
+    );
+}
+
+/// The same method, declared to consume its receiver, is accepted.
+///
+/// This is the half that makes the test above about `own` rather than
+/// about freeing.
+#[test]
+fn declaring_the_receiver_owned_is_accepted() {
+    load(&format!(
+        "{NODE}
+impl Node {{
+    def make(item: i64): Node {{ return Node {{ left: null, right: null, item: item }} }}
+    def dispose(own self: Node) {{ free(self as Ptr<i8>) }}
+}}
+
+def main(): i64 {{
+    let n: Node = Node::make(7)
+    n.dispose()
+    return 0
+}}"
+    ))
+    .expect("a receiver declared owned may be released");
+}
+
+/// A free function releasing a borrowed pointer is the same mistake.
+#[test]
+fn a_free_function_that_frees_a_borrowed_pointer_is_reported() {
+    let err = load(
+        r#"
+import prelude
+import simd
+
+def dispose(p: Ptr<i8>) { free(p) }
+
+def main(): i64 {
+    let p: Ptr<i8> = alloc_i8(16)
+    dispose(p)
+    return 0
+}
+"#,
+    )
+    .expect_err("freeing a borrowed pointer should be refused");
+    assert!(err.contains("dispose"), "should name the function: {err}");
+}
+
+/// And a function that merely reads through a borrowed pointer is not
+/// reported, so the rule is about releasing rather than about pointers.
+#[test]
+fn reading_through_a_borrow_is_not_a_release() {
+    load(
+        r#"
+import prelude
+import simd
+
+def total(p: Ptr<i8>, n: i64): i64 {
+    let q: Ptr<i64> = p as Ptr<i64>
+    let mut sum: i64 = 0
+    let mut i: i64 = 0
+    while i < n {
+        sum = sum + q[i]
+        i = i + 1
+    }
+    return sum
+}
+
+def main(): i64 {
+    let p: Ptr<i8> = alloc_i8(64)
+    let s: i64 = total(p, 4)
+    free(p)
+    return s
+}
+"#,
+    )
+    .expect("reading through a borrow is not releasing it");
+}