From 85d3a49a1a0bd50556b8b724a15aa29b074a2db7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9bastien=20d=27Herbais=20de=20Thun?= Date: Tue, 26 Nov 2024 21:51:46 +0100 Subject: [PATCH] Added warning when explicit return in code (not markup) discards joined content (#5413) Co-authored-by: Laurenz --- crates/typst-eval/src/call.rs | 4 +- crates/typst-eval/src/code.rs | 37 ++++++-- crates/typst-eval/src/flow.rs | 33 ++++++-- .../typst-library/src/foundations/content.rs | 2 +- .../src/introspection/counter.rs | 5 ++ .../typst-library/src/introspection/state.rs | 5 ++ crates/typst-library/src/model/figure.rs | 4 +- tests/suite/scripting/return.typ | 84 +++++++++++++++++++ 8 files changed, 156 insertions(+), 18 deletions(-) diff --git a/crates/typst-eval/src/call.rs b/crates/typst-eval/src/call.rs index 9dfb7693c..f48734cbc 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 918d9d2a4..ba5256c1f 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 231e68998..b5ba487f5 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. The final boolean indicates whether the return was conditional. + Return(Span, Option, 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 { let condition = self.condition(); - if condition.eval(vm)?.cast::().at(condition.span())? { - self.if_body().eval(vm) + let output = if condition.eval(vm)?.cast::().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 { 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 a274b8bfa..69103e080 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 { + pub fn query_first(&self, selector: &Selector) -> Option { 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 2e7180c66..b884844c6 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 62aba5ffb..772a4fbfb 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 abdf2a4ee..3e2777c1a 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 { // Determine the figure's kind. let kind = elem.kind(styles).unwrap_or_else(|| { elem.body() - .query_first(Selector::can::()) + .query_first(&Selector::can::()) .map(|elem| FigureKind::Elem(elem.func())) .unwrap_or_else(|| FigureKind::Elem(ImageElem::elem())) }); @@ -289,7 +289,7 @@ impl Synthesize for Packed { 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, }; diff --git a/tests/suite/scripting/return.typ b/tests/suite/scripting/return.typ index 63e1c0b99..5cb2d15a9 100644 --- a/tests/suite/scripting/return.typ +++ b/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])