Optimise comparison with singleton custom types on JavaScript #4903. … #4951

re-masashi · 2025-09-10T12:02:01Z

…also fixed some clippy errors

…ang#4903. also fixed some clippy errors

lpil

Code is looking good!

I think you've forgotten to commit the updated snapshots!

lpil · 2025-09-10T12:08:26Z

compiler-core/src/javascript/expression.rs

            return docvec![left_doc, operator, right_doc];
        }

+        // Optimise comparison with singleton custom types on JavaScript (https://gleam-lang/gleam/issues/4903)


Could you remove the link to the issue and write a detailed comment explaining what this code is doing please 🙏

Here's an example a good example of such a comment:

gleam/compiler-core/src/javascript/expression.rs

Lines 1045 to 1071 in 1a5ce62

/// In Gleam, the `&&` operator is short-circuiting, meaning that we can't

/// pre-evaluate both sides of it, and use them in the exception that is

/// thrown.

/// Instead, we need to implement this short-circuiting logic ourself.

///

/// If we short-circuit, we must leave the second expression unevaluated,

/// and signal that using the `unevaluated` variant, as detailed in the

/// exception format. For the first expression, we know it must be `false`,

/// otherwise we would have continued by evaluating the second expression.

///

/// Similarly, if we do evaluate the second expression and fail, we know

/// that the first expression must have evaluated to `true`, and the second

/// to `false`. This way, we avoid needing to evaluate either expression

/// twice.

///

/// The generated code then looks something like this:

/// ```javascript

/// if (expr1) {

/// if (!expr2) {

/// <throw exception>

/// }

/// } else {

/// <throw exception>

/// }

/// ```

///

fn assert_and(

lpil · 2025-09-10T12:09:29Z

compiler-core/src/javascript/expression.rs

+            } else {
+                return docvec!["!(", right_doc, " instanceof ", constructor_name, ")"];
+            }
+        }


Could we extract the duplicated bits to helper methods please 🙏

This hasn't been done yet! The patterns are still duplicated. Could we write something like this?:

if let Some(document) = self.zero_arity_variant_equality(left, right, should_be_equal) { return document; } if let Some(document) = self.zero_arity_variant_equality(right, left, should_be_equal) { return document; }

This is still outstanding 🙏

This still hasn't been done

Once again, this still needs to be done

compiler-core/src/javascript/tests/singleton_comparison_optimisation.rs

…ts for the new code

re-masashi · 2025-09-10T15:37:07Z

please review

lpil

Very nice! Thank you. I've left a few more comments inline.

lpil · 2025-09-10T17:25:12Z

.../tests/snapshots/gleam_core__javascript__tests__case_clause_guards__imported_aliased_ok.snap

  let $ = (var0) => { return new Y(var0); };
  let y = $;
-  if (isEqual(y, (var0) => { return new Y(var0); })) {
+  if (y instanceof Y) {


There's a bug here! This has gone from being an equality check against the constructor to being a check if y is any value produced by that constructor.

oh wait. yea. ill fix it.

lpil · 2025-09-10T17:25:53Z

compiler-core/src/javascript/expression.rs

        self.prelude_equal_call(should_be_equal, left, right)
    }

+    fn singleton_equal_helper(


Give this a descriptive name please 🙏

Looks like this still needs to be renamed

lpil · 2025-09-10T17:26:28Z

compiler-core/src/javascript/expression.rs

+            } else {
+                return docvec!["!(", right_doc, " instanceof ", constructor_name, ")"];
+            }
+        }


This hasn't been done yet! The patterns are still duplicated. Could we write something like this?:

if let Some(document) = self.zero_arity_variant_equality(left, right, should_be_equal) { return document; } if let Some(document) = self.zero_arity_variant_equality(right, left, should_be_equal) { return document; }

lpil · 2025-09-10T17:27:13Z

compiler-core/src/javascript/expression.rs

+                    ClauseGuard::Constant(Constant::Record {
+                        arguments, name, ..
+                    }),
+                    // Check if it's a singleton (no arguments)


No arguments doesn't mean a singleton, it could be use of the constructor as a value.

lpil · 2025-09-10T17:27:27Z

compiler-core/src/javascript/expression.rs


            ClauseGuard::Equals { left, right, .. } => {
+                if let (
+                    _,


This value isn't matched on, so no need to put it in a tuple with the other.

lpil · 2025-09-10T17:27:57Z

compiler-core/src/javascript/expression.rs

+        // which supports any shape of data, and so does a lot of extra logic which isn't necessary.
+
+        if let (
+            _,


This value isn't matched on, so no need to put it in a tuple with the other.

lpil · 2025-09-10T17:32:39Z

compiler-core/src/javascript/tests/custom_types.rs

+}
+"#,
+    );
+}


Could we get tests for these please:

The variant is defined in another module and used in a qualified fashion (module.Variant)

The variant is defined in another module and used in an unqualified fashion (import module.{Variant})

The variant is defined in another module and used in an unqualified fashion while aliased (import module.{Variant as OtherName})

Both in expressions and in case clauses please.

re-masashi · 2025-09-10T18:52:55Z

so, i havent been able to come up with a way to add the optimization in the guard clauses. im reverting to all guard clauses having == for now.

re-masashi · 2025-09-10T19:30:52Z

import other_module.{A as B}
pub fn func() {
  case B {
    x if x == B -> True
    _ -> False
  }
}

what should the generated code be?
if (isEqual(x, new B())) or if (x instanceof B)

GearsDatapacks · 2025-09-10T20:03:22Z

We want this optimisation to work for all equality checks, so instanceof

re-masashi · 2025-09-11T08:32:00Z

if something is aliased, for instance, Ok being aliased to Y,
then how will i know if Y is a singleton?

GearsDatapacks · 2025-09-11T11:41:11Z

As far as I can tell, it doesn't change the method of detection at all. Y is still a record with no fields

re-masashi · 2025-09-11T19:46:18Z

As far as I can tell, it doesn't change the method of detection at all. Y is still a record with no fields

but since Ok has a field, shouldnt Y do too?

what should this compile to?

import gleam.{Ok as Y}
pub type X {
  Ok
}
pub fn func() {
  case Y {
    y if y == Y -> True
    _ -> False
  }
}

this?

  let $ = (var0) => { return new Y(var0); };
  let y = $;
  if (y instanceof Y) {
    return true;
  } else {
    return false;
  }

GearsDatapacks · 2025-09-11T20:07:55Z

Nope, Y is not a singleton here.

re-masashi · 2025-09-14T02:38:05Z

im really sorry for asking this but what makes something a singleton? having 0 arity? or what else?

re-masashi · 2025-09-14T03:09:38Z

okay ive figured it out. i just have to clean the code now

GearsDatapacks · 2025-09-14T07:24:02Z

Yes, a singleton as I referred to it in the original issue, though perhaps not the correct term, is a variant of a custom type with no fields, aka 0 arity.

…ode up

re-masashi · 2025-09-15T07:14:17Z

please review

GearsDatapacks

A couple notes

GearsDatapacks · 2025-09-15T20:29:48Z

compiler-core/src/javascript/expression.rs

        self.prelude_equal_call(should_be_equal, left, right)
    }

+    fn singleton_equal_helper(


Looks like this still needs to be renamed

GearsDatapacks · 2025-09-15T20:33:17Z

compiler-core/src/javascript/expression.rs

+                    if is_var(left)
+                        && !looks_like_constructor_alias(left)
+                        && let Some(tag) = ctor_tag(right)
+                    {


I don't really understand why this uses different logic from the expression comparison. Could you change singleton_equal_helper to take a Document instead of a TypedExpr, and reuse that code here?

if it is a constructor alias, then we get a false positive.
the result causes code which returns true or false for all values, but in reality, it should not.

normal expression comparison does not need alias checking like for guards.
in guards, the variable can be an alias for the constructor value itself.

What's a constructor alias? I haven't heard that term used before

not sure if that's the exact term.
but i'm referring to any variable that's an alias for a constructor.
for example

let pluh = Ok

so pluh is a "constructor alias". makes ample sense imo.

Not sure what a constructor alias is here, but I agree the logic here is a bit hard to understand.

I'd like to see a similar pattern as I suggested here: https://github.com/gleam-lang/gleam/pull/4951/files#r2337440251

The logic I think should be:

The pattern is for a variant/variant constructor.

No arguments have been supplied.

The type of the pattern is a Named type. This could be checked with a new type.is_named() method, similar to the existing type.is_result method.

okay. ill do it.

I still think this code is too complex. Here are my thoughts:

I'm not sure why the left-hand-side has to be a variable for this check to take place. You haven't imposed this limitation for regular expression comparison, so it seems odd that it would be required in guards

I also don't know why you have checked that both sides are not constructor values. Again, this was not done for expression comparison so I don't see why it would be different here

You've explained what a constructor alias is, but not why it needs to be checked for here. Could you give a code example where this check is needed?

It is also probably a good idea to do what is suggested for the expressions and make one function for checking whether a comparison is eligible for this optimisation, calling it twice with the different orderings

GearsDatapacks · 2025-09-15T20:34:35Z

compiler-core/src/javascript/expression.rs

+        ClauseGuard::Var { type_, .. } => matches!(&**type_, Type::Fn { .. }), // lambda
+        _ => false,
+    }
+}


It's not really clear what these functions do. The names don't seem to properly describe their behaviour. Although, if you follow the comment above, these functions might not be necessary.

yeah. it seems like i wont need these functions if i use Document instead of TypedExpr.

ill do the needful

I agree, I don't think these functions belong here. I think they would be best being inlined into the places they are used 🙏

re-masashi · 2025-09-19T10:24:52Z

ive made the necessary changes

GearsDatapacks

Looks like a few changes still need to be made

GearsDatapacks · 2025-09-19T10:47:37Z

compiler-core/src/javascript/expression.rs

-                self.prelude_equal_call(false, left, right)
+                let l = self.guard(left);
+                let r = self.guard(right);
+                self.prelude_equal_call(want_equal, l, r)


Please do not rename these variables. l and r are less descriptive names than left and right and there is no reason to change them.

GearsDatapacks · 2025-09-19T10:58:18Z

compiler-core/src/javascript/expression.rs

+            } else {
+                return docvec!["!(", right_doc, " instanceof ", constructor_name, ")"];
+            }
+        }


This still hasn't been done

GearsDatapacks · 2025-09-19T11:16:32Z

compiler-core/src/javascript/expression.rs

+                    if is_var(left)
+                        && !looks_like_constructor_alias(left)
+                        && let Some(tag) = ctor_tag(right)
+                    {


I still think this code is too complex. Here are my thoughts:

I'm not sure why the left-hand-side has to be a variable for this check to take place. You haven't imposed this limitation for regular expression comparison, so it seems odd that it would be required in guards

I also don't know why you have checked that both sides are not constructor values. Again, this was not done for expression comparison so I don't see why it would be different here

You've explained what a constructor alias is, but not why it needs to be checked for here. Could you give a code example where this check is needed?

It is also probably a good idea to do what is suggested for the expressions and make one function for checking whether a comparison is eligible for this optimisation, calling it twice with the different orderings

re-masashi · 2025-09-20T14:53:39Z

I'm not sure why the left-hand-side has to be a variable for this check to take place. You haven't imposed this limitation for regular expression comparison, so it seems odd that it would be required in guards

For normal expression context, either of the sides can be any expression, so the optimization triggers for both.

I also don't know why you have checked that both sides are not constructor values. Again, this was not done for expression comparison so I don't see why it would be different here

The main intent was to avoid redundant optimizations in cases like Active == Active.
but uhh, if the eligibility function does the right type/arity checks, this explicit double check may be unnecessary, as is the case for expressions. ig removing it makes the logic consistent between guards and expressions.

You've explained what a constructor alias is, but not why it needs to be checked for here. Could you give a code example where this check is needed?

“Constructor alias” cases happen when a variable is pattern-bound to the constructor value itself.

for example,

import gleam.{Ok as Y}
pub type X {
  Ok
}
pub fn func() {
  case Y {
    y if y == Y -> True
    _ -> False
  }
}

so y == Y should not optimize to instanceof; otherwise, every instance would match.

It is also probably a good idea to do what is suggested for the expressions and make one function for checking whether a comparison is eligible for this optimisation, calling it twice with the different orderings

okay, we can refactor it i guess. ill do that

GearsDatapacks · 2025-09-20T15:41:16Z

I don't see why Wibble == Wibble shouldn't be optimised. There's no point in the code anyway, and new Wibble() instanceof Wibble works fine. It would simplify the code greatly and there are no drawbacks

re-masashi · 2025-09-22T19:00:47Z

i've made the necessary changes. please check

GearsDatapacks

Still some changes that need to be made

GearsDatapacks · 2025-09-22T21:12:41Z

compiler-core/src/javascript/expression.rs

+            } else {
+                return docvec!["!(", right_doc, " instanceof ", constructor_name, ")"];
+            }
+        }


Once again, this still needs to be done

GearsDatapacks · 2025-09-22T21:14:14Z

compiler-core/src/javascript/expression.rs

+                fn eligible_singleton_cmp(
+                    lhs: &TypedClauseGuard,
+                    rhs: &TypedClauseGuard,
+                ) -> Option<EcoString> {


You haven't really simplified this logic, just refactored it so the code looks slightly different. Could you change it to use the same logic as regular expression comparison please

lpil · 2025-09-23T11:05:12Z

Thank you both!

I think if you focus on that early comment on mine that hasn't been done yet then it'll become clearer how to do the guard side too. https://github.com/gleam-lang/gleam/pull/4951/files#r2336546527

re-masashi · 2025-09-24T11:22:10Z

Thank you both!

I think if you focus on that early comment on mine that hasn't been done yet then it'll become clearer how to do the guard side too. https://github.com/gleam-lang/gleam/pull/4951/files#r2336546527

i extracted the redundant parts into helper methods. please lmk if i need to do anything else

lpil

Hello! Nice, nearly there!

Could you please use the design I requested in my previous comments 🙏 https://github.com/gleam-lang/gleam/pull/4951/files#r2337440251
So instead of having one call to a handle_singleton_equality that duplicates the code for left and right, instead of having one function and calling it twice in the equal method, and the same pattern used for guards too.

The tests with Variant Other(...) look great! Could you also add versions of these tests that check equality against Other, to ensure that the optimisation isn't applied when it shouldn't be. Thank you

You'll also need to rebase on main to fix the tests

re-masashi · 2025-09-26T15:50:34Z

okay! ill do the needful. thanks a lot

re-masashi · 2025-09-26T15:58:36Z

by the way, if i keep the logic the same for the clause guard and normal expressions, then the alias cases get a false positive.

   12    12 │ 
   13    13 │ 
   14    14 │ ----- COMPILED JAVASCRIPT
   15    15 │ import * as $gleam from "../gleam.mjs";
   16       │-import { Ok as Y, CustomType as $CustomType, isEqual } from "../gleam.mjs";
         16 │+import { Ok as Y, CustomType as $CustomType } from "../gleam.mjs";
   17    17 │ 
   18    18 │ export class Ok extends $CustomType {}
   19    19 │ 
   20    20 │ export function func() {
   21    21 │   let $ = (var0) => { return new Y(var0); };
   22    22 │   let y = $;
   23       │-  if (isEqual(y, (var0) => { return new Y(var0); })) {
         23 │+  if (y instanceof Y) {
   24    24 │     return true;
   25    25 │   } else {
   26    26 │     return false;
   27    27 │   }

GearsDatapacks · 2025-09-26T16:41:13Z

You must be doing the check differently, because Y in that case is not a constructor with 0 arity

re-masashi · 2025-09-26T17:47:52Z

You must be doing the check differently, because Y in that case is not a constructor with 0 arity

yeah it's an alias. i need to check for that

re-masashi · 2025-09-26T18:56:48Z

im getting some new "helper" functions in the generated code. i did not change any part of the code which should trigger something like that.

   12    12 │ ----- COMPILED JAVASCRIPT
   13    13 │ import { CustomType as $CustomType } from "../gleam.mjs";
   14    14 │ 
   15    15 │ export class Wibble extends $CustomType {}
         16 │+export const Wibble$Wibble = () => new Wibble();
         17 │+export const Wibble$isWibble = (value) => value instanceof Wibble;
   16    18 │ 
   17    19 │ export class Wobble extends $CustomType {}
         20 │+export const Wibble$Wobble = () => new Wobble();
         21 │+export const Wibble$isWobble = (value) => value instanceof Wobble;
   18    22 │ 
   19    23 │ export function is_not_wibble(w) {
   20    24 │   return !(w instanceof Wibble);
   21    25 │ }

   17    17 │     super();
   18    18 │     this.value = value;
   19    19 │   }
   20    20 │ }
         21 │+export const Result$Ok = (value) => new Ok(value);
         22 │+export const Result$isOk = (value) => value instanceof Ok;
         23 │+export const Result$Ok$value = (value) => value.value;
         24 │+export const Result$Ok$0 = (value) => value.value;
   21    25 │ 
   22    26 │ export class Error extends $CustomType {}
         27 │+export const Result$Error = () => new Error();
         28 │+export const Result$isError = (value) => value instanceof Error;
   23    29 │ 
   24    30 │ export function is_error(r) {
   25    31 │   return r instanceof Error;
   26    32 │ }

re-masashi · 2025-09-26T18:58:10Z

re-masashi@377f9fa

is it because of this commit?

GearsDatapacks · 2025-09-26T19:14:01Z

Yes, this is a change from main which your branch inherited after you rebased. These changes are correct

re-masashi · 2025-09-26T19:16:08Z

okay
ill commit the new changes then

re-masashi · 2025-09-27T08:50:39Z

i hope everything is okay this time. please check

lpil · 2025-09-27T15:10:15Z

The tests with Variant Other(...) look great! Could you also add versions of these tests that check equality against Other, to ensure that the optimisation isn't applied when it shouldn't be. Thank you

These test are missing 🙏

re-masashi · 2025-09-27T19:28:50Z

The tests with Variant Other(...) look great! Could you also add versions of these tests that check equality against Other, to ensure that the optimisation isn't applied when it shouldn't be. Thank you

These test are missing 🙏

OH! il add them rn.
also, i forgot that to format AFTER running clippy. ill fix it rn

… trigger where they shouldnt)

re-masashi · 2025-09-29T09:37:20Z

can someone please review this :(

re-masashi · 2025-10-02T08:01:34Z

do I need to add or remove or change something?

GearsDatapacks

Looking good! A couple small notes

GearsDatapacks · 2025-10-02T08:25:44Z

compiler-core/src/javascript/expression.rs

+        }
+    }
+
+    fn singleton_equal_helper(


Can you give this a more descriptive name please?

GearsDatapacks · 2025-10-02T08:30:32Z

compiler-core/src/javascript/expression.rs

+            && arguments.is_empty()
+            && right.type_().is_named()
+            && let ClauseGuard::Var { type_, .. } = left
+            && !matches!(&**type_, Type::Fn { .. })


Can we use the same logic as the expression equality here, where we check the ValueContructor to see if it has zero arity? I think it would make this easier to read and understand

Optimise comparison with singleton custom types on JavaScript gleam-l…

1a5ce62

…ang#4903. also fixed some clippy errors

re-masashi mentioned this pull request Sep 10, 2025

Optimise comparison with singleton custom types on JavaScript #4903

Open

lpil reviewed Sep 10, 2025

View reviewed changes

compiler-core/src/javascript/tests/singleton_comparison_optimisation.rs Outdated Show resolved Hide resolved

implemented optimizations for guard clauses as well. added proper tes…

c0bc2f3

…ts for the new code

lpil reviewed Sep 10, 2025

View reviewed changes

PLUH

1a7329b

finalized optimization for all cases. added more tests. cleaned the c…

bcd37c2

…ode up

GearsDatapacks reviewed Sep 15, 2025

View reviewed changes

re-masashi added 2 commits September 16, 2025 22:50

refactor

e93197e

refactor

0f67076

GearsDatapacks reviewed Sep 19, 2025

View reviewed changes

refactor

3f65f99

GearsDatapacks reviewed Sep 22, 2025

View reviewed changes

refactor

4f0046e

refactor. addded helper methods for singleton equality codegen

ba10e88

lpil reviewed Sep 26, 2025

View reviewed changes

Merge branch 'gleam-lang:main' into main

eb37f27

refactor

95c73b4

added tests to ensure there are no false positives (optmizations dont…

3d76e01

… trigger where they shouldnt)

GearsDatapacks reviewed Oct 2, 2025

View reviewed changes

refactor

a05544e

	/// In Gleam, the `&&` operator is short-circuiting, meaning that we can't
	/// pre-evaluate both sides of it, and use them in the exception that is
	/// thrown.
	/// Instead, we need to implement this short-circuiting logic ourself.
	///
	/// If we short-circuit, we must leave the second expression unevaluated,
	/// and signal that using the `unevaluated` variant, as detailed in the
	/// exception format. For the first expression, we know it must be `false`,
	/// otherwise we would have continued by evaluating the second expression.
	///
	/// Similarly, if we do evaluate the second expression and fail, we know
	/// that the first expression must have evaluated to `true`, and the second
	/// to `false`. This way, we avoid needing to evaluate either expression
	/// twice.
	///
	/// The generated code then looks something like this:
	/// ```javascript
	/// if (expr1) {
	/// if (!expr2) {
	/// <throw exception>
	/// }
	/// } else {
	/// <throw exception>
	/// }
	/// ```
	///
	fn assert_and(

Uh oh!

Optimise comparison with singleton custom types on JavaScript #4903. … #4951

Are you sure you want to change the base?

Optimise comparison with singleton custom types on JavaScript #4903. … #4951

Conversation

re-masashi commented Sep 10, 2025

Uh oh!

lpil left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

re-masashi commented Sep 10, 2025

Uh oh!

lpil left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GearsDatapacks Sep 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

re-masashi commented Sep 10, 2025

Uh oh!

re-masashi commented Sep 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GearsDatapacks commented Sep 10, 2025

Uh oh!

re-masashi commented Sep 11, 2025

Uh oh!

GearsDatapacks commented Sep 11, 2025

Uh oh!

re-masashi commented Sep 11, 2025

Uh oh!

GearsDatapacks commented Sep 11, 2025

Uh oh!

re-masashi commented Sep 14, 2025

Uh oh!

re-masashi commented Sep 14, 2025

Uh oh!

GearsDatapacks commented Sep 14, 2025

Uh oh!

re-masashi commented Sep 15, 2025

Uh oh!

GearsDatapacks left a comment

Choose a reason for hiding this comment

Uh oh!

GearsDatapacks Sep 15, 2025 •

edited

Loading

re-masashi commented Sep 10, 2025 •

edited

Loading

GearsDatapacks Sep 15, 2025 •

edited

Loading

GearsDatapacks Sep 15, 2025 •

edited

Loading