Repository: dfinity/sdk (rust)

Issue:
FIX ME special opt rule hit!
Just hit this message when deploying to a canister. I was purposely changing the type signature of my API (breaking change), and hit this message instead of signaling a dfx deploy warning.

```
FIX ME! vec record { text; AttributeValue } <: opt UserMetadata/1 via special opt rule.
This means the sender and receiver type has diverged, and can cause data loss.
```

Pull request:
fix: fix some remaining candid issues
# Description

* Support `import` for local did file. Fix SDK-746 and #2874 
* Subtyping check reports special opt rule as error, so that developers can confirm the type in the upgrade. Fix #3519 #3003 

# How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration.

# Checklist:

- [ ] The title of this PR complies with [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/).
- [ ] I have edited the CHANGELOG accordingly.
- [ ] I have made corresponding changes to the documentation.


Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 8afe3ead..55816b23 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/src/dfx/src/lib/models/canister.rs b/src/dfx/src/lib/models/canister.rs
index 002445de..bcb764eb 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::<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(())
 }
 
diff --git a/src/dfx/src/lib/operations/canister/install_canister.rs b/src/dfx/src/lib/operations/canister/install_canister.rs
index 03c30fba..5fe1baef 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<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()),


Test patch:
diff --git a/e2e/assets/prebuilt_custom_canister/dfx.json b/e2e/assets/prebuilt_custom_canister/dfx.json
index dea4e333..5e0b21bb 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 00000000..f926f506
--- /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 29e3722b..5a876507 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 5a9b2958..64dc2af3 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 58ef15b8..3b50c9bd 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 6b794e54..c5be37f0 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 00000000..dc7c35da
--- /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 9790de91..8fee87cd 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 f4d93fc0..986bb90b 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 \"\")"
+}