diff --git a/CHANGELOG.md b/CHANGELOG.md index 2533fe9b1..d65e14e96 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,24 @@ # Changelog +## 2026-04-08 + +### didc 0.6.1 + +* Non-breaking changes: + + `didc check` now reports **all** incompatible changes at once, grouped by method, instead of stopping at the first error + + Clearer error messages: e.g. "missing in new interface" and "function annotation changed from query to update" + +### candid_parser 0.3.1 + +* Non-breaking changes: + + Add `service_compatibility_report()` returning a full grouped compatibility report as a string + +### Candid 0.10.27 + +* Non-breaking changes: + + Add `subtype_check_all()` to collect all subtype errors in one pass (previously stopped at the first) + + Add `Incompatibility` type and `format_report()` for structured, hierarchical error reporting + ## 2026-03-18 ### Candid 0.10.26 diff --git a/Cargo.lock b/Cargo.lock index 8eb17f36e..e0a09b737 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -209,14 +209,14 @@ dependencies = [ [[package]] name = "candid" -version = "0.10.26" +version = "0.10.27" dependencies = [ "anyhow", "bincode", "binread", "byteorder", - "candid_derive 0.10.26", - "candid_parser 0.3.0", + "candid_derive 0.10.27", + "candid_parser 0.3.1", "hex", "ic_principal 0.1.2", "leb128", @@ -247,7 +247,7 @@ dependencies = [ [[package]] name = "candid_derive" -version = "0.10.26" +version = "0.10.27" dependencies = [ "lazy_static", "proc-macro2 1.0.86", @@ -276,11 +276,11 @@ dependencies = [ [[package]] name = "candid_parser" -version = "0.3.0" +version = "0.3.1" dependencies = [ "anyhow", "arbitrary", - "candid 0.10.26", + "candid 0.10.27", "codespan-reporting", "console", "convert_case", @@ -479,10 +479,10 @@ dependencies = [ [[package]] name = "didc" -version = "0.6.0" +version = "0.6.1" dependencies = [ "anyhow", - "candid_parser 0.3.0", + "candid_parser 0.3.1", "clap", "console", "hex", diff --git a/rust/bench/Cargo.lock b/rust/bench/Cargo.lock index ef54d1e25..9b15a6219 100644 --- a/rust/bench/Cargo.lock +++ b/rust/bench/Cargo.lock @@ -156,7 +156,7 @@ dependencies = [ [[package]] name = "candid" -version = "0.10.26" +version = "0.10.27" dependencies = [ "anyhow", "binread", @@ -177,7 +177,7 @@ dependencies = [ [[package]] name = "candid_derive" -version = "0.10.26" +version = "0.10.27" dependencies = [ "lazy_static", "proc-macro2", @@ -187,7 +187,7 @@ dependencies = [ [[package]] name = "candid_parser" -version = "0.3.0" +version = "0.3.1" dependencies = [ "anyhow", "candid", diff --git a/rust/candid/Cargo.toml b/rust/candid/Cargo.toml index 5655ede03..f5b0a8b4c 100644 --- a/rust/candid/Cargo.toml +++ b/rust/candid/Cargo.toml @@ -1,7 +1,7 @@ [package] name = "candid" # sync with the version in `candid_derive/Cargo.toml` -version = "0.10.26" +version = "0.10.27" edition = "2021" rust-version.workspace = true authors = ["DFINITY Team"] @@ -16,7 +16,7 @@ keywords = ["internet-computer", "idl", "candid", "dfinity"] include = ["src", "Cargo.toml", "LICENSE", "README.md"] [dependencies] -candid_derive = { path = "../candid_derive", version = "=0.10.26" } +candid_derive = { path = "../candid_derive", version = "=0.10.27" } ic_principal = { path = "../ic_principal", version = "0.1.0" } binread = { version = "2.2", features = ["debug_template"] } byteorder = "1.5.0" diff --git a/rust/candid/src/types/internal.rs b/rust/candid/src/types/internal.rs index 6e82e15cf..e11364879 100644 --- a/rust/candid/src/types/internal.rs +++ b/rust/candid/src/types/internal.rs @@ -16,9 +16,8 @@ pub struct TypeId { impl TypeId { pub fn of() -> Self { let name = std::any::type_name::(); - #[allow(function_casts_as_integer)] TypeId { - id: TypeId::of:: as usize, + id: TypeId::of:: as *const () as usize, name, } } diff --git a/rust/candid/src/types/subtype.rs b/rust/candid/src/types/subtype.rs index df67df9f7..bf3027aff 100644 --- a/rust/candid/src/types/subtype.rs +++ b/rust/candid/src/types/subtype.rs @@ -4,6 +4,7 @@ use crate::utils::RecursionDepth; use crate::{Error, Result}; use anyhow::Context; use std::collections::{HashMap, HashSet}; +use std::fmt; pub type Gamma = HashSet<(Type, Type)>; @@ -36,6 +37,477 @@ pub fn subtype_with_config( subtype_(report, gamma, env, t1, t2, &RecursionDepth::new()) } +/// A single incompatibility found during subtype checking. +#[derive(Debug, Clone)] +pub struct Incompatibility { + /// Path to the incompatible element, from outermost to innermost. + /// e.g., `["method \"transfer\"", "return type", "record field \"status\""]` + pub path: Vec, + /// Description of the specific incompatibility. + pub message: String, +} + +impl fmt::Display for Incompatibility { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + if self.path.is_empty() { + write!(f, "{}", self.message) + } else { + for (i, segment) in self.path.iter().enumerate() { + if i > 0 { + write!(f, " > ")?; + } + write!(f, "{segment}")?; + } + write!(f, ": {}", self.message) + } + } +} + +/// Format a list of incompatibilities as a grouped, hierarchical report. +/// +/// Errors are grouped by their shared path prefixes so that, for example, +/// five errors under `method "transfer"` appear together rather than as +/// five separate top-level items. +/// +/// ```text +/// method "transfer": +/// return type: +/// - record field a: text is not a subtype of nat +/// - record field b: nat is not a subtype of bool +/// input type: +/// - missing required field amount (type nat) +/// +/// method "get_user": +/// - missing in new interface +/// ``` +pub fn format_report(errors: &[Incompatibility]) -> String { + if errors.is_empty() { + return String::new(); + } + + // Build a tree: each node is either a branch (has children keyed by path segment) + // or a leaf (has a message). + struct Node { + children: Vec<(String, Node)>, + messages: Vec, + } + + impl Node { + fn new() -> Self { + Node { + children: Vec::new(), + messages: Vec::new(), + } + } + fn child(&mut self, key: &str) -> &mut Node { + if let Some(pos) = self.children.iter().position(|(k, _)| k == key) { + &mut self.children[pos].1 + } else { + self.children.push((key.to_string(), Node::new())); + let last = self.children.len() - 1; + &mut self.children[last].1 + } + } + fn insert(&mut self, path: &[String], message: &str) { + if path.is_empty() { + self.messages.push(message.to_string()); + } else { + self.child(&path[0]).insert(&path[1..], message); + } + } + fn render(&self, out: &mut String, indent: usize) { + let pad = " ".repeat(indent); + for msg in &self.messages { + out.push_str(&pad); + out.push_str("- "); + out.push_str(msg); + out.push('\n'); + } + for (key, child) in &self.children { + // If the child has exactly one message and no sub-children, inline it. + if child.children.is_empty() && child.messages.len() == 1 { + out.push_str(&pad); + out.push_str(key); + out.push_str(": "); + out.push_str(&child.messages[0]); + out.push('\n'); + } else { + out.push_str(&pad); + out.push_str(key); + out.push_str(":\n"); + child.render(out, indent + 1); + } + } + } + } + + let mut root = Node::new(); + for e in errors { + root.insert(&e.path, &e.message); + } + + let mut out = String::new(); + root.render(&mut out, 0); + // Remove trailing newline + if out.ends_with('\n') { + out.pop(); + } + out +} + +/// Check if `t1 <: t2`, collecting **all** incompatibilities instead of stopping at the first. +/// +/// Returns an empty `Vec` when `t1` is indeed a subtype of `t2`. +/// This is intended for upgrade compatibility reports where users need to see +/// every breaking change at once. +pub fn subtype_check_all( + gamma: &mut Gamma, + env: &TypeEnv, + t1: &Type, + t2: &Type, +) -> Vec { + let mut errors = Vec::new(); + subtype_collect_( + OptReport::Warning, + gamma, + env, + t1, + t2, + &RecursionDepth::new(), + &mut Vec::new(), + &mut errors, + false, + ); + errors +} + +/// Internal collecting variant of `subtype_`. Instead of short-circuiting on +/// the first error, this continues through all fields/methods/args and pushes +/// every incompatibility it finds into `errors`. +#[allow(clippy::too_many_arguments)] +fn subtype_collect_( + report: OptReport, + gamma: &mut Gamma, + env: &TypeEnv, + t1: &Type, + t2: &Type, + depth: &RecursionDepth, + path: &mut Vec, + errors: &mut Vec, + is_input: bool, +) { + let _guard = match depth.guard() { + Ok(g) => g, + Err(_) => { + errors.push(Incompatibility { + path: path.clone(), + message: "recursion limit exceeded".to_string(), + }); + return; + } + }; + use TypeInner::*; + if t1 == t2 { + return; + } + // Handle Var/Knot (type variables / recursive types) + if matches!(t1.as_ref(), Var(_) | Knot(_)) || matches!(t2.as_ref(), Var(_) | Knot(_)) { + if !gamma.insert((t1.clone(), t2.clone())) { + return; // co-inductive: assume OK + } + let before = errors.len(); + match (t1.as_ref(), t2.as_ref()) { + (Var(id), _) => subtype_collect_( + report, + gamma, + env, + env.rec_find_type_with_depth(id, depth).unwrap(), + t2, + depth, + path, + errors, + is_input, + ), + (_, Var(id)) => subtype_collect_( + report, + gamma, + env, + t1, + env.rec_find_type_with_depth(id, depth).unwrap(), + depth, + path, + errors, + is_input, + ), + (Knot(id), _) => subtype_collect_( + report, + gamma, + env, + &find_type(id).unwrap(), + t2, + depth, + path, + errors, + is_input, + ), + (_, Knot(id)) => subtype_collect_( + report, + gamma, + env, + t1, + &find_type(id).unwrap(), + depth, + path, + errors, + is_input, + ), + (_, _) => unreachable!(), + }; + if errors.len() > before { + gamma.remove(&(t1.clone(), t2.clone())); + } + return; + } + match (t1.as_ref(), t2.as_ref()) { + (_, Reserved) => (), + (Empty, _) => (), + (Nat, Int) => (), + (Vec(ty1), Vec(ty2)) => { + subtype_collect_(report, gamma, env, ty1, ty2, depth, path, errors, is_input); + } + (Null, Opt(_)) => (), + // For opt rules we delegate to the existing subtype_ to test the condition, + // since these are probes, not things that generate multiple independent errors. + (Opt(ty1), Opt(ty2)) if subtype_(report, gamma, env, ty1, ty2, depth).is_ok() => {} + (_, Opt(ty2)) + if subtype_(report, gamma, env, t1, ty2, depth).is_ok() + && !matches!( + env.trace_type_with_depth(ty2, depth) + .map(|t| t.as_ref().clone()), + Ok(Null | Reserved | Opt(_)) + ) => {} + (_, Opt(_)) => { + let msg = format!("WARNING: {t1} <: {t2} due to special subtyping rules involving optional types/fields (see https://github.com/dfinity/candid/blob/c7659ca/spec/Candid.md#upgrading-and-subtyping). This means the two interfaces have diverged, which could cause data loss."); + match report { + OptReport::Silence => (), + OptReport::Warning => eprintln!("{msg}"), + OptReport::Error => { + errors.push(Incompatibility { + path: path.clone(), + message: msg, + }); + } + }; + } + (Record(fs1), Record(fs2)) => { + let fields: HashMap<_, _> = fs1.iter().map(|Field { id, ty }| (id, ty)).collect(); + for Field { id, ty: ty2 } in fs2 { + match fields.get(id) { + Some(ty1) => { + path.push(format!("record field {id}")); + subtype_collect_( + report, gamma, env, ty1, ty2, depth, path, errors, is_input, + ); + path.pop(); + } + None => { + let is_optional = env + .trace_type_with_depth(ty2, depth) + .map(|t| matches!(t.as_ref(), Null | Reserved | Opt(_))) + .unwrap_or(false); + if !is_optional { + errors.push(Incompatibility { + path: path.clone(), + message: if is_input { + format!( + "new service requires field {id} (type {ty2}), \ + which old callers don't provide and is not optional" + ) + } else { + format!( + "new type is missing required field {id} (type {ty2}), \ + which is expected by the old type and is not optional" + ) + }, + }); + } + } + } + } + } + (Variant(fs1), Variant(fs2)) => { + let fields: HashMap<_, _> = fs2.iter().map(|Field { id, ty }| (id, ty)).collect(); + for Field { id, ty: ty1 } in fs1 { + match fields.get(id) { + Some(ty2) => { + path.push(format!("variant field {id}")); + subtype_collect_( + report, gamma, env, ty1, ty2, depth, path, errors, is_input, + ); + path.pop(); + } + None => { + errors.push(Incompatibility { + path: path.clone(), + message: if is_input { + format!( + "old callers may send variant case {id}, \ + which the new service no longer handles" + ) + } else { + format!( + "new variant has field {id} that does not exist in the old type" + ) + }, + }); + } + } + } + } + (Service(ms1), Service(ms2)) => { + let meths: HashMap<_, _> = ms1.iter().cloned().collect(); + for (name, ty2) in ms2 { + match meths.get(name) { + Some(ty1) => { + path.push(format!("method \"{name}\"")); + subtype_collect_(report, gamma, env, ty1, ty2, depth, path, errors, false); + path.pop(); + } + None => { + errors.push(Incompatibility { + path: path.clone(), + message: format!( + "method \"{name}\" is expected by the old interface but missing in the new one" + ), + }); + } + } + } + } + (Func(f1), Func(f2)) => { + if f1.modes != f2.modes { + errors.push(Incompatibility { + path: path.clone(), + message: format!( + "function annotation changed from {old} to {new}", + old = if f2.modes.is_empty() { + "update".to_string() + } else { + pp_modes(&f2.modes) + }, + new = if f1.modes.is_empty() { + "update".to_string() + } else { + pp_modes(&f1.modes) + }, + ), + }); + // Don't return early - also check arg/ret compatibility + } + // Check each argument directly instead of wrapping in a tuple record, + // so we get clean error paths like "input argument 1" instead of "record field 0". + check_func_params( + report, gamma, env, &f2.args, &f1.args, depth, path, errors, "input", true, + ); + check_func_params( + report, gamma, env, &f1.rets, &f2.rets, depth, path, errors, "return", false, + ); + } + (Class(_, t), _) => { + subtype_collect_(report, gamma, env, t, t2, depth, path, errors, is_input); + } + (_, Class(_, t)) => { + subtype_collect_(report, gamma, env, t1, t, depth, path, errors, is_input); + } + (Unknown, _) => unreachable!(), + (_, Unknown) => unreachable!(), + (_, _) => { + errors.push(Incompatibility { + path: path.clone(), + message: format!("{t1} is not a subtype of {t2}"), + }); + } + } +} + +/// Check function parameters (args or rets) for subtype compatibility, +/// collecting all errors. Handles the record-tuple wrapping so that single-arg +/// functions don't produce misleading "record field 0" paths. +/// +/// For inputs (contravariant): `sub_params` = old args, `sup_params` = new args. +/// For outputs (covariant): `sub_params` = new rets, `sup_params` = old rets. +#[allow(clippy::too_many_arguments)] +fn check_func_params( + report: OptReport, + gamma: &mut Gamma, + env: &TypeEnv, + sub_params: &[Type], + sup_params: &[Type], + depth: &RecursionDepth, + path: &mut Vec, + errors: &mut Vec, + label: &str, // "input" or "return" + is_input: bool, // affects wording +) { + // Use the same tuple wrapping as the original subtype_ for correctness, + // but when there's a single parameter, check it directly to avoid noise. + if sub_params.len() == 1 && sup_params.len() == 1 { + path.push(format!("{label} type")); + subtype_collect_( + report, + gamma, + env, + &sub_params[0], + &sup_params[0], + depth, + path, + errors, + is_input, + ); + path.pop(); + } else { + let sub_tuple = to_tuple(sub_params); + let sup_tuple = to_tuple(sup_params); + path.push(if sub_params.len() == sup_params.len() { + format!("{label} types") + } else if is_input { + // sub_params = old args, sup_params = new args (contravariant swap) + format!( + "{label} types (old has {} arg{}, new has {})", + sub_params.len(), + if sub_params.len() == 1 { "" } else { "s" }, + sup_params.len() + ) + } else { + format!( + "{label} types (old has {} value{}, new has {})", + sup_params.len(), + if sup_params.len() == 1 { "" } else { "s" }, + sub_params.len() + ) + }); + subtype_collect_( + report, gamma, env, &sub_tuple, &sup_tuple, depth, path, errors, is_input, + ); + path.pop(); + } +} + +fn pp_modes(modes: &[super::internal::FuncMode]) -> String { + if modes.is_empty() { + return String::new(); + } + modes + .iter() + .map(|m| match m { + super::internal::FuncMode::Oneway => "oneway", + super::internal::FuncMode::Query => "query", + super::internal::FuncMode::CompositeQuery => "composite_query", + }) + .collect::>() + .join(" ") +} + fn subtype_( report: OptReport, gamma: &mut Gamma, diff --git a/rust/candid_derive/Cargo.toml b/rust/candid_derive/Cargo.toml index 9aee2aa3f..efd23f980 100644 --- a/rust/candid_derive/Cargo.toml +++ b/rust/candid_derive/Cargo.toml @@ -1,7 +1,7 @@ [package] name = "candid_derive" # sync with the version in `candid/Cargo.toml` -version = "0.10.26" +version = "0.10.27" edition = "2021" rust-version.workspace = true authors = ["DFINITY Team"] diff --git a/rust/candid_parser/Cargo.toml b/rust/candid_parser/Cargo.toml index 3293440d1..dbb4edffe 100644 --- a/rust/candid_parser/Cargo.toml +++ b/rust/candid_parser/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "candid_parser" -version = "0.3.0" +version = "0.3.1" edition = "2021" rust-version.workspace = true authors = ["DFINITY Team"] diff --git a/rust/candid_parser/src/utils.rs b/rust/candid_parser/src/utils.rs index 1c7df1ca2..03b0d333e 100644 --- a/rust/candid_parser/src/utils.rs +++ b/rust/candid_parser/src/utils.rs @@ -39,6 +39,25 @@ pub fn service_compatible(new: CandidSource, old: CandidSource) -> Result<()> { Ok(()) } +/// Check compatibility of two service types, returning **all** incompatibilities +/// instead of stopping at the first one. +/// +/// Returns an empty `Vec` when the new interface is backward-compatible with the old one. +pub fn service_compatibility_report( + new: CandidSource, + old: CandidSource, +) -> Result> { + let (mut env, t1) = new.load()?; + let t1 = t1.ok_or_else(|| Error::msg("new interface has no main service type"))?; + let (env2, t2) = old.load()?; + let t2 = t2.ok_or_else(|| Error::msg("old interface has no main service type"))?; + let mut gamma = std::collections::HashSet::new(); + let t2 = env.merge_type(env2, t2); + Ok(candid::types::subtype::subtype_check_all( + &mut gamma, &env, &t1, &t2, + )) +} + /// Check structural equality of two service types pub fn service_equal(left: CandidSource, right: CandidSource) -> Result<()> { let (mut env, t1) = left.load()?; diff --git a/rust/candid_parser/tests/compatibility.rs b/rust/candid_parser/tests/compatibility.rs new file mode 100644 index 000000000..2671fe924 --- /dev/null +++ b/rust/candid_parser/tests/compatibility.rs @@ -0,0 +1,538 @@ +use candid::types::subtype::{format_report, Incompatibility}; +use candid_parser::utils::{service_compatibility_report, service_compatible, CandidSource}; + +// --------------------------------------------------------------------------- +// Helpers +// --------------------------------------------------------------------------- + +fn check_compatible(new: &str, old: &str, desc: &str) { + service_compatible(CandidSource::Text(new), CandidSource::Text(old)) + .unwrap_or_else(|e| panic!("[{desc}] expected compatible, got error: {e}")); +} + +fn check_incompatible(new: &str, old: &str, desc: &str) { + assert!( + service_compatible(CandidSource::Text(new), CandidSource::Text(old)).is_err(), + "[{desc}] expected incompatible, but check passed" + ); +} + +fn get_errors(new: &str, old: &str) -> Vec { + service_compatibility_report(CandidSource::Text(new), CandidSource::Text(old)) + .expect("failed to load interfaces") +} + +fn error_strings(new: &str, old: &str) -> Vec { + get_errors(new, old) + .into_iter() + .map(|e| e.to_string()) + .collect() +} + +// =========================================================================== +// Backward-compatible changes must PASS (no false positives) +// =========================================================================== + +#[test] +fn compatible_service_changes() { + let cases: &[(&str, &str, &str)] = &[ + ( + "identical service", + "service : { greet : (text) -> (text) }", + "service : { greet : (text) -> (text) }", + ), + ( + "add new method", + "service : { greet : (text) -> (text); hello : () -> (text) }", + "service : { greet : (text) -> (text) }", + ), + ( + "widen input nat→int (contravariant)", + "service : { pay : (int) -> () }", + "service : { pay : (nat) -> () }", + ), + ( + "narrow return int→nat (covariant, nat <: int)", + "service : { get : () -> (nat) }", + "service : { get : () -> (int) }", + ), + ]; + for &(desc, new, old) in cases { + check_compatible(new, old, desc); + } +} + +#[test] +fn compatible_record_field_changes() { + let cases: &[(&str, &str, &str)] = &[ + ( + "add opt field to input record", + "type R = record { name : text; age : opt nat }; service : { f : (R) -> () }", + "type R = record { name : text }; service : { f : (R) -> () }", + ), + ( + "add opt field to return record", + "type R = record { id : nat; extra : opt text }; service : { f : () -> (R) }", + "type R = record { id : nat }; service : { f : () -> (R) }", + ), + ( + "add null field to return record", + "service : { f : () -> (record { a : nat; b : null }) }", + "service : { f : () -> (record { a : nat }) }", + ), + ( + "add reserved field to return record", + "service : { f : () -> (record { a : nat; b : reserved }) }", + "service : { f : () -> (record { a : nat }) }", + ), + ( + "remove non-opt field from input record (extra fields in subtype are fine)", + "type R = record { name : text }; service : { f : (R) -> () }", + "type R = record { name : text; age : nat }; service : { f : (R) -> () }", + ), + ]; + for &(desc, new, old) in cases { + check_compatible(new, old, desc); + } +} + +#[test] +fn compatible_variant_and_vec_changes() { + let cases: &[(&str, &str, &str)] = &[ + ( + "add variant case to input (contravariant: old callers still match)", + "type V = variant { a; b; c }; service : { f : (V) -> () }", + "type V = variant { a; b }; service : { f : (V) -> () }", + ), + ( + "identical vec types", + "service : { f : () -> (vec nat) }", + "service : { f : () -> (vec nat) }", + ), + ]; + for &(desc, new, old) in cases { + check_compatible(new, old, desc); + } +} + +// =========================================================================== +// Backward-INCOMPATIBLE changes must be caught (no false negatives) +// =========================================================================== + +#[test] +fn incompatible_method_changes() { + let cases: &[(&str, &str, &str)] = &[ + ( + "remove method", + "service : { greet : (text) -> (text) }", + "service : { greet : (text) -> (text); hello : () -> (text) }", + ), + ( + "change func mode query→update", + "service : { get : () -> (nat) }", + "service : { get : () -> (nat) query }", + ), + ( + "change return type nat→text", + "service : { get : () -> (text) }", + "service : { get : () -> (nat) }", + ), + ( + "change input type nat→text", + "service : { set : (text) -> () }", + "service : { set : (nat) -> () }", + ), + ( + "widen return nat→int (covariant: int (int) }", + "service : { get : () -> (nat) }", + ), + ]; + for &(desc, new, old) in cases { + check_incompatible(new, old, desc); + } +} + +#[test] +fn incompatible_record_and_variant_changes() { + let cases: &[(&str, &str, &str)] = &[ + ( + "add required field to input record", + "type R = record { name : text; age : nat }; service : { f : (R) -> () }", + "type R = record { name : text }; service : { f : (R) -> () }", + ), + ( + "remove required field from return record", + "type R = record { name : text }; service : { f : () -> (R) }", + "type R = record { name : text; age : nat }; service : { f : () -> (R) }", + ), + ( + "remove variant case from input (old callers may send it)", + "type V = variant { start; stop }; service : { f : (V) -> () }", + "type V = variant { start; stop; pause }; service : { f : (V) -> () }", + ), + ( + "change vec element type", + "service : { f : () -> (vec text) }", + "service : { f : () -> (vec nat) }", + ), + ]; + for &(desc, new, old) in cases { + check_incompatible(new, old, desc); + } +} + +// =========================================================================== +// ALL incompatibilities collected (not just the first) +// =========================================================================== + +#[test] +fn collects_all_incompatible_methods() { + let old = r#"service : { + method_a : (nat) -> (nat); + method_b : (text) -> (text); + method_c : () -> (nat); + method_d : () -> (); + }"#; + // method_a: OK, method_b: removed, method_c: return changed, method_d: removed + let new = r#"service : { + method_a : (nat) -> (nat); + method_c : () -> (text); + }"#; + let errors = error_strings(new, old); + assert_eq!(errors.len(), 3, "got: {errors:?}"); + + let joined = errors.join("\n"); + for name in ["method_b", "method_c", "method_d"] { + assert!(joined.contains(name), "missing {name} in: {joined}"); + } + assert!( + !joined.contains("method_a"), + "method_a is compatible, should not appear: {joined}" + ); +} + +#[test] +fn collects_all_incompatible_record_fields() { + let old = "type R = record { a : nat; b : text; c : bool }; service : { f : () -> (R) }"; + let new = "type R = record { a : text; b : nat; c : bool }; service : { f : () -> (R) }"; + let errors = error_strings(new, old); + assert_eq!(errors.len(), 2, "got: {errors:?}"); + + let joined = errors.join("\n"); + assert!( + joined.contains("record field a"), + "missing field a: {joined}" + ); + assert!( + joined.contains("record field b"), + "missing field b: {joined}" + ); +} + +#[test] +fn collects_both_input_and_return_errors() { + let old = "service : { call : (nat) -> (nat) }"; + let new = "service : { call : (text) -> (bool) }"; + let errors = error_strings(new, old); + assert_eq!(errors.len(), 2, "got: {errors:?}"); + + let joined = errors.join("\n"); + assert!( + joined.contains("input type"), + "missing input error: {joined}" + ); + assert!( + joined.contains("return type"), + "missing return error: {joined}" + ); +} + +#[test] +fn collects_variant_field_errors() { + // Old callers may send variant cases b or c; new input must accept them + let old = "type V = variant { a; b; c }; service : { f : (V) -> () }"; + let new = "type V = variant { a }; service : { f : (V) -> () }"; + let errors = error_strings(new, old); + assert_eq!(errors.len(), 2, "got: {errors:?}"); + + let joined = errors.join("\n"); + assert!(joined.contains("b"), "missing variant b: {joined}"); + assert!(joined.contains("c"), "missing variant c: {joined}"); +} + +// =========================================================================== +// Error message quality + path context +// =========================================================================== + +#[test] +fn missing_method_error_is_clear() { + let old = "service : { transfer : (nat) -> (); balance : () -> (nat) }"; + let new = "service : { balance : () -> (nat) }"; + let errors = error_strings(new, old); + assert_eq!(errors.len(), 1); + assert!(errors[0].contains("transfer"), "should name the method"); + assert!(errors[0].contains("missing"), "should say 'missing'"); +} + +#[test] +fn type_mismatch_error_names_both_types() { + let old = "service : { f : () -> (nat) }"; + let new = "service : { f : () -> (text) }"; + let errors = error_strings(new, old); + assert_eq!(errors.len(), 1); + assert!( + errors[0].contains("text") && errors[0].contains("nat"), + "should mention both types: {}", + errors[0] + ); +} + +#[test] +fn missing_required_field_error_is_clear() { + let old = "type R = record { name : text; age : nat }; service : { f : () -> (R) }"; + let new = "type R = record { name : text }; service : { f : () -> (R) }"; + let errors = error_strings(new, old); + assert_eq!(errors.len(), 1); + let msg = &errors[0]; + assert!(msg.contains("age"), "should mention field name: {msg}"); + assert!( + msg.contains("missing") || msg.contains("not optional"), + "should explain the problem: {msg}" + ); +} + +#[test] +fn nested_path_shows_full_context() { + let old = r#" + type Inner = record { x : nat }; + type Outer = record { inner : Inner }; + service : { get : () -> (Outer) } + "#; + let new = r#" + type Inner = record { x : text }; + type Outer = record { inner : Inner }; + service : { get : () -> (Outer) } + "#; + let errors = error_strings(new, old); + assert_eq!(errors.len(), 1, "got: {errors:?}"); + let msg = &errors[0]; + assert!(msg.contains("method"), "path should include method: {msg}"); + assert!( + msg.contains("return type"), + "path should include return type: {msg}" + ); + assert!( + msg.contains("record field inner"), + "path should include outer field: {msg}" + ); + assert!( + msg.contains("record field x"), + "path should include inner field: {msg}" + ); +} + +// =========================================================================== +// Multi-arg functions +// =========================================================================== + +#[test] +fn multi_arg_function_incompatibilities() { + let old = "service : { f : (nat, text) -> (bool, nat) }"; + let new = "service : { f : (nat, bool) -> (bool, text) }"; + // Input: old (nat, text) must <: new (nat, bool) → field 1 text (text); g : () -> () }", + "service : { f : (text) -> (text) }", + ), + ( + "service : { f : (int) -> () }", + "service : { f : (nat) -> () }", + ), + ]; + for (new, old) in compatible { + let report = get_errors(new, old); + assert!(report.is_empty(), "report should be empty for compatible"); + assert!( + service_compatible(CandidSource::Text(new), CandidSource::Text(old)).is_ok(), + "service_compatible should pass for compatible" + ); + } + + let incompatible = [ + ( + "service : { f : (text) -> (text) }", + "service : { f : (text) -> (text); g : () -> () }", + ), + ( + "service : { f : (nat) -> () }", + "service : { f : (int) -> () }", + ), + ]; + for (new, old) in incompatible { + let report = get_errors(new, old); + assert!(!report.is_empty(), "report should have errors"); + assert!( + service_compatible(CandidSource::Text(new), CandidSource::Text(old)).is_err(), + "service_compatible should fail" + ); + } +} + +// =========================================================================== +// Hierarchical report formatting +// =========================================================================== + +#[test] +fn format_report_groups_by_method_and_nests() { + let old = r#"service : { + transfer : (record { from : text; to : text; amount : nat }) -> (record { ok : bool; balance : nat }); + balance : () -> (nat); + audit : () -> (record { count : nat; log : text }) query; + config : () -> (record { flag : bool }); + }"#; + let new = r#"service : { + transfer : (record { from : text; to : text; amount : text }) -> (record { ok : text; balance : text }); + balance : () -> (text); + audit : () -> (record { count : text; log : nat }); + }"#; + let errors = get_errors(new, old); + assert!(errors.len() >= 6, "expected many errors, got: {errors:?}"); + + let report = format_report(&errors); + + // Each method should appear exactly once as a group header + for method in ["transfer", "balance", "config"] { + let header_count = report + .lines() + .filter(|l| { + let trimmed = l.trim_start(); + trimmed.starts_with(&format!("method \"{method}\"")) + || trimmed.contains(&format!("method \"{method}\"")) + }) + .count(); + assert!( + header_count <= 1, + "{method} should appear at most once as header, got {header_count} in:\n{report}" + ); + } + + // Should have indented sub-groups + assert!( + report.contains(" return type") || report.contains(" input type"), + "should have indented sub-groups:\n{report}" + ); +} + +#[test] +fn format_report_inlines_single_leaf() { + let old = "service : { get : () -> (nat) }"; + let new = "service : { get : () -> (text) }"; + let report = format_report(&get_errors(new, old)); + // Single error under method > return type should inline compactly + assert!( + report.lines().count() <= 3, + "single error should be compact:\n{report}" + ); +} + +#[test] +fn format_report_handles_path_and_pathless_errors() { + // Pathless errors (e.g. top-level type mismatch) should render as "- message" + let errors = vec![ + Incompatibility { + path: vec![], + message: "top-level mismatch".to_string(), + }, + Incompatibility { + path: vec!["method \"foo\"".to_string()], + message: "missing in new interface".to_string(), + }, + ]; + let report = format_report(&errors); + assert!( + report.contains("- top-level mismatch"), + "pathless error should render with bullet:\n{report}" + ); + assert!( + report.contains("method \"foo\""), + "pathed error should render:\n{report}" + ); +} + +// =========================================================================== +// Input-side message wording +// =========================================================================== + +#[test] +fn input_record_required_field_added_message_names_correct_side() { + let old = "type R = record { name : text }; service : { f : (R) -> () }"; + let new = "type R = record { name : text; age : nat }; service : { f : (R) -> () }"; + let errors = error_strings(new, old); + assert_eq!( + errors.len(), + 1, + "expected exactly one error, got: {errors:?}" + ); + let msg = &errors[0]; + assert!( + !msg.contains("new type is missing"), + "message incorrectly blames the new type for the missing field: {msg}" + ); + assert!( + msg.contains("age"), + "message should mention the field name: {msg}" + ); +} + +#[test] +fn input_variant_case_removed_message_names_correct_side() { + let old = "type V = variant { a; b }; service : { f : (V) -> () }"; + let new = "type V = variant { a }; service : { f : (V) -> () }"; + let errors = error_strings(new, old); + assert_eq!( + errors.len(), + 1, + "expected exactly one error, got: {errors:?}" + ); + let msg = &errors[0]; + assert!( + !msg.contains("new variant has field"), + "message incorrectly says the new variant has the dropped case: {msg}" + ); + assert!( + msg.contains("b"), + "message should mention the variant case name: {msg}" + ); +} + +#[test] +fn input_arg_count_increase_message_has_correct_counts() { + let old = "service : { f : (nat) -> () }"; + let new = "service : { f : (nat, text) -> () }"; + let errors = error_strings(new, old); + assert!(!errors.is_empty(), "expected at least one error, got none"); + let msg = errors + .iter() + .find(|e| e.contains("arg")) + .unwrap_or_else(|| panic!("no arg-count error found in: {errors:?}")); + assert!( + msg.contains("old has 1") && msg.contains("new has 2"), + "arg-count message has wrong old/new values: {msg}" + ); +} diff --git a/tools/didc/Cargo.toml b/tools/didc/Cargo.toml index 52f4f9448..3eecbdd71 100644 --- a/tools/didc/Cargo.toml +++ b/tools/didc/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "didc" -version = "0.6.0" +version = "0.6.1" authors = ["DFINITY Team"] edition = "2021" diff --git a/tools/didc/src/main.rs b/tools/didc/src/main.rs index f22ce6ca8..5495319ac 100644 --- a/tools/didc/src/main.rs +++ b/tools/didc/src/main.rs @@ -191,7 +191,25 @@ fn main() -> Result<()> { if strict { subtype::equal(&mut gamma, &env, &t1, &t2)?; } else { - subtype::subtype(&mut gamma, &env, &t1, &t2)?; + let errors = subtype::subtype_check_all(&mut gamma, &env, &t1, &t2); + if !errors.is_empty() { + let report = subtype::format_report(&errors); + eprintln!( + "{} {} incompatible change{} found:\n", + style("Error:").red().bold(), + errors.len(), + if errors.len() == 1 { "" } else { "s" } + ); + for line in report.lines() { + eprintln!(" {line}"); + } + eprintln!(); + bail!( + "new interface is not backward compatible ({} breaking change{})", + errors.len(), + if errors.len() == 1 { "" } else { "s" } + ); + } } } _ => { diff --git a/tools/ui/src/didjs/lib.rs b/tools/ui/src/didjs/lib.rs index a3a932708..afbd2800d 100644 --- a/tools/ui/src/didjs/lib.rs +++ b/tools/ui/src/didjs/lib.rs @@ -69,7 +69,17 @@ fn subtype(new: String, old: String) -> Result<(), String> { let old_actor = check_prog(&mut old_env, &old).unwrap().unwrap(); let mut gamma = std::collections::HashSet::new(); let old_actor = new_env.merge_type(old_env, old_actor); - subtype::subtype(&mut gamma, &new_env, &new_actor, &old_actor).map_err(|e| e.to_string()) + let errors = subtype::subtype_check_all(&mut gamma, &new_env, &new_actor, &old_actor); + if errors.is_empty() { + Ok(()) + } else { + let report = subtype::format_report(&errors); + Err(format!( + "{} incompatible change{} found:\n\n{report}", + errors.len(), + if errors.len() == 1 { "" } else { "s" } + )) + } } fn retrieve(path: &str) -> Option<(&str, &'static [u8])> {