Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Imperative core, initial refactoring #2723

Merged
merged 3 commits into from
Mar 12, 2017
Merged

Imperative core, initial refactoring #2723

merged 3 commits into from
Mar 12, 2017

Conversation

paf31
Copy link
Contributor

@paf31 paf31 commented Mar 10, 2017

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
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
Copy link
Member

garyb commented Mar 10, 2017

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
Copy link
Contributor Author

paf31 commented Mar 10, 2017

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
Copy link
Contributor Author

paf31 commented Mar 11, 2017

@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
Copy link
Contributor Author

paf31 commented Mar 11, 2017

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

@paf31
Copy link
Contributor Author

paf31 commented Mar 11, 2017

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
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
Copy link
Contributor Author

paf31 commented Mar 11, 2017

@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
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