xls icon indicating copy to clipboard operation
xls copied to clipboard

[enhancement:DSLX:type_system] Ensure parametric env map is completely populated on instantiation

Open cdleary opened this issue 1 year ago • 2 comments

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:

fn p<X: u32, Y: u32>(y: uN[Y]) -> u32 { Y }
fn f() -> u32 { p(u7:0) }

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.

cdleary avatar Jun 19 '24 16:06 cdleary

This might not be the point of your example (since the returned Y could be replaced by X), but would it make sense to flag fn p<X: u32, Y: u32>(y: uN[Y]) -> u32 { Y } for X being unused?

It doesn't currently do so, whereas it would (as kUnusedDefinition) if the variable were defined in the function body.

mikex-oss avatar Jun 20 '24 17:06 mikex-oss

For easy reproducibility of this issue, here are the commands to run to observe the current error hitting the snag while ir conversion:

Code generation

bazel 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

Error: INTERNAL: Not enough symbolic bindings to convert function: p; need {X, Y} got {Y}

hzeller avatar Jun 24 '24 18:06 hzeller