ToString() causes function overload confusion

Monkey Forums/Monkey Programming/ToString() causes function overload confusion

Skn3(Posted 2014) [#1]
Hey,

Just found a potential bug or quirk perhaps..?

Basically, if you add ToString() to a class then monkey is unable to determine if it should use a string or object version of a function overload. I guess that if you add ToString() to a class you are saying this returns as a string, but it came as a complete surprise that this prevented monkey from treating my class as an object when needed. Perhaps when there is a string vs object overload battle like this it should favour the object overload always?

Strict

Function Main:Int()
	Local item:= New Item
	Test(123, "item", "extra")
	Test(123, item, "extra")
	Return 0
End

Class Base
End

Class Item Extends Base
	Method ToString:String()
		'this causes monkey to fail at determining overload
	End
End

Function Test:Void(value:int, data:String, extra:String)
End

Function Test:Void(value:int, data:Base, extra:String)
End



ziggy(Posted 2014) [#2]
While I agree I don't see it as a bug, just the way the compiler handles ambiguity by disallowing it, which is not a bad thing if you ask me.


Pharmhaus(Posted 2014) [#3]
Change Line 4
	Local item:Base = New Item


The Reason:
All objects that have a ToString() Method are handled like Boxes for strings.

An object will be automatically converted to an int, float or string value if that object provides a suitable To:Int(), ToFloat:Float() or ToString:String() method.


This is indeed confusing but I'm not sure if this is a bug.


marksibly(Posted 2014) [#4]
Back in the early days, Monkey generics were much more like Java's where generic containers could only hold objects, not ints, floats etc. So I added some auto object->value handling, like ToString, ToInt etc to make them easier to use.

It's not really needed now, and IMO causes much more hassle than it's worth so I'd love to remove it. But that'd be a relatively big language change that would likely affect some amount of existing code (no idea how much though), so it's probably not gonna happen unless there's a considerably large demand.

But anyway, this is 'by design' and technically not a bug.


ziggy(Posted 2014) [#5]
It's not really needed now, and IMO causes much more hassle than it's worth so I'd love to remove it. But that'd be a relatively big language change that would likely affect some amount of existing code (no idea how much though), so it's probably not gonna happen unless there's a considerably large demand.
Have you considered forcing a implementation of related interfaces to make this? I think it would be nicer and maybe a way to keep some sort of backwards compatibility (with minimal changes in most cases)?


Skn3(Posted 2014) [#6]
But anyway, this is 'by design' and technically not a bug.


Fair enough, it would be cleaner to remove this stuff, but in all the hours and hours and hours I have used monkey, this is the only time it has ever been an issue. So not really a big demand.


Nobuyuki(Posted 2014) [#7]
@marksibly

the biggest lib I know that uses autoboxing is the old non-brl json one. It relies on autoboxing in cases to return values from JSONValue / JSONData type things.

I would recommend a precompiler directive #AUTOBOXING_ENABLED which defaults to false in newer versions...


muddy_shoes(Posted 2014) [#8]
And round and round we go: http://www.monkey-x.com/Community/posts.php?topic=1068


ImmutableOctet(SKNG)(Posted 2014) [#9]
Okay, I'm a bit confused here. If the desired input was the derived class, and not the base class, it works fine. But if the desired input is the base class, it freaks out? This is very strange to me, because the base class doesn't even have a 'ToString' implementation, but the derived class does.

So, why does the super/base class suffer from the implementation of a derived class? This sounds like the same intrusive logic applied to method overloads (Which is still ridiculous by the way).

So, if I understand this correctly; I can't make an overload for a function taking my base-class without making that class final, otherwise someone could break everything simply because they implemented 'ToString' and I didn't? But then, if I did implement it, I'd have a similar problem.

Inheriting classes and associated code should not break my own code / my base/super code, nor should my code break inheriting code. This just seems logical to me, and it's exactly why I think the current way overloads are handled between classes is complete garbage. I get the whole "Call the super-class's implementation, so your class can keep existing overloads" thing, that's awesome. But being completely locked down by the arguments my super-class describes is just frustrating and illogical. Why can't 'Trans' just pickup where it left off with names at compile-time? In the case of a method named 'M', the names from the base class would be: p_M, p_M2, and p_M3, so then why can't the derived class just continue from there: p_M4, p_M5, etc?

Anyway, my ranting aside, I suggest a bug-fix which takes the hint when you write an explicit overload. You know, without getting confused because of an UNRELATED inheriting class which wasn't even imported via the module containing the super class to begin with. Just have it pick overloads which inherit 'Object' over other data-types if one is present. And from there, keep preferring the lowest possible class (And I mean that with the mindset that inheritance goes from top (Object) to bottom (Final class(es) in the tree)) compatible with the type at hand. That'll fix this problem, it shouldn't be a hard algorithm to implement (Assuming all the meta-data's there). With that in 'Trans', it'd effectively fix everyone's problems.

SKN3's code was not at all ambiguous, and the compiler really should know this. I don't think anyone of sound mind would expect this to happen, especially with Monkey's modular philosophies.

You know what, I might just sit down and fork 'Trans' when I have the time.

Also, @Nobuyuki: I'm not sure if that's the same situation. Does the 'json' module expect classes to never be treated as objects before primitive/other data-types in the case of overloads? If it's just a matter of treating an object like a string for example, then as long as there isn't an overload expecting an 'Object', or a super-class to that class, it shouldn't even have an issue (With, or without the fix I described). Plus, in the end people can just call 'ToString', 'ToInt', and 'ToFloat' manually anyway.


Danilo(Posted 2014) [#10]
Skn3 wrote:
it would be cleaner to remove this stuff

+1


Nobuyuki(Posted 2014) [#11]
@ImmutableOctet

as far as I can tell, since there are multiple autobox methods, you have to either call one manually or perform a cast to a primitive, whereby it uses the appropriate unbox function.


ziggy(Posted 2014) [#12]
I would not remove it, as it could be handy sometimes. I would make it based on an interface. Classes with ToString() that also implements iAutoBoxString (or the like) should be the ones that auto box. That's for me a cleaner way to do it IMHO.

Also, the compiler should be more expressive on errors. If he can't tell which overload to use, it would be nicer to list the overload candidates, so it's easier to refine the call instruction to remove ambiguity on the call itself, when possible.It does it very well.


Skn3(Posted 2014) [#13]
I would not remove it, as it could be handy sometimes. I would make it based on an interface. Classes with ToString() that also implements iAutoBoxString (or the like) should be the ones that auto box. That's for me a cleaner way to do it IMHO.


Oh, wasn't sure what you meant earlier. Yeah this would be a great way to deal with it!


ziggy(Posted 2014) [#14]
I would add that, in my honest opinion, in case of potential overload clashes, the not auto-converted call should have preference always. While I agree that disallowing ambiguity is nice, in this case I think it would be very sensible to treat this scenario as an exception to the general rule. I mean, if an object has ToString(), but there's a call that can be resolved without auto conversion, this is the call that should be supposed to be used to solve ambiguity, when solving overloading calls.


Danilo(Posted 2014) [#15]
ziggy wrote:
if an object has ToString(), but there's a call that can be resolved without auto conversion, this is the call that should be supposed to be used to solve ambiguity

Already works like this. When used with "class Base" it works, because there is no conflict.
It just does not know for "class Item" if it should take the ToString() or take the downcast to "class Base".
You have to tell it what to use, by explicit casting.
Strict

Function Main:Int()
    Local item1:= New Base
    Print item1
    Local item2:= New Item
    Print item2
    
    Print "-----"
    
    Test(123, "item", "extra")
    Test(123, item1, "extra")
    'Test(123, item2, "extra") ' downcast item2 to Base or use ToString automatically?

    Print "-----"

    Test(123, String(item2), "extra") ' cast to String
    Test(123, Base(item2)  , "extra") ' cast to Base
    
    Return 0
End

Class Base
    Method ToString:String()
        Return "Base.ToString()"
    End
End

Class Item Extends Base
    Method ToString:String()
        Return "Item.ToString()"
    End
End

Function Test:Void(value:int, data:String, extra:String)
    Print "Test() with data:String"
End

Function Test:Void(value:int, data:Base, extra:String)
    Print "Test() with data:Base"
    Print "  - "+data.ToString()
End



ImmutableOctet(SKNG)(Posted 2014) [#16]
To quote myself:

That'll fix this problem, it shouldn't be a hard algorithm to implement (Assuming all the meta-data's there). With that in 'Trans', it'd effectively fix everyone's problems.



After some detailed investigation, it seems that the meta-data is technically there, but unfortunately it can't be passed without a work-around (Or at least refactoring 'SemantArgs'). I may still write that work-around, but I'll have to see how it goes. The problem is that the function/method call expression container can't pass the data of the expression to 'SemantArgs', so it can do a deeper form of check. The result is unintentional conversion to the wrong overload, but then 'SemantFunc' doesn't get the memo and errors. In other words, currently, it can only know about one overload at a time (When it doesn't know if it should call 'ToString') so it'll just pick the first overload. The problem is, the compiler actually out-smarts itself right after that, and then proceeds to produce an error.

Anyway, if my theory is correct, my idea will work, but it needs some work to be fixed.


EDIT: See below.


ImmutableOctet(SKNG)(Posted 2014) [#17]
Turns out I was chasing a red herring, but regardless, I fixed it. Originally, I was going to use a far more complicated procedure, but then I realized I was over-complicating things. Here's the fix thus far. I haven't really dealt with GitHub publicly, but I'd assume all I need to do is make a pull-request once I've tested it further. If anyone can find a flaw with this hack please let me know. (Testing is appreciated)

It's just a "counting" hack, but it doesn't produce an error for me.

I've got to give Mark credit, sure 'Trans' is one of the messiest modules I've ever seen, but it's well made.


marksibly(Posted 2014) [#18]
This is not a bug.

For better or worse, the ToString method means objects of class Item are implicitly castable to String and, according to the rules of Monkey function overloading, this means the second call to Test() is ambiguous. To resolve it, you can use: Test(123, Base(item), "extra").

I don't like the idea of making an exception here either. If someone is actually using the 'implicit cast to string' feature, they're gonna be potentially just as confused when it's quietly *not* used in some cases. I think the desire for making ToString a 'second class' cast really just reflects the desire to get rid of it altogether, and making a special case for it sort of has the opposite effect.

> I would recommend a precompiler directive #AUTOBOXING_ENABLED which defaults to false in newer versions...

I'm no totally against this, but would probably prefer to use something like #MONKEY_VERSION=100, GLSL style, so instead of the compiler having to support N*M combinations of features, it only has to support N language versions.


ImmutableOctet(SKNG)(Posted 2014) [#19]
As someone who uses 'ToString' for other purposes, this is illogical to me. The better question is, if there's an overload for the actual type, why not use it? You're not really giving an answer, you're just beating around the bush and saying it'd be confusing. The fact is many of us would expect an explicit overload for THE VERY TYPE we passed in to be prioritized over some form of conversion or otherwise. Not only is this the logical conclusion, but technically speaking 'ToBlah' is a language hack itself which in my opinion should not take priority over code that was obviously built without it in mind. I'm not saying to get rid of it, as it does work quite well, and is used by plenty of people, I'm saying to not prioritize it / let it be seen as just as valuable as the actual super-class. My entire problem is exactly how 'ToBlah' was added, not that it was added. Also, the only times it would "Quietly not be used" is when there's a more logical overload to begin with. Not when there's only an overload which takes the type you can/want to convert to. Ask yourself this; would you rather use the overload which clearly takes the type you're giving, or let the compiler get hung up about things being "ambiguous" when that may very well have not been the case for the originally targeted class. Again, this only really happens when you're dealing with super-classes, meaning you could technically break something from someone else's module, simply because they decided they wanted to do something 'Trans' is too "ignorant" to detect. For a modular language this sounds way too intrusive to me. Now, to fix this issue, you could either rewrite half of trans, or you could just use something like my fix. I know you created the language, and thus should have control over it, but the whole "I won't let you make the language more logical because I never thought about every single situation." argument is just petty.

I suggest you take the time to think about this, as my post above states quite clearly how this could break someone else's code for no logical reason. If that's your final decision fine, but you honestly aren't making any sense to me. Sure my hack is just that, a hack, but the point was illustrate the idea, not necessarily provide the final implementation for future versions (Thus the whole "Development" branch thing).

EDIT: Tried to make a few things clearer.


muddy_shoes(Posted 2014) [#20]
It's kind of pointless to argue this. As the thread I linked to above illustrates the issue was raised by me early on in Monkey's development. It has subsequently caused a number of people issues. Every time the response, if there is one, is the same opinion that throwing an error over "ambiguous" method signatures is less confusing than having a resolution mechanism that considers the type/subtype as a better match than the "ToPrimitive" type. That makes no sense to someone from a Java or similar background* where those resolution rules are generally understood but there you have it. When Mark states that having to cast an object to its own type makes more sense to him that having the compiler recognise that type over the unboxing conversion you pretty much have to shrug and accept that he's coming from rather different assumptions about how an OO language should work.

*And it makes less sense in the context that the feature itself is based on Java. Why borrow a bit of functionality and ignore the deeper design thought that makes it successfully usable? Again, *shrug*.


ImmutableOctet(SKNG)(Posted 2014) [#21]
@muddy_shoes: So your argument is that since Java does it one way, Monkey automatically has to do it the same way? This isn't Java, it's Monkey, they aren't the same language. Monkey should have its own reasons for doing something a specific way. That being said, if Java does actually care about the obviously explicit 'super-class overload', what's Monkey's argument against this? Confusion? Hardly, the point of the other overload is to be explicit in the first place, and it's not like you can't just call 'ToBlah' anyway. The difference between the argument to do an unnecessary cast to your super-class versus calling 'ToBlah' is that 'ToBlah' is not always relevant (And 'ToBlah' acting like a language-construct to begin with), not to mention that casting to your super-class is counter-intuitive. Monkey can have it's differences, I'm completely for that without a doubt, but that doesn't mean there shouldn't be real reasoning behind the language's choices and designs. The point I'm trying to make is that Monkey can be like Java in many ways, but there should be nothing saying it should follow everything Java does exactly. Just as we should only follow Java if a valid argument is made.

What needs to be addressed is that primitives are being treated as valid super-classes internally. 'ToBlah' itself is a weird, but reasonably integrated (Method-wise), language construct. It technically doesn't even belong in 'ExtendsType', and that's exactly the problem in the first place; 'ExtendsType' uses 'ToBlah' as a fall-back when it really shouldn't. Those overloads are only ambiguous to 'Trans' because of the specific way 'ToBlah' was hacked in. Again, I'm not saying it shouldn't be there, but some of the code should probably be moved around in 'Trans' so this isn't even an issue. Either that, or we could just use my lazy hack for the time being.

So wait a minute, this has happened many times, you say? Gee, I wonder if that's because choosing the legitimate super-class is the logical decision. And Mark saying the decision was to let it continue to cause errors about illegitimate ambiguity, because? What's worse about this is that the language teaches you in every other situation that fitting in your super-class's shoes is a good design choice. But all of the sudden overloads come in and it's magically not the best choice? I call shenanigans on that one.

The argument for having to cast to your parent/super type is like saying you should cast to the exact derived type an object actually is every time you use it. It's an anti-virtual way of thinking, and it's quite frankly a bad choice to me.

And as I said before, I know it's Mark's decision, but that doesn't mean I shouldn't try to change his mind.

To make a long story short, however, I seem to be in agreement with you, even if your post confused me initially.

EDIT: I should state that my stance of this has changed a bit. Mark's right, this isn't a bug, but that doesn't mean it's not an issue.


muddy_shoes(Posted 2014) [#22]
My argument/position on how I feel this feature should most reasonably work is both above and in the thread I linked. It isn't "Java does this, so Monkey should too". At most it's "You cribbed this from Java so perhaps you should have made use of the time and effort those designers put into figuring out how the feature fits into the language overall", but primarily it's "This feature doesn't deliver what it's meant to the way it currently works in Monkey".

My argument about the value of putting energy into persuading Mark that he's made a bad decision here is also above. The two things are entirely different points and it's probably best not to confuse the two or put words into my mouth while doing it. If you want to spend your time trying to change Mark's mind about this despite the fact that it has been argued several times over three years or so then be my guest. I'd suggest that his heels are firmly planted though.


Nobuyuki(Posted 2014) [#23]
I'd also see no problem with it right now, if only for the fact I almost never use autoboxing. As long as the language doesn't creep into the territory where there's 50 million "don't use that" features, I don't see what problem there is with keeping this around. The same could be said with sigils and things of that nature, although I think with each new iteration the language evolves past its BB roots and in the meantime it doesn't seem to be hurting anyone.