Merge branch 'master' into ens/761-remove-lazy-static

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

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": {

e2e/assets/prebuilt_custom_canister/local_import.did

@@ -0,0 +1,2 @@
+import service "prebuilt_custom_no_build.did";
+

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; };
 }
 

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; };
 }
 

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; };
 }
 

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; };
 }
 

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; };
+}
+

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" {

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 \"\")"
+}

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::<IDLProg>(&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(())
 }
 

src/dfx/src/lib/operations/canister/install_canister.rs

@@ -250,6 +250,7 @@ fn check_candid_compatibility(
     candid: &str,
 ) -> anyhow::Result<Option<String>> {
     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()),