-
-
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 7 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,76 @@ 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. | ||
|
||
// RHS: singleton record constructor (arity 0) | ||
if let TypedExpr::Var { | ||
constructor: | ||
ValueConstructor { | ||
variant: ValueConstructorVariant::Record { arity: 0, name, .. }, | ||
.. | ||
}, | ||
.. | ||
} = right | ||
{ | ||
let left = self | ||
.not_in_tail_position(Some(Ordering::Strict), |this| this.wrap_expression(left)); | ||
|
||
return self.singleton_equal(left, name, should_be_equal); | ||
} | ||
|
||
// LHS: singleton record constructor (arity 0) | ||
if let TypedExpr::Var { | ||
constructor: | ||
ValueConstructor { | ||
variant: ValueConstructorVariant::Record { arity: 0, name, .. }, | ||
.. | ||
}, | ||
.. | ||
} = left | ||
{ | ||
let right = self | ||
.not_in_tail_position(Some(Ordering::Strict), |this| this.wrap_expression(right)); | ||
|
||
return self.singleton_equal(right, name, should_be_equal); | ||
} | ||
|
||
// 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( | ||
&self, | ||
value: Document<'a>, | ||
tag: &EcoString, | ||
should_be_equal: bool, | ||
) -> Document<'a> { | ||
if should_be_equal { | ||
docvec![value, " instanceof ", tag.to_doc()] | ||
} else { | ||
docvec!["!(", value, " instanceof ", tag.to_doc(), ")"] | ||
} | ||
} | ||
|
||
fn equal_with_doc_operands( | ||
&mut self, | ||
left: Document<'a>, | ||
|
@@ -1966,16 +2028,42 @@ impl<'module, 'a> Generator<'module, 'a> { | |
docvec![left, " !== ", right] | ||
} | ||
|
||
ClauseGuard::Equals { left, right, .. } => { | ||
let left = self.guard(left); | ||
let right = self.guard(right); | ||
self.prelude_equal_call(true, left, right) | ||
} | ||
ClauseGuard::Equals { left, right, .. } | ||
| ClauseGuard::NotEquals { left, right, .. } => { | ||
fn eligible_singleton_cmp( | ||
lhs: &TypedClauseGuard, | ||
rhs: &TypedClauseGuard, | ||
) -> Option<EcoString> { | ||
|
||
match rhs { | ||
ClauseGuard::Constant(Constant::Record { | ||
arguments, name, .. | ||
}) if arguments.is_empty() && rhs.type_().is_named() => { | ||
// exclude the `Type::Fn` case | ||
if !matches!(lhs, | ||
ClauseGuard::Var { type_, .. } if matches!(&**type_, Type::Fn { .. })) | ||
{ | ||
return Some(name.clone()); | ||
} | ||
} | ||
_ => {} | ||
} | ||
None | ||
} | ||
|
||
let should_be_equal = matches!(guard, ClauseGuard::Equals { .. }); | ||
|
||
if let Some(tag) = eligible_singleton_cmp(left, right) { | ||
let doc = self.guard(left); | ||
return self.singleton_equal(doc, &tag, should_be_equal); | ||
} | ||
if let Some(tag) = eligible_singleton_cmp(right, left) { | ||
let doc = self.guard(right); | ||
return self.singleton_equal(doc, &tag, should_be_equal); | ||
} | ||
|
||
ClauseGuard::NotEquals { left, right, .. } => { | ||
let left = self.guard(left); | ||
let right = self.guard(right); | ||
self.prelude_equal_call(false, left, right) | ||
self.prelude_equal_call(should_be_equal, left, right) | ||
} | ||
|
||
ClauseGuard::GtFloat { left, right, .. } | ClauseGuard::GtInt { left, right, .. } => { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -687,3 +687,218 @@ 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. |
||
|
||
#[test] | ||
fn variant_defined_in_another_module_qualified_expression() { | ||
assert_js!( | ||
( | ||
"other_module", | ||
r#"pub type Thingy { Variant OtherVariant }"# | ||
), | ||
r#" | ||
import other_module | ||
|
||
pub fn check(x) -> Bool { | ||
x == other_module.Variant | ||
} | ||
"#, | ||
); | ||
} | ||
|
||
#[test] | ||
fn variant_defined_in_another_module_unqualified_expression() { | ||
assert_js!( | ||
("other_module", r#"pub type Thingy { Variant Other(Int) }"#), | ||
r#" | ||
import other_module.{Variant} | ||
|
||
pub fn check(x) -> Bool { | ||
x == Variant | ||
} | ||
"#, | ||
); | ||
} | ||
|
||
#[test] | ||
fn variant_defined_in_another_module_aliased_expression() { | ||
assert_js!( | ||
("other_module", r#"pub type Thingy { Variant Other(Int) }"#), | ||
r#" | ||
import other_module.{Variant as Aliased} | ||
|
||
pub fn check(x) -> Bool { | ||
x == Aliased | ||
} | ||
"#, | ||
); | ||
} | ||
|
||
#[test] | ||
fn variant_defined_in_another_module_qualified_clause_guard() { | ||
assert_js!( | ||
("other_module", r#"pub type Thingy { Variant Other(Int) }"#), | ||
r#" | ||
import other_module | ||
|
||
pub fn process(e) -> String { | ||
case e { | ||
value if value == other_module.Variant -> "match" | ||
_ -> "no match" | ||
} | ||
} | ||
"#, | ||
); | ||
} | ||
|
||
#[test] | ||
fn variant_defined_in_another_module_unqualified_clause_guard() { | ||
assert_js!( | ||
("other_module", r#"pub type Thingy { Variant Other(Int) }"#), | ||
r#" | ||
import other_module.{Variant} | ||
|
||
pub fn process(e) -> String { | ||
case e { | ||
value if value == Variant -> "match" | ||
_ -> "no match" | ||
} | ||
} | ||
"#, | ||
); | ||
} | ||
|
||
#[test] | ||
fn variant_defined_in_another_module_aliased_clause_guard() { | ||
assert_js!( | ||
("other_module", r#"pub type Thingy { Variant Other(Int) }"#), | ||
r#" | ||
import other_module.{Variant as Aliased} | ||
|
||
pub fn process(e) -> String { | ||
case e { | ||
value if value == Aliased -> "match" | ||
_ -> "no match" | ||
} | ||
} | ||
"#, | ||
); | ||
} |
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.
Could we extract the duplicated bits to helper methods please 🙏
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.
surely
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 hasn't been done yet! The patterns are still duplicated. Could we write something like this?:
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 is still outstanding 🙏
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 still hasn't been done
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.
Once again, this still needs to be done