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

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

7 files changed, 72 insertions, 18 deletions

diff --git a/crates/typst-eval/src/call.rs b/crates/typst-eval/src/call.rs
index 9dfb7693..f48734cb 100644
--- a/crates/typst-eval/src/call.rs
+++ b/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 => {}
     }
diff --git a/crates/typst-eval/src/code.rs b/crates/typst-eval/src/code.rs
index 918d9d2a..ba5256c1 100644
--- a/crates/typst-eval/src/code.rs
+++ b/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);
+}
diff --git a/crates/typst-eval/src/flow.rs b/crates/typst-eval/src/flow.rs
index 231e6899..b5ba487f 100644
--- a/crates/typst-eval/src/flow.rs
+++ b/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)
     }
diff --git a/crates/typst-library/src/foundations/content.rs b/crates/typst-library/src/foundations/content.rs
index a274b8bf..69103e08 100644
--- a/crates/typst-library/src/foundations/content.rs
+++ b/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) {
diff --git a/crates/typst-library/src/introspection/counter.rs b/crates/typst-library/src/introspection/counter.rs
index 2e7180c6..b884844c 100644
--- a/crates/typst-library/src/introspection/counter.rs
+++ b/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]
diff --git a/crates/typst-library/src/introspection/state.rs b/crates/typst-library/src/introspection/state.rs
index 62aba5ff..772a4fbf 100644
--- a/crates/typst-library/src/introspection/state.rs
+++ b/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]
diff --git a/crates/typst-library/src/model/figure.rs b/crates/typst-library/src/model/figure.rs
index abdf2a4e..3e2777c1 100644
--- a/crates/typst-library/src/model/figure.rs
+++ b/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,
                 };