Display warning on `equals` comparing non-references by eed3si9n · Pull Request #8120 · scala/scala

eed3si9n · 2019-06-03T22:08:46Z

Ref scala/bug#11551
Ref https://twitter.com/not_xuwei_k/status/1135374786593468416

Scala 2.13.1 or Dotty allows you to write the following expression:

1 equals 1L

or generically as:

scala> def check[A](a1: A, a2: A): Boolean = a1.equals(a2)
def check[A](a1: A, a2: A): Boolean

problem

This looks seemingly innocuous but it's actually not "safe" because of cooperative equality.

scala> 1 equals 1L
val res0: Boolean = false

scala> check(1L, 1)
val res1: Boolean = false

what this changes

This PR extends "sensible check" to equals, to warn against using it as opposed to ==:

[info] Running (fork) scala.tools.nsc.MainGenericRunner -usejavacp
Welcome to Scala 2.13.0-pre-db58db9 (OpenJDK 64-Bit Server VM, Java 1.8.0_232).
Type in expressions for evaluation. Or try :help.

scala> def check[A](a1: A, a2: A): Boolean = a1.equals(a2)
                                                      ^
       warning: comparing values of types A and A using `equals` is unsafe due to cooperative equality; use `==` instead
check: [A](a1: A, a2: A)Boolean

/cc @xuwei-k

xuwei-k · 2019-06-04T04:01:42Z

Is this within expectations? 🤔

--- a/test/files/neg/checksensible.check
+++ b/test/files/neg/checksensible.check
@@ -112,6 +112,9 @@ checksensible.scala:111: warning: comparing values of types AnyVal and Int using
 checksensible.scala:114: warning: comparing values of types A and Int using `equals` is unsafe due to cooperative equality; use `==` instead
   def foo[A](a: A) = a.equals(1)
                              ^
+checksensible.scala:118: warning: comparing values of types Int and Int using `equals` is unsafe due to cooperative equality; use `==` instead
+  1 equals 1
+    ^
 error: No warnings can be incurred under -Werror.
-38 warnings found
+39 warnings found
 one error found
diff --git a/test/files/neg/checksensible.scala b/test/files/neg/checksensible.scala
index 7542509e55..37dfc0f2fc 100644
--- a/test/files/neg/checksensible.scala
+++ b/test/files/neg/checksensible.scala
@@ -114,4 +114,6 @@ class AnyEqualsTest {
   def foo[A](a: A) = a.equals(1)
   // ok
   def bar[A <: AnyRef](a: A) = a.equals(1)
+
+  1 equals 1
 }

sjrd · 2019-06-04T04:55:18Z

I'm not sure I like this. There are a lot of places where calling equals is the right thing to do, for performance and/or semantics.

eed3si9n · 2019-06-04T05:00:13Z

Is this within expectations?

Nope. I need to filter out receiver == actual more.

eed3si9n · 2019-06-04T05:26:16Z

@sjrd

I'm not sure I like this. There are a lot of places where calling equals is the right thing to do, for performance and/or semantics.

Could you share some examples? Something like the following?

def foo[A](a: A) = a.equals(1)

In a normal application either written in Scala or Scala.JS, is the benefit worth the confusion of == not being the same as equals? One direction might be to remove cooperative equality (ref https://contributors.scala-lang.org/t/can-we-get-rid-of-cooperative-equality/1131), but given that won't happen for a while, the logical option would be to discourage the use of Any#equals.

dwijnand · 2019-06-04T06:36:29Z

If we're keeping cooperative equality maybe this warning should be opt-out-able, including globally opt-out-able for codebases where it's throughout.

sjrd · 2019-06-04T07:53:39Z

It's mostly necessary to implement Java-like contracts. Here are the changes I need to apply to the Scala.js codebase to accommodate this PR:
https://gist.github.com/sjrd/bd11d587635c7ce85230a46f9657ef77
That does not include a bunch of warnings in my test suite as well; just the projects that have -Xfatal-warnings on.

dwijnand · 2019-06-04T08:04:42Z

This one is particularly representative:

       if (self.asInstanceOf[AnyRef] eq null) that.asInstanceOf[AnyRef] eq null
-      else self.equals(that)
+      else self.asInstanceOf[AnyRef].equals(that)

Perhaps we should have a method that looks less like a cast and more like "ok, convert this to a reference value, boxing if necessary". Say it's called toAnyRef (🖌), then maybe the warning and error messages could say: use "a == b" or "a.toAnyRef.equals(b)" (and for eq: use a.toAnyRef.eq(b)).

lrytz · 2019-07-22T20:13:00Z

Can we move this one forward? It seems very rare that one would actually have to call Any.equals, maybe we can enumerate these situations?

eed3si9n · 2019-08-13T15:54:54Z

Are there something I can do to improve this PR, or is warning a wrong-headed approach?

lrytz · 2020-01-31T15:04:24Z

@eed3si9n should we close this, as there's no obvious path forward?

eed3si9n · 2020-01-31T15:55:35Z

@lrytz I find that symbolic operator == supposedly documented as:

  final def ==(that: Any): Boolean = this equals that

and equals being different to be unintuitive aspect of Scala. It did cause scala/bug#11551 after all, so even for Scala maintainers, it's a pitfall they could sometimes fall into. If they are not exchangeable, then adding a warning would be improvement.

If we're saying it's a shorthand to force boxing, that's even more surprising, and self.asInstanceOf[AnyRef].eq(that) would make the intent more clear.

lrytz · 2020-02-03T13:40:00Z

I'm supportive of the warning, but it's a complicated topic, so I'd really like to get more input, for example from @sjrd (again), @Ichoran, @NthPortal, @som-snytt, @retronym.

Random idea: would it make sense to have Any.equals protected (at some point, @deprecated first), and only have Object/AnyRef.equals public?

NthPortal · 2020-02-04T23:47:36Z

I find the referenced bug particularly compelling. I think having the warning helps make a sharp edge a little bit safer.

I think it is a reasonable warning, unless someone brings up a use case where the warned-about behaviour is correct and Scala-like. Sébastien is in a fairly unique position of having to conform to Java's behaviour, which is not cooperatively equal; however, if one were to design similar APIs (e.g. like that of Optional) without those constraints, using equals would be a mistake.

eed3si9n · 2020-02-05T15:18:50Z

@lrytz

Random idea: would it make sense to have Any.equals protected (at some point, @deprecated first), and only have Object/AnyRef.equals public?

If we keep the collaborative equality, then I think that makes sense. I'd rather see == become a symbolic alias of equals method for Scala 3.x under strictEquality.

xuwei-k · 2020-02-14T05:40:39Z

FYI scala/scala3#8314

eed3si9n · 2020-02-20T17:18:03Z

Rebased against 2.13.x.

dwijnand · 2020-10-05T15:49:34Z

@sjrd given we have configurable warnings now, are you more warm to this idea? Personally I like the look of it, and I agree that if a standard library maintainer can shoot their own foot, this warning should exist.

som-snytt · 2020-10-05T16:05:09Z

I'm not sure -Wconf has a cat for this. I just saw a PR with a nowarn(msg="something") to suppress it. This is so fundamental, it should be easier.

dwijnand · 2020-10-14T15:27:22Z

I'm inclined to merge this, if you could give it a rebase pls, Eugene.

Ref scala/bug#11551 Ref https://twitter.com/not_xuwei_k/status/1135374786593468416 This extends sensible check to `equals`, to warn against using it as opposed to `==`.

eed3si9n · 2020-10-14T16:12:16Z

Rebased against 2.13.x.

sjrd · 2020-10-14T19:04:40Z

@sjrd given we have configurable warnings now, are you more warm to this idea?

Would I be able to blanket-ignore all such warnings in the entire codebase, without adding any @nowarn in the code (because my code still compiles with 2.11 and 2.12), and without ignoring any other warning?

NthPortal · 2020-10-14T20:34:11Z

@sjrd yes. at the very least by doing -Wconf:msg=comparing values of types.*is unsafe due to cooperative equality:s, or, if it eventually gets its own category, with -Wconf:cat=lint-equals-non-ref:s or something

Edit: cat=other-non-cooperative-equals

lrytz · 2020-10-15T08:21:05Z

src/compiler/scala/tools/nsc/typechecker/RefChecks.scala

+      val actual   = underlyingClass(other.tpe)
+      def typesString = normalizeAll(qual.tpe.widen)+" and "+normalizeAll(other.tpe.widen)
+      def nonSensiblyEquals() = {
+        reporter.warning(pos, s"comparing values of types $typesString using `${name.decode}` is unsafe due to cooperative equality; use `==` instead")


This should call refchecksWarning, going directly do reporter.warning bypasses the Wconf infrastructure. Any takers?

➜ sandbox git:(2.13.x) qs '-Wconf:msg=statement:e,msg=equality:e' Welcome to Scala 2.13.4-20201009-113655-8fbebaa (Java HotSpot(TM) 64-Bit GraalVM EE 19.2.1, Java 1.8.0_231). Type in expressions for evaluation. Or try :help. scala> def f = { 1; 2 } ^ error: a pure expression does nothing in statement position; multiline expressions might require enclosing parentheses scala> def g = 1L equals 1 ^ warning: comparing values of types Long and Int using `equals` is unsafe due to cooperative equality; use `==` instead def g: Boolean

I just noticed this myself, trying to fix up the warning for #9235

@lrytz if all it is is changing that one line, I'll take a stab

@lrytz what's that qs command/alias?

Likely build/quick/bin/scala or perhaps $(git root 2>/dev/null)/build/quick/bin/scala

$(git root 2>/dev/null)/build/quick/bin/scala

If only I was that smart. Though git root doesn't seem to be a built-in. Adopted, thank you.

SethTisue · 2020-11-18T00:51:39Z

PR adjusting the wording a bit: #9320

nchammas · 2024-02-27T18:57:05Z

sjrd yes. at the very least by doing -Wconf:msg=comparing values of types.*is unsafe due to cooperative equality:s, or, if it eventually gets its own category, with -Wconf:cat=lint-equals-non-ref:s or something

Edit: cat=other-non-cooperative-equals

Sorry to dig up an old PR, but where can I find documentation for the other-non-cooperative-equals category?

I'm trying to find the earliest version of Scala that supports it.

SethTisue · 2024-02-27T19:08:00Z

@nchammas re: version, I think you would need to go digging (either in the source code, or just by trying the different versions) to determine that. -Wconf itself landed in Scala 2.13.2, then was backported to Scala 2.12.13 as well. So you don't need to look at any versions older than that.

SethTisue · 2024-02-27T19:10:13Z

and we don't have fine-grained documentation covering individual warning categories. the usual best available source of information, besides the source code, is the PR description of the PR that added that warning in the first place. (usually such PRs are linked from the Scala release notes.)

### What changes were proposed in this pull request? This is a backport of fcc5dbc / #45036 with a tweak so that it works on Scala 2.12. ### Why are the changes needed? This is a correctness bug fix. The original fix against `master` suppresses a warning category that doesn't exist on certain versions of Scala 2.13 and 2.12, and the exact versions are [not documented anywhere][1]. To be safe, this backport simply suppresses all warnings instead of just `other-non-cooperative-equals`. It would be interesting to see if `-Wconf:nowarn` complains, since the warning about non-cooperative equals itself is also not thrown on all versions of Scala, but I don't think that's a priority. [1]: scala/scala#8120 (comment) ### Does this PR introduce _any_ user-facing change? Yes. ### How was this patch tested? CI + added tests. ### Was this patch authored or co-authored using generative AI tooling? No. Closes #45296 from nchammas/SPARK-45599-OpenHashSet. Authored-by: Nicholas Chammas <nicholas.chammas@gmail.com> Signed-off-by: Dongjoon Hyun <dhyun@apple.com>

scala-jenkins added this to the 2.13.1 milestone Jun 3, 2019

eed3si9n force-pushed the wip/deprecate-anyval-equals branch from 998619a to b1cf03a Compare June 4, 2019 02:47

eed3si9n force-pushed the wip/deprecate-anyval-equals branch from b1cf03a to db58db9 Compare June 4, 2019 05:08

szeiger modified the milestones: 2.13.1, 2.13.2 Aug 30, 2019

lrytz self-assigned this Feb 3, 2020

SethTisue modified the milestones: 2.13.2, 2.13.3 Feb 6, 2020

eed3si9n force-pushed the wip/deprecate-anyval-equals branch from db58db9 to 1d6121b Compare February 20, 2020 15:47

SethTisue modified the milestones: 2.13.3, 2.13.4 May 12, 2020

Display warning on equals comparing non-references

ec80eb4

Ref scala/bug#11551 Ref https://twitter.com/not_xuwei_k/status/1135374786593468416 This extends sensible check to `equals`, to warn against using it as opposed to `==`.

eed3si9n force-pushed the wip/deprecate-anyval-equals branch from 1d6121b to ec80eb4 Compare October 14, 2020 16:11

dwijnand merged commit 4e92035 into scala:2.13.x Oct 14, 2020

eed3si9n deleted the wip/deprecate-anyval-equals branch October 15, 2020 01:44

lrytz reviewed Oct 15, 2020

View reviewed changes

This was referenced Oct 15, 2020

Clean up warnings in library module #9235

Merged

Allow -Wconf configuration of non-cooperative equals warning #9247

Merged

dwijnand added release-notes worth highlighting in next release notes and removed release-notes worth highlighting in next release notes labels Nov 10, 2020

SethTisue changed the title ~~Display warning on equals comparing non-references~~ Display warning on equals comparing non-references Nov 18, 2020

SethTisue mentioned this pull request Nov 18, 2020

reword warning about equals and cooperative equality #9320

Merged

lrytz mentioned this pull request Jun 26, 2023

[backport] Warn when equals compares non-references #10446

Merged

nchammas mentioned this pull request Feb 27, 2024

[SPARK-45599][CORE] Use object equality in OpenHashSet apache/spark#45036

Closed

nchammas mentioned this pull request Feb 27, 2024

[SPARK-45599][CORE][3.5] Use object equality in OpenHashSet apache/spark#45296

Closed

Conversation

eed3si9n commented Jun 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

problem

what this changes

Uh oh!

xuwei-k commented Jun 4, 2019

Uh oh!

sjrd commented Jun 4, 2019

Uh oh!

eed3si9n commented Jun 4, 2019

Uh oh!

eed3si9n commented Jun 4, 2019

Uh oh!

dwijnand commented Jun 4, 2019

Uh oh!

sjrd commented Jun 4, 2019

Uh oh!

dwijnand commented Jun 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lrytz commented Jul 22, 2019

Uh oh!

eed3si9n commented Aug 13, 2019

Uh oh!

lrytz commented Jan 31, 2020

Uh oh!

eed3si9n commented Jan 31, 2020

Uh oh!

lrytz commented Feb 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NthPortal commented Feb 4, 2020

Uh oh!

eed3si9n commented Feb 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xuwei-k commented Feb 14, 2020

Uh oh!

eed3si9n commented Feb 20, 2020

Uh oh!

dwijnand commented Oct 5, 2020

Uh oh!

som-snytt commented Oct 5, 2020

Uh oh!

dwijnand commented Oct 14, 2020

Uh oh!

eed3si9n commented Oct 14, 2020

Uh oh!

sjrd commented Oct 14, 2020

Uh oh!

NthPortal commented Oct 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lrytz Oct 15, 2020

Choose a reason for hiding this comment

Uh oh!

NthPortal Oct 15, 2020

Choose a reason for hiding this comment

Uh oh!

NthPortal Oct 15, 2020

Choose a reason for hiding this comment

Uh oh!

NthPortal Oct 16, 2020

Choose a reason for hiding this comment

Uh oh!

dwijnand Oct 16, 2020

Choose a reason for hiding this comment

Uh oh!

lrytz Oct 16, 2020

Choose a reason for hiding this comment

Uh oh!

SethTisue commented Nov 18, 2020

Uh oh!

nchammas commented Feb 27, 2024

Uh oh!

SethTisue commented Feb 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SethTisue commented Feb 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

eed3si9n commented Jun 3, 2019 •

edited

Loading

dwijnand commented Jun 4, 2019 •

edited

Loading

lrytz commented Feb 3, 2020 •

edited

Loading

eed3si9n commented Feb 5, 2020 •

edited

Loading

NthPortal commented Oct 14, 2020 •

edited

Loading

SethTisue commented Feb 27, 2024 •

edited

Loading

SethTisue commented Feb 27, 2024 •

edited

Loading