fix: resolve ic:<principal> imports via --actor-id-alias principal matching#6016
fix: resolve ic:<principal> imports via --actor-id-alias principal matching#6016
Conversation
…tching When a Motoko file uses `ic:aaaaa-aa` (direct principal import), moc now also checks all `--actor-id-alias` entries for one whose principal (2nd arg) matches. If exactly one match is found its explicit DID path is used, bypassing the `--actor-idl` directory lookup entirely. Additional behaviour: - Warns if `--actor-idl` also contains a matching .did file (nudging towards the explicit `--actor-id-alias` form). - Warns and uses the first match when multiple aliases map to the same principal. Three new tests cover: resolution without --actor-idl, precedence over an empty --actor-idl dir, and the ambiguous multi-alias warning.
| | Some actor_base -> | ||
| let full_path = in_base actor_base (Url.idl_basename_of_blob bytes) in | ||
| add_idl_import msgs imported ri_ref at full_path (Either.Right bytes) | ||
| and find_principal_aliases bytes = |
There was a problem hiding this comment.
Design: let … and … implies mutual recursion that doesn't exist.
and in a let … and … in binding means simultaneous/mutually-recursive definition. None of resolve_ic, find_principal_aliases, warn_idl_conflict call each other. The rest of the file uses plain sequential let … in for local helpers. Use that here too:
let resolve_ic bytes = …
in
let find_principal_aliases bytes = …
in
let warn_idl_conflict bytes did_path = …
in| M.fold (fun alias v acc -> match v with | ||
| | Either.Right (b, Some did_path) when b = bytes -> (alias, did_path) :: acc | ||
| | _ -> acc) aliases [] | ||
| and warn_idl_conflict at bytes did_path = |
There was a problem hiding this comment.
Design: at parameter shadows the outer at needlessly.
The enclosing function already has at in scope (from the (f, ri_ref, at) argument tuple). Having warn_idl_conflict take at as a parameter just shadows it. Since the function is only called once, either capture at from the closure or inline the 4-line body directly — both remove the shadowing.
| Diag.add_msg msgs | ||
| (Diag.warning_message at "M0260" "import" | ||
| (Printf.sprintf | ||
| "IDL file for principal also found via --actor-idl; \ |
There was a problem hiding this comment.
Bug: M0260 suggestion is circular and uses principal twice.
"Consider using --actor-id-alias %s %s %s directly."
(Url.encode_principal bytes) (Url.encode_principal bytes) did_pathThe first %s is meant for the alias name but gets the principal again. More importantly: the user already has --actor-id-alias set (that's how we got here) — telling them to add it again is circular. Suggest instead:
"IDL file for this principal is also present under --actor-idl. Consider removing it from the --actor-idl directory to avoid confusion."
| | Some actor_base -> | ||
| let full_path = in_base actor_base (Url.idl_basename_of_blob bytes) in | ||
| add_idl_import msgs imported ri_ref at full_path (Either.Right bytes) | ||
| and find_principal_aliases bytes = |
There was a problem hiding this comment.
Design: --actor-env-alias exclusion is silent and untested.
find_principal_aliases only matches Either.Right (b, Some did_path), so Either.Left entries from --actor-env-alias are silently skipped. That's probably correct (the principal isn't known at compile time for env-aliases), but a user who writes --actor-env-alias mgmt MY_ID /path.did and then import "ic:<principal>" will get the cryptic M0008. Worth a comment here and a test documenting the expected behaviour.
| warn_idl_conflict at bytes did_path; | ||
| add_idl_import msgs imported ri_ref at (Lib.FilePath.normalise did_path) (Either.Right bytes) | ||
| | matches -> | ||
| let aliases_str = String.concat ", " (List.map fst matches) in |
There was a problem hiding this comment.
Bug: List.hd picks the last alphabetical alias, not the first.
M.fold visits keys in ascending alphabetical order. With :: acc (prepend), the list is built in reverse order — so List.hd matches returns the last alphabetically, not the first. The warning says "using the first one found" and the .ok file says (mgmt1, mgmt2), but both are wrong relative to what the code does.
Fix: reverse the accumulator before returning, or sort at the call site:
List.sort (fun (a,_) (b,_) -> String.compare a b) matchesThen update the .ok file accordingly.
| @@ -0,0 +1 @@ | |||
| (unknown location): warning [M0261], Multiple --actor-id-alias entries match principal aaaaa-aa (mgmt1, mgmt2); using the first one found. | |||
There was a problem hiding this comment.
Bug: (unknown location) will not match actual output.
at here is the source region of the import expression — a real file location like /tmp/…XXXXXX.mo:1.8-1.22. The test runner does not strip temp-file paths automatically.
The existing tests that have location-containing output (e.g. actor-env-alias-bad-utf8.sh) pipe through sed "s|$tmp|test.mo|". Add that to actor-id-alias-principal-ambiguous.sh and regenerate the .ok with the correct prefix, e.g.:
test.mo:1.8-1.22: warning [M0261], …
| "M0244", None, "Mutable variable is never reassigned"; | ||
| "M0254", None, "Initial actor requires field" | ||
| "M0254", None, "Initial actor requires field"; | ||
| "M0260", None, "Actor-id-alias shadows --actor-idl lookup"; |
There was a problem hiding this comment.
Nit: add a comment explaining the gap M0255–M0259.
These codes exist in error_codes (not warning_codes), so the jump from M0254 to M0260 looks like a mistake without context:
"M0254", None, "Initial actor requires field";
(* M0255-M0259 are error-only codes, see error_codes above *)
"M0260", None, "Actor-id-alias shadows --actor-idl lookup";| - `"mo:<package-name>/<path>"` then `<pat>` is bound to the library module defined in file `<package-path>/<path>.mo` in directory `<package-path>` referenced by package alias `<package-name>`. The mapping from `<package-name>` to `<package-path>` is determined by a compiler command-line argument `--package <package-name> <package-path>`. For example, `import L "mo:core/List"` defines `L` to reference the `List` library in package alias `core`. | ||
|
|
||
| - `"ic:<canisterid>"` then `<pat>` is bound to a Motoko actor whose Motoko type is determined by the canister’s IDL interface. The IDL interface of canister `<canisterid>` must be found in file `<actorpath>/<canisterid>.did`. The compiler assumes that `<actorpath>` is specified by command line argument `--actor-idl <actorpath>` and that file `<actorpath>/<canisterid>.did` exists. For example, `import C "ic:lg264-qjkae"` defines `C` to reference the actor with canister id `lg264-qjkae` and IDL file `lg264-qjkae.did`. | ||
| - `"ic:<canisterid>"` then `<pat>` is bound to a Motoko actor whose Motoko type is determined by the canister’s IDL interface. The preferred way to supply the IDL file is `--actor-id-alias <alias> <canisterid> <idl-file>`, where `<canisterid>` matches the imported principal; the compiler then uses `<idl-file>` directly without needing a search directory. Alternatively, the IDL file can be found via the legacy `--actor-idl <actorpath>` flag, which requires `<actorpath>/<canisterid>.did` to exist. For example, `import C "ic:lg264-qjkae"` defines `C` to reference the actor with canister id `lg264-qjkae`; supply its IDL with `--actor-id-alias mycanister lg264-qjkae lg264-qjkae.did` or with `--actor-idl <dir>` where `<dir>/lg264-qjkae.did` exists. |
There was a problem hiding this comment.
Nit: clarify that the alias name doesn't matter for ic: imports.
The example uses mycanister as the alias, but since ic:lg264-qjkae is resolved by principal matching (not alias name), mycanister is never consulted. A reader will wonder whether they need to use the canister id as the alias name. Add a note:
Note: when resolving
ic:<principal>imports, only the principal (second argument) of--actor-id-aliasis matched; the alias name is not used and may be any identifier.
| | Some actor_base -> | ||
| let full_path = in_base actor_base (Url.idl_basename_of_blob bytes) in | ||
| add_idl_import msgs imported ri_ref at full_path (Either.Right bytes) | ||
| and find_principal_aliases bytes = |
There was a problem hiding this comment.
Design: let … and … implies mutual recursion that doesn't exist.
and in a let … and … in binding means simultaneous/mutually-recursive definitions. None of resolve_ic, find_principal_aliases, warn_idl_conflict call each other. The rest of the file uses plain sequential let … in for local helpers — use that here too:
let resolve_ic bytes = …
in
let find_principal_aliases bytes = …
in
let warn_idl_conflict bytes did_path = …
in| M.fold (fun alias v acc -> match v with | ||
| | Either.Right (b, Some did_path) when b = bytes -> (alias, did_path) :: acc | ||
| | _ -> acc) aliases [] | ||
| and warn_idl_conflict at bytes did_path = |
There was a problem hiding this comment.
Design: at parameter shadows the outer at needlessly.
The enclosing function already has at in scope from the (f, ri_ref, at) argument tuple. Since warn_idl_conflict is called exactly once, either capture at from the closure or inline the 4-line body — both remove the redundant parameter and the shadowing.
| Diag.add_msg msgs | ||
| (Diag.warning_message at "M0260" "import" | ||
| (Printf.sprintf | ||
| "IDL file for principal also found via --actor-idl; \ |
There was a problem hiding this comment.
Bug: M0260 suggestion is circular and uses the principal twice.
"Consider using --actor-id-alias %s %s %s directly."
(Url.encode_principal bytes) (Url.encode_principal bytes) did_pathThe first %s is meant for an alias name but receives the principal again. More importantly, the user already has --actor-id-alias configured — telling them to add it again is circular. Suggest instead:
"IDL file for this principal is also present under --actor-idl. Consider removing it from the --actor-idl directory to avoid confusion."
| add_idl_import msgs imported ri_ref at full_path (Either.Right bytes) | ||
| and find_principal_aliases bytes = | ||
| (* Collect all --actor-id-alias entries whose principal (2nd arg) matches bytes *) | ||
| M.fold (fun alias v acc -> match v with |
There was a problem hiding this comment.
Design: --actor-env-alias exclusion is silent and untested.
find_principal_aliases only matches Either.Right (b, Some did_path), so Either.Left entries from --actor-env-alias are silently skipped. That's probably correct (the principal isn't known at compile time for env-aliases), but a user who sets --actor-env-alias mgmt MY_ID /path.did and then writes import "ic:<principal>" will get the cryptic M0008. Worth at least a comment here, and a test documenting the expected (fall-through) behaviour.
| warn_idl_conflict at bytes did_path; | ||
| add_idl_import msgs imported ri_ref at (Lib.FilePath.normalise did_path) (Either.Right bytes) | ||
| | matches -> | ||
| let aliases_str = String.concat ", " (List.map fst matches) in |
There was a problem hiding this comment.
Bug: List.hd picks the last alphabetical alias, not the first.
M.fold visits keys in ascending alphabetical order. With :: acc (prepend), the list is in reverse order — List.hd matches returns the last alphabetically, not the first. The warning text "using the first one found" and the .ok file entry (mgmt1, mgmt2) are both inconsistent with what the code actually does.
Fix: sort or reverse before taking the head:
let matches = List.sort (fun (a,_) (b,_) -> String.compare a b) matches inThen update the .ok file accordingly.
| @@ -0,0 +1 @@ | |||
| (unknown location): warning [M0261], Multiple --actor-id-alias entries match principal aaaaa-aa (mgmt1, mgmt2); using the first one found. | |||
There was a problem hiding this comment.
Bug: (unknown location) will not match actual output.
at here is the source region of the import expression — a real file location like /tmp/…XXXXXX.mo:1.8-1.22. The test runner does not strip temp-file paths automatically.
Existing tests that emit locations (e.g. actor-env-alias-bad-utf8.sh) pipe through sed "s|$tmp|test.mo|". Add that to actor-id-alias-principal-ambiguous.sh and regenerate the .ok, which should look like:
test.mo:1.8-1.22: warning [M0261], Multiple --actor-id-alias entries match principal aaaaa-aa (mgmt1, mgmt2); using the first one found.
(Adjust the column numbers after running the test with -a to generate the .ok.)
| - `"mo:<package-name>/<path>"` then `<pat>` is bound to the library module defined in file `<package-path>/<path>.mo` in directory `<package-path>` referenced by package alias `<package-name>`. The mapping from `<package-name>` to `<package-path>` is determined by a compiler command-line argument `--package <package-name> <package-path>`. For example, `import L "mo:core/List"` defines `L` to reference the `List` library in package alias `core`. | ||
|
|
||
| - `"ic:<canisterid>"` then `<pat>` is bound to a Motoko actor whose Motoko type is determined by the canister’s IDL interface. The IDL interface of canister `<canisterid>` must be found in file `<actorpath>/<canisterid>.did`. The compiler assumes that `<actorpath>` is specified by command line argument `--actor-idl <actorpath>` and that file `<actorpath>/<canisterid>.did` exists. For example, `import C "ic:lg264-qjkae"` defines `C` to reference the actor with canister id `lg264-qjkae` and IDL file `lg264-qjkae.did`. | ||
| - `"ic:<canisterid>"` then `<pat>` is bound to a Motoko actor whose Motoko type is determined by the canister’s IDL interface. The preferred way to supply the IDL file is `--actor-id-alias <alias> <canisterid> <idl-file>`, where `<canisterid>` matches the imported principal; the compiler then uses `<idl-file>` directly without needing a search directory. Alternatively, the IDL file can be found via the legacy `--actor-idl <actorpath>` flag, which requires `<actorpath>/<canisterid>.did` to exist. For example, `import C "ic:lg264-qjkae"` defines `C` to reference the actor with canister id `lg264-qjkae`; supply its IDL with `--actor-id-alias mycanister lg264-qjkae lg264-qjkae.did` or with `--actor-idl <dir>` where `<dir>/lg264-qjkae.did` exists. |
There was a problem hiding this comment.
Nit: clarify that the alias name is irrelevant for ic: imports.
The example uses mycanister as the alias name, but since ic:lg264-qjkae is resolved by matching the principal (second argument), mycanister is never consulted. A reader will wonder if they need to use the canister id as the alias. Add a note:
Note: when resolving
ic:<principal>imports, only the principal (second argument) of--actor-id-aliasis matched; the alias name is not used and may be any identifier.
Code ReviewBugs1.
Fix: let matches = List.sort (fun (a,_) (b,_) -> String.compare a b) matches inThen update the 2.
3. M0260 suggestion is circular and uses the principal twice ( "Consider using --actor-id-alias %s %s %s directly."
(Url.encode_principal bytes) (Url.encode_principal bytes) did_pathThe first Design4.
5.
6.
Missing tests7. M0260 code path is never exercised
8.
Nits9. Add a comment explaining the M0255–M0259 gap in "M0254", None, "Initial actor requires field";
(* M0255-M0259 are error-only codes, see error_codes above *)
"M0260", None, "Actor-id-alias shadows --actor-idl lookup";10. Clarify that the alias name is irrelevant for The example uses |
Problem
When a Motoko file uses
ic:aaaaa-aa(a direct principal import),mocalways required--actor-idl <dir>pointing to a directory containingaaaaa-aa.did. The--actor-id-aliasflag — which accepts an explicit DID path — was only consulted forcanister:<alias>imports and had no effect onic:<principal>imports.This meant there was no way to specify a DID file for a specific principal without a directory, even though
--actor-id-aliasis specifically designed to bypass the--actor-idlsearch path.Fix
In
resolve_import_string(src/pipeline/resolve_import.ml), theUrl.Ic bytesarm now:--actor-id-aliasentries whose principal (2nd arg) matches the imported principal bytes.--actor-idldirectory lookup (no behaviour change).--actor-id-alias; additionally warns if--actor-idlalso contains a matching.didfile, suggesting the explicit form directly (M0260).New helper functions
find_principal_aliasesandwarn_idl_conflictare factored out alongsideresolve_icusinglet ... and ....Tests
Three new shell tests in
test/run/:actor-id-alias-by-principal.sh—ic:aaaaa-aaresolves with only--actor-id-alias, no--actor-idlactor-id-alias-principal-over-idl.sh—--actor-id-aliaswins over an empty--actor-idldiractor-id-alias-principal-ambiguous.sh— two aliases for the same principal emit the M0261 ambiguity warningNotes
src/lang_utils/error_codes.ml.design/DFX-Interface.mddescribes--actor-id-aliasas bypassing--actor-idl; this change extends that guarantee to directic:<principal>imports consistently.🤖 Generated with Claude Code