Restore names when generalizing unknowns#4257
Restore names when generalizing unknowns#4257JordanMartinez merged 11 commits intopurescript:masterfrom
Conversation
d18edd4 to
c567fbb
Compare
69b3f72 to
43939e0
Compare
43939e0 to
b736373
Compare
|
I'm asking this more out of curiosity. What does the inferred type look like when a type variable name includes an integer? For example: foo :: forall r1 r2 r3. r1 -> r2 -> r3 -> r1
foo r _ _ = r |
|
It'd end up suffixing another digit regardless. forall r25 r36. r25 -> r36 -> IntI'm thinking maybe this could be helped by adding an forall r2'5 r3'6. r2'5 -> r3'6 -> Int |
|
Good idea, but I think foo :: forall sym a r r'.
( Row.Cons sym a r r'
, IsSymbol sym
) => ....which would produce So, there's a few conflicting issues here:
So, I think this issue I've brought up should be ignored. This PR does address the larger problem, namely, that |
JordanMartinez
left a comment
There was a problem hiding this comment.
LGTM, aside from the changelog shortening.
|
Also, I was curious to see what happens when multiple functions using the same ty var names are used together: -- @shouldWarnWith MissingTypeDeclaration
module Main where
foo :: forall a b c d. a -> b -> c -> d -> a
foo a _ _ _ = a
bar :: forall a b c d. a -> b -> c -> d -> a
bar a _ _ _ = a
baz a x y = bar (foo a 2 3 4) (foo a 2 3 4) (foo x y a a)produces |
MonoidMusician
left a comment
There was a problem hiding this comment.
The error message I added for the ambiguous fundep check needs to be updated to match (see inline comment), but other than that no objections from me.
4d7ef75 to
20bd968
Compare
rhendric
left a comment
There was a problem hiding this comment.
Looks great, aside from some minor housekeeping issues! Thanks for taking this on.
a49cd4b to
50c84bc
Compare
|
@rhendric Any other feedback on this? I believe the issues you raised have been resolved. |
| ) | ||
|
|
||
| lookupUnkName :: (MonadState CheckState m) => Unknown -> m (Maybe Text) | ||
| lookupUnkName u = gets $ M.lookup u . substNames . checkSubstitution |
There was a problem hiding this comment.
I still think the common fromMaybe "t" should be moved into this function (so that it returns an m Text), but that's a minor nit and I only mention it in case you meant to and overlooked it.
There was a problem hiding this comment.
I'm going to merge this as is to ensure this doesn't bitrot. If that's something that was overlooked, it can be fixed in a separate PR.
Closes #4256. This change makes it so the type checker tracks which quantified type variables are replaced with unknowns such that they can then be restored in the future.
Checklist:
[ ] Added myself to CONTRIBUTORS.md (if this is my first contribution)
[ ] Updated or added relevant documentation