Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Suggest wrapping value in Ok or Error if it makes sense #3663

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

giacomocavalieri
Copy link
Member

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
Copy link
Member

Looks like you forgot to accept the snapshot again :)

@giacomocavalieri
Copy link
Member Author

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

@giacomocavalieri giacomocavalieri marked this pull request as ready for review November 7, 2024 09:19
Copy link
Member

@lpil lpil left a comment

Choose a reason for hiding this comment

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

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.

compiler-core/src/error.rs Outdated Show resolved Hide resolved
@lpil lpil marked this pull request as draft November 7, 2024 12:29
@giacomocavalieri giacomocavalieri marked this pull request as ready for review November 7, 2024 21:52
@joshi-monster
Copy link
Contributor

joshi-monster commented Nov 8, 2024

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
Copy link
Member

lpil commented Dec 2, 2024

Hello! Are you still working on this one? Thanks

@giacomocavalieri
Copy link
Member Author

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

@giacomocavalieri
Copy link
Member Author

This is ready now!

Copy link
Member

@lpil lpil left a comment

Choose a reason for hiding this comment

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

Wonderful, thank you! Left a few small notes

compiler-core/src/type_.rs Show resolved Hide resolved
(Type::Tuple { .. }, Type::Fn { .. } | Type::Named { .. }) => false,
(one @ Type::Tuple { .. }, Type::Var { type_ }) => {
type_.as_ref().borrow().same_as_other_type(one)
}
Copy link
Member

Choose a reason for hiding this comment

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

Could you merge all the ones with var on the right hand side and move the new merged clause along with clause with var on the left up to the top please.

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed it! Not sure if it's the look you wanted, let me know if it's better now

(
TypeVar::Unbound { .. },
Type::Named { .. } | Type::Fn { .. } | Type::Tuple { .. },
) => false,
Copy link
Member

Choose a reason for hiding this comment

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

How come this is false?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

@giacomocavalieri
Copy link
Member Author

This should be ready now!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants