Solving IsSymbol instances by LiamGoodacre · Pull Request #2429 · purescript/purescript

LiamGoodacre · 2016-11-15T01:08:15Z

Opening this PR for discussion: there are some TODOs and other things.

Currently many of the example tests fail since I updated tests/support/bower.json to specify the current latest versions of libraries. They fail with The foreign module implementation for module Unsafe.Coerce is missing.. I added purescript-unsafe-coerce as a dependency, but it didn't help. So I'm not sure why that fails.

However my SolvingIsSymbol test does pass 👍

paf31 · 2016-11-15T05:36:07Z

src/Language/PureScript/TypeClassDictionaries.hs

  = TypeClassDictionaryInScope {
    -- | The identifier with which the dictionary can be accessed at runtime
-      tcdName :: Qualified Ident
+      tcdName :: n


Could we just make this into an Expr?

I think I tried something like that previously (think it was Either Expr (Qualified Ident)) but it caused a module dependency cycle. I'll have another look later today.

Makes sense, thanks!

paf31 · 2016-11-15T05:36:44Z

src/Language/PureScript/TypeChecker/Entailment.hs

  -- ^ Should the solver be allowed to defer errors by skipping constraints?
  }

+-- TODO: Where would this live?


I think it's fine here. I was planning to move the deriving logic here later too, to get around the type synonym restrictions.

paf31 · 2016-11-15T05:39:46Z

Looks great, thanks!

Yes, you'll have some issues with library versions if you update them all to v2.0 like that. There is an issue to track the update that needs to happen (#2400) but it's probably quite a bit of work. Maybe you can avoid it for now?

LiamGoodacre · 2016-11-15T08:04:01Z

In that case I should probably use purescript-symbols at version 1.0.1 and drop the TypeConcat example.

rvion · 2016-11-15T08:35:17Z

examples/passing/SolvingIsSymbol.purs

+import Control.Monad.Eff.Console
+
+main = do
+  log (reflectSymbol (SProxy :: SProxy "literal"))


How about checking the result too ?

let a = (reflectSymbol (SProxy :: SProxy "literal")) b = (reflectSymbol (SProxy :: SProxy (TypeConcat "Hello" "World"))) when (a == "literal" && b == "HelloWorld") (log "Done")

Sounds good, thanks

(also I didn't realise outputting Done was interpretted as 'test passing')

LiamGoodacre · 2016-11-15T08:53:41Z

src/Language/PureScript/TypeChecker/Entailment.hs

+      [ ("reflectSymbol", Abs (Left (Ident C.__unused)) (Literal (StringLiteral sym)))
+      ]
+    classTy = TypeConstructor (fmap coerceProperName C.IsSymbol)
+    expr = TypedValue False (Literal inst) (TypeApp classTy (TypeLevelString sym))


I wasn't sure about TypedValue - have I done this correctly? I didn't work out what the boolean argument was for and I didn't see it mentioned in the doc-comment for the constructor.

Do we need TypedValue here? I think it can be removed actually.

It appeared to be required by exprToCoreFn in src/Language/PureScript/CoreFn/Desugar.hs here:

exprToCoreFn ss com _ (A.TypeClassDictionaryConstructorApp name (A.TypedValue _ (A.Literal (A.ObjectLiteral vs)) _)) =

Otherwise it hits the error case:

exprToCoreFn _ _ _ e = error $ "Unexpected value in exprToCoreFn mn: " ++ show e

Maybe you could add a case for

exprToCoreFn ss com _ (A.TypeClassDictionaryConstructorApp name (A.Literal (A.ObjectLiteral vs))) =

or use a pattern guard here then?

LiamGoodacre · 2016-11-15T20:11:00Z

@paf31: making TypeClassDictionaryInScope store the Expr is going to require some reorganising of module dependencies.

The following is a trimmed down version of our modules and their dependencies:

TypeClassDictionaries

Environment
  TypeClassDictionaries

AST.Declarations
  TypeClassDictionaries
  Environment

AST
  AST.Declarations

Externs
  AST
  Environment
  TypeClassDictionaries

TypeChecker.Monad
  Environment
  TypeClassDictionaries

TypeChecker.Entailment
  AST
  Environment
  TypeChecker.Monad
  TypeClassDictionaries

TypeChecker.Types
  TypeChecker.Entailment

TypeChecker
  AST
  Environment
  TypeChecker.Monad
  TypeChecker.Types
  TypeClassDictionaries

We need TypeClassDictionaries to depend on Expr which is defined in AST.Declarations. This causes a cycle between AST.Declarations, Environment, and TypeClassDictionaries.

LiamGoodacre · 2016-11-15T20:22:45Z

An alternative might be to remove the name/expr from TypeClassDictionaryInScope and just pair the dict up with the Expr where needed. I'll have a think about it.

LiamGoodacre · 2016-11-15T20:27:53Z

Although that is actually the same solution as what I've already done with adding the type parameter.

paf31 · 2016-11-17T03:21:46Z

@LiamGoodacre I think we should stick with the original approach actually, using a sum type, now that I understand the problem properly (sorry!)

But let's rename some things to make them more understandable:

tcdName should be named something like tcdValue since it's more general now.
NamedTypeClassDictionaryInScope is a bit wordy. How about just NamedDict or something?

paf31 · 2016-11-17T03:22:20Z

src/Language/PureScript/TypeClassDictionaries.hs

    , tcdDependencies :: Maybe [Constraint]
    }
-    deriving (Show)
+    deriving (Show, Functor)


Might as well derive the usual Functor, Foldable and Traversable here.

paf31 · 2016-11-17T03:24:11Z

A third option is a sum type like

data Evidence
  = NamedDictionary Ident
  | IsSymbolDictionary String

right?

paf31 · 2016-11-17T04:07:56Z

I can confirm that this makes my generics-rep PR build successfully.

I did run into one weird issue though. If I don't import Data.Symbol (and IsSymbol specifically) in my test suite, then I get the following error:

psc: An internal error ocurred during compilation: "Missing value in mnLookup"

paf31 · 2016-11-19T22:19:28Z

I think there is relatively little needed to push this over the finish line, right? Just

Switching back to a sum type instead of using Expr directly.
Dealing with the TypedValue issue.
Maybe some renamings here and there (tcdName in particular)

I think once this is merged, we should release 0.10.3, since it will enable some nice library features which depend on generics-rep deriving (among others).

LiamGoodacre · 2016-11-20T16:39:38Z

Hey thanks for the review. I've been away without internet for a few days so I haven't had a chance to work on this. I should be able to get this done today/tomorrow though 👍

LiamGoodacre · 2016-11-20T21:29:06Z

I've been looking into the Missing value in mnLookup issue from the renameModules part of moduleToJs. This occurs because the dictionaries are created inline and use the type class constructor (which hasn't necessarily been imported). The easiest way to fix this is to not use the constructor and just have the object literal - is that okay to do? This also means that I don't need to change exprToCoreFn.

garyb · 2016-11-20T21:40:15Z

I would say no, not really. The dictionaries are not going to be records when the typechecker gets fixed for #928.

edit: #2205 is a newer version of that actually.

LiamGoodacre · 2016-11-20T21:56:17Z

@garyb yeah that makes sense, thanks. So we'll need to implicitly import the type class constructor during code gen - what would be the preferred way of doing that?

garyb · 2016-11-20T22:00:43Z

Hmm, that's a tricky one actually... I'm not too sure! Probably as part of the CoreFn desugaring process, if we can figure it out there? There used to be some extra import elaboration done in there, but it was removed after it got figured out propery during name desugaring instead. Pretty sure we won't be able to do it that early on in this case though, right?

LiamGoodacre · 2016-11-20T23:23:41Z

src/Language/PureScript/CoreFn/Desugar.hs

  fqValues :: A.Expr -> [ModuleName]
  fqValues (A.Var q) = getQual' q
  fqValues (A.Constructor q) = getQual' q
+  fqValues (A.TypeClassDictionaryConstructorApp C.IsSymbol _) = getQual' C.IsSymbol


This lets us implicitly add an import of Data.Symbol if the IsSymbol class is used. The import also gets deduped later if it's already there.
Is this a good place to add this?

Perhaps it's not great to explicitly mention IsSymbol here: it means we'd have to update multiple places if we're to do similar instance solving in the future. But it's possibly odd to do it for all constructor applications?

Could you please add a comment here explaining why this line is necessary?

paf31 · 2016-11-21T20:33:50Z

src/Language/PureScript/TypeChecker/Entailment.hs

 import qualified Language.PureScript.Constants as C

+type TypeClassDict =
+  TypeClassDictionaryInScope (Either Expr (Qualified Ident))


I think I'd rather use a sum type here, with a constructor for named instances, and one for symbol dictionaries. Then instead of using either id in mkDictionary, you would pattern match, and include the instance generation code there. isSymbolDictionary would just apply the appropriate constructor.

paf31 · 2016-11-21T20:34:30Z

src/Language/PureScript/TypeChecker/Entailment.hs

  }

+-- | Build a type class instance dictionary for the `IsSymbol` class for a symbol
+isSymbolDictionary :: String -> TypeClassDict


I originally read this as a question: "is this a symbol dictionary?" 😄

Maybe something like makeIsSymbolDictionary would be better?

LiamGoodacre · 2016-11-21T22:10:02Z

Currently updating the test example so that it depends on the implicit importing.

LiamGoodacre · 2016-11-21T23:06:52Z

Okay I'm confused: the test is passing on some build jobs and failing on others 😕.

paf31 · 2016-11-22T02:12:16Z

I think you need to update the cabal file.

paf31 · 2016-11-23T00:36:06Z

Nice work!

paf31 added the status: ready for review label Nov 15, 2016

paf31 reviewed Nov 15, 2016

View reviewed changes

rvion reviewed Nov 15, 2016

View reviewed changes

LiamGoodacre commented Nov 15, 2016

View reviewed changes

paf31 reviewed Nov 17, 2016

View reviewed changes

Solve IsSymbol instances

abb1aa0

IsSymbol dict with constructor application

63e1dc3

LiamGoodacre commented Nov 20, 2016

View reviewed changes

paf31 reviewed Nov 21, 2016

View reviewed changes

IsSymbol solving with Evidence type & comments

36f4560

Update IsSymbol tests to depend on implicit Data.Symbol import

7bd2b3e

paf31 merged commit 1e6fc9d into purescript:master Nov 23, 2016

LiamGoodacre deleted the feature/is-symbol-instances branch November 23, 2016 08:38

Conversation

LiamGoodacre commented Nov 15, 2016

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!

paf31 commented Nov 15, 2016

Uh oh!

LiamGoodacre commented Nov 15, 2016

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!

LiamGoodacre commented Nov 15, 2016

Uh oh!

LiamGoodacre commented Nov 15, 2016

Uh oh!

LiamGoodacre commented Nov 15, 2016

Uh oh!

paf31 commented Nov 17, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paf31 commented Nov 17, 2016

Uh oh!

paf31 commented Nov 17, 2016

Uh oh!

paf31 commented Nov 19, 2016

Uh oh!

LiamGoodacre commented Nov 20, 2016

Uh oh!

LiamGoodacre commented Nov 20, 2016

Uh oh!

garyb commented Nov 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LiamGoodacre commented Nov 20, 2016

Uh oh!

garyb commented Nov 20, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LiamGoodacre Nov 20, 2016 • 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!

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!

LiamGoodacre commented Nov 21, 2016

Uh oh!

LiamGoodacre commented Nov 21, 2016

Uh oh!

paf31 commented Nov 22, 2016

Uh oh!

paf31 commented Nov 23, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

garyb commented Nov 20, 2016 •

edited

Loading

LiamGoodacre Nov 20, 2016 •

edited

Loading