TextEnvelope equals is not symmetric

[g4s8]

cactoos/src/main/java/org/cactoos/text/TextEnvelope.java

Lines 79 to 90 in 100885a

	public final boolean equals(final Object obj) {
	return new Unchecked<>(
	new Or(
	() -> this == obj,
	new And(
	() -> obj instanceof Text,
	() -> new UncheckedText(this)
	.asString()
	.equals(new UncheckedText((Text) obj).asString())
	)
	)
	).value();

From Java Object docs:

The equals method implements an equivalence relation on non-null object references:

It is reflexive: for any non-null reference value x, x.equals(x) should return true.
It is symmetric: for any non-null reference values x and y, x.equals(y) should return true if and only if y.equals(x) returns true.
It is transitive: for any non-null reference values x, y, and z, if x.equals(y) returns true and y.equals(z) returns true, then x.equals(z) should return true.
It is consistent: for any non-null reference values x and y, multiple invocations of x.equals(y) consistently return true or consistently return false, provided no information used in equals comparisons on the objects is modified.
For any non-null reference value x, x.equals(null) should return false.

But TextEnvelope equals() implementation is not symmetric:

class Env extends TextEnvelope {
  public Env(String str) {
    super(new TextOf(str));
  }
}

class Txt implements Text {
  private final String str;
  public Txt(final String str) {
    this.str = str;
  }
  @Override
  public String asString() {
    return this.str;
  }
}

final String str = "test";
Text env = new Env(str)
Text txt = new Txt(str)
env.equals(txt) // returns true
txt.equals(env) // returns false

[0crat]

@llorllale/z please, pay attention to this issue

[0crat]

@g4s8/z this project will fix the problem faster if you donate a few dollars to it; just click here and pay via Stripe, it's very fast, convenient and appreciated; thanks a lot!

[llorllale]

@g4s8 you rolled your own implementation of Text with Txt in your example... that means you didn't extend TextEnvelope, and I see that you also didn't implement equals(). Hence this happens: txt.equals(env) // returns false

[g4s8]

@llorllale that's true, but I think it's a valid case, since anybody can implement Text interface without equals() or with standard equals() implementation, e.g.

class MyText implements Text {
   ...
  @Override
  public void equals(Object other) {
    if (!(other instanceof MyText)) {
      return false;
    }
   // other checks
  }
}

So I'd suggest adding type check in TextEnvelope.equals, it will make it symmetric, because equals method is marked as final, so all subclass equals calls will go to TextEnvelope implementation.

[llorllale]

@g4s8 TextEnvelope#equals already has a type check on line 84. I don't understand where the issue is.

[g4s8]

@llorllale but it checks that other object is a Text, I think it should check with TextEnvelpe:

             new And( 
-                () -> obj instanceof Text, 
+                () -> obj instanceof TextEnvelope,
                 () -> new UncheckedText(this)

[llorllale]

@g4s8 oh, I think I see what you mean now.

My question is: would this change really be useful? Is it causing an actual bug somewhere? Or is this suggestion entirely motivated from a purist point of view?

[g4s8]

@llorllale it's motivated from library user point of view: I may not check source code of TextEnvelope before using it, and I can easily implement Text somewhere with equals method, so I may get unpredictable bug which will depend on order of method calls, e.g. if I have text implementtion SomeText and TextEnvelope subclass FormattedText, I'd expect that code:

new SomeText().equals(new FormattedText("..."))

will return same result as

new FormattedText("...").equals(new SomeText())

[victornoel]

@g4s8 @llorllale aren't you in the middle of a discussion trying to solve all that is bad with using inheritance? :)

Envelopes are not meant to implement behaviour but delegate to a wrapped object.

I personally think that TextOf or Text.Default could implement equals, hashCode and toString and take a Scalar so that all the implementation of Text that want to have the same behaviour simply delegate to it. In this case, TextEnvelope will be useful to simplify the task of delegating.

Or introduce an AbstractText, stop robbing envelopes of their meaning and completely accept that you are doing inheritance.

See llorllale/cactoos-matchers#135 for a similar discussion.

[llorllale]

@victornoel nope - @g4s8 is pointing to a deeper problem that would apply to your solution as well.

[victornoel]

@llorllale I understand that, but I believe we would be better served by having a real TextEnvelope and then a ScalarText that can implements equals properly. I think also that if you do that, then the equals method of ScalarText just need to compare the value of Text#asString() with its own and the symmetry problem is solved if I'm not mistaken.

[llorllale]

@g4s8

I can easily implement Text somewhere with equals method, so I may get unpredictable bug which will depend on order of method calls

Two Texts are equal solely if their string contents (defined as the output of asString()) are equal. If your custom Text does not honor this then it's not implementing the contract and it's violating LSP.

[g4s8]

@llorllale but what if I have a Text with dynamic asString result? For instance text based on file:

class FileContent implements Text {
  private Path file;
  
  @Override
  public String asString() {
    return new String(Files.readAllBytes(this.file));
  }
}

in that case I'd say that two instances are equal if they have the same state (file), but checking asString won't get real state of the instance, since file content may change, and equals may return false-positive results, So I'd implement equals like this:

public boolean equals(Object obj) {
  return Objects.equals(this.file, FileContent.class.cast(obj).file);
}

[victornoel]

in that case I'd say that two instances are equal if they have the same state (file), but checking asString won't get real state of the instance, since file content may change, and equals may return false-positive results

@g4s8 raises a good point, and is highlighting another problem of envelopes we haven't tackled yet: when using envelopes for some of your implementations, you can't rely on reflection and casting.

Because if you have MyFileText extends TextEnvelope and delegates to FileText, I suppose you would expect that new FileText(...).equals(new MyFileText(...)) would work if they delegate to the same file.

But they won't, because FileText doesn't know about TextEnvelope and will just conclude MyFileText is not of the right class.

I don't think there is a solution for that, or do you have something in mind (with current or my proposed design, cf llorllale/cactoos-matchers#135 (comment), whatever) @g4s8 ?

[g4s8]

@victornoel @llorllale if I really need to compare two texts by asString result, I'd create new class to do that:

final class EqText implements Text {
  private final Text txt;

  @Override
  public String asString() {
    return this.txt.asString();
  }

  @Override
  public boolean equals(Object other) {
    return (other instanceof EqText) 
      && Objects.equals(this.asString(), EqText.class.cast(other).asString());
}

[fanifieiev]

@g4s8 @paulodamaso @victornoel
I think one of the solutions to make equality work properly is to introduce canEqual method as described here: http://marcus-christie.blogspot.com/2014/03/scala-object-equality-and-canequal.html
By the way, lombok uses the same approach and is used in Takes framework

[victornoel]

@g4s8 I'm going to close this, there is some discussion on that matter in #1241 but concerning TextEnvelope.equals (moved to TextOf, the envelope just being an envelope, so there is no more inherited behaviour from the envelope), I have this conclusion: #1458 (comment)

[0crat]

Job gh:yegor256/cactoos#1156 is not assigned, can't get performer