HaxeFoundation / haxe

Haxe - The Cross-Platform Toolkit
https://haxe.org
6.18k stars 655 forks source link

Busted macro caching #1983

Closed aduros closed 11 years ago

aduros commented 11 years ago

It seems like macros aren't being recompiled/rerun when using the compilation server?

Main.hx:

class Main
{
    private static function main ()
    {
        trace(TestMacro.date());
    }
}

TestMacro.hx:

import haxe.macro.Context;

class TestMacro
{
    // Expands to the build timestamp
    macro public static function date ()
    {
        trace("Date was called!");
        return Context.makeExpr(Date.now().toString(), Context.currentPos());
    }
}
  1. haxe --wait 6000
  2. haxe --connect 6000 -js out.js -main Main: Look at out.js and see the correct date/time.
  3. Run step two again, see that no trace gets printed by the macro and out.js is not updated.
  4. Bonus bug: trivially change Main.hx to invalidate the cache and receive this error: ./Main.hx:5: characters 14-28 : #TestMacro has no field date
Simn commented 11 years ago

I cannot reproduce 4., and 3. is not exactly a bug: The compiler doesn't know that there's an external state like Date.now() and hence caches the module.

@ncannasse: Wasn't there some option to disable caching for certain macros? I recall talking about this long ago when you first introduced the cache, but I can't find anything.

Another option is to detect this kind of "external state function" and invalidate the cache in the macro interpreter. I can only think of Date.now and Math.random right now.

frabbit commented 11 years ago

i don't think that it's possible to detect "external state functions", think about reading Files, Mysql whatever or stateful calls nested deep in the call stack. A possible approach would be an annotation for a macro like @:noCache.

Simn commented 11 years ago

That's irrelevant, all calls are made to the interpreter API so it would be no problem to hook into that. Also, there's already Context.registerModuleDependency to deal with external files.

ncannasse commented 11 years ago

I think you can use registerModuleDependency on a unexisting file, this will disable caching for this module and all the modules using it, forcing the macros to be called againt

stroncium commented 11 years ago

@ncannasse Shouldn't we at least introduce a shortcut for that so it would be more straightforward? Also, I'm not sure if it is already implemented, but I guess at least reading a file should automatically add this file as a dependency.

aduros commented 11 years ago

I'm fine with changing my macro in some way if that works. The bug I'm more concerned about here is step 4 (possibly a separate issue I should have filed separately).

back2dos commented 11 years ago

As for number 4, that looks pretty serious. Pretty sure I've run into something similar already although I got used to restarting the compilation server if compilation fails for weird reasons.

@aduros: If you simply want to set the build stamp, then I suggest you do something like this https://gist.github.com/back2dos/5964415 and then compile with --macro Build.storeInfo().

That's not going to cause recompilation of anything by itself. For me it would be a horror to depend on a module that is uncacheable ;)

Simn commented 11 years ago

Can someone actually reproduce 4? It looks too simple to just fail like that, so there must be some secondary condition required to trigger it. I can't think of anything other than lunar phases though.

ncannasse commented 11 years ago

Is issue 4) reproducible with 3.0.0 or do you use Git version?

ncannasse commented 11 years ago

Ah, I think issue 4) was related to macros functions being deleted by DCE before we set the class restore point, it was fixed on Git

ncannasse commented 11 years ago

Question: was it broken in 3.0.0 or was the issue introduced afterwards?

aduros commented 11 years ago

I was running with commit 02f71b243f33c4, I can't repro 4 when going back to 3.0 or latest, hrmm.

I've definitely seen this in 3.0 stable though. The actual case is a Flambe build, which has DCE turned on and a little more complicated macros.

ncannasse commented 11 years ago

Ok, 4) was a bug that was fixed

aduros commented 11 years ago

Ok, thanks! I'll run with the latest for a while and reopen if I ever hit it again.