Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: suggest conversions between primitive types #4747

Merged
merged 27 commits into from
Nov 11, 2024

Conversation

crusso
Copy link
Contributor

@crusso crusso commented Oct 28, 2024

This PR exploits bidirectional type checking to suggest missing conversions between inferred and expected types.

When the inferred type of an expression does not match the expected type, and both types are primitive, we search
the package environment for suitable direct conversion functions, identified by a to/from prefix and their function type.

Example messages (expect shorted ones in real world usage):

suggest-conversion-ai.mo:7.3-7.5: type error [M0096], expression of type
  Nat8
cannot produce expected type
  Nat
Maybe try conversion:
  `Nat8.toNat(_)`,
  `Nat8.fromNat8(_)`,
  `Nat8.Outer.Inner.toNat(_)` or
  `Nat8.Outer.Inner.fromNat8(_)`?
suggest-conversion-ai.mo:8.3-8.6: type error [M0096], expression of type
  Nat16
cannot produce expected type
  Nat
Maybe try conversion:
  `Nat16.toNat(_)` after adding `import Nat16 = "mo:conv/Nat16"`,
  `Nat16.fromNat16(_)` after adding `import Nat16 = "mo:conv/Nat16"`,
  `Nat16.Outer.Inner.toNat(_)` after adding `import Nat16 = "mo:conv/Nat16"` or
  `Nat16.Outer.Inner.fromNat16(_)` after adding `import Nat16 = "mo:conv/Nat16"`?

By default, we only typecheck and search explicitly imported libraries.

With --ai-errors enabled, the search considers all available libraries available from any included package, even if not explicitly imported, by including all libraries as an implicit import during library resolution. This also allows moc to suggest conversions that are available, but not imported, by suggesting the appropriate library import.

An unfortunate side-effect of this approach is that an implicitly imported library containing incorrect code will cause compilation to fail, but this shouldn't be problem for well-formed packages. It will also increase compilation times since all package libraries will be type-checked, even if not imported. It should not affect code size, since unimported libraries are not linked for compilation.

As part of the PR, I also disable the emissions of warning and hints arising from package libraries. These are particularly annoying for non-imported libraries, but were always annoying even for imported ones (since packages are typically third-party, there is not much users can do to address any warning).

The work here might also provide better support for future VSCode code completion, since it forces the environment to contain all available libraries with their types, not just the imported ones.

For now, we only search for direct conversions. One could use modified Floyd-Warshall or Dijkstra shortest paths algorithm to find the shorted indirect conversions between any two primitive types, but I haven't tried that.

@crusso crusso changed the base branch from claudio/suggest-conversion to master October 28, 2024 17:22
@crusso crusso changed the base branch from master to claudio/suggest-conversion October 28, 2024 17:25
@crusso crusso marked this pull request as ready for review October 30, 2024 21:57
@crusso crusso changed the base branch from claudio/suggest-conversion to master October 30, 2024 21:57
test/fail/conv/Nat16.mo Outdated Show resolved Hide resolved
test/fail/conv/Nat8.mo Outdated Show resolved Hide resolved
Copy link

github-actions bot commented Oct 30, 2024

Comparing from e220418 to fb24a32:
The produced WebAssembly code seems to be completely unchanged.

@crusso crusso changed the title Claudio/suggest conversion extended feat: suggest conversions between primitive types Oct 31, 2024
src/mo_frontend/suggest.ml Outdated Show resolved Hide resolved
@chenyan-dfinity
Copy link
Contributor

How about lossy conversions, e.g., Int -> Nat? A common pattern I see from AI is that they compute a Int number and use that as array index, with if conditions that guarantee that Int is positive.

@crusso
Copy link
Contributor Author

crusso commented Nov 1, 2024

Member

The suggestion don't distinguish between partial/lossy and total conversions, but won't pick up Int.abs either since it doesn't follow to/from pattern. Probably the right thing to do is to add (to base) trapping Int.toNat/Nat.fromInt and maybe even Int.fromNat and Nat.toInt with a warning that it's redundant due to subtyping.

@crusso crusso added the automerge-squash When ready, merge (using squash) label Nov 8, 2024
Copy link
Contributor

@chenyan-dfinity chenyan-dfinity left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mergify mergify bot merged commit 703b494 into master Nov 11, 2024
10 checks passed
@mergify mergify bot removed the automerge-squash When ready, merge (using squash) label Nov 11, 2024
@mergify mergify bot deleted the claudio/suggest-conversion-extended branch November 11, 2024 17:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants