-
Notifications
You must be signed in to change notification settings - Fork 471
Fix option optimisation compiler bug #7766
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
Conversation
Co-authored-by: Antonio Nuno Monteiro <[email protected]>
rescript
@rescript/darwin-arm64
@rescript/darwin-x64
@rescript/linux-arm64
@rescript/linux-x64
@rescript/win32-x64
commit: |
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.
Great stuff! Thanks to all involved.
Thanks a lot @mediremi ! |
And thanks @anmonteiro for the original fix! |
(Normal_optional | ||
( Lprim {loc = _; primitive = Psome_not_nest; args = [Lvar id']} | ||
| Lvar id' )) -> | ||
id_is_for_sure_true_in_boolean tbl id' |
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 this case actively used in the fix?
Wondering how to make sure there's no infinite loop, whether such an invariant for tables is guaranteed to hold.
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.
Without this case the boolean example function is not compiled correctly.
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.
Then I don't understand the logic. It should be an optimization so omitting a case should not turn on more optimization and get a wrong compilation.
I guess the default case should be false not true.
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.
While Normal_optional below, the catch-all case, returns true.
I think it should return false by default, and under certain conditions return true.
So, removing this line should turn off a possibly desired optimization, instead of triggering an incorrect optimization.
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've made the default case Eval_unknown
(no optimisation), and we no longer have a potentially infinite loop 👍
This fixes the compiler bug reported by @jfrolich on Discord:
was being compiled to
The fix is from this Melange PR by @anmonteiro: melange-re/melange#1530 (specifically, commit melange-re/melange@eab306d)