Skip to content

Conversation

@mark-koch
Copy link
Collaborator

@mark-koch mark-koch commented Jun 11, 2025

This is required as a precusor to #1018 to handle monomorphisation: When compiling to Hugr, some bound type variables will need to be replaced with a monotype. We will need access to the compiler context to look this information up.

Instead of requiring the whole compiler context, I added a new ToHugrContext protocol in which we can put everything that is needed during the Hugr translation of types (empty for now but will be filled in a subsequent PR). The main motivation for this is that the context needs to change when we go under a binder.

BREAKING CHANGE: All to_hugr methods on types, arguments, and parameters now require a ToHugrContext
BREAKING CHAGNE: CompileableDef.compile_outer now requires a ToHugrContext
BREAKING CHANGE: guppy.hugr_op now passes the compiler context to the function generating the op

@mark-koch mark-koch requested a review from a team as a code owner June 11, 2025 09:26
@mark-koch mark-koch requested a review from acl-cqc June 11, 2025 09:26
@codecov-commenter
Copy link

codecov-commenter commented Jun 11, 2025

Codecov Report

Attention: Patch coverage is 95.51282% with 7 lines in your changes missing coverage. Please review.

Project coverage is 92.32%. Comparing base (8dd0bc4) to head (7514917).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
guppylang/std/_internal/util.py 72.72% 3 Missing ⚠️
guppylang/compiler/core.py 85.71% 1 Missing ⚠️
guppylang/std/_internal/compiler/list.py 83.33% 1 Missing ⚠️
guppylang/tys/param.py 75.00% 1 Missing ⚠️
guppylang/tys/ty.py 95.83% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1022      +/-   ##
==========================================
- Coverage   92.33%   92.32%   -0.02%     
==========================================
  Files         103      103              
  Lines       10186    10250      +64     
==========================================
+ Hits         9405     9463      +58     
- Misses        781      787       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@mark-koch
Copy link
Collaborator Author

Actually, I'm not sure if this is really needed. @acl-cqc maybe hold off on reviewing this until I've thought a bit more about this

@mark-koch mark-koch marked this pull request as draft June 11, 2025 10:38
@mark-koch mark-koch marked this pull request as ready for review June 17, 2025 09:29
@mark-koch
Copy link
Collaborator Author

@acl-cqc This is ready for review now

@mark-koch mark-koch changed the title chore!: Require CompilerContext when converting types to Hugr chore!: Require a context when converting types to Hugr Jun 17, 2025
@mark-koch mark-koch changed the base branch from main to feat/mono June 17, 2025 09:40
@ss2165 ss2165 requested review from ss2165 and removed request for acl-cqc June 23, 2025 12:27
@mark-koch mark-koch merged commit 7a1f96c into feat/mono Jul 23, 2025
7 checks passed
@mark-koch mark-koch deleted the chore/to-hugr-ctx branch July 23, 2025 14:20
github-merge-queue bot pushed a commit that referenced this pull request Jul 24, 2025
Closes #1035. Tracked PRs:

* #1022
* #1034
* #1035

BREAKING CHANGE: All `to_hugr` methods on types, arguments, and
parameters now require a `ToHugrContext`
BREAKING CHANGE: `CompileableDef.compile_outer` now requires a
`ToHugrContext`
BREAKING CHANGE: `guppy.hugr_op` now passes the compiler context to the
function generating the op
BREAKING CHANGE: `CheckedFunctionDef` now implements
`MonomorphizableDef` instead of `CompileableDef`
BREAKING CHANGE: `CompilerContext.build_compiled_def` now requires an
instantiation for the definition's type parameters
BREAKING CHANGE: The `ToHugrContext` protocol now requires two
additional methods: `type_var_to_hugr` and `const_var_to_hugr`
BREAKING CHANGE: `CompilerContext.{compiled, worklist}` and
`CompilationEngine.compiled` are now indexed by a tuple of `DefId` and
optional `PartiallyMonomorphizedArgs`

---------

Co-authored-by: Alan Lawrence <[email protected]>
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.

4 participants