Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The .equal? method should not be mapped to .isEqual for Java objects (e.g. Joda LocalDate) #5990

Closed
ikaronen-relex opened this issue Dec 4, 2019 · 4 comments · Fixed by #6000
Closed

Comments

@ikaronen-relex
Copy link
Contributor

@ikaronen-relex ikaronen-relex commented Dec 4, 2019

Environment

  • jruby 9.2.8.0 (2.5.3) 2019-08-12 a1ac7ff OpenJDK 64-Bit Server VM 11.0.5+10 on 11.0.5+10 +jit [darwin-x86_64]

Expected Behavior

The following RSpec should pass:

describe Java::OrgJodaTime::LocalDate do
  it 'correctly implements reference equality' do
    a = Java::OrgJodaTime::LocalDate.new(2018, 12, 4)
    b = Java::OrgJodaTime::LocalDate.new(2018, 12, 4)
    expect(a == b).to be_truthy
    expect(a.__id__ == b.__id__).to be_falsy
    expect(a.equal? a).to be_truthy
    expect(a.equal? b).to be_falsy
  end
end

Actual Behavior

The last expect fails, since a.equal? ends up calling the Java isEqual method on the LocalDate object, which implements value equality.

Per https://ruby-doc.org/core-2.5.3/BasicObject.html#method-i-equal-3F, the equal? method should never be overridden and should always implement object identity comparison. However, some Java classes (notably including any JodaTime classes inheriting from AbstractPartial) may have an isEqual method which implements some other, weaker type of comparison and which JRuby automatically calls when equal? is called on such an object from Ruby.

IMO this is not a bug in JodaTime, since it's a pure Java library that is not designed with either Ruby's conventions or JRuby's idiosyncratic method resolution in mind. Instead, I would consider it a bug in JRuby itself: in attempting to bridge the different method naming conventions in Ruby and Java, it should also respect other established conventions of the two platforms and the "principle of least surprise", e.g. by not overriding equal? for classes whose author did not intend to do so.

This is particularly relevant since JRuby itself ships with JodaTime, which includes classes that trigger this bug.

FWIW, we've worked around this issue in our own codebase with a monkey patch like this:

class Java::OrgJodaTime::LocalDate
  def equal?(other)
    __id__ == other.__id__
  end
end

which restores the expected behavior. However, this is really only hiding the problem, and in any case only addresses it for one single class.

@ikaronen-relex
Copy link
Contributor Author

@ikaronen-relex ikaronen-relex commented Dec 4, 2019

Furthermore, this bug has consequences beyond just producing unexpected truthy comparisons. For example, the following spec will fail with a Java::JavaLang::ClassCastException: ReadablePartial objects must have matching field types:

describe Java::OrgJodaTime::LocalDate do
  it 'does not crash when compared to a LocalDateTime' do
    a = Java::OrgJodaTime::LocalDate.new(2018, 12, 4)
    b = Java::OrgJodaTime::LocalDateTime.new(2018, 12, 4, 0, 0)
    expect {a.equal? b}.not_to raise_exception
  end
end

Such broken comparisons can also happen unexpectedly in the background. For example, replacing the expect line in the spec above with expect([a]).to include(b) not only fails (as it should, since they're different kinds of objects) but also crashes when RSpec tries to generate a diff of the lists being compared.

@headius
Copy link
Member

@headius headius commented Dec 12, 2019

Odd...I would not have expected us to map this in this way. Agree, equal? should just be as default, an identity check.

@headius
Copy link
Member

@headius headius commented Dec 12, 2019

I see the issue here; because we try to turn boolean isSomething methods into their equivalent question-mark methods for Ruby, we are breaking the standard equal? behavior. I believe we should make equal? a reserved name and not do this alias.

headius added a commit to headius/jruby that referenced this issue Dec 12, 2019
@headius
Copy link
Member

@headius headius commented Dec 12, 2019

We do have an overridden equal? in the JavaProxy class that wraps all Java objects; it knows how to properly unwrap the contained objects and do the identity check there. Your workaround is actually going to be checking identity equality of those wrappers, so it will fail if the same object ends up in different wrappers (we do not guarantee idempotency of the wrapper object).

#6000 will fix this by marking equal? as a reserved name.

@headius headius added this to the JRuby 9.2.10.0 milestone Dec 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants