-
Notifications
You must be signed in to change notification settings - Fork 270
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
Improve error LSP ranges on type mismatches #5284
base: trunk
Are you sure you want to change the base?
Changes from all commits
6f9bda9
3190486
780c91d
897ac5a
83ed1ad
108ed55
e38eebb
1a59338
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,6 +10,7 @@ module Unison.Typechecker | |
isEqual, | ||
isSubtype, | ||
fitsScheme, | ||
isMismatchMissingDelay, | ||
Env (..), | ||
Notes (..), | ||
Resolution (..), | ||
|
@@ -38,6 +39,7 @@ import Data.Text qualified as Text | |
import Data.Tuple qualified as Tuple | ||
import Unison.ABT qualified as ABT | ||
import Unison.Blank qualified as B | ||
import Unison.Builtin.Decls qualified as BuiltinDecls | ||
import Unison.Codebase.BuiltinAnnotation (BuiltinAnnotation) | ||
import Unison.Name qualified as Name | ||
import Unison.Prelude | ||
|
@@ -48,6 +50,7 @@ import Unison.Syntax.Name qualified as Name (unsafeParseText, unsafeParseVar) | |
import Unison.Term (Term) | ||
import Unison.Term qualified as Term | ||
import Unison.Type (Type) | ||
import Unison.Type qualified as Type | ||
import Unison.Typechecker.Context qualified as Context | ||
import Unison.Typechecker.TypeLookup qualified as TL | ||
import Unison.Typechecker.TypeVar qualified as TypeVar | ||
|
@@ -405,3 +408,13 @@ wellTyped ppe env term = go <$> runResultT (synthesize ppe Context.PatternMatchC | |
-- `forall a b . a -> b -> a` to be different types | ||
-- equals :: Var v => Type v -> Type v -> Bool | ||
-- equals t1 t2 = isSubtype t1 t2 && isSubtype t2 t1 | ||
|
||
-- | Checks if the mismatch between two types is due to a missing delay, if so returns a tag for which type is | ||
-- missing the delay | ||
isMismatchMissingDelay :: (Var v) => Type v loc -> Type v loc -> Maybe (Either (Type v loc) (Type v loc)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @pchiusano is this a reasonable way to do this? I suppose ideally this would be done inside the typechecker itself, but it seems like a pretty simple type to check against 🤔 It works in simple cases; but unsurprisingly doesn't seem to work with polymorphic types; e.g. this doesn't trip the hint:
If adding it correctly to the typechecker is difficult it's still beneficial to get the hint sometimes rather than not at all; but would be nice if it worked in all cases :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. cc @dolio There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So, here's a possible idea for how to make the typechecker facilitate some of this. Up to you if you want to try out implementing it. So, we can detect
Then you can try to parse the improved error information in some situations. Like, if you got a leaf mismatch with an arrow vs. something else, you could look to see if one of these I think with this approach, you might be able to identify the example you mentioned above. I'm not 100% sure that it wouldn't cause some other weird corner cases, though. |
||
isMismatchMissingDelay typeA typeB | ||
| isSubtype (Type.arrow () (Type.ref () BuiltinDecls.unitRef) (typeA $> ())) (typeB $> ()) = | ||
Just (Left typeA) | ||
| isSubtype (ABT.tm (ABT.tm (Type.Ref BuiltinDecls.unitRef) `Type.Arrow` (typeB $> ()))) (typeA $> ()) = | ||
Just (Right typeB) | ||
| otherwise = Nothing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this extra space may have been intentional to allow the types and the
:
s to line up. I would move the space after the:
to preserve that the types line up, or just revert this line altogether.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this has caught me like 3 times now, I admit I don't find it very intuitive 🙃
I'll move it to after the
:
so I don't do this again in 3 weeks haha.