Make applyDebug's free variable check take the context into account #2624
Conversation
| -- Since [Give evaluator acces to inscope let-bindings #2571](https://github.com/clash-lang/clash-compiler/pull/2571) | ||
| -- the evaluator can rewrite expressions using let bindings from the 'TransformContext', | ||
| -- these bindings may reference other things bound in the context which weren't | ||
| -- in the expression before, and in doing so introduces new free variables and | ||
| -- fails this check for new free variables. | ||
| -- To prevent this we filter out all variables from bound in the context. | ||
| -- But only during a caseCon transformation, to not weaken this check unnecessarily. |
There was a problem hiding this comment.
| -- Since [Give evaluator acces to inscope let-bindings #2571](https://github.com/clash-lang/clash-compiler/pull/2571) | |
| -- the evaluator can rewrite expressions using let bindings from the 'TransformContext', | |
| -- these bindings may reference other things bound in the context which weren't | |
| -- in the expression before, and in doing so introduces new free variables and | |
| -- fails this check for new free variables. | |
| -- To prevent this we filter out all variables from bound in the context. | |
| -- But only during a caseCon transformation, to not weaken this check unnecessarily. | |
| -- Since [Give evaluator acces to inscope let-bindings #2571](https://github.com/clash-lang/clash-compiler/pull/2571) | |
| -- the evaluator can rewrite expressions using let bindings from the 'TransformContext'. | |
| -- These bindings may reference other things bound in the context which weren't | |
| -- in the expression before, and in doing so introduces new free variables. Consequently, | |
| -- this fails a check that prevents new free variables from being introduced - | |
| -- as this is a bug in all other cases. To prevent this we filter out all | |
| -- variables from bound in the context. We only do this during a 'caseCon' transformation, | |
| -- to not weaken this check unnecessarily. |
I've reformatted / broke up sentences to make it easier to read.
I'm not sure about this though:
We only do this during a 'caseCon' transformation,
to not weaken this check unnecessarily.
Is this because the evaluator is only called in caseCon? In any case, I think we should defend why we think we only need to do it for caseCon in the note.
There was a problem hiding this comment.
@christiaanb maybe you could comment on this?
There was a problem hiding this comment.
The evaluator is called in multiple places, including caseCon. But as @leonschoorl points out, the "introduce free variables" check is quite useful to catch bugs in the Clash compiler, so we want to leave it enabled in as many places as possible. So it's only disabled in caseCon as we have a confirmed "false positive" for that transformation.
There was a problem hiding this comment.
Okay, this sounds like: we're aware this is an issue, and we're going to wait until another transformation calling the evaluator is going to blow up..
What we should do is check the other transformations calling the evaluator, check whether they suffer from the same issue, and add a comment explaining why we do (not) need a similar approach as introduced in this PR.
martijnbastiaan
force-pushed
the
fix-FV-check
branch
from
56eda6b to
633c7aa
Compare
…2624) (#2681) Fixes #2623 (cherry picked from commit 0aa341a) Co-authored-by: Leon Schoorl <[email protected]>
Fixes #2623
Still TODO: