Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@
- Improve output of record copying. https://github.com/rescript-lang/rescript-compiler/pull/7043
- Provide additional context in error message when `unit` is expected. https://github.com/rescript-lang/rescript-compiler/pull/7045
- Improve error message when passing an object where a record is expected. https://github.com/rescript-lang/rescript-compiler/pull/7101
- Improve code generation or pattern matching of untagged variants. https://github.com/rescript-lang/rescript-compiler/pull/7128

#### :house: Internal

Expand Down
43 changes: 36 additions & 7 deletions compiler/ml/ast_untagged_variants.ml
Original file line number Diff line number Diff line change
Expand Up @@ -372,6 +372,14 @@ module DynamicChecks = struct
| Not of 'a t
| Expr of 'a

let rec size = function
| BinOp (_, x, y) -> 1 + size x + size y
| TagType _ -> 1
| TypeOf x -> 1 + size x
| IsInstanceOf (_, x) -> 1 + size x
| Not x -> 1 + size x
| Expr _ -> 1

let bin op x y = BinOp (op, x, y)
let tag_type t = TagType t
let typeof x = TypeOf x
Expand All @@ -396,7 +404,7 @@ module DynamicChecks = struct
let ( &&& ) x y = bin And x y

let rec is_a_literal_case ~(literal_cases : tag_type list) ~block_cases
(e : _ t) =
~list_literal_cases (e : _ t) =
let literals_overlaps_with_string () =
Ext_list.exists literal_cases (function
| String _ -> true
Expand Down Expand Up @@ -458,12 +466,33 @@ module DynamicChecks = struct
Ext_list.fold_right others is_literal_1 (fun literal_n acc ->
is_literal_case literal_n ||| acc))
in
match block_cases with
| [c] -> is_not_block_case c
| c1 :: (_ :: _ as rest) ->
is_not_block_case c1
&&& is_a_literal_case ~literal_cases ~block_cases:rest e
| [] -> assert false
if list_literal_cases then
let rec mk cases =
match cases with
| [case] -> is_literal_case case
| case :: rest -> is_literal_case case ||| mk rest
| [] -> assert false
in
mk literal_cases
else
match block_cases with
| [c] -> is_not_block_case c
| c1 :: (_ :: _ as rest) ->
is_not_block_case c1
&&& is_a_literal_case ~literal_cases ~block_cases:rest
~list_literal_cases e
| [] -> assert false

let is_a_literal_case ~literal_cases ~block_cases e =
let with_literal_cases =
is_a_literal_case ~literal_cases ~block_cases ~list_literal_cases:true e
in
let without_literal_cases =
is_a_literal_case ~literal_cases ~block_cases ~list_literal_cases:false e
in
if size with_literal_cases <= size without_literal_cases then
with_literal_cases
else without_literal_cases

let is_int_tag ?(has_null_undefined_other = (false, false, false)) (e : _ t) :
_ t =
Expand Down
46 changes: 23 additions & 23 deletions tests/tests/src/UntaggedVariants.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import * as Primitive_array from "rescript/lib/es6/Primitive_array.js";
import * as Primitive_option from "rescript/lib/es6/Primitive_option.js";

function classify(x) {
if (x === "A" && typeof x !== "number") {
if (x === "A") {
return "A";
} else if (typeof x === "number") {
return "An integer";
Expand Down Expand Up @@ -40,13 +40,13 @@ let ListWithTuples = {};
let ListWithObjects = {};

function tuplesToObjects(l) {
if (Array.isArray(l)) {
if (l === undefined) {
return null;
} else {
return {
hd: l[0],
tl: tuplesToObjects(l[1])
};
} else {
return null;
}
}

Expand All @@ -68,7 +68,7 @@ console.log("l1", l1);
console.log("l2", l2);

function isTrue(x) {
if (typeof x !== "object") {
if (x === true) {
return true;
} else {
return x.flag;
Expand All @@ -80,7 +80,7 @@ let Truthy = {
};

function classify$1(x) {
if (x === null || typeof x !== "object") {
if (x === undefined || x === null) {
if (x === null) {
return "null";
} else {
Expand All @@ -96,7 +96,7 @@ let TwoObjects = {
};

function classify$2(x) {
if (x === "A" || x === "B") {
if (x === "B" || x === "A") {
if (x === "A") {
return "a";
} else {
Expand All @@ -112,7 +112,7 @@ let Unknown = {
};

function classify$3(x) {
if (typeof x !== "object" && typeof x !== "number" && (x === "C" || x === "B" || x === "A" || x === "D")) {
if (x === "D" || x === "C" || x === "B" || x === "A") {
switch (x) {
case "A" :
return "a";
Expand Down Expand Up @@ -173,7 +173,7 @@ let WithArray = {
};

function classify$6(x) {
if (!Array.isArray(x) && (x === null || typeof x !== "object") && typeof x !== "number" && typeof x !== "string") {
if (x === null || x === true || x === false) {
switch (x) {
case false :
return "JSONFalse";
Expand Down Expand Up @@ -214,18 +214,18 @@ let Json = {
};

function check(s, y) {
if (!Array.isArray(s)) {
if (s === "B") {
return 42;
}
let x = s[0];
if (!Array.isArray(x)) {
if (x === "B") {
return 42;
}
let tmp = s[1];
if (Array.isArray(tmp) || x === y) {
return 42;
} else {
if (tmp === "B" && x !== y) {
return 41;
} else {
return 42;
}
}

Expand All @@ -234,7 +234,7 @@ let TrickyNested = {
};

function checkEnum(e) {
if (!(e === "Two" || e === "One" || e === "Three")) {
if (!(e === "Three" || e === "Two" || e === "One")) {
return "Something else..." + e;
}
switch (e) {
Expand All @@ -252,7 +252,7 @@ let OverlapString = {
};

function checkEnum$1(e) {
if (!(e === "Two" || e === 1.0 || e === "Three")) {
if (!(e === "Three" || e === "Two" || e === 1.0)) {
return "Something else...";
}
switch (e) {
Expand Down Expand Up @@ -376,7 +376,7 @@ let TestFunctionCase = {
let someJson = '[{"name": "Haan"}, {"name": "Mr"}, false]';

function check$1(s) {
if (!Array.isArray(s) && (s === null || typeof s !== "object") && typeof s !== "number" && typeof s !== "string") {
if (s === null || s === true || s === false || s === undefined) {
console.log("Nope...");
return;
}
Expand All @@ -386,11 +386,11 @@ function check$1(s) {
return;
}
let match = s[0];
if (match === true) {
if ((match === null || match === true || match === false || match === undefined) && match === true) {
Copy link
Member

Choose a reason for hiding this comment

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

@cristianoc Thanks a lot for looking into this so quickly!
Most cases look better now. But this one still feels a bit weird as the lhs of the && is obviously unnecessary.
Is it possible to improve it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes and it's not related to this PR.
You're asking for a back end code simplification.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, then I think this PR is ready to go and we can track the above case in a different issue. 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

with_literal_cases:

(e == Null) || (e ==true) || (e == false) || (e == Undefined)

without_literal_cases:

!(e instanceof Array) && ((e == Null) || (typeof e != Object)) && (typeof e != Int) && (typeof e != String)

So it is true that it generates the smaller expression (the former).
The second one must be simplified in the back-end, and not the first one.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've added extra simplification directly to this PR.

let match$1 = s[1];
if (match$1 === false) {
if ((match$1 === null || match$1 === true || match$1 === false || match$1 === undefined) && match$1 === false) {
let match$2 = s[2];
if (!Array.isArray(match$2) && (match$2 === null || typeof match$2 !== "object") && typeof match$2 !== "number" && typeof match$2 !== "string") {
if (match$2 === null || match$2 === true || match$2 === false || match$2 === undefined) {
console.log("Nope...");
return;
}
Expand All @@ -400,13 +400,13 @@ function check$1(s) {
return;
}
let match$3 = match$2[0];
if (!Array.isArray(match$3) && (match$3 === null || typeof match$3 !== "object") && typeof match$3 !== "number" && typeof match$3 !== "string") {
if (match$3 === null || match$3 === true || match$3 === false || match$3 === undefined) {
console.log("Nope...");
return;
}
if (typeof match$3 === "string" && match$3 === "My name is") {
let match$4 = match$2[1];
if (!Array.isArray(match$4) && (match$4 === null || typeof match$4 !== "object") && typeof match$4 !== "number" && typeof match$4 !== "string") {
if (match$4 === null || match$4 === true || match$4 === false || match$4 === undefined) {
console.log("Nope...");
return;
}
Expand Down Expand Up @@ -476,7 +476,7 @@ let PromiseSync = {
};

async function classify$10(a) {
if (typeof a !== "object" && !(a instanceof Promise) && (a === "test" || a === 12) && !Array.isArray(a)) {
if (a === 12 || a === "test") {
if (a === "test") {
console.log("testing");
return;
Expand Down
10 changes: 5 additions & 5 deletions tests/tests/src/core/Core_JsonTests.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -5,19 +5,19 @@ import * as Test from "./Test.mjs";
function decodeJsonTest() {
let json = {"someProp":{"otherProp": null, "thirdProp": [true, false]}};
let decodedCorrectly;
if (!Array.isArray(json) && (json === null || typeof json !== "object") && typeof json !== "number" && typeof json !== "string" && typeof json !== "boolean" || !(typeof json === "object" && !Array.isArray(json))) {
if (json === null || !(typeof json === "object" && !Array.isArray(json))) {
decodedCorrectly = false;
} else {
let match = json["someProp"];
if (match !== undefined && !(!Array.isArray(match) && (match === null || typeof match !== "object") && typeof match !== "number" && typeof match !== "string" && typeof match !== "boolean" || !(typeof match === "object" && !Array.isArray(match)))) {
if (match !== undefined && !(match === null || !(typeof match === "object" && !Array.isArray(match)))) {
let match$1 = match["thirdProp"];
if (match$1 !== undefined && !(!Array.isArray(match$1) && (match$1 === null || typeof match$1 !== "object") && typeof match$1 !== "number" && typeof match$1 !== "string" && typeof match$1 !== "boolean" || !(Array.isArray(match$1) && match$1.length === 2))) {
if (match$1 !== undefined && !(match$1 === null || !(Array.isArray(match$1) && match$1.length === 2))) {
let match$2 = match$1[0];
if (!Array.isArray(match$2) && (match$2 === null || typeof match$2 !== "object") && typeof match$2 !== "number" && typeof match$2 !== "string" && typeof match$2 !== "boolean" || !(typeof match$2 === "boolean" && match$2)) {
if (match$2 === null || !(typeof match$2 === "boolean" && match$2)) {
decodedCorrectly = false;
} else {
let match$3 = match$1[1];
decodedCorrectly = !Array.isArray(match$3) && (match$3 === null || typeof match$3 !== "object") && typeof match$3 !== "number" && typeof match$3 !== "string" && typeof match$3 !== "boolean" || !(typeof match$3 === "boolean" && !match$3) ? false : true;
decodedCorrectly = match$3 === null || !(typeof match$3 === "boolean" && !match$3) ? false : true;
}
} else {
decodedCorrectly = false;
Expand Down
6 changes: 3 additions & 3 deletions tests/tests/src/core/Core_NullableTests.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ function shouldHandleNullableValues() {
let tUndefined = undefined;
let tValue = "hello";
let tmp;
tmp = (tNull === null || tNull === undefined) && tNull === null ? true : false;
tmp = (tNull === undefined || tNull === null) && tNull === null ? true : false;
Test.run([
[
"Core_NullableTests.res",
Expand All @@ -18,7 +18,7 @@ function shouldHandleNullableValues() {
"Should handle null"
], tmp, (prim0, prim1) => prim0 === prim1, true);
let tmp$1;
tmp$1 = (tUndefined === null || tUndefined === undefined) && tUndefined !== null ? true : false;
tmp$1 = (tUndefined === undefined || tUndefined === null) && tUndefined !== null ? true : false;
Test.run([
[
"Core_NullableTests.res",
Expand All @@ -29,7 +29,7 @@ function shouldHandleNullableValues() {
"Should handle undefined"
], tmp$1, (prim0, prim1) => prim0 === prim1, true);
let tmp$2;
tmp$2 = tValue === null || tValue === undefined || tValue !== "hello" ? false : true;
tmp$2 = tValue === undefined || tValue === null || tValue !== "hello" ? false : true;
Test.run([
[
"Core_NullableTests.res",
Expand Down
22 changes: 22 additions & 0 deletions tests/tests/src/untagged_variants_heuristic.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
// Generated by ReScript, PLEASE EDIT WITH CARE


function f(x) {
if (x !== null) {
return;
}
console.log("abc");
}

function ff(x) {
if (typeof x !== "number") {
return;
}
console.log("abc");
}

export {
f,
ff,
}
/* No side effect */
22 changes: 22 additions & 0 deletions tests/tests/src/untagged_variants_heuristic.res
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
let f = (x: JSON.t) =>
switch x {
| Null => Console.log("abc")
| _ => ()
}

@unboxed
type t =
| A
| B
| C
| D
| E
| F
| G
| Int(int)

let ff = (x: t) =>
switch x {
| Int(_) => Console.log("abc")
| _ => ()
}
14 changes: 7 additions & 7 deletions tests/tests/src/variantsMatching.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -224,17 +224,17 @@ function isUndefined$1(x) {
}

function plus$2(x, y) {
if (x === null || x === undefined) {
if (x === undefined || x === null) {
return y;
} else if (y === null || y === undefined) {
} else if (y === undefined || y === null) {
return x;
} else {
return x + y | 0;
}
}

function kind(x) {
if (x === null || x === undefined) {
if (x === undefined || x === null) {
if (x === null) {
return "null";
} else {
Expand Down Expand Up @@ -274,21 +274,21 @@ function isWhyNot(x) {
}

function plus$3(x, y) {
if (x === undefined || x === null || x === "WhyNotAnotherOne") {
if (x === "WhyNotAnotherOne" || x === undefined || x === null) {
switch (x) {
case null :
case undefined :
return y;
case "WhyNotAnotherOne" :
break;
}
} else if (!(y === undefined || y === null || y === "WhyNotAnotherOne")) {
} else if (!(y === "WhyNotAnotherOne" || y === undefined || y === null)) {
return {
x: x.x + y.x,
y: x.y + y.y
};
}
if (!(y === undefined || y === null || y === "WhyNotAnotherOne")) {
if (!(y === "WhyNotAnotherOne" || y === undefined || y === null)) {
return "WhyNotAnotherOne";
}
switch (y) {
Expand All @@ -301,7 +301,7 @@ function plus$3(x, y) {
}

function kind$1(x) {
if (!(x === undefined || x === null || x === "WhyNotAnotherOne")) {
if (!(x === "WhyNotAnotherOne" || x === undefined || x === null)) {
return "present";
}
switch (x) {
Expand Down
Loading