Refactor impl Eval for ast::FuncCall<'_> (#4435)

Co-authored-by: Ian Wrzesinski <133046678+wrzian@users.noreply.github.com>

crates/typst/src/eval/call.rs

@@ -1,7 +1,10 @@
 use comemo::{Tracked, TrackedMut};
 use ecow::{eco_format, EcoVec};
 
-use crate::diag::{bail, error, At, HintedStrResult, SourceResult, Trace, Tracepoint};
+use crate::diag::{
+    bail, error, At, HintedStrResult, HintedString, SourceDiagnostic, SourceResult,
+    Trace, Tracepoint,
+};
 use crate::engine::{Engine, Sink, Traced};
 use crate::eval::{Access, Eval, FlowEvent, Route, Vm};
 use crate::foundations::{
@@ -10,7 +13,7 @@ use crate::foundations::{
 };
 use crate::introspection::Introspector;
 use crate::math::LrElem;
-use crate::syntax::ast::{self, AstNode};
+use crate::syntax::ast::{self, AstNode, Ident};
 use crate::syntax::{Span, Spanned, SyntaxNode};
 use crate::text::TextElem;
 use crate::utils::LazyHash;
@@ -32,135 +35,25 @@ impl Eval for ast::FuncCall<'_> {
         }
 
         // Try to evaluate as a call to an associated function or field.
-        let (callee, mut args) = if let ast::Expr::FieldAccess(access) = callee {
+        let (callee, args) = if let ast::Expr::FieldAccess(access) = callee {
             let target = access.target();
-            let target_span = target.span();
             let field = access.field();
-            let field_span = field.span();
+            match eval_field_call(target, field, args, span, vm)? {
-
+                FieldCall::Normal(callee, args) => (callee, args),
-            let target = if is_mutating_method(&field) {
+                FieldCall::Resolved(value) => return Ok(value),
-                let mut args = args.eval(vm)?.spanned(span);
-                let target = target.access(vm)?;
-
-                // Only arrays and dictionaries have mutable methods.
-                if matches!(target, Value::Array(_) | Value::Dict(_)) {
-                    args.span = span;
-                    let point = || Tracepoint::Call(Some(field.get().clone()));
-                    return call_method_mut(target, &field, args, span).trace(
-                        vm.world(),
-                        point,
-                        span,
-                    );
-                }
-
-                target.clone()
-            } else {
-                access.target().eval(vm)?
-            };
-
-            let mut args = args.eval(vm)?.spanned(span);
-
-            // Handle plugins.
-            if let Value::Plugin(plugin) = &target {
-                let bytes = args.all::<Bytes>()?;
-                args.finish()?;
-                return Ok(plugin.call(&field, bytes).at(span)?.into_value());
-            }
-
-            // Prioritize associated functions on the value's type (i.e.,
-            // methods) over its fields. A function call on a field is only
-            // allowed for functions, types, modules (because they are scopes),
-            // and symbols (because they have modifiers).
-            //
-            // For dictionaries, it is not allowed because it would be ambiguous
-            // (prioritizing associated functions would make an addition of a
-            // new associated function a breaking change and prioritizing fields
-            // would break associated functions for certain dictionaries).
-            if let Some(callee) = target.ty().scope().get(&field) {
-                let this = Arg {
-                    span: target_span,
-                    name: None,
-                    value: Spanned::new(target, target_span),
-                };
-                args.span = span;
-                args.items.insert(0, this);
-                (callee.clone(), args)
-            } else if matches!(
-                target,
-                Value::Symbol(_) | Value::Func(_) | Value::Type(_) | Value::Module(_)
-            ) {
-                (target.field(&field).at(field_span)?, args)
-            } else {
-                let mut error = error!(
-                    field_span,
-                    "type {} has no method `{}`",
-                    target.ty(),
-                    field.as_str()
-                );
-
-                let mut field_hint = || {
-                    if target.field(&field).is_ok() {
-                        error.hint(eco_format!(
-                            "did you mean to access the field `{}`?",
-                            field.as_str()
-                        ));
-                    }
-                };
-
-                match target {
-                    Value::Dict(ref dict) => {
-                        if matches!(dict.get(&field), Ok(Value::Func(_))) {
-                            error.hint(eco_format!(
-                                "to call the function stored in the dictionary, surround \
-                                 the field access with parentheses, e.g. `(dict.{})(..)`",
-                               field.as_str(),
-                            ));
-                        } else {
-                            field_hint();
-                        }
-                    }
-                    _ => field_hint(),
-                }
-
-                bail!(error);
             }
         } else {
+            // Function call order: we evaluate the callee before the arguments.
             (callee.eval(vm)?, args.eval(vm)?.spanned(span))
         };
 
         let func_result = callee.clone().cast::<Func>();
         if in_math && func_result.is_err() {
-            // For non-functions in math, we wrap the arguments in parentheses.
+            return wrap_args_in_math(callee, callee_span, args, trailing_comma);
-            let mut body = Content::empty();
-            for (i, arg) in args.all::<Content>()?.into_iter().enumerate() {
-                if i > 0 {
-                    body += TextElem::packed(',');
-                }
-                body += arg;
-            }
-            if trailing_comma {
-                body += TextElem::packed(',');
-            }
-            return Ok(Value::Content(
-                callee.display().spanned(callee_span)
-                    + LrElem::new(TextElem::packed('(') + body + TextElem::packed(')'))
-                        .pack(),
-            ));
         }
 
         let func = func_result
-            .map_err(|mut err| {
+            .map_err(|err| hint_if_shadowed_std(vm, &self.callee(), err))
-                if let ast::Expr::Ident(ident) = self.callee() {
-                    let ident = ident.get();
-                    if vm.scopes.check_std_shadowed(ident) {
-                        err.hint(eco_format!(
-                        "use `std.{}` to access the shadowed standard library function",
-                        ident,
-                    ));
-                    }
-                }
-                err
-            })
             .at(callee_span)?;
 
         let point = || Tracepoint::Call(func.name().map(Into::into));
@@ -368,6 +261,108 @@ pub(crate) fn call_closure(
     Ok(output)
 }
 
+/// This used only as the return value of `eval_field_call`.
+/// - `Normal` means that we have a function to call and the arguments to call it with.
+/// - `Resolved` means that we have already resolved the call and have the value.
+enum FieldCall {
+    Normal(Value, Args),
+    Resolved(Value),
+}
+
+/// Evaluate a field call's callee and arguments.
+///
+/// This follows the normal function call order: we evaluate the callee before the
+/// arguments.
+///
+/// Prioritize associated functions on the value's type (e.g., methods) over its fields.
+/// A function call on a field is only allowed for functions, types, modules (because
+/// they are scopes), and symbols (because they have modifiers or associated functions).
+///
+/// For dictionaries, it is not allowed because it would be ambiguous - prioritizing
+/// associated functions would make an addition of a new associated function a breaking
+/// change and prioritizing fields would break associated functions for certain
+/// dictionaries.
+fn eval_field_call(
+    target_expr: ast::Expr,
+    field: Ident,
+    args: ast::Args,
+    span: Span,
+    vm: &mut Vm,
+) -> SourceResult<FieldCall> {
+    // Evaluate the field-call's target and overall arguments.
+    let (target, mut args) = if is_mutating_method(&field) {
+        // If `field` looks like a mutating method, we evaluate the arguments first,
+        // because `target_expr.access(vm)` mutably borrows the `vm`, so that we can't
+        // evaluate the arguments after it.
+        let args = args.eval(vm)?.spanned(span);
+        // However, this difference from the normal call order is not observable because
+        // expressions like `(1, arr.len(), 2, 3).push(arr.pop())` evaluate the target to
+        // a temporary which we disallow mutation on (returning an error).
+        // Theoretically this could be observed if a method matching `is_mutating_method`
+        // was added to some type in the future and we didn't update this function.
+        match target_expr.access(vm)? {
+            // Only arrays and dictionaries have mutable methods.
+            target @ (Value::Array(_) | Value::Dict(_)) => {
+                let value = call_method_mut(target, &field, args, span);
+                let point = || Tracepoint::Call(Some(field.get().clone()));
+                return Ok(FieldCall::Resolved(value.trace(vm.world(), point, span)?));
+            }
+            target => (target.clone(), args),
+        }
+    } else {
+        let target = target_expr.eval(vm)?;
+        let args = args.eval(vm)?.spanned(span);
+        (target, args)
+    };
+
+    if let Value::Plugin(plugin) = &target {
+        // Call plugins by converting args to bytes.
+        let bytes = args.all::<Bytes>()?;
+        args.finish()?;
+        let value = plugin.call(&field, bytes).at(span)?.into_value();
+        Ok(FieldCall::Resolved(value))
+    } else if let Some(callee) = target.ty().scope().get(&field) {
+        args.insert(0, target_expr.span(), target);
+        Ok(FieldCall::Normal(callee.clone(), args))
+    } else if matches!(
+        target,
+        Value::Symbol(_) | Value::Func(_) | Value::Type(_) | Value::Module(_)
+    ) {
+        // Certain value types may have their own ways to access method fields.
+        // e.g. `$arrow.r(v)$`, `table.cell[..]`
+        let value = target.field(&field).at(field.span())?;
+        Ok(FieldCall::Normal(value, args))
+    } else {
+        // Otherwise we cannot call this field.
+        bail!(missing_field_call_error(target, field))
+    }
+}
+
+/// Produce an error when we cannot call the field.
+fn missing_field_call_error(target: Value, field: Ident) -> SourceDiagnostic {
+    let mut error =
+        error!(field.span(), "type {} has no method `{}`", target.ty(), field.as_str());
+
+    match target {
+        Value::Dict(ref dict) if matches!(dict.get(&field), Ok(Value::Func(_))) => {
+            error.hint(eco_format!(
+                "to call the function stored in the dictionary, surround \
+                the field access with parentheses, e.g. `(dict.{})(..)`",
+                field.as_str(),
+            ));
+        }
+        _ if target.field(&field).is_ok() => {
+            error.hint(eco_format!(
+                "did you mean to access the field `{}`?",
+                field.as_str(),
+            ));
+        }
+        _ => {}
+    }
+    error
+}
+
+/// Check if the expression is in a math context.
 fn in_math(expr: ast::Expr) -> bool {
     match expr {
         ast::Expr::MathIdent(_) => true,
@@ -376,6 +371,46 @@ fn in_math(expr: ast::Expr) -> bool {
     }
 }
 
+/// For non-functions in math, we wrap the arguments in parentheses.
+fn wrap_args_in_math(
+    callee: Value,
+    callee_span: Span,
+    mut args: Args,
+    trailing_comma: bool,
+) -> SourceResult<Value> {
+    let mut body = Content::empty();
+    for (i, arg) in args.all::<Content>()?.into_iter().enumerate() {
+        if i > 0 {
+            body += TextElem::packed(',');
+        }
+        body += arg;
+    }
+    if trailing_comma {
+        body += TextElem::packed(',');
+    }
+    Ok(Value::Content(
+        callee.display().spanned(callee_span)
+            + LrElem::new(TextElem::packed('(') + body + TextElem::packed(')')).pack(),
+    ))
+}
+
+/// Provide a hint if the callee is a shadowed standard library function.
+fn hint_if_shadowed_std(
+    vm: &mut Vm,
+    callee: &ast::Expr,
+    mut err: HintedString,
+) -> HintedString {
+    if let ast::Expr::Ident(ident) = callee {
+        let ident = ident.get();
+        if vm.scopes.check_std_shadowed(ident) {
+            err.hint(eco_format!(
+                "use `std.{ident}` to access the shadowed standard library function",
+            ));
+        }
+    }
+    err
+}
+
 /// A visitor that determines which variables to capture for a closure.
 pub struct CapturesVisitor<'a> {
     external: Option<&'a Scopes<'a>>,

crates/typst/src/foundations/args.rs

@@ -76,6 +76,18 @@ impl Args {
         self.items.iter().filter(|slot| slot.name.is_none()).count()
     }
 
+    /// Insert a positional argument at a specific index.
+    pub fn insert(&mut self, index: usize, span: Span, value: Value) {
+        self.items.insert(
+            index,
+            Arg {
+                span: self.span,
+                name: None,
+                value: Spanned::new(value, span),
+            },
+        )
+    }
+
     /// Push a positional argument.
     pub fn push(&mut self, span: Span, value: Value) {
         self.items.push(Arg {