From 1d9802c8d61ed2c0e0e199356bca98a30a387f73 Mon Sep 17 00:00:00 2001 From: Teakowa Date: Sat, 26 Sep 2026 22:11:29 +0800 Subject: [PATCH 1/2] fix(analysis): count elements the way the client-checked OverPy counter does The element count was 37% below the client on a production project. Number literals, constants that wrap a literal, variable reads, comparison operators, omitted defaulted arguments, and the Else If, Else and End actions were undercounted, and disabled rules, actions and conditions made the whole program uncountable. Derive the missing costs from OverPy's per-action #!debugElementCount on 2867 actions of that project, count disabled actions and conditions like enabled ones, count hero pairs per direct argument, and keep the closing End of loops and subroutine rules while an If left open at the end of any other rule is not counted. Every action and rule count now equals OverPy's, and the total is 30067, OverPy's own figure. Document the model and its evidence in element-count.md and reproduce one rule per case in the tests. Fixes #305 --- .../workshop-rs/src/analysis/element_count.rs | 217 ++++++++++---- crates/workshop-rs/tests/element_count.rs | 276 +++++++++++++++++- .../0015-contextual-literals-are-preserved.md | 5 + docs/element-count.md | 79 +++-- 4 files changed, 481 insertions(+), 96 deletions(-) diff --git a/crates/workshop-rs/src/analysis/element_count.rs b/crates/workshop-rs/src/analysis/element_count.rs index 0e8ddd9..9da0151 100644 --- a/crates/workshop-rs/src/analysis/element_count.rs +++ b/crates/workshop-rs/src/analysis/element_count.rs @@ -1,12 +1,11 @@ //! Canonical Workshop element-count analysis. //! //! The calculator operates on the canonical public program, not source-language syntax or -//! emitted text. Its rules are the documented Workshop.codes model: rules, -//! actions, conditions, and ordinary values cost one element; arrays and -//! evaluate-once values cost two; localized strings cost two; direct action or -//! condition arguments are reduced by one; and every pair of hero literals in -//! those arguments adds one. Custom game settings and rule parameters cost -//! zero. +//! emitted text. Every occurrence of a component costs its base amount at any +//! nesting depth, direct action and condition arguments cost one less, and +//! every pair of hero literals in one direct argument adds one. The base costs +//! and the block-closing rules are documented, with their evidence, in +//! `docs/element-count.md`. use std::collections::HashMap; use std::fmt; @@ -133,6 +132,7 @@ impl Program { values: HashMap::new(), actions: HashMap::new(), next_node_id: 0, + name_slot: false, }; let mut rules = Vec::with_capacity(self.rules.len()); for rule in self.rules.iter() { @@ -199,6 +199,8 @@ struct Counter<'a> { values: HashMap, actions: HashMap, next_node_id: usize, + /// Set while counting an argument that names a variable rather than reads it. + name_slot: bool, } impl Counter<'_> { @@ -212,23 +214,13 @@ impl Counter<'_> { let node_id = self.next_node_id(); let mut children = Vec::with_capacity(rule.conditions.len() + rule.actions.len()); for condition in &rule.conditions { - if condition.disabled { - return Err(ElementCountError::Unsupported { - kind: ElementNodeKind::Condition, - name: "disabled condition".to_string(), - span: self - .program - .values - .get(condition.value) - .and_then(|v| v.span), - reason: "the element cost of a disabled condition is not established" - .to_string(), - }); - } children.push(self.condition(condition.value)?.node); } - for action in &rule.actions { - children.push(self.action(*action)?.node); + // A subroutine keeps its closing `End`; any other rule closes an open `If`. + let closes_open_blocks = !matches!(rule.event, wir::Event::Subroutine(_)); + for (index, action) in rule.actions.iter().enumerate() { + let trailing = closes_open_blocks && index + 1 == rule.actions.len(); + children.push(self.action(*action, trailing)?.node); } Ok(Counted::finish( ElementNodeKind::Rule, @@ -256,7 +248,7 @@ impl Counter<'_> { let mut heroes = 0; for argument in args { let counted = self.value(*argument, true)?; - heroes += counted.heroes; + heroes += counted.heroes / 2 * 2; children.push(counted.node); } (children, heroes) @@ -278,7 +270,7 @@ impl Counter<'_> { )) } - fn action(&mut self, id: ActionId) -> Result { + fn action(&mut self, id: ActionId, trailing: bool) -> Result { let node_id = self.next_node_id(); if let Some(&active_id) = self.actions.get(&id.index()) { return Err(ElementCountError::Cycle { @@ -287,12 +279,16 @@ impl Counter<'_> { }); } self.actions.insert(id.index(), node_id); - let Some(action) = self.program.actions.get(id) else { + let mut action = self.program.actions.get(id); + while let Some(Action::Disabled { action: inner, .. }) = action { + action = self.program.actions.get(*inner); + } + let Some(action) = action else { return Err(ElementCountError::InvalidProgram { message: format!("dangling action {}", id.index()), }); }; - let result = self.action_inner(action, node_id); + let result = self.action_inner(action, node_id, trailing); self.actions.remove(&id.index()); result } @@ -301,10 +297,12 @@ impl Counter<'_> { &mut self, action: &Action, node_id: usize, + trailing: bool, ) -> Result { let span = action.span(); let mut children = Vec::new(); let mut heroes = 0; + let mut base = 1; let name; match action { Action::SetGlobalVariable { value, .. } @@ -332,15 +330,24 @@ impl Counter<'_> { .. } => { name = "if"; - for branch in branches { + // `Else If` and `Else` are actions of their own; so is `End`, + // except that an `If` still open where the rule ends is closed + // by the rule. Loops keep their `End`. + base += branches.len().saturating_sub(1) + + usize::from(else_body.is_some()) + + usize::from(!trailing); + for (branch_index, branch) in branches.iter().enumerate() { self.push_action_value(&mut children, &mut heroes, branch.condition)?; - for nested in &branch.body { - children.push(self.action(*nested)?.node); + let last_body = else_body.is_none() && branch_index + 1 == branches.len(); + for (index, nested) in branch.body.iter().enumerate() { + let end = trailing && last_body && index + 1 == branch.body.len(); + children.push(self.action(*nested, end)?.node); } } if let Some(body) = else_body { - for nested in body { - children.push(self.action(*nested)?.node); + for (index, nested) in body.iter().enumerate() { + let end = trailing && index + 1 == body.len(); + children.push(self.action(*nested, end)?.node); } } } @@ -348,9 +355,10 @@ impl Counter<'_> { condition, body, .. } => { name = "while"; + base += 1; self.push_action_value(&mut children, &mut heroes, *condition)?; for nested in body { - children.push(self.action(*nested)?.node); + children.push(self.action(*nested, false)?.node); } } Action::ForGlobalVariable { @@ -361,11 +369,12 @@ impl Counter<'_> { .. } => { name = "for global variable"; + base += 1; for value in [start, stop, step] { self.push_action_value(&mut children, &mut heroes, *value)?; } for nested in body { - children.push(self.action(*nested)?.node); + children.push(self.action(*nested, false)?.node); } } Action::ForPlayerVariable { @@ -377,19 +386,17 @@ impl Counter<'_> { .. } => { name = "for player variable"; + base += 1; for value in [player, start, stop, step] { self.push_action_value(&mut children, &mut heroes, *value)?; } for nested in body { - children.push(self.action(*nested)?.node); + children.push(self.action(*nested, false)?.node); } } Action::Disabled { .. } => { - return Err(ElementCountError::Unsupported { - kind: ElementNodeKind::Action, - name: "disabled action".to_string(), - span, - reason: "the element cost of a disabled action is not established".to_string(), + return Err(ElementCountError::InvalidProgram { + message: "a disabled action wraps no enabled action".to_string(), }); } Action::Call { @@ -406,9 +413,22 @@ impl Counter<'_> { }); } name = action_name.as_str(); - for argument in args { + for (index, argument) in args.iter().enumerate() { + self.name_slot = variable_slot(self.catalog, Kind::Action, action_name, index) + || (index == 0 && takes_variable(self.catalog, Kind::Action, action_name)); self.push_action_value(&mut children, &mut heroes, *argument)?; } + // An omitted argument is still a filled slot; its default is a + // direct argument, so it costs its own value minus one. + base = (base as isize + + omitted_default_cost( + self.catalog, + Kind::Action, + action_name, + args.len(), + true, + )) + .max(0) as usize; } } Ok(Counted::finish( @@ -416,7 +436,7 @@ impl Counter<'_> { node_id, name, span, - 1, + base, pair_surcharge(heroes), children, heroes, @@ -430,12 +450,14 @@ impl Counter<'_> { id: ValueId, ) -> Result<(), ElementCountError> { let counted = self.value(id, true)?; - *heroes += counted.heroes; + // Pairs are counted within each direct argument, not across them. + *heroes += counted.heroes / 2 * 2; children.push(counted.node); Ok(()) } fn value(&mut self, id: ValueId, top_level: bool) -> Result { + let name_slot = std::mem::take(&mut self.name_slot); let node_id = self.next_node_id(); if let Some(&active_id) = self.values.get(&id.index()) { return Err(ElementCountError::Cycle { @@ -451,26 +473,32 @@ impl Counter<'_> { }; let span = value.span; let result = match &value.value { - Value::Number { .. } => self.value_node(node_id, "number", span, 1, vec![], 0), + Value::Number { .. } => self.value_node(node_id, "number", span, 2, vec![], 0), Value::String(_) => self.value_node(node_id, "string", span, 1, vec![], 0), Value::LocalizedString(_) => { self.value_node(node_id, "localized string", span, 2, vec![], 0) } Value::Bool(_) => self.value_node(node_id, "boolean", span, 1, vec![], 0), Value::Null => self.value_node(node_id, "null", span, 1, vec![], 0), - Value::Array(elements) => self.value_children(node_id, "array", span, 2, elements), + Value::Array(elements) => { + self.value_children(node_id, "array", span, 2, elements, None) + } Value::Vector { x, y, z } => { - self.value_children(node_id, "vector", span, 1, &[*x, *y, *z]) + self.value_children(node_id, "vector", span, 1, &[*x, *y, *z], None) } Value::Enum { value_type, .. } => { let heroes = usize::from(value_type == "Hero"); - self.value_node(node_id, value_type, span, 1, vec![], heroes) + let base = literal_enum_cost(value_type); + self.value_node(node_id, value_type, span, base, vec![], heroes) } Value::GlobalVariable(_) => { - self.value_node(node_id, "global variable", span, 1, vec![], 0) + let base = if name_slot { 1 } else { 2 }; + self.value_node(node_id, "global variable", span, base, vec![], 0) } Value::PlayerVariable { player, .. } => { - self.value_children(node_id, "player variable", span, 1, &[*player]) + // A named player variable is two direct arguments (player, name). + let base = if name_slot { 0 } else { 2 }; + self.value_children(node_id, "player variable", span, base, &[*player], None) } Value::Subroutine(_) => self.value_node(node_id, "subroutine", span, 1, vec![], 0), Value::EventPlayer => self.value_node(node_id, "event player", span, 1, vec![], 0), @@ -501,16 +529,28 @@ impl Counter<'_> { } else { args.clone() }; - let base = if name == "array" - || name == "evalOnce" - || name.starts_with("workshopSetting") - || name.starts_with("createWorkshopSetting") - { + let base = if is_comparison(name) { + // The operator is a literal of its own. + 2 + } else if name == "array" || name == "evaluateOnce" { 2 } else { 1 }; - self.value_children(node_id, name, span, base, &child_ids) + let omitted = + omitted_default_cost(self.catalog, Kind::Value, name, args.len(), false); + let mut counted = self.value_children( + node_id, + name, + span, + (base as isize + omitted) as usize, + &child_ids, + Some((Kind::Value, name)), + )?; + let setting = workshop_setting_adjustment(name); + counted.node.adjustment += setting; + counted.node.count = (counted.node.count as isize + setting).max(0) as usize; + Ok(counted) } } }?; @@ -551,10 +591,13 @@ impl Counter<'_> { span: Option, base: usize, ids: &[ValueId], + owner: Option<(Kind, &str)>, ) -> Result { let mut children = Vec::with_capacity(ids.len()); let mut heroes = 0; - for child in ids { + for (index, child) in ids.iter().enumerate() { + self.name_slot = + owner.is_some_and(|(kind, name)| variable_slot(self.catalog, kind, name, index)); let counted = self.value(*child, false)?; heroes += counted.heroes; children.push(counted.node); @@ -594,3 +637,69 @@ fn is_canonical_helper(name: &str) -> bool { | "removeFromArrayByIndex" ) } + +/// The cost of the defaults filled in for arguments a call leaves out. +fn omitted_default_cost( + catalog: &Catalog, + kind: Kind, + name: &str, + given: usize, + direct: bool, +) -> isize { + let Some(entry) = catalog.entry(kind, name) else { + return 0; + }; + (given..entry.param_count()) + .filter_map(|index| entry.param_default(index)) + .map(|default| { + let cost = if default.parse::().is_ok() { + 2 + } else { + default.split('.').next().map_or(1, literal_enum_cost) as isize + }; + cost - isize::from(direct) + }) + .sum() +} + +/// Enum constants the client spells as a value wrapping a literal (`Team(Team 1)`, +/// `Hero(Ana)`, `Color(White)`, `Button(Reload)`, `Map(...)`) cost both nodes. +fn literal_enum_cost(value_type: &str) -> usize { + if matches!(value_type, "Team" | "Hero" | "Color" | "Button" | "Map") { + 2 + } else { + 1 + } +} + +/// Whether argument `index` of a call names a variable instead of reading one. +fn variable_slot(catalog: &Catalog, kind: Kind, name: &str, index: usize) -> bool { + catalog.entry(kind, name).is_some_and(|entry| { + entry.param_type(index) == Some("Variable") + || entry + .params() + .get(index) + .is_some_and(|param| param == "Variable") + }) +} + +/// Whether a call has a parameter that names a variable; the player and the +/// variable of a player variable are then one argument here. +fn takes_variable(catalog: &Catalog, kind: Kind, name: &str) -> bool { + catalog.entry(kind, name).is_some_and(|entry| { + (0..entry.param_count()).any(|index| variable_slot(catalog, kind, name, index)) + }) +} + +/// The fixed adjustment the client applies to a Workshop setting value, whose +/// category, name and bounds are literals that are not separate elements. +fn workshop_setting_adjustment(name: &str) -> isize { + match name { + "workshopSettingInteger" + | "workshopSettingFloat" + | "createWorkshopSettingInt" + | "createWorkshopSettingFloat" => -3, + "workshopSettingCombo" | "createWorkshopSettingEnum" => -2, + _ => 0, + } +} diff --git a/crates/workshop-rs/tests/element_count.rs b/crates/workshop-rs/tests/element_count.rs index 671cae6..7db5e30 100644 --- a/crates/workshop-rs/tests/element_count.rs +++ b/crates/workshop-rs/tests/element_count.rs @@ -5,7 +5,7 @@ use workshop_rs::catalog::{Catalog, Locale}; use workshop_rs::convert::{self, ConvertOptions}; use workshop_rs::parser; use workshop_rs::settings::{Settings, SettingsNode}; -use workshop_rs::{Action, Condition, Event, Program, Rule, Value, Variable}; +use workshop_rs::{Action, Event, Program, Rule, Value, Variable}; fn catalog() -> Catalog { Catalog::builtin().unwrap() @@ -64,8 +64,8 @@ fn arrays_localized_strings_and_hero_pairs_are_visible_in_the_tree() { let array_program = program_with_value(array); let array_report = array_program.element_count(&catalog()).unwrap(); assert_eq!( - array_report.total, 5, - "rule + action + (array 2 + literals 2 - top-level 1)" + array_report.total, 7, + "rule + action + (array 2 - top-level 1) + two numbers at 2 each" ); let hero_program = program_with_value(Value::Array(vec![ @@ -79,7 +79,10 @@ fn arrays_localized_strings_and_hero_pairs_are_visible_in_the_tree() { }, ])); let hero_report = hero_program.element_count(&catalog()).unwrap(); - assert_eq!(hero_report.total, 6, "hero pair surcharge adds one element"); + assert_eq!( + hero_report.total, 8, + "rule + action + array 1 + two hero constants at 2 each + the pair surcharge" + ); assert_eq!(hero_report.rules[0].children[0].adjustment, 1); let localized_report = program_with_value(Value::LocalizedString("hello".to_string())) @@ -219,18 +222,263 @@ fn public_api_rejects_unsupported_and_invalid_programs_explicitly() { unknown_error, ElementCountError::InvalidProgram { message } if message.contains("unknown action") )); +} - let mut invalid = Program::new(); - invalid.rule( - Rule::new("invalid", Event::Global).condition(Condition::disabled(Value::Bool(true))), +#[test] +fn disabled_conditions_and_actions_cost_what_enabled_ones_do() { + let catalog = catalog(); + let locale = Locale::new("en-US"); + let enabled = parser::parse( + r#"rule ("r") { event { Ongoing - Global; } conditions { Is Game In Progress; } actions { Wait(1, Ignore Condition); } }"#, + &catalog, + &locale, + ) + .unwrap(); + let disabled = parser::parse( + r#"rule ("r") { event { Ongoing - Global; } conditions { disabled Is Game In Progress; } actions { disabled Wait(1, Ignore Condition); } }"#, + &catalog, + &locale, + ) + .unwrap(); + assert_eq!( + disabled.element_count(&catalog).unwrap().total, + enabled.element_count(&catalog).unwrap().total ); - let invalid_error = invalid - .element_count(&catalog()) - .expect_err("unsupported condition must not produce an exact count"); - assert!(matches!( - invalid_error, - ElementCountError::Unsupported { name, .. } if name == "disabled condition" - )); +} + +/// Workshop text compiled by pinned OverPy 9.7.10 with `#!debugElementCount`, and +/// the total that compiler reports. On a production project OverPy's total is +/// within 0.01% of the client's; each program here isolates one rule of the model. +const OVERPY_COUNTS: &[(&str, usize, &str)] = &[ + ( + "wait_number", + 5, + r#"rule ("r") { + event { + Ongoing - Each Player; + All; + All; + } + actions { + Wait(4, Ignore Condition); + Set Move Speed(Event Player, 50); + } +}"#, + ), + ( + "custom_string_defaults", + 6, + r#"rule ("r") { + event { + Ongoing - Each Player; + All; + All; + } + actions { + Small Message(Event Player, Custom String("hello")); + } +}"#, + ), + ( + "variable_reads", + 6, + r#"variables { + global: + 0: g + player: + 0: p +} + +rule ("r") { + event { + Ongoing - Each Player; + All; + All; + } + actions { + Set Global Variable(g, (Event Player).p); + Set Player Variable(Event Player, p, Global.g); + } +}"#, + ), + ( + "compare_in_action", + 10, + r#"variables { + global: + 0: g +} + +rule ("r") { + event { + Ongoing - Each Player; + All; + All; + } + actions { + Wait Until(Compare(Global.g, ==, 3), 5); + Set Global Variable(g, 1); + } +}"#, + ), + ( + "if_else_end", + 19, + r#"variables { + global: + 0: g +} + +rule ("r") { + event { + Ongoing - Each Player; + All; + All; + } + actions { + Wait(1, Ignore Condition); + If(Compare(Global.g, ==, 3)); + Set Global Variable(g, 1); + Else; + Set Global Variable(g, 2); + End; + Wait(2, Ignore Condition); + Set Global Variable(g, 7); + } +}"#, + ), + ( + "trailing_if_omits_end", + 11, + r#"variables { + global: + 0: g +} + +rule ("r") { + event { + Ongoing - Each Player; + All; + All; + } + actions { + Wait(1, Ignore Condition); + If(Compare(Global.g, ==, 3)); + Set Global Variable(g, 1); + } +}"#, + ), + ( + "subroutine_keeps_end", + 14, + r#"variables { + global: + 0: g +} + +subroutines { + 0: s +} + +rule ("Subroutine s") { + event { + Subroutine; + s; + } + actions { + Wait(1, Ignore Condition); + If(Compare(Global.g, ==, 3)); + Set Global Variable(g, 1); + End; + } +} + +rule ("r") { + event { + Ongoing - Each Player; + All; + All; + } + actions { + Call Subroutine(s); + } +}"#, + ), + ( + "while_keeps_end", + 10, + r#"variables { + global: + 0: g +} + +rule ("r") { + event { + Ongoing - Each Player; + All; + All; + } + actions { + While(Compare(Global.g, <, 3)); + Modify Global Variable(g, Add, 1); + End; + } +}"#, + ), + ( + "hero_pairs_per_argument", + 19, + r#"variables { + global: + 0: g +} + +rule ("r") { + event { + Ongoing - Each Player; + All; + All; + } + actions { + Set Global Variable(g, Array(Hero(Ana), Hero(Mercy))); + Set Player Allowed Heroes(Event Player, Array(Hero(Ana), Hero(Mercy))); + Set Player Allowed Heroes(Event Player, Hero(Ana)); + Set Player Allowed Heroes(Event Player, Hero(Mercy)); + } +}"#, + ), + ( + "team_and_color_constants", + 9, + r#"variables { + global: + 0: g +} + +rule ("r") { + event { + Ongoing - Each Player; + All; + All; + } + actions { + Set Global Variable(g, Color(Team 1)); + Set Global Variable(g, Team 2); + Set Global Variable(g, Button(Reload)); + Set Global Variable(g, Map(Ilios)); + } +}"#, + ), +]; + +#[test] +fn counts_match_the_pinned_overpy_element_counter() { + let catalog = catalog(); + for (name, expected, text) in OVERPY_COUNTS { + let program = parser::parse(text, &catalog, &Locale::new("en-US")).unwrap(); + let report = program.element_count(&catalog).unwrap(); + assert_eq!(report.total, *expected, "{name}"); + } } fn assert_unique_node_ids(node: &workshop_rs::actions::ElementCountNode, ids: &mut HashSet) { diff --git a/docs/adr/0015-contextual-literals-are-preserved.md b/docs/adr/0015-contextual-literals-are-preserved.md index e9d6e05..d5b2622 100644 --- a/docs/adr/0015-contextual-literals-are-preserved.md +++ b/docs/adr/0015-contextual-literals-are-preserved.md @@ -59,6 +59,11 @@ in another locale lost it. [ADR-0005](0005-seasonal-client-validation.md) and does not change this decision. + Update: OverPy's per-action element counts agree with the client's total on + a production project to 0.01%, and the model now charges the difference + (see [element-count.md](../element-count.md)); a numeric literal costs one + element more than `False`, `True`, or `Null` in the same position. + ## Alternatives considered - **Keep normalization and document that round trips are lossy.** Rejected: diff --git a/docs/element-count.md b/docs/element-count.md index 0c6498c..5471e72 100644 --- a/docs/element-count.md +++ b/docs/element-count.md @@ -16,24 +16,39 @@ aggregate total. Consumers should map nodes by their ordered tree position and source span when one is available; report-local IDs have no meaning across reports. -The model follows the documented Workshop element-count rules: +The model counts every occurrence of every component, at any nesting depth; +nothing is deduplicated. The base costs are the Workshop.codes reference +calibrated against the client-checked counts of pinned OverPy (see Evidence): -| Program component | Base cost | +| Program component | Cost | | --- | ---: | -| Rule | 1 | -| Action | 1 | -| Condition | 1 | -| Ordinary value or literal | 1 | -| Array | 2 | -| Workshop setting value (`Workshop Setting ...`) | 2 | -| Evaluate Once | 2 | -| Localized/preset string | 2 | +| Rule, action, condition | 1 each | +| Value | 1 | +| Number literal (the value and its literal) | 2 | +| `False`, `True`, `Null` | 1 | +| Constant written as a value wrapping a literal: `Hero`, `Team`, `Color`, `Button`, `Map` | 2 | +| Any other enum choice (a wait behavior, a reevaluation mode, ...) | 1 | +| Comparison value outside a rule condition (its operator is a literal) | 2 | +| Array, Evaluate Once, localized/preset string | 2 | +| String literal | 1 | +| Read of a global or player variable (the value and the variable name) | 2 | +| Workshop setting value | 1, then -3 for integer and float, -2 for combo | +| `Else If`, `Else`, `End` | 1 each, as actions of their own | -Rule event parameters, action syntax parameters such as a variable name or -modify operator, comments, and custom game settings do not contribute. A -direct action or condition argument is reduced by one. For each pair of hero -literals anywhere below the direct arguments of one action or condition, one -element is added. Disabling a rule, action, or condition has no effect. +Rule event parameters, action syntax parameters, comments, and custom game +settings do not contribute. A variable named as an argument (`Set Global +Variable At Index`, the chase actions) counts as that argument only; the player +and the name of a named player variable are two direct arguments. A direct +action or condition argument costs one less. For each pair of hero literals +anywhere below one direct argument, one element is added; pairs are counted per +argument, not across arguments. An argument a call leaves out still fills its +slot: the catalog default counts at its cost (the three replacement slots of +`Custom String` each count a `Null`). + +`End` closes a block, with two exceptions. An `If` still open where a rule ends +is closed by the rule and its `End` is neither written nor counted; a `While`, +a `For`, and any block in a subroutine keep theirs. Disabling a rule, action, +or condition has no effect on the count. The calculator is locale-independent: it reads canonical identities and never emitted spellings. It validates the public program and catalog identities before @@ -49,19 +64,27 @@ estimate of runtime CPU cost or execution performance. The independent behavioral source for the supported rules is the [Workshop.codes element-count calculation reference](https://workshop.codes/wiki/articles/element-count-calculation). -Known evidence gap: the model charges a numeric literal and `False`, `True`, or -`Null` the same cost, as the Workshop.codes reference groups them under one -literal cost. OverPy's `#!debugElementCount` charges a numeric literal argument -one element and `False`, `True`, or `Null` none, and its `#!optimizeForSize` -substitutions rely on that difference. No client capture establishes either -rule, so counts for programs that use these substitutions are unverified. -Parsing keeps the authored literal -([ADR-0015](adr/0015-contextual-literals-are-preserved.md)), so the analysis -can apply the rule a capture establishes. +## Evidence + +The rules above were derived and checked against `#!debugElementCount` of pinned +OverPy 9.7.10, which reports an element count for every action and condition. +On the `main` entry of a production project (309 rules, 2867 leaf actions) +every action count and every rule count agrees, and the total is 30067, OverPy's +own figure. The client counted OverPy's build of that project at 30070, three +elements more; the cause of those three is not known. Small programs that +isolate one rule each, with OverPy's totals, are in +`tests/element_count.rs`. + +The model is calibrated on one project. Constructs it does not exercise follow +the Workshop.codes reference, and the client has not been captured per +construct. + +Numeric literals cost more than `False`, `True`, and `Null`, which is what +OverPy's `#!optimizeForSize` substitutions exploit +([ADR-0015](adr/0015-contextual-literals-are-preserved.md)). Parsing keeps the +authored literal, so the analysis sees the difference. -This initial API does not claim live-client/editor -validation or source-language debug-count compatibility. Those belong to later -client-backed/consumer integration work after the canonical Program surface is -stable. The current real-project `rework.ow` fixture still stops in the parser +The analysis does not claim live-client/editor validation beyond the evidence +above. The current real-project `rework.ow` fixture still stops in the parser on an ambiguous bare `None` enum spelling, so it is not counted as a passing real-program result until that independent parser gap is resolved. From b32e90a5e3ddf15eb62f7027db8303caae4c9aa1 Mon Sep 17 00:00:00 2001 From: Teakowa Date: Sun, 27 Sep 2026 00:24:05 +0800 Subject: [PATCH 2/2] fix(analysis): count the last action's End the way the canonical emitter writes it The count omitted the closing End of any final If in a non-subroutine rule and kept it in subroutine rules and, through nested last Ifs, elsewhere. The countable program does not record whether a trailing End was written, so a written and an omitted End are one program. Follow the canonical emitter: only the last action of a rule, when it is an If, is closed without End, in every rule. Nested blocks, While and For keep theirs. Add tests that a written and an omitted trailing End cost the same, that only the last action is closed without End, and that the count of a program equals the count of its emitted text. State the eight rules where the count differs from OverPy on the calibration project. --- .../workshop-rs/src/analysis/element_count.rs | 33 +++---- crates/workshop-rs/tests/element_count.rs | 94 +++++++++++-------- docs/element-count.md | 15 ++- 3 files changed, 81 insertions(+), 61 deletions(-) diff --git a/crates/workshop-rs/src/analysis/element_count.rs b/crates/workshop-rs/src/analysis/element_count.rs index 9da0151..53cbc25 100644 --- a/crates/workshop-rs/src/analysis/element_count.rs +++ b/crates/workshop-rs/src/analysis/element_count.rs @@ -216,11 +216,9 @@ impl Counter<'_> { for condition in &rule.conditions { children.push(self.condition(condition.value)?.node); } - // A subroutine keeps its closing `End`; any other rule closes an open `If`. - let closes_open_blocks = !matches!(rule.event, wir::Event::Subroutine(_)); for (index, action) in rule.actions.iter().enumerate() { - let trailing = closes_open_blocks && index + 1 == rule.actions.len(); - children.push(self.action(*action, trailing)?.node); + let rule_final = index + 1 == rule.actions.len(); + children.push(self.action(*action, rule_final)?.node); } Ok(Counted::finish( ElementNodeKind::Rule, @@ -270,7 +268,7 @@ impl Counter<'_> { )) } - fn action(&mut self, id: ActionId, trailing: bool) -> Result { + fn action(&mut self, id: ActionId, rule_final: bool) -> Result { let node_id = self.next_node_id(); if let Some(&active_id) = self.actions.get(&id.index()) { return Err(ElementCountError::Cycle { @@ -288,7 +286,7 @@ impl Counter<'_> { message: format!("dangling action {}", id.index()), }); }; - let result = self.action_inner(action, node_id, trailing); + let result = self.action_inner(action, node_id, rule_final); self.actions.remove(&id.index()); result } @@ -297,7 +295,7 @@ impl Counter<'_> { &mut self, action: &Action, node_id: usize, - trailing: bool, + rule_final: bool, ) -> Result { let span = action.span(); let mut children = Vec::new(); @@ -330,24 +328,21 @@ impl Counter<'_> { .. } => { name = "if"; - // `Else If` and `Else` are actions of their own; so is `End`, - // except that an `If` still open where the rule ends is closed - // by the rule. Loops keep their `End`. + // `Else If` and `Else` are actions of their own; so is `End`. The + // canonical emitter closes the last action of a rule without its + // `End`, and only that one: nested and loop blocks keep theirs. base += branches.len().saturating_sub(1) + usize::from(else_body.is_some()) - + usize::from(!trailing); - for (branch_index, branch) in branches.iter().enumerate() { + + usize::from(!rule_final); + for branch in branches { self.push_action_value(&mut children, &mut heroes, branch.condition)?; - let last_body = else_body.is_none() && branch_index + 1 == branches.len(); - for (index, nested) in branch.body.iter().enumerate() { - let end = trailing && last_body && index + 1 == branch.body.len(); - children.push(self.action(*nested, end)?.node); + for nested in &branch.body { + children.push(self.action(*nested, false)?.node); } } if let Some(body) = else_body { - for (index, nested) in body.iter().enumerate() { - let end = trailing && index + 1 == body.len(); - children.push(self.action(*nested, end)?.node); + for nested in body { + children.push(self.action(*nested, false)?.node); } } } diff --git a/crates/workshop-rs/tests/element_count.rs b/crates/workshop-rs/tests/element_count.rs index 7db5e30..b84ee52 100644 --- a/crates/workshop-rs/tests/element_count.rs +++ b/crates/workshop-rs/tests/element_count.rs @@ -248,7 +248,9 @@ fn disabled_conditions_and_actions_cost_what_enabled_ones_do() { /// Workshop text compiled by pinned OverPy 9.7.10 with `#!debugElementCount`, and /// the total that compiler reports. On a production project OverPy's total is -/// within 0.01% of the client's; each program here isolates one rule of the model. +/// within 0.01% of the client's; each program here isolates one rule of the model. OverPy also closes the last `If` +/// of a subroutine rule with an `End`, which the canonical emitter omits, so no case +/// with such a rule is listed. const OVERPY_COUNTS: &[(&str, usize, &str)] = &[ ( "wait_number", @@ -366,42 +368,6 @@ rule ("r") { If(Compare(Global.g, ==, 3)); Set Global Variable(g, 1); } -}"#, - ), - ( - "subroutine_keeps_end", - 14, - r#"variables { - global: - 0: g -} - -subroutines { - 0: s -} - -rule ("Subroutine s") { - event { - Subroutine; - s; - } - actions { - Wait(1, Ignore Condition); - If(Compare(Global.g, ==, 3)); - Set Global Variable(g, 1); - End; - } -} - -rule ("r") { - event { - Ongoing - Each Player; - All; - All; - } - actions { - Call Subroutine(s); - } }"#, ), ( @@ -471,6 +437,60 @@ rule ("r") { ), ]; +#[test] +fn a_written_and_an_omitted_trailing_end_are_one_program_with_one_cost() { + let catalog = catalog(); + let locale = Locale::new("en-US"); + let rule = |end: &str| { + format!( + r#"rule ("r") {{ event {{ Ongoing - Global; }} actions {{ If(True); Wait(1, Ignore Condition); {end} }} }}"# + ) + }; + let omitted = parser::parse(&rule(""), &catalog, &locale).unwrap(); + let written = parser::parse(&rule("End;"), &catalog, &locale).unwrap(); + let omitted = omitted.element_count(&catalog).unwrap().total; + assert_eq!(omitted, written.element_count(&catalog).unwrap().total); + // rule 1 + If 1 + its condition 0 + Wait (1 + number 1 + behavior 0); no `End` + assert_eq!(omitted, 4); +} + +#[test] +fn only_the_last_action_of_a_rule_is_closed_without_end() { + let catalog = catalog(); + let locale = Locale::new("en-US"); + let text = |body: &str| { + format!(r#"rule ("r") {{ event {{ Ongoing - Global; }} actions {{ {body} }} }}"#) + }; + let count = |body: &str| { + parser::parse(&text(body), &catalog, &locale) + .unwrap() + .element_count(&catalog) + .unwrap() + .total + }; + // the middle `If` and a nested last `If` keep their `End` + assert_eq!(count("If(True); End; Wait(1, Ignore Condition);"), 5); + assert_eq!(count("If(True); If(True); End; End;"), 4); + // a loop keeps its `End` even as the last action + assert_eq!(count("While(True); End;"), 3); +} + +#[test] +fn the_count_is_that_of_the_canonical_emitted_form() { + let catalog = catalog(); + let locale = Locale::new("en-US"); + for (name, _, text) in OVERPY_COUNTS { + let program = parser::parse(text, &catalog, &locale).unwrap(); + let emitted = workshop_rs::emitter::emit(&program, &catalog, &locale).unwrap(); + let reparsed = parser::parse(&emitted, &catalog, &locale).unwrap(); + assert_eq!( + program.element_count(&catalog).unwrap().total, + reparsed.element_count(&catalog).unwrap().total, + "{name}" + ); + } +} + #[test] fn counts_match_the_pinned_overpy_element_counter() { let catalog = catalog(); diff --git a/docs/element-count.md b/docs/element-count.md index 5471e72..a4c67f4 100644 --- a/docs/element-count.md +++ b/docs/element-count.md @@ -45,9 +45,11 @@ argument, not across arguments. An argument a call leaves out still fills its slot: the catalog default counts at its cost (the three replacement slots of `Custom String` each count a `Null`). -`End` closes a block, with two exceptions. An `If` still open where a rule ends -is closed by the rule and its `End` is neither written nor counted; a `While`, -a `For`, and any block in a subroutine keep theirs. Disabling a rule, action, +`End` closes a block. The count follows the canonical emitted form: the emitter +writes the last action of a rule, when it is an `If`, without its `End`, and +the count omits it too, in a subroutine rule as well; a written and an omitted +trailing `End` are the same program and cost the same. Nested blocks, `While`, +and `For` always have theirs. Disabling a rule, action, or condition has no effect on the count. The calculator is locale-independent: it reads canonical identities and @@ -69,8 +71,11 @@ The independent behavioral source for the supported rules is the The rules above were derived and checked against `#!debugElementCount` of pinned OverPy 9.7.10, which reports an element count for every action and condition. On the `main` entry of a production project (309 rules, 2867 leaf actions) -every action count and every rule count agrees, and the total is 30067, OverPy's -own figure. The client counted OverPy's build of that project at 30070, three +every action count agrees, and the total is 30065 against OverPy's 30067. Eight +rules differ by one element, all because the count follows the canonical emitter +where OverPy's spelling differs: five subroutine rules whose final `If` OverPy +closes with an `End` (the emitter omits it), and three rules whose last `If` +ends in a nested `If` that OverPy closes without `End` (the emitter writes it). The client counted OverPy's build of that project at 30070, three elements more; the cause of those three is not known. Small programs that isolate one rule each, with OverPy's totals, are in `tests/element_count.rs`.