Fix error on missing type class name with non-exported superclass (#3132) by parsonsmatt · Pull Request #3173 · purescript/purescript

parsonsmatt · 2017-12-19T06:33:57Z

This PR adds some test cases for #3132 and might even have a fix 😄

Any call to internalError will trigger an MVar bug. That should be fixed too. But the real underlying bug is that superclasses must be exported.

1) Passing examples '3132.purs' should compile and run without error Error found: in module Main at /home/matt/Projects/purescript/examples/passing/3132.purs line 19, column 1 - line 1 9, column 34 No type class instance was found for class ClassDefinitions.Zero because the class was not in scope. Perhaps it was not exported. while solving type class constraint ClassDefinitions.Zero while checking that expression #dict Zero has type Zero in value declaration inst3 See https://github.com/purescript/documentation/blob/master/errors/UnknownClass.md for more information, or to contribute content related to this error.

…esolve

When you provide a concrete type that satisfies the instance, all is well. So the bug only triggers when you ask for the instance in the instance context.

parsonsmatt

The PR is ready for review now 😄

parsonsmatt · 2017-12-20T00:09:53Z

-    else Just $ "Expected these errors: " ++ show expected ++ ", but got these: " ++ show actual
+    else Just $ "Expected these errors: " ++ show expected ++ ", but got these: " 
+      ++ show actual ++ ", full error messages: " 
+      ++ unlines (map (P.renderBox . P.prettyPrintSingleError P.defaultPPEOptions) (P.runMultipleErrors errs))


This modification to the reporting made it much easier to debug exactly what was going on with a mistaken error code.

I'd be in favour of merging this in a separate PR.

Cool, I'll factor it out then.

parsonsmatt · 2017-12-20T00:10:08Z

+      maybe
+        (throwError . errorMessage . UnknownClass $ className)
+        pure
+        $ M.lookup className tcs


If there's a more suitable idiom, I'd be happy to give it a shot 🙂

parsonsmatt · 2017-12-20T00:11:34Z

+data B
+
+instance inst2 :: C2 A
+instance inst3 :: (C2 a) => C3 a B


It seems like the effect of having a "hidden" nullary class/instance is to prevent polymorphic instances like this. Note that C2 A is totally fine, despite C1 not being in scope, and C3 A B is also totally fine. But requiring C2 a => C3 a B causes the missing name error.

Hm, I'm not sure. It doesn't seem right to me that this code would cause an unknown class error from C1 not being in scope, and it also doesn't seem right to me that hidden nullary classes can prevent you from writing instances like this.

My suggestion is that we say this code should actually compile successfully, and in fact we need to modify the compiler to make sure that hidden classes like C1 are made available in the relevant places in the compiler to make sure that internal error doesn't get hit.

I think the real problem with all this, is that the ClassDefinitions module should demand that you export C1 because it's required by C2. That would be the rule we use for all the other cases for this kind of thing.

parsonsmatt · 2017-12-20T18:17:58Z

So this PR fixes the MVar bug and properly reports the missing class constraint. It appears this has also uncovered some behavior that we want to modify, with two suggestions:

My suggestion is that we say this code should actually compile successfully, and in fact we need to modify the compiler to make sure that hidden classes like C1 are made available in the relevant places in the compiler to make sure that internal error doesn't get hit.
I think the real problem with all this, is that the ClassDefinitions module should demand that you export C1 because it's required by C2. That would be the rule we use for all the other cases for this kind of thing.

hdgarrood · 2017-12-20T18:25:08Z

I think suggestion 2 (@garyb's) makes the most sense.

parsonsmatt · 2017-12-20T18:34:16Z

Do we want to merge this as-is, and create another issue/PR for that, or work that change into this PR? I think that's going to require a good bit more work

hdgarrood · 2017-12-20T18:48:06Z

I think I'd say that the MVar bug is actually within Make, in that any use of error in one of the threads in there manifests as an error whose message implies that there's a bug in the threading code when there isn't. That is, I don't think I'd say that this PR fixes the MVar bug.

As far as I can see, I think it's correct that we use internalError in TypeChecker.Entailment there, because if we go down that code path, it means there's a bug somewhere else in the compiler. Therefore, I think we want this to manifest as an internal compiler error (as it does now) rather than a regular compiler error.

Given this, I think the best course of action is to not merge this, and instead to expand the the TransitiveExportError check to include superclass constraints.

parsonsmatt · 2017-12-21T19:27:08Z

Cool, I think I can get that implemented 😄 Thanks for the feedback!

hdgarrood · 2017-12-21T21:55:43Z

Of course, thank you for all your work on this!

parsonsmatt · 2017-12-21T22:07:46Z

I'm actually planning on keeping the work on this branch; mind re-opening?

parsonsmatt · 2017-12-22T04:43:06Z

@@ -0,0 +1,18 @@
+-- @shouldFailWith TransitiveExportError


Updated the reason this should fail.

Can you paste the full error you get when you attempt to compile this?

Failing examples '3132.purs' should fail with 'UnknownName' FAILED [1] Failures: tests/TestCompiler.hs:218: 1) Failing examples '3132.purs' should fail with 'UnknownName' Expected these errors: ["UnknownName"], but got these: ["TransitiveExportError"], full error messages: in module Main at /home/matt/Projects/purescript/examples/failing/3132.purs line 1, column 1 - line 18, column 7 An export for class C3 requires the following to also be exported: class C1 class C2 See https://github.com/purescript/documentation/blob/master/errors/TransitiveExportError.md for more information, or to contribute content related to this error.

Great, thank you. I was just wondering. :)

"Does this fail with the right type of error, but also the right reason that I am expecting?" is definitely a good thing to check 😄

parsonsmatt · 2017-12-22T04:43:16Z

  | AnonymousArgument
  -- |
-  -- A typed hole that will be turned into a hint/error duing typechecking
+  -- A typed hole that will be turned into a hint/error during typechecking


Minor typo fix

parsonsmatt · 2017-12-22T04:43:28Z

-  -- ^ Available type class dictionaries
+  -- ^ Available type class dictionaries. When looking up 'Nothing' in the
+  -- outer map, this returns the map of type class dictionaries in local
+  -- scope (ie dictionaries brought in by a constrained type).


Updating this based on comments in Slack

parsonsmatt · 2017-12-22T04:56:12Z

+        S.intersection moduleClassDeclarationSet
+          $ untilSame 
+            (\s -> s <> foldMap (\n -> fromMaybe S.empty (M.lookup n classesToSuperClasses)) s)
+            (fromMaybe S.empty (M.lookup qname classesToSuperClasses))


This is going to do a lot of recomputation to find the fixed point of that set.

I suppose you could argue that it's not necessary to obtain transitive superclasses, only immediate ones, as I think this check will fail in the transitive case if and only if it fails in the immediate case. Obtaining transitive dependencies just means we get a more useful error if it does fail, I think?

E.g. If we're exporting C3 here, we also need to export C2 and C1:

module X (class C3) where class C1 a class (C1 a) <= C2 a class (C2 a) <= C3 a

And whether we obtain transitive dependencies here or not, we're going to fail with a transitive export error; obtaining transitive superclasses presumably just means that we have a more useful error, which lists both C2 and C1 as opposed to only C2.

Perhaps as a middle ground we could only obtain immediate superclasses in the first pass, and then if there are any errors, obtain transitive superclasses for the offending classes for the sake of a more detailed error? Does that sound feasible?

That is a really good point -- we're already checking every exported item in the list, so we only need to verify that the immediate superclasses (also defined in the module) are exported. The transitive superclasses defined in module would be caught on when they're being inspected 😄

You're correct that this method does provide a nicer error message, in that you'll get one error message telling you to add all the things to the exports.

We can get "best of both worlds" if we can catch the error and add the lookup for transitive superclasses there before rethrowing.

parsonsmatt · 2017-12-22T04:56:51Z

+    for_ dctors $ \dctors' ->
+      for_ dctors' $ \dctor ->
+        for_ (M.lookup (qualify' dctor) (dataConstructors env)) $ \(_, _, ty, _) -> 
+          checkExport dr extract ty


The inconsistent use of for_ and case made this hard to parse, so I simplified. I can revert if wanted.

This definitely seems like an improvement to me 👍

parsonsmatt · 2017-12-22T04:59:59Z

  go t@(TypeApp t1 t2) = f t <+> go t1 <+> go t2
  go t@(ForAll _ ty _) = f t <+> go ty
-  go t@(ConstrainedType c ty) = foldl (<+>) (f t) (map go (constraintArgs c)) <+> go ty
+  go t@(ConstrainedType c ty) = foldl' (<+>) (f t) (map go (constraintArgs c)) <+> go ty


Maybe unnecessary, but it can't hurt to be sure.

Perhaps it would be better to do this in a separate PR?

Sure thing 🙂

kritzcreek

I'll take a more thorough look later, this already looks like a very good start though!

kritzcreek · 2017-12-22T10:20:50Z

+                  Nothing)
+          $ decls
+      moduleClassDeclarationSet :: S.Set (Qualified (ProperName 'ClassName))
+      moduleClassDeclarationSet = 


Could just use Data.Map.keysSet here

hdgarrood · 2017-12-22T14:04:43Z

  go t@(TypeApp t1 t2) = f t <+> go t1 <+> go t2
  go t@(ForAll _ ty _) = f t <+> go ty
-  go t@(ConstrainedType c ty) = foldl (<+>) (f t) (map go (constraintArgs c)) <+> go ty
+  go t@(ConstrainedType c ty) = foldl' (<+>) (f t) (map go (constraintArgs c)) <+> go ty


Perhaps it would be better to do this in a separate PR?

hdgarrood · 2017-12-22T14:05:05Z

 -- for all implied superclass instances.
 newDictionaries
-  :: MonadState CheckState m
+  :: (MonadState CheckState m, MonadError MultipleErrors m)


Can this change be reverted now?

hdgarrood · 2017-12-22T14:10:18Z

+    for_ dctors $ \dctors' ->
+      for_ dctors' $ \dctor ->
+        for_ (M.lookup (qualify' dctor) (dataConstructors env)) $ \(_, _, ty, _) -> 
+          checkExport dr extract ty


This definitely seems like an improvement to me 👍

hdgarrood · 2017-12-22T15:17:31Z

+        S.intersection moduleClassDeclarationSet
+          $ untilSame 
+            (\s -> s <> foldMap (\n -> fromMaybe S.empty (M.lookup n classesToSuperClasses)) s)
+            (fromMaybe S.empty (M.lookup qname classesToSuperClasses))


I suppose you could argue that it's not necessary to obtain transitive superclasses, only immediate ones, as I think this check will fail in the transitive case if and only if it fails in the immediate case. Obtaining transitive dependencies just means we get a more useful error if it does fail, I think?

E.g. If we're exporting C3 here, we also need to export C2 and C1:

module X (class C3) where class C1 a class (C1 a) <= C2 a class (C2 a) <= C3 a

And whether we obtain transitive dependencies here or not, we're going to fail with a transitive export error; obtaining transitive superclasses presumably just means that we have a more useful error, which lists both C2 and C1 as opposed to only C2.

Perhaps as a middle ground we could only obtain immediate superclasses in the first pass, and then if there are any errors, obtain transitive superclasses for the offending classes for the sake of a more detailed error? Does that sound feasible?

hdgarrood · 2017-12-22T15:23:09Z

    findClasses = everythingOnTypes (++) go
      where
-      go (ConstrainedType c _) = (fmap (TypeClassRef (declRefSourceSpan ref)) . extractCurrentModuleClass . constraintClass) c
+      go (ConstrainedType c _) = (fmap (TypeClassRef (declRefSourceSpan ref)) . extractCurrentModuleClass . constraintClass) c 


Trailing space here

hdgarrood · 2017-12-22T15:23:44Z

@@ -0,0 +1,18 @@
+-- @shouldFailWith TransitiveExportError


Can you paste the full error you get when you attempt to compile this?

parsonsmatt · 2017-12-22T22:42:48Z

Only grabbing transitive dependencies on rethrowing the error:

  1) Failing examples '3132.purs' should fail with 'UnknownName'
       Expected these errors: ["UnknownName"], but got these: ["TransitiveExportError"], full error messages:
       in module Main
       at /home/matt/Projects/purescript/examples/failing/3132.purs line 1, column 1 - line 18, column 7

         An export for class C3 requires the following to also be exported:

           class C1
           class C2



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

hdgarrood · 2017-12-22T23:06:07Z

+  qualify' :: a -> Qualified a
+  qualify' = Qualified (Just mn)
+
+  annotateSuperClasses 


Could you please add a comment summarising why we only obtain transitive superclasses after the immediate check has failed?

hdgarrood · 2017-12-22T23:11:35Z

  -- been exported from the module
  checkClassesAreExported :: DeclarationRef -> m ()
-  checkClassesAreExported ref = checkMemberExport findClasses ref
+  checkClassesAreExported ref = checkMemberExport findClasses  ref


Sorry to nitpick but would you mind reverting this? I'm just trying to keep git blame useful where we can.

Not a problem at all, I should have caught it :)

hdgarrood · 2017-12-22T23:16:38Z

+    unless (null unexportedClasses)
+      . throwError . errorMessage' (declRefSourceSpan dr)
+      . TransitiveExportError dr
+      $ mapMaybe (\n -> M.lookup n classMap) (toList unexportedClasses)


It seems to me that there's a bit of a code smell here as we chuck away the second argument of TransitiveExportError here almost immediately, i.e. once we reach annotateSuperClasses. Have you considered consolidating annotateSuperClasses and this into a single function?

Great catch. Merged.

hdgarrood · 2017-12-22T23:39:23Z

+    classesToSuperClasses <- gets
+      ( M.map
+        ( S.fromList
+        . fmap constraintClass


Rather than performing the intersections below to make sure we only keep local superclasses*, can we do a filter here which inspects the Constraint values and only keeps the superclasses which have been defined in the current module? That way, if I understand correctly, we won't need to construct the moduleClassDeclarations map (which saves us from iterating over all of the declarations in the module).

*that is the reason for the intersections below, right?

okay, that's a much better idea 😂

hdgarrood · 2017-12-22T23:57:20Z

I also just noticed that the source span for the error is a bit dodgy - it seems to cover almost the entire file! I guess ideally it would only point to the part of the export list which is exporting the offending class.

hdgarrood · 2017-12-22T23:58:08Z

Oh, maybe that's a problem in the part of the compiler that elaborates exports though, so perhaps best not to worry about that here.

hdgarrood · 2017-12-23T00:02:12Z

Yeah, the same thing happens with the other types of TransitiveExportError currently, so let's address that separately.

hdgarrood · 2017-12-23T00:31:10Z

+    unless (null unexported)
+      . throwError . errorMessage' (declRefSourceSpan dr)
+      . TransitiveExportError dr
+      $ mapMaybe (\n -> M.lookup n classMap) (toList transitiveSuperClasses)


I think we can get rid of the classMap argument here; the DeclarationRef values in the second argument to TransitiveExportError aren't intended to have real source spans (since they are things which are supposed to be in the exports list but aren't), so I think we can just reuse the one from dr. That is what the other TransitiveExportError checks seem to be doing, too.

Do you think it would work to use map (TypeClassRef (declRefSourceSpan dr)) on this line instead of mapMaybe (\n -> M.lookup n classMap)?

Great call :) That cut out a lot of code.

hdgarrood · 2017-12-24T18:17:28Z

Great! Sorry to keep making suggestions, but now that you've been able to remove all that code I have just one last one: I quite like how typeCheckModule reads for the bit until you get to the first where, and I think it would be nice to preserve that. Could you perhaps extract all the code you've added there into a separate function, so that we end up with something like this?

   warnAndRethrow (addHint (ErrorInModule mn)) $ do
     modify (\s -> s { checkCurrentModule = Just mn })
     decls' <- typeCheckAll mn exps decls
     checkSuperClassesAreExported <- getSuperClassExportCheck
     for_ exps $ \e -> do
       checkTypesAreExported e
       checkClassMembersAreExported e
       checkClassesAreExported e
       checkSuperClassesAreExported e
     return $ Module ss coms mn decls' (Just exps)

parsonsmatt · 2017-12-24T18:34:19Z

I really appreciate the feedback and suggestions 😄

hdgarrood

Looks great, thanks very much!

kritzcreek

Works for me, took me a while to follow the code, but I'd say that's just inherent to the problem being tackled here, lots of indirections.

kritzcreek · 2018-01-20T14:06:08Z

+    classesToSuperClasses <- gets
+      ( M.map
+        ( S.fromList
+        . filter (\(Qualified mn' _) -> mn' == Just mn)


We have a helper for this: filter (isQualifiedWith mn)

hdgarrood · 2018-01-20T15:02:31Z

Thanks @kritzcreek! I think I'll just merge this now to make sure it doesn't rot. Maybe I (or anyone who feels like it) could look through the compiler to find places we can use isQualifiedWith; I imagine this won't be the only one.

parsonsmatt added 10 commits December 18, 2017 23:31

Add test case for purescript#3132

1b8bf7f

Trigger The Bug

1045220

consistent formatting

617c33a

Removing Tri class does not affect mvar

6fe082a

MVar failure only occurs when there are two out of scope classes to r…

25af8a0

…esolve

simplify test cases

91d43a3

mvar error goes away; get 'no type class instance was found....'

6f94f1d

Move to failing with correct error message

b90705e

More interesting behavior

19c5148

When you provide a concrete type that satisfies the instance, all is well. So the bug only triggers when you ask for the instance in the instance context.

parsonsmatt mentioned this pull request Dec 19, 2017

Disallow hiding superclasses of exported classes #3132

Closed

This provides the correct error message for this case.

9c3bce3

parsonsmatt commented Dec 20, 2017

View reviewed changes

parsonsmatt changed the title ~~Fix "Operation blocked on MVar" problem with explicit export lists (#3132)~~ Fix error on missing type class name with non-exported superclass (#3132) Dec 20, 2017

Remove error message display

8ce5fdd

hdgarrood closed this Dec 21, 2017

hdgarrood reopened this Dec 21, 2017

parsonsmatt added 3 commits December 21, 2017 21:42

Fail for the right reason

d33974d

clean warns

fe368da

revert

1712813

parsonsmatt commented Dec 22, 2017

View reviewed changes

formatting

acc17c1

kritzcreek reviewed Dec 22, 2017

View reviewed changes

hdgarrood reviewed Dec 22, 2017

View reviewed changes

parsonsmatt added 4 commits December 22, 2017 08:56

Whitespace, revert some changes

1d5a381

Merge branch 'master' into matt/3132

b813b9e

simplification

0908728

only grab transitive deps if we need them

57f68f9

hdgarrood reviewed Dec 22, 2017

View reviewed changes

parsonsmatt added 2 commits December 22, 2017 17:11

No need to rethrow

2d27b06

Add comment

3cbb47b

hdgarrood reviewed Dec 23, 2017

View reviewed changes

simplification pass

5b11b1c

nice :D

cec7245

hdgarrood approved these changes Dec 24, 2017

View reviewed changes

kritzcreek approved these changes Jan 20, 2018

View reviewed changes

hdgarrood merged commit 950f184 into purescript:master Jan 20, 2018

Conversation

parsonsmatt commented Dec 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

parsonsmatt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

parsonsmatt commented Dec 20, 2017

Uh oh!

hdgarrood commented Dec 20, 2017

Uh oh!

parsonsmatt commented Dec 20, 2017

Uh oh!

hdgarrood commented Dec 20, 2017

Uh oh!

parsonsmatt commented Dec 21, 2017

Uh oh!

hdgarrood commented Dec 21, 2017

Uh oh!

parsonsmatt commented Dec 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kritzcreek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

parsonsmatt commented Dec 19, 2017 •

edited

Loading