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

Some tink_sql macro problem caused by a Nicolas-change #6321

Closed
Simn opened this issue Jun 1, 2017 · 13 comments
Closed

Some tink_sql macro problem caused by a Nicolas-change #6321

Simn opened this issue Jun 1, 2017 · 13 comments
Assignees
Labels
platform-macro Everything related to Haxe macros regression

Comments

@Simn
Copy link
Member

Simn commented Jun 1, 2017

I don't have a reproducible example. We discuss the problem here: haxetink/tink_sql#22

It's about the error tink.sql.Table0<Unknown<0>> has no field init which I managed to bisect to this commit: 34f80449f27

@kevinresol @back2dos

@Simn Simn added platform-macro Everything related to Haxe macros regression labels Jun 1, 2017
@ncannasse
Copy link
Member

ncannasse commented Jun 1, 2017

The commit is correct, we should NOT force typing while doing these.
We should investigate how/why the error did occur without cl_build being done.
That might be an unsound macro code, such as performing some typing in a build macro while the Table0 class is not yet initialized.

@nadako
Copy link
Member

nadako commented Jun 7, 2017

I'm getting a similar issue with latest Haxe, here's a reduced example:
A.hx

class A {
    public var a:Int;
}

B.hx

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

Macro.hx

class Macro {
    static function doStuff() {
        switch (haxe.macro.Context.getType("B")) {
            case TInst(_.get() => cl, _):
                for (field in cl.fields.get())
                    field.expr();
            default:
                throw false;
        }
    }
}
$ haxe --macro "Macro.doStuff()"
B.hx:3: characters 17-20 : A has no field a

This somehow worked before, and if it's considered unsound then it would be nice to have a better error message, hinting about the actual issue (e.g. "A is not yet built while building B while running Macro.doStuff, check your macros and reorder types" or something like that).

@nadako
Copy link
Member

nadako commented Jun 7, 2017

Note that reverting 34f8044 makes the error go away.

@ncannasse
Copy link
Member

The problem in @nadako example is the status of --macro. since it's a call and not a @:build, those should be run within the PTypeExpr pass

@Simn
Copy link
Member Author

Simn commented Jun 25, 2017

That doesn't sound right. They are init macros after all and are supposed to run early.

@ncannasse
Copy link
Member

@Simn I don't agree, they contain actual expressions/calls so should be in PTypeExpr mode like function code - I think we don't have any other pending classes being typed at this moment so it should not affect other code

@back2dos
Copy link
Member

back2dos commented Sep 6, 2017

Things were just fine for the past 5 years or so, when a macro was considered sound when it generated code that passes the typer. So I wonder: can somebody explain for stupid people what the change does and what it's benefits are and what to do about the fact that for me this practically breaks every non-trivial macro? It's a major hindrance in preparing our code for Haxe 4.

@ncannasse
Copy link
Member

ncannasse commented Sep 6, 2017

It's actually quite complex to explain, but I'll try.

Usually what you want is to have:

parse class A
construct class A
type method A.foo
type method A.bar
...
done

Where "construct" means that we populate the fields list of class A.

But things get more complex with several classes, so you would go like this:

parse class A
construct class A
     (found class B)
     parse class B
     construct class B
type A.foo
type A.bar
type B.foo
done

So as you can see constructing a class might load other classes, and because of the possible recursion (B references A) it means that during "construction" you should not try accessing other classes fields since they might actually be on the "stack" waiting to be constructed.

That's similar for "build" macros: because we are not typing something yet (because build is between "construct" and "type field"), it means that we should not do any typing in build macros because this typing might actually already be on the "stack", for instance in the following case:

parse class A
construct class A
type A.foo
     (found class B)
     parse class B
     construct class B
     build class B -- > request type for A.foo (while it's on the stack)
done

So in short, "build" step is before "type" step, and before the change, this is what was happening:

parse class A
construct class A
build class A
    (getType class B)
    parse class B
    construct class B
    type B.foo -- HERE
        parse class C
        ...

Which is very bad because we start typing a lot of things while we haven't even finished building the class A !

My change keeps us at the "build" step when calling getType in a build macro, so the result will be:

parse class A
construct class A
build class A
    (getType class B)
    parse class B
    construct class B
    ...
    build done
type B.foo
...

So it returns earlier, and do the typing afterwards as it should.

@ncannasse
Copy link
Member

Actually my comment is not entirely correct because in our compiler has a single step for both "construct + build", maybe separating these two would help a bit

@ncannasse
Copy link
Member

ncannasse commented Sep 6, 2017

Regarding @nadako issue:

For some reason, f.expr() seems to do some follow (in haxe compiler) on the type of B.b method, and this trigger the "type B.b" pass while the "construct" passes (class A one in particular) have not been executed yet.

That's because of the particular status of the init macro, which operate on a empty context which current pass is not yet setup to "PBuildClass" (construct+build)

@ncannasse
Copy link
Member

I have fixed @nadako bug report with cfcadcc , this should fix other similar issues. Please note that now trying to access within a macro a field that it's currently being typed with result in an exception instead of getting the unbound TMono. This should provide a get base for a lot of new bug reports.

Please TEST YOUR MACROS, ty :)

@nadako
Copy link
Member

nadako commented Sep 8, 2017

Please TEST YOUR MACROS, ty :)

My codebase still compiles :)

@back2dos
Copy link
Member

back2dos commented Sep 8, 2017

FWIW the tink_sql problem seems to be gone (at least the tests appear to compile). Still can't compile any actual project though, mostly because of #6567

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
platform-macro Everything related to Haxe macros regression
Projects
None yet
Development

No branches or pull requests

4 participants