Skip to content

Regression in pattern matching optional fields. #7440

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

Merged
merged 1 commit into from
May 8, 2025

Conversation

cristianoc
Copy link
Collaborator

Fixes #7400

Fix regression in pattern matching for optional fields.

There's an optimisation where {field: VariantCase} is compiled to field:VariantCase instead of field:Some(VariantCase) to avoid generating code that handles undefined, since VariantCase is going to be a string. However, that optimisation happens early in the pattern matching compiler, and can end up interfering with other cases. For example, when VariantCase is the only case in a variant, and the following case is _, the compiler is induced in thinking that the variant case covers all cases, and the case for _ is never generated. Similar behaviour happens when handling the last case of a variant followed by _.

Fixes #7400

Fix regression in pattern  matching for optional fields.

There's an optimisation where `{field: VariantCase}` is compiled to `field:VariantCase` instead of `field:Some(VariantCase)` to avoid generating code that handles undefined, since `VariantCase` is going to be a string.
However, that optimisation happens early in the pattern matching compiler, and can end up interfering with other cases. For example, when `VariantCase` is the only case in a variant, and the following case is `_`, the compiler is induced in thinking that the variant case covers all cases, and the case for `_` is never generated.
Similar behaviour happens when handling the last case of a variant followed by `_`.
@cristianoc cristianoc force-pushed the pm-opt-fields-regression branch from 3c49718 to 295652e Compare May 8, 2025 08:12
@@ -25,7 +25,11 @@ function decodeGroup(group) {
}

function decodeNull(x) {
let tmp = x.field;
let match = x.field;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has changed because dicts always add optional fields, and in this case it contains a json value, and one of the possible json values is undefined.

@cristianoc cristianoc requested review from cknitt and zth May 8, 2025 08:14
Copy link

pkg-pr-new bot commented May 8, 2025

Open in StackBlitz

rescript

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

@rescript/darwin-arm64

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

@rescript/darwin-x64

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

@rescript/linux-arm64

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

@rescript/linux-x64

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

@rescript/win32-x64

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

commit: 295652e

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 work!

@cristianoc cristianoc merged commit 244cdad into master May 8, 2025
21 checks passed
@cristianoc cristianoc deleted the pm-opt-fields-regression branch May 8, 2025 08:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ReScript v12 - pattern matching on optional fields breaks other cases
2 participants