Repository: gleam-lang/gleam (rust)

Issue:
Unexpected argument order on calls with labelled parameters
Hi!

I encounter what I believe to be a bug if the two following properties hold:
- Unlabelled argument order doesn’t differ from function’s parameter order
- Labelled argument order doesn’t matters

Here is a reduction of my original code that motivated me to digg into this issue. The bug is kind of sneaky because two String parameters are silently reversed when a third (labelled) argument corresponding to the first function parameter is added.

```gleam
// This compiles fine
pub fn heading_test() {
  heading("Hello", "/world", size: 1)
}

fn heading(size size: Int, text text: String, href href: String) {
  // Crashes here
  let assert "Hello" = text
  let assert "/world" = href
}
```

----

Using different types to make the issue more obvious. Every line below with a `Error expecting XXX` comment doesn’t type check.

```gleam
type A { A }
type B { B }
type C { C }
type D { D }

fn foo(a a: A, b b: B, c c: C, d d: D) {
  Nil
}

pub fn labelled_call_issue_test() {
  // Calls with a single labelled argument
  foo(A, B, C, d: D)
  foo(A, B, D, c: C)
  foo(A, C, D, b: B) // Error expecting A, D, C, b: B
  foo(B, C, D, a: A) // Error expecting D, B, C, a: A

  // Calls with two labelled arguments
  foo(A, B, c: C, d: D)
  foo(A, B, d: D, c: C)
  foo(A, C, b: B, d: D)
  foo(A, C, d: D, b: B)
  foo(A, D, b: B, c: C)
  foo(A, D, c: C, b: B)
  foo(B, C, a: A, d: D) // Error expecting C, B, a: A, d: D
  foo(B, C, d: D, a: A) // Error expecting C, B, d: D, a: A
  foo(B, D, a: A, c: C) // Error expecting D, B, a: A, c: C
  foo(B, D, c: C, a: A) // Error expecting D, B, c: C, a: A
  foo(C, D, a: A, b: B)
  foo(C, D, b: B, a: A) // Error expecting D, C, b: B, a: A -> labelled argument order matters here

  // No issue with any permutation of a 3 labelled arguments call
  foo(A, b: B, c: C, d: D)
  foo(A, b: B, d: D, c: C)
  foo(A, c: C, b: B, d: D)
  foo(A, c: C, d: D, b: B)
  foo(A, d: D, b: B, c: C)
  foo(A, d: D, c: C, b: B)
  foo(B, a: A, c: C, d: D)
  foo(B, a: A, d: D, c: C)
  foo(B, c: C, a: A, d: D)
  foo(B, c: C, d: D, a: A)
  foo(B, d: D, a: A, c: C)
  foo(B, d: D, c: C, a: A)
  foo(C, a: A, b: B, d: D)
  foo(C, a: A, d: D, b: B)
  foo(C, b: B, a: A, d: D)
  foo(C, b: B, d: D, a: A)
  foo(C, d: D, a: A, b: B)
  foo(C, d: D, b: B, a: A)
}

```




```shell
❯ gleam --version && uname -v
gleam 1.5.0
Darwin Kernel Version 23.6.0: Wed Jul 31 20:48:52 PDT 2024; root:xnu-10063.141.1.700.5~1/RELEASE_ARM64_T6020
```

_Sonoma 14.7 (23H124)_

Pull request:
Fix bug with labelled argument order
Fixes #3671 
This fixes a bug with the reordering of arguments where reordering was just done using `[T]::swap`. This works for simple cases of argument reordering, but breaks down in quite a few cases as shown in that issue.
This implements a more complex reordering method, which removes the labelled arguments first, then reinserts them in order.

A couple of questions:
- I've just put all the erroneous cases from that issue into one test, should I separate them?
- This new method is not super efficient, since is has to iterate the arguments, remove any labelled ones, then iterate again to insert them back. I can think of a few other ways to do this, although they are a bit obscure and could make the code difficult to understand. Let me know if I should change it

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index a2578663b..ff3952213 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -172,6 +172,10 @@
 - Fixed the prelude re-export in generated TypeScript definitions.
   ([Richard Viney](https://github.com/richard-viney))
 
+- Fixed a bug where the compiler would incorrectly type-check and compile
+  calls to functions with labelled arguments in certain cases.
+  ([Surya Rose](https://github.com/GearsDatapacks))
+
 ## v1.5.1 - 2024-09-26
 
 ### Bug Fixes
diff --git a/compiler-cli/src/panic.rs b/compiler-cli/src/panic.rs
index 020ed2bf7..b52aa6156 100644
--- a/compiler-cli/src/panic.rs
+++ b/compiler-cli/src/panic.rs
@@ -1,13 +1,13 @@
 #![allow(clippy::unwrap_used)]
-use std::panic::PanicInfo;
+use std::panic::PanicHookInfo;
 
 pub fn add_handler() {
-    std::panic::set_hook(Box::new(move |info: &PanicInfo<'_>| {
+    std::panic::set_hook(Box::new(move |info: &PanicHookInfo<'_>| {
         print_compiler_bug_message(info)
     }));
 }
 
-fn print_compiler_bug_message(info: &PanicInfo<'_>) {
+fn print_compiler_bug_message(info: &PanicHookInfo<'_>) {
     let message = match (
         info.payload().downcast_ref::<&str>(),
         info.payload().downcast_ref::<String>(),
diff --git a/compiler-core/src/type_/fields.rs b/compiler-core/src/type_/fields.rs
index 3adc2e3ac..748493e74 100644
--- a/compiler-core/src/type_/fields.rs
+++ b/compiler-core/src/type_/fields.rs
@@ -39,10 +39,11 @@ impl FieldMap {
     /// Reorder an argument list so that labelled fields supplied out-of-order are
     /// in the correct order.
     ///
-    pub fn reorder<A>(&self, args: &mut [CallArg<A>], location: SrcSpan) -> Result<(), Error> {
+    pub fn reorder<A>(&self, args: &mut Vec<CallArg<A>>, location: SrcSpan) -> Result<(), Error> {
         let mut labelled_arguments_given = false;
         let mut seen_labels = HashSet::new();
         let mut unknown_labels = Vec::new();
+        let number_of_arguments = args.len();
 
         if self.arity as usize != args.len() {
             return Err(Error::IncorrectArity {
@@ -69,11 +70,18 @@ impl FieldMap {
             }
         }
 
-        let mut i = 0;
-        while i < args.len() {
+        // Keeps track of which labelled arguments need to be inserted into which indices
+        let mut labelled_arguments = HashMap::new();
+
+        // We iterate the argument in reverse order, because we have to remove elements
+        // from the `args` list quite a lot, and removing from the end of a list is more
+        // efficient than removing from the beginning or the middle.
+        let mut i = args.len();
+        while i > 0 {
+            i -= 1;
             let (label, &location) = match &args.get(i).expect("Field indexing to get label").label
             {
-                // A labelled argument, we may need to reposition it in the array vector
+                // A labelled argument, we may need to reposition it
                 Some(l) => (
                     l,
                     &args
@@ -84,7 +92,6 @@ impl FieldMap {
 
                 // Not a labelled argument
                 None => {
-                    i += 1;
                     continue;
                 }
             };
@@ -92,27 +99,29 @@ impl FieldMap {
             let position = match self.fields.get(label) {
                 None => {
                     unknown_labels.push((label.clone(), location));
-                    i += 1;
                     continue;
                 }
 
                 Some(&p) => p,
             };
 
-            // If the argument is already in the right place
-            if position as usize == i {
-                let _ = seen_labels.insert(label.clone());
-                i += 1;
-            } else {
-                if seen_labels.contains(label) {
-                    return Err(Error::DuplicateArgument {
-                        location,
-                        label: label.clone(),
-                    });
-                }
-                let _ = seen_labels.insert(label.clone());
+            if seen_labels.contains(label) {
+                return Err(Error::DuplicateArgument {
+                    location,
+                    label: label.clone(),
+                });
+            }
+            let _ = seen_labels.insert(label.clone());
+
+            // Add this argument to the `labelled_arguments` map, and remove if from the
+            // existing arguments list. It will be reinserted later in the correct index
+            let _ = labelled_arguments.insert(position as usize, args.remove(i));
+        }
 
-                args.swap(position as usize, i);
+        // The labelled arguments must be reinserted in order
+        for i in 0..number_of_arguments {
+            if let Some(arg) = labelled_arguments.remove(&i) {
+                args.insert(i, arg);
             }
         }
 


Test patch:
diff --git a/compiler-core/src/erlang/tests/functions.rs b/compiler-core/src/erlang/tests/functions.rs
index 4ab96145d..f92ac9df4 100644
--- a/compiler-core/src/erlang/tests/functions.rs
+++ b/compiler-core/src/erlang/tests/functions.rs
@@ -109,3 +109,31 @@ pub fn main() {
 "#
     );
 }
+
+#[test]
+fn labelled_argument_ordering() {
+    // https://github.com/gleam-lang/gleam/issues/3671
+    assert_erl!(
+        "
+type A { A }
+type B { B }
+type C { C }
+type D { D }
+
+fn wibble(a a: A, b b: B, c c: C, d d: D) {
+  Nil
+}
+
+pub fn main() {
+  wibble(A, C, D, b: B)
+  wibble(A, C, D, b: B)
+  wibble(B, C, D, a: A)
+  wibble(B, C, a: A, d: D)
+  wibble(B, C, d: D, a: A)
+  wibble(B, D, a: A, c: C)
+  wibble(B, D, c: C, a: A)
+  wibble(C, D, b: B, a: A)
+}
+"
+    );
+}
diff --git a/compiler-core/src/erlang/tests/snapshots/gleam_core__erlang__tests__functions__labelled_argument_ordering.snap b/compiler-core/src/erlang/tests/snapshots/gleam_core__erlang__tests__functions__labelled_argument_ordering.snap
new file mode 100644
index 000000000..ee636b685
--- /dev/null
+++ b/compiler-core/src/erlang/tests/snapshots/gleam_core__erlang__tests__functions__labelled_argument_ordering.snap
@@ -0,0 +1,34 @@
+---
+source: compiler-core/src/erlang/tests/functions.rs
+expression: "\ntype A { A }\ntype B { B }\ntype C { C }\ntype D { D }\n\nfn wibble(a a: A, b b: B, c c: C, d d: D) {\n  Nil\n}\n\npub fn main() {\n  wibble(A, C, D, b: B)\n  wibble(A, C, D, b: B)\n  wibble(B, C, D, a: A)\n  wibble(B, C, a: A, d: D)\n  wibble(B, C, d: D, a: A)\n  wibble(B, D, a: A, c: C)\n  wibble(B, D, c: C, a: A)\n  wibble(C, D, b: B, a: A)\n}\n"
+---
+-module(my@mod).
+-compile([no_auto_import, nowarn_unused_vars, nowarn_unused_function, nowarn_nomatch]).
+
+-export([main/0]).
+-export_type([a/0, b/0, c/0, d/0]).
+
+-type a() :: a.
+
+-type b() :: b.
+
+-type c() :: c.
+
+-type d() :: d.
+
+-file("/root/project/test/my/mod.gleam", 7).
+-spec wibble(a(), b(), c(), d()) -> nil.
+wibble(A, B, C, D) ->
+    nil.
+
+-file("/root/project/test/my/mod.gleam", 11).
+-spec main() -> nil.
+main() ->
+    wibble(a, b, c, d),
+    wibble(a, b, c, d),
+    wibble(a, b, c, d),
+    wibble(a, b, c, d),
+    wibble(a, b, c, d),
+    wibble(a, b, c, d),
+    wibble(a, b, c, d),
+    wibble(a, b, c, d).
diff --git a/compiler-core/src/javascript/tests/functions.rs b/compiler-core/src/javascript/tests/functions.rs
index 4ec9c697b..4535f5a74 100644
--- a/compiler-core/src/javascript/tests/functions.rs
+++ b/compiler-core/src/javascript/tests/functions.rs
@@ -459,3 +459,31 @@ fn function_literals_get_properly_wrapped_3() {
 "#
     );
 }
+
+#[test]
+fn labelled_argument_ordering() {
+    // https://github.com/gleam-lang/gleam/issues/3671
+    assert_js!(
+        "
+type A { A }
+type B { B }
+type C { C }
+type D { D }
+
+fn wibble(a a: A, b b: B, c c: C, d d: D) {
+  Nil
+}
+
+pub fn main() {
+  wibble(A, C, D, b: B)
+  wibble(A, C, D, b: B)
+  wibble(B, C, D, a: A)
+  wibble(B, C, a: A, d: D)
+  wibble(B, C, d: D, a: A)
+  wibble(B, D, a: A, c: C)
+  wibble(B, D, c: C, a: A)
+  wibble(C, D, b: B, a: A)
+}
+"
+    );
+}
diff --git a/compiler-core/src/javascript/tests/snapshots/gleam_core__javascript__tests__functions__labelled_argument_ordering.snap b/compiler-core/src/javascript/tests/snapshots/gleam_core__javascript__tests__functions__labelled_argument_ordering.snap
new file mode 100644
index 000000000..e712b335a
--- /dev/null
+++ b/compiler-core/src/javascript/tests/snapshots/gleam_core__javascript__tests__functions__labelled_argument_ordering.snap
@@ -0,0 +1,28 @@
+---
+source: compiler-core/src/javascript/tests/functions.rs
+expression: "\ntype A { A }\ntype B { B }\ntype C { C }\ntype D { D }\n\nfn wibble(a a: A, b b: B, c c: C, d d: D) {\n  Nil\n}\n\npub fn main() {\n  wibble(A, C, D, b: B)\n  wibble(A, C, D, b: B)\n  wibble(B, C, D, a: A)\n  wibble(B, C, a: A, d: D)\n  wibble(B, C, d: D, a: A)\n  wibble(B, D, a: A, c: C)\n  wibble(B, D, c: C, a: A)\n  wibble(C, D, b: B, a: A)\n}\n"
+---
+import { CustomType as $CustomType } from "../gleam.mjs";
+
+class A extends $CustomType {}
+
+class B extends $CustomType {}
+
+class C extends $CustomType {}
+
+class D extends $CustomType {}
+
+function wibble(a, b, c, d) {
+  return undefined;
+}
+
+export function main() {
+  wibble(new A(), new B(), new C(), new D());
+  wibble(new A(), new B(), new C(), new D());
+  wibble(new A(), new B(), new C(), new D());
+  wibble(new A(), new B(), new C(), new D());
+  wibble(new A(), new B(), new C(), new D());
+  wibble(new A(), new B(), new C(), new D());
+  wibble(new A(), new B(), new C(), new D());
+  return wibble(new A(), new B(), new C(), new D());
+}
diff --git a/compiler-core/src/type_/tests.rs b/compiler-core/src/type_/tests.rs
index 6b1d6c0e5..191efecc1 100644
--- a/compiler-core/src/type_/tests.rs
+++ b/compiler-core/src/type_/tests.rs
@@ -2457,3 +2457,32 @@ pub fn main() {
         vec![("main", "fn() -> Int")]
     );
 }
+
+#[test]
+fn labelled_argument_ordering() {
+    // https://github.com/gleam-lang/gleam/issues/3671
+    assert_module_infer!(
+        "
+type A { A }
+type B { B }
+type C { C }
+type D { D }
+
+fn wibble(a a: A, b b: B, c c: C, d d: D) {
+  Nil
+}
+
+pub fn main() {
+  wibble(A, C, D, b: B)
+  wibble(A, C, D, b: B)
+  wibble(B, C, D, a: A)
+  wibble(B, C, a: A, d: D)
+  wibble(B, C, d: D, a: A)
+  wibble(B, D, a: A, c: C)
+  wibble(B, D, c: C, a: A)
+  wibble(C, D, b: B, a: A)
+}
+",
+        vec![("main", "fn() -> Nil")]
+    );
+}