From 28a889754ebc9b4554833f6475779f5ccf706c4a Mon Sep 17 00:00:00 2001 From: Teakowa Date: Sun, 27 Sep 2026 18:21:01 +0800 Subject: [PATCH 1/2] refactor!: remove catalog-generated typed constructors from the public API Catalog ids and parameter lists no longer shape Rust public items, so catalog-only updates can ship as minor releases. Catalog actions and values are built with Action::call / Value::call. Closes #311 Co-authored-by: Indent --- crates/workshop-rs/Cargo.toml | 3 - crates/workshop-rs/build.rs | 281 ------------------ crates/workshop-rs/src/catalog/mod.rs | 6 +- crates/workshop-rs/src/program.rs | 2 - crates/workshop-rs/tests/catalog.rs | 25 ++ crates/workshop-rs/tests/integration.rs | 2 - crates/workshop-rs/tests/public_api.rs | 80 ++++- crates/workshop-rs/tests/typed_api.rs | 56 ---- docs/README.md | 1 + .../0008-canonical-public-program-boundary.md | 3 +- .../0016-catalog-content-outside-rust-api.md | 61 ++++ docs/adr/README.md | 3 +- docs/compatibility-facades.md | 10 + 13 files changed, 182 insertions(+), 351 deletions(-) delete mode 100644 crates/workshop-rs/build.rs delete mode 100644 crates/workshop-rs/tests/typed_api.rs create mode 100644 docs/adr/0016-catalog-content-outside-rust-api.md diff --git a/crates/workshop-rs/Cargo.toml b/crates/workshop-rs/Cargo.toml index b93f4af..f24ba35 100644 --- a/crates/workshop-rs/Cargo.toml +++ b/crates/workshop-rs/Cargo.toml @@ -17,9 +17,6 @@ serde_json.workspace = true sha2.workspace = true unicode-ident.workspace = true -[build-dependencies] -serde_json.workspace = true - [dev-dependencies] regex = "1" diff --git a/crates/workshop-rs/build.rs b/crates/workshop-rs/build.rs deleted file mode 100644 index b9a22d7..0000000 --- a/crates/workshop-rs/build.rs +++ /dev/null @@ -1,281 +0,0 @@ -use std::{collections::HashSet, env, fmt::Write, fs, path::PathBuf}; - -use serde_json::Value; - -fn main() { - println!("cargo:rerun-if-changed=src/catalog/data/catalog.json"); - - let catalog_path = PathBuf::from(env::var_os("CARGO_MANIFEST_DIR").unwrap()) - .join("src/catalog/data/catalog.json"); - let catalog = fs::read_to_string(&catalog_path) - .unwrap_or_else(|error| panic!("cannot read {}: {error}", catalog_path.display())); - let catalog: Value = serde_json::from_str(&catalog) - .unwrap_or_else(|error| panic!("cannot parse {}: {error}", catalog_path.display())); - - let mut generated = String::new(); - generate_impl( - &mut generated, - &catalog, - "Action", - "actions", - &["call", "disabled"], - ); - generate_impl( - &mut generated, - &catalog, - "Value", - "values", - &[ - "call", - "number", - "string", - "global_variable", - "player_variable", - ], - ); - - let output_path = PathBuf::from(env::var_os("OUT_DIR").unwrap()).join("typed_api.rs"); - fs::write(&output_path, generated) - .unwrap_or_else(|error| panic!("cannot write {}: {error}", output_path.display())); -} - -fn generate_impl( - output: &mut String, - catalog: &Value, - type_name: &str, - section: &str, - reserved: &[&str], -) { - let entries = catalog - .get(section) - .and_then(Value::as_array) - .unwrap_or_else(|| panic!("catalog section '{section}' is not an array")); - - writeln!(output, "impl {type_name} {{").unwrap(); - let mut generated_names = Vec::new(); - for entry in entries { - let id = entry - .get("id") - .and_then(Value::as_str) - .unwrap_or_else(|| panic!("{section} entry has no string id")); - let method_name = method_name(type_name, id); - if reserved.contains(&method_name.as_str()) { - panic!("catalog {section} id '{id}' conflicts with {type_name}::{method_name}"); - } - if generated_names.iter().any(|name| name == &method_name) { - panic!("catalog {section} ids collide as {type_name}::{method_name}"); - } - generated_names.push(method_name.clone()); - - let params = entry - .get("params") - .and_then(Value::as_array) - .cloned() - .unwrap_or_default(); - let parameter_names_source = entry.get("paramNames").and_then(Value::as_array); - if !params.is_empty() && parameter_names_source.is_none() { - panic!("catalog {section} entry '{id}' must declare paramNames"); - } - let parameter_names_source = parameter_names_source.unwrap_or(¶ms); - if parameter_names_source.len() != params.len() { - panic!( - "catalog {section} entry '{id}' must declare one paramNames entry per params entry" - ); - } - let variadic = entry - .get("variadic") - .and_then(Value::as_bool) - .unwrap_or(false); - let mut used_parameter_names = HashSet::new(); - let parameter_names: Vec = parameter_names_source - .iter() - .enumerate() - .map(|(index, parameter)| { - let parameter = parameter.as_str().unwrap_or_else(|| { - panic!("{section} entry '{id}' has a non-string parameter name") - }); - let base = identifier(parameter, &format!("arg_{index}")); - let mut name = base.clone(); - let mut suffix = 2; - while !used_parameter_names.insert(name.clone()) { - name = format!("{base}_{suffix}"); - suffix += 1; - } - name - }) - .collect(); - - let description = format!("Constructs the canonical Workshop {section} `{id}`."); - writeln!(output, " #[doc = {:?}]", description).unwrap(); - if !parameter_names.is_empty() { - writeln!( - output, - " #[doc = \"Parameters: {}.\"]", - parameter_names.join(", ") - ) - .unwrap(); - } - if variadic { - if parameter_names.len() != 1 { - panic!("catalog variadic {section} entry '{id}' must have one repeated parameter"); - } - writeln!( - output, - " pub fn {method_name}(values: impl IntoIterator>) -> Self {{" - ) - .unwrap(); - if type_name == "Value" && id == "array" { - output.push_str( - " Self::Array(values.into_iter().map(|value| value.into()).collect())\n", - ); - } else { - writeln!( - output, - " Self::call({id:?}, values.into_iter().map(|value| value.into()))" - ) - .unwrap(); - } - output.push_str(" }\n"); - continue; - } - - if parameter_names.len() >= 7 { - output.push_str(" #[allow(clippy::too_many_arguments)]\n"); - } - let parameters = parameter_names - .iter() - .map(|name| format!("{name}: impl Into")) - .collect::>() - .join(", "); - writeln!(output, " pub fn {method_name}({parameters}) -> Self {{").unwrap(); - let arguments = parameter_names - .iter() - .map(|name| format!("{name}.into()")) - .collect::>() - .join(", "); - if type_name == "Value" { - match id { - "emptyArray" => output.push_str(" Self::Array(Vec::new())\n"), - "eventPlayer" => output.push_str(" Self::EventPlayer\n"), - "null" => output.push_str(" Self::Null\n"), - "vector" => { - if parameter_names.len() != 3 { - panic!("catalog values entry 'vector' must have three parameters"); - } - writeln!( - output, - " Self::Vector {{ x: Box::new({}.into()), y: Box::new({}.into()), z: Box::new({}.into()) }}", - parameter_names[0], parameter_names[1], parameter_names[2] - ) - .unwrap(); - } - _ => writeln!(output, " Self::call({id:?}, [{arguments}])").unwrap(), - } - } else { - writeln!(output, " Self::call({id:?}, [{arguments}])").unwrap(); - } - output.push_str(" }\n"); - } - output.push_str("}\n\n"); -} - -fn method_name(type_name: &str, id: &str) -> String { - if type_name == "Value" && id == "string" { - "format_string".to_string() - } else { - identifier(id, "builtin") - } -} - -fn identifier(value: &str, fallback: &str) -> String { - if value.starts_with('{') && value.ends_with('}') { - return format!("arg_{}", &value[1..value.len() - 1]); - } - - let mut result = String::new(); - let characters: Vec = value.chars().collect(); - for (index, character) in characters.iter().enumerate() { - if character.is_ascii_uppercase() - && index > 0 - && !result.ends_with('_') - && (characters[index - 1].is_ascii_lowercase() - || characters - .get(index + 1) - .is_some_and(|next| next.is_ascii_lowercase())) - { - result.push('_'); - } - if character.is_ascii_alphanumeric() { - result.push(character.to_ascii_lowercase()); - } else if !result.ends_with('_') { - result.push('_'); - } - } - while result.ends_with('_') { - result.pop(); - } - while result.starts_with('_') { - result.remove(0); - } - if result.is_empty() { - return fallback.to_string(); - } - if is_keyword(&result) { - result.push('_'); - } - result -} - -fn is_keyword(value: &str) -> bool { - matches!( - value, - "as" | "break" - | "const" - | "continue" - | "crate" - | "else" - | "enum" - | "extern" - | "false" - | "fn" - | "for" - | "if" - | "impl" - | "in" - | "let" - | "loop" - | "match" - | "mod" - | "move" - | "mut" - | "pub" - | "ref" - | "return" - | "self" - | "static" - | "struct" - | "super" - | "trait" - | "true" - | "type" - | "unsafe" - | "use" - | "where" - | "while" - | "async" - | "await" - | "dyn" - | "abstract" - | "become" - | "box" - | "do" - | "final" - | "macro" - | "override" - | "priv" - | "typeof" - | "unsized" - | "virtual" - | "yield" - ) -} diff --git a/crates/workshop-rs/src/catalog/mod.rs b/crates/workshop-rs/src/catalog/mod.rs index 2a7df91..91e8bc7 100644 --- a/crates/workshop-rs/src/catalog/mod.rs +++ b/crates/workshop-rs/src/catalog/mod.rs @@ -136,8 +136,7 @@ pub struct CatalogEntry { pub kind: Kind, /// Parameter names, when the catalog documents them. params: Vec, - /// Reviewed semantic parameter names for consumer-facing typed APIs, - /// parallel to `params`. + /// Reviewed semantic parameter names, parallel to `params`. param_names: Vec, /// Reviewed localized spellings for each parameter, parallel to `params`. param_aliases: Vec>>, @@ -475,8 +474,7 @@ struct EntryFile { aliases: HashMap, #[serde(default)] params: Vec, - /// Reviewed semantic parameter names for consumer-facing typed APIs, - /// parallel to `params`. + /// Reviewed semantic parameter names, parallel to `params`. #[serde(default)] param_names: Vec, #[serde(default)] diff --git a/crates/workshop-rs/src/program.rs b/crates/workshop-rs/src/program.rs index 2dd34fd..88284ed 100644 --- a/crates/workshop-rs/src/program.rs +++ b/crates/workshop-rs/src/program.rs @@ -2122,5 +2122,3 @@ impl, const N: usize> From<[T; N]> for Value { Self::Array(values.into_iter().map(Into::into).collect()) } } - -include!(concat!(env!("OUT_DIR"), "/typed_api.rs")); diff --git a/crates/workshop-rs/tests/catalog.rs b/crates/workshop-rs/tests/catalog.rs index 7529faa..5e80bad 100644 --- a/crates/workshop-rs/tests/catalog.rs +++ b/crates/workshop-rs/tests/catalog.rs @@ -416,6 +416,31 @@ fn documented_action_and_value_signatures_are_inventory_entries() { assert_eq!(array.param_type(3), Some("Object|Array")); } +#[test] +fn builtin_entries_with_parameters_declare_reviewed_parameter_names() { + let path = concat!(env!("CARGO_MANIFEST_DIR"), "/src/catalog/data/catalog.json"); + let raw: serde_json::Value = + serde_json::from_str(&std::fs::read_to_string(path).expect("catalog source")) + .expect("catalog source is JSON"); + for section in ["actions", "values"] { + for entry in raw[section].as_array().expect("catalog section") { + let id = entry["id"].as_str().expect("entry id"); + let params = entry["params"].as_array().map_or(0, Vec::len); + if params == 0 { + continue; + } + let names = entry["paramNames"] + .as_array() + .unwrap_or_else(|| panic!("{section} entry '{id}' must declare paramNames")); + assert_eq!( + names.len(), + params, + "{section} entry '{id}' must declare one paramNames entry per params entry" + ); + } + } +} + #[test] fn catalog_defaults_remain_available_through_position_queries() { let catalog = builtin(); diff --git a/crates/workshop-rs/tests/integration.rs b/crates/workshop-rs/tests/integration.rs index b46afa1..af6f389 100644 --- a/crates/workshop-rs/tests/integration.rs +++ b/crates/workshop-rs/tests/integration.rs @@ -24,5 +24,3 @@ mod program_model; mod public_api; #[path = "real_projects.rs"] mod real_projects; -#[path = "typed_api.rs"] -mod typed_api; diff --git a/crates/workshop-rs/tests/public_api.rs b/crates/workshop-rs/tests/public_api.rs index 9df0c4c..a97a49a 100644 --- a/crates/workshop-rs/tests/public_api.rs +++ b/crates/workshop-rs/tests/public_api.rs @@ -1,6 +1,6 @@ use workshop_rs::catalog::{Catalog, Locale}; use workshop_rs::settings::{PathPart, schema}; -use workshop_rs::{emitter, parser, roundtrip, rules}; +use workshop_rs::{Action, Event, Program, Rule, Value, emitter, parser, roundtrip, rules}; #[test] fn canonical_program_operations_cover_parse_validate_inspect_emit_and_roundtrip() { @@ -41,3 +41,81 @@ fn settings_schema_exposes_enum_values_without_the_internal_table() { assert_eq!(enabled.id(), "enabled"); assert_eq!(enabled.english_name(), "Enabled"); } + +#[test] +fn catalog_actions_and_values_are_built_by_canonical_id() { + let catalog = Catalog::builtin().expect("built-in catalog"); + let locale = Locale::new("en-US"); + let numbers = |values: &[i32]| values.iter().copied().map(Value::from).collect::>(); + + let mut program = Program::new(); + program.rule( + Rule::new("catalog calls", Event::Global) + .action(Action::call( + "damage", + [Value::EventPlayer, Value::Null, Value::from(10)], + )) + .action(Action::call( + "teleport", + [ + Value::EventPlayer, + Value::call("vector", numbers(&[1, 2, 3])), + ], + )) + .action(Action::call( + "setSlowMotion", + [Value::call( + "countOf", + [Value::call("array", numbers(&[4, 5, 6]))], + )], + )), + ); + program.validate().expect("catalog calls validate"); + rules::validate_canonical_ids(&program, &catalog).expect("catalog ids resolve"); + + let emitted = emitter::emit(&program, &catalog, &locale).expect("catalog calls emit"); + assert!( + emitted.contains("Damage(Event Player, Null, 10);"), + "{emitted}" + ); + assert!( + emitted.contains("Teleport(Event Player, Vector(1, 2, 3));"), + "{emitted}" + ); + assert!( + emitted.contains("Set Slow Motion(Count Of(Array(4, 5, 6)));"), + "{emitted}" + ); + + let reparsed = parser::parse(&emitted, &catalog, &locale).expect("emitted Workshop reparses"); + assert!(roundtrip::equivalent(&program, &reparsed)); + + let actions = &reparsed.rules[0].actions; + assert!(matches!( + &actions[0], + Action::Call { name, args } if name == "damage" + && matches!(&args[..], [Value::EventPlayer, Value::Null, Value::Number(amount)] if *amount == 10.0) + )); + assert!(matches!( + &actions[1], + Action::Call { name, args } if name == "teleport" + && matches!(&args[..], [Value::EventPlayer, position] if is_call(position, "vector", &[1.0, 2.0, 3.0])) + )); + assert!(matches!( + &actions[2], + Action::Call { name, args } if name == "setSlowMotion" + && matches!(&args[..], [Value::Call { name, args }] if name == "countOf" + && matches!(&args[..], [array] if is_call(array, "array", &[4.0, 5.0, 6.0]))) + )); +} + +fn is_call(value: &Value, id: &str, numbers: &[f64]) -> bool { + matches!( + value, + Value::Call { name, args } if name == id + && args.len() == numbers.len() + && args.iter().zip(numbers).all(|(arg, expected)| { + matches!(arg, Value::Number(actual) if actual == expected) + }) + ) +} diff --git a/crates/workshop-rs/tests/typed_api.rs b/crates/workshop-rs/tests/typed_api.rs deleted file mode 100644 index 66a67ae..0000000 --- a/crates/workshop-rs/tests/typed_api.rs +++ /dev/null @@ -1,56 +0,0 @@ -use workshop_rs::{Action, Value}; - -#[test] -fn typed_constructors_preserve_canonical_ids_and_order() { - let action = Action::set_slow_motion(0.5); - assert!(matches!( - action, - Action::Call { name, args } if name == "setSlowMotion" - && matches!(&args[..], [Value::Number(value)] if *value == 0.5) - )); - - let value = Value::get_max_health(Value::event_player()); - assert!(matches!( - value, - Value::Call { name, args } if name == "getMaxHealth" - && matches!(&args[..], [Value::EventPlayer]) - )); -} - -#[test] -fn typed_values_accept_obvious_rust_literals() { - let value = Value::vector(1, 2, 3); - assert!(matches!( - value, - Value::Vector { x, y, z } - if matches!(x.as_ref(), Value::Number(value) if *value == 1.0) - && matches!(y.as_ref(), Value::Number(value) if *value == 2.0) - && matches!(z.as_ref(), Value::Number(value) if *value == 3.0) - )); - - let array = Value::array(["first", "second"]); - assert!(matches!( - array, - Value::Array(values) - if matches!(&values[..], [Value::String(first), Value::String(second)] - if first == "first" && second == "second") - )); - - assert!(matches!(Value::empty_array(), Value::Array(values) if values.is_empty())); - assert!(matches!(Value::null(), Value::Null)); -} - -#[test] -fn generic_calls_remain_available_for_dynamic_consumers() { - let action = Action::call("runtimeAction", [Value::from(1)]); - let value = Value::call("runtimeValue", [Value::from("input")]); - - assert!(matches!( - action, - Action::Call { name, args } if name == "runtimeAction" && args.len() == 1 - )); - assert!(matches!( - value, - Value::Call { name, args } if name == "runtimeValue" && args.len() == 1 - )); -} diff --git a/docs/README.md b/docs/README.md index bc7684f..40c694a 100644 --- a/docs/README.md +++ b/docs/README.md @@ -94,6 +94,7 @@ Current registry: - [ADR-0011: Contextual Workshop semantics at the catalog/code boundary](adr/0011-contextual-semantic-placement.md) - [ADR-0012: Tests-first Workshop verification](adr/0012-tests-first-verification.md) - [ADR-0013: Source mapping across the provider boundary](adr/0013-source-mapping-across-provider-boundary.md) +- [ADR-0016: Catalog content stays outside the Rust public API](adr/0016-catalog-content-outside-rust-api.md) ## Release and operations diff --git a/docs/adr/0008-canonical-public-program-boundary.md b/docs/adr/0008-canonical-public-program-boundary.md index 89625ec..6c34622 100644 --- a/docs/adr/0008-canonical-public-program-boundary.md +++ b/docs/adr/0008-canonical-public-program-boundary.md @@ -1,6 +1,7 @@ # ADR-0008: Canonical public Workshop `Program` boundary -- Status: Accepted (backfilled) +- Status: Accepted (backfilled); decision 5 superseded by + [ADR-0016](0016-catalog-content-outside-rust-api.md) - Date: 2026-09-12 - Related: [Issue #32](https://github.com/wrightkit/workshop-rs/issues/32), diff --git a/docs/adr/0016-catalog-content-outside-rust-api.md b/docs/adr/0016-catalog-content-outside-rust-api.md new file mode 100644 index 0000000..4a8b05a --- /dev/null +++ b/docs/adr/0016-catalog-content-outside-rust-api.md @@ -0,0 +1,61 @@ +# ADR-0016: Catalog content stays outside the Rust public API + +- Status: Accepted +- Date: 2026-09-27 +- Related: [Issue #311](https://github.com/wrightkit/workshop-rs/issues/311), + [Issue #252](https://github.com/wrightkit/workshop-rs/issues/252), + [Issue #299](https://github.com/wrightkit/workshop-rs/issues/299); + [ADR-0001](0001-catalog-boundaries.md), + [ADR-0008](0008-canonical-public-program-boundary.md) + +## Context + +[ADR-0008](0008-canonical-public-program-boundary.md) decision 5 generated a +typed inherent constructor on `Action` or `Value` for every catalog action and +value, named after the catalog id and taking one argument per catalog +parameter. At v0.9.1 that was 474 public methods (215 on `Action`, 259 on +`Value`). Each one forwarded to `Action::call` / `Value::call`, except that a +few values built a dedicated variant (`Value::Null`, `Value::EventPlayer`, +`Value::Vector`, `Value::Array`). + +The method names and signatures came from catalog data, so catalog content was +part of the Rust semver surface. Removing or renaming an id, or adding a +parameter to an existing action, removed a public method or changed its +signature; #299 shipped as a breaking release for that reason. Catalog updates +follow Overwatch's seasonal cadence and need to ship as minor releases under +1.x. + +No WrightKit consumer used the generated constructors. `opy-rs`, Wright, and +`deltin-rs` build catalog actions and values through `Action::call` / +`Value::call`. + +## Decision + +1. `workshop-rs` does not expose catalog-generated Rust items. The typed + constructors are removed; this replaces ADR-0008 decision 5. The rest of + ADR-0008 is unchanged. +2. Catalog actions and values are built with `Action::call` / `Value::call` + from the canonical id and the arguments in catalog parameter order. + Canonical validation against a catalog decides whether a call is valid. + Hand-written constructors and the public `Action` / `Value` variants remain + supported. +3. A catalog-only change (adding, removing, or renaming an entry or parameter) + produces no Rust public-API difference. Its compatibility is a Workshop data + question, reported through catalog identity and validation, not a + `cargo semver-checks` question. + +## Alternatives considered + +- **Keep the constructors and ship catalog changes as breaking releases.** + Rejected: every seasonal update would need a new major version under 1.x. +- **Keep the constructors behind `#[doc(hidden)]`.** Rejected: hidden items are + still public Rust API, so the semver consequence is unchanged. +- **A catalog-versioned typed builder, or a separate typed-constructor crate.** + Not decided here. It would be additive and needs its own decision. + +## Consequences + +- Catalog updates can ship as minor releases. +- Consumers get no compile-time check of catalog ids or parameter counts from + the Rust API; canonical validation reports them instead. +- The build no longer runs a catalog code generator. diff --git a/docs/adr/README.md b/docs/adr/README.md index 763c970..ad88134 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -26,7 +26,7 @@ ADR. - [ADR-0005: Seasonal Workshop client validation workflow](0005-seasonal-client-validation.md) (partially superseded) - [ADR-0006: Canonical typed Workshop settings semantics](0006-settings-semantic-schema.md) - [ADR-0007: Hero gameplay domain API and source boundary](0007-gameplay-domain-api.md) -- [ADR-0008: Canonical public Workshop `Program` boundary](0008-canonical-public-program-boundary.md) +- [ADR-0008: Canonical public Workshop `Program` boundary](0008-canonical-public-program-boundary.md) (partially superseded) - [ADR-0009: Domain-local Workshop ownership and verification placement](0009-domain-local-ownership.md) - [ADR-0010: Canonical Workshop target-layout and resource analysis](0010-target-layout-and-resource-analysis.md) - [ADR-0011: Contextual Workshop semantics at the catalog/code boundary](0011-contextual-semantic-placement.md) (partially superseded) @@ -34,6 +34,7 @@ ADR. - [ADR-0013: Source mapping across the provider boundary](0013-source-mapping-across-provider-boundary.md) - [ADR-0014: Evidence for canonical validation of slot acceptance](0014-validation-evidence-for-slot-acceptance.md) - [ADR-0015: Contextual literal substitutions are accepted, not normalized](0015-contextual-literals-are-preserved.md) +- [ADR-0016: Catalog content stays outside the Rust public API](0016-catalog-content-outside-rust-api.md) ADR-0007 was originally committed with a duplicate `ADR-0002` identifier. The number was corrected to ADR-0007; the recorded gameplay decision is unchanged. diff --git a/docs/compatibility-facades.md b/docs/compatibility-facades.md index 4236cf6..c73c261 100644 --- a/docs/compatibility-facades.md +++ b/docs/compatibility-facades.md @@ -29,6 +29,16 @@ parse-context contract shared by Workshop parsing and frontends that supply expected enum domains. Both are intentional public APIs, not compatibility-only facades; the catalog remains the sole source of signature data. +## Catalog-backed actions and values + +Catalog actions and values are built with `Action::call` and `Value::call`, +passing the canonical catalog id and the arguments in catalog parameter order. +Canonical validation against a catalog decides whether the call is valid. The +crate exposes no Rust items generated from catalog data, so adding, removing, +or renaming a catalog entry or parameter does not change the Rust public API +and can ship in a minor release +([ADR-0016](adr/0016-catalog-content-outside-rust-api.md)). + ## Compatibility-only facades No compatibility-only public facades are intentionally retained. WIR storage From a0dfeedfdfe4a9bf2f6dc3d3db7b4a4e449ca99b Mon Sep 17 00:00:00 2001 From: Teakowa Date: Sun, 27 Sep 2026 18:51:51 +0800 Subject: [PATCH 2/2] docs: document call shapes for special catalog values Address review of #316: state which Value shapes the parser produces for null, eventPlayer, vector, array, and emptyArray; narrow the catalog-only change claim to Rust item names, signatures, and types; correct the #[doc(hidden)] rationale; list ADR-0014 and ADR-0015 in the docs index; read the embedded catalog in the paramNames test; tidy the public API test. Co-authored-by: Indent --- crates/workshop-rs/tests/catalog.rs | 4 +--- crates/workshop-rs/tests/public_api.rs | 15 +++++++++------ docs/README.md | 2 ++ .../0016-catalog-content-outside-rust-api.md | 19 +++++++++++++++---- docs/compatibility-facades.md | 19 ++++++++++++++----- 5 files changed, 41 insertions(+), 18 deletions(-) diff --git a/crates/workshop-rs/tests/catalog.rs b/crates/workshop-rs/tests/catalog.rs index 5e80bad..0d92ae9 100644 --- a/crates/workshop-rs/tests/catalog.rs +++ b/crates/workshop-rs/tests/catalog.rs @@ -418,10 +418,8 @@ fn documented_action_and_value_signatures_are_inventory_entries() { #[test] fn builtin_entries_with_parameters_declare_reviewed_parameter_names() { - let path = concat!(env!("CARGO_MANIFEST_DIR"), "/src/catalog/data/catalog.json"); let raw: serde_json::Value = - serde_json::from_str(&std::fs::read_to_string(path).expect("catalog source")) - .expect("catalog source is JSON"); + serde_json::from_str(workshop_rs::catalog::CATALOG_DATA).expect("embedded catalog is JSON"); for section in ["actions", "values"] { for entry in raw[section].as_array().expect("catalog section") { let id = entry["id"].as_str().expect("entry id"); diff --git a/crates/workshop-rs/tests/public_api.rs b/crates/workshop-rs/tests/public_api.rs index a97a49a..ff31b2a 100644 --- a/crates/workshop-rs/tests/public_api.rs +++ b/crates/workshop-rs/tests/public_api.rs @@ -88,18 +88,19 @@ fn catalog_actions_and_values_are_built_by_canonical_id() { ); let reparsed = parser::parse(&emitted, &catalog, &locale).expect("emitted Workshop reparses"); - assert!(roundtrip::equivalent(&program, &reparsed)); + assert!(roundtrip::equivalent(&program, &reparsed), "{emitted}"); let actions = &reparsed.rules[0].actions; assert!(matches!( &actions[0], Action::Call { name, args } if name == "damage" - && matches!(&args[..], [Value::EventPlayer, Value::Null, Value::Number(amount)] if *amount == 10.0) + && matches!(&args[..], [Value::EventPlayer, Value::Null, amount] if is_number(amount, 10.0)) )); assert!(matches!( &actions[1], Action::Call { name, args } if name == "teleport" - && matches!(&args[..], [Value::EventPlayer, position] if is_call(position, "vector", &[1.0, 2.0, 3.0])) + && matches!(&args[..], [Value::EventPlayer, position] + if is_call(position, "vector", &[1.0, 2.0, 3.0])) )); assert!(matches!( &actions[2], @@ -114,8 +115,10 @@ fn is_call(value: &Value, id: &str, numbers: &[f64]) -> bool { value, Value::Call { name, args } if name == id && args.len() == numbers.len() - && args.iter().zip(numbers).all(|(arg, expected)| { - matches!(arg, Value::Number(actual) if actual == expected) - }) + && args.iter().zip(numbers).all(|(arg, expected)| is_number(arg, *expected)) ) } + +fn is_number(value: &Value, expected: f64) -> bool { + matches!(value, Value::Number(actual) if *actual == expected) +} diff --git a/docs/README.md b/docs/README.md index 40c694a..eeb3d8f 100644 --- a/docs/README.md +++ b/docs/README.md @@ -94,6 +94,8 @@ Current registry: - [ADR-0011: Contextual Workshop semantics at the catalog/code boundary](adr/0011-contextual-semantic-placement.md) - [ADR-0012: Tests-first Workshop verification](adr/0012-tests-first-verification.md) - [ADR-0013: Source mapping across the provider boundary](adr/0013-source-mapping-across-provider-boundary.md) +- [ADR-0014: Evidence for canonical validation of slot acceptance](adr/0014-validation-evidence-for-slot-acceptance.md) +- [ADR-0015: Contextual literal substitutions are accepted, not normalized](adr/0015-contextual-literals-are-preserved.md) - [ADR-0016: Catalog content stays outside the Rust public API](adr/0016-catalog-content-outside-rust-api.md) ## Release and operations diff --git a/docs/adr/0016-catalog-content-outside-rust-api.md b/docs/adr/0016-catalog-content-outside-rust-api.md index 4a8b05a..cd46670 100644 --- a/docs/adr/0016-catalog-content-outside-rust-api.md +++ b/docs/adr/0016-catalog-content-outside-rust-api.md @@ -38,10 +38,15 @@ No WrightKit consumer used the generated constructors. `opy-rs`, Wright, and from the canonical id and the arguments in catalog parameter order. Canonical validation against a catalog decides whether a call is valid. Hand-written constructors and the public `Action` / `Value` variants remain - supported. + supported. `null` and `eventPlayer` are built with `Value::Null` and + `Value::EventPlayer`, the shape the parser produces; the parser produces + `Value::Call` for other catalog values, including `vector`, `array`, and + `emptyArray`. 3. A catalog-only change (adding, removing, or renaming an entry or parameter) - produces no Rust public-API difference. Its compatibility is a Workshop data - question, reported through catalog identity and validation, not a + does not change any Rust item's name, signature, or type. Only the value of + the embedded catalog text (`catalog::CATALOG_DATA`) and the data behind + `Catalog::builtin()` change. Its compatibility is a Workshop data question, + reported through catalog identity and validation, not a `cargo semver-checks` question. ## Alternatives considered @@ -49,7 +54,9 @@ No WrightKit consumer used the generated constructors. `opy-rs`, Wright, and - **Keep the constructors and ship catalog changes as breaking releases.** Rejected: every seasonal update would need a new major version under 1.x. - **Keep the constructors behind `#[doc(hidden)]`.** Rejected: hidden items are - still public Rust API, so the semver consequence is unchanged. + still public Rust API, so a catalog edit would still break any caller of an + affected method. The `Public API compatibility` CI job excludes + `#[doc(hidden)]` paths, so it would no longer report those breaks. - **A catalog-versioned typed builder, or a separate typed-constructor crate.** Not decided here. It would be additive and needs its own decision. @@ -58,4 +65,8 @@ No WrightKit consumer used the generated constructors. `opy-rs`, Wright, and - Catalog updates can ship as minor releases. - Consumers get no compile-time check of catalog ids or parameter counts from the Rust API; canonical validation reports them instead. +- The removed constructors built `Value::Vector` and `Value::Array` for + `vector`, `array`, and `emptyArray`. Code migrating to `Value::call` gets the + `Value::Call` shape the parser produces, and an empty array built this way + counts as the parsed `Empty Array` element rather than an `Array` value. - The build no longer runs a catalog code generator. diff --git a/docs/compatibility-facades.md b/docs/compatibility-facades.md index c73c261..feb032d 100644 --- a/docs/compatibility-facades.md +++ b/docs/compatibility-facades.md @@ -33,11 +33,20 @@ facades; the catalog remains the sole source of signature data. Catalog actions and values are built with `Action::call` and `Value::call`, passing the canonical catalog id and the arguments in catalog parameter order. -Canonical validation against a catalog decides whether the call is valid. The -crate exposes no Rust items generated from catalog data, so adding, removing, -or renaming a catalog entry or parameter does not change the Rust public API -and can ship in a minor release -([ADR-0016](adr/0016-catalog-content-outside-rust-api.md)). +Canonical validation against a catalog decides whether the call is valid. No +Rust item is generated from catalog data, so adding, removing, or renaming a +catalog entry or parameter does not change any Rust item's name, signature, or +type and can ship in a minor release +([ADR-0016](adr/0016-catalog-content-outside-rust-api.md)). The embedded +catalog text in `catalog::CATALOG_DATA` changes with the data; its type does +not. + +Build `null` and `eventPlayer` with the `Value::Null` and `Value::EventPlayer` +variants, which is what the parser produces. `Value::call("null", [])` and +`Value::call("eventPlayer", [])` emit the same text but are not round-trip +equivalent to their re-parsed form. For every other catalog value, including +`vector`, `array`, and `emptyArray`, the parser produces `Value::Call` with the +canonical id, so `Value::call` builds the parsed shape. ## Compatibility-only facades