Skip to content
Open
Show file tree
Hide file tree
Changes from 6 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
3 changes: 2 additions & 1 deletion compiler-core/src/analyse.rs
Original file line number Diff line number Diff line change
Expand Up @@ -51,9 +51,10 @@ use vec1::Vec1;

use self::imports::Importer;

#[derive(Debug, Clone, PartialEq, Eq)]
#[derive(Debug, Clone, PartialEq, Eq, Default)]
pub enum Inferred<T> {
Known(T),
#[default]
Unknown,
}

Expand Down
6 changes: 0 additions & 6 deletions compiler-core/src/ast.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2473,12 +2473,6 @@ impl<T> BitArraySize<T> {
}
}

impl Default for Inferred<()> {
fn default() -> Self {
Self::Unknown
}
}

#[derive(Debug, Clone, PartialEq, Eq, Hash)]
pub enum AssignName {
Variable(EcoString),
Expand Down
2 changes: 1 addition & 1 deletion compiler-core/src/erlang.rs
Original file line number Diff line number Diff line change
Expand Up @@ -708,7 +708,7 @@ fn string_inner(value: &str) -> Document<'_> {
.replace_all(value, |caps: &Captures<'_>| {
let slashes = caps.get(1).map_or("", |m| m.as_str());

if slashes.len() % 2 == 0 {
if slashes.len().is_multiple_of(2) {
format!("{slashes}u")
} else {
format!("{slashes}x")
Expand Down
120 changes: 111 additions & 9 deletions compiler-core/src/javascript/expression.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Copy link
Member

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 🙏

Copy link
Author

Choose a reason for hiding this comment

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

surely

Copy link
Member

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?:

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;
}

Copy link
Member

Choose a reason for hiding this comment

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

This is still outstanding 🙏

Copy link
Member

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

Copy link
Member

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


// 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>,
Expand Down Expand Up @@ -1966,16 +2028,56 @@ 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 is_variable(g: &TypedClauseGuard) -> bool {
matches!(g, ClauseGuard::Var { .. })
}

// returns Some(name) if g is a zero arity constructor *value*
fn zero_arity_constructor_tag(g: &TypedClauseGuard) -> Option<EcoString> {
match g {
ClauseGuard::Constant(Constant::Record {
arguments, name, ..
}) if arguments.is_empty() && g.type_().is_named() => Some(name.clone()),
_ => None,
}
}

// true when the variable is the alias introduced by pattern
fn is_constructor_alias(g: &TypedClauseGuard) -> bool {
match g {
ClauseGuard::Var { type_, .. } => matches!(&**type_, Type::Fn { .. }),
_ => false,
}
}

let want_equal = matches!(guard, ClauseGuard::Equals { .. });
let both_sides_ctor = zero_arity_constructor_tag(left).is_some()
&& zero_arity_constructor_tag(right).is_some();

if !both_sides_ctor {
// variable == constructor_value
if is_variable(left)
&& !is_constructor_alias(left)
&& let Some(tag) = zero_arity_constructor_tag(right)
{
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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:

  1. The pattern is for a variant/variant constructor.
  2. No arguments have been supplied.
  3. 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.

Copy link
Author

Choose a reason for hiding this comment

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

okay. ill do it.

Copy link
Member

Choose a reason for hiding this comment

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

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

let val = self.guard(left);
return self.singleton_equal(val, &tag, want_equal);
}
// constructor_value == variable
if is_variable(right)
&& !is_constructor_alias(right)
&& let Some(tag) = zero_arity_constructor_tag(left)
{
let val = self.guard(right);
return self.singleton_equal(val, &tag, want_equal);
}
}

ClauseGuard::NotEquals { left, right, .. } => {
let left = self.guard(left);
let right = self.guard(right);
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)
Copy link
Member

Choose a reason for hiding this comment

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

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.

}

ClauseGuard::GtFloat { left, right, .. } | ClauseGuard::GtInt { left, right, .. } => {
Expand Down
215 changes: 215 additions & 0 deletions compiler-core/src/javascript/tests/custom_types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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"
}
}
"#,
);
}
Copy link
Member

Choose a reason for hiding this comment

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

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.


#[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"
}
}
"#,
);
}
Loading