Gleam: Stop type inference of binary operators from relying on the local scope

[lpil]

Currently we insert all the binary operators into a new scope

https://github.com/gleam-lang/gleam/blob/c03aad42362a651f5c287b106695f123aa3dbae5/src/typ.rs#L453-L585

This is wasteful, and sometimes results in them being suggested as variables in error messages.

Instead have specific code for handling these in the binop_infer function.

[tomwhatmore]

Took a look at this last night and came up with a couple of possible solutions, but they feel quite hacky, so would appreciate some guidance on what you had in mind.

Option 1

Modify infer_value_constructor() to specifically check for binary operator names first and build the relevant ValueConstructor there. If it doesn't match then move on to the current env.get_variable(name) call.

This would be the smallest change to the current system, only requiring one new function and a bit of extra logic in infer_value_constructor().

Option 2

Modify infer_binop to perform the typing itself. I don't entirely understand the current logic here I'm afraid so I might be missing something, but as far as I can tell infer_binop constructs a new UntypedExpr::Var and sends it off down a chain of functions to get typed (do_infer_call() -> infer() -> infer_var() -> infer_value_constructor()). This seems unnecessary, as we know the type of a bin op already and could just construct a TypedExpr::Var directly instead. Would still need to run the args typing code from infer() but that can be done with a bit of refactoring. I've only tried this with + so far but it seems to work.

Option 3

Both of the above are using ValueConstructorVariant::LocalVariable which doesn't seem ideal. Could we add a new variant ValueConstructorVariant::BinOp { name: String } and use that instead? Would work in conjunction with either of the above but would make more sense with 2.

Option 4

Hard mode. Is a binary operator actually a ValueConstructorVariant at all? Why is being turned into a TypedExpr::Var in the first place? It feels like maybe there's a "proper" refactor that separates them out from variables entirely, but I don't understand the system well enough to know what that would look like!

[lpil]

Thanks for looking into this!

I think option 4 is the right path! When inferred a binop we can match on what kind of operator it is and perform the typing in a static way. We don't need to use the local scope because the operators never change in behaviour.

Here's (a pseudocode) example of what I was thinking:

match name {
  BinOp::Eq => {
    let left = infer(left, env)?;
    let right = infer(right, env)?;
    unify(left, right, env)?;
    Ok(TypedExpr::BinOp { location, name, typ, right, left })
  }
} 

I speculate once the refactoring is done this will make the code simpler overall!

[lpil]

Would you like me to assign this to you?

[tomwhatmore]

That looks a lot nicer (and simpler!)

Yeah assign it to me, I'll get on it this evening.

[lpil]

Thank you!

[tomwhatmore]

Have very nearly got this done but I have a single failing test that I'm not sure how to address.

https://github.com/gleam-lang/gleam/blob/3aeed2f61b20fff08c61a0b911993e7126a5cf78/src/typ/tests.rs#L892-L902

Instead of id: 9, it now returns id: 7, so the test fails.

I assume it has something to do with the removal of this:

https://github.com/gleam-lang/gleam/blob/3aeed2f61b20fff08c61a0b911993e7126a5cf78/src/typ.rs#L526-L531

Meaning the generic vars are now being inserting in a different order. Does it actually matter or should I just update the test to expect an id of 7 and move on?

[lpil]

Yes that's right, change the id to the new one :)