-
-
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 4 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 |
---|---|---|
|
@@ -178,6 +178,26 @@ pub(crate) struct Generator<'module, 'ast> { | |
pub let_assert_always_panics: bool, | ||
} | ||
|
||
fn ctor_tag(g: &TypedClauseGuard) -> Option<EcoString> { | ||
match g { | ||
ClauseGuard::Constant(Constant::Record { | ||
arguments, name, .. | ||
}) if arguments.is_empty() => Some(name.clone()), | ||
_ => None, | ||
} | ||
} | ||
|
||
fn is_var(g: &TypedClauseGuard) -> bool { | ||
matches!(g, ClauseGuard::Var { .. }) | ||
} | ||
|
||
fn looks_like_constructor_alias(var: &TypedClauseGuard) -> bool { | ||
match var { | ||
ClauseGuard::Var { type_, .. } => matches!(&**type_, Type::Fn { .. }), // lambda | ||
_ => false, | ||
} | ||
} | ||
|
||
impl<'module, 'a> Generator<'module, 'a> { | ||
#[allow(clippy::too_many_arguments)] // TODO: FIXME | ||
pub fn new( | ||
|
@@ -1536,14 +1556,74 @@ 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 | ||
{ | ||
return self.singleton_equal_helper(left, name, should_be_equal); | ||
} | ||
|
||
// LHS: singleton record constructor (arity 0) | ||
if let TypedExpr::Var { | ||
constructor: | ||
ValueConstructor { | ||
variant: ValueConstructorVariant::Record { arity: 0, name, .. }, | ||
.. | ||
}, | ||
.. | ||
} = left | ||
{ | ||
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>, | ||
|
@@ -1966,16 +2046,40 @@ 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, .. } => { | ||
let should_be_equal = matches!(guard, ClauseGuard::Equals { .. }); | ||
let both_ctor_values = ctor_tag(left).is_some() && ctor_tag(right).is_some(); | ||
|
||
if !both_ctor_values { | ||
if is_var(left) | ||
&& !looks_like_constructor_alias(left) | ||
&& let Some(tag) = ctor_tag(right) | ||
{ | ||
|
||
let l = self.guard(left); | ||
return if should_be_equal { | ||
docvec![l, " instanceof ", tag.to_doc()] | ||
} else { | ||
docvec!["!(", l, " instanceof ", tag.to_doc(), ")"] | ||
}; | ||
} | ||
if is_var(right) | ||
&& !looks_like_constructor_alias(right) | ||
&& let Some(tag) = ctor_tag(left) | ||
{ | ||
let r = self.guard(right); | ||
return if should_be_equal { | ||
docvec![r, " instanceof ", tag.to_doc()] | ||
} else { | ||
docvec!["!(", r, " instanceof ", tag.to_doc(), ")"] | ||
}; | ||
} | ||
} | ||
|
||
ClauseGuard::NotEquals { left, right, .. } => { | ||
let left = self.guard(left); | ||
let right = self.guard(right); | ||
self.prelude_equal_call(false, left, right) | ||
// Otherwise fallback to normal equality check | ||
let left_doc = self.guard(left); | ||
let right_doc = self.guard(right); | ||
self.prelude_equal_call(should_be_equal, left_doc, right_doc) | ||
} | ||
|
||
ClauseGuard::GtFloat { left, right, .. } | ClauseGuard::GtInt { left, right, .. } => { | ||
|
Uh oh!
There was an error while loading. Please reload this page.
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.
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.
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.
yeah. it seems like i wont need these functions if i use
Document
instead ofTypedExpr
.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.
ill do the needful
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.
I agree, I don't think these functions belong here. I think they would be best being inlined into the places they are used 🙏
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.
okay sure