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

Infix progress #162

Merged
merged 20 commits into from Jan 2, 2014
Merged

Infix progress #162

merged 20 commits into from Jan 2, 2014

Conversation

natefaubion
Copy link
Contributor

This is an ongoing PR to track the progress of infix macros. Please chime in at any point. I've broken it down into goals that I'll periodically update so you can see how far along it is:

  • Refactor the macro macro to translate infix rules into infix cases.
  • Refactor syntaxCase to understand the infix form and matching semantics.
  • Add "reverse" support to the pattern matcher (return a shallow-reversed pattern tree)
  • Plumb together enforest and expandTermTree to keep track of previous syntax/terms.
  • Plumb the previous syntax/terms into the macro transformers.
  • Rewrite enforestVarStatement so we can lookbehind match on var assignment
  • Fix withSyntax which is broken for some reason
  • Fix for-let enforestation/hygiene
  • Test cases

For the time being, I'm using the following form for infix macros because it's the easiest to parse (anything before the bar is lookbehind, anything after is business as usual):

macro foo {
  rule infix { $lhs | $rhs } => { ... }
  case infix { $lhs | $name $rhs } => { ... }
}

Another proposed forms for infix cases:

macro foo {
  rule infix { $lhs | $rhs } => { ... }
  case infix { $lhs |$name| $rhs } => { ... }
}

And @disnet liked the haskelly:

macro foo {
  rule infix { $lhs | $rhs } => { ... }
  case infix { $lhs `$name` $rhs } => { ... }
}

@jlongster
Copy link
Contributor

This is awesome! I'll see if I can grasp the code quick enough to help...

@natefaubion
Copy link
Contributor Author

It works well enough to at least play with. The main hitch is currently var enforestation. Trying to do lookbehind matching on the rhs of a var assignment does not currently work. Tests fail on building withSyntax for some reason.

@disnet I'm currently explicitly passing prevStx and prevTerms around as function arguments and as part of the result object. I went with that because I didn't want to bother with making sure I clean the context when going into child delims. It also lets me optimize the destructed syntax for terms on lookbehind matching easily since I can have a reference to the before and after (it's actually required currently, as TermTree::destruct is not pure in some cases). Let me know if you see a more convenient way.

@natefaubion
Copy link
Contributor Author

@disnet Here's a problem I'm having with var enforestation. Let's take a normal macro:

macro str {
  rule {} => { .toString() }
}

var foo = true str;

This gives you the expected result of var foo = true.toString(); but not for the reasons you'd think at first glance. enforestVarStatement only does a single enforest on the rhs of an assignment to get a single Expr term (though right now it doesn't actually check if its an expression). The problem is, true str is not a single Expr term. It gets enforested to:

var foo = true str;
// -->
<VariableStatement foo = true> str;
// --->
<VariableStatement foo = true> <Punc .> <Call toString()> <Punc ;>

Again, this isn't really a problem for normal macros, as it all works out in the end to correct syntax. The problem is with lookbehind matching, we only want to match on complete term trees to avoid senseless code. So say I turn this into a postfix macro:

macro str {
  rule infix { $lhs:expr | } => { myToString($lhs) }
}

var foo = true str;
// -->
<VariableStatement foo = true> str;
// --> Match error

It will result in a match error, because we'd be splitting a term tree if we took the true. Now, you could say that we can just take everything until we get to a ; or , and then work it out, but you also have to take ASI into consideration:

var foo = true
str;

is a valid invocation of this macro if we just slurp until we get to a ; or ,, though it probably shouldn't be.

So I guess the question is, what do you think we should do in the case of a macro after the initial rhs term which could potentially "extend" the expression? Without lookbehind, it wan't a problem, but with lookbehind you could potentially match on the extension, when it should be part of the previous expression.

@natefaubion
Copy link
Contributor Author

This is a macro expansion that seems to make sense to me:

macro plus {
    rule infix { $lhs:expr | rhs:expr } {
        $lhs + $rhs
    }
}

// Now
var foo = 12 plus 4;
<VariableStatement: var foo = 12> plus 4 ;
Error: No match (term split)

// Ideal?
var foo = 12 plus 4;
<Keyword: var> foo = 12 plus 4 ;
<Keyword: var> <VariableAssignment: foo => 12 plus 4 ;
<Keyword: var> <VariableAssignment: foo => <Lit: 12> plus 4 ;
<Keyword: var> <VariableAssignment: foo => <Lit: 12> plus <Lit: 4> ;
<Keyword: var> <VariableAssignment: foo => 12 + 4 ;
<Keyword: var> <VariableAssignment: foo => <Lit: 12> + 4 ;
<Keyword: var> <VariableAssignment: foo => <Lit: 12> + <Lit: 4> ;
<Keyword: var> <VariableAssignment: foo => <Binop: 12 + 4> ;
<VariableStatement: var foo = 12 + 4> <Punc: ;>

In this, we keep enforesting the lhs assignment until there isn't a reason to anymore (until it stops forming expressions), instead of just running enforest once. VariableAssignment is the lhs ident and = because we want people to be able to match on the ident, but not split the ident and =.

I think this is actually what you want for my trivial str case above.

macro str {
  rule infix { } => { .toString() }
}

var foo = true str;
<Keyword: var> foo = true str ;
<Keyword: var> <VariableAssignment: foo => true str ;
<Keyword: var> <VariableAssignment: foo => <Lit: true> str ;
<Keyword: var> <VariableAssignment: foo => <Lit: true> . toString () ;
<Keyword: var> <VariableAssignment: foo => <ObjDotGet: true . toString> () ;
<Keyword: var> <VariableAssignment: foo => <Call: true . toString ()> ;
<VariableStatement: foo = true.toString()> <Punc: ;>

Does this seem like it makes sense?

Edit: Here's a more complete example with commas

var foo = 12 plus 4,
    bar = 42 plus 1;
<Keyword: var> foo = 12 plus 4 , bar = 42 plus 1 ;
<Keyword: var> <VariableAssignment: foo => 12 plus 4 , bar = 42 plus 1 ;
<Keyword: var> <VariableAssignment: foo => <Lit: 12> plus 4 , bar = 42 plus 1 ;
<Keyword: var> <VariableAssignment: foo => <Lit: 12> plus <Lit: 4> , bar = 42 plus 1 ;
<Keyword: var> <VariableAssignment: foo => 12 + 4 , bar = 42 plus 1 ;
<Keyword: var> <VariableAssignment: foo => <Lit: 12> + 4 , bar = 42 plus 1 ;
<Keyword: var> <VariableAssignment: foo => <Lit: 12> + <Lit: 4> , bar = 42 plus 1 ;
<Keyword: var> <VariableAssignment: foo => <Binop: 12 + 4> , bar = 42 plus 1 ;
<Keyword: var> <VariableDeclaration: foo = 12 + 4> <Punc: ,> bar = 42 plus 1 ;
<Keyword: var> <VariableDeclaration: foo = 12 + 4> <Punc: ,> <VariableAssignment: bar => 42 plus 1 ;
<Keyword: var> <VariableDeclaration: foo = 12 + 4> <Punc: ,> <VariableAssignment: bar => <Lit: 42> plus 1 ;
<Keyword: var> <VariableDeclaration: foo = 12 + 4> <Punc: ,> <VariableAssignment: bar => <Lit: 42> plus <Lit: 1> ;
<Keyword: var> <VariableDeclaration: foo = 12 + 4> <Punc: ,> <VariableAssignment: bar => 42 + 1 ;
<Keyword: var> <VariableDeclaration: foo = 12 + 4> <Punc: ,> <VariableAssignment: bar => <Lit: 42> + 1 ;
<Keyword: var> <VariableDeclaration: foo = 12 + 4> <Punc: ,> <VariableAssignment: bar => <Lit: 42> + <Lit: 1> ;
<Keyword: var> <VariableDeclaration: foo = 12 + 4> <Punc: ,> <VariableAssignment: bar => <BinOp: 42 + 1> ;
<Keyword: var> <VariableDeclaration: foo = 12 + 4> <Punc: ,> <VariableDeclaration: bar = 42 + 1> ;
<VariableStatement: var foo = 12 + 4 , bar = 42 + 1> <Punc: ;>

This would all happen in enforestVarStatement. I don't think you would want an infix macro to be able to extend beyond the original var when matching on the lhs because vars are a bit like an implicit delimiter.

@disnet
Copy link
Member

disnet commented Dec 19, 2013

I've been traveling for the holidays so haven't had time to completely grok this yet but I think your plan of enforesting the assignment until you reach a fixedpoint and having var act like a delimiter for infix macros seems right to me.

@disnet
Copy link
Member

disnet commented Dec 19, 2013

In:

<Keyword: var> <VariableAssignment: foo => 12 plus 4 ;

why do we want to allow the macro to match past the =? By analogy plus in (function(foo) {})(12 plus 4) doesn't know it's inside a function call. Can we just treat = in a var assignment (and , with multiple decls) as an implicit delimiter start and not allow an infix macro to match past it?

@natefaubion
Copy link
Contributor Author

The reason I wanted to match past it is because you could potentially do some cool transformations that require more context of the surrounding scope without having to override common keywords like function or var as macros.

Here is an obviously deficient macro, but you get the idea:

macro async {
  rule infix { var $name:ident = | $call:expr $rest ... } => {
    $call.then(function($name) {
      $rest ...
    })
  }
}

But maybe its too much? Maybe it doesn't give you enough to really implement ideas like that correctly? I just wanted a potential solution for having more expressive macros that don't require tedious macro composition if you use more than one.

`enforestVarStatement` is now greedy when it comes to enforesting
expressions on the rhs of an assignment. This allows us to juxtapose
an expression and a macro that might extend the expression.

Additionally, it fixes a longstanding bug wherein the lhs identifier
was being enforested, potentially causing macro expansion.
@natefaubion
Copy link
Contributor Author

So I decided to not allow lookbehind matching on the = and identifier in a var statement. The implementation would be extremely complicated and would likely be very problematic. I may play around with it again at some point, though.

enforestVarStatement is now greedy and will keep enforesting the rhs as long as it returns an expression. This lets us implement the arrow function macro as described in #159.

I went ahead and fixed the bug in #120 wherein the rhs identifier was being enforested. I also changed it so the = is no longer enforested, but I can revert this if you want.

There are just a couple more things left to cleanup, so hopefully I'll get it finished up this weekend, and we can start testing like crazy, as I'm sure there are many bugs. If you want to check out this branch, you can run a build and play around with it.

@jlongster
Copy link
Contributor

Should this work yet?

macro => {
    rule infix { $args | $body } => {
        function $args $body
    }
}

(x) => { x * x };

Doesn't seem to match:

% ./bin/sjs t.sjs
x$279 => {
    x$279 * x$279;
};

The => does match if I don't do infix and use it for other things.

@natefaubion
Copy link
Contributor Author

@jlongster it should work now.

Here's a complete macro for arrow functions:

macro => {
  rule infix { $arg:ident | { $body ... } } => {
    (function($arg) { $body ... }).bind(this)
  }
  rule infix { ($args:ident (,) ...) | { $body ... } } => {
    (function($args (,) ...) { $body ... }).bind(this)
  }
  rule infix { $arg:ident | $body:expr } => {
    (function($arg) { return $body }).bind(this)
  }
  rule infix { ($args:ident (,) ...) | $body:expr } => {
    (function($args (,) ...) { return $body }).bind(this)
  }
}

@natefaubion
Copy link
Contributor Author

@disnet It turns out withSyntax was not broken, but the test cases for it were failing to build. For whatever reason, the macro transformer was being generated as:

function( ... ) {
  return;
  (function( ... ) { ... }(...);
}

The odd thing was, if I added a 'var asdf = it' declaration at the top of the test file (test_macro_case.js), it would work fine. Is there some weird hygiene related thing going on here that affects how code potentially gets generated? It seems totally bizarre to me that adding that var declaration would mean the difference between return (function ... and return; (function ... in the generated code of the macro transformer.

I ended up fixing it by removing the outer parens around the returned function expression in syntaxCase, but I have no idea what's really going on.

@natefaubion
Copy link
Contributor Author

@disnet Currently the destruct implementations of term trees are side effecting. I can't call destruct multiple times without breaking compilation. I need to get the destructed syntax of the term so I can add it to prevStx for infix matching. I thought I could get around it by returning a destructed property from step which just slices the diff from stx, but that doesn't take macro expansions into consideration. So I think I just need to rewrite the destruct implementations to be non-side-effecting and return pure copies. Is there anything that relies on destruct's side effects?

@disnet
Copy link
Member

disnet commented Dec 21, 2013

I don't think so.

@natefaubion
Copy link
Contributor Author

But wouldn't that mean it should have been broken for everything? It only did that for the withSyntax tests, and only after the first withSyntax test. And why would creating a var alias for it have fixed it?

@disnet
Copy link
Member

disnet commented Dec 21, 2013

The first withSyntax test is near line 69: https://github.com/natefaubion/sweet.js/blob/master/test/test_macro_case.js#L71

Maybe adding the var alias just pushed the line numbers to line up correctly?

@disnet
Copy link
Member

disnet commented Dec 21, 2013

Oh! Actually I think the real reason it works now is that by using return function ... instead of return (function ... the parser completely ignores the line number when parsing the return statement if the following token is an identifier: https://github.com/mozilla/sweet.js/blob/master/src/parser.js#L3733-L3737

Which is actually wrong: http://sweetjs.org/browser/editor.html#function%20foo%20%28%29%20{%0Areturn%0Afoo;%0A}

Ugh! ASI is the worst.

@natefaubion
Copy link
Contributor Author

@disnet I was able to get around the destructed issue by making a simple adjustment in case of lookbehind matching. So no need to rewrite destruct.

It's looking pretty good. I just need to write up a bunch of test cases.

@natefaubion
Copy link
Contributor Author

So should I revert my change in stxcase, and just change the context on the parens to name_stx?

@disnet
Copy link
Member

disnet commented Dec 21, 2013

No need. It's actually simpler to just have a function expression without the parens.

@disnet
Copy link
Member

disnet commented Dec 21, 2013

I think there's a problem with trying to match a literal |:

macro n {
    rule infix { $x $[|] | $y } => { $x + $y }
}

// fails to match
10 | n 100

@disnet
Copy link
Member

disnet commented Dec 21, 2013

Oh! That's probably because it's splitting an expression right? Is there a way to make that obvious in the error?

@natefaubion
Copy link
Contributor Author

In this case, it's actually because you're in the middle of building a BinOp term. The macro is being invoked as the rhs of a | op, and isn't getting the previous syntax and terms to match on. I think this can actually be mitigated. When building the BinOp term, we'll have to go ahead and create a Punc for the operator and push it onto the prevTerms and prevStx for the rhs enforestation. Then you have to check if it matched on any lookbehind syntax afterwards. If it did, we don't build the term, and instead just continue on with it's result. We'll probably have to do this in a couple of other places where we have to enforest a rhs before building the actual term.

@natefaubion
Copy link
Contributor Author

@disnet I think I've taken this as far as I can without getting more widespread testing. One thing to note, is I moved the greedy expression expansion to get_expression so it applies to everything in :expr position. Without it, you'd get something like this (assuming => is an infix macro):

macro test {
  rule { $arg:expr } => {
    foo($arg)
  }
}

test a => a + 1
// -->
foo(a) => a + 1

It would take only the a as the expression (like the problem we were having on var rhs assignment). With the changes to get_expression, this will work as one would expect. As well as something more complicated like:

macro test {
  rule { $arg:expr ... } => {
    foo($arg (,) ...);
  }
}

test a => a + 1 b => b + 2 c => c + 3
// -->
foo(function(a) {
  return a + 1;
}.bind(this), function(b) {
  return b + 2;
}.bind(this), function(c) {
  return c + 3;
}.bind(this));

This means that any macro with rule infix { $lhs:expr | $rhs:expr } is essentially an infixr 0 operator. For example:

macro $ {
  rule infix { $lhs:expr | $rhs:expr } => {
    $lhs($rhs)
  }
}

foo $ bar $ baz $ 42
// -->
foo(bar(baz(42)))

Let me know if you think I should revert this.

@jlongster
Copy link
Contributor

@natefaubion I really like the idea of making it more consistent like that. I've been meaning to look into when "inside out" expansion is triggered, but haven't grasped it yet because I thought it happened whenever :expr is used. disnet explained it in another issue but I didn't get it on first read and I've been meaning to read it more thoroughly. Regardless, forcing :expr seems like a cool way to force "inside out" expansion because the definition of an "expression" is very ambiguous when there are more macros on the right-hand side.

That's something that is very hard to do in most lisps/schemes btw, so it will be really neat to have that. Granted Lisp hits these problems much more rarely because the actual code shape is a lot more consistent. This lets us have our cake and eat it too.

I'm happy to test this more though I probably won't do too much because I'm not working the next few days. The only thing I really want is the fat arrow operator for my es6-macros. I'll play around with it in more depth though.

@disnet
Copy link
Member

disnet commented Jan 1, 2014

This is looking great! I'll merge it in and assuming we don't run into any blockers do a npm release next week.

@disnet disnet merged commit 8835e04 into sweet-js:master Jan 2, 2014
@michaelficarra
Copy link

👏 Nice work, @natefaubion.

@puffnfresh
Copy link
Contributor

Woo! 👍

@vendethiel
Copy link

👍

@disnet disnet mentioned this pull request Jan 3, 2014
@alexandru
Copy link

This is so awesome 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants