Skip to content

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Aug 13, 2025
Merged

Fix option optimisation compiler bug #7766

merged 4 commits into from
Aug 13, 2025

Conversation

mediremi
Copy link
Contributor

This fixes the compiler bug reported by @jfrolich on Discord:

let foo = (~val1: bool, ~val2: bool) => {
  let a = Some(val1)
  let b = Some(val2)

  switch (a, b) {
  | (_, Some(true))
  | (Some(true), _) => "a"
  | _ => "b"
  }
}

was being compiled to

function foo(val1, val2) {
  return "a";
}

The fix is from this Melange PR by @anmonteiro: melange-re/melange#1530 (specifically, commit melange-re/melange@eab306d)

@mediremi mediremi changed the title Fix option optimisation Fix option optimisation compiler bug Aug 12, 2025
@mediremi mediremi marked this pull request as ready for review August 12, 2025 17:26
@pkg-pr-new pkg.pr.new
Copy link

pkg-pr-new bot commented Aug 12, 2025 •
edited
Loading

Open in StackBlitz

rescript

npm i https://pkg.pr.new/rescript-lang/rescript@7766

@rescript/darwin-arm64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/darwin-arm64@7766

@rescript/darwin-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/darwin-x64@7766

@rescript/linux-arm64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/linux-arm64@7766

@rescript/linux-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/linux-x64@7766

@rescript/win32-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/win32-x64@7766

commit: f203622

@zth zth requested review from tsnobip, zth and cristianoc August 13, 2025 06:25
zth
zth approved these changes Aug 13, 2025
View reviewed changes
Copy link
Collaborator

@zth zth left a 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.

@tsnobip
Copy link
Member

tsnobip commented Aug 13, 2025

Thanks a lot @mediremi !

@tsnobip
Copy link
Member

tsnobip commented Aug 13, 2025

And thanks @anmonteiro for the original fix!

cristianoc
cristianoc reviewed Aug 13, 2025
View reviewed changes
compiler/core/lam_pass_remove_alias.ml Outdated
(Normal_optional
( Lprim {loc = _; primitive = Psome_not_nest; args = [Lvar id']}
| Lvar id' )) ->
id_is_for_sure_true_in_boolean tbl id'
Copy link
Collaborator

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.

Copy link
Contributor Author

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.

Copy link
Collaborator

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.

Copy link
Collaborator

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.

Copy link
Contributor Author

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 👍

@zth zth merged commit e92879e into rescript-lang:master Aug 13, 2025
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zth zth zth approved these changes

@tsnobip tsnobip tsnobip approved these changes

@cristianoc cristianoc cristianoc approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mediremi @tsnobip @zth @cristianoc