Skip to content

Restore names when generalizing unknowns#4257

Merged
JordanMartinez merged 11 commits intopurescript:masterfrom
vtrl:restore-names
Mar 16, 2022
Merged

Restore names when generalizing unknowns#4257
JordanMartinez merged 11 commits intopurescript:masterfrom
vtrl:restore-names

Conversation

@purefunctor
Copy link
Copy Markdown
Member

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 a file to CHANGELOG.d for this PR (see CHANGELOG.d/README.md)
    [ ] Added myself to CONTRIBUTORS.md (if this is my first contribution)
  • Linked any existing issues or proposals that this pull request should close
    [ ] Updated or added relevant documentation
  • Added a test for the contribution (if applicable)

@JordanMartinez
Copy link
Copy Markdown
Contributor

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

@purefunctor
Copy link
Copy Markdown
Member Author

It'd end up suffixing another digit regardless.

forall r25 r36. r25 -> r36 -> Int

I'm thinking maybe this could be helped by adding an ':

forall r2'5 r3'6. r2'5 -> r3'6 -> Int

@JordanMartinez
Copy link
Copy Markdown
Contributor

Good idea, but I think ' is used more frequently than integers when quickly distinguishing one type var from another:

foo :: forall sym a r r'.
  ( Row.Cons sym a r r'
  , IsSymbol sym
  ) => ....

which would produce

forall sym4 a5 r'6 r''7. ...

So, there's a few conflicting issues here:

  • No matter which separater one uses, this issue will always arise if one terminates the ty var name with that same separator.
  • If one accounts for the ty var name conditionally (e.g. "if it ends with integer, insert '; if ends with ', insert _; ..."), the inferred ty var names will no longer be consistent (e.g. a1, a'1, a_1)
  • There are only so many chars one can use before appending extra chars to the ty var name makes it longer than it needs to be and thus unreadable / noisy (e.g. a_tyVarNum1, a'_tyVarNum2, a__tyVarNum3).
  • While prepending the integer to the front of the ty var would make it easily distinguishable, a ty var name cannot start with an integer, so it breaks copy-paste workflows.

So, I think this issue I've brought up should be ignored. This PR does address the larger problem, namely, that forall t0 t1 t2 t3 is harder to read and compare to its original function than forall a0 b0 c0 d0.

Copy link
Copy Markdown
Contributor

@JordanMartinez JordanMartinez left a comment

Choose a reason for hiding this comment

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

LGTM, aside from the changelog shortening.

@JordanMartinez
Copy link
Copy Markdown
Contributor

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 forall c14 d15 b25 d27. d27 -> c14 -> b25 -> d15 -> d27

Warning found:
in module �[33mMain�[0m
at tests/purs/warning/4256v2.purs:10:1 - 10:58 (line 10, column 1 - line 10, column 58)

  No type declaration was provided for the top-level declaration of �[33mbaz�[0m.
  It is good practice to provide type declarations as a form of documentation.
  The inferred type of �[33mbaz�[0m was:
  �[33m                                                         �[0m
  �[33m  forall c14 d15 b25 d27. d27 -> c14 -> b25 -> d15 -> d27�[0m
  �[33m                                                         �[0m

in value declaration �[33mbaz�[0m

See https://github.com/purescript/documentation/blob/master/errors/MissingTypeDeclaration.md for more information,
or to contribute content related to this warning.

Copy link
Copy Markdown
Contributor

@MonoidMusician MonoidMusician left a comment

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

@rhendric rhendric left a comment

Choose a reason for hiding this comment

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

Looks great, aside from some minor housekeeping issues! Thanks for taking this on.

@JordanMartinez
Copy link
Copy Markdown
Contributor

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

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

@JordanMartinez JordanMartinez merged commit 2667489 into purescript:master Mar 16, 2022
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.

Track original names of quantified variables instantiated into unknowns

4 participants