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

add "macros" option to install global macros in compile API #127

Merged
merged 3 commits into from
Nov 21, 2013

Conversation

jlongster
Copy link
Contributor

This is a quick tweak to the compile API that lets you pass in additional macros to install globally. This difference between this and simply concat'ing sources is that this generates correct source map information.

I haven't reflected this in sjs.js, which we probably should do. Whatever is passed to -m should probably use this. I'm writing my own Grunt plugin to use it so I just need the lower-level support.

Additionally, in my Grunt plugin I'm automatically adding require('source-map-support').install(); at the beginning of the generated source, which nicely makes node use the sourcemaps in stack traces automatically. It might be worth adding an option to sjs for this as well. (I'll publish my grunt plugin at some point)

Let me know what you think about this, I can continue to tweak this PR based on feedback.

@disnet
Copy link
Member

disnet commented Nov 20, 2013

So this doesn't quite do what you think it does. The stxcaseModule was hacked in to force modules to use an export keyword to actually load the macro name. So in "macros/stxcase.js" you see stuff like export # after the macro definition. Only the exported macros are loaded whereas right now the -m flag loads all the macros it finds.

Actually, maybe this is exactly what we want. Force people to use the ES6 module syntax now even though it's not a complete implementation. It should be forward compatible too.

Source-map-support looks awesome! Definitely worth adding to sjs.

Also, just in case you haven't see it, @natefaubion published a sweet.js grunt plugin a few days ago: https://github.com/natefaubion/grunt-sweet.js

@jlongster
Copy link
Contributor Author

I did figure that out after reading expandTopLevel in expand.js, and I added export statements for my macros. It seems intentional so that it doesn't conflict with file-level macros. Like you said, that seems like a good thing to extend to a global macro file. I don't really mind either way, the most important thing to me is that sourcemaps are accurate.

I hadn't seen his grunt plugin, thanks! Mine has a few custom features, like injecting the source-map-support code and automatically loading the global macro file, but eventually we should merge it into one.

@natefaubion
Copy link
Contributor

Actually, maybe this is exactly what we want. Force people to use the ES6 module syntax now even though it's not a complete implementation. It should be forward compatible too.

+1

This would let us hide helper macros and avoid obscure names, right?

@jlongster My Grunt plugin is very open to contributions. I just put out something simple since people were asking about one and nothing had been made yet.

@jlongster
Copy link
Contributor Author

@natefaubion cool, I'll merge in a few things I've done and open a PR to see what you think.

@disnet
Copy link
Member

disnet commented Nov 20, 2013

This would let us hide helper macros and avoid obscure names, right?

Yep.

Cool, we'll do this then. @jlongster do you want to update this PR to have sjs -m use this path?

@natefaubion
Copy link
Contributor

We still have to worry about collisions with other 'global' macros though regardless of whether its exported, right? I could potentially see that causing issues. IE, expecting a helper macro to be hidden since you aren't exporting it, but still clashing with other global macros. It very quickly descends into 'we need real modules'. Its a good step for source maps (and macro syntax errors) though.

@disnet
Copy link
Member

disnet commented Nov 20, 2013

Yes, exactly. Both exported and non-exported macros will conflict the other module macros. Last defined wins I think.
Once we get sourcemaps working I'll start in on real module support.

@jlongster
Copy link
Contributor Author

Done. Just tested and it seems to work. Not sure if you want me to commit the rebuilt code of if you want to test it first. I can commit the built code too.

@disnet
Copy link
Member

disnet commented Nov 21, 2013

Awesome! I'll run the build when I merge. Thanks!

@jlongster
Copy link
Contributor Author

Hm, I'm not sure if this is completely working. Looks like the way we export macros from the global file causes issues.

I'm trying to share code, so I wrote a macro like this:

macro _destructure {
    case { $ctx $x } => { return #{ $x } }
}
export _destructure;

let var = macro {
    case { $ctx $l = $r } => { 
        return #{
            _destructure $l
        };
    }
}
export var

This should compile this: var foo = bar;. But I get this error when loading the above macro from a file with sjs -m:

{ type: 3,
  value: 'foo',
  lineNumber: 2,
  lineStart: 1,
  range: [ 5, 8 ],
  sm_lineNumber: 2,
  sm_lineStart: 1,
  sm_range: [ 5, 8 ] }

/Users/james/projects/influx-reboot/node_modules/sweet.js/lib/parser.js:3330
            throw e$1134;
                  ^
Error: Line 2: Unexpected identifier
    at throwError$721 (/Users/james/projects/influx-reboot/node_modules/sweet.js/lib/parser.js:1082:25)
    at throwUnexpected$723 (/Users/james/projects/influx-reboot/node_modules/sweet.js/lib/parser.js:1122:13)
    at consumeSemicolon$729 (/Users/james/projects/influx-reboot/node_modules/sweet.js/lib/parser.js:1194:13)

It works just fine if I include the 2 macros inline in the file and use it directly. It seems like there's some problem finding the _destructure macro. You can see that I even tried to explicitly export it.

@disnet
Copy link
Member

disnet commented Nov 21, 2013

I think it's working correctly. It's just that these macros expand var foo = bar; to foo bar.

@disnet
Copy link
Member

disnet commented Nov 21, 2013

Err, never mind. That's wrong...too quick to comment.

@disnet
Copy link
Member

disnet commented Nov 21, 2013

This is happening because we don't have hygiene for export right. The _destructure identifier is being bound incorrectly. Working on a fix.

@jlongster
Copy link
Contributor Author

Cool, thanks. For now, I can just share code with simple js functions and syntax-case macros.

@jlongster
Copy link
Contributor Author

What do you think is the best way to share code across macros? I haven't found a great solution yet.

Here's my use case: I want to implement destructuring, but it should work with var, let, and const. So I need to make 3 macros for each of them, but share the bulk of the destructuring logic across them.

The best way in my opinion would be to have something where I could create functions at expand-time in the global scope, something like this:

run-at-expand-time {
    function helper($x) { 
        return $x;
    }
}

macro foo {
    rule { $ctx $x } => {
        return helper($x);
    }
}

macro bar {
    rule { $ctx $x } => {
        return helper($x);
    }
}

Once your fix is in, we could recursively expand forms, but that will require exporting the helper macros, which is probably good enough anyway, but the above would still be cool.

@jlongster
Copy link
Contributor Author

Back in the Scheme days we used to use an eval hack to install functions at expand time. You might think it's kind of gross, but it actually worked pretty well (I was using Gambit Scheme which didn't have modules, so this is is a hack until we get modules).

This actually works!

macro eval_expand_time {
    case { $ctx } => {
        eval("bar = function () { console.log('hi'); }");
        return [];
    }
}

eval_expand_time;

macro foo {
    case { $ctx } => {
        bar();
        return [];
    }
}

foo;

So what would be really cool is if we could get this to work :)

macro eval_expand_time {
    case { $ctx $expr ... } => {
        eval(codegen.generate(parser.parse(#{ $expr ... })));
        return [];
    }
}

eval_expand_time helpers = {}
eval_expand_time helpers.bar = function() { console.log("hi"); };

Unfortunately the parser throws an error when trying to do something with location tracking... and codegen isn't available in the macro. Not sure what you think about this, but it could be helpful until we have modules.

@natefaubion
Copy link
Contributor

you could do something like this:

macro shared_helpers {
  rule {} => {
    {
      hello: function(x) {
        console.log(x);
      }
    }
  }
}

macro test {
  case { _ } => {
    var utils = shared_helpers;
    utils.hello('bar');
    return [];
  }
}

test

@jlongster
Copy link
Contributor Author

@natefaubion yeah, I did something like that originally. I expanded things out to a helper macro: var x = 5; expanded to _destructure var x = 5 which could grab the declaration keyword. The problem is with this is described in #issuecomment-28963005; with the new implementation of a global module, the helper macro isn't being exported correctly so the 2nd expansion can't find it.

I just realized that macro-creating macros work, so I can do this, which is actually a very nice way to do it:

macro _destructure {
    rule { $declare  } => {
        let $declare = macro {
            rule { $id = $val } => { $declare $id = $val }
        }
    }
}

_destructure var;
_destructure let;

var x = 5;
let y = 5;

This outputs:

;
;
var x$161 = 5;
let y = 5;

Not sure why y isn't hygienically renamed though... I guess sweet.js doesn't honor lets yet?

@natefaubion
Copy link
Contributor

Correct, sweet doesn't touch lets or consts.

@jlongster
Copy link
Contributor Author

That's considered a bug though, right? Seems like it should... Doesn't affect me in this case since I'm not actually introducing any new variables with let.

Sorry that this bug is somewhat derailed, but I think it's good discussion.

@natefaubion
Copy link
Contributor

It seems it's just something that hasn't been done, as it has different/more complicated scoping rules than var (though I won't speak for @disnet).

@disnet
Copy link
Member

disnet commented Nov 21, 2013

Right let and const have slightly different scoping rules and every time I need to touch the hygiene code I have to go into my happy place for a while (n.b. not a very happy place).

But the real reason we haven't gotten to them is that I just assumed they wouldn't parse. We're using an older fork of esrpima (issue to upgrade to the ES6 version is #96) and I didn't realize it actually supported let/const. Cool! Guess it's time for me to get into my happy place :)

About expand-time eval: I really don't want to support that if at all possible. You'll get into headaches once you want to use a macro in your expand-time eval because suddenly you have multiple expand and run times you need to manage by hand. Modules deal with this so hopefully the macro-generating macro trick is sufficient in the interim.

@jlongster
Copy link
Contributor Author

Haha, understandable. It was always kind of a hack, so don't worry about it. Modules are a much better way to handle the multiple run/expand-times.

macro-generating macros are great, and if you fix the export hygiene I think we are good for most things until modules come along.

@jlongster
Copy link
Contributor Author

If beers help you while you are in your "happy" place, I'm willing to provide as many as needed. This stuff is really impressive and I can't wait to get it even farther.

@disnet
Copy link
Member

disnet commented Nov 21, 2013

haha awesome :)

disnet added a commit that referenced this pull request Nov 21, 2013
All compile-time values defined in the `builtinSource` need to be
carried over to the main expasion. This allows exported macros to
safely generate non-exported macros.
@disnet
Copy link
Member

disnet commented Nov 21, 2013

It should work now. Super easy fix, just had to sleep on it :)

@jlongster
Copy link
Contributor Author

wooo! It's totally workable now. thanks!

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

3 participants