bmx-ng / bcc

A next-generation bcc parser for BlitzMax
zlib License
33 stars 12 forks source link

Function pointers: reflection.mod does no longer compile (Regression) #263

Closed GWRon closed 7 years ago

GWRon commented 7 years ago

With most current revision brl.mod/reflection.mod fails to compile:

Compile Error: Unable to find overload for sort(Int,Int(TFunction,TFunction)). Argument #2 is "Int(TFunction,TFunction)" but declaration is "Int(Object,Object)". 
[/BlitzMaxNG/mod/brl.mod/reflection.mod/reflection.bmx;1925;0]
Build Error: failed to compile (65280) /BlitzMaxNG/mod/brl.mod/reflection.mod/reflection.bmx

I think it is related to @HurryStarfish's adjustments (or its increase regarding "strictness" which bcc-ng seemingly missed before).

woollybah commented 7 years ago

Are you sure? It's working here. Was fixed in a recent commit.

GWRon commented 7 years ago

I used the most current revision (fetched your latest commit). Compiled that with vanilla.

Fetched latest brl.mod and pub.mod.

Executed $ ./bmk makemods -r -a

[ 74%] Archiving:enet.release.linux.x86.a
[ 74%] Archiving:freejoy.release.linux.x86.a
[ 74%] Archiving:libpng.release.linux.x86.a
ar: ar: Erzeugen von /BlitzMaxNG/mod/brl.mod/stream.mod/stream.release.linux.x86.a
Erzeugen von /BlitzMaxNG/mod/brl.mod/socket.mod/socket.release.linux.x86.a
ar: Erzeugen von /BlitzMaxNG/mod/pub.mod/enet.mod/enet.release.linux.x86.a
ar: Erzeugen von /BlitzMaxNG/mod/pub.mod/freejoy.mod/freejoy.release.linux.x86.a
ar: Erzeugen von /BlitzMaxNG/mod/pub.mod/libpng.mod/libpng.release.linux.x86.a
[ 74%] Compiling:vulkan.bmx.release.linux.x86.c
Compile Error: Unable to find overload for sort(Int,Int(TFunction,TFunction)). Argument #2 is "Int(TFunction,TFunction)" but declaration is "Int(Object,Object)". 
[/BlitzMaxNG/mod/brl.mod/reflection.mod/reflection.bmx;1925;0]
Build Error: failed to compile (65280) /BlitzMaxNG/mod/brl.mod/reflection.mod/reflection.bmx

I also did all steps again to make sure I am not using something outdated.

woollybah commented 7 years ago

Hmm... yeah, I think that "bonus" feature broke it...

GWRon commented 7 years ago

I know that this new feature will break some things - but all in all it might be a great addition - once it is working. It is better to have bcc run on solid grounds than having some features work because some "bugs" make bcc just not caring about a specific thing.

GWRon commented 7 years ago

This is not fixed yet.

Same error:

[...]
[ 74%] Compiling:vulkan.bmx.release.linux.x86.c
Compile Error: Unable to convert from Int(TFunction,TFunction) to Int(Object,Object).
[/BlitzMaxNG/mod/brl.mod/reflection.mod/reflection.bmx;1925;0]
Build Error: failed to compile (65280) /BlitzMaxNG/mod/brl.mod/reflection.mod/reflection.bmx
woollybah commented 7 years ago

Indeed not. My dev bcc appears to be out of sync with the repo. Still, mine works ;-)

GWRon commented 7 years ago

You should not cherry pick (pun intended) :-)

PS/Reminder: once this works "flawless" it would be a good idea to bring all releases of bmx-ng to a common level (Win, Linux, Mac, RasPI) - shouldn't be that troublesome with your builder/create-script.

HurryStarfish commented 7 years ago

This was not caused by the latest fix/bonus feature. It is simply a bug in BRL.Reflection. You can check back the description of my "Proper support for function pointers" pull request, I mentioned/explained it there. I am planning to create a pull request with a couple Reflection improvements shortly, which will address this.

GWRon commented 7 years ago

Reflection should still behave similar to "vanilla reflection". Else you should try to backport everything to "vanilla/brl.mod/reflection.mod" (and best : incorporate grable's "extended reflection" (as I am using it :-)).

I created this issue as it helps tracking problems arising from certain changes.

So if I got it right now: function(obj:object) cannot be replaced with function(obj:TFunction) when used as param. Yes, this indeed is not allowed in vanilla and could therefore get "fixed" in NG too (except there is a valid reason to keep it).

Edit:

SuperStrict
Framework Brl.StandardIO

Function CallF( f:int(o:object) )
    f(null)
End Function

Function objFunc:int(o:object)
    print "objFunc"
End Function

Function typeFunc:int(t:TType)
    print "typeFunc"
End Function

Type TType
End Type

CallF( typeFunc )

Should fail... as it does in vanilla. And your PR made it fail in NG too.

For some convenience: this is your Pull request

243

HurryStarfish commented 7 years ago

It shouldn't matter whether it's a function pointer variable or param: EnumFunctions in BRL.Reflection currently tries to pass Int(TFunction, TFunction) to a place where Int(Object, Object) is expected, which is neither allowed in vanilla, nor with the "bonus feature", because it's unsafe and can lead to crashes. It's a BRL.Reflection bug, which simply wasn't noticed until the "Proper support for function pointers" pull request. So the recent "bonus feature" doesn't need to be removed, it's unrelated.

EDIT regarding the code example you edited in up there: Yes, this fails in vanilla, and also fails with the "bonus feature". It has to, because otherwise you could do a call like f("hello") inside CallF and that means you'd be passing a String to a function that expects a TType.

GWRon commented 7 years ago

So 49b31731c1f9c6dc3f618532781ea94821a3e673 corrects no typo but is wrong?

@ reflection bug I think I understood already - especially as I am doing that compare-casts in my game more than a handful times and it is also advised so in the "blitzmax help files".

HurryStarfish commented 7 years ago

So 49b3173 corrects no typo but is wrong?

Yes. It's what I tried to explain here. I did make two suggestions on how to fix Reflections here, but as previously mentioned, I am also planning to create a pull request for it soon.

GWRon commented 7 years ago

So the commit should either get reverted - or a new one should correct it (with annotating a bit more - so things wont get mixed up that easily). At least I needed a bit to "understand". And see how I already forgot it again when the same problem appeared another time.

woollybah commented 7 years ago

Should be fixed now. I also updated the reflection module.

HurryStarfish commented 7 years ago

Reflection should still behave similar to "vanilla reflection". Else you should try to backport everything to "vanilla/brl.mod/reflection.mod" (and best : incorporate grable's "extended reflection" (as I am using it :-)).

I agree. I use the "extended reflection" in my vanilla BlitzMax too. But NG reflection also has a few other issues I am currently looking into (such as overloads not being handled correctly, or SuperType() always returning Object for interfaces).

GWRon commented 7 years ago

Ahh, so extending it to support the new features.

Would be nice to have full support to expose const and globals. But feature wishes are better placed in a more appropiate issue/"Thread".