#1365 Method compareTo not called for '=' operator

[Erich]

For objects inheriting from Orderable, the compareTo() method isn't called for the '=' operator if the compared-to object is of a different class. Comparison result for '=' will always be false.

Here's a small example how to reproduce stand-alone:

c17 = .c~new(17)
d17 = .c~new(17) 

-- comparing c17 with d17 will call compareTo() for any operator
say (c17 <= d17) (c17 >= d17) (c17 = d17)  -- 1 1 1 

-- comparing c17 with 17 will call compareTo() for any operator except '='):
say (c17 <=  17) (c17 >=  17) (c17 =  17)  -- 1 1 0 

::class c public inherit Orderable

::method init
  expose n
  use arg n

::method string
  expose n
  return n

::method compareTo
  expose n
  use arg other
--say "compareTo" n other~string  
  return n~compare(other~string)  

[Rony G. Flatscher]

As in my posting in https://sourceforge.net/p/oorexx/mailman/message/35229331/ please find enclosed a patch (against trunk, ie 5.0.0) which should fix this problem. It also assures that all strict comparisons assert that other is of the same type as self (derived from using it originally in the methods "==" and "\==").

[Rick McGuire]

The change to the strict comparisons is just repeating the same mistake made in == and \==. The compareTo method should be free to decide the criteria used for strictness. For example, the DateTime class might be enhanced to allow comparison to string forms of the date. This would introduce a difference between how < and << works that does not occure with = and ==.

[Rony G. Flatscher]

This then would mean to just remove the test for self and other being instances of the same type in all strict comparison methods?

[Rick McGuire]

That test was the root cause for this bug and is the obvious fix. Reintroducing the same bug in some other location will just cause me to open another bug report.

[Rony G. Flatscher]

To be clear: I am not after a fight, but after a fix that is acceptable for everyone, which means to clarify the problem at hand first, if the supplied patch is not acceptable.

The cause of the problem that I saw was, that the compare method "=" was taken as a synonymous to a strict compare "==". Short of any other available information the code of "==" suggested that you defined "strict" at the time of writing to mean that both objects need to be of the same type, before continuing the comparison process.

If the "compareTo" method is supposed to carry out the "strict" semantics, it simply cannot do it, as it only is able to return "-1", "0", "1" depending on which object is regarded to be smaller, equal or larger. There is no way to indicate to "compareTo" to carry out its work with strict semantics.

However, in the case of "Orderable" it might be sufficient to not carry out any strict comparisons with assumptions that may not match the need of the programmers. If the behaviour of "Orderable" as implemented (whatever implementation) is not acceptable for the strict semantics, the programmer would always be free to implement the strict comparison methods him/herself.

[An alternative, which might be (although it may break existing programs) to leave out the strict comparison methods from Orderable or to make them simply abstract.]

Enclosed is a patch, that removes the tests in the strict comparison methods "==" and "\==" of "self" or "other" to be of the same type.

[Anonymous]

The purpose of the Comparable class was to make it simple to implement all of the comparison operators for another class. No more, no less. The mistake here was makeing "=" and "==" behave just like the base Object implementation and have a bypass. That was an error and should be fixed for BOTH operators.

It might be nice to have a StrictComprable class that also incorporated the concept of string comparisons using a strictCompareTo method, but that's the province of an RFE, not this bug report.

[Rony G. Flatscher]

Afterthought: actually, if a programmer overrode the strict comparison methods, then the default implementation of Orderable in the version_2 patch would cause problems, as the Orderable non-strict comparison "=" and "\=" would use the new strict versions, which may be different.

Hence the enclosed patch implements "=" and "\=" independent of "==" and "\==". "<>" and "><" forward to "\=".

[Erich]

Committed code fix with revision [r11122].
Added tests with revision [r11123].

[Erich]

The removal of if \other~isa(self~class) then return .false from the == and = methods of Orderable has an interesting side effect.

If a class which inherits from Orderable, decides to disallow comparisons with objects from another class by adding something like .ArgUtil~validateClass("other", other, .File) to their compareTo method (which is what our File class does) , then any comparison( * ) if object == .nil will fail with an error. (A comparison if .nil == object will still work.)

For whatever reason I had been under the impression, that ooRexx had implemented an exceptional shortcut for object == .nil comparisons to be identical to .nil == object but after re-reading this in rexxref, it seems that this shortcut only applies to String, not to other classes.

What does that mean to the above change?

Do we see this at the discretion of the implementing class? Should File class take care of that issue, if it wants to, or not, if it doesn't like so?

Or does this call for an RFE to make any object == .nil comparisons identical to .nil == object?

( * ) == is shown as an example here, the issue applies to any of the comparison operators

[Erich]

Should File class take care of that issue, if it wants to, or not, if it doesn't like so?
If so, how could it do it? As compareTo is restricted to return -1, 0, 1, it would have to inspect .context~stackFrames[2]~name to be able to figure out what to return. And then, what is the correct answer to something like object >= .nil ?

This doesn't sound like a valid path we should go ..

That would leave us with the option to go back to each of the Orderable comparison methods and add an explicit test for .nil before handing over to compareTo

[Rick McGuire]

The implementation of the comparison is always done by the object on the
left hand side of the operator, which is why "object == .nil" and ".nil ==
object" can return different results. In the first situation, the
comparison is handled by the object's == method, the second is handled by
.nil's, which only returns true based on object identity. The only special
exception that exists for .nil is in the implementation of the String
comparison methods.

There's really no was to fix this really in the context of how Orderable
works other than to maybe choose to raise a condition from the compareTo
method if you thought it was important to handle this situation. Orderable
is intended to make it simple to add all of the comparison methods to a
class where the comparison are simple without requiring a ton of individual
methods being coded up. For situations outside of that scope, one should
just create an interface like Orderable that fully meets your requirements.

On Sat, Aug 6, 2016 at 5:47 AM, Erich erich_st@users.sf.net wrote:

Should File class take care of that issue, if it wants to, or not, if it
doesn't like so?
If so, how could it do it? As compareTo is restricted to return -1, 0,
1, it would have to inspect .context~stackFrames[2]~name to be able to
figure out what to return. And then, what is the correct answer to
something like object >= .nil ?

This doesn't sound like a valid path we should go ..

That would leave us with the option to go back to each of the Orderable
comparison methods and add an explicit test for .nil before handing over
to compareTo

---

** [bugs:#1365] Method compareTo not called for '=' operator**

Status: accepted
Group: 5.0.0
Created: Mon Feb 22, 2016 09:22 PM UTC by Erich
Last Updated: Sat Aug 06, 2016 08:56 AM UTC
Owner: Erich

For objects inheriting from Orderable, the compareTo() method isn't called
for the '=' operator if the compared-to object is of a different class.
Comparison result for '=' will always be false.

Here's a small example how to reproduce stand-alone:

~~~~
c17 = .c~new(17)
d17 = .c~new(17)

-- comparing c17 with d17 will call compareTo() for any operator
say (c17 <= d17) (c17 >= d17) (c17 = d17) -- 1 1 1

-- comparing c17 with 17 will call compareTo() for any operator except
'='):
say (c17 <= 17) (c17 >= 17) (c17 = 17) -- 1 1 0

::class c public inherit Orderable

::method init
expose n
use arg n

::method string
expose n
return n

::method compareTo
expose n
use arg other
--say "compareTo" n other~string
return n~compare(other~string)
~~~~

---

Sent from sourceforge.net because you indicated interest in <
https://sourceforge.net/p/oorexx/bugs/1365/>

To unsubscribe from further messages, please visit <
https://sourceforge.net/auth/subscriptions/>

[Erich]

I believe I fully understand this, but with the above change to Orderable, we have to decide how to fix the now broken File.

f = .File~new('\temp'); if f == .nil then .. raises an error. In fact, even
.File~new('name', .File~new('\temp')) doesn't work any more, because init internally uses if dir == .nil ..

Same withDateTime (and TimeSpan and any other user-defined class inheriting from Orderable)
d = .DateTime~new; if d == .nil .. raises an error

Except for reverting above change, I can only see two alternatives: adding a .nil check to all Orderable comparison methods, or extending the "special exception" for String to any Object.

[Rick McGuire]

The fix to these classes is simple, just reverse the order of the
comparison arguments. The choice is simple, either fix the code in
Orderable so that compareTo is called in all situations and fix the
situations where it fails or decide not to fix this problem at all. If
equality methods keep the check in place, then I'd argue that ALL of the
comparison methods should have the same check. That would reduce the
utility a little, but would be consistent.

Rick

On Sat, Aug 6, 2016 at 7:32 AM, Erich erich_st@users.sf.net wrote:

I believe I fully understand this, but with the above change to
Orderable, we have to decide how to fix the now broken File.

f = .File~new('\temp'); if f == .nil then .. raises an error. In fact,
even
.File~new('name', .File~new('\temp')) doesn't work any more, because
init internally uses if dir == .nil ..

Same withDateTime (and TimeSpan and any other user-defined class
inheriting from Orderable)
d = .DateTime~new; if d == .nil .. raises an error

Except for reverting above change, I can only see two alternatives: adding
a .nil check to all Orderable comparison methods, or extending the
"special exception" for String to any Object.

You cannot extend the special exception to any Object. The behavior is
defined by the method implementation, so there is no way to have that
implemented automatically.

Rick

---

** [bugs:#1365] Method compareTo not called for '=' operator**

Status: accepted
Group: 5.0.0
Created: Mon Feb 22, 2016 09:22 PM UTC by Erich
Last Updated: Sat Aug 06, 2016 09:47 AM UTC
Owner: Erich

For objects inheriting from Orderable, the compareTo() method isn't called
for the '=' operator if the compared-to object is of a different class.
Comparison result for '=' will always be false.

Here's a small example how to reproduce stand-alone:

~~~~
c17 = .c~new(17)
d17 = .c~new(17)

-- comparing c17 with d17 will call compareTo() for any operator
say (c17 <= d17) (c17 >= d17) (c17 = d17) -- 1 1 1

-- comparing c17 with 17 will call compareTo() for any operator except
'='):
say (c17 <= 17) (c17 >= 17) (c17 = 17) -- 1 1 0

::class c public inherit Orderable

::method init
expose n
use arg n

::method string
expose n
return n

::method compareTo
expose n
use arg other
--say "compareTo" n other~string
return n~compare(other~string)
~~~~

---

Sent from sourceforge.net because you indicated interest in <
https://sourceforge.net/p/oorexx/bugs/1365/>

To unsubscribe from further messages, please visit <
https://sourceforge.net/auth/subscriptions/>

[Erich]

Committed fixes in revision [r11124]
Committed additional tests with revision [r11125]

This re-enables object .. .nil comparisons for =, ==, \=, \== and their aliases.
object .. .nil comparisons with other comparison operators (e. g. <, >>=) now issue an intentional error message. (All this applies to objects inheriting from Orderable only!)

I notice, that with String we have a precendent how string < .nil etc. works, but I didn't think this as appropriate and went ahead with an intentional error message.

I'm open for discussion, of course.

[Erich]

fixed error message inserts with revision [r11128]