Fix incomplete type traversals by rhendric · Pull Request #4155 · purescript/purescript

rhendric · 2021-07-14T01:00:27Z

Description of the change

Our type traversals were ignoring the optional kind in Skolem types; assuming that wasn't intentional, this fixes it. It also fixes a double application of the traversing function on non-recursive constructors in everywhereOnTypesTopDownM. I'm not aware of any user-facing errors caused by these issues.

Both of these issues are caught by the new tests, which involve an amount of QuickCheck machinery rather disproportionate to the fixes themselves, but our traversals have had other issues in the past and this seems like a prudent precaution.

Checklist:

Added a file to CHANGELOG.d for this PR (see CHANGELOG.d/README.md)
Added a test for the contribution (if applicable)

rhendric · 2021-07-14T03:26:45Z

@kl0tl @natefaubion I've gotten myself into a bit of a pickle here, and it's at the intersection of Coercible and polykinds.

There's one golden test that fails to match as a consequence of adding a skolem's kind to the type traversals. In that test, a NoInstanceFound error is raised due to a Coercible constraint not being solved. The constraint contains an unknown kind in a skolem, but otherwise doesn't contain unknowns. So the test output is now changed in two ways: first, it reports that there are unknown types in the instance head, and second, it lists a new unknown type. But, at least with the invisible kind applications that are the default in error messages, the unknown type never appears in the rest of the message, which is clearly not good. And I'm also not sure that it's good that an unknown in the kind of a skolem should trigger the ‘unknown type in instance head’ hint—can kinds have an effect on Coercible solving?

Is it true, then, that the original error message is the correct one to show? If so, how should we go about ignoring the kind unknowns? (If all kind information is irrelevant in Coercible solving, maybe we should suppress unknowns in KindApps as well? Or was there actually intent behind leaving skolem kinds untraversed?) And how should ignoring those unknowns interact with whether --verbose-errors is in use?

kl0tl · 2021-07-16T10:38:26Z

can kinds have an effect on Coercible solving?

The interaction solver only unifies kinds in order to emit KindsDoNotUnify errors instead of NoInstanceFound errors when solving heterogeneously kinded Coercible contraints, so annotating kinds to instantiate unknowns in Coercible constraints should never resolve NoInstanceFound errors.

Is it true, then, that the original error message is the correct one to show? If so, how should we go about ignoring the kind unknowns? (If all kind information is irrelevant in Coercible solving, maybe we should suppress unknowns in KindApps as well? Or was there actually intent behind leaving skolem kinds untraversed?)

I don’t know if skolem kinds were purposefuly ignored, but I think we can erase skolems kinds and kinds applications when computing whether insoluble Coercible constraints contain unknowns. That would get rid of the “The instance head contains unknown type variables. Consider adding a type annotation.” part of the error message.

And how should ignoring those unknowns interact with whether --verbose-errors is in use?

We could avoid listing “invisible” unknowns by erasing skolem kinds in insoluble Coercible constraints arguments and in ErrorSolvingConstraint hints, while stripping redundants hints, and since we erase kind applications when pretty printing types unless --verbose-errors is enabled I think we should erase them when simplifying error messages.

Also, do we hide kinds arguments of constraints on purpose?

Taking the failing CoercibleRepresentational test as example, should we display

 No type class instance was found for

-    Prim.Coerce.Coercible a2
+    Prim.Coerce.Coercible @t4 a2
                               b3

with --verbose-errors, and

 while solving type class constraint

-  Prim.Coerce.Coercible (Phantom @t0 a1)
+  Prim.Coerce.Coercible @Type (Phantom @t0 a1)
                               (Phantom @t2 b3)

in hints, regardless of the flag?

rhendric · 2021-07-16T17:10:30Z

Also, do we hide kinds arguments of constraints on purpose?

~~The first behavior I think we have already~~ (Edit: nope, see below); the second is due to not doing anything with the second wildcard in

purescript/src/Language/PureScript/Errors.hs

Line 1495 in 2b9c7a6

renderHint (ErrorSolvingConstraint (Constraint _ nm _ ts _)) detail =

which was introduced in the polykinds commit. Possibly an oversight, possibly intentional?

Edit: The first behavior is similarly connected to the second wildcard in

purescript/src/Language/PureScript/Errors.hs

Line 884 in 2b9c7a6

renderSimpleErrorMessage (NoInstanceFound (Constraint _ nm _ ts _) unks) =

so yeah, I see now why you would call these out together. Should we render constraints analogously to how we render types in error messages?

rhendric

This is now in a state where everything passes and I'm mostly happy with what I had to do to get there. The three unexpected consequences of fixing these traversals are noted in review comments (as well as in code comments, where appropriate).

rhendric · 2021-12-21T18:49:04Z

lib/purescript-cst/src/Language/PureScript/Types.hs

+  Skolem ann name _ i sc ->
+    Skolem ann name Nothing i sc


Unexpected consequence #⁠1: Removing kinds from skolems when erasing kind applications. This field is really just another form of kind annotation, and so it seems of a piece with the rest of what this function does—I'm 100% not conflicted about this.

rhendric · 2021-12-21T18:55:42Z

src/Language/PureScript/Errors.hs

+  -- We intentionally remove the kinds from skolems, because they are never
+  -- presented when pretty-printing. Any unknowns in those kinds shouldn't
+  -- appear in the list of unknowns unless used somewhere else.
+  replaceTypes (Skolem ann name _ s sko) = do
    m <- get
    case M.lookup s (umSkolemMap m) of
      Nothing -> do
        let s' = umNextIndex m
        put $ m { umSkolemMap = M.insert s (T.unpack name, s', Just (fst ann)) (umSkolemMap m), umNextIndex = s' + 1 }
-        return (Skolem ann name mbK s' sko)
-      Just (_, s', _) -> return (Skolem ann name mbK s' sko)
+        return (Skolem ann name Nothing s' sko)
+      Just (_, s', _) -> return (Skolem ann name Nothing s' sko)


Unexpected consequence #⁠2: Also removing kinds from skolems when traversing a type looking for unknowns to report in an error message. As the comment says, this is because we never print the kinds from skolems; we drop them on the ground when converting them to PrettyPrintType.

purescript/src/Language/PureScript/Pretty/Types.hs

Line 73 in d2fb21b

go _ (Skolem _ t _ n _) = PPSkolem t n

This seems slightly less principled but still fairly practical; I feel 90% good about this. Only caveat is we might need to change this back if we do decide to print the kinds from skolems under some conditions (like when --verbose-errors is in use).

src/Language/PureScript/TypeChecker/Entailment/Coercible.hs

rhendric · 2022-02-27T05:18:17Z

lib/purescript-cst/src/Language/PureScript/Types.hs

+-- | Check if a type contains unknowns in a position that is relevant to
+-- constraint solving. (Kinds are not.)
 containsUnknowns :: Type a -> Bool
-containsUnknowns = everythingOnTypes (||) go where
+containsUnknowns = everythingOnTypes (||) go . eraseKindApps where


Unexpected consequence #⁠3 (take two): ignoring unknowns in kinds when deciding whether to show the unknown types hint in NoInstanceFound errors. Previously, I thought this would only apply to Coercible, but after merging the apartness checker fixes in #4149, it seems that unknowns need to be stripped from kinds when solving all constraints. I'm no expert on polykinds, but it would make sense to me that kind applications and annotations in a constraint can't affect how the constraint is solved. My confidence here is down to something like 60%—it's marginally my best guess at the right thing to do, but I think we need some confirmation from additional brains; I don't claim to fully understand how polymorphic class kinds can interact with entailment. (@natefaubion?)

My take is that non-ambiguous kind polymorphism on classes are equivalent to adding extra arguments to the class with fundeps pointing from the type to the kind. That is:

class Wat :: forall k. k -> Constraint class Wat ok

Would be kind of like saying:

class Wat k ok | ok -> k

for the purposes of dispatch.

In the case of ambiguous kind variables, it's not really any different from a normal parameter, it would just be provided through explicit kind applications.

I'm not sure how this affects your interpretation of "kind applications and kind annotations in a constraint can't affect how the constraint is solved".

The kind-of-like-fundeps are the thing I'm asking about here; so if ok is known but k is unknown, and no instance of Wat ok is found, it's not for lack of type annotations, because k is determined by ok. And from that, I conclude that it's correct to strip out kinds from types before checking those types for unknowns when deciding whether to show the ‘consider adding type annotations’ hint.

So... is this a safe change to make?

I stand by my above comments. More to the point of safety, though, this is a change to a function that is only ever used to determine whether or not to display a particular hint in an error message; the harm of getting this wrong is minimal.

hdgarrood · 2022-05-22T21:40:40Z

@rhendric I just made an attempt at #1647 and the approach I am taking appears to work, although I have stumbled due to missing cases in onTypesInErrorMessage, in a similar vein to this. The subterms trick won't work here unfortunately; do you have any other suggestions? I'm tempted to pull in a syb dependency in the tests and just compare against everywhere.

rhendric · 2022-05-23T00:01:39Z

Mind sharing your branch? subterms is just the shrinking strategy here, so even if that doesn't work (I imagine recursivelyShrink would be better) you could still have a functional test with forAll instead of forAllShrink.

hdgarrood · 2022-05-23T16:25:24Z

👍 I'll make a PR

hdgarrood · 2022-05-23T16:35:32Z

See #4337. Come to think of it, maybe everywhere isn't really what I'm after, I think I might just want gmapT, because I don't want to visit every Type node in this case, I just want to visit the Type values that are immediate children of the ErrorMessageHint and SimpleErrorMessage constructors.

rhendric · 2022-05-23T18:32:24Z

I didn't see any new property-based tests in #4337, so I'm still not 100% sure I'm following you, but I think gmapQ is actually the thing to use here, assuming a test that does roughly this:

create a Gen ErrorMessage by using genericArbitraryUG with an environment that includes a trivial SourceType generator—pure $ srcTypeLevelInt 0, perhaps
use a function with onTypesInErrorMessage that increments each TypeLevelInt by 1
use gmapQ on the result to collect all the types, and then assert that they're all TypeLevelInt _ 1 (to verify that none of them are unvisited or visited multiple times)

hdgarrood · 2022-05-23T18:38:21Z

Oh, sorry, I don't yet have anything that tries to verify that onTypesInErrorMessage is hitting all of the cases it should be hitting. Thanks, I'll give that a try!

rhendric marked this pull request as draft July 14, 2021 01:41

rhendric force-pushed the rhendric/fix-type-traversals branch from cff6e8c to 6aaee0e Compare December 21, 2021 18:06

rhendric marked this pull request as ready for review December 21, 2021 18:46

rhendric commented Dec 21, 2021

View reviewed changes

rhendric force-pushed the rhendric/fix-type-traversals branch 2 times, most recently from 47bf081 to bceab35 Compare February 27, 2022 05:05

rhendric commented Feb 27, 2022

View reviewed changes

rhendric force-pushed the rhendric/fix-type-traversals branch from bceab35 to bba194f Compare April 20, 2022 20:45

Fix incomplete type traversals

2c71381

rhendric force-pushed the rhendric/fix-type-traversals branch from bba194f to 2c71381 Compare April 22, 2022 18:46

rhendric added this to the vAny milestone May 11, 2022

JordanMartinez approved these changes May 13, 2022

View reviewed changes

rhendric merged commit a7ae87f into purescript:master May 15, 2022

rhendric deleted the rhendric/fix-type-traversals branch May 15, 2022 22:21

rhendric mentioned this pull request May 16, 2022

Drop dependency on microlens libraries #4327

Merged

5 tasks

Conversation

rhendric commented Jul 14, 2021

Uh oh!

rhendric commented Jul 14, 2021

Uh oh!

kl0tl commented Jul 16, 2021

Uh oh!

rhendric commented Jul 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rhendric left a comment

Choose a reason for hiding this comment

Uh oh!

rhendric Dec 21, 2021

Choose a reason for hiding this comment

Uh oh!

rhendric Dec 21, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rhendric Feb 27, 2022

Choose a reason for hiding this comment

Uh oh!

natefaubion Feb 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rhendric Feb 28, 2022

Choose a reason for hiding this comment

Uh oh!

JordanMartinez May 12, 2022

Choose a reason for hiding this comment

Uh oh!

rhendric May 13, 2022

Choose a reason for hiding this comment

Uh oh!

hdgarrood commented May 22, 2022

Uh oh!

rhendric commented May 23, 2022

Uh oh!

hdgarrood commented May 23, 2022

Uh oh!

hdgarrood commented May 23, 2022

Uh oh!

rhendric commented May 23, 2022

Uh oh!

hdgarrood commented May 23, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

rhendric commented Jul 16, 2021 •

edited

Loading

natefaubion Feb 27, 2022 •

edited

Loading