List.contains not working on 1.6.2

[ciscoheat]

When testing borrow_library_items.k with 1.6.2, it's now possible to borrow many of the same book, previously it wasn't. The check is made by a call to List.contains on line 112.

[ciscoheat]

Sorry, I was referring to my current version of the file. It's on line 87: https://github.com/jcoplien/trygve/blob/master/examples/borrow_library_items.k#L87

[jcoplien]

Den 04/03/2016 kl. 19.17 skrev Andreas notifications@github.com:

Sorry, I was referring to my current version of the file. It's on line 87: https://github.com/jcoplien/trygve/blob/master/examples/borrow_library_items.k#L87 https://github.com/jcoplien/trygve/blob/master/examples/borrow_library_items.k#L87 — Reply to this email directly or view it on GitHub https://github.com/jcoplien/trygve/issues/92#issuecomment-192390375.

Can you provide some sample input that reproduces the problem?

[ciscoheat]

Sure, enter 1234 as pin code, then select option 1 repeatedly. The list will eventually be filled with the same book more than one time.

[jcoplien]

Ah.

It’s amazing that this ever worked at all — frankly, I’m scratching my head as to why it did work. And I’d kind of like to understand that. But I know why it doesn’t work now. When you call contains, trygve just turns around and calls Java’s List.contains(x). That method in turn just goes through each item in the List (which all are wrapped in an RTObject) and invokes the equals method on x with successive List members as parameters, until it finds a match (and thereafter returns true) or comes to the end of the List (and returns false). Each of those checks of course invokes RTObject.equals(RTObject) inside the environment, which doesn’t have a clue about how to compare two Books, so you get undefined behaviour.

To fix this you need to add the following script to Book:

public int compareTo(Book other) {
    return if (name() == other.name()) 0 else -1 // or anything != 0
}

(Or maybe you want to check the ID as well. It’s up to you — you get to define what equality means — just like an American Republican.) Then everything fits together and all will be well. That is, RTObject.equals(RTObject)will do a lookup on your Book class and find that it has the wherewithal to compare two Books. (If you use this code verbatim it should work with 1.6.3. If you want to name the formal parameter more suitably — say, as book — you’ll need 1.6.4. There’s a 1.6.3 bug in that regard, which your example helped find.)

Not a bug. (But it did help find another one.)

I know this is a bit off-pissing because what once worked now doesn’t. But do you understand that it’s truly amazing that it worked at all? How in the hell should List know how to compare a book object with those in the list to see if they were the same?

Den 04/03/2016 kl. 20.27 skrev Andreas notifications@github.com:

Sure, enter 1234 as pin code, then select option 1 repeatedly. The list will eventually be filled with the same book more than one time.

— Reply to this email directly or view it on GitHub https://github.com/jcoplien/trygve/issues/92#issuecomment-192428286.

[ciscoheat]

Nice analysis, I'm not off-pissed at all. :) I thought it worked because it was using object identity for the comparison, and without a compareTo method, it would be the default?

[jcoplien]

I wonder if it did just use object identity? I can’t imagine how it would figure out to do that, though.

[jcoplien]

Can we close this? (BTW, I found that it was using object identity and that the introduction of the compareTo link took that away. I've restored the old behavior as the default — but it's still probably a good idea to add a compareTo method.)

[ciscoheat]

Yes no problem, it's working again and it's great to have a compareTo option. I'm working on the data model for the library example, I'll use it then.

[jcoplien]

Thanks, Andreas.

Den 07/03/2016 kl. 02.38 skrev Andreas <notifications@github.com mailto:notifications@github.com>:

Closed #92 https://github.com/jcoplien/trygve/issues/92.

— Reply to this email directly or view it on GitHub https://github.com/jcoplien/trygve/issues/92#event-579530908.