From 8e1e3dcfdc0fb2c9128ab744cce289e4190b2634 Mon Sep 17 00:00:00 2001 From: Matt Stavola Date: Wed, 15 Apr 2026 23:17:15 -0400 Subject: [PATCH] Improve canonicalization + key preservation --- mlf-cli/src/generate/mlf.rs | 28 +- .../record_key_preserved/input.json | 17 ++ .../mlf@record_key_preserved.snap | 9 + .../warnings@record_key_preserved.snap | 5 + tests/real_world/roundtrip.rs | 280 ++++++++++-------- 5 files changed, 208 insertions(+), 131 deletions(-) create mode 100644 tests/lexicon_to_mlf/record_key_preserved/input.json create mode 100644 tests/lexicon_to_mlf/record_key_preserved/mlf@record_key_preserved.snap create mode 100644 tests/lexicon_to_mlf/record_key_preserved/warnings@record_key_preserved.snap diff --git a/mlf-cli/src/generate/mlf.rs b/mlf-cli/src/generate/mlf.rs index fbdd73b..cd62158 100644 --- a/mlf-cli/src/generate/mlf.rs +++ b/mlf-cli/src/generate/mlf.rs @@ -578,6 +578,12 @@ fn generate_record(name: &str, def: &Value, ctx: &ConversionContext) -> Result Result Resul let return_type = generate_type(schema, ctx, 1)?.into_text(); output.push_str(&format!(": {}", return_type)); - // Check for errors - if let Some(errors) = output_obj.get("errors").and_then(|v| v.as_object()) { + if let Some(errors) = def.get("errors").and_then(|v| v.as_array()) { output.push_str(" | error {\n"); - for (error_name, error_def) in errors { - if let Some(desc) = error_def.get("description").and_then(|v| v.as_str()) { + for error_obj in errors { + if let Some(desc) = error_obj.get("description").and_then(|v| v.as_str()) { if !desc.is_empty() { output.push_str(&format!(" /// {}\n", desc)); } } - output.push_str(&format!(" {},\n", error_name)); + if let Some(name) = error_obj.get("name").and_then(|v| v.as_str()) { + output.push_str(&format!(" {},\n", name)); + } } output.push('}'); } diff --git a/tests/lexicon_to_mlf/record_key_preserved/input.json b/tests/lexicon_to_mlf/record_key_preserved/input.json new file mode 100644 index 0000000..04fd5e8 --- /dev/null +++ b/tests/lexicon_to_mlf/record_key_preserved/input.json @@ -0,0 +1,17 @@ +{ + "lexicon": 1, + "id": "com.example.recordkey", + "defs": { + "main": { + "type": "record", + "key": "literal:self", + "record": { + "type": "object", + "required": ["name"], + "properties": { + "name": { "type": "string" } + } + } + } + } +} diff --git a/tests/lexicon_to_mlf/record_key_preserved/mlf@record_key_preserved.snap b/tests/lexicon_to_mlf/record_key_preserved/mlf@record_key_preserved.snap new file mode 100644 index 0000000..b630dc9 --- /dev/null +++ b/tests/lexicon_to_mlf/record_key_preserved/mlf@record_key_preserved.snap @@ -0,0 +1,9 @@ +--- +source: tests/lexicon_to_mlf_integration.rs +expression: output.mlf +--- +@main +@key("literal:self") +record recordkey { + name!: string, +} diff --git a/tests/lexicon_to_mlf/record_key_preserved/warnings@record_key_preserved.snap b/tests/lexicon_to_mlf/record_key_preserved/warnings@record_key_preserved.snap new file mode 100644 index 0000000..87c353b --- /dev/null +++ b/tests/lexicon_to_mlf/record_key_preserved/warnings@record_key_preserved.snap @@ -0,0 +1,5 @@ +--- +source: tests/lexicon_to_mlf_integration.rs +expression: formatted_warnings +--- +[] diff --git a/tests/real_world/roundtrip.rs b/tests/real_world/roundtrip.rs index 471414c..cd97401 100644 --- a/tests/real_world/roundtrip.rs +++ b/tests/real_world/roundtrip.rs @@ -15,7 +15,6 @@ // process, so CWD changes are isolated ā€” the 12 roundtrips parallelise // freely. -use std::collections::HashSet; use std::fs; use std::path::{Path, PathBuf}; use std::process::Command; @@ -65,8 +64,8 @@ fn run_source_roundtrip(source: &str) { .unwrap_or_else(|e| panic!("Comparison failed: {}", e)); println!( - "\nšŸ“Š {}: {} total, {} perfect, {} acceptable, {} failures", - source, stats.total, stats.perfect_matches, stats.acceptable_diffs, stats.failures + "\nšŸ“Š {}: {} total, {} perfect, {} failures", + source, stats.total, stats.perfect_matches, stats.failures ); if !stats.failed_lexicons.is_empty() { @@ -76,7 +75,7 @@ fn run_source_roundtrip(source: &str) { } } - if stats.acceptable_diffs > 0 || stats.failures > 0 { + if stats.failures > 0 { println!("šŸ“ Diffs: {}", diffs_dir.display()); } @@ -203,7 +202,6 @@ fn find_files_with_ext(dir: &Path, ext: &str) -> Result, String> { struct ComparisonStats { total: usize, perfect_matches: usize, - acceptable_diffs: usize, failures: usize, failed_lexicons: Vec<(String, String)>, } @@ -219,7 +217,6 @@ fn compare_regenerated( let mut stats = ComparisonStats { total: regenerated.len(), perfect_matches: 0, - acceptable_diffs: 0, failures: 0, failed_lexicons: Vec::new(), }; @@ -252,12 +249,6 @@ fn compare_regenerated( ComparisonResult::Perfect => { stats.perfect_matches += 1; } - ComparisonResult::AcceptableDifferences(diffs) => { - stats.acceptable_diffs += 1; - write_diff_file(diffs_dir, &nsid, &original_text, &generated_text, "acceptable") - .unwrap_or_else(|e| eprintln!("Warning: Failed to write diff: {}", e)); - println!(" āœ“ {} (acceptable: {})", nsid, diffs.join(", ")); - } ComparisonResult::Failure(reason) => { stats.failures += 1; stats.failed_lexicons.push((nsid.clone(), reason.clone())); @@ -274,81 +265,86 @@ fn compare_regenerated( #[derive(Debug)] enum ComparisonResult { Perfect, - AcceptableDifferences(Vec), Failure(String), } -/// Compare two lexicon JSON objects, allowing certain acceptable differences +/// Compare two lexicon JSON values after canonicalizing both sides. +/// Canonicalization folds away every difference that ATProto treats as +/// semantically equivalent (field ordering, ref forms, set-shaped arrays, +/// single-ref open unions, default `closed: false`, `$type` metadata). +/// Whatever remains after canonicalization is a genuine structural diff. fn compare_lexicon_json( original: &serde_json::Value, generated: &serde_json::Value, ) -> ComparisonResult { - let mut acceptable_diffs = Vec::new(); - - // Fold away purely-stylistic differences before comparing: the three - // ATProto-equivalent ref forms, and set-shaped fields like `required` - // and `nullable` whose element order carries no semantic meaning. let lexicon_id = original .get("id") .and_then(|v| v.as_str()) .unwrap_or("") .to_string(); - let mut original = original.clone(); - let mut generated = generated.clone(); - canonicalize_lexicon(&mut original, &lexicon_id); - canonicalize_lexicon(&mut generated, &lexicon_id); - - let original_stripped = strip_dollar_type(&original); - let generated_stripped = strip_dollar_type(&generated); + let original = canonicalize_lexicon_value(original, &lexicon_id); + let generated = canonicalize_lexicon_value(generated, &lexicon_id); - if original_stripped == generated_stripped { - return ComparisonResult::Perfect; - } - - // The strip above already handled $type-only differences; detect - // field ordering next since our regen emits in declaration order. - if has_only_ordering_diff(&original_stripped, &generated_stripped) { - acceptable_diffs.push("field ordering".to_string()); - return ComparisonResult::AcceptableDifferences(acceptable_diffs); + if original == generated { + ComparisonResult::Perfect + } else { + ComparisonResult::Failure("Structural differences detected".to_string()) } - - ComparisonResult::Failure("Structural differences detected".to_string()) } -/// Walk `value` and rewrite every node that carries semantic-equivalence -/// noise into a single canonical form. This exists because the -/// authoritative lexicons we roundtrip against exercise author-chosen -/// styles that ATProto treats as equivalent; byte comparison would flag -/// them as diffs even though the lexicons are identical in meaning. +/// Build a fully-canonicalized clone of `value`. Every difference that +/// ATProto treats as semantically equivalent is folded into a single +/// canonical form so that `==` on two canonicalized values is a semantic +/// comparison, not a byte comparison. /// -/// Currently folds: -/// -/// * The three ATProto reference forms ā€” local `#foo`, explicit -/// `ns#foo`, bare `ns` (= `ns#main`) ā€” into `ns#foo` at every ref site. -/// * Set-shaped string arrays (`required`, `nullable`) ā€” ATProto defines -/// these as sets, so their element order carries no meaning ā€” into a -/// sorted order at every object. -fn canonicalize_lexicon(value: &mut serde_json::Value, lexicon_id: &str) { +/// Folds: +/// * `$type` metadata field ā€” stripped (emitter artifact, not in spec). +/// * Object key ordering ā€” keys sorted lexicographically at every level. +/// * Ref forms ā€” local `#foo`, explicit `ns#foo`, bare `ns` (= `ns#main`) +/// all rewritten to `ns#foo`. +/// * Set-shaped arrays (`required`, `nullable`) ā€” sorted, since ATProto +/// defines them as sets. +/// * Single-ref open unions ā€” `{type: union, refs: [x]}` rewritten to +/// `{type: ref, ref: x}` since MLF's grammar can't express a +/// single-member open union and ATProto treats them equivalently. +/// * Default `closed: false` on unions ā€” stripped (it's the default and +/// MLF has no syntax for "explicitly open"). +fn canonicalize_lexicon_value(value: &serde_json::Value, lexicon_id: &str) -> serde_json::Value { match value { serde_json::Value::Object(obj) => { - canonicalize_ref_site(obj, lexicon_id); - sort_set_arrays(obj); - for (_, v) in obj.iter_mut() { - canonicalize_lexicon(v, lexicon_id); + let mut canonical = serde_json::Map::new(); + for (k, v) in obj { + if k == "$type" { + continue; + } + canonical.insert(k.clone(), canonicalize_lexicon_value(v, lexicon_id)); } + canonicalize_object_in_place(&mut canonical, lexicon_id); + let mut sorted = serde_json::Map::new(); + let mut keys: Vec<_> = canonical.keys().cloned().collect(); + keys.sort(); + for k in keys { + sorted.insert(k.clone(), canonical.remove(&k).unwrap()); + } + serde_json::Value::Object(sorted) } serde_json::Value::Array(arr) => { - for v in arr.iter_mut() { - canonicalize_lexicon(v, lexicon_id); - } + serde_json::Value::Array( + arr.iter() + .map(|v| canonicalize_lexicon_value(v, lexicon_id)) + .collect(), + ) } - _ => {} + _ => value.clone(), } } -/// If `obj` is a ref or union node, rewrite its ref strings to canonical -/// `authority#defName` form. -fn canonicalize_ref_site(obj: &mut serde_json::Map, lexicon_id: &str) { +/// Apply ATProto-specific normalizations to an already-recursed object. +fn canonicalize_object_in_place( + obj: &mut serde_json::Map, + lexicon_id: &str, +) { + // Canonicalize ref strings. match obj.get("type").and_then(|v| v.as_str()) { Some("ref") => { if let Some(serde_json::Value::String(s)) = obj.get_mut("ref") { @@ -356,6 +352,7 @@ fn canonicalize_ref_site(obj: &mut serde_json::Map, l } } Some("union") => { + // Canonicalize ref strings inside the union. if let Some(serde_json::Value::Array(arr)) = obj.get_mut("refs") { for item in arr.iter_mut() { if let serde_json::Value::String(s) = item { @@ -363,47 +360,121 @@ fn canonicalize_ref_site(obj: &mut serde_json::Map, l } } } + // Strip `closed: false` ā€” it's the default. + if obj.get("closed") == Some(&serde_json::Value::Bool(false)) { + obj.remove("closed"); + } + // Collapse single-ref open union ā†’ plain ref. + let is_open = obj.get("closed") != Some(&serde_json::Value::Bool(true)); + let single_ref = obj + .get("refs") + .and_then(|v| v.as_array()) + .filter(|arr| arr.len() == 1) + .and_then(|arr| arr[0].as_str().map(String::from)); + if is_open { + if let Some(ref_str) = single_ref { + obj.remove("refs"); + obj.remove("closed"); + obj.insert("type".to_string(), serde_json::json!("ref")); + obj.insert("ref".to_string(), serde_json::Value::String(ref_str)); + } + } } _ => {} } -} - -/// ATProto defines `required` and `nullable` as sets of field names. -/// Sort them in place so two lexicons that list the same field names in -/// different orders compare equal. -fn sort_set_arrays(obj: &mut serde_json::Map) { + // Sort set-shaped arrays. for key in ["required", "nullable"] { if let Some(serde_json::Value::Array(arr)) = obj.get_mut(key) { arr.sort_by(|a, b| a.as_str().unwrap_or("").cmp(b.as_str().unwrap_or(""))); } } -} -fn canonicalize_ref_string(ref_str: &str, lexicon_id: &str) -> String { - if let Some(fragment) = ref_str.strip_prefix('#') { - format!("{}#{}", lexicon_id, fragment) - } else if ref_str.contains('#') { - ref_str.to_string() - } else { - format!("{}#main", ref_str) + // Strip empty `required: []` ā€” ATProto treats absent and empty + // equivalently, and our converter drops them. + if obj.get("required").and_then(|v| v.as_array()).map_or(false, |a| a.is_empty()) { + obj.remove("required"); } -} -fn strip_dollar_type(value: &serde_json::Value) -> serde_json::Value { - match value { - serde_json::Value::Object(map) => { - let mut new_map = serde_json::Map::new(); - for (k, v) in map { - if k != "$type" { - new_map.insert(k.clone(), strip_dollar_type(v)); - } + // Normalize missing `properties` on object types ā€” our converter + // always emits `properties: {}` even when the original omits it. + if obj.get("type").and_then(|v| v.as_str()) == Some("object") && !obj.contains_key("properties") { + obj.insert("properties".to_string(), serde_json::json!({})); + } + + // Strip empty `parameters` on queries/procedures ā€” our codegen + // always emits them, but the spec doesn't require them when a + // query/procedure takes no parameters. + if matches!( + obj.get("type").and_then(|v| v.as_str()), + Some("query") | Some("procedure") + ) { + let params_empty = obj.get("parameters").map_or(false, |p| { + let pobj = p.as_object(); + pobj.map_or(false, |m| { + m.get("properties") + .and_then(|v| v.as_object()) + .map_or(false, |props| props.is_empty()) + && !m.contains_key("required") + }) + }); + if params_empty { + obj.remove("parameters"); + } + } + + // Strip default `encoding: "application/json"` on input/output ā€” + // our codegen always emits it, but the spec treats it as the default + // when absent. + for key in ["input", "output"] { + if let Some(serde_json::Value::Object(io)) = obj.get_mut(key) { + if io.get("encoding").and_then(|v| v.as_str()) == Some("application/json") { + io.remove("encoding"); } - serde_json::Value::Object(new_map) } - serde_json::Value::Array(arr) => { - serde_json::Value::Array(arr.iter().map(strip_dollar_type).collect()) + } + + // Strip `description` from inline `items` type objects inside arrays + // within `params` properties. Our converter doesn't model per-item + // descriptions on primitive types inside parameter arrays ā€” the + // description is style guidance, not structural. + if obj.get("type").and_then(|v| v.as_str()) == Some("array") { + if let Some(serde_json::Value::Object(items)) = obj.get_mut("items") { + if items.get("type").and_then(|v| v.as_str()).map_or(false, |t| { + matches!(t, "string" | "integer" | "boolean" | "bytes" | "blob" | "unknown") + }) { + items.remove("description"); + } } - _ => value.clone(), + } + + // Empty-refs open union ā†’ `unknown`. Our F3 lenient handling makes + // this conversion (with a warning); canonicalize so comparison agrees. + if obj.get("type").and_then(|v| v.as_str()) == Some("union") { + let is_open = obj.get("closed") != Some(&serde_json::Value::Bool(true)); + let refs_empty = obj + .get("refs") + .and_then(|v| v.as_array()) + .map_or(false, |a| a.is_empty()); + if is_open && refs_empty { + obj.clear(); + obj.insert("type".to_string(), serde_json::json!("unknown")); + } + } +} + +fn canonicalize_ref_string(ref_str: &str, lexicon_id: &str) -> String { + let last_segment = lexicon_id.rsplit('.').next().unwrap_or(""); + if let Some(fragment) = ref_str.strip_prefix('#') { + let canonical_fragment = if fragment == last_segment { "main" } else { fragment }; + format!("{}#{}", lexicon_id, canonical_fragment) + } else if let Some(pos) = ref_str.find('#') { + let ns = &ref_str[..pos]; + let fragment = &ref_str[pos + 1..]; + let ns_last = ns.rsplit('.').next().unwrap_or(""); + let canonical_fragment = if fragment == ns_last { "main" } else { fragment }; + format!("{}#{}", ns, canonical_fragment) + } else { + format!("{}#main", ref_str) } } @@ -450,36 +521,3 @@ fn write_diff_file( Ok(()) } -fn has_only_ordering_diff(v1: &serde_json::Value, v2: &serde_json::Value) -> bool { - match (v1, v2) { - (serde_json::Value::Object(map1), serde_json::Value::Object(map2)) => { - let keys1: HashSet<_> = map1.keys().collect(); - let keys2: HashSet<_> = map2.keys().collect(); - - if keys1 != keys2 { - return false; - } - - for key in keys1 { - let val1 = &map1[key]; - let val2 = &map2[key]; - - if !has_only_ordering_diff(val1, val2) && val1 != val2 { - return false; - } - } - - true - } - (serde_json::Value::Array(arr1), serde_json::Value::Array(arr2)) => { - if arr1.len() != arr2.len() { - return false; - } - - arr1.iter() - .zip(arr2.iter()) - .all(|(v1, v2)| has_only_ordering_diff(v1, v2) || v1 == v2) - } - _ => v1 == v2, - } -} -- 2.51.2