From cbfd9884a94b55486f9b07296f23a01b34d080bd Mon Sep 17 00:00:00 2001 From: Laurenz Date: Fri, 27 Oct 2023 13:24:37 +0200 Subject: [PATCH] Fix argument parsing bug Things like `luma(1, key: "val")` didn't produce an error before because `args.finish()?` wasn't called. This changes `args: Args` to `args: &mut Args` to make it impossible for that to happen. --- crates/typst-macros/src/func.rs | 2 +- crates/typst/src/eval/array.rs | 115 ++++++++++++++---------------- crates/typst/src/eval/func.rs | 10 +-- crates/typst/src/geom/color.rs | 21 ++---- crates/typst/src/geom/gradient.rs | 13 ++-- tests/typ/compiler/array.typ | 14 +++- tests/typ/compute/construct.typ | 4 ++ 7 files changed, 90 insertions(+), 89 deletions(-) diff --git a/crates/typst-macros/src/func.rs b/crates/typst-macros/src/func.rs index 87d57f19d..5b8501d0f 100644 --- a/crates/typst-macros/src/func.rs +++ b/crates/typst-macros/src/func.rs @@ -317,7 +317,7 @@ fn create_wrapper_closure(func: &Func) -> TokenStream { .map(|tokens| quote! { #tokens, }); let vm_ = func.special.vm.then(|| quote! { vm, }); let vt_ = func.special.vt.then(|| quote! { &mut vm.vt, }); - let args_ = func.special.args.then(|| quote! { args.take(), }); + let args_ = func.special.args.then(|| quote! { args, }); let span_ = func.special.span.then(|| quote! { args.span, }); let forwarded = func.params.iter().filter(|param| !param.external).map(bind); quote! { diff --git a/crates/typst/src/eval/array.rs b/crates/typst/src/eval/array.rs index f139907f6..22e38f89b 100644 --- a/crates/typst/src/eval/array.rs +++ b/crates/typst/src/eval/array.rs @@ -354,7 +354,7 @@ impl Array { pub fn range( /// The real arguments (the other arguments are just for the docs, this /// function is a bit involved, so we parse the arguments manually). - args: Args, + args: &mut Args, /// The start of the range (inclusive). #[external] #[default] @@ -367,13 +367,11 @@ impl Array { #[default(NonZeroI64::new(1).unwrap())] step: NonZeroI64, ) -> SourceResult { - let mut args = args; let first = args.expect::("end")?; let (start, end) = match args.eat::()? { Some(second) => (first, second), None => (0, first), }; - args.finish()?; let step = step.get(); @@ -412,15 +410,15 @@ impl Array { /// transformed with the given function. #[func] pub fn map( - &self, + self, /// The virtual machine. vm: &mut Vm, /// The function to apply to each item. mapper: Func, ) -> SourceResult { - self.iter() + self.into_iter() .map(|item| { - let args = Args::new(mapper.span(), [item.clone()]); + let args = Args::new(mapper.span(), [item]); mapper.call_vm(vm, args) }) .collect() @@ -433,20 +431,20 @@ impl Array { /// a let binding or for loop. #[func] pub fn enumerate( - &self, + self, /// The index returned for the first pair of the returned list. #[named] #[default(0)] start: i64, ) -> StrResult { - self.iter() + self.into_iter() .enumerate() .map(|(i, value)| { Ok(array![ start .checked_add_unsigned(i as u64) .ok_or("array index is too large")?, - value.clone() + value ] .into_value()) }) @@ -464,33 +462,29 @@ impl Array { /// `{((1, 3, 6), (2, 4, 7), (3, 5, 8))}`. #[func] pub fn zip( - &self, + self, /// The real arguments (the other arguments are just for the docs, this /// function is a bit involved, so we parse the arguments manually). - args: Args, + args: &mut Args, /// The arrays to zip with. #[external] #[variadic] others: Vec, ) -> SourceResult { - let mut args = args; + let remaining = args.remaining(); // Fast path for one array. - if args.remaining() == 0 { - return Ok(self - .iter() - .map(|item| array![item.clone()].into_value()) - .collect()); + if remaining == 0 { + return Ok(self.into_iter().map(|item| array![item].into_value()).collect()); } // Fast path for just two arrays. - if args.remaining() == 1 { + if remaining == 1 { let other = args.expect::("others")?; - args.finish()?; return Ok(self - .iter() + .into_iter() .zip(other) - .map(|(first, second)| array![first.clone(), second].into_value()) + .map(|(first, second)| array![first, second].into_value()) .collect()); } @@ -501,9 +495,8 @@ impl Array { .into_iter() .map(|i| i.into_iter()) .collect::>(); - args.finish()?; - for this in self.iter() { + for this in self { let mut row = Self::with_capacity(1 + iterators.len()); row.push(this.clone()); @@ -524,7 +517,7 @@ impl Array { /// Folds all items into a single value using an accumulator function. #[func] pub fn fold( - &self, + self, /// The virtual machine. vm: &mut Vm, /// The initial value to start with. @@ -534,8 +527,8 @@ impl Array { folder: Func, ) -> SourceResult { let mut acc = init; - for item in self.iter() { - let args = Args::new(folder.span(), [acc, item.clone()]); + for item in self { + let args = Args::new(folder.span(), [acc, item]); acc = folder.call_vm(vm, args)?; } Ok(acc) @@ -544,20 +537,19 @@ impl Array { /// Sums all items (works for all types that can be added). #[func] pub fn sum( - &self, + self, /// What to return if the array is empty. Must be set if the array can /// be empty. #[named] default: Option, ) -> StrResult { - let mut acc = self - .0 - .first() - .cloned() + let mut iter = self.into_iter(); + let mut acc = iter + .next() .or(default) .ok_or("cannot calculate sum of empty array with no default")?; - for i in self.iter().skip(1) { - acc = add(acc, i.clone())?; + for item in iter { + acc = add(acc, item)?; } Ok(acc) } @@ -566,20 +558,19 @@ impl Array { /// multiplied). #[func] pub fn product( - &self, + self, /// What to return if the array is empty. Must be set if the array can /// be empty. #[named] default: Option, ) -> StrResult { - let mut acc = self - .0 - .first() - .cloned() + let mut iter = self.into_iter(); + let mut acc = iter + .next() .or(default) .ok_or("cannot calculate product of empty array with no default")?; - for i in self.iter().skip(1) { - acc = mul(acc, i.clone())?; + for item in iter { + acc = mul(acc, item)?; } Ok(acc) } @@ -587,14 +578,14 @@ impl Array { /// Whether the given function returns `{true}` for any item in the array. #[func] pub fn any( - &self, + self, /// The virtual machine. vm: &mut Vm, /// The function to apply to each item. Must return a boolean. test: Func, ) -> SourceResult { - for item in self.iter() { - let args = Args::new(test.span(), [item.clone()]); + for item in self { + let args = Args::new(test.span(), [item]); if test.call_vm(vm, args)?.cast::().at(test.span())? { return Ok(true); } @@ -606,14 +597,14 @@ impl Array { /// Whether the given function returns `{true}` for all items in the array. #[func] pub fn all( - &self, + self, /// The virtual machine. vm: &mut Vm, /// The function to apply to each item. Must return a boolean. test: Func, ) -> SourceResult { - for item in self.iter() { - let args = Args::new(test.span(), [item.clone()]); + for item in self { + let args = Args::new(test.span(), [item]); if !test.call_vm(vm, args)?.cast::().at(test.span())? { return Ok(false); } @@ -624,13 +615,13 @@ impl Array { /// Combine all nested arrays into a single flat one. #[func] - pub fn flatten(&self) -> Array { + pub fn flatten(self) -> Array { let mut flat = EcoVec::with_capacity(self.0.len()); - for item in self.iter() { + for item in self { if let Value::Array(nested) = item { - flat.extend(nested.flatten().into_iter()); + flat.extend(nested.flatten()); } else { - flat.push(item.clone()); + flat.push(item); } } flat.into() @@ -638,8 +629,8 @@ impl Array { /// Return a new array with the same items, but in reverse order. #[func(title = "Reverse")] - pub fn rev(&self) -> Array { - self.0.iter().cloned().rev().collect() + pub fn rev(self) -> Array { + self.into_iter().rev().collect() } /// Split the array at occurrences of the specified value. @@ -658,7 +649,7 @@ impl Array { /// Combine all items in the array into one. #[func] pub fn join( - &self, + self, /// A value to insert between each item of the array. #[default] separator: Option, @@ -671,7 +662,7 @@ impl Array { let mut last = last; let mut result = Value::None; - for (i, value) in self.iter().cloned().enumerate() { + for (i, value) in self.into_iter().enumerate() { if i > 0 { if i + 1 == len && last.is_some() { result = ops::join(result, last.take().unwrap())?; @@ -690,7 +681,7 @@ impl Array { /// adjacent elements. #[func] pub fn intersperse( - &self, + self, /// The value that will be placed between each adjacent element. separator: Value, ) -> Array { @@ -701,7 +692,7 @@ impl Array { n => (2 * n) - 1, }; let mut vec = EcoVec::with_capacity(size); - let mut iter = self.iter().cloned(); + let mut iter = self.into_iter(); if let Some(first) = iter.next() { vec.push(first); @@ -722,7 +713,7 @@ impl Array { /// function (if given) yields an error. #[func] pub fn sorted( - &self, + self, /// The virtual machine. vm: &mut Vm, /// The callsite span. @@ -733,7 +724,7 @@ impl Array { key: Option, ) -> SourceResult { let mut result = Ok(()); - let mut vec = self.0.clone(); + let mut vec = self.0; let mut key_of = |x: Value| match &key { // NOTE: We are relying on `comemo`'s memoization of function // evaluation to not excessively reevaluate the `key`. @@ -772,7 +763,7 @@ impl Array { /// ``` #[func(title = "Deduplicate")] pub fn dedup( - &self, + self, /// The virtual machine. vm: &mut Vm, /// If given, applies this function to the elements in the array to @@ -791,10 +782,10 @@ impl Array { // This algorithm is O(N^2) because we cannot rely on `HashSet` since: // 1. We would like to preserve the order of the elements. // 2. We cannot hash arbitrary `Value`. - 'outer: for value in self.iter() { + 'outer: for value in self { let key = key_of(value.clone())?; if out.is_empty() { - out.push(value.clone()); + out.push(value); continue; } @@ -804,7 +795,7 @@ impl Array { } } - out.push(value.clone()); + out.push(value); } Ok(Self(out)) diff --git a/crates/typst/src/eval/func.rs b/crates/typst/src/eval/func.rs index d90038502..b75e28fa0 100644 --- a/crates/typst/src/eval/func.rs +++ b/crates/typst/src/eval/func.rs @@ -331,14 +331,17 @@ impl Func { self, /// The real arguments (the other argument is just for the docs). /// The docs argument cannot be called `args`. - args: Args, + args: &mut Args, /// The arguments to apply to the function. #[external] #[variadic] arguments: Vec, ) -> Func { let span = self.span; - Self { repr: Repr::With(Arc::new((self, args))), span } + Self { + repr: Repr::With(Arc::new((self, args.take()))), + span, + } } /// Returns a selector that filters for elements belonging to this function @@ -348,13 +351,12 @@ impl Func { self, /// The real arguments (the other argument is just for the docs). /// The docs argument cannot be called `args`. - args: Args, + args: &mut Args, /// The fields to filter for. #[variadic] #[external] fields: Vec, ) -> StrResult { - let mut args = args; let fields = args.to_named(); args.items.retain(|arg| arg.name.is_none()); Ok(self diff --git a/crates/typst/src/geom/color.rs b/crates/typst/src/geom/color.rs index 260137ea9..8c12483b7 100644 --- a/crates/typst/src/geom/color.rs +++ b/crates/typst/src/geom/color.rs @@ -254,7 +254,7 @@ impl Color { pub fn luma( /// The real arguments (the other arguments are just for the docs, this /// function is a bit involved, so we parse the arguments manually). - args: Args, + args: &mut Args, /// The lightness component. #[external] lightness: Component, @@ -264,7 +264,6 @@ impl Color { #[external] color: Color, ) -> SourceResult { - let mut args = args; Ok(if let Some(color) = args.find::()? { color.to_luma() } else { @@ -300,7 +299,7 @@ impl Color { pub fn oklab( /// The real arguments (the other arguments are just for the docs, this /// function is a bit involved, so we parse the arguments manually). - args: Args, + args: &mut Args, /// The cyan component. #[external] lightness: RatioComponent, @@ -319,7 +318,6 @@ impl Color { #[external] color: Color, ) -> SourceResult { - let mut args = args; Ok(if let Some(color) = args.find::()? { color.to_oklab() } else { @@ -363,7 +361,7 @@ impl Color { pub fn linear_rgb( /// The real arguments (the other arguments are just for the docs, this /// function is a bit involved, so we parse the arguments manually). - args: Args, + args: &mut Args, /// The red component. #[external] red: Component, @@ -382,7 +380,6 @@ impl Color { #[external] color: Color, ) -> SourceResult { - let mut args = args; Ok(if let Some(color) = args.find::()? { color.to_linear_rgb() } else { @@ -421,7 +418,7 @@ impl Color { pub fn rgb( /// The real arguments (the other arguments are just for the docs, this /// function is a bit involved, so we parse the arguments manually). - args: Args, + args: &mut Args, /// The red component. #[external] red: Component, @@ -454,7 +451,6 @@ impl Color { #[external] color: Color, ) -> SourceResult { - let mut args = args; Ok(if let Some(string) = args.find::>()? { Self::from_str(&string.v).at(string.span)? } else if let Some(color) = args.find::()? { @@ -497,7 +493,7 @@ impl Color { pub fn cmyk( /// The real arguments (the other arguments are just for the docs, this /// function is a bit involved, so we parse the arguments manually). - args: Args, + args: &mut Args, /// The cyan component. #[external] cyan: RatioComponent, @@ -516,7 +512,6 @@ impl Color { #[external] color: Color, ) -> SourceResult { - let mut args = args; Ok(if let Some(color) = args.find::()? { color.to_cmyk() } else { @@ -557,7 +552,7 @@ impl Color { pub fn hsl( /// The real arguments (the other arguments are just for the docs, this /// function is a bit involved, so we parse the arguments manually). - args: Args, + args: &mut Args, /// The hue angle. #[external] hue: Angle, @@ -576,7 +571,6 @@ impl Color { #[external] color: Color, ) -> SourceResult { - let mut args = args; Ok(if let Some(color) = args.find::()? { color.to_hsl() } else { @@ -617,7 +611,7 @@ impl Color { pub fn hsv( /// The real arguments (the other arguments are just for the docs, this /// function is a bit involved, so we parse the arguments manually). - args: Args, + args: &mut Args, /// The hue angle. #[external] hue: Angle, @@ -636,7 +630,6 @@ impl Color { #[external] color: Color, ) -> SourceResult { - let mut args = args; Ok(if let Some(color) = args.find::()? { color.to_hsv() } else { diff --git a/crates/typst/src/geom/gradient.rs b/crates/typst/src/geom/gradient.rs index f2281930c..2db484a3c 100644 --- a/crates/typst/src/geom/gradient.rs +++ b/crates/typst/src/geom/gradient.rs @@ -182,7 +182,7 @@ impl Gradient { #[func(title = "Linear Gradient")] pub fn linear( /// The args of this function. - args: Args, + args: &mut Args, /// The call site of this function. span: Span, /// The color [stops](#stops) of the gradient. @@ -212,12 +212,6 @@ impl Gradient { #[external] angle: Angle, ) -> SourceResult { - let mut args = args; - if stops.len() < 2 { - bail!(error!(span, "a gradient must have at least two stops") - .with_hint("try filling the shape with a single color instead")); - } - let angle = if let Some(angle) = args.named::("angle")? { angle } else if let Some(dir) = args.named::("dir")? { @@ -231,6 +225,11 @@ impl Gradient { Angle::rad(0.0) }; + if stops.len() < 2 { + bail!(error!(span, "a gradient must have at least two stops") + .with_hint("try filling the shape with a single color instead")); + } + Ok(Self::Linear(Arc::new(LinearGradient { stops: process_stops(&stops)?, angle, diff --git a/tests/typ/compiler/array.typ b/tests/typ/compiler/array.typ index 4191d6ff9..2ce471587 100644 --- a/tests/typ/compiler/array.typ +++ b/tests/typ/compiler/array.typ @@ -248,6 +248,10 @@ #test((2, 1, 3, -10, -5, 8, 6, -7, 2).sorted(key: x => x), (-10, -7, -5, 1, 2, 2, 3, 6, 8)) #test((2, 1, 3, -10, -5, 8, 6, -7, 2).sorted(key: x => x * x), (1, 2, 2, 3, -5, 6, -7, 8, -10)) +--- +// Error: 12-18 unexpected argument +#().sorted(x => x) + --- // Test the `zip` method. #test(().zip(()), ()) @@ -262,7 +266,7 @@ #test((1,).zip((2,), (3,)), ((1, 2, 3),)) #test((1, 2, 3).zip(), ((1,), (2,), (3,))) #test(array.zip(()), ()) -#test(array.zip(("a", "b")), (("a",), ("b",))) + --- // Test the `enumerate` method. @@ -289,6 +293,14 @@ #test(("Hello", "World", "Hi", "There").dedup(key: x => x.len()), ("Hello", "Hi")) #test(("Hello", "World", "Hi", "There").dedup(key: x => x.at(0)), ("Hello", "World", "There")) +--- +// Error: 9-26 unexpected argument: val +#().zip(val: "applicable") + +--- +// Error: 13-30 unexpected argument: val +#().zip((), val: "applicable") + --- // Error: 32-37 cannot divide by zero #(1, 2, 0, 3).sorted(key: x => 5 / x) diff --git a/tests/typ/compute/construct.typ b/tests/typ/compute/construct.typ index d5ded96a0..58693a49a 100644 --- a/tests/typ/compute/construct.typ +++ b/tests/typ/compute/construct.typ @@ -116,6 +116,10 @@ // Error: 21-26 expected integer or ratio, found boolean #rgb(10%, 20%, 30%, false) +--- +// Error: 10-20 unexpected argument: key +#luma(1, key: "val") + --- // Error: 12-24 expected float or ratio, found string // Error: 26-39 expected float or ratio, found string