Added warning when explicit return in code (not markup) discards joined content (#5413)

Co-authored-by: Laurenz <laurmaedje@gmail.com>

crates/typst-eval/src/call.rs

@@ -253,8 +253,8 @@ pub fn eval_closure(
     // Handle control flow.
     let output = body.eval(&mut vm)?;
     match vm.flow {
-        Some(FlowEvent::Return(_, Some(explicit))) => return Ok(explicit),
-        Some(FlowEvent::Return(_, None)) => {}
+        Some(FlowEvent::Return(_, Some(explicit), _)) => return Ok(explicit),
+        Some(FlowEvent::Return(_, None, _)) => {}
         Some(flow) => bail!(flow.forbidden()),
         None => {}
     }

crates/typst-eval/src/code.rs

@@ -1,12 +1,15 @@
 use ecow::{eco_vec, EcoVec};
-use typst_library::diag::{bail, error, At, SourceResult};
+use typst_library::diag::{bail, error, warning, At, SourceResult};
+use typst_library::engine::Engine;
 use typst_library::foundations::{
-    ops, Array, Capturer, Closure, Content, ContextElem, Dict, Func, NativeElement, Str,
-    Value,
+    ops, Array, Capturer, Closure, Content, ContextElem, Dict, Func, NativeElement,
+    Selector, Str, Value,
 };
+use typst_library::introspection::{Counter, State};
 use typst_syntax::ast::{self, AstNode};
+use typst_utils::singleton;
 
-use crate::{CapturesVisitor, Eval, Vm};
+use crate::{CapturesVisitor, Eval, FlowEvent, Vm};
 
 impl Eval for ast::Code<'_> {
     type Output = Value;
@@ -54,7 +57,8 @@ fn eval_code<'a>(
 
         output = ops::join(output, value).at(span)?;
 
-        if vm.flow.is_some() {
+        if let Some(event) = &vm.flow {
+            warn_for_discarded_content(&mut vm.engine, event, &output);
             break;
         }
     }
@@ -358,3 +362,26 @@ impl Eval for ast::Contextual<'_> {
         Ok(ContextElem::new(func).pack())
     }
 }
+
+/// Emits a warning when we discard content while returning unconditionally.
+fn warn_for_discarded_content(engine: &mut Engine, event: &FlowEvent, joined: &Value) {
+    let FlowEvent::Return(span, Some(_), false) = event else { return };
+    let Value::Content(tree) = &joined else { return };
+
+    let selector = singleton!(
+        Selector,
+        Selector::Or(eco_vec![State::select_any(), Counter::select_any()])
+    );
+
+    let mut warning = warning!(
+        *span,
+        "this return unconditionally discards the content before it";
+        hint: "try omitting the `return` to automatically join all values"
+    );
+
+    if tree.query_first(selector).is_some() {
+        warning.hint("state/counter updates are content that must end up in the document to have an effect");
+    }
+
+    engine.sink.warn(warning);
+}

crates/typst-eval/src/flow.rs

@@ -17,8 +17,8 @@ pub enum FlowEvent {
     /// Skip the remainder of the current iteration in a loop.
     Continue(Span),
     /// Stop execution of a function early, optionally returning an explicit
-    /// value.
-    Return(Span, Option<Value>),
+    /// value. The final boolean indicates whether the return was conditional.
+    Return(Span, Option<Value>, bool),
 }
 
 impl FlowEvent {
@@ -31,7 +31,7 @@ impl FlowEvent {
             Self::Continue(span) => {
                 error!(span, "cannot continue outside of loop")
             }
-            Self::Return(span, _) => {
+            Self::Return(span, _, _) => {
                 error!(span, "cannot return outside of function")
             }
         }
@@ -43,13 +43,20 @@ impl Eval for ast::Conditional<'_> {
 
     fn eval(self, vm: &mut Vm) -> SourceResult<Self::Output> {
         let condition = self.condition();
-        if condition.eval(vm)?.cast::<bool>().at(condition.span())? {
-            self.if_body().eval(vm)
+        let output = if condition.eval(vm)?.cast::<bool>().at(condition.span())? {
+            self.if_body().eval(vm)?
         } else if let Some(else_body) = self.else_body() {
-            else_body.eval(vm)
+            else_body.eval(vm)?
         } else {
-            Ok(Value::None)
+            Value::None
+        };
+
+        // Mark the return as conditional.
+        if let Some(FlowEvent::Return(_, _, conditional)) = &mut vm.flow {
+            *conditional = true;
         }
+
+        Ok(output)
     }
 }
 
@@ -95,6 +102,11 @@ impl Eval for ast::WhileLoop<'_> {
             vm.flow = flow;
         }
 
+        // Mark the return as conditional.
+        if let Some(FlowEvent::Return(_, _, conditional)) = &mut vm.flow {
+            *conditional = true;
+        }
+
         Ok(output)
     }
 }
@@ -168,6 +180,11 @@ impl Eval for ast::ForLoop<'_> {
             vm.flow = flow;
         }
 
+        // Mark the return as conditional.
+        if let Some(FlowEvent::Return(_, _, conditional)) = &mut vm.flow {
+            *conditional = true;
+        }
+
         Ok(output)
     }
 }
@@ -200,7 +217,7 @@ impl Eval for ast::FuncReturn<'_> {
     fn eval(self, vm: &mut Vm) -> SourceResult<Self::Output> {
         let value = self.body().map(|body| body.eval(vm)).transpose()?;
         if vm.flow.is_none() {
-            vm.flow = Some(FlowEvent::Return(self.span(), value));
+            vm.flow = Some(FlowEvent::Return(self.span(), value, false));
         }
         Ok(Value::None)
     }

crates/typst-library/src/foundations/content.rs

@@ -426,7 +426,7 @@ impl Content {
     /// selector.
     ///
     /// Elements produced in `show` rules will not be included in the results.
-    pub fn query_first(&self, selector: Selector) -> Option<Content> {
+    pub fn query_first(&self, selector: &Selector) -> Option<Content> {
         let mut result = None;
         self.traverse(&mut |element| {
             if result.is_none() && selector.matches(&element, None) {

crates/typst-library/src/introspection/counter.rs

@@ -393,6 +393,11 @@ impl Counter {
         let context = Context::new(Some(location), styles);
         state.display(engine, context.track(), &numbering)
     }
+
+    /// Selects all state updates.
+    pub fn select_any() -> Selector {
+        CounterUpdateElem::elem().select()
+    }
 }
 
 #[scope]

crates/typst-library/src/introspection/state.rs

@@ -261,6 +261,11 @@ impl State {
     fn selector(&self) -> Selector {
         select_where!(StateUpdateElem, Key => self.key.clone())
     }
+
+    /// Selects all state updates.
+    pub fn select_any() -> Selector {
+        StateUpdateElem::elem().select()
+    }
 }
 
 #[scope]

crates/typst-library/src/model/figure.rs

@@ -257,7 +257,7 @@ impl Synthesize for Packed<FigureElem> {
         // Determine the figure's kind.
         let kind = elem.kind(styles).unwrap_or_else(|| {
             elem.body()
-                .query_first(Selector::can::<dyn Figurable>())
+                .query_first(&Selector::can::<dyn Figurable>())
                 .map(|elem| FigureKind::Elem(elem.func()))
                 .unwrap_or_else(|| FigureKind::Elem(ImageElem::elem()))
         });
@@ -289,7 +289,7 @@ impl Synthesize for Packed<FigureElem> {
                 let descendant = match kind {
                     FigureKind::Elem(func) => elem
                         .body()
-                        .query_first(Selector::Elem(func, None))
+                        .query_first(&Selector::Elem(func, None))
                         .map(Cow::Owned),
                     FigureKind::Name(_) => None,
                 };

tests/suite/scripting/return.typ

@@ -85,3 +85,87 @@
   // Error: 16-16 expected semicolon or line break
   #return a + b Hello World
 ]
+
+--- return-discard-content ---
+// Test that discarding joined content is a warning.
+#let f() = {
+  [Hello, World!]
+  // Warning: 3-16 this return unconditionally discards the content before it
+  // Hint: 3-16 try omitting the `return` to automatically join all values
+  return "nope"
+}
+
+#test(f(), "nope")
+
+--- return-discard-content-nested ---
+#let f() = {
+  [Hello, World!]
+  {
+    // Warning: 5-18 this return unconditionally discards the content before it
+    // Hint: 5-18 try omitting the `return` to automatically join all values
+    return "nope"
+  }
+}
+
+#test(f(), "nope")
+
+--- return-discard-state ---
+// Test that discarding a joined content with state is special warning
+
+#let f() = {
+  state("hello").update("world")
+
+  // Warning: 3-19 this return unconditionally discards the content before it
+  // Hint: 3-19 try omitting the `return` to automatically join all values
+  // Hint: 3-19 state/counter updates are content that must end up in the document to have an effect
+  return [ Hello ]
+}
+
+#test(f(), [ Hello ])
+
+--- return-discard-loop ---
+// Test that return from within a control flow construct is not a warning.
+#let f1() = {
+  state("hello").update("world")
+  for x in range(3) {
+    return "nope1"
+  }
+}
+
+#let f2() = {
+  state("hello").update("world")
+  let i = 0
+  while i < 10 {
+    return "nope2"
+  }
+}
+
+#test(f1(), "nope1")
+#test(f2(), "nope2")
+
+--- return-no-discard ---
+// Test that returning the joined content is not a warning.
+#let f() = {
+  state("hello").update("world")
+  return
+}
+
+#test(f(), state("hello").update("world"))
+
+--- return-discard-not-content ---
+// Test that non-content joined value is not a warning.
+#let f() = {
+  (33,)
+  return (66,)
+}
+
+#test(f(), (66, ))
+
+--- return-discard-markup ---
+// Test that discarding markup is not a warning.
+#let f() = [
+  hello
+  #return [nope]
+]
+
+#test(f(), [nope])