Merge pull request #230 from nikomatsakis/explicit-end-lease-refactors

Refactors from (failed) explicit end lease branch

book/docs/design_docs/why_not_explicit_end_for_lease.md

@@ -0,0 +1,64 @@
+# Why not have an explicit end for lease?
+
+For a while I attempted to have an **explicit end** for every lease
+rather than adopting the "stacked borrows"-like approach in which
+leases are ended by their owner taking contrary action.
+
+In many ways, explicit ends of leases is a cleaner approach.
+The difference is where the error occurs:
+
+```
+class Counter(counter)
+
+let x = Counter(0)
+
+// Create a lease of x
+let y = x.lease
+
+// In the "explicit end" version, this is an error.
+// In the "permissive" variant, it cancels the lease.
+x.counter = 1
+
+// In the "permissive" variant, error occurs here.
+print(y.counter)
+```
+
+It's hard to say which of these statements is wrong.
+Deferring the error until something has definitively gone wrong
+is more permissive and hence the required approach for Rust's unsafe code
+(which wants to be as backwards compatible with common practice as possible).
+Dada's untyped mode is analogous to unsafe code, so that's plausibly a good choice here,
+but I wanted to try the alternative.
+
+## Where it goes wrong
+
+To make things usable, you don't want to _EXPLICITLY_ end leases,
+so we want to have some kind of _drop_ that is auto-inserted.
+I imagined we would do this based on liveness information.
+But that has a flaw: when you compute liveness, you see direct
+uses of a variable, but not indirect. Consider this (correct) code:
+
+```
+class Counter(counter)
+let x = Counter(0)
+let y = x.lease // <-- last (direct) use of `x`
+print(y.counter)
+```
+
+Liveness-based analysis would drop `x` immediately after the lease,
+since it has no more direct uses. But that's not what we want.
+
+We could in principle say that when something is dropped,
+it remains in scope until the leases end,
+but that's basically GC.
+
+## Doing better requires type data
+
+We could do better if we took types into account, but gradual typing implies they may not be available.
+Besides, that's getting pretty complex.
+
+## Implications for Rust
+
+I would like to have liveness-based drops for Rust, but this reveals the (obvious in hindsight) flaw:
+we never know when raw pointers are out there. So unless we know that, or declare it UB in some way,
+we can't promote drops earlier.

book/sidebars.js

@@ -63,6 +63,7 @@ const sidebars = {
         "design_docs/calculus",
         "design_docs/sketchy_ideas",
         "design_docs/sharing_synthesized_values",
+        "design_docs/why_not_explicit_end_for_lease",
         "reference",
       ],
     },

components/dada-brew/src/brew.rs

@@ -1,19 +1,17 @@
 use dada_id::prelude::*;
-use dada_ir::{
-    code::{
-        bir::{self, BirData},
-        validated::{self, ExprOrigin},
-    },
-    storage::Atomic,
+use dada_ir::code::{
+    bir::{self, BirData},
+    validated::{self, ExprOrigin},
 };
 use dada_parse::prelude::*;
 use salsa::DebugWithDb;
 
 use crate::{
-    brewery::{Brewery, LoopContext},
-    cursor::Cursor,
+    brewery::Brewery,
+    scope::{LoopContext, Scope, ScopeCause},
 };
 
+/// Convert a [`validated::Tree`] to [BIR](`bir::Bir`).
 #[salsa::tracked]
 pub fn brew(db: &dyn crate::Db, validated_tree: validated::Tree) -> bir::Bir {
     let function = validated_tree.function(db);
@@ -34,18 +32,20 @@ pub fn brew(db: &dyn crate::Db, validated_tree: validated::Tree) -> bir::Bir {
     );
     let num_parameters = validated_tree.data(db).num_parameters;
 
-    // Compile the root expression and -- assuming it doesn't diverse --
+    // Compile the root expression and -- assuming it doesn't diverge --
     // return the resulting value.
     let root_expr = validated_tree.data(db).root_expr;
     let root_expr_origin = validated_tree.origins(db)[root_expr];
-    let mut cursor = Cursor::new(brewery, root_expr_origin);
+    let mut cursor = Scope::root(brewery, root_expr_origin);
+    let marker = cursor.mark_variables();
     if let Some(place) = cursor.brew_expr_to_temporary(brewery, root_expr) {
         cursor.terminate_and_diverge(
             brewery,
             bir::TerminatorData::Return(place),
             root_expr_origin,
         );
     }
+    cursor.clear_variables_since_marker(marker, brewery, root_expr_origin);
     let start_basic_block = cursor.complete();
 
     let bir = bir::Bir::new(
@@ -66,7 +66,7 @@ pub fn brew(db: &dyn crate::Db, validated_tree: validated::Tree) -> bir::Bir {
     bir
 }
 
-impl Cursor {
+impl Scope<'_> {
     #[tracing::instrument(level = "debug", skip_all)]
     pub(crate) fn brew_expr_for_side_effects(
         &mut self,
@@ -75,23 +75,25 @@ impl Cursor {
     ) {
         tracing::debug!("expr = {:?}", expr.data(brewery.validated_tables()));
         let origin = brewery.origin(expr);
-        let temporary_scope = self.push_temporary_scope(brewery);
+        let temporary_marker = self.mark_variables();
         match expr.data(brewery.validated_tables()) {
             validated::ExprData::Break {
                 from_expr,
                 with_value,
             } => {
                 self.push_breakpoint_start(brewery, origin);
-                let loop_context = brewery.loop_context(*from_expr);
+                let (loop_context, variables) = self.loop_context(*from_expr);
                 self.brew_expr_and_assign_to(brewery, loop_context.loop_value, *with_value);
                 self.push_breakpoint_end(brewery, Some(loop_context.loop_value), origin);
+                self.push_clear_variables(brewery, &variables, origin);
                 self.terminate_and_goto(brewery, loop_context.break_block, origin);
             }
 
             validated::ExprData::Continue(from_expr) => {
                 self.push_breakpoint_start(brewery, origin);
-                let loop_context = brewery.loop_context(*from_expr);
+                let (loop_context, variables) = self.loop_context(*from_expr);
                 self.push_breakpoint_end(brewery, None::<bir::Place>, origin);
+                self.push_clear_variables(brewery, &variables, origin);
                 self.terminate_and_goto(brewery, loop_context.continue_block, origin);
             }
 
@@ -123,8 +125,9 @@ impl Cursor {
 
             validated::ExprData::Declare(vars, subexpr) => {
                 self.push_breakpoint_start(brewery, origin);
+                let variables_marker = self.push_declared_variables(vars, brewery);
                 self.brew_expr_for_side_effects(brewery, *subexpr);
-                self.pop_declared_variables(brewery, vars, origin);
+                self.clear_variables_since_marker(variables_marker, brewery, origin);
                 self.push_breakpoint_end(brewery, None::<bir::Place>, origin);
             }
 
@@ -151,7 +154,7 @@ impl Cursor {
                 let _ = self.brew_expr_to_temporary(brewery, expr);
             }
         }
-        self.pop_temporary_scope(brewery, temporary_scope);
+        self.clear_variables_since_marker(temporary_marker, brewery, origin);
     }
 
     /// Compiles expr into a temporary `t` and returns `Some(t)`.
@@ -163,7 +166,7 @@ impl Cursor {
     ) -> Option<bir::Place> {
         let origin = brewery.origin(expr);
         // Spill into a temporary
-        let temp_place = add_temporary_place(brewery, origin);
+        let temp_place = self.add_temporary(brewery, origin);
         self.brew_expr_and_assign_to(brewery, temp_place, expr);
         Some(brewery.place_from_target_place(temp_place))
     }
@@ -179,7 +182,7 @@ impl Cursor {
         expr: validated::Expr,
     ) {
         let origin = brewery.origin(expr);
-        let temporary_scope = self.push_temporary_scope(brewery);
+        let temporary_marker = self.mark_variables();
         match expr.data(brewery.validated_tables()) {
             validated::ExprData::Await(future) => {
                 self.push_breakpoint_start(brewery, origin);
@@ -211,11 +214,12 @@ impl Cursor {
                     );
                     self.push_breakpoint_end(brewery, Some(target), origin); // "cusp" of an if is after it completes
 
-                    let mut if_true_cursor = self.with_end_block(if_true_block);
+                    let mut if_true_cursor = self.subscope(Some(if_true_block), ScopeCause::Branch);
                     if_true_cursor.brew_expr_and_assign_to(brewery, target, *if_true);
                     if_true_cursor.terminate_and_goto(brewery, join_block, origin);
 
-                    let mut if_false_cursor = self.with_end_block(if_false_block);
+                    let mut if_false_cursor =
+                        self.subscope(Some(if_false_block), ScopeCause::Branch);
                     if_false_cursor.brew_expr_and_assign_to(brewery, target, *if_false);
                     if_false_cursor.terminate_and_goto(brewery, join_block, origin);
                 }
@@ -231,19 +235,18 @@ impl Cursor {
                 );
                 self.push_breakpoint_end(brewery, Some(target), origin); // "cusp" of a loop is after it breaks
 
-                let body_brewery = &mut brewery.subbrewery();
-                body_brewery.push_loop_context(
-                    expr,
-                    LoopContext {
+                let mut body_cursor = self.subscope(
+                    Some(body_block),
+                    ScopeCause::Loop(LoopContext {
                         continue_block: body_block,
                         break_block,
                         loop_value: target,
-                    },
+                        expr,
+                    }),
                 );
-                let mut body_cursor = self.with_end_block(body_block);
-                body_cursor.brew_expr_for_side_effects(body_brewery, *body);
+                body_cursor.brew_expr_for_side_effects(brewery, *body);
                 body_cursor.terminate_and_diverge(
-                    body_brewery,
+                    brewery,
                     bir::TerminatorData::Goto(body_block),
                     origin,
                 );
@@ -462,8 +465,9 @@ impl Cursor {
 
             validated::ExprData::Declare(vars, subexpr) => {
                 self.push_breakpoint_start(brewery, origin);
+                let variables_marker = self.push_declared_variables(vars, brewery);
                 self.brew_expr_and_assign_to(brewery, target, *subexpr);
-                self.pop_declared_variables(brewery, vars, origin);
+                self.clear_variables_since_marker(variables_marker, brewery, origin);
                 self.push_breakpoint_end(brewery, None::<bir::Place>, origin);
             }
 
@@ -474,7 +478,7 @@ impl Cursor {
                 self.brew_expr_for_side_effects(brewery, expr);
             }
         };
-        self.pop_temporary_scope(brewery, temporary_scope);
+        self.clear_variables_since_marker(temporary_marker, brewery, origin);
     }
 
     /// Brews a place to a bir place, returning a vector of the
@@ -550,21 +554,3 @@ impl Cursor {
         brewery.add(bir::TargetPlaceData::LocalVariable(bir_var), origin)
     }
 }
-
-fn add_temporary(brewery: &mut Brewery, origin: ExprOrigin) -> bir::LocalVariable {
-    let temporary = brewery.add(
-        bir::LocalVariableData {
-            name: None,
-            atomic: Atomic::No,
-        },
-        validated::LocalVariableOrigin::Temporary(origin.into()),
-    );
-    tracing::debug!("created temporary: temp{{{:?}}}", u32::from(temporary));
-    brewery.push_temporary(temporary);
-    temporary
-}
-
-fn add_temporary_place(brewery: &mut Brewery, origin: ExprOrigin) -> bir::TargetPlace {
-    let temporary_var = add_temporary(brewery, origin);
-    brewery.add(bir::TargetPlaceData::LocalVariable(temporary_var), origin)
-}