-
-
Notifications
You must be signed in to change notification settings - Fork 873
Optimise comparison with singleton custom types on JavaScript #4903. … #4951
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
base: main
Are you sure you want to change the base?
Changes from 2 commits
1a5ce62
c0bc2f3
1a7329b
bcd37c2
e93197e
0f67076
3f65f99
4f0046e
ba10e88
eb37f27
95c73b4
3d76e01
a05544e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1536,14 +1536,80 @@ impl<'module, 'a> Generator<'module, 'a> { | |
return docvec![left_doc, operator, right_doc]; | ||
} | ||
|
||
// For comparison with singleton custom types, ie, one with no fields. | ||
// If you have some code like this | ||
// ```gleam | ||
// pub type Wibble { | ||
// Wibble | ||
// Wobble | ||
// } | ||
|
||
// pub fn is_wibble(w: Wibble) -> Bool { | ||
// w == Wibble | ||
// } | ||
// ``` | ||
// Instead of `isEqual(w, new Wibble())`, generate `w instanceof Wibble` | ||
// because the first approach needs to construct a new Wibble, and then call the isEqual function, | ||
// which supports any shape of data, and so does a lot of extra logic which isn't necessary. | ||
|
||
if let ( | ||
_, | ||
// RHS: singleton record constructor (arity 0) | ||
TypedExpr::Var { | ||
constructor: | ||
ValueConstructor { | ||
variant: ValueConstructorVariant::Record { arity: 0, name, .. }, | ||
.. | ||
}, | ||
.. | ||
}, | ||
) = (left, right) | ||
{ | ||
return self.singleton_equal_helper(left, name, should_be_equal); | ||
} | ||
|
||
if let ( | ||
// LHS: singleton record constructor (arity 0) | ||
TypedExpr::Var { | ||
constructor: | ||
ValueConstructor { | ||
variant: ValueConstructorVariant::Record { arity: 0, name, .. }, | ||
.. | ||
}, | ||
.. | ||
}, | ||
_, | ||
) = (left, right) | ||
{ | ||
return self.singleton_equal_helper(right, name, should_be_equal); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we extract the duplicated bits to helper methods please 🙏 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. surely There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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;
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is still outstanding 🙏 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This still hasn't been done There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Once again, this still needs to be done |
||
|
||
// Other types must be compared using structural equality | ||
let left = | ||
self.not_in_tail_position(Some(Ordering::Strict), |this| this.wrap_expression(left)); | ||
let right = | ||
self.not_in_tail_position(Some(Ordering::Strict), |this| this.wrap_expression(right)); | ||
|
||
self.prelude_equal_call(should_be_equal, left, right) | ||
} | ||
|
||
fn singleton_equal_helper( | ||
|
||
&mut self, | ||
expr: &'a TypedExpr, | ||
name: &EcoString, | ||
should_be_equal: bool, | ||
) -> Document<'a> { | ||
let expr_doc = | ||
self.not_in_tail_position(Some(Ordering::Strict), |this| this.wrap_expression(expr)); | ||
let constructor_name = name.to_doc(); | ||
|
||
if should_be_equal { | ||
docvec![expr_doc, " instanceof ", constructor_name] | ||
} else { | ||
docvec!["!(", expr_doc, " instanceof ", constructor_name, ")"] | ||
} | ||
} | ||
|
||
fn equal_with_doc_operands( | ||
&mut self, | ||
left: Document<'a>, | ||
|
@@ -1967,12 +2033,62 @@ impl<'module, 'a> Generator<'module, 'a> { | |
} | ||
|
||
ClauseGuard::Equals { left, right, .. } => { | ||
if let ( | ||
_, | ||
|
||
ClauseGuard::Constant(Constant::Record { | ||
arguments, name, .. | ||
}), | ||
// Check if it's a singleton (no arguments) | ||
|
||
) = (&**left, &**right) | ||
&& arguments.is_empty() | ||
{ | ||
let left_doc = self.guard(left); | ||
return docvec![left_doc, " instanceof ", name.to_doc()]; | ||
} | ||
|
||
if let ( | ||
ClauseGuard::Constant(Constant::Record { | ||
arguments, name, .. | ||
}), | ||
_, | ||
// Check if it's a singleton (no arguments) | ||
) = (&**left, &**right) | ||
&& arguments.is_empty() | ||
{ | ||
let right_doc = self.guard(right); | ||
return docvec![right_doc, " instanceof ", name.to_doc()]; | ||
} | ||
|
||
let left = self.guard(left); | ||
let right = self.guard(right); | ||
self.prelude_equal_call(true, left, right) | ||
} | ||
|
||
ClauseGuard::NotEquals { left, right, .. } => { | ||
if let ( | ||
_, | ||
ClauseGuard::Constant(Constant::Record { | ||
arguments, name, .. | ||
}), | ||
) = (&**left, &**right) | ||
&& arguments.is_empty() | ||
{ | ||
let left_doc = self.guard(left); | ||
return docvec!["!(", left_doc, " instanceof ", name.to_doc(), ")"]; | ||
} | ||
|
||
if let ( | ||
ClauseGuard::Constant(Constant::Record { | ||
arguments, name, .. | ||
}), | ||
_, | ||
) = (&**left, &**right) | ||
&& arguments.is_empty() | ||
{ | ||
let right_doc = self.guard(right); | ||
return docvec!["!(", right_doc, " instanceof ", name.to_doc(), ")"]; | ||
} | ||
|
||
let left = self.guard(left); | ||
let right = self.guard(right); | ||
self.prelude_equal_call(false, left, right) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -687,3 +687,122 @@ pub type Wibble { | |
"# | ||
); | ||
} | ||
|
||
#[test] | ||
fn singleton_record_equality() { | ||
assert_js!( | ||
r#" | ||
pub type Wibble { | ||
Wibble | ||
Wobble | ||
} | ||
|
||
pub fn is_wibble(w: Wibble) -> Bool { | ||
w == Wibble | ||
} | ||
"#, | ||
); | ||
} | ||
|
||
#[test] | ||
fn singleton_record_inequality() { | ||
assert_js!( | ||
r#" | ||
pub type Wibble { | ||
Wibble | ||
Wobble | ||
} | ||
|
||
pub fn is_not_wibble(w: Wibble) -> Bool { | ||
w != Wibble | ||
} | ||
"#, | ||
); | ||
} | ||
|
||
#[test] | ||
fn singleton_record_reverse_order() { | ||
assert_js!( | ||
r#" | ||
pub type Wibble { | ||
Wibble | ||
Wobble | ||
} | ||
|
||
pub fn is_wibble_reverse(w: Wibble) -> Bool { | ||
Wibble == w | ||
} | ||
"#, | ||
); | ||
} | ||
|
||
#[test] | ||
fn non_singleton_record_equality() { | ||
assert_js!( | ||
r#" | ||
pub type Person { | ||
Person(name: String, age: Int) | ||
} | ||
|
||
pub fn same_person(p1: Person, p2: Person) -> Bool { | ||
p1 == p2 | ||
} | ||
"#, | ||
); | ||
} | ||
|
||
#[test] | ||
fn multiple_singleton_constructors() { | ||
assert_js!( | ||
r#" | ||
pub type Status { | ||
Loading | ||
Success | ||
Error | ||
} | ||
|
||
pub fn is_loading(s: Status) -> Bool { | ||
s == Loading | ||
} | ||
|
||
pub fn is_success(s: Status) -> Bool { | ||
s == Success | ||
} | ||
"#, | ||
); | ||
} | ||
|
||
#[test] | ||
fn mixed_singleton_and_non_singleton() { | ||
assert_js!( | ||
r#" | ||
pub type Result { | ||
Ok(value: Int) | ||
Error | ||
} | ||
|
||
pub fn is_error(r: Result) -> Bool { | ||
r == Error | ||
} | ||
"#, | ||
); | ||
} | ||
|
||
#[test] | ||
fn singleton_in_case_guard() { | ||
assert_js!( | ||
r#" | ||
pub type State { | ||
Active | ||
Inactive | ||
} | ||
|
||
pub fn process(s: State) -> String { | ||
case s { | ||
state if state == Active -> "active" | ||
_ -> "inactive" | ||
} | ||
} | ||
"#, | ||
); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we get tests for these please:
Both in expressions and in case clauses please. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,8 +1,6 @@ | ||
--- | ||
source: compiler-core/src/javascript/tests/case_clause_guards.rs | ||
assertion_line: 489 | ||
expression: "import gleam.{Ok as Y}\npub type X {\n Ok\n}\npub fn func() {\n case Y {\n y if y == Y -> True\n _ -> False\n }\n}\n" | ||
snapshot_kind: text | ||
--- | ||
----- SOURCE CODE | ||
import gleam.{Ok as Y} | ||
|
@@ -19,14 +17,14 @@ pub fn func() { | |
|
||
----- COMPILED JAVASCRIPT | ||
import * as $gleam from "../gleam.mjs"; | ||
import { Ok as Y, CustomType as $CustomType, isEqual } from "../gleam.mjs"; | ||
import { Ok as Y, CustomType as $CustomType } from "../gleam.mjs"; | ||
|
||
export class Ok extends $CustomType {} | ||
|
||
export function func() { | ||
let $ = (var0) => { return new Y(var0); }; | ||
let y = $; | ||
if (isEqual(y, (var0) => { return new Y(var0); })) { | ||
if (y instanceof Y) { | ||
|
||
return true; | ||
} else { | ||
return false; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
--- | ||
source: compiler-core/src/javascript/tests/custom_types.rs | ||
expression: "\npub type Result {\n Ok(value: Int)\n Error\n}\n\npub fn is_error(r: Result) -> Bool {\n r == Error\n}\n" | ||
--- | ||
----- SOURCE CODE | ||
|
||
pub type Result { | ||
Ok(value: Int) | ||
Error | ||
} | ||
|
||
pub fn is_error(r: Result) -> Bool { | ||
r == Error | ||
} | ||
|
||
|
||
----- COMPILED JAVASCRIPT | ||
import { CustomType as $CustomType } from "../gleam.mjs"; | ||
|
||
export class Ok extends $CustomType { | ||
constructor(value) { | ||
super(); | ||
this.value = value; | ||
} | ||
} | ||
|
||
export class Error extends $CustomType {} | ||
|
||
export function is_error(r) { | ||
return r instanceof Error; | ||
} |
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.
This value isn't matched on, so no need to put it in a tuple with the other.