From 0105eb7382801b56781308ea94b3aeffa6fd867f Mon Sep 17 00:00:00 2001 From: Marmare314 <49279081+Marmare314@users.noreply.github.com> Date: Thu, 13 Apr 2023 16:07:58 +0200 Subject: [PATCH] Fix function sinks (#638) --- src/eval/args.rs | 11 ++++++ src/eval/func.rs | 61 ++++++++++++++++++++++++---------- src/eval/mod.rs | 16 +++------ src/syntax/ast.rs | 6 ++-- src/syntax/parser.rs | 15 ++++++++- tests/typ/compiler/closure.typ | 4 +++ tests/typ/compiler/spread.typ | 34 +++++++++++++++++-- 7 files changed, 111 insertions(+), 36 deletions(-) diff --git a/src/eval/args.rs b/src/eval/args.rs index 129da4632..66ca4a85a 100644 --- a/src/eval/args.rs +++ b/src/eval/args.rs @@ -61,6 +61,17 @@ impl Args { Ok(None) } + /// Consume n positional arguments if possible. + pub fn consume(&mut self, n: usize) -> SourceResult> { + if n > self.items.len() { + bail!(self.span, "not enough arguments"); + } + let vec = self.items.to_vec(); + let (left, right) = vec.split_at(n); + self.items = right.into(); + return Ok(left.into()); + } + /// Consume and cast the first positional argument. /// /// Returns a `missing argument: {what}` error if no positional argument is diff --git a/src/eval/func.rs b/src/eval/func.rs index d64ca732a..582eabd4f 100644 --- a/src/eval/func.rs +++ b/src/eval/func.rs @@ -260,15 +260,22 @@ pub(super) struct Closure { pub name: Option, /// Captured values from outer scopes. pub captured: Scope, - /// The parameter names and default values. Parameters with default value - /// are named parameters. - pub params: Vec<(Ident, Option)>, - /// The name of an argument sink where remaining arguments are placed. - pub sink: Option, + /// The list of parameters. + pub params: Vec, /// The expression the closure should evaluate to. pub body: Expr, } +#[derive(Hash)] +pub enum Param { + /// A positional parameter: `x`. + Pos(Ident), + /// A named parameter with a default value: `draw: false`. + Named(Ident, Value), + /// An argument sink: `..args`. + Sink(Option), +} + impl Closure { /// Call the function in the context with the arguments. #[allow(clippy::too_many_arguments)] @@ -304,21 +311,38 @@ impl Closure { } // Parse the arguments according to the parameter list. - for (param, default) in &closure.params { - vm.define( - param.clone(), - match default { - Some(default) => { - args.named::(param)?.unwrap_or_else(|| default.clone()) + let num_pos_params = + closure.params.iter().filter(|p| matches!(p, Param::Pos(_))).count(); + let num_pos_args = args.to_pos().len() as usize; + let sink_size = num_pos_args.checked_sub(num_pos_params); + + let mut sink = None; + let mut sink_pos_values = None; + for p in &closure.params { + match p { + Param::Pos(ident) => { + vm.define(ident.clone(), args.expect::(ident)?); + } + Param::Sink(ident) => { + sink = ident.clone(); + if let Some(sink_size) = sink_size { + sink_pos_values = Some(args.consume(sink_size)?); } - None => args.expect::(param)?, - }, - ); + } + Param::Named(ident, default) => { + let value = + args.named::(ident)?.unwrap_or_else(|| default.clone()); + vm.define(ident.clone(), value); + } + } } - // Put the remaining arguments into the sink. - if let Some(sink) = &closure.sink { - vm.define(sink.clone(), args.take()); + if let Some(sink) = sink { + let mut remaining_args = args.take(); + if let Some(sink_pos_values) = sink_pos_values { + remaining_args.items.extend(sink_pos_values); + } + vm.define(sink, remaining_args); } // Ensure all arguments have been used. @@ -407,7 +431,8 @@ impl<'a> CapturesVisitor<'a> { match param { ast::Param::Pos(ident) => self.bind(ident), ast::Param::Named(named) => self.bind(named.name()), - ast::Param::Sink(ident) => self.bind(ident), + ast::Param::Sink(Some(ident)) => self.bind(ident), + _ => {} } } diff --git a/src/eval/mod.rs b/src/eval/mod.rs index 5a4504816..1b3c6ea3a 100644 --- a/src/eval/mod.rs +++ b/src/eval/mod.rs @@ -1149,24 +1149,17 @@ impl Eval for ast::Closure { visitor.finish() }; - let mut params = Vec::new(); - let mut sink = None; - // Collect parameters and an optional sink parameter. + let mut params = Vec::new(); for param in self.params().children() { match param { ast::Param::Pos(name) => { - params.push((name, None)); + params.push(Param::Pos(name)); } ast::Param::Named(named) => { - params.push((named.name(), Some(named.expr().eval(vm)?))); - } - ast::Param::Sink(name) => { - if sink.is_some() { - bail!(name.span(), "only one argument sink is allowed"); - } - sink = Some(name); + params.push(Param::Named(named.name(), named.expr().eval(vm)?)); } + ast::Param::Sink(name) => params.push(Param::Sink(name)), } } @@ -1176,7 +1169,6 @@ impl Eval for ast::Closure { name, captured, params, - sink, body: self.body(), }; diff --git a/src/syntax/ast.rs b/src/syntax/ast.rs index 4119802ea..e492297ac 100644 --- a/src/syntax/ast.rs +++ b/src/syntax/ast.rs @@ -1570,7 +1570,7 @@ pub enum Param { /// A named parameter with a default value: `draw: false`. Named(Named), /// An argument sink: `..args`. - Sink(Ident), + Sink(Option), } impl AstNode for Param { @@ -1578,7 +1578,7 @@ impl AstNode for Param { match node.kind() { SyntaxKind::Ident => node.cast().map(Self::Pos), SyntaxKind::Named => node.cast().map(Self::Named), - SyntaxKind::Spread => node.cast_first_match().map(Self::Sink), + SyntaxKind::Spread => Some(Self::Sink(node.cast_first_match())), _ => Option::None, } } @@ -1587,7 +1587,7 @@ impl AstNode for Param { match self { Self::Pos(v) => v.as_untyped(), Self::Named(v) => v.as_untyped(), - Self::Sink(v) => v.as_untyped(), + Self::Sink(_) => self.as_untyped(), } } } diff --git a/src/syntax/parser.rs b/src/syntax/parser.rs index 42183f3a7..7c05eebcd 100644 --- a/src/syntax/parser.rs +++ b/src/syntax/parser.rs @@ -1069,6 +1069,7 @@ fn validate_dict(p: &mut Parser, m: Marker) { } fn validate_params(p: &mut Parser, m: Marker) { + let mut used_spread = false; let mut used = HashSet::new(); for child in p.post_process(m) { match child.kind() { @@ -1086,12 +1087,24 @@ fn validate_params(p: &mut Parser, m: Marker) { } SyntaxKind::Spread => { let Some(within) = child.children_mut().last_mut() else { continue }; - if within.kind() != SyntaxKind::Ident { + if used_spread { + child.convert_to_error("only one argument sink is allowed"); + continue; + } + used_spread = true; + if within.kind() == SyntaxKind::Dots { + continue; + } else if within.kind() != SyntaxKind::Ident { within.convert_to_error(eco_format!( "expected identifier, found {}", within.kind().name(), )); child.make_erroneous(); + continue; + } + if !used.insert(within.text().clone()) { + within.convert_to_error("duplicate parameter"); + child.make_erroneous(); } } SyntaxKind::LeftParen | SyntaxKind::RightParen | SyntaxKind::Comma => {} diff --git a/tests/typ/compiler/closure.typ b/tests/typ/compiler/closure.typ index bdd8a549c..a1e51d56c 100644 --- a/tests/typ/compiler/closure.typ +++ b/tests/typ/compiler/closure.typ @@ -155,6 +155,10 @@ // Error: 35-36 duplicate parameter #let f(a, b, a: none, b: none, c, b) = none +--- +// Error: 13-14 duplicate parameter +#let f(a, ..a) = none + --- // Error: 7-17 expected identifier, named pair or argument sink, found keyed pair #((a, "named": b) => none) diff --git a/tests/typ/compiler/spread.typ b/tests/typ/compiler/spread.typ index 690e89313..1bfef7b08 100644 --- a/tests/typ/compiler/spread.typ +++ b/tests/typ/compiler/spread.typ @@ -27,7 +27,7 @@ #{ let save(..args) = { test(type(args), "arguments") - test(repr(args), "(1, 2, three: true)") + test(repr(args), "(three: true, 1, 2)") } save(1, 2, three: true) @@ -55,6 +55,11 @@ #f(..if false {}) #f(..for x in () []) +--- +// unnamed spread +#let f(.., a) = a +#test(f(1, 2, 3), 3) + --- // Error: 13-19 cannot spread string #calc.min(.."nope") @@ -64,7 +69,7 @@ #let f(..true) = none --- -// Error: 15-16 only one argument sink is allowed +// Error: 13-16 only one argument sink is allowed #let f(..a, ..b) = none --- @@ -91,3 +96,28 @@ --- // Error: 5-11 cannot spread array into dictionary #(..(1, 2), a: 1) + +--- +// Spread at beginning. +#{ + let f(..a, b) = (a, b) + test(repr(f(1)), "((), 1)") + test(repr(f(1, 2, 3)), "((1, 2), 3)") + test(repr(f(1, 2, 3, 4, 5)), "((1, 2, 3, 4), 5)") +} + +--- +// Spread in the middle. +#{ + let f(a, ..b, c) = (a, b, c) + test(repr(f(1, 2)), "(1, (), 2)") + test(repr(f(1, 2, 3, 4, 5)), "(1, (2, 3, 4), 5)") +} + +--- +#{ + let f(..a, b, c, d) = none + + // Error: 4-10 missing argument: d + f(1, 2) +}