From 4055764f5db412bc9bd3de7ac11b2ed7fd1f53c8 Mon Sep 17 00:00:00 2001 From: Yann Hamdaoui Date: Tue, 30 Jan 2024 18:45:38 +0100 Subject: [PATCH 1/7] Refactoring of pattern destructuring This commit is a preliminary work for the upcoming ADTs (and more generally the introduction of pattern matching, instead of just having destructuring). The refactoring aims at installing a consistent naming, simplify the representation of patterns and their associated method, and pave the way for other patterns that just records. Indeed, the previous implementation often made the implicit assumptions that patterns were only record patterns. --- core/src/destructuring.rs | 259 ++++++------- core/src/parser/grammar.lalrpop | 79 ++-- core/src/parser/utils.rs | 30 +- core/src/pretty.rs | 45 +-- core/src/term/mod.rs | 8 +- core/src/transform/desugar_destructuring.rs | 308 ++++++++-------- core/src/transform/free_vars.rs | 76 ++-- core/src/typecheck/destructuring.rs | 382 +++++++------------- core/src/typecheck/mod.rs | 60 +-- lsp/nls/src/main.rs | 1 + lsp/nls/src/pattern.rs | 111 ++++++ lsp/nls/src/position.rs | 15 +- lsp/nls/src/usage.rs | 30 +- 13 files changed, 696 insertions(+), 708 deletions(-) create mode 100644 lsp/nls/src/pattern.rs diff --git a/core/src/destructuring.rs b/core/src/destructuring.rs index c9031f56a..f4541e3ff 100644 --- a/core/src/destructuring.rs +++ b/core/src/destructuring.rs @@ -1,6 +1,4 @@ -//! In this module, you have the main structures used in the destructuring feature of nickel. -//! Also, there are implementation managing the generation of a contract from a pattern. - +//! Pattern matching and destructuring of Nickel values. use std::collections::{hash_map::Entry, HashMap}; use crate::{ @@ -16,55 +14,70 @@ use crate::{ }; #[derive(Debug, PartialEq, Clone)] -pub enum FieldPattern { - /// An assignment match like `{ ..., a = b, ... }` - Ident(LocIdent), - /// A nested record pattern like `{ ..., a = { b, c }, ... }` +pub enum Pattern { + /// A simple pattern consisting of an identifier. Match anything and bind the result to the + /// corresponding identfier. + Any(LocIdent), + /// A record pattern as in `{ a = { b, c } }` RecordPattern(RecordPattern), - /// An aliased nested record pattern like `{ ..., a = b @ { c, d }, ... }` - AliasedRecordPattern { + /// An aliased pattern as in `x @ { a = b @ { c, d }, ..}` + AliasedPattern { alias: LocIdent, - pattern: RecordPattern, + pattern: Box, }, } -/// A match field in a `Destruct` pattern. Every field can be annotated with a type, with contracts -/// or with a default value. #[derive(Debug, PartialEq, Clone)] -pub enum Match { - /// `{..., a=b, ...}` will bind the field `a` of the record to variable `b`. Here, `a` is the - /// first field of this variant. Any annotations or metadata associated with `a` go into - /// the `Field` field, and `b` goes into the `FieldPattern` field, which can actually be a - /// nested destruct pattern. - Assign(LocIdent, Field, FieldPattern), - /// Simple binding. the `Ident` is bind to a variable with the same name. - Simple(LocIdent, Field), +pub struct LocPattern { + pub pattern: Pattern, + pub span: RawSpan, } -impl Match { - fn ident(&self) -> LocIdent { - match self { - Match::Assign(ident, ..) | Match::Simple(ident, ..) => *ident, - } - } +/// A field pattern inside a record pattern. Every field can be annotated with a type, contracts or +/// with a default value. +#[derive(Debug, PartialEq, Clone)] +pub struct FieldPattern { + /// The name of the matched field. For example, in `{..., foo = {bar, baz}, ...}`, the matched + /// identifier is `foo`. + pub matched_id: LocIdent, + /// The potentital decoration of the pattern, such as a type annotation, contract annotations, + /// or a default value. + pub decoration: Field, + /// The pattern on the right-hand side of the `=`. A pattern like `{foo, bar}`, without the `=` + /// sign, is considered to be `{foo=foo, bar=bar}`. In this case, `pattern` will be + /// [Pattern::Any]. + pub pattern: LocPattern, + pub span: RawSpan, } -/// Last match field of a `Destruct`. +/// The last match in a data structure pattern. This can either be a normal match, or an ellipsis +/// which can capture the rest of the data structure. The type parameter `P` is the type of the +/// pattern of the data structure: currently, ellipsis matches are only supported for record, but +/// we'll probably support them for arrays as well. +/// +/// # Example +/// +/// - In `{foo={}, bar}`, the last match is an normal match. +/// - In `{foo={}, bar, ..}`, the last match is a non-capturing ellipsis. +/// - In `{foo={}, bar, ..rest}`, the last match is a capturing ellipsis. #[derive(Debug, PartialEq, Clone)] -pub enum LastMatch { +pub enum LastPattern

{ /// The last field is a normal match. In this case the pattern is "closed" so every record /// fields should be matched. - Match(Box), + Normal(Box

), /// The pattern is "open" `, ..}`. Optionally you can bind a record containing the remaining /// fields to an `Identifier` using the syntax `, ..y}`. Ellipsis(Option), } -/// A destructured record pattern +/// A record pattern. #[derive(Debug, PartialEq, Clone)] pub struct RecordPattern { - pub matches: Vec, + /// The patterns for each field in the record. + pub patterns: Vec, + /// If the pattern is open, i.e. if it ended with an ellipsis, capturing the rest or not. pub open: bool, + /// If the pattern is open and the rest is captured, the capturing identifier is stored here. pub rest: Option, pub span: RawSpan, } @@ -93,8 +106,8 @@ impl RecordPattern { pub fn check_matches(&self) -> Result<(), ParseError> { let mut matches = HashMap::new(); - for m in self.matches.iter() { - let binding = m.ident(); + for pat in self.patterns.iter() { + let binding = pat.matched_id; let label = binding.label().to_owned(); match matches.entry(label) { Entry::Occupied(occupied_entry) => { @@ -111,22 +124,67 @@ impl RecordPattern { Ok(()) } +} + +impl FieldPattern { + /// Convert this field pattern to a field binding with metadata. It's used to generate the + /// record contract associated to a record pattern. + pub fn as_record_binding(&self) -> (LocIdent, Field) { + let mut decoration = self.decoration.clone(); + + // If the inner pattern gives rise to a contract, add it the to the field decoration. + decoration + .metadata + .annotation + .contracts + .extend(self.pattern.elaborate_contract()); + + (self.matched_id, decoration) + } +} + +// We don't implement elaborate_contract for `FieldPattern`, which is of a slightly different +// nature (it's a part of a record pattern). Instead, we call to `FieldPattern::as_record_binding`, +// which takes care of elaborating to an appropriate record field. +pub trait ElaborateContract { + /// Elaborate a contract from this pattern. The contract will check both the structure of the + /// matched value (e.g. the presence of fields in a record) and incoporate user provided + /// contracts and annotations, as well as default values. + /// + /// Some patterns don't give rise to any contract (e.g. `Any`), in which case this function + /// returns `None`. + fn elaborate_contract(&self) -> Option; +} + +impl ElaborateContract for Pattern { + fn elaborate_contract(&self) -> Option { + match self { + Pattern::Any(_) => None, + Pattern::RecordPattern(pat) => pat.elaborate_contract(), + Pattern::AliasedPattern { pattern, .. } => pattern.elaborate_contract(), + } + } +} - /// Generate the contract elaborated from this pattern. - pub fn into_contract(self) -> LabeledType { - let span = self.span; - self.into_contract_with_span(span) +impl ElaborateContract for LocPattern { + fn elaborate_contract(&self) -> Option { + self.pattern.elaborate_contract() } +} + +impl ElaborateContract for RecordPattern { + fn elaborate_contract(&self) -> Option { + let pos = TermPos::Original(self.span); - fn into_contract_with_span(self, span: RawSpan) -> LabeledType { - let is_open = self.is_open(); - let pos = TermPos::Original(span); let typ = Type { typ: TypeF::Flat( Term::Record(RecordData::new( - self.inner().into_iter().map(Match::as_binding).collect(), + self.patterns + .iter() + .map(FieldPattern::as_record_binding) + .collect(), RecordAttrs { - open: is_open, + open: self.open, ..Default::default() }, None, @@ -135,123 +193,14 @@ impl RecordPattern { ), pos, }; - LabeledType { + + Some(LabeledType { typ: typ.clone(), label: Label { typ: typ.into(), - span, + span: self.span, ..Default::default() }, - } - } - - /// Get the inner vector of `Matches` of the pattern. If `Empty` return a empty vector. - pub fn inner(self) -> Vec { - self.matches - } - - /// Is this pattern open? Does it finish with `, ..}` form? - pub fn is_open(&self) -> bool { - self.open - } -} - -impl Match { - /// Convert the `Match` to a field binding with metadata. It's used to generate the record - /// contract representing a record pattern destructuring. - pub fn as_binding(self) -> (LocIdent, Field) { - match self { - Match::Assign(id, field, FieldPattern::Ident(_)) | Match::Simple(id, field) => { - (id, field) - } - - // In this case we fuse spans of the `Ident` (LHS) with the destruct (RHS) - // because we can have two cases: - // - // - extra field on the destructuring `d` - // - missing field on the `id` - Match::Assign( - id, - mut field, - FieldPattern::RecordPattern(pattern) - | FieldPattern::AliasedRecordPattern { pattern, .. }, - ) => { - let span = RawSpan::fuse(id.pos.unwrap(), pattern.span).unwrap(); - field - .metadata - .annotation - .contracts - .push(pattern.into_contract_with_span(span)); - - (id, field) - } - } - } - - /// Returns info about each variable bound in a particular pattern. - /// It also tells the "path" to the bound variable; this is just the - /// record field names traversed to get to a pattern. - pub fn to_flattened_bindings(&self) -> Vec<(Vec, LocIdent, Field)> { - fn get_span(id: &LocIdent, pattern: &RecordPattern) -> RawSpan { - RawSpan::fuse(id.pos.unwrap(), pattern.span).unwrap() - } - - fn flatten_matches( - id: &LocIdent, - matches: &[Match], - ) -> Vec<(Vec, LocIdent, Field)> { - matches - .iter() - .flat_map(|m| m.to_flattened_bindings()) - .map(|(mut path, bind, field)| { - path.push(*id); - (path, bind, field) - }) - .collect() - } - - match self { - Match::Simple(id, field) => vec![(vec![*id], *id, field.clone())], - Match::Assign(id, field, FieldPattern::Ident(bind_id)) => { - vec![(vec![*id], *bind_id, field.clone())] - } - Match::Assign( - id, - field, - FieldPattern::RecordPattern(ref pattern @ RecordPattern { ref matches, .. }), - ) => { - let span = get_span(id, pattern); - let pattern = pattern.clone(); - let mut field = field.clone(); - field - .metadata - .annotation - .contracts - .push(pattern.into_contract_with_span(span)); - - flatten_matches(id, matches) - } - Match::Assign( - id, - field, - FieldPattern::AliasedRecordPattern { - alias: bind_id, - pattern: ref pattern @ RecordPattern { ref matches, .. }, - }, - ) => { - let span = get_span(id, pattern); - let pattern = pattern.clone(); - let mut field = field.clone(); - field - .metadata - .annotation - .contracts - .push(pattern.into_contract_with_span(span)); - - let mut flattened = flatten_matches(id, matches); - flattened.push((vec![*id], *bind_id, field)); - flattened - } - } + }) } } diff --git a/core/src/parser/grammar.lalrpop b/core/src/parser/grammar.lalrpop index ad2ffbd7b..20880052c 100644 --- a/core/src/parser/grammar.lalrpop +++ b/core/src/parser/grammar.lalrpop @@ -55,7 +55,7 @@ use crate::{ mk_opn, mk_fun, identifier::LocIdent, - destructuring::{Match, FieldPattern, LastMatch, RecordPattern}, + destructuring::*, term::{ *, record::{RecordAttrs, Field, FieldMetadata}, @@ -513,52 +513,85 @@ FieldPathElem: FieldPathElem = { }; // Last field of a pattern -LastMatch: LastMatch = { - Match => LastMatch::Match(Box::new(<>)), - ".." => LastMatch::Ellipsis(<>), +LastFieldPat: LastPattern = { + FieldPattern => LastPattern::Normal(Box::new(<>)), + ".." => LastPattern::Ellipsis(<>), }; // The right hand side of an `=` inside a destructuring pattern. #[inline] -Pattern: FieldPattern = { - "@")?> => { - if let Some(id) = id { - FieldPattern::AliasedRecordPattern { alias: id, pattern: pat } +Pattern: LocPattern = { + "@")?> => { + let record_pat_span = record_pat.span; + let record_pat = Pattern::RecordPattern(record_pat); + + let pattern = if let Some(alias) = id { + Pattern::AliasedPattern { + alias, + pattern: Box::new(LocPattern { + pattern: record_pat, + span: record_pat_span, + }), + } } else { - FieldPattern::RecordPattern(pat) - } + record_pat + }; + + LocPattern { pattern, span: mk_span(src_id, l, r) } + }, + Ident => { + let span = <>.pos.unwrap(); + + LocPattern { pattern: Pattern::Any(<>), span } }, - Ident => FieldPattern::Ident(<>), }; // A full pattern at the left-hand side of a destructuring let. RecordPattern: RecordPattern = { - "{" ",")*> "}" =>? { + "{" ",")*> "}" =>? { let (open, rest) = match last { - Some(LastMatch::Match(m)) => { - matches.push(*m); + Some(LastPattern::Normal(m)) => { + field_pats.push(*m); (false,None) }, - Some(LastMatch::Ellipsis(rest)) => (true, rest), + Some(LastPattern::Ellipsis(rest)) => (true, rest), _ => (false, None), }; let span = mk_span(src_id, start, end); - let pattern = RecordPattern{ matches, open, rest, span }; + let pattern = RecordPattern { patterns: field_pats, open, rest, span }; pattern.check_matches()?; Ok(pattern) }, }; // A binding `ident = ` inside a destructuring pattern. -Match: Match = { - ?> "=" => { - let field = metadata_with_default(anns, default); - Match::Assign(left, field, right) +FieldPattern: FieldPattern = { + ?> + "=" => { + let decoration = metadata_with_default(anns, default); + + FieldPattern { + matched_id, + decoration, + pattern, + span: mk_span(src_id, l, r), + } }, - ?> => { - let field = metadata_with_default(anns, default); - Match::Simple(id, field) + ?> => { + let decoration = metadata_with_default(anns, default); + + FieldPattern { + matched_id, + decoration, + pattern: LocPattern { + pattern: Pattern::Any(matched_id), + //unwrap(): the position of an parsed identifier should always + //be defined + span: matched_id.pos.unwrap(), + }, + span: mk_span(src_id, l, r) + } }, }; diff --git a/core/src/parser/utils.rs b/core/src/parser/utils.rs index baa0a2065..10d9141e6 100644 --- a/core/src/parser/utils.rs +++ b/core/src/parser/utils.rs @@ -10,7 +10,7 @@ use super::error::ParseError; use crate::{ combine::Combine, - destructuring::FieldPattern, + destructuring::{LocPattern, Pattern}, eval::{ merge::{merge_doc, split}, operation::RecPriority, @@ -651,35 +651,33 @@ pub fn mk_merge_label(src_id: FileId, l: usize, r: usize) -> MergeLabel { /// and is recursive because recursive let-patterns are currently not supported. pub fn mk_let( rec: bool, - assgn: FieldPattern, + pat: LocPattern, t1: RichTerm, t2: RichTerm, span: RawSpan, ) -> Result { - match assgn { - FieldPattern::Ident(id) if rec => Ok(mk_term::let_rec_in(id, t1, t2)), - FieldPattern::Ident(id) => Ok(mk_term::let_in(id, t1, t2)), + match pat.pattern { + Pattern::Any(id) if rec => Ok(mk_term::let_rec_in(id, t1, t2)), + Pattern::Any(id) => Ok(mk_term::let_in(id, t1, t2)), _ if rec => Err(ParseError::RecursiveLetPattern(span)), - FieldPattern::RecordPattern(pat) => { + Pattern::AliasedPattern { alias, pattern } => { + Ok(mk_term::let_pat(Some(alias), *pattern, t1, t2)) + } + _ => { let id: Option = None; Ok(mk_term::let_pat(id, pat, t1, t2)) } - FieldPattern::AliasedRecordPattern { alias, pattern } => { - Ok(mk_term::let_pat(Some(alias), pattern, t1, t2)) - } } } /// Generate a `Fun` or a `FunPattern` (depending on `assgn` having a pattern or not) /// from the parsing of a function definition. This function panics if the definition /// somehow has neither an `Ident` nor a non-`Empty` `Destruct` pattern. -pub fn mk_fun(assgn: FieldPattern, body: RichTerm) -> Term { - match assgn { - FieldPattern::Ident(id) => Term::Fun(id, body), - FieldPattern::RecordPattern(pat) => Term::FunPattern(None, pat, body), - FieldPattern::AliasedRecordPattern { alias, pattern } => { - Term::FunPattern(Some(alias), pattern, body) - } +pub fn mk_fun(pat: LocPattern, body: RichTerm) -> Term { + match pat.pattern { + Pattern::Any(id) => Term::Fun(id, body), + Pattern::AliasedPattern { alias, pattern } => Term::FunPattern(Some(alias), *pattern, body), + _ => Term::FunPattern(None, pat, body), } } diff --git a/core/src/pretty.rs b/core/src/pretty.rs index 317114215..932804957 100644 --- a/core/src/pretty.rs +++ b/core/src/pretty.rs @@ -1,6 +1,6 @@ use std::fmt; -use crate::destructuring::{self, FieldPattern, RecordPattern}; +use crate::destructuring::{self, Pattern, RecordPattern}; use crate::identifier::LocIdent; use crate::parser::lexer::KEYWORDS; use crate::term::record::RecordData; @@ -539,7 +539,7 @@ where } } -impl<'a, D, A> Pretty<'a, D, A> for &FieldPattern +impl<'a, D, A> Pretty<'a, D, A> for &Pattern where D: NickelAllocatorExt<'a, A>, D::Doc: Clone, @@ -547,10 +547,10 @@ where { fn pretty(self, allocator: &'a D) -> DocBuilder<'a, D, A> { match self { - FieldPattern::Ident(id) => allocator.as_string(id), - FieldPattern::RecordPattern(rp) => rp.pretty(allocator), - FieldPattern::AliasedRecordPattern { alias, pattern } => { - docs![allocator, alias.to_string(), " @ ", pattern] + Pattern::Any(id) => allocator.as_string(id), + Pattern::RecordPattern(rp) => rp.pretty(allocator), + Pattern::AliasedPattern { alias, pattern } => { + docs![allocator, alias.to_string(), " @ ", &pattern.pattern] } } } @@ -564,7 +564,7 @@ where { fn pretty(self, allocator: &'a D) -> DocBuilder<'a, D, A> { let RecordPattern { - matches, + patterns: matches, open, rest, .. @@ -573,17 +573,11 @@ where allocator, allocator.line(), allocator.intersperse( - matches.iter().map(|m| { - let (id, field, pattern_opt) = match m { - destructuring::Match::Simple(id, field) => (id, field, None), - destructuring::Match::Assign(id, field, pattern) => { - (id, field, Some(pattern)) - } - }; + matches.iter().map(|field_pat| { docs![ allocator, - id.to_string(), - match field { + field_pat.matched_id.to_string(), + match &field_pat.decoration { Field { value: Some(value), metadata: @@ -604,13 +598,14 @@ where ), allocator.line(), "? ", - allocator.atom(value), + allocator.atom(&value), ], - _ => allocator.field_metadata(&field.metadata, false), + field => allocator.field_metadata(&field.metadata, false), }, - match pattern_opt { - Some(pattern) => docs![allocator, allocator.line(), "= ", pattern], - _ => allocator.nil(), + match &field_pat.pattern.pattern { + destructuring::Pattern::Any(id) if *id == field_pat.matched_id => + allocator.nil(), + pat => docs![allocator, allocator.line(), "= ", pat], }, "," ] @@ -692,11 +687,11 @@ where FunPattern(..) => { let mut params = vec![]; let mut rt = self; - while let FunPattern(id, dst, t) = rt { + while let FunPattern(id, pat, t) = rt { params.push(if let Some(id) = id { - docs![allocator, id.to_string(), " @ ", dst] + docs![allocator, id.to_string(), " @ ", &pat.pattern] } else { - dst.pretty(allocator) + pat.pattern.pretty(allocator) }); rt = t.as_ref(); } @@ -750,7 +745,7 @@ where } else { allocator.nil() }, - pattern, + &pattern.pattern, if let Annotated(annot, _) = rt.as_ref() { annot.pretty(allocator) } else { diff --git a/core/src/term/mod.rs b/core/src/term/mod.rs index 6396895bb..49f2a22b1 100644 --- a/core/src/term/mod.rs +++ b/core/src/term/mod.rs @@ -18,7 +18,7 @@ use record::{Field, FieldDeps, FieldMetadata, RecordData, RecordDeps}; use string::NickelString; use crate::{ - destructuring::RecordPattern, + destructuring::LocPattern, error::{EvalError, ParseError}, eval::cache::CacheIndex, eval::Environment, @@ -95,7 +95,7 @@ pub enum Term { /// A function able to destruct its arguments. #[serde(skip)] - FunPattern(Option, RecordPattern, RichTerm), + FunPattern(Option, LocPattern, RichTerm), /// A blame label. #[serde(skip)] @@ -107,7 +107,7 @@ pub enum Term { /// A destructuring let-binding. #[serde(skip)] - LetPattern(Option, RecordPattern, RichTerm, RichTerm), + LetPattern(Option, LocPattern, RichTerm, RichTerm), /// An application. #[serde(skip)] @@ -2482,7 +2482,7 @@ pub mod make { where T1: Into, T2: Into, - D: Into, + D: Into, I: Into, { Term::LetPattern(id.map(|i| i.into()), pat.into(), t1.into(), t2.into()).into() diff --git a/core/src/transform/desugar_destructuring.rs b/core/src/transform/desugar_destructuring.rs index effea3f8a..4ba8e2a34 100644 --- a/core/src/transform/desugar_destructuring.rs +++ b/core/src/transform/desugar_destructuring.rs @@ -1,16 +1,19 @@ -//! Desugar destructuring +//! Destructuring desugaring //! -//! Replace a let-binding with destructuring by a classical let-binding. -//! It will first destruct the pattern and create a new var for each field of the pattern. -//! After that, it will construct a new Record/Array from the extracted fields. +//! Replace a let-binding with destructuring by a sequence of normal let-binding. //! //! # Example //! -//! ## The let pattern: +//! ## Let-binding +//! +//! The following destruring let-binding: +//! //! ```text //! let x @ {a, b=d, ..} = {a=1,b=2,c="ignored"} in ... //! ``` +//! //! will be transformed to: +//! //! ```text //! let x = {a=1,b=2,c="ignored"} in //! let a = x.a in @@ -18,11 +21,16 @@ //! ... //! ``` //! -//! ## The function pattern +//! ## Function +//! +//! The following desctructuring function: +//! //! ```text //! let f = fun x@{a, b=c} {d ? 2, ..w} => in ... //! ``` +//! //! will be transformed to: +//! //! ```text //! let f = fun x %unnamed% => ( //! let {a, b=c} = x in @@ -30,193 +38,185 @@ //! //! ) in ... //! ``` -use crate::destructuring::{FieldPattern, Match, RecordPattern}; +use crate::destructuring::*; use crate::identifier::LocIdent; use crate::match_sharedterm; use crate::term::{ make::{op1, op2}, BinaryOp::DynRemove, - BindingType, LetAttrs, RecordOpKind, RichTerm, Term, TypeAnnotation, + LetAttrs, RecordOpKind, RichTerm, Term, TypeAnnotation, UnaryOp::StaticAccess, }; -/// Entry point of the patterns desugaring. -/// It desugar a `RichTerm` if possible (the term is a let pattern or a function with patterns in -/// its arguments). -/// ## Warning: -/// The transformation is generally not recursive. The result can contain patterns itself. +/// Entry point of the destructuring desugaring transformation. +/// +/// As other `transform_one` variants, this transformation is not recursive and only desugars the +/// top-level constructor of the pattern. It might return a term which still contains simpler +/// destructuring patterns to be desugared in children nodes. pub fn transform_one(rt: RichTerm) -> RichTerm { - match *rt.term { - Term::LetPattern(..) => desugar_with_contract(rt), - Term::FunPattern(..) => desugar_fun(rt), - _ => rt, - } -} - -/// Desugar a function with patterns as arguments. -/// This function does not perform nested transformation because internally it's only used in a top -/// down traversal. This means that the return value is a normal `Term::Fun` but it can contain -/// `Term::FunPattern` and `Term::LetPattern` inside. -pub fn desugar_fun(rt: RichTerm) -> RichTerm { match_sharedterm!(match (rt.term) { - Term::FunPattern(x, pat, t_) => { - let x = x.unwrap_or_else(LocIdent::fresh); - let t_pos = t_.pos; - RichTerm::new( - Term::Fun( - x, - RichTerm::new( - Term::LetPattern(None, pat, Term::Var(x).into(), t_), - t_pos, /* TODO: should we use rt.pos? */ - ), - ), - rt.pos, - ) - } + Term::LetPattern(id, pat, bound, body) => + RichTerm::new(desugar_let(id, pat, bound, body), rt.pos), + Term::FunPattern(id, pat, body) => RichTerm::new(desugar_fun(id, pat, body), rt.pos), _ => rt, }) } -/// Wrap the desugar `LetPattern` in a meta value containing the "Record contract" needed to check -/// the pattern exhaustively and also fill the default values (`?` operator) if not presents in the -/// record. This function should be, in the general case, considered as the entry point of the let -/// patterns transformation. -pub fn desugar_with_contract(rt: RichTerm) -> RichTerm { - match_sharedterm!(match (rt.term) { - Term::LetPattern(x, pat, bound, body) => { - let pos = body.pos; - let contract = pat.clone().into_contract(); - let annotated = { - let t_pos = bound.pos; - RichTerm::new( - Term::Annotated( - TypeAnnotation { - contracts: vec![contract], - ..Default::default() - }, - bound, - ), - t_pos, - ) - }; - desugar(RichTerm::new( - Term::LetPattern(x, pat, annotated, body), - pos, - )) +/// Desugar a destructuring function. +/// +/// A function `fun => body` is desugared to `fun x => let = x in body`. The inner +/// destructuring let isn't desugared further, as the general program transformation machinery will +/// take care of transforming the body of the function in a second step. +pub fn desugar_fun(id: Option, pat: LocPattern, body: RichTerm) -> Term { + let id = id.unwrap_or_else(LocIdent::fresh); + let pos_body = body.pos; + + Term::Fun( + id, + RichTerm::new( + Term::LetPattern(None, pat, Term::Var(id).into(), body), + // TODO: should we use rt.pos? + pos_body, + ), + ) +} + +/// Elaborate a contract from the pattern if it is a record pattern and applies to the value before +/// actually destructuring it. Then convert the let pattern to a sequence of normal let-bindings. +pub fn desugar_let(id: Option, pat: LocPattern, bound: RichTerm, body: RichTerm) -> Term { + let contract = pat.elaborate_contract(); + + let annotated = { + let t_pos = bound.pos; + RichTerm::new( + Term::Annotated( + TypeAnnotation { + contracts: contract.into_iter().collect(), + ..Default::default() + }, + bound, + ), + t_pos, + ) + }; + + let pat = if let Some(alias) = id { + Pattern::AliasedPattern { + alias, + pattern: Box::new(pat), } - _ => rt, - }) + } else { + pat.pattern + }; + + pat.desugar(annotated, body) } -/// Main transformation function to desugar let patterns. WARNING: In a real usage case, you will -/// want to generate also the contract associated to this pattern destructuring. Do not consider -/// this function as the entry point of the transformation. For that, use `desugar_with_contract`. -pub fn desugar(rt: RichTerm) -> RichTerm { - match_sharedterm!(match (rt.term) { - Term::LetPattern(x, pat, t_, body) => { - let pos = body.pos; - let x = x.unwrap_or_else(LocIdent::fresh); - RichTerm::new( - Term::Let( - x, - t_, - destruct_term(x, &pat, bind_open_field(x, &pat, body)), - Default::default(), - ), - pos, - ) +trait Desugar { + /// Elaborate a destructuring let-binding matching a pattern `self` against a value `destr` to + /// a sequence of normal let-bindings and primitive operations. + /// + /// This function ignores the user-supplied contracts of the pattern and doesn't generate a + /// safety contract to check that the value has the expected shape: this guarding contract must + /// be introduced separately and prior to calling to [Desugar::desugar]. In practice, this + /// contract is introduced by [desugar_let]. [Desugar::desugar] is only concerned with + /// destructuring the value and binding its parts to appropriate variables. + fn desugar(self, destr: RichTerm, body: RichTerm) -> Term; +} + +impl Desugar for LocPattern { + fn desugar(self, destr: RichTerm, body: RichTerm) -> Term { + self.pattern.desugar(destr, body) + } +} + +impl Desugar for Pattern { + fn desugar(self, destr: RichTerm, body: RichTerm) -> Term { + match self { + // If the pattern is an unconstrained identifier, we just bind it to the value. + Pattern::Any(id) => Term::Let(id, destr, body, LetAttrs::default()), + Pattern::RecordPattern(pat) => pat.desugar(destr, body), + // If the pattern is aliased, `x @ ` matching `destruct`, we introduce a heading + // let-binding `let x = destruct in `, where `` is the + // desugaring of `` matching `x`. + Pattern::AliasedPattern { alias, pattern } => { + let pos = body.pos; + let inner = RichTerm::new( + pattern.desugar(RichTerm::new(Term::Var(alias), alias.pos), body), + pos, + ); + + Term::Let(alias, destr, inner, LetAttrs::default()) + } } - _ => rt, - }) + } +} + +impl Desugar for FieldPattern { + // For a filed pattern, we assume that the `destr` argument is the whole record being + // destructured. We extract the field from `destr` and desugar the rest of the pattern against + // `destr.matched_id`. + fn desugar(self, destr: RichTerm, body: RichTerm) -> Term { + let extracted = op1(StaticAccess(self.matched_id), destr.clone()); + self.pattern.desugar(extracted, body) + } +} + +impl Desugar for RecordPattern { + fn desugar(self, destr: RichTerm, body: RichTerm) -> Term { + let pos = body.pos; + // The body is the rest of the term being transformed, which contains the code that uses + // the bindings introduced by the pattern. After having extracting all fields from the + // value, we potentially need to capture the rest in a variable for patterns with a + // capturing tail. For example, `let {foo, bar, ..rest} = destr in body` should be + // desugared as `let foo = destr.foo in let bar = destr.bar in let rest = in body` + // (where `` is some expression removing `foo` and `bar` from `destr`). + // + // Because body is the continuation, we need to first append the rest capture to the + // original body before passing it to the [Desugar::desugar] of each individual field. + let body_with_rest = bind_rest(&self, destr.clone(), body); + + self.patterns + .into_iter() + .fold(body_with_rest, |acc, field_pat| { + RichTerm::new(field_pat.desugar(destr.clone(), acc), pos) + }) + .term + .into_owned() + } } -/// Wrap `body` in a let construct binding the open part of the pattern to the required value. -/// Having `let {a,..y} = {a=1, b=2, c=3} in ` will bind `y` to `{b=2,c=3}` in `BODY`. Here, -/// `x` is the identifier pointing to the full record. If having `val @ {...} = ... in ...` the -/// variable x should be `Ident("val")` but if we have a `@` binding less form, you will probably -/// generate a fresh variable. -fn bind_open_field(x: LocIdent, pat: &RecordPattern, body: RichTerm) -> RichTerm { - let (matches, var) = match pat { +fn bind_rest(pat: &RecordPattern, destr: RichTerm, body: RichTerm) -> RichTerm { + let capture_var = match pat { RecordPattern { - matches, open: true, rest: Some(x), .. - } => (matches, *x), + } => *x, RecordPattern { - matches, open: true, rest: None, .. - } => (matches, LocIdent::fresh()), - RecordPattern { + } + | RecordPattern { open: false, rest: None, .. } => return body, _ => panic!("A closed pattern can not have a rest binding"), }; + Term::Let( - var, - matches.iter().fold(Term::Var(x).into(), |x, m| match m { - Match::Simple(i, _) | Match::Assign(i, _, _) => op2( + capture_var, + pat.patterns.iter().fold(destr, |acc, field_pat| { + op2( DynRemove(RecordOpKind::default()), - Term::Str((*i).into()), - x, - ), + Term::Str(field_pat.matched_id.ident().into()), + acc, + ) }), body, Default::default(), ) .into() } - -/// Core of the destructuring. Bind all the variables of the pattern except the "open" (`..y`) -/// part. For that, see `bind_open_field`. -fn destruct_term(x: LocIdent, pat: &RecordPattern, body: RichTerm) -> RichTerm { - let pos = body.pos; - let RecordPattern { matches, .. } = pat; - matches.iter().fold(body, move |t, m| match m { - Match::Simple(id, _) => RichTerm::new( - Term::Let( - *id, - op1(StaticAccess(*id), Term::Var(x)), - t, - Default::default(), - ), - pos, - ), - Match::Assign(f, _, FieldPattern::Ident(id)) => desugar(RichTerm::new( - Term::Let( - *id, - op1(StaticAccess(*f), Term::Var(x)), - t, - LetAttrs { - binding_type: BindingType::Normal, - rec: false, - }, - ), - pos, - )), - Match::Assign(f, _, FieldPattern::RecordPattern(pattern)) => desugar(RichTerm::new( - Term::LetPattern( - None, - pattern.clone(), - op1(StaticAccess(*f), Term::Var(x)), - t, - ), - pos, - )), - Match::Assign(f, _, FieldPattern::AliasedRecordPattern { alias, pattern }) => { - desugar(RichTerm::new( - Term::LetPattern( - Some(*alias), - pattern.clone(), - op1(StaticAccess(*f), Term::Var(x)), - t, - ), - pos, - )) - } - }) -} diff --git a/core/src/transform/free_vars.rs b/core/src/transform/free_vars.rs index 8b633c9a0..302a35df0 100644 --- a/core/src/transform/free_vars.rs +++ b/core/src/transform/free_vars.rs @@ -4,7 +4,7 @@ //! the recursive fields that actually appear in the definition of each field when computing the //! fixpoint. use crate::{ - destructuring::{FieldPattern, Match, RecordPattern}, + destructuring::{FieldPattern, LocPattern, Pattern, RecordPattern}, identifier::Ident, term::{ record::{Field, FieldDeps, RecordDeps}, @@ -51,11 +51,11 @@ impl CollectFreeVars for RichTerm { free_vars.extend(fresh); } - Term::FunPattern(id, dest_pat, body) => { + Term::FunPattern(id, pat, body) => { let mut fresh = HashSet::new(); body.collect_free_vars(&mut fresh); - bind_pattern(dest_pat, &mut fresh); + pat.remove_bindings(&mut fresh); if let Some(id) = id { fresh.remove(&id.ident()); } @@ -76,12 +76,12 @@ impl CollectFreeVars for RichTerm { free_vars.extend(fresh); } - Term::LetPattern(id, dest_pat, t1, t2) => { + Term::LetPattern(id, pat, t1, t2) => { let mut fresh = HashSet::new(); t1.collect_free_vars(free_vars); t2.collect_free_vars(&mut fresh); - bind_pattern(dest_pat, &mut fresh); + pat.remove_bindings(&mut fresh); if let Some(id) = id { fresh.remove(&id.ident()); } @@ -234,41 +234,49 @@ impl CollectFreeVars for Field { } } -/// Remove the variables bound by a destructuring pattern from a set of free variables. -fn bind_pattern(dest_pat: &RecordPattern, free_vars: &mut HashSet) { - let RecordPattern { matches, rest, .. } = dest_pat; - for m in matches { - bind_match(m, free_vars); +trait RemoveBindings { + /// For a binding form that introduces new variables in scope, typically patterns, remove the + /// variable introduced by this binding form from the provided set of free variables. + fn remove_bindings(&self, working_set: &mut HashSet); +} + +impl RemoveBindings for Pattern { + fn remove_bindings(&self, working_set: &mut HashSet) { + match self { + Pattern::Any(id) => { + working_set.remove(&id.ident()); + } + Pattern::RecordPattern(record_pat) => { + record_pat.remove_bindings(working_set); + } + Pattern::AliasedPattern { alias, pattern } => { + working_set.remove(&alias.ident()); + pattern.remove_bindings(working_set); + } + } } +} - if let Some(rest) = rest { - free_vars.remove(&rest.ident()); +impl RemoveBindings for LocPattern { + fn remove_bindings(&self, working_set: &mut HashSet) { + self.pattern.remove_bindings(working_set); } } -/// Remove the variables bound by a match expression (constituents of a destructuring pattern) from -/// a set of free variables. -fn bind_match(m: &Match, free_vars: &mut HashSet) { - match m { - Match::Assign(_, _, FieldPattern::Ident(ident)) => { - free_vars.remove(&ident.ident()); - } - Match::Assign(_, _, FieldPattern::RecordPattern(sub_pattern)) => { - bind_pattern(sub_pattern, free_vars); - } - Match::Assign( - _, - _, - FieldPattern::AliasedRecordPattern { - alias, - pattern: sub_pattern, - }, - ) => { - free_vars.remove(&alias.ident()); - bind_pattern(sub_pattern, free_vars); +impl RemoveBindings for FieldPattern { + fn remove_bindings(&self, working_set: &mut HashSet) { + self.pattern.remove_bindings(working_set); + } +} + +impl RemoveBindings for RecordPattern { + fn remove_bindings(&self, working_set: &mut HashSet) { + for m in &self.patterns { + m.remove_bindings(working_set); } - Match::Simple(id, _) => { - free_vars.remove(&id.ident()); + + if let Some(rest) = self.rest { + working_set.remove(&rest.ident()); } } } diff --git a/core/src/typecheck/destructuring.rs b/core/src/typecheck/destructuring.rs index d43102a03..a966d041d 100644 --- a/core/src/typecheck/destructuring.rs +++ b/core/src/typecheck/destructuring.rs @@ -1,280 +1,174 @@ use crate::{ - destructuring::{FieldPattern, Match, RecordPattern}, + destructuring::{FieldPattern, Pattern, RecordPattern}, error::TypecheckError, identifier::LocIdent, mk_uty_record_row, - term::{IndexMap, LabeledType}, typ::{RecordRowF, RecordRowsF, TypeF}, - typecheck::{UnifRecordRow, Unify}, }; use super::{ - mk_uniftype, Context, Environment, GenericUnifRecordRowsIteratorItem, State, UnifRecordRows, - UnifType, VarLevelsData, + mk_uniftype, Context, State, UnifRecordRow, UnifRecordRows, UnifType, Unify, VarLevelsData, }; -pub fn build_pattern_type_walk_mode( - state: &mut State, - ctxt: &Context, - pat: &RecordPattern, -) -> Result { - build_pattern_type(state, ctxt, pat, TypecheckMode::Walk) -} - -pub fn build_pattern_type_check_mode( - state: &mut State, - ctxt: &Context, - pat: &RecordPattern, -) -> Result { - build_pattern_type(state, ctxt, pat, TypecheckMode::Check) -} - #[derive(Debug, Clone, Copy, PartialEq, Eq)] -enum TypecheckMode { +pub(super) enum TypecheckMode { Walk, - Check, + Enforce, } -/// Build a `UnifType` from a `Destruct` pattern. The type of each "leaf" -/// identifier will be assigned based on the `mode` argument. The -/// current possibilities are for each leaf to have type `Dyn`, to use an -/// explicit type annotation, or to be assigned a fresh unification variable. -fn build_pattern_type( - state: &mut State, - ctxt: &Context, - pat: &RecordPattern, - mode: TypecheckMode, -) -> Result { - fn new_leaf_type( +pub(super) trait PatternTypes { + /// The type produced by the pattern. Depending on the nature of the pattern, this type may + /// vary: for example, a record pattern will record rows, while a general pattern will produce + /// a general [super::UnifType] + type PatType; + + /// Builds the type associated to the whole pattern, as well as the types associated to each + /// binding introduced by this pattern. When matching a value against a pattern in a statically + /// typed code, either by destructuring or by applying a match expression, the type of the + /// value will be checked against the type generated by `pattern_type` and the bindings will be + /// added to the type environment. + /// + /// The type of each "leaf" identifier will be assigned based on the `mode` argument. The + /// current possibilities are for each leaf to have type `Dyn`, to use an explicit type + /// annotation, or to be assigned a fresh unification variable. + fn pattern_types( + &self, state: &mut State, ctxt: &Context, mode: TypecheckMode, - ty_annot: Option, - ) -> UnifType { - match mode { - TypecheckMode::Walk => mk_uniftype::dynamic(), - TypecheckMode::Check => { - if let Some(l_ty) = ty_annot { - UnifType::from_type(l_ty.typ, &ctxt.term_env) - } else { - state.table.fresh_type_uvar(ctxt.var_level) - } - } - } + ) -> Result<(Self::PatType, Vec<(LocIdent, UnifType)>), TypecheckError> { + let mut bindings = Vec::new(); + let typ = self.pattern_types_inj(&mut bindings, state, ctxt, mode)?; + Ok((typ, bindings)) } - let tail = if pat.open { - match mode { - // We use a dynamic tail here since we're in walk mode, - // but if/when we remove dynamic record tails this could - // likely be made an empty tail with no impact. - TypecheckMode::Walk => mk_uty_record_row!(; RecordRowsF::TailDyn), - TypecheckMode::Check => state.table.fresh_rrows_uvar(ctxt.var_level), - } - } else { - UnifRecordRows::Concrete { - rrows: RecordRowsF::Empty, - var_levels_data: VarLevelsData::new_no_uvars(), - } - }; - - let mut rows = pat.matches.iter().map(|m| match m { - Match::Simple(id, field) => Ok(RecordRowF { - id: *id, - typ: Box::new(new_leaf_type( - state, - ctxt, - mode, - field.metadata.annotation.typ.clone(), - )), - }), - Match::Assign(id, field, FieldPattern::Ident(_)) => Ok(RecordRowF { - id: *id, - typ: Box::new(new_leaf_type( - state, - ctxt, - mode, - field.metadata.annotation.typ.clone(), - )), - }), - Match::Assign( - id, - field, - FieldPattern::RecordPattern(r_pat) - | FieldPattern::AliasedRecordPattern { pattern: r_pat, .. }, - ) => { - let row_tys = build_pattern_type(state, ctxt, r_pat, mode)?; - let ty = UnifType::concrete(TypeF::Record(row_tys)); - - // If there are type annotations within nested record patterns - // then we need to unify them with the pattern type we've built - // to ensure (1) that they're mutually compatible and (2) that - // we assign the annotated types to the right unification variables. - if let Some(annot_ty) = &field.metadata.annotation.typ { - let pos = annot_ty.typ.pos; - let annot_uty = UnifType::from_type(annot_ty.typ.clone(), &ctxt.term_env); - ty.clone() - .unify(annot_uty, state, ctxt) - .map_err(|e| e.into_typecheck_err(state, pos))?; - } - - Ok(RecordRowF { - id: *id, - typ: Box::new(ty), - }) - } - }); - - rows.try_fold(tail, |tail, row: Result| { - Ok(UnifRecordRows::concrete(RecordRowsF::Extend { - row: row?, - tail: Box::new(tail), - })) - }) + /// Same as `pattern_types`, but inject the bindings in a working vector instead of returning + /// them. Implementors should implement this method whose signature avoid creating and + /// combining many short-lived vectors when walking recursively through a pattern. + fn pattern_types_inj( + &self, + bindings: &mut Vec<(LocIdent, UnifType)>, + state: &mut State, + ctxt: &Context, + mode: TypecheckMode, + ) -> Result; } -/// Extend `env` with any new bindings brought into scope in `pat`. The -/// types of these bindings will be inferred from `pat_ty`. +/// Builds the type associated to a record pattern. When matching a value against a pattern in a +/// statically typed code, for example in a let destructuring or via a match expression, the type +/// of the value will be checked against the type generated by `build_pattern_type`. /// -/// For example, if `pat` represents the pattern `{ a, ..rest }` and -/// `pat_ty` is `{ a : Num, b : Str }` then the `env` will be extended -/// with `a : Num` and `rest : { b : Str }`. -pub fn inject_pattern_variables( - state: &State, - env: &mut Environment, - pat: &RecordPattern, - pat_ty: UnifRecordRows, -) { - let pat_ty = pat_ty.into_root(state.table); - let mut type_map = RecordTypes::from(&pat_ty); - - pat.matches.iter().for_each(|m| match m { - Match::Simple(id, ..) => { - let ty = type_map.get_type(id); - env.insert(id.ident(), ty); - } - Match::Assign(id, _, FieldPattern::Ident(bind_id)) => { - let ty = type_map.get_type(id); - env.insert(bind_id.ident(), ty); - } - Match::Assign(id, _, FieldPattern::RecordPattern(pat)) => { - let ty = type_map.get_type(id); - - // Since we don't have a `bind_id` in this branch, - // we can infer that `id` is an intermediate value that - // isn't accessible from the code. e.g. the `foo` in a - // binding like: - // - // ``` - // let { foo = { bar = baz } } = { foo.bar = 1 } in ... - // ``` - // - // As such, we don't need to add it to the environment. - let UnifType::Concrete { - typ: TypeF::Record(rs), - .. - } = ty - else { - unreachable!( - "since this is a destructured record, \ - its type was constructed by build_pattern_ty, \ - which means it must be a concrete record type" - ) - }; - inject_pattern_variables(state, env, pat, rs) - } - Match::Assign(id, _, FieldPattern::AliasedRecordPattern { alias, pattern }) => { - let ty = type_map.get_type(id); - - env.insert(alias.ident(), ty.clone()); +/// The type of each "leaf" identifier will be assigned based on the `mode` argument. The current +/// possibilities are for each leaf to have type `Dyn`, to use an explicit type annotation, or to +/// be assigned a fresh unification variable. +impl PatternTypes for RecordPattern { + type PatType = UnifRecordRows; + + fn pattern_types_inj( + &self, + bindings: &mut Vec<(LocIdent, UnifType)>, + state: &mut State, + ctxt: &Context, + mode: TypecheckMode, + ) -> Result { + let tail = if self.open { + match mode { + // We use a dynamic tail here since we're in walk mode, + // but if/when we remove dynamic record tails this could + // likely be made an empty tail with no impact. + TypecheckMode::Walk => mk_uty_record_row!(; RecordRowsF::TailDyn), + TypecheckMode::Enforce => state.table.fresh_rrows_uvar(ctxt.var_level), + } + } else { + UnifRecordRows::Concrete { + rrows: RecordRowsF::Empty, + var_levels_data: VarLevelsData::new_no_uvars(), + } + }; - let UnifType::Concrete { - typ: TypeF::Record(rs), - .. - } = ty - else { - unreachable!( - "since this is a destructured record, \ - its type was constructed by build_pattern_ty, \ - which means it must be a concrete record type" - ) - }; - inject_pattern_variables(state, env, pattern, rs) + if let Some(rest) = self.rest { + bindings.push((rest, UnifType::concrete(TypeF::Record(tail.clone())))); } - }); - if let Some(id) = pat.rest { - let rest_ty = type_map.rest(); - env.insert(id.ident(), rest_ty); + self.patterns + .iter() + .map(|field_pat| field_pat.pattern_types_inj(bindings, state, ctxt, mode)) + .try_fold(tail, |tail, row: Result| { + Ok(UnifRecordRows::concrete(RecordRowsF::Extend { + row: row?, + tail: Box::new(tail), + })) + }) } } -/// A map of identifiers in a destructured record to their types. -/// -/// This allows us to be resilient to ordering differences between the -/// pattern and its type. As well as keeping track of which identifiers -/// have already been "used" in the pattern, to ensure that we can -/// correctly construct the type of a `..rest` match, if it exists. -struct RecordTypes { - known_types: IndexMap, - tail: UnifRecordRows, -} +impl PatternTypes for Pattern { + type PatType = UnifType; -impl From<&UnifRecordRows> for RecordTypes { - fn from(u: &UnifRecordRows) -> Self { - let (known_types, tail) = - u.iter() - .fold((IndexMap::new(), None), |(mut m, _), ty| match ty { - GenericUnifRecordRowsIteratorItem::Row(rt) => { - m.insert(rt.id, rt.typ.clone()); - (m, None) - } - GenericUnifRecordRowsIteratorItem::TailDyn => { - (m, Some(UnifRecordRows::concrete(RecordRowsF::TailDyn))) - } - GenericUnifRecordRowsIteratorItem::TailVar(v) => { - (m, Some(UnifRecordRows::concrete(RecordRowsF::TailVar(*v)))) - } - GenericUnifRecordRowsIteratorItem::TailUnifVar { id, init_level } => { - (m, Some(UnifRecordRows::UnifVar { id, init_level })) - } - GenericUnifRecordRowsIteratorItem::TailConstant(n) => { - (m, Some(UnifRecordRows::Constant(n))) - } - }); - RecordTypes { - known_types, - tail: tail.unwrap_or(UnifRecordRows::concrete(RecordRowsF::Empty)), + fn pattern_types_inj( + &self, + bindings: &mut Vec<(LocIdent, UnifType)>, + state: &mut State, + ctxt: &Context, + mode: TypecheckMode, + ) -> Result { + match self { + Pattern::Any(id) => { + let typ = match mode { + TypecheckMode::Walk => mk_uniftype::dynamic(), + TypecheckMode::Enforce => state.table.fresh_type_uvar(ctxt.var_level), + }; + bindings.push((*id, typ.clone())); + + Ok(typ) + } + Pattern::RecordPattern(record_pat) => Ok(UnifType::concrete(TypeF::Record( + record_pat.pattern_types_inj(bindings, state, ctxt, mode)?, + ))), + Pattern::AliasedPattern { alias, pattern } => { + let typ = pattern + .pattern + .pattern_types_inj(bindings, state, ctxt, mode)?; + + bindings.push((*alias, typ.clone())); + Ok(typ) + } } } } -impl RecordTypes { - /// Returns the type of the identifier `id` in the record. - /// - /// In the case of `RecordTypes::Rows`, `id` is also removed from the - /// map, so that it won't be considered as part of the "tail type" - /// when `rest` is called. - fn get_type(&mut self, id: &LocIdent) -> UnifType { - self.known_types - .remove(id) - .expect("Scopes of identifiers in destruct patterns should be checked already") - } +impl PatternTypes for FieldPattern { + type PatType = UnifRecordRow; - /// Returns the "tail type" of the record. I.e., the record's tail - /// plus any "unused" matches from `known_types`. - fn rest(self) -> UnifType { - let Self { known_types, tail } = self; - let rows = known_types.iter().map(|(id, ty)| RecordRowF { - id: *id, - typ: Box::new(ty.clone()), - }); - let rrows = rows.fold(tail, |tail, row| { - UnifRecordRows::concrete(RecordRowsF::Extend { - row, - tail: Box::new(tail), - }) - }); - UnifType::concrete(TypeF::Record(rrows)) + fn pattern_types_inj( + &self, + bindings: &mut Vec<(LocIdent, UnifType)>, + state: &mut State, + ctxt: &Context, + mode: TypecheckMode, + ) -> Result { + let ty_row = self + .pattern + .pattern + .pattern_types_inj(bindings, state, ctxt, mode)?; + + // If there are type annotations within nested record patterns + // then we need to unify them with the pattern type we've built + // to ensure (1) that they're mutually compatible and (2) that + // we assign the annotated types to the right unification variables. + if let Some(annot_ty) = &self.decoration.metadata.annotation.typ { + let pos = annot_ty.typ.pos; + let annot_uty = UnifType::from_type(annot_ty.typ.clone(), &ctxt.term_env); + + ty_row + .clone() + .unify(annot_uty, state, ctxt) + .map_err(|e| e.into_typecheck_err(state, pos))?; + } + + Ok(RecordRowF { + id: self.matched_id, + typ: Box::new(ty_row), + }) } } diff --git a/core/src/typecheck/mod.rs b/core/src/typecheck/mod.rs index bfb4c917a..6049a70bd 100644 --- a/core/src/typecheck/mod.rs +++ b/core/src/typecheck/mod.rs @@ -21,9 +21,9 @@ //! to enforce mode, and is switched back to walk mode when entering an expression annotated with a //! contract. Type and contract annotations thus serve as a switch for the typechecking mode. //! -//! Note that the static typing part is based on the bidirectional typing framework, which defines -//! two different modes. Thus, the enforce mode is itself divided again into **checking** mode and -//! **inference** mode. +//! Note that the static typing part (enforce mode) is based on the bidirectional typing framework, +//! which defines two different modes. Thus, the enforce mode is itself divided again into +//! **checking** mode and **inference** mode. //! //! # Type inference //! @@ -83,6 +83,7 @@ pub mod mk_uniftype; pub mod eq; pub mod unif; +use destructuring::PatternTypes; use eq::{SimpleTermEnvironment, TermEnvironment}; use error::*; use indexmap::IndexMap; @@ -1471,8 +1472,8 @@ fn walk( ctxt.type_env.insert(id.ident(), mk_uniftype::dynamic()); } - let pattern_ty = destructuring::build_pattern_type_walk_mode(state, &ctxt, pat)?; - destructuring::inject_pattern_variables(state, &mut ctxt.type_env, pat, pattern_ty); + let (_, pat_bindings) = pat.pattern.pattern_types(state, &ctxt, destructuring::TypecheckMode::Walk)?; + ctxt.type_env.extend(pat_bindings.into_iter().map(|(id, typ)| (id.ident(), typ))); walk(state, ctxt, visitor, t) } Term::Array(terms, _) => terms @@ -1514,13 +1515,12 @@ fn walk( ctxt.type_env.insert(x.ident(), ty_let); } - let pattern_ty = destructuring::build_pattern_type_walk_mode(state, &ctxt, pat)?; - for item in pattern_ty.iter() { - if let GenericUnifRecordRowsIteratorItem::Row(row) = item { - visitor.visit_ident(&row.id, row.typ.clone()); - } + let (_, pat_bindings) = pat.pattern.pattern_types(state, &ctxt, destructuring::TypecheckMode::Walk)?; + + for (id, typ) in pat_bindings { + visitor.visit_ident(&id, typ.clone()); + ctxt.type_env.insert(id.ident(), typ); } - destructuring::inject_pattern_variables(state, &mut ctxt.type_env, pat, pattern_ty); walk(state, ctxt, visitor, rt) } @@ -1819,8 +1819,11 @@ fn check( check(state, ctxt, visitor, t, trg) } Term::FunPattern(x, pat, t) => { - let src_rows_ty = destructuring::build_pattern_type_check_mode(state, &ctxt, pat)?; - let src = UnifType::concrete(TypeF::Record(src_rows_ty.clone())); + let (pat_ty, pat_bindings) = + pat.pattern + .pattern_types(state, &ctxt, destructuring::TypecheckMode::Enforce)?; + + let src = pat_ty; let trg = state.table.fresh_type_uvar(ctxt.var_level); let arr = mk_uty_arrow!(src.clone(), trg.clone()); @@ -1829,7 +1832,11 @@ fn check( ctxt.type_env.insert(x.ident(), src); } - destructuring::inject_pattern_variables(state, &mut ctxt.type_env, pat, src_rows_ty); + for (id, typ) in pat_bindings { + visitor.visit_ident(&id, typ.clone()); + ctxt.type_env.insert(id.ident(), typ); + } + ty.unify(arr, state, &ctxt) .map_err(|err| err.into_typecheck_err(state, rt.pos))?; check(state, ctxt, visitor, t, trg) @@ -1874,15 +1881,16 @@ fn check( } Term::LetPattern(x, pat, re, rt) => { // The inferred type of the pattern w/ unification vars - let pattern_rows_type = - destructuring::build_pattern_type_check_mode(state, &ctxt, pat)?; - let pattern_type = UnifType::concrete(TypeF::Record(pattern_rows_type.clone())); + let (pat_ty, pat_bindings) = + pat.pattern + .pattern_types(state, &ctxt, destructuring::TypecheckMode::Enforce)?; + // The inferred type of the expr being bound let ty_let = binding_type(state, re.as_ref(), &ctxt, true); ty_let .clone() - .unify(pattern_type, state, &ctxt) + .unify(pat_ty, state, &ctxt) .map_err(|e| e.into_typecheck_err(state, re.pos))?; check(state, ctxt.clone(), visitor, re, ty_let.clone())?; @@ -1892,19 +1900,11 @@ fn check( ctxt.type_env.insert(x.ident(), ty_let); } - for item in pattern_rows_type.iter() { - if let GenericUnifRecordRowsIteratorItem::Row(row) = item { - visitor.visit_ident(&row.id, row.typ.clone()); - } + for (id, typ) in pat_bindings { + visitor.visit_ident(&id, typ.clone()); + ctxt.type_env.insert(id.ident(), typ); } - destructuring::inject_pattern_variables( - state, - &mut ctxt.type_env, - pat, - pattern_rows_type, - ); - check(state, ctxt, visitor, rt, ty) } Term::Match { cases, default } => { @@ -2814,7 +2814,7 @@ pub trait TypecheckVisitor { /// inference kicks in. fn visit_term(&mut self, _term: &RichTerm, _ty: UnifType) {} - /// Record the type of an identifier. + /// Record the type of a bound identifier. fn visit_ident(&mut self, _ident: &LocIdent, _new_type: UnifType) {} } diff --git a/lsp/nls/src/main.rs b/lsp/nls/src/main.rs index 722f0e929..54879ef5f 100644 --- a/lsp/nls/src/main.rs +++ b/lsp/nls/src/main.rs @@ -20,6 +20,7 @@ mod position; mod requests; mod server; use server::Server; +mod pattern; mod term; mod trace; mod usage; diff --git a/lsp/nls/src/pattern.rs b/lsp/nls/src/pattern.rs new file mode 100644 index 000000000..b4a6edfec --- /dev/null +++ b/lsp/nls/src/pattern.rs @@ -0,0 +1,111 @@ +//! Pattern analysis. + +use nickel_lang_core::{destructuring::*, identifier::LocIdent, term::record::Field}; + +pub(super) trait Bindings { + /// Returns a list of all variables bound by this pattern, together with the path to the field they + /// match and the associated decoration. + /// + /// # Example + /// + /// For a pattern `{a = x @ {foo, bar | Number = z}, d = e}`, the result of this function + /// contains: + /// + /// - `(["a"], "foo", empty field)` for the `x` variable + /// - `(["a", "foo"], "foo", empty field)` for the `foo` variable + /// - `(["a", "bar"], "z", field with Number contract)` for the `z` variable + /// - `(["d"], "e", empty field)` for the `e` variable + fn bindings(&self) -> Vec<(Vec, LocIdent, Field)>; +} + +trait InjectBindings { + /// Same as [Self::bindings], but work relative to a current path inside a pattern and injects + /// the bindings into a working vector instead of returning the result. This method is mostly + /// used internally and is the one performing the actual work. + /// + /// Other modules of the LSP should use [Bindings::bindings] directly. + /// + /// # Parameters + /// + /// - `bindings`: the vector to inject the bindings into. + /// - `path`: the field path to the sub-pattern being analysed. + /// - `decoration`: the decoration associated with a potential parent field pattern. For + /// example, when injecting the bindings of `{foo ? 5 = x @ y @ z}`, all the introduced + /// variables should refer to the decoration of `foo`. This decoration is thus passed along + /// when calling to the sub-patterns' [Bindings::inject_bindings]. + fn inject_bindings( + &self, + bindings: &mut Vec<(Vec, LocIdent, Field)>, + path: Vec, + parent_deco: Option<&Field>, + ); +} + +impl Bindings for LocPattern { + fn bindings(&self) -> Vec<(Vec, LocIdent, Field)> { + let mut bindings = Vec::new(); + self.inject_bindings(&mut bindings, Vec::new(), None); + bindings + } +} + +impl InjectBindings for LocPattern { + fn inject_bindings( + &self, + bindings: &mut Vec<(Vec, LocIdent, Field)>, + path: Vec, + parent_deco: Option<&Field>, + ) { + self.pattern.inject_bindings(bindings, path, parent_deco); + } +} + +impl InjectBindings for Pattern { + fn inject_bindings( + &self, + bindings: &mut Vec<(Vec, LocIdent, Field)>, + path: Vec, + parent_deco: Option<&Field>, + ) { + match self { + Pattern::Any(id) => { + bindings.push((path, *id, parent_deco.cloned().unwrap_or_default())) + } + Pattern::RecordPattern(record_pat) => { + record_pat.inject_bindings(bindings, path, parent_deco) + } + Pattern::AliasedPattern { alias, pattern } => { + pattern.inject_bindings(bindings, path.clone(), parent_deco.clone()); + bindings.push((path, *alias, parent_deco.cloned().unwrap_or_default())); + } + } + } +} + +impl InjectBindings for RecordPattern { + fn inject_bindings( + &self, + bindings: &mut Vec<(Vec, LocIdent, Field)>, + path: Vec, + _parent_deco: Option<&Field>, + ) { + for field_pat in self.patterns.iter() { + // Field patterns have their own decoration, so there's no need to propagate + // `parent_deco` any further + field_pat.inject_bindings(bindings, path.clone(), None); + } + } +} + +impl InjectBindings for FieldPattern { + fn inject_bindings( + &self, + bindings: &mut Vec<(Vec, LocIdent, Field)>, + mut path: Vec, + _parent_deco: Option<&Field>, + ) { + path.push(self.matched_id); + self.pattern + .inject_bindings(bindings, path, Some(&self.decoration)); + } +} diff --git a/lsp/nls/src/position.rs b/lsp/nls/src/position.rs index 00b8f723a..c172bac8e 100644 --- a/lsp/nls/src/position.rs +++ b/lsp/nls/src/position.rs @@ -2,11 +2,12 @@ use std::ops::Range; use codespan::ByteIndex; use nickel_lang_core::{ + destructuring::Pattern, position::TermPos, term::{RichTerm, Term, Traverse, TraverseControl}, }; -use crate::{identifier::LocIdent, term::RichTermPtr}; +use crate::{identifier::LocIdent, pattern::Bindings, term::RichTermPtr}; /// Turn a collection of "nested" ranges into a collection of disjoint ranges. /// @@ -125,12 +126,12 @@ impl PositionLookup { match term.as_ref() { Term::Fun(id, _) | Term::Let(id, _, _, _) => idents.push(*id), Term::FunPattern(id, pat, _) | Term::LetPattern(id, pat, _, _) => { - let ids = pat.matches.iter().flat_map(|m| { - m.to_flattened_bindings() - .into_iter() - .map(|(_path, id, _)| id) - }); - idents.extend(ids.chain(*id).chain(pat.rest)) + let ids = pat.bindings().into_iter().map(|(_path, id, _)| id); + + // TODO[pattern]: what about aliased record patterns? + if let Pattern::RecordPattern(record_pat) = &pat.pattern { + idents.extend(ids.chain(*id).chain(record_pat.rest)) + } } Term::Var(id) => idents.push(*id), Term::Record(data) | Term::RecRecord(data, _, _) => { diff --git a/lsp/nls/src/usage.rs b/lsp/nls/src/usage.rs index ddccfc461..f04446f83 100644 --- a/lsp/nls/src/usage.rs +++ b/lsp/nls/src/usage.rs @@ -7,7 +7,7 @@ use nickel_lang_core::{ term::{RichTerm, Term, Traverse, TraverseControl}, }; -use crate::{field_walker::Def, identifier::LocIdent}; +use crate::{field_walker::Def, identifier::LocIdent, pattern::Bindings}; pub type Environment = GenericEnvironment; @@ -116,11 +116,10 @@ impl UsageLookup { new_env.insert_def(Def::Fn { ident }); } - for m in &pat.matches { - for (_path, id, _field) in m.to_flattened_bindings() { - new_env.insert_def(Def::Fn { ident: id.into() }); - } + for (_path, id, _field) in pat.bindings() { + new_env.insert_def(Def::Fn { ident: id.into() }); } + TraverseControl::ContinueWithScope(new_env) } Term::Let(id, val, body, attrs) => { @@ -151,18 +150,17 @@ impl UsageLookup { self.add_sym(def); } - for m in &pat.matches { - for (path, id, _field) in m.to_flattened_bindings() { - let path = path.iter().map(|i| i.ident()).rev().collect(); - let def = Def::Let { - ident: LocIdent::from(id), - value: val.clone(), - path, - }; - new_env.insert_def(def.clone()); - self.add_sym(def); - } + for (path, id, _field) in pat.bindings() { + let path = path.iter().map(|i| i.ident()).collect(); + let def = Def::Let { + ident: LocIdent::from(id), + value: val.clone(), + path, + }; + new_env.insert_def(def.clone()); + self.add_sym(def); } + TraverseControl::ContinueWithScope(new_env) } Term::RecRecord(data, ..) | Term::Record(data) => { From 2ac4a7a36b47b877808d567597f6decf40024a41 Mon Sep 17 00:00:00 2001 From: Yann Hamdaoui Date: Mon, 5 Feb 2024 19:06:13 +0100 Subject: [PATCH 2/7] Avoid illegal state for record patterns Record pattern was previously using a tuple `(open: bool, rest: Option)` to represent the presence of either no tail, an non-capturing tail `..`, or a capturing tail `..rest`. This representation allows the illegal state `(false, Some("x"))`: indeed, if the tail is capturing, the record contract is necessarily open. This commit flattens this representation to a single enum that correctly represents those three different cases, instead of the 4 allowed by the previous representation. --- core/src/destructuring.rs | 30 +++++++++++++++++---- core/src/parser/grammar.lalrpop | 15 +++++++---- core/src/pretty.rs | 23 +++++----------- core/src/transform/desugar_destructuring.rs | 15 ++--------- core/src/transform/free_vars.rs | 4 +-- core/src/typecheck/destructuring.rs | 6 ++--- lsp/nls/src/position.rs | 10 +++++-- 7 files changed, 57 insertions(+), 46 deletions(-) diff --git a/core/src/destructuring.rs b/core/src/destructuring.rs index f4541e3ff..709f21087 100644 --- a/core/src/destructuring.rs +++ b/core/src/destructuring.rs @@ -75,13 +75,24 @@ pub enum LastPattern

{ pub struct RecordPattern { /// The patterns for each field in the record. pub patterns: Vec, - /// If the pattern is open, i.e. if it ended with an ellipsis, capturing the rest or not. - pub open: bool, - /// If the pattern is open and the rest is captured, the capturing identifier is stored here. - pub rest: Option, + /// The tail of the pattern, indicating if the pattern is open, i.e. if it ended with an + /// ellipsis, capturing the rest or not. + pub tail: RecordPatternTail, pub span: RawSpan, } +/// The tail of a record pattern which might capture the rest of the record. +#[derive(Debug, PartialEq, Clone)] +pub enum RecordPatternTail { + /// The pattern is closed, i.e. it doesn't allow more fields. For example, `{foo, bar}`. + Empty, + /// The pattern ends with an ellipsis, making it open. For example, `{foo, bar, ..}`. + Open, + /// The pattern ends with an ellispis and a variable capturing the rest of the record. For + /// example, `{foo, bar, ..rest}`. + Capture(LocIdent), +} + impl RecordPattern { /// Check the matches for duplication, and raise an error if any occur. /// @@ -124,6 +135,15 @@ impl RecordPattern { Ok(()) } + + /// Check if this record contract is open, meaning that it accepts additional fields to be + /// present, whether the rest is captured or not. + pub fn is_open(&self) -> bool { + matches!( + self.tail, + RecordPatternTail::Open | RecordPatternTail::Capture(_) + ) + } } impl FieldPattern { @@ -184,7 +204,7 @@ impl ElaborateContract for RecordPattern { .map(FieldPattern::as_record_binding) .collect(), RecordAttrs { - open: self.open, + open: self.is_open(), ..Default::default() }, None, diff --git a/core/src/parser/grammar.lalrpop b/core/src/parser/grammar.lalrpop index 20880052c..522fa582b 100644 --- a/core/src/parser/grammar.lalrpop +++ b/core/src/parser/grammar.lalrpop @@ -549,17 +549,22 @@ Pattern: LocPattern = { // A full pattern at the left-hand side of a destructuring let. RecordPattern: RecordPattern = { "{" ",")*> "}" =>? { - let (open, rest) = match last { + let tail = match last { Some(LastPattern::Normal(m)) => { field_pats.push(*m); - (false,None) + RecordPatternTail::Empty }, - Some(LastPattern::Ellipsis(rest)) => (true, rest), - _ => (false, None), + Some(LastPattern::Ellipsis(Some(captured))) => { + RecordPatternTail::Capture(captured) + } + Some(LastPattern::Ellipsis(None)) => { + RecordPatternTail::Open + } + None => RecordPatternTail::Empty, }; let span = mk_span(src_id, start, end); - let pattern = RecordPattern { patterns: field_pats, open, rest, span }; + let pattern = RecordPattern { patterns: field_pats, tail, span }; pattern.check_matches()?; Ok(pattern) }, diff --git a/core/src/pretty.rs b/core/src/pretty.rs index 932804957..bcda0c9a3 100644 --- a/core/src/pretty.rs +++ b/core/src/pretty.rs @@ -1,6 +1,6 @@ use std::fmt; -use crate::destructuring::{self, Pattern, RecordPattern}; +use crate::destructuring::{self, Pattern, RecordPattern, RecordPatternTail}; use crate::identifier::LocIdent; use crate::parser::lexer::KEYWORDS; use crate::term::record::RecordData; @@ -565,8 +565,7 @@ where fn pretty(self, allocator: &'a D) -> DocBuilder<'a, D, A> { let RecordPattern { patterns: matches, - open, - rest, + tail, .. } = self; docs![ @@ -613,19 +612,11 @@ where }), allocator.line() ), - if *open { - docs![ - allocator, - allocator.line(), - "..", - if let Some(rest) = rest { - allocator.as_string(rest) - } else { - allocator.nil() - }, - ] - } else { - allocator.nil() + match tail { + RecordPatternTail::Empty => allocator.nil(), + RecordPatternTail::Open => docs![allocator, allocator.line(), ".."], + RecordPatternTail::Capture(id) => + docs![allocator, allocator.line(), "..", id.ident().to_string()], }, ] .nest(2) diff --git a/core/src/transform/desugar_destructuring.rs b/core/src/transform/desugar_destructuring.rs index 4ba8e2a34..55d16ea82 100644 --- a/core/src/transform/desugar_destructuring.rs +++ b/core/src/transform/desugar_destructuring.rs @@ -189,21 +189,10 @@ impl Desugar for RecordPattern { fn bind_rest(pat: &RecordPattern, destr: RichTerm, body: RichTerm) -> RichTerm { let capture_var = match pat { RecordPattern { - open: true, - rest: Some(x), + tail: RecordPatternTail::Capture(x), .. } => *x, - RecordPattern { - open: true, - rest: None, - .. - } - | RecordPattern { - open: false, - rest: None, - .. - } => return body, - _ => panic!("A closed pattern can not have a rest binding"), + _ => return body, }; Term::Let( diff --git a/core/src/transform/free_vars.rs b/core/src/transform/free_vars.rs index 302a35df0..b58552664 100644 --- a/core/src/transform/free_vars.rs +++ b/core/src/transform/free_vars.rs @@ -4,7 +4,7 @@ //! the recursive fields that actually appear in the definition of each field when computing the //! fixpoint. use crate::{ - destructuring::{FieldPattern, LocPattern, Pattern, RecordPattern}, + destructuring::*, identifier::Ident, term::{ record::{Field, FieldDeps, RecordDeps}, @@ -275,7 +275,7 @@ impl RemoveBindings for RecordPattern { m.remove_bindings(working_set); } - if let Some(rest) = self.rest { + if let RecordPatternTail::Capture(rest) = self.tail { working_set.remove(&rest.ident()); } } diff --git a/core/src/typecheck/destructuring.rs b/core/src/typecheck/destructuring.rs index a966d041d..d80025048 100644 --- a/core/src/typecheck/destructuring.rs +++ b/core/src/typecheck/destructuring.rs @@ -1,5 +1,5 @@ use crate::{ - destructuring::{FieldPattern, Pattern, RecordPattern}, + destructuring::{FieldPattern, Pattern, RecordPattern, RecordPatternTail}, error::TypecheckError, identifier::LocIdent, mk_uty_record_row, @@ -71,7 +71,7 @@ impl PatternTypes for RecordPattern { ctxt: &Context, mode: TypecheckMode, ) -> Result { - let tail = if self.open { + let tail = if self.is_open() { match mode { // We use a dynamic tail here since we're in walk mode, // but if/when we remove dynamic record tails this could @@ -86,7 +86,7 @@ impl PatternTypes for RecordPattern { } }; - if let Some(rest) = self.rest { + if let RecordPatternTail::Capture(rest) = self.tail { bindings.push((rest, UnifType::concrete(TypeF::Record(tail.clone())))); } diff --git a/lsp/nls/src/position.rs b/lsp/nls/src/position.rs index c172bac8e..388675eb8 100644 --- a/lsp/nls/src/position.rs +++ b/lsp/nls/src/position.rs @@ -2,7 +2,7 @@ use std::ops::Range; use codespan::ByteIndex; use nickel_lang_core::{ - destructuring::Pattern, + destructuring::{Pattern, RecordPatternTail}, position::TermPos, term::{RichTerm, Term, Traverse, TraverseControl}, }; @@ -128,9 +128,15 @@ impl PositionLookup { Term::FunPattern(id, pat, _) | Term::LetPattern(id, pat, _, _) => { let ids = pat.bindings().into_iter().map(|(_path, id, _)| id); + idents.extend(ids); + idents.extend(*id); + // TODO[pattern]: what about aliased record patterns? + // TODO[pattern]: what about nested patterns with tails? if let Pattern::RecordPattern(record_pat) = &pat.pattern { - idents.extend(ids.chain(*id).chain(record_pat.rest)) + if let RecordPatternTail::Capture(rest) = &record_pat.tail { + idents.push(*rest) + } } } Term::Var(id) => idents.push(*id), From a4704b1056e8919d8cce953b3365273e78ff23e9 Mon Sep 17 00:00:00 2001 From: Yann Hamdaoui Date: Mon, 5 Feb 2024 20:02:03 +0100 Subject: [PATCH 3/7] Move the alias field to Pattern The AST of patterns had a special node for an aliased pattern, which was a variant containing the alias and a potentital nested pattern. However, this doesn't model correctly patterns: usually, it doesn't make sense to stack aliases (and the parser won't accept it), but the previous representation accepted ASTs for things like `x @ y @ z @ `, which incurs additional burden to handle, although it can actually never happen. Additionally, the alias of the top pattern was duplicated as an optional field in the `LetPattern` and `FunPattern` nodes of the `Term` AST. This commit makes things simpler by storing `alias` as an optional field directly in the `Pattern` struct, which makes it accessible without having to pattern match on an enum variant, and forbids nested aliases. Doing so, we remove the duplication from the `LetPattern` and `FunPattern`, which now only takes a pattern instead of an optional identifier and a pattern, leading to code simplification. --- core/src/destructuring.rs | 29 ++++++----- core/src/parser/grammar.lalrpop | 32 ++++-------- core/src/parser/utils.rs | 23 +++----- core/src/pretty.rs | 49 ++++++++++------- core/src/term/mod.rs | 40 +++++++------- core/src/transform/desugar_destructuring.rs | 58 +++++++++------------ core/src/transform/free_vars.rs | 26 ++++----- core/src/typecheck/destructuring.rs | 37 +++++++++---- core/src/typecheck/mod.rs | 44 +++++++++------- lsp/nls/src/field_walker.rs | 4 +- lsp/nls/src/pattern.rs | 22 ++++---- lsp/nls/src/position.rs | 8 +-- lsp/nls/src/usage.rs | 18 +------ 13 files changed, 188 insertions(+), 202 deletions(-) diff --git a/core/src/destructuring.rs b/core/src/destructuring.rs index 709f21087..08ddf9d76 100644 --- a/core/src/destructuring.rs +++ b/core/src/destructuring.rs @@ -14,22 +14,24 @@ use crate::{ }; #[derive(Debug, PartialEq, Clone)] -pub enum Pattern { +pub enum PatternData { /// A simple pattern consisting of an identifier. Match anything and bind the result to the /// corresponding identfier. Any(LocIdent), /// A record pattern as in `{ a = { b, c } }` RecordPattern(RecordPattern), - /// An aliased pattern as in `x @ { a = b @ { c, d }, ..}` - AliasedPattern { - alias: LocIdent, - pattern: Box, - }, } +/// A generic pattern, that can appear in a match expression (not yet implemented) or in a +/// destructuring let-binding. #[derive(Debug, PartialEq, Clone)] -pub struct LocPattern { - pub pattern: Pattern, +pub struct Pattern { + /// The content of this pattern + pub pattern: PatternData, + /// A potential alias for this pattern, capturing the whole matched value. In the source + /// language, an alias is introduced by `x @ `, where `x` is an arbitrary identifier. + pub alias: Option, + /// The span of the pattern in the source. pub span: RawSpan, } @@ -46,7 +48,7 @@ pub struct FieldPattern { /// The pattern on the right-hand side of the `=`. A pattern like `{foo, bar}`, without the `=` /// sign, is considered to be `{foo=foo, bar=bar}`. In this case, `pattern` will be /// [Pattern::Any]. - pub pattern: LocPattern, + pub pattern: Pattern, pub span: RawSpan, } @@ -176,17 +178,16 @@ pub trait ElaborateContract { fn elaborate_contract(&self) -> Option; } -impl ElaborateContract for Pattern { +impl ElaborateContract for PatternData { fn elaborate_contract(&self) -> Option { match self { - Pattern::Any(_) => None, - Pattern::RecordPattern(pat) => pat.elaborate_contract(), - Pattern::AliasedPattern { pattern, .. } => pattern.elaborate_contract(), + PatternData::Any(_) => None, + PatternData::RecordPattern(pat) => pat.elaborate_contract(), } } } -impl ElaborateContract for LocPattern { +impl ElaborateContract for Pattern { fn elaborate_contract(&self) -> Option { self.pattern.elaborate_contract() } diff --git a/core/src/parser/grammar.lalrpop b/core/src/parser/grammar.lalrpop index 522fa582b..8720680b3 100644 --- a/core/src/parser/grammar.lalrpop +++ b/core/src/parser/grammar.lalrpop @@ -520,29 +520,18 @@ LastFieldPat: LastPattern = { // The right hand side of an `=` inside a destructuring pattern. #[inline] -Pattern: LocPattern = { - "@")?> => { - let record_pat_span = record_pat.span; - let record_pat = Pattern::RecordPattern(record_pat); - - let pattern = if let Some(alias) = id { - Pattern::AliasedPattern { - alias, - pattern: Box::new(LocPattern { - pattern: record_pat, - span: record_pat_span, - }), - } - } else { - record_pat - }; - - LocPattern { pattern, span: mk_span(src_id, l, r) } +Pattern: Pattern = { + "@")?> => { + Pattern { + alias, + pattern: PatternData::RecordPattern(record_pat), + span: mk_span(src_id, l, r), + } }, Ident => { let span = <>.pos.unwrap(); - LocPattern { pattern: Pattern::Any(<>), span } + Pattern { pattern: PatternData::Any(<>), alias: None, span } }, }; @@ -589,11 +578,12 @@ FieldPattern: FieldPattern = { FieldPattern { matched_id, decoration, - pattern: LocPattern { - pattern: Pattern::Any(matched_id), + pattern: Pattern { + pattern: PatternData::Any(matched_id), //unwrap(): the position of an parsed identifier should always //be defined span: matched_id.pos.unwrap(), + alias: None, }, span: mk_span(src_id, l, r) } diff --git a/core/src/parser/utils.rs b/core/src/parser/utils.rs index 10d9141e6..6129d7e35 100644 --- a/core/src/parser/utils.rs +++ b/core/src/parser/utils.rs @@ -10,7 +10,7 @@ use super::error::ParseError; use crate::{ combine::Combine, - destructuring::{LocPattern, Pattern}, + destructuring::{Pattern, PatternData}, eval::{ merge::{merge_doc, split}, operation::RecPriority, @@ -651,33 +651,26 @@ pub fn mk_merge_label(src_id: FileId, l: usize, r: usize) -> MergeLabel { /// and is recursive because recursive let-patterns are currently not supported. pub fn mk_let( rec: bool, - pat: LocPattern, + pat: Pattern, t1: RichTerm, t2: RichTerm, span: RawSpan, ) -> Result { match pat.pattern { - Pattern::Any(id) if rec => Ok(mk_term::let_rec_in(id, t1, t2)), - Pattern::Any(id) => Ok(mk_term::let_in(id, t1, t2)), + PatternData::Any(id) if rec => Ok(mk_term::let_rec_in(id, t1, t2)), + PatternData::Any(id) => Ok(mk_term::let_in(id, t1, t2)), _ if rec => Err(ParseError::RecursiveLetPattern(span)), - Pattern::AliasedPattern { alias, pattern } => { - Ok(mk_term::let_pat(Some(alias), *pattern, t1, t2)) - } - _ => { - let id: Option = None; - Ok(mk_term::let_pat(id, pat, t1, t2)) - } + _ => Ok(mk_term::let_pat(pat, t1, t2)), } } /// Generate a `Fun` or a `FunPattern` (depending on `assgn` having a pattern or not) /// from the parsing of a function definition. This function panics if the definition /// somehow has neither an `Ident` nor a non-`Empty` `Destruct` pattern. -pub fn mk_fun(pat: LocPattern, body: RichTerm) -> Term { +pub fn mk_fun(pat: Pattern, body: RichTerm) -> Term { match pat.pattern { - Pattern::Any(id) => Term::Fun(id, body), - Pattern::AliasedPattern { alias, pattern } => Term::FunPattern(Some(alias), *pattern, body), - _ => Term::FunPattern(None, pat, body), + PatternData::Any(id) => Term::Fun(id, body), + _ => Term::FunPattern(pat, body), } } diff --git a/core/src/pretty.rs b/core/src/pretty.rs index bcda0c9a3..5042c1145 100644 --- a/core/src/pretty.rs +++ b/core/src/pretty.rs @@ -1,6 +1,6 @@ use std::fmt; -use crate::destructuring::{self, Pattern, RecordPattern, RecordPatternTail}; +use crate::destructuring::{self, Pattern, PatternData, RecordPattern, RecordPatternTail}; use crate::identifier::LocIdent; use crate::parser::lexer::KEYWORDS; use crate::term::record::RecordData; @@ -540,6 +540,29 @@ where } impl<'a, D, A> Pretty<'a, D, A> for &Pattern +where + D: NickelAllocatorExt<'a, A>, + D::Doc: Clone, + A: Clone + 'a, +{ + fn pretty(self, allocator: &'a D) -> DocBuilder<'a, D, A> { + let alias_prefix = if let Some(alias) = self.alias { + docs![ + allocator, + alias.to_string(), + allocator.space(), + "@", + allocator.space() + ] + } else { + allocator.nil() + }; + + docs![allocator, alias_prefix, &self.pattern] + } +} + +impl<'a, D, A> Pretty<'a, D, A> for &PatternData where D: NickelAllocatorExt<'a, A>, D::Doc: Clone, @@ -547,11 +570,8 @@ where { fn pretty(self, allocator: &'a D) -> DocBuilder<'a, D, A> { match self { - Pattern::Any(id) => allocator.as_string(id), - Pattern::RecordPattern(rp) => rp.pretty(allocator), - Pattern::AliasedPattern { alias, pattern } => { - docs![allocator, alias.to_string(), " @ ", &pattern.pattern] - } + PatternData::Any(id) => allocator.as_string(id), + PatternData::RecordPattern(rp) => rp.pretty(allocator), } } } @@ -602,7 +622,7 @@ where field => allocator.field_metadata(&field.metadata, false), }, match &field_pat.pattern.pattern { - destructuring::Pattern::Any(id) if *id == field_pat.matched_id => + destructuring::PatternData::Any(id) if *id == field_pat.matched_id => allocator.nil(), pat => docs![allocator, allocator.line(), "= ", pat], }, @@ -678,12 +698,8 @@ where FunPattern(..) => { let mut params = vec![]; let mut rt = self; - while let FunPattern(id, pat, t) = rt { - params.push(if let Some(id) = id { - docs![allocator, id.to_string(), " @ ", &pat.pattern] - } else { - pat.pattern.pretty(allocator) - }); + while let FunPattern(pat, t) = rt { + params.push(pat.pattern.pretty(allocator)); rt = t.as_ref(); } docs![ @@ -728,14 +744,9 @@ where .append(allocator.line()) .append(body.pretty(allocator).nest(2)) .group(), - LetPattern(opt_id, pattern, rt, body) => docs![ + LetPattern(pattern, rt, body) => docs![ allocator, "let ", - if let Some(id) = opt_id { - docs![allocator, id.to_string(), " @ "] - } else { - allocator.nil() - }, &pattern.pattern, if let Annotated(annot, _) = rt.as_ref() { annot.pretty(allocator) diff --git a/core/src/term/mod.rs b/core/src/term/mod.rs index 49f2a22b1..9d61c530b 100644 --- a/core/src/term/mod.rs +++ b/core/src/term/mod.rs @@ -18,7 +18,7 @@ use record::{Field, FieldDeps, FieldMetadata, RecordData, RecordDeps}; use string::NickelString; use crate::{ - destructuring::LocPattern, + destructuring::Pattern, error::{EvalError, ParseError}, eval::cache::CacheIndex, eval::Environment, @@ -93,9 +93,9 @@ pub enum Term { #[serde(skip)] Fun(LocIdent, RichTerm), - /// A function able to destruct its arguments. + /// A destructuring function. #[serde(skip)] - FunPattern(Option, LocPattern, RichTerm), + FunPattern(Pattern, RichTerm), /// A blame label. #[serde(skip)] @@ -107,7 +107,7 @@ pub enum Term { /// A destructuring let-binding. #[serde(skip)] - LetPattern(Option, LocPattern, RichTerm, RichTerm), + LetPattern(Pattern, RichTerm, RichTerm), /// An application. #[serde(skip)] @@ -268,8 +268,9 @@ pub enum Term { } // PartialEq is mostly used for tests, when it's handy to compare something to an expected result. -// Most of the instance aren't really meaningful to use outside of very simple cases, and you +// Most of the instances aren't really meaningful to use outside of very simple cases, and you // should avoid comparing terms directly. +// // We have to implement this instance by hand because of the `Closure` node. impl PartialEq for Term { #[track_caller] @@ -280,15 +281,13 @@ impl PartialEq for Term { (Self::Str(l0), Self::Str(r0)) => l0 == r0, (Self::StrChunks(l0), Self::StrChunks(r0)) => l0 == r0, (Self::Fun(l0, l1), Self::Fun(r0, r1)) => l0 == r0 && l1 == r1, - (Self::FunPattern(l0, l1, l2), Self::FunPattern(r0, r1, r2)) => { - l0 == r0 && l1 == r1 && l2 == r2 - } + (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, l3), Self::Let(r0, r1, r2, r3)) => { l0 == r0 && l1 == r1 && l2 == r2 && l3 == r3 } - (Self::LetPattern(l0, l1, l2, l3), Self::LetPattern(r0, r1, r2, r3)) => { - l0 == r0 && l1 == r1 && l2 == r2 && l3 == r3 + (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, @@ -848,7 +847,7 @@ impl Term { Term::Bool(_) => Some("Bool".to_owned()), Term::Num(_) => Some("Number".to_owned()), Term::Str(_) => Some("String".to_owned()), - Term::Fun(_, _) | Term::FunPattern(_, _, _) => Some("Function".to_owned()), + Term::Fun(_, _) | Term::FunPattern(_, _) => Some("Function".to_owned()), Term::Match { .. } => Some("MatchExpression".to_owned()), Term::Lbl(_) => Some("Label".to_owned()), Term::Enum(_) => Some("Enum".to_owned()), @@ -1939,19 +1938,19 @@ impl Traverse for RichTerm { let t = t.traverse(f, order)?; RichTerm::new(Term::Fun(id, t), pos) } - Term::FunPattern(id, d, t) => { + Term::FunPattern(pat, t) => { let t = t.traverse(f, order)?; - RichTerm::new(Term::FunPattern(id, d, t), pos) + RichTerm::new(Term::FunPattern(pat, t), pos) } Term::Let(id, t1, t2, attrs) => { let t1 = t1.traverse(f, order)?; let t2 = t2.traverse(f, order)?; RichTerm::new(Term::Let(id, t1, t2, attrs), pos) } - Term::LetPattern(id, pat, t1, t2) => { + Term::LetPattern(pat, t1, t2) => { let t1 = t1.traverse(f, order)?; let t2 = t2.traverse(f, order)?; - RichTerm::new(Term::LetPattern(id, pat, t1, t2), pos) + RichTerm::new(Term::LetPattern(pat, t1, t2), pos) } Term::App(t1, t2) => { let t1 = t1.traverse(f, order)?; @@ -2118,12 +2117,12 @@ impl Traverse for RichTerm { } }), Term::Fun(_, t) - | Term::FunPattern(_, _, t) + | Term::FunPattern(_, t) | Term::EnumVariant { arg: t, .. } | Term::Op1(_, t) | Term::Sealed(_, t, _) => t.traverse_ref(f, state), Term::Let(_, t1, t2, _) - | Term::LetPattern(_, _, t1, t2) + | Term::LetPattern(_, t1, t2) | Term::App(t1, t2) | Term::Op2(_, t1, t2) => t1 .traverse_ref(f, state) @@ -2478,14 +2477,13 @@ pub mod make { let_in_(true, id, t1, t2) } - pub fn let_pat(id: Option, pat: D, t1: T1, t2: T2) -> RichTerm + pub fn let_pat(pat: D, t1: T1, t2: T2) -> RichTerm where T1: Into, T2: Into, - D: Into, - I: Into, + D: Into, { - Term::LetPattern(id.map(|i| i.into()), pat.into(), t1.into(), t2.into()).into() + Term::LetPattern(pat.into(), t1.into(), t2.into()).into() } #[cfg(test)] diff --git a/core/src/transform/desugar_destructuring.rs b/core/src/transform/desugar_destructuring.rs index 55d16ea82..9ff6e92ed 100644 --- a/core/src/transform/desugar_destructuring.rs +++ b/core/src/transform/desugar_destructuring.rs @@ -55,9 +55,8 @@ use crate::term::{ /// destructuring patterns to be desugared in children nodes. pub fn transform_one(rt: RichTerm) -> RichTerm { match_sharedterm!(match (rt.term) { - Term::LetPattern(id, pat, bound, body) => - RichTerm::new(desugar_let(id, pat, bound, body), rt.pos), - Term::FunPattern(id, pat, body) => RichTerm::new(desugar_fun(id, pat, body), rt.pos), + Term::LetPattern(pat, bound, body) => RichTerm::new(desugar_let(pat, bound, body), rt.pos), + Term::FunPattern(pat, body) => RichTerm::new(desugar_fun(pat, body), rt.pos), _ => rt, }) } @@ -67,14 +66,14 @@ pub fn transform_one(rt: RichTerm) -> RichTerm { /// A function `fun => body` is desugared to `fun x => let = x in body`. The inner /// destructuring let isn't desugared further, as the general program transformation machinery will /// take care of transforming the body of the function in a second step. -pub fn desugar_fun(id: Option, pat: LocPattern, body: RichTerm) -> Term { - let id = id.unwrap_or_else(LocIdent::fresh); +pub fn desugar_fun(mut pat: Pattern, body: RichTerm) -> Term { + let id = pat.alias.take().unwrap_or_else(LocIdent::fresh); let pos_body = body.pos; Term::Fun( id, RichTerm::new( - Term::LetPattern(None, pat, Term::Var(id).into(), body), + Term::LetPattern(pat, Term::Var(id).into(), body), // TODO: should we use rt.pos? pos_body, ), @@ -83,7 +82,7 @@ pub fn desugar_fun(id: Option, pat: LocPattern, body: RichTerm) -> Ter /// Elaborate a contract from the pattern if it is a record pattern and applies to the value before /// actually destructuring it. Then convert the let pattern to a sequence of normal let-bindings. -pub fn desugar_let(id: Option, pat: LocPattern, bound: RichTerm, body: RichTerm) -> Term { +pub fn desugar_let(pat: Pattern, bound: RichTerm, body: RichTerm) -> Term { let contract = pat.elaborate_contract(); let annotated = { @@ -100,15 +99,6 @@ pub fn desugar_let(id: Option, pat: LocPattern, bound: RichTerm, body: ) }; - let pat = if let Some(alias) = id { - Pattern::AliasedPattern { - alias, - pattern: Box::new(pat), - } - } else { - pat.pattern - }; - pat.desugar(annotated, body) } @@ -124,30 +114,32 @@ trait Desugar { fn desugar(self, destr: RichTerm, body: RichTerm) -> Term; } -impl Desugar for LocPattern { +impl Desugar for Pattern { fn desugar(self, destr: RichTerm, body: RichTerm) -> Term { - self.pattern.desugar(destr, body) + // If the pattern is aliased, `x @ ` matching `destr`, we introduce a heading + // let-binding `let x = destruct in `, where `` is the desugaring + // of `` matching `x` followed by the original `body`. + if let Some(alias) = self.alias { + let pos = body.pos; + let inner = RichTerm::new( + self.pattern + .desugar(RichTerm::new(Term::Var(alias), alias.pos), body), + pos, + ); + + Term::Let(alias, destr, inner, LetAttrs::default()) + } else { + self.pattern.desugar(destr, body) + } } } -impl Desugar for Pattern { +impl Desugar for PatternData { fn desugar(self, destr: RichTerm, body: RichTerm) -> Term { match self { // If the pattern is an unconstrained identifier, we just bind it to the value. - Pattern::Any(id) => Term::Let(id, destr, body, LetAttrs::default()), - Pattern::RecordPattern(pat) => pat.desugar(destr, body), - // If the pattern is aliased, `x @ ` matching `destruct`, we introduce a heading - // let-binding `let x = destruct in `, where `` is the - // desugaring of `` matching `x`. - Pattern::AliasedPattern { alias, pattern } => { - let pos = body.pos; - let inner = RichTerm::new( - pattern.desugar(RichTerm::new(Term::Var(alias), alias.pos), body), - pos, - ); - - Term::Let(alias, destr, inner, LetAttrs::default()) - } + PatternData::Any(id) => Term::Let(id, destr, body, LetAttrs::default()), + PatternData::RecordPattern(pat) => pat.desugar(destr, body), } } } diff --git a/core/src/transform/free_vars.rs b/core/src/transform/free_vars.rs index b58552664..6b216399e 100644 --- a/core/src/transform/free_vars.rs +++ b/core/src/transform/free_vars.rs @@ -51,14 +51,11 @@ impl CollectFreeVars for RichTerm { free_vars.extend(fresh); } - Term::FunPattern(id, pat, body) => { + Term::FunPattern(pat, body) => { let mut fresh = HashSet::new(); body.collect_free_vars(&mut fresh); pat.remove_bindings(&mut fresh); - if let Some(id) = id { - fresh.remove(&id.ident()); - } free_vars.extend(fresh); } @@ -76,15 +73,12 @@ impl CollectFreeVars for RichTerm { free_vars.extend(fresh); } - Term::LetPattern(id, pat, t1, t2) => { + Term::LetPattern(pat, t1, t2) => { let mut fresh = HashSet::new(); t1.collect_free_vars(free_vars); t2.collect_free_vars(&mut fresh); pat.remove_bindings(&mut fresh); - if let Some(id) = id { - fresh.remove(&id.ident()); - } free_vars.extend(fresh); } @@ -240,26 +234,26 @@ trait RemoveBindings { fn remove_bindings(&self, working_set: &mut HashSet); } -impl RemoveBindings for Pattern { +impl RemoveBindings for PatternData { fn remove_bindings(&self, working_set: &mut HashSet) { match self { - Pattern::Any(id) => { + PatternData::Any(id) => { working_set.remove(&id.ident()); } - Pattern::RecordPattern(record_pat) => { + PatternData::RecordPattern(record_pat) => { record_pat.remove_bindings(working_set); } - Pattern::AliasedPattern { alias, pattern } => { - working_set.remove(&alias.ident()); - pattern.remove_bindings(working_set); - } } } } -impl RemoveBindings for LocPattern { +impl RemoveBindings for Pattern { fn remove_bindings(&self, working_set: &mut HashSet) { self.pattern.remove_bindings(working_set); + + if let Some(alias) = self.alias { + working_set.remove(&alias.ident()); + } } } diff --git a/core/src/typecheck/destructuring.rs b/core/src/typecheck/destructuring.rs index d80025048..2222a5254 100644 --- a/core/src/typecheck/destructuring.rs +++ b/core/src/typecheck/destructuring.rs @@ -1,5 +1,5 @@ use crate::{ - destructuring::{FieldPattern, Pattern, RecordPattern, RecordPatternTail}, + destructuring::*, error::TypecheckError, identifier::LocIdent, mk_uty_record_row, @@ -105,6 +105,28 @@ impl PatternTypes for RecordPattern { impl PatternTypes for Pattern { type PatType = UnifType; + fn pattern_types_inj( + &self, + bindings: &mut Vec<(LocIdent, UnifType)>, + state: &mut State, + ctxt: &Context, + mode: TypecheckMode, + ) -> Result { + let typ = self + .pattern + .pattern_types_inj(bindings, state, ctxt, mode)?; + + if let Some(alias) = self.alias { + bindings.push((alias, typ.clone())); + } + + Ok(typ) + } +} + +impl PatternTypes for PatternData { + type PatType = UnifType; + fn pattern_types_inj( &self, bindings: &mut Vec<(LocIdent, UnifType)>, @@ -113,26 +135,19 @@ impl PatternTypes for Pattern { mode: TypecheckMode, ) -> Result { match self { - Pattern::Any(id) => { + PatternData::Any(id) => { let typ = match mode { TypecheckMode::Walk => mk_uniftype::dynamic(), TypecheckMode::Enforce => state.table.fresh_type_uvar(ctxt.var_level), }; + bindings.push((*id, typ.clone())); Ok(typ) } - Pattern::RecordPattern(record_pat) => Ok(UnifType::concrete(TypeF::Record( + PatternData::RecordPattern(record_pat) => Ok(UnifType::concrete(TypeF::Record( record_pat.pattern_types_inj(bindings, state, ctxt, mode)?, ))), - Pattern::AliasedPattern { alias, pattern } => { - let typ = pattern - .pattern - .pattern_types_inj(bindings, state, ctxt, mode)?; - - bindings.push((*alias, typ.clone())); - Ok(typ) - } } } } diff --git a/core/src/typecheck/mod.rs b/core/src/typecheck/mod.rs index 6049a70bd..749400532 100644 --- a/core/src/typecheck/mod.rs +++ b/core/src/typecheck/mod.rs @@ -1466,14 +1466,10 @@ fn walk( ctxt.type_env.insert(id.ident(), mk_uniftype::dynamic()); walk(state, ctxt, visitor, t) } - Term::FunPattern(id, pat, t) => { - if let Some(id) = id { - // The parameter of an unannotated function is always assigned type `Dyn`. - ctxt.type_env.insert(id.ident(), mk_uniftype::dynamic()); - } - - let (_, pat_bindings) = pat.pattern.pattern_types(state, &ctxt, destructuring::TypecheckMode::Walk)?; + Term::FunPattern(pat, t) => { + let (_, pat_bindings) = pat.pattern_types(state, &ctxt, destructuring::TypecheckMode::Walk)?; ctxt.type_env.extend(pat_bindings.into_iter().map(|(id, typ)| (id.ident(), typ))); + walk(state, ctxt, visitor, t) } Term::Array(terms, _) => terms @@ -1506,15 +1502,22 @@ fn walk( walk(state, ctxt, visitor, rt) } - Term::LetPattern(x, pat, re, rt) => { + Term::LetPattern(pat, re, rt) => { let ty_let = binding_type(state, re.as_ref(), &ctxt, false); + walk(state, ctxt.clone(), visitor, re)?; - if let Some(x) = x { - visitor.visit_ident(x, ty_let.clone()); - ctxt.type_env.insert(x.ident(), ty_let); + // 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 { + visitor.visit_ident(alias, ty_let.clone()); + ctxt.type_env.insert(alias.ident(), ty_let); } + // [^separate-alias-treatment]: Note that we call `pattern_types` on the inner pattern + // data, which doesn't take into account the potential heading alias `x @ `. + // This is on purpose, as the alias has been treated separately, so we don't want to + // shadow it with a less precise type. let (_, pat_bindings) = pat.pattern.pattern_types(state, &ctxt, destructuring::TypecheckMode::Walk)?; for (id, typ) in pat_bindings { @@ -1818,7 +1821,8 @@ fn check( ctxt.type_env.insert(x.ident(), src); check(state, ctxt, visitor, t, trg) } - Term::FunPattern(x, pat, t) => { + Term::FunPattern(pat, t) => { + // See [^separate-alias-treatment]. let (pat_ty, pat_bindings) = pat.pattern .pattern_types(state, &ctxt, destructuring::TypecheckMode::Enforce)?; @@ -1827,9 +1831,9 @@ fn check( let trg = state.table.fresh_type_uvar(ctxt.var_level); let arr = mk_uty_arrow!(src.clone(), trg.clone()); - if let Some(x) = x { - visitor.visit_ident(x, src.clone()); - ctxt.type_env.insert(x.ident(), src); + if let Some(alias) = &pat.alias { + visitor.visit_ident(alias, src.clone()); + ctxt.type_env.insert(alias.ident(), src); } for (id, typ) in pat_bindings { @@ -1879,8 +1883,8 @@ fn check( } check(state, ctxt, visitor, rt, ty) } - Term::LetPattern(x, pat, re, rt) => { - // The inferred type of the pattern w/ unification vars + Term::LetPattern(pat, re, rt) => { + // See [^separate-alias-treatment]. let (pat_ty, pat_bindings) = pat.pattern .pattern_types(state, &ctxt, destructuring::TypecheckMode::Enforce)?; @@ -1895,9 +1899,9 @@ fn check( check(state, ctxt.clone(), visitor, re, ty_let.clone())?; - if let Some(x) = x { - visitor.visit_ident(x, ty_let.clone()); - ctxt.type_env.insert(x.ident(), ty_let); + if let Some(alias) = &pat.alias { + visitor.visit_ident(alias, ty_let.clone()); + ctxt.type_env.insert(alias.ident(), ty_let); } for (id, typ) in pat_bindings { diff --git a/lsp/nls/src/field_walker.rs b/lsp/nls/src/field_walker.rs index 979036ad8..505f4963c 100644 --- a/lsp/nls/src/field_walker.rs +++ b/lsp/nls/src/field_walker.rs @@ -446,9 +446,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::StaticAccess(id), term) => { self.containers_at_path(term, std::iter::once(id.ident())) } diff --git a/lsp/nls/src/pattern.rs b/lsp/nls/src/pattern.rs index b4a6edfec..247fb77b1 100644 --- a/lsp/nls/src/pattern.rs +++ b/lsp/nls/src/pattern.rs @@ -41,7 +41,7 @@ trait InjectBindings { ); } -impl Bindings for LocPattern { +impl Bindings for Pattern { fn bindings(&self) -> Vec<(Vec, LocIdent, Field)> { let mut bindings = Vec::new(); self.inject_bindings(&mut bindings, Vec::new(), None); @@ -49,18 +49,26 @@ impl Bindings for LocPattern { } } -impl InjectBindings for LocPattern { +impl InjectBindings for Pattern { fn inject_bindings( &self, bindings: &mut Vec<(Vec, LocIdent, Field)>, path: Vec, parent_deco: Option<&Field>, ) { + if let Some(alias) = self.alias { + bindings.push(( + path.clone(), + alias, + parent_deco.cloned().unwrap_or_default(), + )); + } + self.pattern.inject_bindings(bindings, path, parent_deco); } } -impl InjectBindings for Pattern { +impl InjectBindings for PatternData { fn inject_bindings( &self, bindings: &mut Vec<(Vec, LocIdent, Field)>, @@ -68,16 +76,12 @@ impl InjectBindings for Pattern { parent_deco: Option<&Field>, ) { match self { - Pattern::Any(id) => { + PatternData::Any(id) => { bindings.push((path, *id, parent_deco.cloned().unwrap_or_default())) } - Pattern::RecordPattern(record_pat) => { + PatternData::RecordPattern(record_pat) => { record_pat.inject_bindings(bindings, path, parent_deco) } - Pattern::AliasedPattern { alias, pattern } => { - pattern.inject_bindings(bindings, path.clone(), parent_deco.clone()); - bindings.push((path, *alias, parent_deco.cloned().unwrap_or_default())); - } } } } diff --git a/lsp/nls/src/position.rs b/lsp/nls/src/position.rs index 388675eb8..1ada3ce7d 100644 --- a/lsp/nls/src/position.rs +++ b/lsp/nls/src/position.rs @@ -2,7 +2,7 @@ use std::ops::Range; use codespan::ByteIndex; use nickel_lang_core::{ - destructuring::{Pattern, RecordPatternTail}, + destructuring::{PatternData, RecordPatternTail}, position::TermPos, term::{RichTerm, Term, Traverse, TraverseControl}, }; @@ -125,15 +125,15 @@ impl PositionLookup { match term.as_ref() { Term::Fun(id, _) | Term::Let(id, _, _, _) => idents.push(*id), - Term::FunPattern(id, pat, _) | Term::LetPattern(id, pat, _, _) => { + Term::FunPattern(pat, _) | Term::LetPattern(pat, _, _) => { let ids = pat.bindings().into_iter().map(|(_path, id, _)| id); idents.extend(ids); - idents.extend(*id); + idents.extend(pat.alias); // TODO[pattern]: what about aliased record patterns? // TODO[pattern]: what about nested patterns with tails? - if let Pattern::RecordPattern(record_pat) = &pat.pattern { + if let PatternData::RecordPattern(record_pat) = &pat.pattern { if let RecordPatternTail::Capture(rest) = &record_pat.tail { idents.push(*rest) } diff --git a/lsp/nls/src/usage.rs b/lsp/nls/src/usage.rs index f04446f83..6aef367ad 100644 --- a/lsp/nls/src/usage.rs +++ b/lsp/nls/src/usage.rs @@ -109,12 +109,8 @@ impl UsageLookup { new_env.insert_def(Def::Fn { ident }); TraverseControl::ContinueWithScope(new_env) } - Term::FunPattern(maybe_id, pat, _body) => { + Term::FunPattern(pat, _body) => { let mut new_env = env.clone(); - if let Some(id) = maybe_id { - let ident = LocIdent::from(*id); - new_env.insert_def(Def::Fn { ident }); - } for (_path, id, _field) in pat.bindings() { new_env.insert_def(Def::Fn { ident: id.into() }); @@ -137,18 +133,8 @@ impl UsageLookup { TraverseControl::SkipBranch } - Term::LetPattern(maybe_id, pat, val, _body) => { + Term::LetPattern(pat, val, _body) => { let mut new_env = env.clone(); - if let Some(id) = maybe_id { - let def = Def::Let { - ident: LocIdent::from(*id), - value: val.clone(), - path: Vec::new(), - }; - - new_env.insert_def(def.clone()); - self.add_sym(def); - } for (path, id, _field) in pat.bindings() { let path = path.iter().map(|i| i.ident()).collect(); From c966dc040436a4bf22a409fc3156c5a9d65d45ac Mon Sep 17 00:00:00 2001 From: Yann Hamdaoui Date: Mon, 5 Feb 2024 20:15:52 +0100 Subject: [PATCH 4/7] Restore old behavior in typed patterns The refactoring of patterns has introduced a slightly different algorithm for typechecking patterns, which isn't entirely backward-compatible, although it's more consistent. We'll probably rule out (i.e. depreacte) the offending special cases, but until then, this commit restores the previous behavior, which fixes a previously failing test. --- core/src/parser/tests.rs | 8 --- core/src/pretty.rs | 6 +- core/src/typecheck/destructuring.rs | 61 +++++++++++++------ .../tests/integration/inputs/core/records.ncl | 9 +++ 4 files changed, 56 insertions(+), 28 deletions(-) diff --git a/core/src/parser/tests.rs b/core/src/parser/tests.rs index 19bbca443..58748ded8 100644 --- a/core/src/parser/tests.rs +++ b/core/src/parser/tests.rs @@ -174,14 +174,6 @@ fn variables() { assert!(parse("x1_x_").is_ok()); } -#[test] -fn functions() { - assert_eq!( - crate::transform::desugar_destructuring::desugar_fun(parse_without_pos("fun x => x")), - mk_term::id() - ); -} - #[test] fn lets() { assert_matches!(parse("let x1 = x2 in x3"), Ok(..)); diff --git a/core/src/pretty.rs b/core/src/pretty.rs index 5042c1145..43c287c9b 100644 --- a/core/src/pretty.rs +++ b/core/src/pretty.rs @@ -624,7 +624,7 @@ where match &field_pat.pattern.pattern { destructuring::PatternData::Any(id) if *id == field_pat.matched_id => allocator.nil(), - pat => docs![allocator, allocator.line(), "= ", pat], + _ => docs![allocator, allocator.line(), "= ", &field_pat.pattern], }, "," ] @@ -699,7 +699,7 @@ where let mut params = vec![]; let mut rt = self; while let FunPattern(pat, t) = rt { - params.push(pat.pattern.pretty(allocator)); + params.push(pat.pretty(allocator)); rt = t.as_ref(); } docs![ @@ -747,7 +747,7 @@ where LetPattern(pattern, rt, body) => docs![ allocator, "let ", - &pattern.pattern, + pattern, if let Annotated(annot, _) = rt.as_ref() { annot.pretty(allocator) } else { diff --git a/core/src/typecheck/destructuring.rs b/core/src/typecheck/destructuring.rs index 2222a5254..fcdd78962 100644 --- a/core/src/typecheck/destructuring.rs +++ b/core/src/typecheck/destructuring.rs @@ -162,24 +162,51 @@ impl PatternTypes for FieldPattern { ctxt: &Context, mode: TypecheckMode, ) -> Result { - let ty_row = self - .pattern - .pattern - .pattern_types_inj(bindings, state, ctxt, mode)?; + // If there is a static type annotations in a nested record patterns then we need to unify + // them with the pattern type we've built to ensure (1) that they're mutually compatible + // and (2) that we assign the annotated types to the right unification variables. + let ty_row = match ( + &self.decoration.metadata.annotation.typ, + &self.pattern.pattern, + mode, + ) { + // However, in walk mode, we only do that when the nested pattern isn't a leaf (i.e. + // `Any`) for backward-compatibility reasons. + // + // Before this function was refactored, Nickel has been allowing things like `let {foo + // : Number} = {foo = 1} in foo` in walk mode, which would fail to typecheck with the + // generic approach: the pattern is parsed as `{foo : Number = foo}`, the second + // occurrence of `foo` gets type `Dyn` in walk mode, but `Dyn` fails to unify with + // `Number`. In this case, we don't recursively call `pattern_types_inj` in the first + // place and just declare that the type of `foo` is `Number`. + // + // This special case should probably be ruled out, requiring the users to use `let {foo + // | Number}` instead, at least outside of a statically typed code block. But before + // this happens, we special case the old behavior and eschew unification. + (Some(annot_ty), PatternData::Any(id), TypecheckMode::Walk) => { + let ty_row = UnifType::from_type(annot_ty.typ.clone(), &ctxt.term_env); + bindings.push((*id, ty_row.clone())); + ty_row + } + (Some(annot_ty), _, _) => { + let pos = annot_ty.typ.pos; + let annot_uty = UnifType::from_type(annot_ty.typ.clone(), &ctxt.term_env); - // If there are type annotations within nested record patterns - // then we need to unify them with the pattern type we've built - // to ensure (1) that they're mutually compatible and (2) that - // we assign the annotated types to the right unification variables. - if let Some(annot_ty) = &self.decoration.metadata.annotation.typ { - let pos = annot_ty.typ.pos; - let annot_uty = UnifType::from_type(annot_ty.typ.clone(), &ctxt.term_env); - - ty_row - .clone() - .unify(annot_uty, state, ctxt) - .map_err(|e| e.into_typecheck_err(state, pos))?; - } + let ty_row = self + .pattern + .pattern_types_inj(bindings, state, ctxt, mode)?; + + ty_row + .clone() + .unify(annot_uty, state, ctxt) + .map_err(|e| e.into_typecheck_err(state, pos))?; + + ty_row + } + _ => self + .pattern + .pattern_types_inj(bindings, state, ctxt, mode)?, + }; Ok(RecordRowF { id: self.matched_id, diff --git a/core/tests/integration/inputs/core/records.ncl b/core/tests/integration/inputs/core/records.ncl index 379363727..a53cb4e23 100644 --- a/core/tests/integration/inputs/core/records.ncl +++ b/core/tests/integration/inputs/core/records.ncl @@ -100,7 +100,16 @@ let { check, .. } = import "../lib/assert.ncl" in foo = 1, bar : Number = foo, }.bar == 1, + # This form will probably be deprecated, because using a type annotation + # outside of a statically typed block is confusing (will the body of `foo` be + # typechecked? the current answer is no). + # + # The correct form one line after, with a `|`. + # + # However, we keep this test until the deprecation and eventual removal to + # make sure we don't break backward compatibility until then let { foo : Number } = { foo = 1 } in foo == 1, + let { foo | Number } = { foo = 1 } in foo == 1, # recursive overriding with common fields # regression tests for [#579](https://github.com/tweag/nickel/issues/579) From 055898611b0e4ee4cb5fe0ed0e4c4150b1720140 Mon Sep 17 00:00:00 2001 From: Yann Hamdaoui Date: Tue, 6 Feb 2024 17:56:13 +0100 Subject: [PATCH 5/7] Various renamings in destructuring (now pattern) This commit only applies pure renaming of several symbols of the destructuring module for improved clarity. The whole module is also moved to `term::pattern`, as patterns are just syntactic component of the term AST. --- core/src/lib.rs | 1 - core/src/parser/grammar.lalrpop | 18 ++++----- core/src/parser/utils.rs | 6 +-- core/src/pretty.rs | 15 ++++--- core/src/term/mod.rs | 3 +- .../src/{destructuring.rs => term/pattern.rs} | 34 ++++++++-------- core/src/transform/desugar_destructuring.rs | 8 ++-- core/src/transform/free_vars.rs | 6 +-- core/src/typecheck/mod.rs | 16 ++++---- .../{destructuring.rs => pattern.rs} | 16 ++++---- lsp/nls/src/pattern.rs | 39 ++++++++++--------- lsp/nls/src/position.rs | 8 ++-- 12 files changed, 88 insertions(+), 82 deletions(-) rename core/src/{destructuring.rs => term/pattern.rs} (88%) rename core/src/typecheck/{destructuring.rs => pattern.rs} (95%) diff --git a/core/src/lib.rs b/core/src/lib.rs index 930d1529a..e1785af77 100644 --- a/core/src/lib.rs +++ b/core/src/lib.rs @@ -2,7 +2,6 @@ pub mod cache; pub mod closurize; pub mod combine; pub mod deserialize; -pub mod destructuring; pub mod environment; pub mod error; pub mod eval; diff --git a/core/src/parser/grammar.lalrpop b/core/src/parser/grammar.lalrpop index 8720680b3..1cd596779 100644 --- a/core/src/parser/grammar.lalrpop +++ b/core/src/parser/grammar.lalrpop @@ -55,12 +55,12 @@ use crate::{ mk_opn, mk_fun, identifier::LocIdent, - destructuring::*, term::{ *, record::{RecordAttrs, Field, FieldMetadata}, array::Array, make as mk_term, + pattern::*, }, typ::*, position::{TermPos, RawSpan}, @@ -524,14 +524,14 @@ Pattern: Pattern = { "@")?> => { Pattern { alias, - pattern: PatternData::RecordPattern(record_pat), + data: PatternData::Record(record_pat), span: mk_span(src_id, l, r), } }, Ident => { let span = <>.pos.unwrap(); - Pattern { pattern: PatternData::Any(<>), alias: None, span } + Pattern { data: PatternData::Any(<>), alias: None, span } }, }; @@ -554,7 +554,7 @@ RecordPattern: RecordPattern = { let span = mk_span(src_id, start, end); let pattern = RecordPattern { patterns: field_pats, tail, span }; - pattern.check_matches()?; + pattern.check_dup()?; Ok(pattern) }, }; @@ -563,23 +563,23 @@ RecordPattern: RecordPattern = { FieldPattern: FieldPattern = { ?> "=" => { - let decoration = metadata_with_default(anns, default); + let extra = metadata_with_default(anns, default); FieldPattern { matched_id, - decoration, + extra, pattern, span: mk_span(src_id, l, r), } }, ?> => { - let decoration = metadata_with_default(anns, default); + let extra = metadata_with_default(anns, default); FieldPattern { matched_id, - decoration, + extra, pattern: Pattern { - pattern: PatternData::Any(matched_id), + data: PatternData::Any(matched_id), //unwrap(): the position of an parsed identifier should always //be defined span: matched_id.pos.unwrap(), diff --git a/core/src/parser/utils.rs b/core/src/parser/utils.rs index 6129d7e35..f18a69abf 100644 --- a/core/src/parser/utils.rs +++ b/core/src/parser/utils.rs @@ -10,7 +10,6 @@ use super::error::ParseError; use crate::{ combine::Combine, - destructuring::{Pattern, PatternData}, eval::{ merge::{merge_doc, split}, operation::RecPriority, @@ -19,6 +18,7 @@ use crate::{ label::{Label, MergeKind, MergeLabel}, mk_app, mk_fun, position::{RawSpan, TermPos}, + term::pattern::{Pattern, PatternData}, term::{ make as mk_term, record::{Field, FieldMetadata, RecordAttrs, RecordData}, @@ -656,7 +656,7 @@ pub fn mk_let( t2: RichTerm, span: RawSpan, ) -> Result { - match pat.pattern { + match pat.data { PatternData::Any(id) if rec => Ok(mk_term::let_rec_in(id, t1, t2)), PatternData::Any(id) => Ok(mk_term::let_in(id, t1, t2)), _ if rec => Err(ParseError::RecursiveLetPattern(span)), @@ -668,7 +668,7 @@ pub fn mk_let( /// from the parsing of a function definition. This function panics if the definition /// somehow has neither an `Ident` nor a non-`Empty` `Destruct` pattern. pub fn mk_fun(pat: Pattern, body: RichTerm) -> Term { - match pat.pattern { + match pat.data { PatternData::Any(id) => Term::Fun(id, body), _ => Term::FunPattern(pat, body), } diff --git a/core/src/pretty.rs b/core/src/pretty.rs index 43c287c9b..f090acc79 100644 --- a/core/src/pretty.rs +++ b/core/src/pretty.rs @@ -1,8 +1,8 @@ use std::fmt; -use crate::destructuring::{self, Pattern, PatternData, RecordPattern, RecordPatternTail}; use crate::identifier::LocIdent; use crate::parser::lexer::KEYWORDS; +use crate::term::pattern::{Pattern, PatternData, RecordPattern, RecordPatternTail}; use crate::term::record::RecordData; use crate::term::{ record::{Field, FieldMetadata}, @@ -558,7 +558,7 @@ where allocator.nil() }; - docs![allocator, alias_prefix, &self.pattern] + docs![allocator, alias_prefix, &self.data] } } @@ -571,7 +571,7 @@ where fn pretty(self, allocator: &'a D) -> DocBuilder<'a, D, A> { match self { PatternData::Any(id) => allocator.as_string(id), - PatternData::RecordPattern(rp) => rp.pretty(allocator), + PatternData::Record(rp) => rp.pretty(allocator), } } } @@ -596,7 +596,7 @@ where docs![ allocator, field_pat.matched_id.to_string(), - match &field_pat.decoration { + match &field_pat.extra { Field { value: Some(value), metadata: @@ -617,13 +617,12 @@ where ), allocator.line(), "? ", - allocator.atom(&value), + allocator.atom(value), ], field => allocator.field_metadata(&field.metadata, false), }, - match &field_pat.pattern.pattern { - destructuring::PatternData::Any(id) if *id == field_pat.matched_id => - allocator.nil(), + match &field_pat.pattern.data { + PatternData::Any(id) if *id == field_pat.matched_id => allocator.nil(), _ => docs![allocator, allocator.line(), "= ", &field_pat.pattern], }, "," diff --git a/core/src/term/mod.rs b/core/src/term/mod.rs index 9d61c530b..905fde8af 100644 --- a/core/src/term/mod.rs +++ b/core/src/term/mod.rs @@ -10,15 +10,16 @@ //! It also features types and type annotations, and other typechecking or contracts-related //! constructs (label, symbols, etc.). pub mod array; +pub mod pattern; pub mod record; pub mod string; use array::{Array, ArrayAttrs}; +use pattern::Pattern; use record::{Field, FieldDeps, FieldMetadata, RecordData, RecordDeps}; use string::NickelString; use crate::{ - destructuring::Pattern, error::{EvalError, ParseError}, eval::cache::CacheIndex, eval::Environment, diff --git a/core/src/destructuring.rs b/core/src/term/pattern.rs similarity index 88% rename from core/src/destructuring.rs rename to core/src/term/pattern.rs index 08ddf9d76..def5f972a 100644 --- a/core/src/destructuring.rs +++ b/core/src/term/pattern.rs @@ -19,7 +19,7 @@ pub enum PatternData { /// corresponding identfier. Any(LocIdent), /// A record pattern as in `{ a = { b, c } }` - RecordPattern(RecordPattern), + Record(RecordPattern), } /// A generic pattern, that can appear in a match expression (not yet implemented) or in a @@ -27,7 +27,7 @@ pub enum PatternData { #[derive(Debug, PartialEq, Clone)] pub struct Pattern { /// The content of this pattern - pub pattern: PatternData, + pub data: PatternData, /// A potential alias for this pattern, capturing the whole matched value. In the source /// language, an alias is introduced by `x @ `, where `x` is an arbitrary identifier. pub alias: Option, @@ -42,12 +42,12 @@ pub struct FieldPattern { /// The name of the matched field. For example, in `{..., foo = {bar, baz}, ...}`, the matched /// identifier is `foo`. pub matched_id: LocIdent, - /// The potentital decoration of the pattern, such as a type annotation, contract annotations, - /// or a default value. - pub decoration: Field, + /// Potential extra annotations of this field pattern, such as a type annotation, contract + /// annotations, or a default value, represented as record field. + pub extra: Field, /// The pattern on the right-hand side of the `=`. A pattern like `{foo, bar}`, without the `=` - /// sign, is considered to be `{foo=foo, bar=bar}`. In this case, `pattern` will be - /// [Pattern::Any]. + /// sign, is parsed as `{foo=foo, bar=bar}`. In this case, `pattern.data` will be + /// [PatternData::Any]. pub pattern: Pattern, pub span: RawSpan, } @@ -57,6 +57,8 @@ pub struct FieldPattern { /// pattern of the data structure: currently, ellipsis matches are only supported for record, but /// we'll probably support them for arrays as well. /// +/// This enum is mostly used during parsing. +/// /// # Example /// /// - In `{foo={}, bar}`, the last match is an normal match. @@ -116,13 +118,13 @@ impl RecordPattern { /// /// This function aims to raise errors in the first case, but maintain the /// behaviour in the second case. - pub fn check_matches(&self) -> Result<(), ParseError> { - let mut matches = HashMap::new(); + pub fn check_dup(&self) -> Result<(), ParseError> { + let mut bindings = HashMap::new(); for pat in self.patterns.iter() { let binding = pat.matched_id; let label = binding.label().to_owned(); - match matches.entry(label) { + match bindings.entry(label) { Entry::Occupied(occupied_entry) => { return Err(ParseError::DuplicateIdentInRecordPattern { ident: binding, @@ -149,10 +151,10 @@ impl RecordPattern { } impl FieldPattern { - /// Convert this field pattern to a field binding with metadata. It's used to generate the + /// Convert this field pattern to a record field binding with metadata. Used to generate the /// record contract associated to a record pattern. pub fn as_record_binding(&self) -> (LocIdent, Field) { - let mut decoration = self.decoration.clone(); + let mut decoration = self.extra.clone(); // If the inner pattern gives rise to a contract, add it the to the field decoration. decoration @@ -167,10 +169,10 @@ impl FieldPattern { // We don't implement elaborate_contract for `FieldPattern`, which is of a slightly different // nature (it's a part of a record pattern). Instead, we call to `FieldPattern::as_record_binding`, -// which takes care of elaborating to an appropriate record field. +// which takes care of elaborating a field pattern to an appropriate record field. pub trait ElaborateContract { /// Elaborate a contract from this pattern. The contract will check both the structure of the - /// matched value (e.g. the presence of fields in a record) and incoporate user provided + /// matched value (e.g. the presence of fields in a record) and incoporate user-provided /// contracts and annotations, as well as default values. /// /// Some patterns don't give rise to any contract (e.g. `Any`), in which case this function @@ -182,14 +184,14 @@ impl ElaborateContract for PatternData { fn elaborate_contract(&self) -> Option { match self { PatternData::Any(_) => None, - PatternData::RecordPattern(pat) => pat.elaborate_contract(), + PatternData::Record(pat) => pat.elaborate_contract(), } } } impl ElaborateContract for Pattern { fn elaborate_contract(&self) -> Option { - self.pattern.elaborate_contract() + self.data.elaborate_contract() } } diff --git a/core/src/transform/desugar_destructuring.rs b/core/src/transform/desugar_destructuring.rs index 9ff6e92ed..61ff9b93e 100644 --- a/core/src/transform/desugar_destructuring.rs +++ b/core/src/transform/desugar_destructuring.rs @@ -38,9 +38,9 @@ //! //! ) in ... //! ``` -use crate::destructuring::*; use crate::identifier::LocIdent; use crate::match_sharedterm; +use crate::term::pattern::*; use crate::term::{ make::{op1, op2}, BinaryOp::DynRemove, @@ -122,14 +122,14 @@ impl Desugar for Pattern { if let Some(alias) = self.alias { let pos = body.pos; let inner = RichTerm::new( - self.pattern + self.data .desugar(RichTerm::new(Term::Var(alias), alias.pos), body), pos, ); Term::Let(alias, destr, inner, LetAttrs::default()) } else { - self.pattern.desugar(destr, body) + self.data.desugar(destr, body) } } } @@ -139,7 +139,7 @@ impl Desugar for PatternData { match self { // If the pattern is an unconstrained identifier, we just bind it to the value. PatternData::Any(id) => Term::Let(id, destr, body, LetAttrs::default()), - PatternData::RecordPattern(pat) => pat.desugar(destr, body), + PatternData::Record(pat) => pat.desugar(destr, body), } } } diff --git a/core/src/transform/free_vars.rs b/core/src/transform/free_vars.rs index 6b216399e..a3dba464d 100644 --- a/core/src/transform/free_vars.rs +++ b/core/src/transform/free_vars.rs @@ -4,8 +4,8 @@ //! the recursive fields that actually appear in the definition of each field when computing the //! fixpoint. use crate::{ - destructuring::*, identifier::Ident, + term::pattern::*, term::{ record::{Field, FieldDeps, RecordDeps}, IndexMap, RichTerm, SharedTerm, StrChunk, Term, @@ -240,7 +240,7 @@ impl RemoveBindings for PatternData { PatternData::Any(id) => { working_set.remove(&id.ident()); } - PatternData::RecordPattern(record_pat) => { + PatternData::Record(record_pat) => { record_pat.remove_bindings(working_set); } } @@ -249,7 +249,7 @@ impl RemoveBindings for PatternData { impl RemoveBindings for Pattern { fn remove_bindings(&self, working_set: &mut HashSet) { - self.pattern.remove_bindings(working_set); + self.data.remove_bindings(working_set); if let Some(alias) = self.alias { working_set.remove(&alias.ident()); diff --git a/core/src/typecheck/mod.rs b/core/src/typecheck/mod.rs index 749400532..24c7c9fcc 100644 --- a/core/src/typecheck/mod.rs +++ b/core/src/typecheck/mod.rs @@ -74,20 +74,20 @@ use std::{ num::NonZeroU16, }; -mod destructuring; pub mod error; pub mod operation; +mod pattern; pub mod reporting; #[macro_use] pub mod mk_uniftype; pub mod eq; pub mod unif; -use destructuring::PatternTypes; use eq::{SimpleTermEnvironment, TermEnvironment}; use error::*; use indexmap::IndexMap; use operation::{get_bop_type, get_nop_type, get_uop_type}; +use pattern::PatternTypes; use unif::*; /// The max depth parameter used to limit the work performed when inferring the type of the stdlib. @@ -1467,7 +1467,7 @@ fn walk( walk(state, ctxt, visitor, t) } Term::FunPattern(pat, t) => { - let (_, pat_bindings) = pat.pattern_types(state, &ctxt, destructuring::TypecheckMode::Walk)?; + let (_, pat_bindings) = pat.pattern_types(state, &ctxt, pattern::TypecheckMode::Walk)?; ctxt.type_env.extend(pat_bindings.into_iter().map(|(id, typ)| (id.ident(), typ))); walk(state, ctxt, visitor, t) @@ -1518,7 +1518,7 @@ fn walk( // data, which doesn't take into account the potential heading alias `x @ `. // This is on purpose, as the alias has been treated separately, so we don't want to // shadow it with a less precise type. - let (_, pat_bindings) = pat.pattern.pattern_types(state, &ctxt, destructuring::TypecheckMode::Walk)?; + let (_, pat_bindings) = pat.data.pattern_types(state, &ctxt, pattern::TypecheckMode::Walk)?; for (id, typ) in pat_bindings { visitor.visit_ident(&id, typ.clone()); @@ -1824,8 +1824,8 @@ fn check( Term::FunPattern(pat, t) => { // See [^separate-alias-treatment]. let (pat_ty, pat_bindings) = - pat.pattern - .pattern_types(state, &ctxt, destructuring::TypecheckMode::Enforce)?; + pat.data + .pattern_types(state, &ctxt, pattern::TypecheckMode::Enforce)?; let src = pat_ty; let trg = state.table.fresh_type_uvar(ctxt.var_level); @@ -1886,8 +1886,8 @@ fn check( Term::LetPattern(pat, re, rt) => { // See [^separate-alias-treatment]. let (pat_ty, pat_bindings) = - pat.pattern - .pattern_types(state, &ctxt, destructuring::TypecheckMode::Enforce)?; + pat.data + .pattern_types(state, &ctxt, pattern::TypecheckMode::Enforce)?; // The inferred type of the expr being bound let ty_let = binding_type(state, re.as_ref(), &ctxt, true); diff --git a/core/src/typecheck/destructuring.rs b/core/src/typecheck/pattern.rs similarity index 95% rename from core/src/typecheck/destructuring.rs rename to core/src/typecheck/pattern.rs index fcdd78962..338fe3135 100644 --- a/core/src/typecheck/destructuring.rs +++ b/core/src/typecheck/pattern.rs @@ -1,8 +1,8 @@ use crate::{ - destructuring::*, error::TypecheckError, identifier::LocIdent, mk_uty_record_row, + term::pattern::*, typ::{RecordRowF, RecordRowsF, TypeF}, }; @@ -16,6 +16,8 @@ pub(super) enum TypecheckMode { Enforce, } +pub type TypeBindings = Vec<(LocIdent, UnifType)>; + pub(super) trait PatternTypes { /// The type produced by the pattern. Depending on the nature of the pattern, this type may /// vary: for example, a record pattern will record rows, while a general pattern will produce @@ -36,7 +38,7 @@ pub(super) trait PatternTypes { state: &mut State, ctxt: &Context, mode: TypecheckMode, - ) -> Result<(Self::PatType, Vec<(LocIdent, UnifType)>), TypecheckError> { + ) -> Result<(Self::PatType, TypeBindings), TypecheckError> { let mut bindings = Vec::new(); let typ = self.pattern_types_inj(&mut bindings, state, ctxt, mode)?; Ok((typ, bindings)) @@ -112,9 +114,7 @@ impl PatternTypes for Pattern { ctxt: &Context, mode: TypecheckMode, ) -> Result { - let typ = self - .pattern - .pattern_types_inj(bindings, state, ctxt, mode)?; + let typ = self.data.pattern_types_inj(bindings, state, ctxt, mode)?; if let Some(alias) = self.alias { bindings.push((alias, typ.clone())); @@ -145,7 +145,7 @@ impl PatternTypes for PatternData { Ok(typ) } - PatternData::RecordPattern(record_pat) => Ok(UnifType::concrete(TypeF::Record( + PatternData::Record(record_pat) => Ok(UnifType::concrete(TypeF::Record( record_pat.pattern_types_inj(bindings, state, ctxt, mode)?, ))), } @@ -166,8 +166,8 @@ impl PatternTypes for FieldPattern { // them with the pattern type we've built to ensure (1) that they're mutually compatible // and (2) that we assign the annotated types to the right unification variables. let ty_row = match ( - &self.decoration.metadata.annotation.typ, - &self.pattern.pattern, + &self.extra.metadata.annotation.typ, + &self.pattern.data, mode, ) { // However, in walk mode, we only do that when the nested pattern isn't a leaf (i.e. diff --git a/lsp/nls/src/pattern.rs b/lsp/nls/src/pattern.rs index 247fb77b1..da12168e7 100644 --- a/lsp/nls/src/pattern.rs +++ b/lsp/nls/src/pattern.rs @@ -1,10 +1,13 @@ //! Pattern analysis. -use nickel_lang_core::{destructuring::*, identifier::LocIdent, term::record::Field}; +use nickel_lang_core::{ + identifier::LocIdent, + term::{pattern::*, record::Field}, +}; pub(super) trait Bindings { - /// Returns a list of all variables bound by this pattern, together with the path to the field they - /// match and the associated decoration. + /// Returns a list of all variables bound by this pattern, together with the path to the field + /// they match and the associated extra annotations. /// /// # Example /// @@ -19,9 +22,9 @@ pub(super) trait Bindings { } trait InjectBindings { - /// Same as [Self::bindings], but work relative to a current path inside a pattern and injects - /// the bindings into a working vector instead of returning the result. This method is mostly - /// used internally and is the one performing the actual work. + /// Same as [Bindings::bindings], but work relative to a current path inside a pattern and + /// injects the bindings into a working vector instead of returning the result. This method is + /// mostly used internally and is the one performing the actual work. /// /// Other modules of the LSP should use [Bindings::bindings] directly. /// @@ -29,15 +32,15 @@ trait InjectBindings { /// /// - `bindings`: the vector to inject the bindings into. /// - `path`: the field path to the sub-pattern being analysed. - /// - `decoration`: the decoration associated with a potential parent field pattern. For - /// example, when injecting the bindings of `{foo ? 5 = x @ y @ z}`, all the introduced - /// variables should refer to the decoration of `foo`. This decoration is thus passed along - /// when calling to the sub-patterns' [Bindings::inject_bindings]. + /// - `parent_extra`: the extra annotations associated with a potential parent field pattern. + /// For example, when injecting the bindings of `{foo ? 5 = x @ y @ z}`, all the introduced + /// variables should refer to default annotation of `foo`. This annotation is thus passed + /// along when calling to the sub-patterns' [Self::inject_bindings]. fn inject_bindings( &self, bindings: &mut Vec<(Vec, LocIdent, Field)>, path: Vec, - parent_deco: Option<&Field>, + parent_extra: Option<&Field>, ); } @@ -64,7 +67,7 @@ impl InjectBindings for Pattern { )); } - self.pattern.inject_bindings(bindings, path, parent_deco); + self.data.inject_bindings(bindings, path, parent_deco); } } @@ -79,7 +82,7 @@ impl InjectBindings for PatternData { PatternData::Any(id) => { bindings.push((path, *id, parent_deco.cloned().unwrap_or_default())) } - PatternData::RecordPattern(record_pat) => { + PatternData::Record(record_pat) => { record_pat.inject_bindings(bindings, path, parent_deco) } } @@ -91,11 +94,11 @@ impl InjectBindings for RecordPattern { &self, bindings: &mut Vec<(Vec, LocIdent, Field)>, path: Vec, - _parent_deco: Option<&Field>, + _parent_extra: Option<&Field>, ) { for field_pat in self.patterns.iter() { - // Field patterns have their own decoration, so there's no need to propagate - // `parent_deco` any further + // Field patterns have their own annotation, so there's no need to propagate + // `parent_extra` any further field_pat.inject_bindings(bindings, path.clone(), None); } } @@ -106,10 +109,10 @@ impl InjectBindings for FieldPattern { &self, bindings: &mut Vec<(Vec, LocIdent, Field)>, mut path: Vec, - _parent_deco: Option<&Field>, + _parent_extra: Option<&Field>, ) { path.push(self.matched_id); self.pattern - .inject_bindings(bindings, path, Some(&self.decoration)); + .inject_bindings(bindings, path, Some(&self.extra)); } } diff --git a/lsp/nls/src/position.rs b/lsp/nls/src/position.rs index 1ada3ce7d..4b42dac36 100644 --- a/lsp/nls/src/position.rs +++ b/lsp/nls/src/position.rs @@ -2,9 +2,11 @@ use std::ops::Range; use codespan::ByteIndex; use nickel_lang_core::{ - destructuring::{PatternData, RecordPatternTail}, position::TermPos, - term::{RichTerm, Term, Traverse, TraverseControl}, + term::{ + pattern::{PatternData, RecordPatternTail}, + RichTerm, Term, Traverse, TraverseControl, + }, }; use crate::{identifier::LocIdent, pattern::Bindings, term::RichTermPtr}; @@ -133,7 +135,7 @@ impl PositionLookup { // TODO[pattern]: what about aliased record patterns? // TODO[pattern]: what about nested patterns with tails? - if let PatternData::RecordPattern(record_pat) = &pat.pattern { + if let PatternData::Record(record_pat) = &pat.data { if let RecordPatternTail::Capture(rest) = &record_pat.tail { idents.push(*rest) } From 59782630702e2f9946af12055cc1696b0f15ed79 Mon Sep 17 00:00:00 2001 From: Yann Hamdaoui Date: Fri, 9 Feb 2024 15:12:04 +0100 Subject: [PATCH 6/7] Apply suggestions from code review Co-authored-by: Viktor Kleen --- core/src/transform/desugar_destructuring.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/core/src/transform/desugar_destructuring.rs b/core/src/transform/desugar_destructuring.rs index 61ff9b93e..ba6aeabae 100644 --- a/core/src/transform/desugar_destructuring.rs +++ b/core/src/transform/desugar_destructuring.rs @@ -6,7 +6,7 @@ //! //! ## Let-binding //! -//! The following destruring let-binding: +//! The following destructuring let-binding: //! //! ```text //! let x @ {a, b=d, ..} = {a=1,b=2,c="ignored"} in ... @@ -23,7 +23,7 @@ //! //! ## Function //! -//! The following desctructuring function: +//! The following destructuring function: //! //! ```text //! let f = fun x@{a, b=c} {d ? 2, ..w} => in ... @@ -80,7 +80,7 @@ pub fn desugar_fun(mut pat: Pattern, body: RichTerm) -> Term { ) } -/// Elaborate a contract from the pattern if it is a record pattern and applies to the value before +/// Elaborate a contract from the pattern if it is a record pattern and apply it to the value before /// actually destructuring it. Then convert the let pattern to a sequence of normal let-bindings. pub fn desugar_let(pat: Pattern, bound: RichTerm, body: RichTerm) -> Term { let contract = pat.elaborate_contract(); @@ -145,7 +145,7 @@ impl Desugar for PatternData { } impl Desugar for FieldPattern { - // For a filed pattern, we assume that the `destr` argument is the whole record being + // For a field pattern, we assume that the `destr` argument is the whole record being // destructured. We extract the field from `destr` and desugar the rest of the pattern against // `destr.matched_id`. fn desugar(self, destr: RichTerm, body: RichTerm) -> Term { @@ -158,7 +158,7 @@ impl Desugar for RecordPattern { fn desugar(self, destr: RichTerm, body: RichTerm) -> Term { let pos = body.pos; // The body is the rest of the term being transformed, which contains the code that uses - // the bindings introduced by the pattern. After having extracting all fields from the + // the bindings introduced by the pattern. After having extracted all fields from the // value, we potentially need to capture the rest in a variable for patterns with a // capturing tail. For example, `let {foo, bar, ..rest} = destr in body` should be // desugared as `let foo = destr.foo in let bar = destr.bar in let rest = in body` From e024740c195ca5bb7407dfc37d417460b536d2de Mon Sep 17 00:00:00 2001 From: Yann Hamdaoui Date: Fri, 9 Feb 2024 15:17:22 +0100 Subject: [PATCH 7/7] Post-rebase fixup --- lsp/nls/src/server.rs | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/lsp/nls/src/server.rs b/lsp/nls/src/server.rs index a6984f231..e21b198be 100644 --- a/lsp/nls/src/server.rs +++ b/lsp/nls/src/server.rs @@ -38,6 +38,7 @@ use crate::{ diagnostic::DiagnosticCompat, field_walker::{Def, FieldResolver}, identifier::LocIdent, + pattern::Bindings, requests::{completion, formatting, goto, hover, symbols}, trace::Trace, }; @@ -392,16 +393,16 @@ impl Server { }) .collect() } - (Term::LetPattern(_, pat, value, _), Some(hovered_id)) => { - let (mut path, _, _) = pat - .matches - .iter() - .flat_map(|m| m.to_flattened_bindings()) + (Term::LetPattern(pat, value, _), Some(hovered_id)) => { + let (path, _, _) = pat + .bindings() + .into_iter() .find(|(_path, bound_id, _)| bound_id.ident() == hovered_id.ident)?; - path.reverse(); + let (last, path) = path.split_last()?; let path: Vec<_> = path.iter().map(|id| id.ident()).collect(); let parents = resolver.resolve_path(value, path.iter().copied()); + parents .iter() .filter_map(|parent| {