Corefn Json representation by coot · Pull Request #3049 · purescript/purescript

coot · 2017-08-19T17:29:26Z

Modify corefn json representation in a way to be able reproduce CoreFn from it.

Polymorphic ModuleT type: when parsing foreign declarations we don't have access to their type, and thus we need a `ModuleT ()` type. The original `Module` type becomes an alias for `ModuleT Type`.

It is required when runnings `moduleToJs`.

coot · 2017-08-20T12:12:52Z

I think this is ready for a review.

paf31 · 2017-08-20T19:49:27Z

I'm wondering if we should add a purs codegen command, which runs the JS codegen on a corefn file. That way, we could implement DCE and other optimizer passes outside the compiler.

Also, @joneshf, do you have suggestions for the corefn JSON format, since we should break them here (or in a separate PR, but before 0.12)?

joneshf · 2017-08-20T22:01:19Z

I'd suggest a separate PR. I don't really know what's happening here and I don't want to do a drive-by on this. I put some comments in the other PR

coot · 2017-08-20T22:14:32Z

This adds

annotations to curefn representation
represents binds as lists rather than objects so the order of recursive binds is preserved
adds moduleFromJSON that reconstructs CoreFn from its json representation (up to the type of foreign declaration.

This allows to pick up CoreFn from its json rep and turn it into javascript code.

coot · 2017-08-21T10:12:12Z

purs codegen would be really useful for writting tools that work with CoreFn in purescript where you just purs compile --dump-corefn; run-some-tool; purs codegen.

- change the json representation of CoreFn - store module name on the module object - `buildMakeAction`: output single module json object, rather an object with a single key (module name) pointing to module json object.

- read the new json CoreFn representation - extend tests so that all the CoreFn structures are tested.

- `moduleToJSON`: include module comments inside `comments` field. - `moduleFromJSON` - add tests

coot · 2017-08-24T17:07:33Z

I update the CoreFn json representation, taking into account comments in #3039
Here is a dump of Data.Newtype.

coot · 2017-08-24T19:33:09Z

I run the zephyr's test suite which compiles quite a few libraries. So the new json representation seems to work quite fine.

Maybe some things could be named better or represented in a better way.
@joneshf, @rightfold any comments?

joneshf

Thanks so much for making these changes! I think it looks great!

joneshf · 2017-08-25T13:37:42Z

src/Language/PureScript/CoreFn/ToJSON.hs

-literalToJSON t (ArrayLiteral xs) = toJSON ("ArrayLiteral", map t xs)
-literalToJSON t (ObjectLiteral xs) = toJSON ("ObjectLiteral", recordToJSON t xs)
+literalToJSON _ (NumericLiteral (Left n))
+  = object


I don't think you need to go through and replace all of this now, but there's an alternative approach which skips over creating the intermediate data structure: https://hackage.haskell.org/package/aeson-1.2.1.0/docs/Data-Aeson.html#g:7

You'd change expressions from

object [ T.pack "foo" .= foo , T.pack "bar" .= bar ]

to expressions like

pairs ( T.pack "foo" .= foo <> T.pack "bar" .= bar )

It's supposedly faster and uses less memory. So given that the json blobs have gotten bigger, it might be a change that makes sense.

@joneshf I think we'd need to add ToJSON instances to make it work (since the right hand side of .= expects an instance of ToJSON. This is a bit more work so I'd like to leave it for later.

You could just encode to Value on the right-hand-side which picks the trivial ToJSON instance.

@kritzcreek you're right, but let's do that in another PR.

joneshf · 2017-08-25T13:40:05Z

src/Language/PureScript/CoreFn/ToJSON.hs

                          , T.pack "foreign"    .= map (identToJSON . fst) (moduleForeign m)
                          , T.pack "decls"      .= map bindToJSON (moduleDecls m)
                          , T.pack "builtWith"  .= toJSON (showVersion v)
+                          , T.pack "comments"   .= map toJSON (moduleComments m)


no-longer-on-githu-b · 2017-09-09T16:30:44Z

I am very happy with this. 👍

coot · 2017-09-09T18:46:52Z

I have more, experimenting in zephyr. I'd like to add type annotations to corefn, would that be fine? (but this can be in another PR).

no-longer-on-githu-b · 2017-09-09T18:48:48Z

Yeah, that's fine, if they're available. I'd like to use them in my Go backend after finishing the purescript-purescript-compiler-backend-utils library. But I don't need them per se (can use interface{} for everything), so I'm fine with them not being there. But maybe other people find them more important.

paf31 · 2017-09-09T22:48:17Z

I'm a little worried about the size of the generated JSON. Part of the reason for the unconventional format before was that I wanted to reduce the size and keep the output somewhat human-readable. Could you please see how many lines are generated for a bigger file? Maybe something like Thermite's main module? 4000+ lines for Data.Newtype seems quite a lot.

paf31 · 2017-09-14T02:57:12Z

@coot Could you please try moving the source file info to the top level?

paf31 · 2017-09-14T19:49:06Z

Was it not possible to just change the JSON renderer for SourcePos?

coot · 2017-09-15T09:54:00Z

I think it was, if you prefer that I'll update the PR. The only reason for having it exposed on the corefn module, is for tooling that might use it - if it's written in haskell and one would liket use the comipler moduleFromJSON. There is no tool for the moment that needs that though and it's very easy to get access to the path anyway - so maybe that's not a good enough reason to keep this.

metaleap · 2017-09-25T14:09:58Z

Looking forward to a more complete (incl types) JSON dump-corefn format! Can't wait for this to land, will it be in 0.12? (The old --dump-imperative-core would be great too but seems it's been postponed edit: just added that one, #3097) Not sure why anyone cares about the file size of an intermediate format to be further machine-processed. @rightfold where's your "Go backend" to be found? I was just about to embark on exactly such hackery, but if it's FOSS I'd rather contribute to yours

no-longer-on-githu-b · 2017-09-25T15:58:11Z

I didn’t start working on the go backend, I want to wait for the new corefn dump to be merged and released. 😆 It will probably be similar to my Python backend https://github.com/rightfold/pegasus Sent from my iPhone.

…

Op 25 sep. 2017 om 16:10 heeft Phil Schumann ***@***.***> het volgende geschreven: Can't wait for this to land, will it be in 0.12? (The old --dump-imperative-core would be great too but seems it's been postponed) Not sure why anyone cares about the file size of an intermediate format to be further machine-processed. @rightfold where's your "Go backend" to be found? I was just about to embark on exactly such hackery, but if it's FOSS I'd rather contribute to yours — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

metaleap · 2017-09-28T18:02:02Z

The latest commit in this PR from 2 weeks ago doesn't build at my end, anything I'm missing? stack is up to date. Well it passed your CI so the prob must be at my end, but I've applied all changes to a fresh unmodified fork of the current purescript/purescript master..

purescript-0.11.6: unregistering (local file changes: purescript.cabal src/Language/PureScript/CodeGen/JS.hs src/Language/PureScript/CoreFn/Desugar.hs ...)
purescript-0.11.6: configure (lib + exe)
Configuring purescript-0.11.6...
purescript-0.11.6: build (lib + exe)
Preprocessing library for purescript-0.11.6..
Building library for purescript-0.11.6..
[100 of 151] Compiling Language.PureScript.CoreFn.Module ( src/Language/PureScript/CoreFn/Module.hs, .stack-work/dist/x86_64-linux/Cabal-2.0.0.2/build/Language/PureScript/CoreFn/Module.o )
[101 of 151] Compiling Language.PureScript.CoreFn.Desugar ( src/Language/PureScript/CoreFn/Desugar.hs, .stack-work/dist/x86_64-linux/Cabal-2.0.0.2/build/Language/PureScript/CoreFn/Desugar.o )

/home/rox/g/purescript-nu-corefn/src/Language/PureScript/CoreFn/Desugar.hs:66:17: error:
    • The constructor ‘A.ValueDeclaration’ should have 1 argument, but has been given 5
    • In the pattern:
        A.ValueDeclaration (ss, com) name _ _ [A.MkUnguarded e]
      In an equation for ‘declToCoreFn’:
          declToCoreFn
            (A.ValueDeclaration (ss, com) name _ _ [A.MkUnguarded e])
            = [NonRec (ssA ss) name (exprToCoreFn ss com Nothing e)]
      In an equation for ‘moduleToCoreFn’:
          moduleToCoreFn env (A.Module modSS coms mn decls (Just exps))
            = let
                imports
                  = mapMaybe importToCoreFn decls ++ fmap ... (findQualModules decls)
                imports' = dedupeImports imports
                ....
              in Module coms mn (spanName modSS) imports' exps' externs decls'
            where
                dedupeImports :: [(Ann, ModuleName)] -> [(Ann, ModuleName)]
                dedupeImports
                  = fmap swap . M.toList . M.fromListWith const . fmap swap
                ssA :: SourceSpan -> Ann
                ssA ss = (ss, [], Nothing, Nothing)
                ....
   |
66 |   declToCoreFn (A.ValueDeclaration (ss, com) name _ _ [A.MkUnguarded e]) =
   |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

/home/rox/g/purescript-nu-corefn/src/Language/PureScript/CoreFn/Desugar.hs:195:12: error:
    • The constructor ‘A.TypeInstanceDeclaration’ should have 8 arguments, but has been given 6
    • In the pattern: A.TypeInstanceDeclaration _ _ _ q _ _
      In an equation for ‘fqDecls’:
          fqDecls (A.TypeInstanceDeclaration _ _ _ q _ _) = getQual' q
      In an equation for ‘findQualModules’:
          findQualModules decls
            = let
                (f, _, _, _, _)
                  = everythingOnValues
                      (++) fqDecls fqValues fqBinders (const ...) (const ...)
              in f `concatMap` decls
            where
                fqDecls :: A.Declaration -> [ModuleName]
                fqDecls (A.TypeInstanceDeclaration _ _ _ q _ _) = getQual' q
                fqDecls (A.ValueFixityDeclaration _ _ q _) = getQual' q
                fqDecls (A.TypeFixityDeclaration _ _ q _) = getQual' q
                fqDecls _ = []
                fqValues :: A.Expr -> [ModuleName]
                fqValues (A.Var q) = getQual' q
                fqValues (A.Constructor q) = getQual' q
                fqValues (A.TypeClassDictionaryConstructorApp c _) = getQual' c
                fqValues _ = []
                ....
    |
195 |   fqDecls (A.TypeInstanceDeclaration _ _ _ q _ _) = getQual' q
    |            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

--  While building package purescript-0.11.6 using:
      /home/rox/.stack/setup-exe-cache/x86_64-linux/Cabal-simple_mPHDZzAJ_2.0.0.2_ghc-8.2.1 --builddir=.stack-work/dist/x86_64-linux/Cabal-2.0.0.2 build lib:purescript exe:purs --ghc-options " -ddump-hi -ddump-to-file"

Just tried to begin using this new --dump-corefn JSON representation as this PR has been just sitting here for some 2-5 weeks depending on how one counts 😀 and I really need to dump more type-related meta-data than the current version does!

coot · 2017-09-28T21:52:24Z

@metaleap I merged master. It should be compile now for you. Try this branch rather than applying a patch to master.

metaleap · 2017-09-28T22:48:58Z

That was the ticket for me to build at my end, thanks, now I can begin toying with this =)

paf31 · 2017-10-01T19:10:01Z

src/Language/PureScript/CoreFn/Module.hs

  , moduleImports :: [(a, ModuleName)]
  , moduleExports :: [Ident]
-  , moduleForeign :: [ForeignDecl]
+  , moduleForeign :: [ForeignDeclT t]


Could you please add a comment explaining why this type argument is needed?

paf31 · 2017-10-01T19:15:09Z

@coot @rightfold @joneshf @kritzcreek @natefaubion Any more comments or shall I merge this now?

paf31

Looks great to me! Thanks for all of the work on this.

coot · 2017-10-01T19:27:13Z

My pleasure!

kritzcreek

Looks great, I made a few comments, but they are all optional suggestions.

kritzcreek · 2017-10-01T19:25:05Z

src/Language/PureScript/CoreFn/FromJSON.hs

+  case t of
+    "ProductType" -> return ProductType
+    "SumType"     -> return SumType
+    _             -> fail ("not regonized ConstructorType: " ++ T.unpack t)


small typo here "regonized"

kritzcreek · 2017-10-01T19:47:06Z

src/Language/PureScript/CoreFn/FromJSON.hs

+  bindFromObj o = do
+    type_ <- o .: "bindType" :: Parser Text
+    case type_ of
+      "NonRec"  -> (uncurry . uncurry $ NonRec) <$> bindFromObj' o


(uncurry . uncurry) NonRec

Would be a little less noisy

kritzcreek · 2017-10-01T19:49:56Z

src/Language/PureScript/CoreFn/ToJSON.hs

-literalToJSON t (ArrayLiteral xs) = toJSON ("ArrayLiteral", map t xs)
-literalToJSON t (ObjectLiteral xs) = toJSON ("ObjectLiteral", recordToJSON t xs)
+literalToJSON _ (NumericLiteral (Left n))
+  = object


You could just encode to Value on the right-hand-side which picks the trivial ToJSON instance.

LiamGoodacre · 2017-10-01T22:59:56Z

src/Language/PureScript/CoreFn/Module.hs

 -- The CoreFn module representation
 --
+-- The json CoreFn representation does not contain type information.  When
+-- parsing it one gets back `ModuleT () Ann` rathern than `ModuleT Type Ann`,


Typo: rathern

paf31 · 2017-10-02T00:11:25Z

src/Language/PureScript/CoreFn/Module.hs

 -- |
 -- The CoreFn module representation
 --
+-- The json CoreFn representation does not contain type information.  When


It looks like we don't even use the types in moduleForeign. If that's the case, I'd actually rather just get rid of the types completely and lose the type argument.

As long as they're kept around in externs, don't drop them there! (Just chiming in as I'm wrestling with an alternative backend not targeting a dynamic/untyped language like JS/Py/Erl etc ... from what I can tell I'll have to reconstruct a lot from literals & externs & such as I'm working off CoreImp that I dump from the final rawJS --- to get all the optimizations and the basic fn-to-imp transforms.) Just a quick reminder to not forget the awesome use-case for alternative backends, some of which may want to keep around as much type info as possible, to minimize usage of boxed-variants (ie what is object in Java or interface{} in Go, etc) being passed around.

In my fork I experimentally added all Anns to your --dump-corefn module, to see what I can get beyond what's in externs. Not that much more but last I checked, it did have a bit more detail. A bit exploratory all right now, to write a backend based on --dump-corefoo outputs rather than right in Haskell in the fork, but it's fun for now. All right I'm done blogging & derailing the thread 😀

As far as I can tell, we don't have any type information in the generated JSON right now.

joneshf · 2017-10-02T01:18:19Z

This is great! I'm with it.

paf31 · 2017-10-02T02:23:19Z

Let's merge this and fix up any remaining issues in master if we find any. Thanks!

@paf31

* CoreFn.FromJSON: parse json representation of CoreFn * CoreFn.Module: ModuleT Polymorphic ModuleT type: when parsing foreign declarations we don't have access to their type, and thus we need a `ModuleT ()` type. The original `Module` type becomes an alias for `ModuleT Type`. * Language.PureScript.CoreFn.DCE module * CoreFn: include `SourceSpan`s in json representation * CoreFn: add Meta to json representation It is required when runnings `moduleToJs`. * Update TestCoreFn * Change json encoding of CoreFn Bind * Remove `purs dce` command. * Remove CoreFN.DCE module * clean * remove white space in TestCoreFn * Object CoreFn representation. * CoreFn moduleToJSON: new representation - change the json representation of CoreFn - store module name on the module object - `buildMakeAction`: output single module json object, rather an object with a single key (module name) pointing to module json object. * CoreFn.FromJSON: update `moduleFromJson` - read the new json CoreFn representation - extend tests so that all the CoreFn structures are tested. * Add Module Comments to json CoreFn representation - `moduleToJSON`: include module comments inside `comments` field. - `moduleFromJSON` - add tests * CoreFn json repr: modulePath to the top level * Applied @paf31 & @kritzcreek review

paulyoung · 2017-10-22T20:11:21Z

#3049 (comment)

I'm wondering if we should add a purs codegen command, which runs the JS codegen on a corefn file.

I was just wondering if it was possible to use --dump-corefn without producing JavaScript.

coot · 2017-10-22T20:39:21Z

I think that `--dump-corefn` shouldn't generate javascript at all.

…

On 20:11 Sun 22 Oct , Paul Young wrote: #3049 (comment) > I'm wondering if we should add a `purs codegen` command, which runs the JS codegen on a corefn file. I was just wondering if it was possible to use `--dump-corefn` without producing JavaScript. -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #3049 (comment)

Marcin Szamotulski added 11 commits August 19, 2017 07:57

CoreFn.Module: ModuleT

61c4d82

Polymorphic ModuleT type: when parsing foreign declarations we don't have access to their type, and thus we need a `ModuleT ()` type. The original `Module` type becomes an alias for `ModuleT Type`.

CoreFn.FromJSON: parse json representation of CoreFn

d992961

Language.PureScript.CoreFn.DCE module

fa03859

CoreFn: include SourceSpans in json representation

e413924

CoreFn: add Meta to json representation

987e640

It is required when runnings `moduleToJs`.

Update TestCoreFn

10c3a1c

Change json encoding of CoreFn Bind

de4af05

Remove purs dce command.

d6c6946

Remove CoreFN.DCE module

de5728d

clean

be187b4

remove white space in TestCoreFn

c225e7a

coot mentioned this pull request Aug 22, 2017

WIP: dce on CoreFn #3039

Closed

Marcin Szamotulski added 4 commits August 24, 2017 08:51

Object CoreFn representation.

7f8e74d

CoreFn moduleToJSON: new representation

f2a337f

- change the json representation of CoreFn - store module name on the module object - `buildMakeAction`: output single module json object, rather an object with a single key (module name) pointing to module json object.

CoreFn.FromJSON: update moduleFromJson

15374ea

- read the new json CoreFn representation - extend tests so that all the CoreFn structures are tested.

Add Module Comments to json CoreFn representation

a986e4f

- `moduleToJSON`: include module comments inside `comments` field. - `moduleFromJSON` - add tests

joneshf reviewed Aug 25, 2017

View reviewed changes

paf31 added the status: ready for review label Sep 9, 2017

paf31 mentioned this pull request Sep 9, 2017

Typed CoreFn WIP #2658

Closed

CoreFn json repr: modulePath to the top level

d1f6f84

Merge branch 'master' into corefn-json

a99284b

paf31 reviewed Oct 1, 2017

View reviewed changes

paf31 approved these changes Oct 1, 2017

View reviewed changes

kritzcreek approved these changes Oct 1, 2017

View reviewed changes

Applied @paf31 & @kritzcreek review

d080d58

LiamGoodacre reviewed Oct 1, 2017

View reviewed changes

paf31 reviewed Oct 2, 2017

View reviewed changes

paf31 merged commit d31638f into purescript:master Oct 2, 2017

coot deleted the corefn-json branch October 2, 2017 18:29

gabejohnson mentioned this pull request Nov 28, 2017

Determining the location of FFI files by module name #3162

Closed

paulyoung mentioned this pull request Jan 9, 2018

Add flag to disable Javascript backend #3196

Closed

gabejohnson mentioned this pull request Feb 26, 2018

Prevent codegen when --dump-corefn is supplied #3258

Closed

garyb mentioned this pull request Apr 29, 2018

Bump version for v0.12.0-rc1 #3340

Merged

paulyoung mentioned this pull request May 16, 2020

Fix CoreFn FromJSON version parsing and add test #3877

Merged

Conversation

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

Uh oh!

Uh oh!

coot commented Aug 20, 2017

Uh oh!

paf31 commented Aug 20, 2017

Uh oh!

joneshf commented Aug 20, 2017

Uh oh!

coot commented Aug 20, 2017

Uh oh!

coot commented Aug 21, 2017

Uh oh!

coot commented Aug 24, 2017

Uh oh!

coot commented Aug 24, 2017

Uh oh!

joneshf 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!

no-longer-on-githu-b commented Sep 9, 2017

Uh oh!

coot commented Sep 9, 2017

Uh oh!

no-longer-on-githu-b commented Sep 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

paf31 commented Sep 9, 2017

Uh oh!

paf31 commented Sep 14, 2017

Uh oh!

paf31 commented Sep 14, 2017

Uh oh!

coot commented Sep 15, 2017

Uh oh!

metaleap commented Sep 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

no-longer-on-githu-b commented Sep 25, 2017 via email

Uh oh!

metaleap commented Sep 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coot commented Sep 28, 2017

Uh oh!

metaleap commented Sep 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paf31 commented Oct 1, 2017

Uh oh!

paf31 left a comment

Choose a reason for hiding this comment

Uh oh!

coot commented Oct 1, 2017

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!

coot commented Aug 19, 2017 •

edited

Loading

no-longer-on-githu-b commented Sep 9, 2017 •

edited

Loading

metaleap commented Sep 25, 2017 •

edited

Loading

metaleap commented Sep 28, 2017 •

edited

Loading

metaleap Oct 2, 2017 •

edited

Loading