diff --git a/CHANGELOG.md b/CHANGELOG.md index 8afe3eadce..55816b23af 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,14 @@ # UNRELEASED +### fix: support `import` for local did file + +If the local did file contains `import` or init args, dfx will rewrite the did file when storing in canister metadata. +Due to current limitations of the Candid parser, comments will be dropped during rewriting. +If the local did file doesn't contain `import` or init args, we will not perform the rewriting, thus preserving the comments. + +### fix: subtyping check reports the special opt rule as error + # 0.16.0 ### feat: large canister modules now supported diff --git a/e2e/assets/prebuilt_custom_canister/dfx.json b/e2e/assets/prebuilt_custom_canister/dfx.json index dea4e33388..5e0b21bbb9 100644 --- a/e2e/assets/prebuilt_custom_canister/dfx.json +++ b/e2e/assets/prebuilt_custom_canister/dfx.json @@ -29,6 +29,15 @@ "candid": "custom_with_build_step.did", "wasm": "main.wasm", "build": "echo just a build step" + }, + "prebuilt_local_import": { + "type": "custom", + "candid": "local_import.did", + "wasm": "main.wasm", + "build": [], + "metadata": [ + { "name": "candid:service" } + ] } }, "networks": { diff --git a/e2e/assets/prebuilt_custom_canister/local_import.did b/e2e/assets/prebuilt_custom_canister/local_import.did new file mode 100644 index 0000000000..f926f50663 --- /dev/null +++ b/e2e/assets/prebuilt_custom_canister/local_import.did @@ -0,0 +1,2 @@ +import service "prebuilt_custom_no_build.did"; + diff --git a/e2e/assets/upgrade/v1.mo b/e2e/assets/upgrade/v1.mo index 29e3722bcf..5a8765075f 100644 --- a/e2e/assets/upgrade/v1.mo +++ b/e2e/assets/upgrade/v1.mo @@ -4,6 +4,9 @@ actor { state += 1; return state; }; + public func f() : async ?Int { + return ?42; + }; public query func read() : async Int { return state; }; } diff --git a/e2e/assets/upgrade/v2.mo b/e2e/assets/upgrade/v2.mo index 5a9b295832..64dc2af3ec 100644 --- a/e2e/assets/upgrade/v2.mo +++ b/e2e/assets/upgrade/v2.mo @@ -6,6 +6,9 @@ actor { new_state += 1; return new_state; }; + public func f() : async ?Int { + return ?42; + }; public query func read() : async Nat { return new_state; }; } diff --git a/e2e/assets/upgrade/v2_bad.mo b/e2e/assets/upgrade/v2_bad.mo index 58ef15b8d5..3b50c9bd6a 100644 --- a/e2e/assets/upgrade/v2_bad.mo +++ b/e2e/assets/upgrade/v2_bad.mo @@ -4,6 +4,9 @@ actor { state += 1; return state; }; + public func f() : async ?Int { + return ?42; + }; public query func read() : async Nat { return state; }; } diff --git a/e2e/assets/upgrade/v3_bad.mo b/e2e/assets/upgrade/v3_bad.mo index 6b794e54c9..c5be37f070 100644 --- a/e2e/assets/upgrade/v3_bad.mo +++ b/e2e/assets/upgrade/v3_bad.mo @@ -4,6 +4,9 @@ actor { state += 1; return state; }; + public func f() : async ?Int { + return ?42; + }; public query func read2() : async Int { return state; }; } diff --git a/e2e/assets/upgrade/v4_bad.mo b/e2e/assets/upgrade/v4_bad.mo new file mode 100644 index 0000000000..dc7c35da1d --- /dev/null +++ b/e2e/assets/upgrade/v4_bad.mo @@ -0,0 +1,12 @@ +actor { + stable var state : Int = 0; + public func inc() : async Int { + state += 1; + return state; + }; + public func f() : async ?Text { + return ?""; + }; + public query func read() : async Int { return state; }; +} + diff --git a/e2e/tests-dfx/metadata.bash b/e2e/tests-dfx/metadata.bash index 9790de9178..8fee87cdf2 100644 --- a/e2e/tests-dfx/metadata.bash +++ b/e2e/tests-dfx/metadata.bash @@ -169,6 +169,10 @@ teardown() { # this canister has a build step, but it is an empty array, so dfx leaves the candid:service metadata as-is dfx canister metadata prebuilt_custom_empty_build candid:service >from_canister.txt diff main.did from_canister.txt + + # this canister has a local import in did file, the metadata should flatten the definitions + assert_command dfx canister metadata prebuilt_local_import candid:service + assert_eq "service : { getCanisterId : () -> (principal) query }" } @test "can read canister metadata from replica" { diff --git a/e2e/tests-dfx/upgrade_check.bash b/e2e/tests-dfx/upgrade_check.bash index f4d93fc025..986bb90b62 100644 --- a/e2e/tests-dfx/upgrade_check.bash +++ b/e2e/tests-dfx/upgrade_check.bash @@ -64,3 +64,17 @@ teardown() { assert_command dfx canister call hello_backend read2 '()' assert_match "(1 : int)" } + +@test "warning for using special opt rule" { + install_asset upgrade + dfx_start + dfx deploy + dfx canister call hello_backend f '()' + jq '.canisters.hello_backend.main="v4_bad.mo"' dfx.json | sponge dfx.json + echo yes | ( + assert_command dfx deploy + assert_match "Candid interface compatibility check failed" + ) + assert_command dfx canister call hello_backend f '()' + assert_match "(opt \"\")" +} diff --git a/src/dfx/src/lib/models/canister.rs b/src/dfx/src/lib/models/canister.rs index 002445de10..bcb764ebc1 100644 --- a/src/dfx/src/lib/models/canister.rs +++ b/src/dfx/src/lib/models/canister.rs @@ -338,30 +338,40 @@ fn wasm_opt_level_convert(opt_level: WasmOptLevel) -> OptLevel { } fn separate_candid(path: &Path) -> DfxResult<(String, String)> { + use candid::pretty::candid::{compile, pp_args}; + use candid::types::internal::TypeInner; + use candid_parser::{ + pretty_parse, + types::{Dec, IDLProg}, + }; + let did = dfx_core::fs::read_to_string(path)?; + let prog = pretty_parse::(&format!("{}", path.display()), &did)?; + let has_imports = prog + .decs + .iter() + .any(|dec| matches!(dec, Dec::ImportType(_) | Dec::ImportServ(_))); let (env, actor) = check_candid_file(path)?; let actor = actor.ok_or_else(|| anyhow!("provided candid file contains no main service"))?; - if let candid::types::internal::TypeInner::Class(args, ty) = actor.as_ref() { - use candid_parser::pretty::{ - candid::{compile, pp_ty}, - utils::{concat, enclose}, + let actor = env.trace_type(&actor)?; + let has_init_args = matches!(actor.as_ref(), TypeInner::Class(_, _)); + if has_imports || has_init_args { + let (init_args, serv) = match actor.as_ref() { + TypeInner::Class(args, ty) => (args.clone(), ty.clone()), + TypeInner::Service(_) => (vec![], actor), + _ => unreachable!(), }; - - let actor = Some(ty.clone()); - let service_did = compile(&env, &actor); - let doc = concat(args.iter().map(pp_ty), ","); - let init_args = enclose("(", doc, ")").pretty(80).to_string(); - Ok((service_did, init_args)) + let init_args = pp_args(&init_args).pretty(80).to_string(); + let service = compile(&env, &Some(serv)); + Ok((service, init_args)) } else { - // The original candid from builder output doesn't contain init_args - // Use it directly to avoid items reordering - let service_did = dfx_core::fs::read_to_string(path)?; - let init_args = String::from("()"); - Ok((service_did, init_args)) + // Keep the original did file to preserve comments + Ok((did, "()".to_string())) } } #[context("{} is not a valid subtype of {}", specified_idl_path.display(), compiled_idl_path.display())] fn check_valid_subtype(compiled_idl_path: &Path, specified_idl_path: &Path) -> DfxResult { + use candid::types::subtype::{subtype_with_config, OptReport}; let (mut env, opt_specified) = check_candid_file(specified_idl_path).context("Checking specified candid file.")?; let specified_type = @@ -372,7 +382,13 @@ fn check_valid_subtype(compiled_idl_path: &Path, specified_idl_path: &Path) -> D opt_compiled.expect("Compiled did file should contain some service interface"); let mut gamma = HashSet::new(); let specified_type = env.merge_type(env2, specified_type); - candid::types::subtype::subtype(&mut gamma, &env, &compiled_type, &specified_type)?; + subtype_with_config( + OptReport::Error, + &mut gamma, + &env, + &compiled_type, + &specified_type, + )?; Ok(()) } diff --git a/src/dfx/src/lib/operations/canister/install_canister.rs b/src/dfx/src/lib/operations/canister/install_canister.rs index 03c30fba67..5fe1baefdc 100644 --- a/src/dfx/src/lib/operations/canister/install_canister.rs +++ b/src/dfx/src/lib/operations/canister/install_canister.rs @@ -250,6 +250,7 @@ fn check_candid_compatibility( candid: &str, ) -> anyhow::Result> { use crate::util::check_candid_file; + use candid::types::subtype::{subtype_with_config, OptReport}; let candid_path = canister_info.get_constructor_idl_path(); let deployed_path = canister_info .get_constructor_idl_path() @@ -269,7 +270,7 @@ fn check_candid_compatibility( .ok_or_else(|| anyhow!("Deployed did file should contain some service interface"))?; let mut gamma = HashSet::new(); let old_type = env.merge_type(env2, old_type); - let result = candid::types::subtype::subtype(&mut gamma, &env, &new_type, &old_type); + let result = subtype_with_config(OptReport::Error, &mut gamma, &env, &new_type, &old_type); Ok(match result { Ok(_) => None, Err(e) => Some(e.to_string()),