Display original function definition generic names on function signature helper when arguments are unbound

[scristobal]

While working on #4570, I noticed that generics were not displayed with the original names, so I also changed that. Hope the feature is interesting.

• The function signature helper now displays original function definition
  generic names when arguments are unbound. For example, in this code:

  pub fn wibble(x: something, y: fn() -> something, z: anything) { Nil }
  
  pub fn main() {
      wibble( )
          // ↑ triggering signature help here, will produce:
          //      wibble(something, fn() -> something, anything)
          // instead of
          //      wibble(a, fn() -> a, b) -> Nil
  }

link to snapshot

It also works in scenarios where not all parameters are unbounded.

pub fn wibble(x: something, y: fn() -> something, z: anything) { Nil }

pub fn main() {
    wibble(1, )
          // ↑ triggering signature help here, will produce:
          //     wibble(Int, fn() -> Int, anything) -> Nil
          // instead of
          //     wibble(Int, fn() -> Int, a) -> Nil
}

link to snapshot

[GearsDatapacks]

Thank you! Looks good! I've left a comment about the implementation of this inline.

[GearsDatapacks]

I find it easier to read when the displayed code is in a separate codeblock, rather than a comment.

[GearsDatapacks]

The Names struct already keeps track of generic variable names, so shouldn't this functionality come with using the new printer?

[scristobal]

That's what I thought exactly! but the problem is that some types_ are actually Unbound, hence the id of the Generic and the Unbound do not match.

I created a draft PR to reproduce my original approach, using the new Printer and the Names from the current module, like you suggested. Also addded a few dbg! statements, this is the output of the one failed snapshot test:

---- language_server::tests::signature_help::help_for_use_function_shows_next_unlabelled_argument stdout ----
[compiler-core/src/language_server/signature_help.rs:110:5] &module.ast.names = Names {
    local_types: {
        (
            "gleam",
            "Int",
        ) <> "Int",
        (
            "gleam",
            "List",
        ) <> "List",
        (
            "gleam",
            "UtfCodepoint",
        ) <> "UtfCodepoint",
        (
            "gleam",
            "Float",
        ) <> "Float",
        (
            "gleam",
            "BitArray",
        ) <> "BitArray",
        (
            "gleam",
            "String",
        ) <> "String",
        (
            "gleam",
            "Nil",
        ) <> "Nil",
        (
            "gleam",
            "Bool",
        ) <> "Bool",
        (
            "gleam",
            "Result",
        ) <> "Result",
    },
    imported_modules: {},
    type_variables: {
        36: "a",
    },
    local_value_constructors: {
        (
            "gleam",
            "True",
        ) <> "True",
        (
            "gleam",
            "Error",
        ) <> "Error",
        (
            "gleam",
            "Nil",
        ) <> "Nil",
        (
            "gleam",
            "False",
        ) <> "False",
        (
            "gleam",
            "Ok",
        ) <> "Ok",
    },
}
[compiler-core/src/type_/printer.rs:318:9] type_ = Named {
    publicity: Public,
    package: "",
    module: "gleam",
    name: "Bool",
    args: [],
    inferred_variant: None,
}
[compiler-core/src/type_/printer.rs:318:9] type_ = Var {
    type_: RefCell {
        value: Unbound {
            id: 39,
        },
    },
}
[compiler-core/src/type_/printer.rs:318:9] type_ = Fn {
    args: [],
    return_: Var {
        type_: RefCell {
            value: Unbound {
                id: 39,
            },
        },
    },
}
[compiler-core/src/type_/printer.rs:318:9] type_ = Named {
    publicity: Public,
    package: "",
    module: "gleam",
    name: "Float",
    args: [],
    inferred_variant: None,
}
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ Snapshot Summary ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
Snapshot file: compiler-core/src/language_server/tests/snapshots/gleam_core__language_server__tests__signature_help__help_for_use_function_shows_next_unlabelled_argument.snap
Snapshot: help_for_use_function_shows_next_unlabelled_argument
Source: compiler-core/src/language_server/tests/signature_help.rs:414
──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
Expression:
pub fn guard(a a: Bool, b b: a, c c: fn() -> a) { 1.0 }

pub fn main() {
    use <- guard(b: 1,)
}

──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
-old snapshot
+new results
────────────┬─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
    6     6 │
    7     7 │
    8     8 │
    9     9 │ ----- Signature help -----
   10       │-guard(a: Bool, b: a, c: fn() -> a) -> Float
         10 │+guard(a: Bool, b: b, c: fn() -> b) -> Float
   11    11 │       ▔▔▔▔▔▔▔
   12    12 │
   13    13 │ No documentation
────────────┴─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
To update snapshots run `cargo insta review`
Stopped on the first failure. Run `cargo insta test` to run all snapshots.

thread 'language_server::tests::signature_help::help_for_use_function_shows_next_unlabelled_argument' panicked at /Users/samu/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/insta-1.41.1/src/runtime.rs:680:13:
snapshot assertion for 'help_for_use_function_shows_next_unlabelled_argument' failed in line 414

This particular snapshot was confusing, because it also produced guard(a: Bool, b: b, c: fn() -> b) -> Float instead of guard(a: Bool, b: a, c: fn() -> a) -> Float to avoid collision with user-defined generics in pub fn guard(a a: Bool, b b: a, c c: fn() -> a) { 1.0 }

or I might totally misused Printer or Names

[GearsDatapacks]

Ah Ok. I must be misunderstanding something then, because from my knowledge, top-level functions should not be unbound.
Ah! Is it because they are being instantiated so that they can be parameterised? Probably. Maybe we can add those unbound IDs to the Names struct as well?

[giacomocavalieri]

I think it might be a bug related to this: #2533

[GearsDatapacks]

No, I don't think so. I managed to work around it in my comment below

[scristobal]

thank you both for jumping in!

[GearsDatapacks]

I did a bit of experimenting, and I found that we can (mostly) fix this issue by registering unbound type variable IDs when we are instantiating, as demonstrated in this commit.

This also avoids the issue that the current implementation has, which is in cases like the following:

fn identity(x: a) -> a { x }

fn main() {
  let a = Ok(10)
  identity(a)
}

This will print fn identity(a) for signature help, whereas it should print fn identity(Result(Int, a))

[scristobal]

I did a bit of experimenting, and I found that we can (mostly) fix this issue by registering unbound type variable IDs when we are instantiating, as demonstrated in this commit.

This also avoids the issue that the current implementation has, which is in cases like the following:

fn identity(x: a) -> a { x }

fn main() {
  let a = Ok(10)
  identity(a)
}

This will print fn identity(a) for signature help, whereas it should print fn identity(Result(Int, a))

I get it! and that is actually a much more simpler solution!

Then if you agree, I will cherry pick your commit over #4589 and continue there.

[GearsDatapacks]

Sounds good. It might need a bit of tweaking though as it was mostly a quick and dirty proof of concept

[scristobal]

fn identity(x: a) -> a { x }

fn main() {
let a = Ok(10)
identity(a)
}

excellent! thank you!