feat!: Monomorphise everything during type checking #1441
feat!: Monomorphise everything during type checking #1441
Conversation
|
This PR contains breaking changes to the public Python API. Breaking changes summary |
Bencher Report
| state = get_tracing_state() | ||
| # Only capture errors that are tagged with the span of the current | ||
| # tracing node | ||
| if err.error.span is None or to_span(state.node) == to_span(err.error.span): |
There was a problem hiding this comment.
This is slightly annoying:
Before this PR, we ensured that all functions called by comptime functions are checked before we enter capture_guppy_errors. Thus, errors in regular guppy functions would be reported using the normal mechanism.
Now, we can only check those called functions while we're tracing (since we only know the monomorphic instantiation at that point) so their errors would be intercepted here. This guard is a hacky solution to avoid this.
The proper solution would be to do comptime tracing during checking, instead of during Hugr construction, but that's a bigger refactor
There was a problem hiding this comment.
First thing to note is that the method doc is wrong - it mentions "GuppyComptimeException" and such does not exist!
IIUC the issue here is to distinguish between errors checking the guppy generated directly by the comptime block, and those in newly-created instantations of non-comptime methods?
Are you sure that the refactor to do comptime tracing during checking, isn't something we should do first? I think it might help with a few other questions/concerns here and I suspect it'll be a requirement for signature metaprogramming (where, in some sense, many more functions are processed in a way that's like tracing - perhaps all, perhaps not).
Failing that, is there an intermediate solution - can you make guppy-comptime enqueue the instantiations that it needs, and then do "another round" of checking (emptying the queue) after comptime?
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1441 +/- ##
==========================================
- Coverage 93.54% 93.34% -0.20%
==========================================
Files 127 127
Lines 11796 11703 -93
==========================================
- Hits 11034 10924 -110
- Misses 762 779 +17 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| 10 | @guppy | ||
| 11 | def main(n: nat @ comptime) -> None: | ||
| 11 | def bar(n: nat @ comptime) -> None: | ||
| 12 | foo[n](42) | ||
| | ^^ Expected constant `n`, got `42` | ||
| | ^^ Expected constant `43`, got `42` |
There was a problem hiding this comment.
These errors are worse now, classic problem with C++ style template debugging :/
| Note: Parameter `tag` was instantiated to `aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | ||
| aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | ||
| aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | ||
| aaaa` | ||
|
|
There was a problem hiding this comment.
Shame that we also no longer have this
Maybe, we want this as a more general hint to tell us what parameters were instantiated to?
There was a problem hiding this comment.
Yes, can we not ("just"!!) include the current monomorphization as part of the context of the error? (I guess it would be potentially a chain of instantiations, which might be hard to identify given the worklist, but even just the nearest-enclosing instantiation would be something)
acl-cqc
left a comment
acl-cqc
left a comment
There was a problem hiding this comment.
Thanks Mark! A lot to think about here... a few more bits you can cleanup/remove, some thoughts on typechecking, some possible further refactors.
I see that uninstantiated functions do not produce errors (even if they contain e.g. undeclared variables), simply because uncalled functions do not...that does feel a bit sneaky/surprising (I mean it's kinda like python "errors delayed until runtime" but we would often use mypy to detect them ahead of time and guppy takes the role of mypy...)
OTOH it's good to see that a function that's invalid for all type arguments, produces the error only for one set of type arguments....although I think that is just because we only issue one error before we stop....
However this does not produce an error:
T = guppy.type_var("T")
@guppy
def baz(x: T, y: T) -> int:
return x + y
@guppy
def main(x: int, y: int) -> None:
baz(x, y)
main.compile_function()
see comment on engine.py but I'd be in favour of doing that even for unreached functions, really!
| inputs: Row[Variable], | ||
| return_ty: Type, | ||
| generic_params: dict[str, Parameter], | ||
| generic_inst: dict[str, Argument], |
There was a problem hiding this comment.
Super-nit but I preferred generic_args - clearer that it's just the arguments, whereas an "instantiation" might be just that, or (say) both the arguments and the (parametric/polymorphic) thing being instantiated
| inputs: Row[Variable], | ||
| return_ty: Type, | ||
| generic_params: dict[str, Parameter], | ||
| generic_args: dict[str, Argument], |
There was a problem hiding this comment.
Might be worth documenting/commenting that this function also creates a new instantiation
| f_locals: dict[str, Any] | ||
| f_globals: dict[str, Any] | ||
| f_builtins: dict[str, Any] | ||
| def_id: DefId | None |
There was a problem hiding this comment.
I think we could clarify the doc comment: "Wrapper around one Definition in the DEF_STORE" or something like that. (Can it really be "in a stack frame"? In a function scope, perhaps - I would have thought that stack frames were dynamic, runtime, objects?)
| @@ -463,7 +466,7 @@ def _check_global( | |||
| case ParamDef(): | |||
| # Check if this parameter is in our generic_params | |||
There was a problem hiding this comment.
How would a ParamDef have gotten into globals if it isn't in scope?
| # Check if this parameter is in our generic_params | ||
| # (e.g., used in type signature) | ||
| if name in self.ctx.generic_params: | ||
| if name in self.ctx.generic_param_inst: |
There was a problem hiding this comment.
So I'll just note that there are some checks that are independent of instantiation, that could be performed once on the parsed/parametric non-instantiated function. This is one such. So....if I declare a polymorphic function, but don't instantiate it, and it contains type errors (irrespective of the "values"==types of those type parameters)...presumably we will now have no error?
| def check_valid_entry_point(defn: ParsedDef) -> MonoArgs: | ||
| """Checks if the given definition is a valid compilation entry-point. | ||
|
|
||
| In particular, ensures that the definition doesn't depend on generic parameters and |
There was a problem hiding this comment.
super-nit: the "and returns...." should be in the first sentence....or, and perhaps better, the () could just go in the caller
| state = get_tracing_state() | ||
| # Only capture errors that are tagged with the span of the current | ||
| # tracing node | ||
| if err.error.span is None or to_span(state.node) == to_span(err.error.span): |
There was a problem hiding this comment.
First thing to note is that the method doc is wrong - it mentions "GuppyComptimeException" and such does not exist!
IIUC the issue here is to distinguish between errors checking the guppy generated directly by the comptime block, and those in newly-created instantations of non-comptime methods?
Are you sure that the refactor to do comptime tracing during checking, isn't something we should do first? I think it might help with a few other questions/concerns here and I suspect it'll be a requirement for signature metaprogramming (where, in some sense, many more functions are processed in a way that's like tracing - perhaps all, perhaps not).
Failing that, is there an intermediate solution - can you make guppy-comptime enqueue the instantiations that it needs, and then do "another round" of checking (emptying the queue) after comptime?
| param_var_mapping: dict[str, Parameter] = field(default_factory=dict) | ||
|
|
||
| #: Type parameters that are bound to concrete arguments | ||
| param_inst: Mapping[str, Argument] = field(default_factory=dict) |
There was a problem hiding this comment.
This is what I was saying you should call generic_args earlier - but param_inst could be good too - and/or maybe a new type: it's just like Inst except keyed by string rather than int
| 10 | @guppy | ||
| 11 | def main(n: nat @ comptime) -> None: | ||
| 11 | def bar(n: nat @ comptime) -> None: | ||
| 12 | foo[n](42) | ||
| | ^^ Expected constant `n`, got `42` | ||
| | ^^ Expected constant `43`, got `42` |
|
|
||
| @guppy | ||
| def main(n: nat @ comptime) -> None: | ||
| def bar(n: nat @ comptime) -> None: |
There was a problem hiding this comment.
Yeah, it's not even obvious to me that foo should take two arguments (one static/"type argument", one "runtime"/value argument)
…the CompilerContext
mark-koch
force-pushed
the
feat/early-mono
branch
from
279c775 to
45e4c05
Compare
4 participants
This PR updates to compiler to monomorphize every generic definition. The resulting Hugr will contain no more generic functions.
Monomorphization happens during type checking, so every version is type checked separately.
This is a precursor for #1299.
Recommend reviewing commit by commit
BREAKING CHANGE: Removed
MonomorphizableDef,MonomorphizedDefPartiallyMonomorphizedArgs, partially_monomorphize_args. UseGenericCheckableDefandMonoArgsinstead.BREAKING CHANGE:
Context.generic_paramsrenamed toContext.generic_inst, now storing an instantiation instead of parametersBREAKING CHANGE: Removed
NoneType.preserveandTupleType.preservefields