blitz-research / monkey

Blitz Research Monkey Source
225 stars 59 forks source link

Fixed "unable to determine overload" overloading error when giving correct types #55

Closed TacoOblivion closed 8 years ago

TacoOblivion commented 10 years ago

This fixes the bug mentioned here on the forums. I tested it pretty well although without some sort of confirmation, I cannot say for sure if there's some very rare case that would cause this to cause a fail.

blitz-research commented 10 years ago

Just to reiterate, the issue you posted about on the forums is NOT bug! It's just the way Monkey works: if there is more than one 'inexact match', the overload fails. In your sample code, since StringList<>Object, there are NO exact matches.

Anyway, nice try with the tweak but it breaks (at least) this code:

Class C
End

Class D Extends C
End

Class E Extends D
End

Function Bug( c:C )
    Print "C!"
End

Function Bug( d:D )
    Print "D!"
End

Function Bug( e:E )
    Print "E!"
End

Function Main:Int()
    Bug( New C )
    Bug( New D )    'Now Broken!
    Bug( New E )
End

But in general, any fix that allows for implicit conversion to a base class is very likely going to have to take 'type distance' into account. eg: in the above code, you could consider the distance from C to C 0, the distance from D to C 1, the distance from E to C 2, and the distance from E to D 1 etc.

The 'best match' could then be considered the match with the least distance, eg:

Class C
End

Class D Extends C
End

Class E Extends D
End

Function Bug( c:C )
    Print "C!"
End

Function Bug( d:D )
    Print "D!"
End

Function Main:Int()
    Bug( New E ) 'what gets called?
End

Here, we probably want Bug( d:D ) to be called, as D is the 'nearest' to E, aka 'most specific'.

Complicating factors include: what is the 'distance' between int/float etc? What about multiple params where each param is a different distance? What about interfaces? What about null?

All in all, it gets pretty complicated, eg: http://docs.oracle.com/javase/specs/jls/se5.0/html/expressions.html#15.12.2

Also, with all these magic rules in place, unexpected stuff does happen now and then (eg: a method you are unaware of ends up getting called). I decided when writing Monkey to keep it simple to save people from such surprises, but I now think that was probably a mistake as the extra power is worth the initial confusion in the long run, and I may take another look at all this again at a later date.

TacoOblivion commented 10 years ago

I see a way to fix the above mentioned code to act as expected and to deal with the 'distance' of multiple parameters as well as null. I don't know about interfaces or int/float as those haven't been an issue. I will push it after I make the fix. Then you can take another look.

Edit: The fix is slightly larger than I envisioned, but not horrible. It'll probably take me all day after I wake up to write a proper fix because I don't know trans code very well.

blitz-research commented 10 years ago

Okay, but to be honest I'll need quite a bit of convincing to include this - this stuff has been tweaked considerably over time and I really don't want to break it!

TacoOblivion commented 10 years ago

Well, take a look at this. I'm thinking that it would be better for you to write it otherwise it gets messy because I don't know trans well enough.

I tested it a lot and confirmed that both C# and Java conform to this.

Class C
End

Class D Extends C
End

Class E Extends D
End

Function Bug( c:C, d:D, c2:C )
    Print "C!"
End

Function Bug( d:D, c:C, d2:D )
    Print "D!"
End

Function Bug( d:D, d2:D, d3:D )
    Print "E!"
End

Function Main:Int()
    Bug(New E(), New E(), New E()) 'E
    Bug(New D(), New C(), New D()) 'D
    Bug(New C(), New D(), New C()) 'C
    Bug(New C(), New D(), New D()) 'C
End

Note: I am not done yet. I just got a lot of it working and wanted to show that it's not too bad. I still have to get null, which in my C# and Java tests finds the datatype furthest from Object.

TacoOblivion commented 10 years ago

Question, is implicit conversion supposed to be allowed for boxes? Because it's blocked by my changes now.

That is, is this code wrong? (I was surprised that it worked on v78e. The principle of least astonishment is being broken.)

Local test:Object = Object(IntObject(33))
Print IntObject(test).value