macros and type building issue

[nadako]

I can't compile my work project codebase after commit eca310eac210fc3379ef560ab3fd22c20fc6344b, it says that it can't find a field which is a macro method. I'll try to reduce it to a reproducible example, but it won't be easy because the code involved is complex.

[Simn]

Check please, regression.

[nadako]

Okay, the problem seems to come from a mix of macros and conditional compilation, a type is not normally included in compilation, but is typed through macro Context.getType, and I get errors from that class's methods that doesn't seem to find methods of a third class. I'll try to make an example.

[nadako]

Here's a somewhat reduced example:

A.hx

class A {
    public function f() {}
}

B.hx

class B {
    var a:A;
    function g() {
        a.f();
    }
}

Main.hx

@:build(Macro.build())
class Main {
    static function main() {
    }
}

Macro.hx

import haxe.macro.Context;
import haxe.macro.Expr;

class Macro {
    static function build():Array<Field> {
        var fields = Context.getBuildFields();
        var c = switch (Context.getType("B")) {
            case TInst(_.get() => c, _): c;
            default: throw false;
        }
        for (f in c.fields.get())
            Context.follow(f.type);
        return fields;
    }
}

This results an error: B.hx:4: characters 8-11 : A has no field f (compile with haxe -main Main -js main.js)

[ncannasse]

I'm not sure if I'll fix it.

The problem is that you should not be allowed to access any type structure, such as doing a follow, in a build. Because this type might already be on the stack.

Before my patch, getType() was forcing this build to be performed, but we don't want to force it this early because we might have a class that extends the class being built on the to-build stack !

We could force a rebuild in Context.follow maybe, but it will also cause other kind of issues...

[nadako]

So what's the correct way of fixing my code then? Should I explicitly include those types so they are built before my @:build macros?

Also it would be nice to have a better error message for that.

[Simn]

This example does seem to have a clear build stack:

Main def
Main structure (enters build macro)
    B def (from getType)
    B structure (from follow)
        A def (from var a:A)
        A structure (from a.f())

This would only be a problem if B referenced the structure of Main or if A referenced the structure of B or Main.

I don't think we should break this in a dot release...

[ncannasse]

@Simn we could force the build of the queried module in getType, but we cannot do what was done before, which was to set the pass to PTypeField, which was forcing all builds to be performed, recursively.

[ncannasse]

More generally : calling cl_build() is fine, we now even get a false result so we can eventually raise an error if it's already on the stack. That's different from flushing all pending builds.

[nadako]

Still wonder what to do with this in my code...

[Simn]

Shouldn't the structure building be forced upon the field access itself (a.f)?

[nadako]

Yes, that's basically how I fixed it for now. The issue is that I don't need the actual class in my compilation, i just need some info about it to build a remoting-like wrapper.

[ncannasse]

@Simn when we are currently typing a method (or expr), we are already in the PTypeField pass, which means every type/module we load will have its cl_build() (and all other pending builds) forced before returning it. So we don't need manual cl_build()

Of course, the problem is that we don't want that to happen during our build macros, hence my recent change. We could force some specific build when doing getType, which is my proposed fix for this issue.

[Simn]

Of course, the problem is that we don't want that to happen during our build macros

I think this is the part that confuses me. We only need the build macro results when we are accessing the type's structure, so in my mind they are run in a pass that allows follow. I thought that was the main reason why we don't allow changing the inheritance chain in build macros.

[ncannasse]

Let's say we are macro-building class A . If we force building of all pending classes as soon as we use getType (which is just what is doing load_module in PTypeField pass), then we might endup building a subclass of A in the process.

[Simn]

Isn't that precisely the case your if not (csup.cl_build()) then cancel_build csup; change from #2117 deals with?

I'm not sure about fixing this in getType. I think that would only shift the problem to another stage because we could again end up building something too early.

[ncannasse]

I'll have to look again at it, the previous behavior was causing quite big issues with the things I'm working on.

[ncannasse]

@nadako can you confirm ?

[nadako]

I don't know. I dodged this issue by adjusting my macros already. It compiles with latest version, so I guess it's okay. I'll reopen if anything comes up.

[Simn]

Someone just threw this stack dump at me: https://gist.github.com/PDeveloper/1b609544405e219d2ffa

I didn't investigate it yet and it doesn't look like much fun.

[Simn]

Something interesting in that stacktrace is that it hits typeload.ml:3200 which is the last line of this segment:

(*
    we exceptionnaly allow follow here because we don't care the type we get as long as it's not our own
*)
(match d.d_data with
| CTExtend _ -> ()
| _ ->
    if t.t_type == follow tt then error "Recursive typedef is not allowed" p);

I've always wondered about this. While it's true that we don't care about the structure, it might very well still trigger class building or worse. Any thoughts on this @ncannasse?

[ncannasse]

@Simn I don't think that's it, since we go into typeload.ml 3506 (near the top of the stack), which requires PTypeExpr. I suggest eventually a subclass declared before the main class in the same module.

[ncannasse]

Ouch, macros are involved... hence the PTypeExpr I guess

[Simn]

IRC log from earlier today:

[13:17:03] <PSvils> Simn: aha, it's from Luxe: `luxe.resource.Resource`
[13:17:11] <Simn> Oh god...
[13:17:41] <Simn> Now how do we debug this...
[13:18:12] <PSvils> he...
[13:18:21] <PSvils> what is the issue in layman terms?
[13:18:24] <PSvils> it can't find the type?
[13:19:40] <Simn> It's some typing order issue, the compiler tries to build the class before it is properly initialized.
[13:19:49] <Simn> Basically it tries to do pass 2 before pass 1.
[13:19:50] <PSvils> hmm
[13:19:53] <PSvils> ok
[13:20:02] <Simn> It's related to changes Nicolas recently made.
[13:21:20] <PSvils> in the flow/luxe build system, there are several --macros going on
[13:21:26] <PSvils> not sure if related.
[13:21:31] <Simn> It might be
[13:21:43] <Simn> Can you link me the stacktrace again?
[13:25:15] <PSvils> yep, hold up
[13:25:42] <PSvils> Simn: https://gist.github.com/PDeveloper/0b97212b68a753d897cb
[13:26:21] <Simn> Ok, there's Context.getType involved.
[13:28:30] <Simn> Can you try replacing typeload.ml line 3200 with `());`?
[13:50:36] <PSvils> Simn: that whole if statement just with `());`?
[13:50:41] <Simn> Yah
[13:50:44] <Simn> So the check is gone.
[14:04:20] <PSvils> Simn: hmm, I get a type not found error now
[14:04:51] <PSvils> for a different class
[14:05:04] <Simn> Well I guess that's an improvement...
[14:05:28] <PSvils> checking where the class comes from etc.
[14:06:57] <PSvils> aha, it seems like it's a generated type from a macro
[14:07:58] <Simn> Hmm... from a --macro macro?
[14:09:15] <PSvils> yep
[14:12:57] <Simn> Does compiling with -v tell you anything interesting? Like when that error occurs...
[14:13:12] <Simn> There's a chance that luxe relies on unspecified typing order.
[14:19:03] <PSvils> hmm, it parses Types.hx, and then a Keycodes.hx, and then the error happens for Type not found. And it's in Types.hx where it calls the macro function that should create the type that's not found
[14:20:08] <Simn> I thought it was a --macro macro that defines the type?
[14:22:27] <PSvils> you're right, I did say that...
[14:22:28] <PSvils> checking
[14:23:26] <PSvils> okay, I got confused a bit, the --macro only defines the string for which type to use < irrelevant. the actual build macro is a @:genericBuild() macro, where the type is passed as a generic type parameter to the type with the @:genericBuild...erm...
[14:24:37] <PSvils> it doesn't find `TypeADef`. above that, there is `var TypeAInit = ApplyType<TypeA>`
[14:24:55] <PSvils> ApplyType is:
[14:24:56] <PSvils> @:genericBuild(snow.types.TypeCreate.build())
[14:24:56] <PSvils>     private class ApplyType<Const> {}
[14:27:01] <Simn> Ah, that seems very much related.
[14:27:20] <Simn> Because you commented out the `follow tt` the typedef target is no longer built this early.
[14:28:06] <PSvils> ah ok
[14:28:11] <Simn> This might indeed be a luxe problem, but it's not so easy to tell.

[ncannasse]

So yes, it might indeed be related to what you reported. A typedef to a macro generic class

[ncannasse]

Maybe you can try to return Building instead of assert

[ncannasse]

but yes that's an issue, we shouldn't enter macros when we are still setuping our module types. I think this recursive check should be moved somewhere else, maybe using a TLazy on the typedef

[ncannasse]

I made an untested PR, could you confirm with user that it works well now?

[ruby0x1]

@ncannasse @Simn @nadako I am confused why nobody asked me directly or even mentioned that genericBuild types would be broken in this way, and on a minor release. It's been 30 days and I only found out about this indirectly?

[Simn]

This issue was closed 3 days ago and the breaking change is mentioned here: https://github.com/HaxeFoundation/haxe/wiki/Breaking-changes-in-Haxe-3.3.0#typing

[ruby0x1]

In the documentation here, http://haxe.org/manual/macro-generic-build.html

Normal build-macros are run per-type and are already very powerful. In some cases it is useful to run a build macro per type usage instead, i.e. whenever it actually appears in the code. Among other things this allows accessing the concrete type parameters in the macro.

The part in bold is the rule I was using to build from, and in 3.2.1 this was doing what it says on the manual. In 3.3 this is no longer true, the genericBuild macro is not invoked at the usage at all. It is however with @:build.

I don't understand the purpose of the generic build and const if the return value is not invoked like the use cases in the manual describe. Could you please elaborate?

[Simn]

This is still true, but that doesn't change the fact that the exact time when this happens is undefined because type building order in general is undefined. We might want to change the wording a bit so that it doesn't suggest any specific timing ("when"), but other than that I don't see much of a problem here.

From what I understand the problem came up because you had a @:genericBuild macro which had side-effects in the form of Context.defineType.

[ruby0x1]

So the example in the documentation at the same page at the bottom:

Here the macro logic could load a file and use its contents to generate a custom type.

Maybe I don't understand the purpose of defineType if the type can never be used because it can never be relied on to exist at any point during the build.

[Simn]

You can use defineType and let the macro return the type path to the defined type.

[ncannasse]

@underscorediscovery this shouldn't change anything, a lot of parts of the compiler are already lazily evaluated could you elaborate on some actual issue it might cause?

[nadako]

Let's not ignite drama here. The purpose of defineType is to define a type. If you later use that type, it'll be available. As far as I understood, the issue is that type isn't built at the time the typedef is defined anymore (e.g. typedef A = MyGenericBuildClass<Some>), right? That sounds like quite a minor change that shouldn't be the problem, unless you're somehow rely on the (undefined) type build order, which is a bad idea in general.

[ruby0x1]

Really? That's how you see asking questions to better understand a system I misunderstood from the documentation? Coming into a discussion yelling about drama is the perfect way to create it. You are not being constructive as you may hope @nadako, it's just silly.

@Simn In your example, var a: A<"myfile.txt">; does make sense, but what about subsequent use of the type? For instance if a is a member variable, how does this react if used elsewhere: a = new A<"myfile.txt">( args )

[ncannasse]

Actually there's something about it, for instance I had the following in my code:

private typedef Init = haxe.macro.MacroType<[cdb.Module.build("myDataFile.cdb")]>;
typedef Hero = LevelData_hero; // type created by MacroType

So I had to make sure that the compiler was actually running the MacroType macro before we start loading the type after that. But @:genericBuild should not be concerned by this.

[ruby0x1]

I read that that MacroType was being deprecated in favor of genericBuild, hence me moving away from it. That's the exact same thing that I was doing that is "relying on undefined orders"....

[ncannasse]

@underscorediscovery please calm down a bit, I agree with nadako that you're putting too much drama here. If you have some question regarding the changes or even better an actual bug report, please report it.

[ruby0x1]

I'm done here.

[ncannasse]

@underscorediscovery so if I understand correctly, you have some @genericBuild which are defining several types and returning one of them, and then some code that reference these other types?

[nadako]

I'm sorry Sven, but silly is saying things like this:

the genericBuild macro is not invoked at the usage at all

Maybe I don't understand the purpose of defineType if the type can never be used because it can never be relied on to exist at any point during the build.

You know that's not true and that this issue was about some edge-case, while you're talking like everything is broken now. This is being dramatic.

[ncannasse]

Usually defineType is meant to create types that are used by the type which is currently being built, so it's not a problem at which point the macro is called. It becomes one in case you want to access these defined types, but there are only some valid cases for it since you need to be absolutely sure that the typing is done before being able to access these types.

In the case of an expression, the macro is evaluated immediately, so any code referencing the newly defined types after it should be safe. In the code of a module declaration it's more tricky. There are valid reasons not to evaluate immediately (because of the bugs reported when doing so) but you might still want to be able to force things to evaluate.

Maybe a metadata on @:genericBuild for it?

[Simn]

I'm not sure, that would probably just bring back the original stack overflow. In my opinion something can either be defined or undefined, and making it slightly less undefined is not worth it if there's a risk involved.

[ncannasse]

@Simn as long as the macro involved does not reference types that are currently being typed (if it's self-enclosed, not using any getType for instance) it should be safe. However I agree that if it's using getType, then it's unsound.

[back2dos]

FWIW I am fully confused here. It seems my libs are unaffected, but this still makes me a little nervous. Could anyone please try to clarify what it is the changes made, ideally by providing a non-normative sketch of how things are handled now vs. how they were handled before? Thanks.

On Wed, Apr 6, 2016 at 10:28 PM, Nicolas Cannasse notifications@github.com wrote:

@Simn https://github.com/Simn as long as the macro involved does not reference types that are currently being typed (if it's self-enclosed, not using any getType for instance) it should be safe. However I agree that if it's using getType, then it's unsound.

— You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub https://github.com/HaxeFoundation/haxe/issues/4825#issuecomment-206546624

[Simn]

// Macro.hx
class Macro {
    static function buildSomething() {
        haxe.macro.Context.defineType(macro class K { });
        return null;
    }
}

// Main.hx
@:genericBuild(Macro.buildSomething())
class C<T> { }

typedef T = C<String>;

class Main {
    static var k:K; // Type not found : K
    public static function main() { }
}

On 3.2.1 the macro is "probably" executed when typedef T = C<String> is first seen and the defined type K is then available to use as a class field type on Main. On current Haxe the macro execution is delayed so it may or may not be run before the compiler comes across static var k:K.

[back2dos]

Ok, sounds reasonable.

Maybe @:eager typedef T = C<String> might be a good way to allow users to get the old behavior at their own risk?

On Wed, Apr 6, 2016 at 11:42 PM, Simon Krajewski notifications@github.com wrote:

// Macro.hxclass Macro { static function buildSomething() { haxe.macro.Context.defineType(macro class K { }); return null; } } // Main.hx @:genericBuild(Macro.buildSomething())class C { } typedef T = C; class Main { static var k:K; // Type not found : K public static function main() { } }

On 3.2.1 the macro is "probably" executed when typedef T = C is first seen and the defined type K is then available to use as a class field type on Main. On current Haxe the macro execution is delayed so it may or may not be run before the compiler comes across static var k:K.

— You are receiving this because you commented. Reply to this email directly or view it on GitHub https://github.com/HaxeFoundation/haxe/issues/4825#issuecomment-206583773

[nadako]

What I'd like to know is how MacroType differs from @:genericBuild? Is it just the @:eager version?

[ncannasse]

MacroType don't take types as parameters but expressions, and are eagerly evaluated. I propose we add @:eager, but not on the typedef, on the @:genericBuild class (with doc mention "use at your own risks"). Most likely @underscorediscovery stack overflow involved two mutually recursive @:genericBuild, but only one will require eagerness.