Skip to content

Imperative core, initial refactoring#2723

Merged
paf31 merged 3 commits into
masterfrom
phil/core-imp
Mar 12, 2017
Merged

Imperative core, initial refactoring#2723
paf31 merged 3 commits into
masterfrom
phil/core-imp

Conversation

@paf31

@paf31 paf31 commented Mar 10, 2017

Copy link
Copy Markdown
Contributor

This is not what I think the final imperative core should look like, but it does the initial refactoring to move the modules around and rename various things. I would still like to (at least)

  • Separate expressions and statements
  • Add an annotation type argument

but I would prefer to do these things after this PR is merged, so that it doesn't sit and get out of date while we decide what the AST should look like.

Thoughts?

@paf31 paf31 requested a review from garyb March 10, 2017 04:22
@paf31 paf31 changed the title Imperative core, first cut Imperative core, initial refactoring Mar 10, 2017
@michaelficarra

Copy link
Copy Markdown
Contributor

Should a generic imperative core really have constructs like typeof and instanceof? Those concepts are very specific to JS, no? Does the compiler even generate them?

@garyb

garyb commented Mar 10, 2017

Copy link
Copy Markdown
Member

I agree with @michaelficarra, and have a bunch of suggestions of my own, but if the idea is to do this incrementally then I'm fine with merging it as we have it just now. I just have two immediate points:

  • I'd suggest we call the type CoreImp rather than AST, to match CoreFn.
  • I think we might actually need a JS AST as well though, as I'm not sure CoreImp will be trivially printable as JS.

@paf31

paf31 commented Mar 10, 2017

Copy link
Copy Markdown
Contributor Author

Sounds good. I'll try to work on those later.

Yes, I agree, ideally the core language shouldn't use JS constructs like typeof directly, but hopefully we can refactor things from here into a more sensible AST with a well-defined semantics.

@paf31

paf31 commented Mar 11, 2017

Copy link
Copy Markdown
Contributor Author

@garyb It's called Expr in CoreFn. I was thinking we'd break this up into expressions and statements, which is why I chose AST as a generic name for now.

I think we should wait to add a separate JS AST until we know we need it. Allocating a whole new AST might incur a performance penalty, however slight. So, since the two structures are identical right now, I'd like to see if we can do this incrementally instead.

@paf31

paf31 commented Mar 11, 2017

Copy link
Copy Markdown
Contributor Author

I've removed typeof, but we need instanceof for tag checks on ADTs.

@paf31

paf31 commented Mar 11, 2017

Copy link
Copy Markdown
Contributor Author

I've made some progress on breaking the AST into expressions and statements. I tried something a bit different this time - instead of using a separate ADT for each syntactic domain, I just modified the existing AST type by making it into a GADT. It works quite nicely and simplifies the traversals quite a bit.

@michaelficarra

Copy link
Copy Markdown
Contributor

@paf31 Wait, we're still using instanceof for ADT tag checks? I thought we were moving away from that. Something more like value.__tag === Constructor.prototype.__tag has to be faster.

@paf31

paf31 commented Mar 11, 2017

Copy link
Copy Markdown
Contributor Author

@michaelficarra Yes we are, but regardless of whether we use instanceof for JavaScript, we might still need something in the AST to say "extract the tag from this value". We could just always rely on object accessors, but then we lose the ability to use faster representations in other backends.

@michaelficarra

Copy link
Copy Markdown
Contributor

👍 I love it.

@paf31 paf31 merged commit 0f43bb6 into master Mar 12, 2017
@paf31 paf31 deleted the phil/core-imp branch March 12, 2017 22:40
mrkgnao pushed a commit to mrkgnao/purescript that referenced this pull request Mar 13, 2017
* Extract first version of an imperative core from the JS AST

* Remove redundant import

* Remove TypeOf
mrkgnao pushed a commit to mrkgnao/purescript that referenced this pull request Mar 13, 2017
* Extract first version of an imperative core from the JS AST

* Remove redundant import

* Remove TypeOf
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants