Repository: tweag/nickel
Pull request #2031 (2024-09-06 12:30:43)

Let rec patterns
Depends on #2010 

Allows `let rec` bindings to contain patterns. Also fixes a bug in lsp completions, where pattern bindings were accidentally already treated as recursive.

The destructuring is no longer desugared to a match block (which allows for slightly better diagnostics), but it is still as eager as a match block destructuring and it still uses the same pattern compilation under the hood. The motivations for destructuring eagerly,

- to share code with pattern compilation for match bindings,
- to have similar behavior in match blocks and let bindings, and
- to catch errors earlier (with lazy destructuring, `let 'Foo = 'Bar in true` won't error)

github-actions[bot]: <h1><a href="https://bencher.dev/"><img src="https://bencher.dev/favicon.svg" width="32" height="32" alt="🐰" /></a>Bencher</h1><table><tr><td>Report</td><td>Fri, September  6, 2024 at 12:40:14 UTC</td></tr><tr><td>Project</td><td><a href="https://bencher.dev/perf/nickel">nickel</a></td></tr><tr><td>Branch</td><td>2031/merge</td></tr><tr><td>Testbed</td><td>ubuntu-latest</td></tr></table><blockquote><p><b>⚠️ WARNING:</b> The following Measure does not have a Threshold. Without a Threshold, no Alerts will ever be generated!</p><ul><li>Latency (latency)</li></ul><p><a href="https://bencher.dev/console/projects/nickel/thresholds/add">Click here to create a new Threshold</a><br />For more information, see <a href="https://bencher.dev/docs/explanation/thresholds/">the Threshold documentation</a>.<br />To only post results if a Threshold exists, set <a href="https://bencher.dev/docs/explanation/bencher-run/#--ci-only-thresholds">the <code lang="rust">--ci-only-thresholds</code> CLI flag</a>.</p></blockquote><details><summary>Click to view all benchmark results</summary><table><tr><th>Benchmark</th><th>Latency</th><th>Latency Results<br/>nanoseconds (ns)</th></tr><tr><td>fibonacci 10</td><td>➖ (<a href="https://bencher.dev/perf/nickel?branches=1caf1aa0-c1b2-44fa-9849-f2ec24e84a06&testbeds=66263ed0-9556-4385-9f30-26466ee26211&benchmarks=3cfaf19e-c9c5-4341-95cd-703456d0fc9f&measures=2b5b9371-696f-422d-b1a5-262ce8157b6b&start_time=1723034414000&end_time=1725626414000">view plot</a>)</td><td>490,610.00</td></tr><tr><td>pidigits 100</td><td>➖ (<a href="https://bencher.dev/perf/nickel?branches=1caf1aa0-c1b2-44fa-9849-f2ec24e84a06&testbeds=66263ed0-9556-4385-9f30-26466ee26211&benchmarks=5be25c57-d46f-4b48-b6f0-dee01b84220a&measures=2b5b9371-696f-422d-b1a5-262ce8157b6b&start_time=1723034414000&end_time=1725626414000">view plot</a>)</td><td>3,161,300.00</td></tr><tr><td>product 30</td><td>➖ (<a href="https://bencher.dev/perf/nickel?branches=1caf1aa0-c1b2-44fa-9849-f2ec24e84a06&testbeds=66263ed0-9556-4385-9f30-26466ee26211&benchmarks=2f5f7f6a-4e54-4542-aa09-da5671953fbb&measures=2b5b9371-696f-422d-b1a5-262ce8157b6b&start_time=1723034414000&end_time=1725626414000">view plot</a>)</td><td>802,470.00</td></tr><tr><td>scalar 10</td><td>➖ (<a href="https://bencher.dev/perf/nickel?branches=1caf1aa0-c1b2-44fa-9849-f2ec24e84a06&testbeds=66263ed0-9556-4385-9f30-26466ee26211&benchmarks=211bb66a-58f8-488b-97e4-7ea032870038&measures=2b5b9371-696f-422d-b1a5-262ce8157b6b&start_time=1723034414000&end_time=1725626414000">view plot</a>)</td><td>1,477,900.00</td></tr><tr><td>sum 30</td><td>➖ (<a href="https://bencher.dev/perf/nickel?branches=1caf1aa0-c1b2-44fa-9849-f2ec24e84a06&testbeds=66263ed0-9556-4385-9f30-26466ee26211&benchmarks=3f1710c5-ce75-4dfb-aadb-1435fed8a093&measures=2b5b9371-696f-422d-b1a5-262ce8157b6b&start_time=1723034414000&end_time=1725626414000">view plot</a>)</td><td>793,270.00</td></tr></table></details><br/><small><a href="https://bencher.dev">Bencher - Continuous Benchmarking</a></small><br/><small><a href="https://bencher.dev/perf/nickel">View Public Perf Page</a></small><br/><small><a href="https://bencher.dev/docs/">Docs</a> | <a href="https://bencher.dev/repo/">Repo</a> | <a href="https://bencher.dev/chat/">Chat</a> | <a href="https://bencher.dev/help/">Help</a></small><div id="bencher.dev/projects/nickel/id/2031-merge/ubuntu-latest/rust_criterion"></div>
yannham: @jneem once it's rebased could please tag me, and I'll take a look!
jneem: @yannham it's ready for a look

Patch:
diff --git a/core/src/error/mod.rs b/core/src/error/mod.rs
index dcb2d122..27a26740 100644
--- a/core/src/error/mod.rs
+++ b/core/src/error/mod.rs
@@ -27,7 +27,7 @@ use crate::{
     position::{RawSpan, TermPos},
     repl,
     serialize::{ExportFormat, NickelPointer},
-    term::{record::FieldMetadata, Number, RichTerm, Term},
+    term::{pattern::Pattern, record::FieldMetadata, Number, RichTerm, Term},
     typ::{EnumRow, RecordRow, Type, TypeF, VarKindDiscriminant},
 };
 
@@ -168,6 +168,12 @@ pub enum EvalError {
         /// The position of the `match` expression
         pos: TermPos,
     },
+    FailedDestructuring {
+        /// The original term matched.
+        value: RichTerm,
+        /// The pattern that failed to match.
+        pattern: Pattern,
+    },
     /// Tried to query a field of something that wasn't a record.
     QueryNonRecord {
         /// Position of the original unevaluated expression.
@@ -1468,6 +1474,20 @@ impl IntoDiagnostics<FileId> for EvalError {
                     .with_message("unmatched pattern")
                     .with_labels(labels)]
             }
+            EvalError::FailedDestructuring { value, pattern } => {
+                let mut labels = Vec::new();
+
+                if let Some(span) = pattern.pos.into_opt() {
+                    labels.push(primary(&span).with_message("this pattern"));
+                }
+
+                labels
+                    .push(secondary_term(&value, files).with_message("this value failed to match"));
+
+                vec![Diagnostic::error()
+                    .with_message("destructuring failed")
+                    .with_labels(labels)]
+            }
             EvalError::IllegalPolymorphicTailAccess {
                 action,
                 label: contract_label,
diff --git a/core/src/parser/grammar.lalrpop b/core/src/parser/grammar.lalrpop
index 366fe7bd..991673ec 100644
--- a/core/src/parser/grammar.lalrpop
+++ b/core/src/parser/grammar.lalrpop
@@ -232,11 +232,11 @@ UniTerm: UniTerm = {
     InfixExpr,
     AnnotatedInfixExpr,
     AsUniTerm<Forall>,
-    "let" <l: @L> <recursive:"rec"?> <r: @R>
+    "let" <recursive:"rec"?>
         <mut bindings:(<LetBinding> ",")*> <last:LetBinding> ","?
         "in" <body: Term> =>? {
         bindings.push(last);
-        Ok(UniTerm::from(mk_let(recursive.is_some(), bindings, body, mk_span(src_id, l, r))?))
+        Ok(UniTerm::from(mk_let(recursive.is_some(), bindings, body)?))
     },
     <l: @L> "fun" <pats: PatternFun+> "=>" <t: Term> <r: @R> => {
         let pos = mk_pos(src_id, l, r);
diff --git a/core/src/parser/utils.rs b/core/src/parser/utils.rs
index f5040dec..4ac5b0ca 100644
--- a/core/src/parser/utils.rs
+++ b/core/src/parser/utils.rs
@@ -630,14 +630,11 @@ pub fn mk_merge_label(src_id: FileId, l: usize, r: usize) -> MergeLabel {
 }
 
 /// Generate a `Let` or a `LetPattern` (depending on whether there's a binding
-/// with a record pattern) from the parsing of a let definition. This function
-/// fails if the definition has both a pattern and is recursive because
-/// recursive let-patterns are currently not supported.
+/// with a record pattern) from the parsing of a let definition.
 pub fn mk_let(
     rec: bool,
     bindings: Vec<LetBinding>,
     body: RichTerm,
-    span: RawSpan,
 ) -> Result<RichTerm, ParseError> {
     let all_simple = bindings
         .iter()
@@ -678,10 +675,9 @@ pub fn mk_let(
             }),
             body,
         ))
-    } else if rec {
-        Err(ParseError::RecursiveLetPattern(span))
     } else {
         Ok(mk_term::let_pat_in(
+            rec,
             bindings.into_iter().map(|mut b| {
                 if let Some(ann) = b.annot {
                     b.value = ann.annotation.attach_term(b.value);
diff --git a/core/src/pretty.rs b/core/src/pretty.rs
index 86495491..97f2101b 100644
--- a/core/src/pretty.rs
+++ b/core/src/pretty.rs
@@ -864,9 +864,10 @@ where
             Lbl(_lbl) => allocator.text("%<label>").append(allocator.line()),
             Let(bindings, body, attrs) => docs![
                 allocator,
-                "let ",
+                "let",
+                allocator.space(),
                 if attrs.rec {
-                    allocator.text("rec ")
+                    docs![allocator, "rec", allocator.space()]
                 } else {
                     allocator.nil()
                 },
@@ -883,9 +884,15 @@ where
             .append(allocator.line())
             .append(body.pretty(allocator).nest(2))
             .group(),
-            LetPattern(bindings, body) => docs![
+            LetPattern(bindings, body, attrs) => docs![
                 allocator,
-                "let ",
+                "let",
+                allocator.space(),
+                if attrs.rec {
+                    docs![allocator, "rec", allocator.space()]
+                } else {
+                    allocator.nil()
+                },
                 allocator.intersperse(
                     bindings
                         .iter()
diff --git a/core/src/term/mod.rs b/core/src/term/mod.rs
index fe208985..12b72dd7 100644
--- a/core/src/term/mod.rs
+++ b/core/src/term/mod.rs
@@ -114,7 +114,7 @@ pub enum Term {
 
     /// A destructuring let-binding.
     #[serde(skip)]
-    LetPattern(SmallVec<[(Pattern, RichTerm); 1]>, RichTerm),
+    LetPattern(SmallVec<[(Pattern, RichTerm); 1]>, RichTerm, LetAttrs),
 
     /// An application.
     #[serde(skip)]
@@ -345,7 +345,9 @@ impl PartialEq for Term {
             (Self::FunPattern(l0, l1), Self::FunPattern(r0, r1)) => l0 == r0 && l1 == r1,
             (Self::Lbl(l0), Self::Lbl(r0)) => l0 == r0,
             (Self::Let(l0, l1, l2), Self::Let(r0, r1, r2)) => l0 == r0 && l1 == r1 && l2 == r2,
-            (Self::LetPattern(l0, l1), Self::LetPattern(r0, r1)) => l0 == r0 && l1 == r1,
+            (Self::LetPattern(l0, l1, l2), Self::LetPattern(r0, r1, r2)) => {
+                l0 == r0 && l1 == r1 && l2 == r2
+            }
             (Self::App(l0, l1), Self::App(r0, r1)) => l0 == r0 && l1 == r1,
             (Self::Var(l0), Self::Var(r0)) => l0 == r0,
             (Self::Enum(l0), Self::Enum(r0)) => l0 == r0,
@@ -2205,13 +2207,13 @@ impl Traverse<RichTerm> for RichTerm {
                 let body = body.traverse(f, order)?;
                 RichTerm::new(Term::Let(bindings, body, attrs), pos)
             }
-            Term::LetPattern(bindings, body) => {
+            Term::LetPattern(bindings, body, attrs) => {
                 let bindings = bindings
                     .into_iter()
                     .map(|(key, val)| Ok((key, val.traverse(f, order)?)))
                     .collect::<Result<_, E>>()?;
                 let body = body.traverse(f, order)?;
-                RichTerm::new(Term::LetPattern(bindings, body), pos)
+                RichTerm::new(Term::LetPattern(bindings, body, attrs), pos)
             }
             Term::App(t1, t2) => {
                 let t1 = t1.traverse(f, order)?;
@@ -2404,7 +2406,7 @@ impl Traverse<RichTerm> for RichTerm {
                 .iter()
                 .find_map(|(_id, t)| t.traverse_ref(f, state))
                 .or_else(|| body.traverse_ref(f, state)),
-            Term::LetPattern(bindings, body) => bindings
+            Term::LetPattern(bindings, body, _) => bindings
                 .iter()
                 .find_map(|(_pat, t)| t.traverse_ref(f, state))
                 .or_else(|| body.traverse_ref(f, state)),
@@ -2763,23 +2765,29 @@ pub mod make {
         Term::LetPattern(
             std::iter::once((pat.into(), t1.into())).collect(),
             t2.into(),
+            LetAttrs::default(),
         )
         .into()
     }
 
-    pub fn let_pat_in<D, T1, T2, Iter>(bindings: Iter, body: T2) -> RichTerm
+    pub fn let_pat_in<D, T1, T2, Iter>(rec: bool, bindings: Iter, body: T2) -> RichTerm
     where
         T1: Into<RichTerm>,
         T2: Into<RichTerm>,
         D: Into<Pattern>,
         Iter: IntoIterator<Item = (D, T1)>,
     {
+        let attrs = LetAttrs {
+            binding_type: BindingType::Normal,
+            rec,
+        };
         Term::LetPattern(
             bindings
                 .into_iter()
                 .map(|(pat, t)| (pat.into(), t.into()))
                 .collect(),
             body.into(),
+            attrs,
         )
         .into()
     }
diff --git a/core/src/transform/desugar_destructuring.rs b/core/src/transform/desugar_destructuring.rs
index cfbfcbdc..404189dd 100644
--- a/core/src/transform/desugar_destructuring.rs
+++ b/core/src/transform/desugar_destructuring.rs
@@ -1,19 +1,17 @@
 //! Destructuring desugaring
 //!
 //! Replace a let and function bindings with destructuring by simpler constructs.
-use std::rc::Rc;
+use smallvec::SmallVec;
 
 use crate::{
+    error::EvalError,
     identifier::LocIdent,
     match_sharedterm,
-    position::TermPos,
-    term::{
-        array::{Array, ArrayAttrs},
-        pattern::*,
-        MatchBranch, MatchData, RichTerm, Term,
-    },
+    term::{make, pattern::*, record::RecordData, BinaryOp, BindingType, LetAttrs, RichTerm, Term},
 };
 
+use self::{bindings::Bindings, compile::CompilePart};
+
 /// Entry point of the destructuring desugaring transformation.
 ///
 /// As other `transform_one` variants, this transformation is not recursive and only desugars the
@@ -21,7 +19,8 @@ use crate::{
 /// destructuring patterns to be desugared in children nodes.
 pub fn transform_one(rt: RichTerm) -> RichTerm {
     match_sharedterm!(match (rt.term) {
-        Term::LetPattern(bindings, body) => RichTerm::new(desugar_let(bindings, body), rt.pos),
+        Term::LetPattern(bindings, body, attrs) =>
+            RichTerm::new(desugar_let(bindings, body, attrs.rec), rt.pos),
         Term::FunPattern(pat, body) => RichTerm::new(desugar_fun(pat, body), rt.pos),
         _ => rt,
     })
@@ -39,7 +38,11 @@ pub fn desugar_fun(mut pat: Pattern, body: RichTerm) -> Term {
     Term::Fun(
         id,
         RichTerm::new(
-            Term::LetPattern(std::iter::once((pat, Term::Var(id).into())).collect(), body),
+            Term::LetPattern(
+                std::iter::once((pat, Term::Var(id).into())).collect(),
+                body,
+                LetAttrs::default(),
+            ),
             // TODO: should we use rt.pos?
             pos_body,
         ),
@@ -48,63 +51,116 @@ pub fn desugar_fun(mut pat: Pattern, body: RichTerm) -> Term {
 
 /// Desugar a destructuring let-binding.
 ///
-/// A let-binding `let <pat1> = <bound1>, <pat2> = <bound2> in body` is desugared to
-/// `[<bound1>, <bound2>] |> match { [<pat1>, <pat2>] => body }`. If there is only one
-/// pattern, we drop the arrays in the pattern and the bound (so, `<bound1> |> match { <pat1> => body }`).
+/// A let-binding
+///
+/// ```text
+/// let
+///   <pat1> = <bound1>,
+///   <pat2> = <bound2>
+/// in body
+/// ```
+///
+/// is desugared to
+///
+/// ```text
+/// let
+///   %b1 = <bound1>,
+///   %b2 = <bound2>,
+///   %empty_record = {},
+/// in
+/// let
+///   %r1 = <pat1.compile_part(%b1, %empty_record)>,
+///   %r2 = <pat2.compile_part(%b2, %empty_record)>,
+/// in
+/// let
+///   foo = %r1.foo,
+///   ...
+///   baz = %r2.baz,
+/// in
+///   if %r1 == null then <error1> else if %r2 == null then <error2> else body
+/// ```
+/// where `foo` and `baz` are names bound in `<pat1>` and `<pat2>`.
+///
+/// There's some ambiguity about where to put the error-checking. It might be natural
+/// to put it before trying to access `%r1.foo`, but that would only raise the error
+/// if someone tries to evaluate `foo`. Putting it in the body as above raises
+/// an error, for example, in `let 'Foo = 'Bar in true`.
+///
+/// A recursive let-binding is desugared almost the same way, except that everything is
+/// shoved into a single let-rec block instead of three nested blocks.
 pub fn desugar_let(
-    bindings: impl IntoIterator<Item = (Pattern, RichTerm)>,
+    bindings: SmallVec<[(Pattern, RichTerm); 1]>,
     body: RichTerm,
+    rec: bool,
 ) -> Term {
-    // the position of the match expression is used during error reporting, so we try to provide a
-    // sensible one.
-    let mut bounds_pos = TermPos::None;
-    let mut patterns_pos = TermPos::None;
-    let mut patterns = Vec::new();
-    let mut bound = Vec::new();
-    for (pat, rt) in bindings {
-        bounds_pos = bounds_pos.fuse(rt.pos);
-        patterns_pos = patterns_pos.fuse(pat.pos);
-        patterns.push(pat);
-        bound.push(rt);
+    // Outer bindings are the ones we called %b1 and %b2, and %empty_record_id in the doc above.
+    let mut outer_bindings = SmallVec::new();
+    // Mid bindings are the ones we called %r1 and %r2 above.
+    let mut mid_bindings = SmallVec::new();
+    // Inner bindings are the ones that bind the actual variables defined in the patterns.
+    let mut inner_bindings = SmallVec::new();
+    let mut error_tests = Vec::new();
+
+    let empty_record_id = LocIdent::fresh();
+    outer_bindings.push((empty_record_id, Term::Record(RecordData::empty()).into()));
+    for (pat, rhs) in bindings {
+        let pos = pat.pos.fuse(rhs.pos);
+        let outer_id = LocIdent::fresh();
+        outer_bindings.push((outer_id, rhs.clone()));
+
+        let mid_id = LocIdent::fresh();
+        mid_bindings.push((mid_id, pat.compile_part(outer_id, empty_record_id)));
+
+        let error_case = RichTerm::new(
+            Term::RuntimeError(EvalError::FailedDestructuring {
+                value: rhs.clone(),
+                pattern: pat.clone(),
+            }),
+            pos,
+        );
+
+        let is_record_null = make::op2(BinaryOp::Eq, Term::Var(mid_id), Term::Null);
+        error_tests.push((is_record_null, error_case));
+
+        for (_path, id, _field) in pat.bindings() {
+            inner_bindings.push((
+                id,
+                make::static_access(Term::Var(mid_id), std::iter::once(id)),
+            ));
+        }
     }
 
-    let (pattern, bound) = if patterns.len() == 1 {
-        // unwrap: pattern and bound have the same length (1)
-        (
-            patterns.into_iter().next().unwrap(),
-            bound.into_iter().next().unwrap(),
+    let checked_body = error_tests
+        .into_iter()
+        .rev()
+        .fold(body, |acc, (check, error)| {
+            make::if_then_else(check, error, acc)
+        });
+
+    let attrs = LetAttrs {
+        binding_type: BindingType::Normal,
+        rec,
+    };
+    if rec {
+        Term::Let(
+            outer_bindings
+                .into_iter()
+                .chain(mid_bindings)
+                .chain(inner_bindings)
+                .collect(),
+            checked_body,
+            attrs,
         )
     } else {
-        (
-            Pattern {
-                data: PatternData::Array(ArrayPattern {
-                    patterns,
-                    tail: TailPattern::Empty,
-                    pos: patterns_pos,
-                }),
-                alias: None,
-                pos: patterns_pos,
-            },
-            RichTerm::from(Term::Array(
-                Array::new(Rc::from(bound)),
-                ArrayAttrs::default(),
-            ))
-            .with_pos(bounds_pos),
+        Term::Let(
+            outer_bindings,
+            Term::Let(
+                mid_bindings,
+                Term::Let(inner_bindings, checked_body, attrs.clone()).into(),
+                attrs.clone(),
+            )
+            .into(),
+            attrs,
         )
-    };
-
-    // `(match { [<pat1>, ..., <patn>] => <body> }) [<bound1>, ..., <boundn>]`
-    Term::App(
-        RichTerm::new(
-            Term::Match(MatchData {
-                branches: vec![MatchBranch {
-                    pattern,
-                    guard: None,
-                    body,
-                }],
-            }),
-            bounds_pos.fuse(patterns_pos),
-        ),
-        bound,
-    )
+    }
 }
diff --git a/core/src/transform/free_vars.rs b/core/src/transform/free_vars.rs
index 76e58ed8..a01643eb 100644
--- a/core/src/transform/free_vars.rs
+++ b/core/src/transform/free_vars.rs
@@ -78,11 +78,15 @@ impl CollectFreeVars for RichTerm {
 
                 free_vars.extend(fresh);
             }
-            Term::LetPattern(bindings, body) => {
+            Term::LetPattern(bindings, body, attrs) => {
                 let mut fresh = HashSet::new();
 
                 for (_pat, rt) in bindings.iter_mut() {
-                    rt.collect_free_vars(free_vars);
+                    if attrs.rec {
+                        rt.collect_free_vars(&mut fresh);
+                    } else {
+                        rt.collect_free_vars(free_vars);
+                    }
                 }
 
                 body.collect_free_vars(&mut fresh);
diff --git a/core/src/typecheck/mod.rs b/core/src/typecheck/mod.rs
index 42db98e7..a2c335de 100644
--- a/core/src/typecheck/mod.rs
+++ b/core/src/typecheck/mod.rs
@@ -58,13 +58,12 @@ use crate::{
     environment::Environment as GenericEnvironment,
     error::TypecheckError,
     identifier::{Ident, LocIdent},
-    stdlib as nickel_stdlib,
+    mk_uty_arrow, mk_uty_enum, mk_uty_record, mk_uty_record_row, stdlib as nickel_stdlib,
     term::{
-        record::Field, LabeledType, MatchBranch, RichTerm, StrChunk, Term, Traverse, TraverseOrder,
-        TypeAnnotation,
+        pattern::bindings::Bindings as _, record::Field, LabeledType, MatchBranch, RichTerm,
+        StrChunk, Term, Traverse, TraverseOrder, TypeAnnotation,
     },
     typ::*,
-    {mk_uty_arrow, mk_uty_enum, mk_uty_record, mk_uty_record_row},
 };
 
 use std::{
@@ -1522,14 +1521,23 @@ fn walk<V: TypecheckVisitor>(
 
             walk(state, ctxt, visitor, rt)
         }
-        Term::LetPattern(bindings, rt) => {
+        Term::LetPattern(bindings, rt, attrs) => {
+            // For a recursive let block, shadow all the names we're about to bind, so
+            // we aren't influenced by variables defined in an outer scope.
+            if attrs.rec {
+                for (pat, _re) in bindings {
+                    for (_path, id, _fld) in pat.bindings() {
+                        ctxt.type_env
+                            .insert(id.ident(), state.table.fresh_type_uvar(ctxt.var_level));
+                    }
+                }
+            }
+
             let start_ctxt = ctxt.clone();
 
             for (pat, re) in bindings {
                 let ty_let = binding_type(state, re.as_ref(), &start_ctxt, false);
 
-                walk(state, start_ctxt.clone(), visitor, re)?;
-
                 // In the case of a let-binding, we want to guess a better type than `Dyn` when we can
                 // do so cheaply for the whole pattern.
                 if let Some(alias) = &pat.alias {
@@ -1541,6 +1549,9 @@ fn walk<V: TypecheckVisitor>(
                 // data, which doesn't take into account the potential heading alias `x @ <pattern>`.
                 // This is on purpose, as the alias has been treated separately, so we don't want to
                 // shadow it with a less precise type.
+                //
+                // The use of start_ctxt here looks like it might be wrong for let rec, but in fact
+                // it's unused in TypecheckMode::Walk anyway.
                 let PatternTypeData {bindings: pat_bindings, ..} = pat.data.pattern_types(state, &start_ctxt, pattern::TypecheckMode::Walk)?;
 
                 for (id, typ) in pat_bindings {
@@ -1549,6 +1560,11 @@ fn walk<V: TypecheckVisitor>(
                 }
             }
 
+            let re_ctxt = if attrs.rec { ctxt.clone() } else { start_ctxt.clone() };
+            for (_pat, re) in bindings {
+                walk(state, re_ctxt.clone(), visitor, re)?;
+            }
+
             walk(state, ctxt, visitor, rt)
         }
         Term::App(e, t) => {
@@ -2004,7 +2020,19 @@ fn check<V: TypecheckVisitor>(
 
             check(state, ctxt.clone(), visitor, rt, ty)
         }
-        Term::LetPattern(bindings, rt) => {
+        Term::LetPattern(bindings, rt, attrs) => {
+            // For a recursive let block, shadow all the names we're about to bind, so
+            // we aren't influenced by variables defined in an outer scope.
+            if attrs.rec {
+                for (pat, _re) in bindings {
+                    for (_path, id, _fld) in pat.bindings() {
+                        ctxt.type_env
+                            .insert(id.ident(), state.table.fresh_type_uvar(ctxt.var_level));
+                    }
+                }
+            }
+
+            let mut tys = Vec::new();
             let start_ctxt = ctxt.clone();
             for (pat, re) in bindings {
                 // See [^separate-alias-treatment].
@@ -2023,17 +2051,21 @@ fn check<V: TypecheckVisitor>(
                     .unify(ty_let.clone(), state, &start_ctxt)
                     .map_err(|e| e.into_typecheck_err(state, re.pos))?;
 
-                check(state, start_ctxt.clone(), visitor, re, ty_let.clone())?;
-
                 if let Some(alias) = &pat.alias {
                     visitor.visit_ident(alias, ty_let.clone());
-                    ctxt.type_env.insert(alias.ident(), ty_let);
+                    ctxt.type_env.insert(alias.ident(), ty_let.clone());
                 }
 
                 for (id, typ) in pat_types.bindings {
                     visitor.visit_ident(&id, typ.clone());
                     ctxt.type_env.insert(id.ident(), typ);
                 }
+                tys.push((re, ty_let));
+            }
+
+            let re_ctxt = if attrs.rec { &ctxt } else { &start_ctxt };
+            for (re, ty_let) in tys {
+                check(state, re_ctxt.clone(), visitor, re, ty_let)?;
             }
 
             check(state, ctxt, visitor, rt, ty)
diff --git a/doc/manual/syntax.md b/doc/manual/syntax.md
index 88e55274..2774bb9d 100644
--- a/doc/manual/syntax.md
+++ b/doc/manual/syntax.md
@@ -937,7 +937,7 @@ Examples:
 [ 2, 3 ]
 
 > let 'Invalid x = {} in x
-error: unmatched pattern
+error: destructuring failed
 [...]
 ```
 
diff --git a/lsp/nls/src/field_walker.rs b/lsp/nls/src/field_walker.rs
index fdfa9dae..a8359ad9 100644
--- a/lsp/nls/src/field_walker.rs
+++ b/lsp/nls/src/field_walker.rs
@@ -432,7 +432,7 @@ impl<'a> FieldResolver<'a> {
             Term::Op2(BinaryOp::Merge(_), t1, t2) => {
                 combine(self.resolve_container(t1), self.resolve_container(t2))
             }
-            Term::Let(_, body, _) | Term::LetPattern(_, body) => self.resolve_container(body),
+            Term::Let(_, body, _) | Term::LetPattern(_, body, _) => self.resolve_container(body),
             Term::Op1(UnaryOp::RecordAccess(id), term) => {
                 self.containers_at_path(term, std::iter::once(id.ident()))
             }
diff --git a/lsp/nls/src/position.rs b/lsp/nls/src/position.rs
index 47881caa..332c91b4 100644
--- a/lsp/nls/src/position.rs
+++ b/lsp/nls/src/position.rs
@@ -126,7 +126,7 @@ impl PositionLookup {
                 Term::Let(bindings, _, _) => {
                     idents.extend(bindings.iter().map(|(id, _)| *id));
                 }
-                Term::LetPattern(bindings, _) => {
+                Term::LetPattern(bindings, _, _) => {
                     for (pat, _) in bindings {
                         let ids = pat.bindings().into_iter().map(|(_path, id, _)| id);
                         idents.extend(ids);
diff --git a/lsp/nls/src/usage.rs b/lsp/nls/src/usage.rs
index 24ad68ee..69463c63 100644
--- a/lsp/nls/src/usage.rs
+++ b/lsp/nls/src/usage.rs
@@ -172,7 +172,7 @@ impl UsageLookup {
 
                         TraverseControl::SkipBranch
                     }
-                    Term::LetPattern(bindings, _body) => {
+                    Term::LetPattern(bindings, body, attrs) => {
                         let mut new_env = env.clone();
 
                         for (pat, val) in bindings {
@@ -188,7 +188,12 @@ impl UsageLookup {
                             }
                         }
 
-                        TraverseControl::ContinueWithScope(new_env)
+                        for (_, val) in bindings {
+                            self.fill(val, if attrs.rec { &new_env } else { env });
+                        }
+                        self.fill(body, &new_env);
+
+                        TraverseControl::SkipBranch
                     }
                     Term::RecRecord(data, ..) | Term::Record(data) => {
                         let mut new_env = env.clone();
diff --git a/lsp/nls/src/world.rs b/lsp/nls/src/world.rs
index 77d20797..7c8cb0ea 100644
--- a/lsp/nls/src/world.rs
+++ b/lsp/nls/src/world.rs
@@ -269,7 +269,7 @@ impl World {
                         })
                         .collect()
                 }
-                (Term::LetPattern(bindings, _), Some(hovered_id)) => {
+                (Term::LetPattern(bindings, _, _), Some(hovered_id)) => {
                     let mut spans = Vec::new();
                     for (pat, value) in bindings {
                         if let Some((path, _, _)) = pat


Test patch:
diff --git a/cli/tests/snapshot/inputs/errors/let_rec_block_with_pat.ncl b/cli/tests/snapshot/inputs/errors/let_rec_block_with_pat.ncl
deleted file mode 100644
index eda9309a..00000000
--- a/cli/tests/snapshot/inputs/errors/let_rec_block_with_pat.ncl
+++ /dev/null
@@ -1,7 +0,0 @@
-# capture = 'stderr'
-# command = ['eval']
-let rec
-  a = 2,
-  { b } = { b = 1 }
-in
-b
diff --git a/cli/tests/snapshot/snapshots/snapshot__eval_stderr_destructuring_closed_fail.ncl.snap b/cli/tests/snapshot/snapshots/snapshot__eval_stderr_destructuring_closed_fail.ncl.snap
index eb3c6adb..0fd4c245 100644
--- a/cli/tests/snapshot/snapshots/snapshot__eval_stderr_destructuring_closed_fail.ncl.snap
+++ b/cli/tests/snapshot/snapshots/snapshot__eval_stderr_destructuring_closed_fail.ncl.snap
@@ -2,11 +2,10 @@
 source: cli/tests/snapshot/main.rs
 expression: err
 ---
-error: unmatched pattern
+error: destructuring failed
   ┌─ [INPUTS_PATH]/errors/destructuring_closed_fail.ncl:3:5
   │
 3 │ let {a} = {a=1, b=2}
-  │     ^^^^^^^^^^^^^^^^
-  │     │     │
-  │     │     this value doesn't match any branch
-  │     in this match expression
+  │     ^^^   ---------- this value failed to match
+  │     │      
+  │     this pattern
diff --git a/cli/tests/snapshot/snapshots/snapshot__eval_stderr_let_rec_block_with_pat.ncl.snap b/cli/tests/snapshot/snapshots/snapshot__eval_stderr_let_rec_block_with_pat.ncl.snap
deleted file mode 100644
index 6f7be5b0..00000000
--- a/cli/tests/snapshot/snapshots/snapshot__eval_stderr_let_rec_block_with_pat.ncl.snap
+++ /dev/null
@@ -1,12 +0,0 @@
----
-source: cli/tests/snapshot/main.rs
-expression: err
----
-error: recursive destructuring is not supported
-  ┌─ [INPUTS_PATH]/errors/let_rec_block_with_pat.ncl:3:5
-  │
-3 │ let rec
-  │     ^^^
-  │
-  = A destructuring let-binding can't be recursive. Try removing the `rec` from `let rec`.
-  = You can reference other fields of a record recursively from within a field, so you might not need the recursive let.
diff --git a/core/tests/integration/inputs/core/let_rec_block_with_pat.ncl b/core/tests/integration/inputs/core/let_rec_block_with_pat.ncl
new file mode 100644
index 00000000..6ea18913
--- /dev/null
+++ b/core/tests/integration/inputs/core/let_rec_block_with_pat.ncl
@@ -0,0 +1,6 @@
+# test.type = 'pass'
+let rec
+  a = 2,
+  { b } = { b = a }
+in
+b == 2
diff --git a/core/tests/integration/inputs/destructuring/adt_enum_tag_tag_mismatch.ncl b/core/tests/integration/inputs/destructuring/adt_enum_tag_tag_mismatch.ncl
index 09ab2159..bd909315 100644
--- a/core/tests/integration/inputs/destructuring/adt_enum_tag_tag_mismatch.ncl
+++ b/core/tests/integration/inputs/destructuring/adt_enum_tag_tag_mismatch.ncl
@@ -1,6 +1,6 @@
 # test.type = 'error'
 #
 # [test.metadata]
-# error = 'EvalError::NonExhaustiveEnumMatch'
+# error = 'EvalError::FailedDestructuring'
 let 'Foo = 'Bar in
 true
diff --git a/core/tests/integration/inputs/destructuring/adt_extra_arg.ncl b/core/tests/integration/inputs/destructuring/adt_extra_arg.ncl
index facfe98c..657a8c94 100644
--- a/core/tests/integration/inputs/destructuring/adt_extra_arg.ncl
+++ b/core/tests/integration/inputs/destructuring/adt_extra_arg.ncl
@@ -1,6 +1,6 @@
 # test.type = 'error'
 #
 # [test.metadata]
-# error = 'EvalError::NAryPrimopTypeError'
+# error = 'EvalError::FailedDestructuring'
 let 'Foo = 'Foo 5 in
 true
diff --git a/core/tests/integration/inputs/destructuring/adt_missing_arg.ncl b/core/tests/integration/inputs/destructuring/adt_missing_arg.ncl
index 8e844917..6c77f8d8 100644
--- a/core/tests/integration/inputs/destructuring/adt_missing_arg.ncl
+++ b/core/tests/integration/inputs/destructuring/adt_missing_arg.ncl
@@ -1,6 +1,6 @@
 # test.type = 'error'
 #
 # [test.metadata]
-# error = 'EvalError::NonExhaustiveMatch'
+# error = 'EvalError::FailedDestructuring'
 let 'Foo x = 'Foo in
 x
diff --git a/core/tests/integration/inputs/destructuring/adt_wrong_tag.ncl b/core/tests/integration/inputs/destructuring/adt_wrong_tag.ncl
index b7e49ae7..5c6cd7c7 100644
--- a/core/tests/integration/inputs/destructuring/adt_wrong_tag.ncl
+++ b/core/tests/integration/inputs/destructuring/adt_wrong_tag.ncl
@@ -1,6 +1,6 @@
 # test.type = 'error'
 #
 # [test.metadata]
-# error = 'EvalError::NonExhaustiveMatch'
+# error = 'EvalError::FailedDestructuring'
 let 'Foo x = 'Bar 5  in
 x
diff --git a/core/tests/integration/inputs/destructuring/adt_wrong_type.ncl b/core/tests/integration/inputs/destructuring/adt_wrong_type.ncl
index 85188777..119b2e86 100644
--- a/core/tests/integration/inputs/destructuring/adt_wrong_type.ncl
+++ b/core/tests/integration/inputs/destructuring/adt_wrong_type.ncl
@@ -1,6 +1,6 @@
 # test.type = 'error'
 #
 # [test.metadata]
-# error = 'EvalError::NonExhaustiveMatch'
+# error = 'EvalError::FailedDestructuring'
 let 'Foo x = {bar = 1}  in
 x
diff --git a/core/tests/integration/inputs/destructuring/closed_fail.ncl b/core/tests/integration/inputs/destructuring/closed_fail.ncl
index b04e2a38..83475c8e 100644
--- a/core/tests/integration/inputs/destructuring/closed_fail.ncl
+++ b/core/tests/integration/inputs/destructuring/closed_fail.ncl
@@ -1,6 +1,6 @@
 # test.type = 'error'
 #
 # [test.metadata]
-# error = 'EvalError::NonExhaustiveMatch'
+# error = 'EvalError::FailedDestructuring'
 let {a} = {a=1, b=2}
 in a == 1
diff --git a/core/tests/integration/inputs/destructuring/constant_bool_fail.ncl b/core/tests/integration/inputs/destructuring/constant_bool_fail.ncl
index 78d64cd2..6e6966ec 100644
--- a/core/tests/integration/inputs/destructuring/constant_bool_fail.ncl
+++ b/core/tests/integration/inputs/destructuring/constant_bool_fail.ncl
@@ -1,5 +1,5 @@
 # test.type = 'error'
 #
 # [test.metadata]
-# error = 'EvalError::NonExhaustiveMatch'
+# error = 'EvalError::FailedDestructuring'
 let true = false in true
diff --git a/core/tests/integration/inputs/destructuring/constant_null_fail.ncl b/core/tests/integration/inputs/destructuring/constant_null_fail.ncl
index 1cb75351..202b9a20 100644
--- a/core/tests/integration/inputs/destructuring/constant_null_fail.ncl
+++ b/core/tests/integration/inputs/destructuring/constant_null_fail.ncl
@@ -1,5 +1,5 @@
 # test.type = 'error'
 #
 # [test.metadata]
-# error = 'EvalError::NonExhaustiveMatch'
+# error = 'EvalError::FailedDestructuring'
 let null = 0 in true
diff --git a/core/tests/integration/inputs/destructuring/constant_number_fail.ncl b/core/tests/integration/inputs/destructuring/constant_number_fail.ncl
index bdf99a42..baaba35f 100644
--- a/core/tests/integration/inputs/destructuring/constant_number_fail.ncl
+++ b/core/tests/integration/inputs/destructuring/constant_number_fail.ncl
@@ -1,5 +1,5 @@
 # test.type = 'error'
 #
 # [test.metadata]
-# error = 'EvalError::NonExhaustiveMatch'
+# error = 'EvalError::FailedDestructuring'
 let 1 = 1 + 1 in true
diff --git a/core/tests/integration/inputs/destructuring/constant_string_fail.ncl b/core/tests/integration/inputs/destructuring/constant_string_fail.ncl
index 7a6760ea..f4164b2d 100644
--- a/core/tests/integration/inputs/destructuring/constant_string_fail.ncl
+++ b/core/tests/integration/inputs/destructuring/constant_string_fail.ncl
@@ -1,5 +1,5 @@
 # test.type = 'error'
 #
 # [test.metadata]
-# error = 'EvalError::NonExhaustiveMatch'
+# error = 'EvalError::FailedDestructuring'
 let "ab" = "a" ++ "b" ++ "c" in true
diff --git a/core/tests/integration/inputs/typecheck/let_blocks.ncl b/core/tests/integration/inputs/typecheck/let_blocks.ncl
index 90a51759..87ef432d 100644
--- a/core/tests/integration/inputs/typecheck/let_blocks.ncl
+++ b/core/tests/integration/inputs/typecheck/let_blocks.ncl
@@ -28,6 +28,11 @@
     { x, y } = { x = 1, y = x },
     z = 2,
   in y + z,
+
+  let rec
+    a = 2,
+    { b } = { b = a }
+  in b,
 ]
 in
 true): _
diff --git a/core/tests/integration/main.rs b/core/tests/integration/main.rs
index 4764f4cb..6aa5b8c4 100644
--- a/core/tests/integration/main.rs
+++ b/core/tests/integration/main.rs
@@ -171,6 +171,8 @@ enum ErrorExpectation {
     EvalNonExhaustiveMatch,
     #[serde(rename = "EvalError::NonExhaustiveEnumMatch")]
     EvalNonExhaustiveEnumMatch,
+    #[serde(rename = "EvalError::FailedDestructuring")]
+    EvalFailedDestructuring,
     #[serde(rename = "TypecheckError::UnboundIdentifier")]
     TypecheckUnboundIdentifier { identifier: String },
     #[serde(rename = "TypecheckError::UnboundTypeVariable")]
@@ -247,6 +249,7 @@ impl PartialEq<Error> for ErrorExpectation {
                 EvalNonExhaustiveEnumMatch,
                 Error::EvalError(EvalError::NonExhaustiveEnumMatch { .. }),
             )
+            | (EvalFailedDestructuring, Error::EvalError(EvalError::FailedDestructuring { .. }))
             | (
                 TypecheckRecordRowMismatch,
                 Error::TypecheckError(TypecheckError::RecordRowMismatch { .. }),
@@ -410,6 +413,7 @@ impl std::fmt::Display for ErrorExpectation {
             }
             EvalNonExhaustiveMatch => "EvalError::NonExhaustiveMatch".to_owned(),
             EvalNonExhaustiveEnumMatch => "EvalError::NonExhaustiveEnumMatch".to_owned(),
+            EvalFailedDestructuring => "EvalError::FailedDestructuring".to_owned(),
             TypecheckUnboundIdentifier { identifier } => {
                 format!("TypecheckError::UnboundIdentifier({identifier})")
             }
diff --git a/lsp/nls/tests/inputs/completion-patterns-rec.ncl b/lsp/nls/tests/inputs/completion-patterns-rec.ncl
new file mode 100644
index 00000000..56bc8be0
--- /dev/null
+++ b/lsp/nls/tests/inputs/completion-patterns-rec.ncl
@@ -0,0 +1,88 @@
+### /input.ncl
+let rec outer@{ foo = inner@{ bar = innermost, .. }, .. } = {
+  extra = 1,
+  foo = {
+    more = 1,
+    bar = {
+      most = 1,
+      baz = 1,
+    }
+  },
+}
+in
+[
+  outer.foo,
+  outer.foo.bar,
+  outer.foo.bar.baz,
+  inner.bar,
+  inner.bar.baz,
+  innermost.baz,
+]
+### [[request]]
+### type = "Completion"
+### textDocument.uri = "file:///input.ncl"
+### position = { line = 12, character = 10 }
+###
+### [[request]]
+### type = "Completion"
+### textDocument.uri = "file:///input.ncl"
+### position = { line = 13, character = 14 }
+###
+### [[request]]
+### type = "Completion"
+### textDocument.uri = "file:///input.ncl"
+### position = { line = 14, character = 18 }
+###
+### [[request]]
+### type = "Completion"
+### textDocument.uri = "file:///input.ncl"
+### position = { line = 15, character = 10 }
+###
+### [[request]]
+### type = "Completion"
+### textDocument.uri = "file:///input.ncl"
+### position = { line = 16, character = 14 }
+###
+### [[request]]
+### type = "Completion"
+### textDocument.uri = "file:///input.ncl"
+### position = { line = 17, character = 14 }
+###
+### # Record field completions
+###
+### [[request]]
+### type = "Completion"
+### textDocument.uri = "file:///input.ncl"
+### position = { line = 1, character = 3 }
+###
+### [[request]]
+### type = "Completion"
+### textDocument.uri = "file:///input.ncl"
+### position = { line = 3, character = 5 }
+###
+### [[request]]
+### type = "Completion"
+### textDocument.uri = "file:///input.ncl"
+### position = { line = 5, character = 7 }
+###
+### # Env completions
+###
+### [[request]]
+### type = "Completion"
+### textDocument.uri = "file:///input.ncl"
+### position = { line = 1, character = 10 }
+###
+### [[request]]
+### type = "Completion"
+### textDocument.uri = "file:///input.ncl"
+### position = { line = 3, character = 11 }
+###
+### [[request]]
+### type = "Completion"
+### textDocument.uri = "file:///input.ncl"
+### position = { line = 5, character = 13 }
+###
+### [[request]]
+### type = "Completion"
+### textDocument.uri = "file:///input.ncl"
+### position = { line = 11, character = 1 }
diff --git a/lsp/nls/tests/inputs/completion-patterns.ncl b/lsp/nls/tests/inputs/completion-patterns.ncl
index 6a53aa4d..31c944b2 100644
--- a/lsp/nls/tests/inputs/completion-patterns.ncl
+++ b/lsp/nls/tests/inputs/completion-patterns.ncl
@@ -81,3 +81,8 @@ in
 ### type = "Completion"
 ### textDocument.uri = "file:///input.ncl"
 ### position = { line = 5, character = 13 }
+###
+### [[request]]
+### type = "Completion"
+### textDocument.uri = "file:///input.ncl"
+### position = { line = 11, character = 1 }
diff --git a/lsp/nls/tests/snapshots/main__lsp__nls__tests__inputs__completion-patterns-rec.ncl.snap b/lsp/nls/tests/snapshots/main__lsp__nls__tests__inputs__completion-patterns-rec.ncl.snap
new file mode 100644
index 00000000..7b0360c9
--- /dev/null
+++ b/lsp/nls/tests/snapshots/main__lsp__nls__tests__inputs__completion-patterns-rec.ncl.snap
@@ -0,0 +1,17 @@
+---
+source: lsp/nls/tests/main.rs
+expression: output
+---
+[extra, foo]
+[bar, more]
+[baz, most]
+[bar (Dyn), more]
+[baz, most]
+[baz, most]
+[foo]
+[bar]
+[baz]
+[inner, innermost, outer, std]
+[extra, foo, inner, innermost, outer, std]
+[bar, extra, foo, inner, innermost, more, outer, std]
+[inner, innermost, outer, std]
diff --git a/lsp/nls/tests/snapshots/main__lsp__nls__tests__inputs__completion-patterns.ncl.snap b/lsp/nls/tests/snapshots/main__lsp__nls__tests__inputs__completion-patterns.ncl.snap
index 8c28c32e..e34b3a13 100644
--- a/lsp/nls/tests/snapshots/main__lsp__nls__tests__inputs__completion-patterns.ncl.snap
+++ b/lsp/nls/tests/snapshots/main__lsp__nls__tests__inputs__completion-patterns.ncl.snap
@@ -11,6 +11,7 @@ expression: output
 [foo]
 [bar]
 [baz]
+[std]
+[extra, foo, std]
+[bar, extra, foo, more, std]
 [inner, innermost, outer, std]
-[extra, foo, inner, innermost, outer, std]
-[bar, extra, foo, inner, innermost, more, outer, std]