Suggest wrapping value in Ok or Error if it makes sense

[giacomocavalieri]

As suggested by @GearsDatapacks the compiler can now suggest to wrap a value into an Ok/Error if it would make the types line up:

pub fn main() {
  wibble(1)
//       ^ Did you mean to wrap this in an `Ok`?
}

pub fn wibble(value: Result(Int, String)) { todo }

This is a WIP because I still have to figure out how to pretty print this information in pattern matching branches, right now it looks quite bad 😁

[GearsDatapacks]

Looks like you forgot to accept the snapshot again :)

[giacomocavalieri]

Ah yeah this time it's intentional, I'm not really satisfied with how the error looks for case expressions 😆

[lpil]

Nice! Left 1 comment inline.

Could we have some tests for in use and returned from a function annotated as being result? Those will be the most common I think.

[joshi-monster]

Hey! 💜

I tried your same_as function to see if it could work for me to detect my record update type changes, and I noticed 2 problems with it where it might be too strict 🙂

• Record (Named) types are currently considered to be different types if their inferred variant differs. The way it works also affects types with only one variant.
• Unbound and generic type variables are only considered equal if they refer to the same type variable.

The last one means that your new hint doesn't work for these cases for example:

pub fn wibble() -> Result(List(String), Nil) {
  [] // List(a) `same_as` List(String)
}
pub fn wobble() -> Result(List(b), Nil) {
  [] // List(a) `same_as` List(b)
}

I feel like it would be more useful if unbound type variables where allowed to match any type on the right-hand side; Something like is_assignable_to or can_unify might maybe be more useful? Of course if that's out of scope please ignore me, I was just getting excited about that one 😊

~ yoshi

[lpil]

Hello! Are you still working on this one? Thanks

[giacomocavalieri]

Oh my I hadn't noticed the conflicts, once those are solved I think this is ready for review!

[giacomocavalieri]

This is ready now!

[lpil]

Wonderful, thank you! Left a few small notes

[lpil]

How come this is false?

[giacomocavalieri]

I thought an unbound variable is never meant to survive the type inference process, so I chose a safe default there

[lpil]

That's only in the interface, there can be many unbound variables within.

[giacomocavalieri]

This should be ready now!

[lpil]

This error is wrong! Ok([first, ..rest] -> first) would not be valid.

[giacomocavalieri]

The message is referring to the first bit but the way the two overlapping spans are rendered it's not very clear; do you think we should stop highlighting the entire case branch in this case? However that would mean that the error message is not very precise now:

[first, ..rest] -> first
                   ^^^^^ Did you mean to wrap this in an `Ok`?

This case clause was found to return a different type than the previous
one, but all case clauses must return the same type.

And it would be more confusing when the part on the right of -> is on its own line as the clause would not be displayed in the error!

  first
  ^^^^^ Did you mean to wrap this in an `Ok`?

This case clause was found to return a different type than the previous
one, but all case clauses must return the same type.

[lpil]

I think we should continue to highlight the entire cause as the source of the type error, but a label about wrapping an expression should only point to the code that is to be wrapped.

If this is a lot of work then another option could be to not have this suggestion for case clauses.

[giacomocavalieri]

The tricky thing is it is already pointing to just the code that needs to be wrapped but the way the library renders it it's not immediately obvious. If the then clause were not on the same line what you'd see is this:

     case wibble {
       A -> Error("a")
  ->   wobble ->
 |     1
 |     ^ Did you mean to wrap this ...
 |
 incorrect branch

But when the two are on the same line the two look like this

wobble -> 1
^^^^^^^^^^^
          |
          Did you mean to wrap this
incorrect branch

The way it gets rendered is not the best, maybe I could try and remove it for branches when the then clause is on the same line or for case matches entirely

[lpil]

Ah, I see, that is annoying. Sounds good to me- changing printing sounds like it would be irritating.

[lpil]

I'm finding this quite hard to read with all the nesting, it's easy to miss that it's returning a tuple. Could you assign this bit to a variable pleaes