From 3d5432c318c42b164b6f748433cb316439cea7c1 Mon Sep 17 00:00:00 2001 From: Brandon Thomas Date: Sun, 4 Oct 2026 13:40:46 -0400 Subject: [PATCH] Never crash: engine without expect() (job polling, commands, menus) Removes the engine's production expect() calls: background-job polling (camera, mask, roto, point and warp tracking, render queue) uses let-else / if-let instead of take().expect("job"), command helpers bind what they checked instead of re-reading it with expect, the menu bar falls back to an empty tree (logged) instead of expect()ing that a constant parses, and autotrace/text animator group lookups no longer expect a just-inserted group. perf.rs, the benchmark harness behind `effectcraft-cli bench --ops`, opts out with a module-level allow. Test-only unreachable!() calls become panic!() (clippy.toml allows panics in tests). Denies the panic lints in the crate. Co-Authored-By: Claude Opus 5.5 --- clippy.toml | 4 ++++ crates/engine/src/camera_track.rs | 2 +- crates/engine/src/commands/autotrace.rs | 16 +++++++------ crates/engine/src/commands/expr_tools.rs | 11 +++++---- crates/engine/src/commands/layer.rs | 2 +- crates/engine/src/commands/scene_detect.rs | 2 +- crates/engine/src/commands/text_anim.rs | 26 +++++++++------------- crates/engine/src/commands/text_edit.rs | 5 +++-- crates/engine/src/lib.rs | 2 ++ crates/engine/src/mask_track.rs | 3 +-- crates/engine/src/menus.rs | 8 ++++++- crates/engine/src/perf.rs | 4 ++++ crates/engine/src/render_queue.rs | 7 +++--- crates/engine/src/roto.rs | 2 +- crates/engine/src/tests_effects.rs | 2 +- crates/engine/src/tests_proxy.rs | 4 ++-- crates/engine/src/tracking.rs | 3 +-- crates/engine/src/warp.rs | 2 +- 18 files changed, 57 insertions(+), 48 deletions(-) create mode 100644 clippy.toml diff --git a/clippy.toml b/clippy.toml new file mode 100644 index 00000000..4b6c79f5 --- /dev/null +++ b/clippy.toml @@ -0,0 +1,4 @@ +allow-unwrap-in-tests = true +allow-expect-in-tests = true +allow-panic-in-tests = true +allow-indexing-slicing-in-tests = true diff --git a/crates/engine/src/camera_track.rs b/crates/engine/src/camera_track.rs index 79a45884..cdfa5b3c 100644 --- a/crates/engine/src/camera_track.rs +++ b/crates/engine/src/camera_track.rs @@ -382,7 +382,7 @@ impl Session { if !job.is_finished() { return false; } - let mut job = self.camera_job.take().expect("job"); + let Some(mut job) = self.camera_job.take() else { return false }; if let Some(t) = job.thread.take() { let _ = t.join(); } diff --git a/crates/engine/src/commands/autotrace.rs b/crates/engine/src/commands/autotrace.rs index 093468b6..7f33fcdf 100644 --- a/crates/engine/src/commands/autotrace.rs +++ b/crates/engine/src/commands/autotrace.rs @@ -162,7 +162,7 @@ fn run(s: &mut Session, p: &Value) -> Result { let base = masks.children.len(); for k in 0..n_masks { // Depth decides the mode: outlines add, holes subtract. - let first = frames.iter().find_map(|f| f.get(k)).expect("mask"); + let Some(first) = frames.iter().find_map(|f| f.get(k)) else { continue }; let mode = if first.depth % 2 == 0 { MaskMode::Add } else { MaskMode::Subtract }; let mode = if k == 0 { MaskMode::Add } else { mode }; let mut g = build::mask( @@ -173,14 +173,16 @@ fn run(s: &mut Session, p: &Value) -> Result { build::MASK_COLORS[(base + k) % build::MASK_COLORS.len()], ); if frames.len() > 1 { - let path = g.get_mut("path").expect("path"); - for (f, lt) in frames.iter().zip(&layer_times) { - if let Some(tr) = f.get(k) { - effectcraft_keyframe::set_key(&mut path.keys, Keyframe::new(*lt, KV::Path(tr.path.clone()))); + if let Some(path) = g.get_mut("path") { + for (f, lt) in frames.iter().zip(&layer_times) { + if let Some(tr) = f.get(k) { + effectcraft_keyframe::set_key(&mut path.keys, Keyframe::new(*lt, KV::Path(tr.path.clone()))); + } } } - let op = g.get_mut("opacity").expect("opacity"); - if frames.iter().any(|f| f.get(k).is_none()) { + if let Some(op) = g.get_mut("opacity") + && frames.iter().any(|f| f.get(k).is_none()) + { for (f, lt) in frames.iter().zip(&layer_times) { let mut key = Keyframe::new(*lt, KV::Scalar(if f.get(k).is_some() { 100.0 } else { 0.0 })); key.in_interp = Interp::Hold; diff --git a/crates/engine/src/commands/expr_tools.rs b/crates/engine/src/commands/expr_tools.rs index 5648fec1..d266b94f 100644 --- a/crates/engine/src/commands/expr_tools.rs +++ b/crates/engine/src/commands/expr_tools.rs @@ -2,7 +2,7 @@ //! Language menu (the ⓕ button next to an expression), written from the public Expression //! Language Reference. -use effectcraft_project::{ItemId, Layer, Property}; +use effectcraft_project::{Expression, ItemId, Layer, Property}; use effectcraft_render::EvalCtx; use effectcraft_time::Tick; use serde::Serialize; @@ -45,14 +45,13 @@ pub fn errors(s: &Session, cid: ItemId, t: Tick) -> Vec { let ctx = EvalCtx { footage: Some(s.footage.as_ref()), expr: s.expr.as_deref(), ..EvalCtx::new(&s.project, cid, comp, t) }; let mut out = vec![]; for (i, l) in comp.layers.iter().enumerate() { - let mut props: Vec<&Property> = vec![]; + let mut props: Vec<(&Property, &Expression)> = vec![]; l.props.walk("", &mut |_, p| { - if p.expr.as_ref().is_some_and(|e| !e.text.trim().is_empty()) { - props.push(p); + if let Some(e) = p.expr.as_ref().filter(|e| !e.text.trim().is_empty()) { + props.push((p, e)); } }); - for p in props { - let e = p.expr.as_ref().expect("filtered"); + for (p, e) in props { let msg = if e.enabled { match &ctx.expr { Some(h) => h.eval(&ctx, l, p, &p.value_at(l.layer_time(t))).err(), diff --git a/crates/engine/src/commands/layer.rs b/crates/engine/src/commands/layer.rs index 09afbf73..83916405 100644 --- a/crates/engine/src/commands/layer.rs +++ b/crates/engine/src/commands/layer.rs @@ -955,8 +955,8 @@ fn set_text(s: &mut Session, p: &Value) -> Result { && let Some(doc) = super::text_edit::layer_doc(s, lid) && doc.char_len() > 0 && let Some(obj) = p.as_object() + && let Some(e) = s.state.text_edit.as_mut() { - let e = s.state.text_edit.as_mut().expect("checked"); let mut st = e.pending.clone().unwrap_or_else(|| doc.insertion_style(r.start)); let mut any = false; for (k, v) in obj { diff --git a/crates/engine/src/commands/scene_detect.rs b/crates/engine/src/commands/scene_detect.rs index 8d523e21..07519b65 100644 --- a/crates/engine/src/commands/scene_detect.rs +++ b/crates/engine/src/commands/scene_detect.rs @@ -136,7 +136,7 @@ fn apply(s: &mut Session, cid: ItemId, lid: LayerId, mode: Mode, cuts: &[Tick]) let i = cm.layers.iter().position(|l| l.id == lid).ok_or(EngineError::NoComp)?; let mut ids = vec![lid]; for t in cuts { - let cur = cm.layers.iter().position(|l| l.id == *ids.last().expect("piece")).unwrap_or(i); + let cur = ids.last().and_then(|last| cm.layers.iter().position(|l| l.id == *last)).unwrap_or(i); let l = &mut cm.layers[cur]; if !(*t > l.in_point && *t < l.out_point) { continue; diff --git a/crates/engine/src/commands/text_anim.rs b/crates/engine/src/commands/text_anim.rs index fefb8d08..1496af84 100644 --- a/crates/engine/src/commands/text_anim.rs +++ b/crates/engine/src/commands/text_anim.rs @@ -72,14 +72,11 @@ fn animator_uid(s: &Session, l: &Layer, p: &Value, cmd: &str) -> Result { /// Add the properties of `kind` to an animator's Properties group (skipping ones it has). fn add_props(ids: &mut Ids, anim: &mut PropGroup, kind: &str, three_d: bool) -> usize { let list = build::text_anim_props(ids, kind, three_d); - let props = match anim.sub_mut("properties") { - Some(p) => p, - None => { - let g = ids.group("properties", "Properties"); - anim.children.push(g.into()); - anim.sub_mut("properties").expect("just added") - } - }; + if anim.sub("properties").is_none() { + let g = ids.group("properties", "Properties"); + anim.children.push(g.into()); + } + let Some(props) = anim.sub_mut("properties") else { return 0 }; let mut n = 0; for pr in list { if props.get(&pr.match_id).is_none() { @@ -161,14 +158,11 @@ pub(crate) fn add_selector(s: &mut Session, p: &Value) -> Result { let mut ids = Ids(&mut next); let l = layer_mut(proj, cid, lid)?; let anim = l.props.find_group_mut(auid).ok_or_else(|| bad(C, "no such animator"))?; - let sels = match anim.sub_mut("selectors") { - Some(g) => g, - None => { - let g = ids.group("selectors", "Selectors"); - anim.children.insert(0, g.into()); - anim.sub_mut("selectors").expect("just added") - } - }; + if anim.sub("selectors").is_none() { + let g = ids.group("selectors", "Selectors"); + anim.children.insert(0, g.into()); + } + let sels = anim.sub_mut("selectors").ok_or_else(|| bad(C, "the animator has no Selectors group"))?; let (m, label) = match kind.as_str() { "range" => ("rangeSelector", "Range Selector"), "wiggly" => ("wigglySelector", "Wiggly Selector"), diff --git a/crates/engine/src/commands/text_edit.rs b/crates/engine/src/commands/text_edit.rs index 37ed9da0..58a93f23 100644 --- a/crates/engine/src/commands/text_edit.rs +++ b/crates/engine/src/commands/text_edit.rs @@ -455,8 +455,9 @@ pub(crate) fn sanitize(s: &mut Session) { match layer_doc(s, e.layer) { Some(d) => { let n = d.char_len(); - if e.anchor > n || e.caret > n { - let ed = s.state.text_edit.as_mut().expect("checked"); + if (e.anchor > n || e.caret > n) + && let Some(ed) = s.state.text_edit.as_mut() + { ed.anchor = ed.anchor.min(n); ed.caret = ed.caret.min(n); } diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index 26dc819a..2166280e 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -8,6 +8,8 @@ //! The project is an `Arc` edited copy-on-write; undo keeps whole-project snapshots //! (compositions are `Arc`s, so untouched comps are shared). +#![deny(clippy::unwrap_used, clippy::expect_used, clippy::panic, clippy::unimplemented, clippy::todo, clippy::unreachable)] + pub mod autosave; pub mod camera_track; pub mod commands; diff --git a/crates/engine/src/mask_track.rs b/crates/engine/src/mask_track.rs index 56bb0fa1..1ce05725 100644 --- a/crates/engine/src/mask_track.rs +++ b/crates/engine/src/mask_track.rs @@ -439,8 +439,7 @@ impl Session { } self.bump(); } - if finished { - let mut job = self.mask_job.take().expect("job"); + if finished && let Some(mut job) = self.mask_job.take() { if let Some(t) = job.thread.take() { let _ = t.join(); } diff --git a/crates/engine/src/menus.rs b/crates/engine/src/menus.rs index 2a197579..26847a6a 100644 --- a/crates/engine/src/menus.rs +++ b/crates/engine/src/menus.rs @@ -196,7 +196,13 @@ pub fn top_level() -> Vec<&'static str> { /// The menu bar for this platform. pub fn menu_bar() -> &'static [MenuNode] { static BAR: OnceLock> = OnceLock::new(); - BAR.get_or_init(|| parse(TREE, cfg!(target_os = "macos")).expect("menu tree parses")) + BAR.get_or_init(|| { + // The tree is a constant (a test parses it on every platform); never panic over it. + parse(TREE, cfg!(target_os = "macos")).unwrap_or_else(|e| { + log::error!("menu tree: {e}"); + Vec::new() + }) + }) } /// Every entry with its submenu path (`["Layer", "Blending Mode"]`), depth first. diff --git a/crates/engine/src/perf.rs b/crates/engine/src/perf.rs index f6f37b98..ab1723d9 100644 --- a/crates/engine/src/perf.rs +++ b/crates/engine/src/perf.rs @@ -3,6 +3,10 @@ //! frame). The UI-side numbers (first UI frame, timeline and Project panel draw) live in //! `effectcraft-ui-egui`'s `bench` module. See `docs/architecture.md` ▸ Performance. +// A benchmark harness (`effectcraft-cli bench --ops`, tests): a failed step should fail loudly, +// as in tests and benches (AGENTS.md, "Never crash"). Nothing here runs in the app. +#![allow(clippy::expect_used)] + use std::sync::Arc; use effectcraft_keyframe::Value as KValue; diff --git a/crates/engine/src/render_queue.rs b/crates/engine/src/render_queue.rs index c8d26937..fb871d53 100644 --- a/crates/engine/src/render_queue.rs +++ b/crates/engine/src/render_queue.rs @@ -513,8 +513,8 @@ impl Session { off.cancel(r.id); } self.drain_remote_render(); - let j = self.render_job.as_ref().expect("job"); - let r = j.remote.as_ref().expect("remote"); + let Some(j) = self.render_job.as_ref() else { return true }; + let Some(r) = j.remote.as_ref() else { return true }; let finished: Vec = lock(&j.shared.updates).iter().filter_map(|u| if let ItemUpdate::Finished { id, .. } = u { Some(*id) } else { None }).collect(); for id in &r.ids { @@ -570,8 +570,7 @@ impl Session { } self.bump(); } - if finished { - let mut job = self.render_job.take().expect("job"); + if finished && let Some(mut job) = self.render_job.take() { if let Some(t) = job.thread.take() { let _ = t.join(); } diff --git a/crates/engine/src/roto.rs b/crates/engine/src/roto.rs index ad922ed5..c5beda06 100644 --- a/crates/engine/src/roto.rs +++ b/crates/engine/src/roto.rs @@ -587,7 +587,7 @@ impl Session { if !job.is_finished() { return false; } - let mut job = self.roto_job.take().expect("job"); + let Some(mut job) = self.roto_job.take() else { return false }; if let Some(t) = job.thread.take() { let _ = t.join(); } diff --git a/crates/engine/src/tests_effects.rs b/crates/engine/src/tests_effects.rs index 0eac1011..94e6a0cc 100644 --- a/crates/engine/src/tests_effects.rs +++ b/crates/engine/src/tests_effects.rs @@ -185,7 +185,7 @@ fn opening_an_old_project_upgrades_effect_instances() { { let comp = std::sync::Arc::make_mut(match &mut old.items.get_mut(&cid).unwrap().kind { effectcraft_project::ItemKind::Comp(c) => c, - _ => unreachable!(), + _ => panic!("not a composition"), }); let l = comp.layers.iter_mut().find(|l| l.id == LayerId(a)).unwrap(); for n in &mut l.props.sub_mut("effects").unwrap().children { diff --git a/crates/engine/src/tests_proxy.rs b/crates/engine/src/tests_proxy.rs index 83ae59be..73ac2ad0 100644 --- a/crates/engine/src/tests_proxy.rs +++ b/crates/engine/src/tests_proxy.rs @@ -182,7 +182,7 @@ fn interpret_proxy_settings_apply_to_the_proxy() { assert!(px.footage.invert_alpha); let f = match &s.project.item(full).unwrap().kind { ItemKind::Footage(f) => f.clone(), - _ => unreachable!(), + _ => panic!("not footage"), }; assert_eq!(f.alpha, AlphaMode::Straight, "the main footage keeps its interpretation"); } @@ -243,7 +243,7 @@ fn guess_alpha_invert_and_linear_light() { let h = import(&mut s, "/m/half.png"); let foot = |s: &Session, i: ItemId| match &s.project.item(i).unwrap().kind { ItemKind::Footage(f) => f.clone(), - _ => unreachable!(), + _ => panic!("not footage"), }; s.execute("file.interpretFootage", json!({"items": [b.0, w.0, h.0], "alpha": "guess"})).unwrap(); assert_eq!(foot(&s, b).alpha, AlphaMode::Premultiplied); diff --git a/crates/engine/src/tracking.rs b/crates/engine/src/tracking.rs index 1a03cb4c..5ebe92d7 100644 --- a/crates/engine/src/tracking.rs +++ b/crates/engine/src/tracking.rs @@ -407,8 +407,7 @@ impl Session { } self.bump(); } - if finished { - let mut job = self.track_job.take().expect("job"); + if finished && let Some(mut job) = self.track_job.take() { if let Some(t) = job.thread.take() { let _ = t.join(); } diff --git a/crates/engine/src/warp.rs b/crates/engine/src/warp.rs index 45007bc6..761e83c2 100644 --- a/crates/engine/src/warp.rs +++ b/crates/engine/src/warp.rs @@ -375,7 +375,7 @@ impl Session { if !job.is_finished() { return false; } - let mut job = self.warp_job.take().expect("job"); + let Some(mut job) = self.warp_job.take() else { return false }; if let Some(t) = job.thread.take() { let _ = t.join(); }