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

haxe.macro.Compiler.setFieldType #57

Open
filt3rek opened this issue Dec 25, 2020 · 11 comments
Open

haxe.macro.Compiler.setFieldType #57

filt3rek opened this issue Dec 25, 2020 · 11 comments

Comments

@filt3rek
Copy link

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),
};

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
Copy link
Author

filt3rek commented Dec 26, 2020

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
Copy link
Contributor

Got the same exact problem as you.

@filt3rek
Copy link
Author

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

@grepsuzette
Copy link
Contributor

grepsuzette commented May 31, 2021

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
Copy link
Author

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
Copy link
Contributor

grepsuzette commented May 31, 2021

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
Copy link
Contributor

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
Copy link
Member

jonasmalacofilho commented Jun 1, 2021

@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
Copy link
Author

filt3rek commented Jun 1, 2021

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
Copy link
Member

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
Copy link
Author

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 ;)

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

No branches or pull requests

3 participants