HaxeFoundation / record-macros

Macro-based ORM (object-relational mapping)
MIT License
49 stars 24 forks source link

haxe.macro.Compiler.setFieldType #57

Open filt3rek opened 3 years ago

filt3rek commented 3 years ago

Hej !

Happy Christmas everyone !

I'm sorry, I always come with strange things... Last changes in RecordMacros.hx related to :

var get = {
    args : [],
    params : [],
    ret : t,
    expr: macro return @:privateAccess $i{tname}.manager.__get(this, $v{f.name}, $v{relKey}, $v{lock}),
    //expr : Context.parse("return untyped "+tname+".manager.__get(this,'"+f.name+"','"+relKey+"',"+lock+")",pos),
};

https://github.com/HaxeFoundation/record-macros/blob/aaa5f70d233e1aedd0ef2e53f821469125f57c06/src/sys/db/RecordMacros.hx#L1380

Makes Haxe complain when trying to build my code. I've done a small example, let's say we have that : Main.hx

class Main{
    static var foo : Foo;
    public static function main(){}
}

Foo.hx

class Foo extends sys.db.Object{
    var id      : sys.db.Types.SId;
    @:relation( barID )
    public var bar  : Bar;
}

Bar.hx

class Bar extends sys.db.Object{
    var id : sys.db.Types.SId;
}

my/Bar.hx

package my;
class Bar extends sys.db.Object{
    var id  : sys.db.Types.SId;
    var b   : Bool;
}

build.hxml

-lib record-macros

#--macro haxe.macro.Compiler.setFieldType( "Foo", "bar", "my.Bar" )

-main Main
-php www

If this line is commented, all runs well, but when it's set, I get this error : record-macros/git/src/sys/db/RecordMacros.hx:1391: characters 46-53 : Unknown identifier : my.Bar

Is there something we could do to make it work again like before please ? (Before, untyped was used but now, with the strict synthax, the compiler complains...)

Thanks for reading, Cheers,

Michal

filt3rek commented 3 years ago

Hej,

I'm sorry, there is nothing to do with type patching. In fact, you use $i{ tname } but tname can be dotted so ident does'nt work here. Doing that it works fine $p{ p } :

var ttype = t, p;
while( true )
    switch(ttype) {
        case TPath(t):
            if( t.params.length == 1 && (t.name == "Null" || t.name == "SNull") ) {
                ttype = switch( t.params[0] ) {
                    case TPType(t): t;
                    default: throw "assert";
                };
                continue;
            }
            p = t.pack.copy();
            p.push(t.name);
            if( t.sub != null ) p.push(t.sub);
            break;
        default:
            Context.error("Relation type should be a type path", f.pos);
    }
    function e(expr) return { expr : expr, pos : pos };
    var get = {
        args : [],
        params : [],
        ret : t,
        expr: macro return @:privateAccess $p{p}.manager.__get(this, $v{f.name}, $v{relKey}, $v{lock}),
    };
    var set = {
        args : [{ name : "_v", opt : false, type : t, value : null }],
        params : [],
        ret : t,
        expr: macro return @:privateAccess $p{p}.manager.__set(this, $v{f.name}, $v{relKey}, _v),
    };
grepsuzette commented 3 years ago

Got the same exact problem as you.

filt3rek commented 3 years ago

Hej Emugel ! Just replace $i{ tname } with $p{ p } and then it works fine.

grepsuzette commented 3 years ago

Thank you sir!

I had one error with String instead of Array<String> (inside the macro), I really am not sure why, but your listing has removed a certain line tname = p.join(".");, for me it will work if I change this line to tname = p;, like below:

                             var ttype = t, tname;
                              while( true )
                                  switch(ttype) {
                                  case TPath(t):
                                      if( t.params.length == 1 && (t.name == "Null" || t.name == "SNull") ) {
                                          ttype = switch( t.params[0] ) {
                                          case TPType(t): t;
                                          default: throw "assert";
                                          };
                                          continue;
                                      }
                                      var p = t.pack.copy();
                                      p.push(t.name);
                                      if( t.sub != null ) p.push(t.sub);
~                                     // tname = p.join(".");
+                                     tname = p; 
                                      break;
                                  default:
                                      Context.error("Relation type should be a type path", f.pos);
                                  }
                              function e(expr) return { expr : expr, pos : pos };
                              var get = {
                                  args : [],
                                  params : [],
                                  ret : t, 
                                 expr: macro return @:privateAccess $p{tname}.manager.__get(this, $v{f.name}, $v{relKey}, $v{lock}),
                              };
                              var set = {
                                  args : [{ name : "_v", opt : false, type : t, value : null }],
                                  params : [],
                                  ret : t, 
~                                 expr: macro return @:privateAccess $p{tname}.manager.__set(this, $v{f.name}, $v{relKey}, _v), 
                              };

Do you see why?

Edit: even though it compiles and seems to work I am not sure at all it is correct, it would be better we inform of a likely regression here.

filt3rek commented 3 years ago

Hmmm I don't know why it throws you this error. I'm on Haxe 4, are you too ? And It works just fine with tname = p.join("."); but maybe It's a specific case I didn't noticed ?

grepsuzette commented 3 years ago

Right now Haxe 4.2.1. In your snippet you renamed tname to p, since there is a local var p below is it possible it's because of this?

I mean, according to the doc, $p{}: Array<String> -> Expr Generates a field expression from the given string array.. But you use $p{p} and not $p{tname}, so even if you reintroduce tname = p.join("."); it will not produce the String should be Array<String> error.

grepsuzette commented 3 years ago

I had actually misread your patch, the original var p = t.pack.copy(); became p = t.pack.copy(); in your code. So it seems a potential fix, thanks! Hope the lib can be updated though :)

jonasmalacofilho commented 3 years ago

@filt3rek

Can you please open a PR with your fix and a small test case?

(EDIT: please @ me directly on the PR so I can review/approve it, I'm not longer watching/officially maintaining this repository).

filt3rek commented 3 years ago

Hej Jonas !

Thanks for your reply even if you don't maintain this anymore ! I'm not really comfortable with PR and have not much time to look at that this time. I'll try to take a look at this as soon I've time.

That's pitty that noone want to maintain this lib anymore, I still use it in my projects and find it really light and useful. IMHO it needs just a big refactoring to put it up to date with new Haxe features...

Thanks anyway for your interest.

jonasmalacofilho commented 3 years ago

I'll try to take a look at this as soon I've time.

Let me know if I can help you get started.

And maybe in the future you can help maintain it too : )

filt3rek commented 3 years ago

Jonas,

I'll be on holiday soon, I'll contact you by PM and yes if you could help me get started and maybe talk a little about how I can help in maintaining this lib, I will be thanksful.

See you soon ;)