-
Notifications
You must be signed in to change notification settings - Fork 165
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
[enhancement:DSLX:type_system] Ensure parametric env map is completely populated on instantiation #1495
Comments
Previously we'd get an internal error by running head-first into the bytecode interpreter and it returning us an unbound error from its execution. This institutes a look-before-you-leap: we look for freevars from the parametric expression that are present in the parametric bindings, and check that those are present in the parametric environment map. If they are not, we provide a more precise and helpful error message. Ideally we'd be even more aggressive about reporting parametrics that are unbound at the point of instantiation, that is what google#1495 is for and why there's a bit of code stubbed out with a TODO in this PR -- there is some investigation needed for the examples that fail with that enabled, but this change strictly moves the ball forward by resolving the issue seen in google#1473. Fixes google#1473 One step towards google#1495
Previously we'd get an internal error by running head-first into the bytecode interpreter and it returning us an unbound error from its execution. This institutes a look-before-you-leap: we look for freevars from the parametric expression that are present in the parametric bindings, and check that those are present in the parametric environment map. If they are not, we provide a more precise and helpful error message. Ideally we'd be even more aggressive about reporting parametrics that are unbound at the point of instantiation, that is what google#1495 is for and why there's a bit of code stubbed out with a TODO in this PR -- there is some investigation needed for the examples that fail with that enabled, but this change strictly moves the ball forward by resolving the issue seen in google#1473. Fixes google#1473 One step towards google#1495 Add license header to .x file. git-clang-format
Previously we'd get an internal error by running head-first into the bytecode interpreter and it returning us an unbound error from its execution. This institutes a look-before-you-leap: we look for freevars from the parametric expression that are present in the parametric bindings, and check that those are present in the parametric environment map. If they are not, we provide a more precise and helpful error message. Ideally we'd be even more aggressive about reporting parametrics that are unbound at the point of instantiation, that is what google#1495 is for and why there's a bit of code stubbed out with a TODO in this PR -- there is some investigation needed for the examples that fail with that enabled, but this change strictly moves the ball forward by resolving the issue seen in google#1473. Fixes google#1473 One step towards google#1495
This might not be the point of your example (since the returned It doesn't currently do so, whereas it would (as |
For easy reproducibility of this issue, here are the commands to run to observe the current error hitting the snag while ir conversion: Code generationbazel build -c opt xls/dslx/ir_convert:ir_converter_main
IR_CONVERTER=bazel-bin/xls/dslx/ir_convert/ir_converter_main
${IR_CONVERTER} /tmp/foo.x Result
|
What's hard to do? (limit 100 words)
Right now we accept situations where the caller doesn't fully populate the parametric env map. For example in this program:
We could eagerly state that X was not populated in the parametric environment and flag that as an issue.
There is some fallout to address when we try to tighten up this invariant, which is why I'm filing an issue for it instead of fixing it directly in the related issue #1473
Current best alternative workaround (limit 100 words)
The current lenient policy is "ok" it just causes some failures at IR conversion time instead of eagerly presenting them at typechecking time as we would like.
Your view of the "best case XLS enhancement" (limit 100 words)
We are able to eagerly flag all parametric environment bindings that have not been populated when they are instantiated.
The text was updated successfully, but these errors were encountered: