-
Notifications
You must be signed in to change notification settings - Fork 152
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
Fix eta-expansion in evaluator #2782
Conversation
I can confirm that this also fixes the original non-minimized version of the problem. @christiaanb do you still want to keep this as a draft PR? |
I'll add the test case and make it ready for review. |
tests/shouldwork/Issues/T2781.hs
Outdated
fullMeshSwCcTest sysClk = spiDone | ||
where | ||
(spiDone | ||
, ugnsStable -- :: Vec 1 (Signal System Bool) -- this signature causes "Evaluator.instantiate: Not a tylambda: Lambda" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better remove this comment to avoid confusion.
Originally the bug was triggered by the addition of this type signature, hence the comment.
But this curious property was lost somewhere during minimization of the test case, in the current form it always triggered the bug.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll remove the comment.
|
||
go _ supply (Left tv) = (supply, Right tv) | ||
go names supply (Right ty) = | ||
let ((supply1, _), n) = mkUniqSystemId (supply, names) ("x", ty) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it safe to just ignore the updated InscopeSet
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it's not. The new binders should not shadow each other. I'll fix this.
For some eta-reduced 'e', we used to bogusly eta-expand to: \x.(\y. e y) x We now correctly expand to: \x.\y.(e x) y Fixes #2781
For some eta-reduced 'e', we used to bogusly eta-expand to:
We now correctly expand to:
Fixes #2781
Still TODO: