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

candidate unsafe == wart #46

Closed
wants to merge 6 commits into from
Closed

Conversation

tpolecat
Copy link
Collaborator

This is in response to #45.

This wart disables == for operands of non-conforming types. It disables, for instance, comparison between List and Set, but allows comparison between Option[Int] and None. It would be nice, if the types are not equal, to require the supertype to be abstract; however I have not been able to figure out how to determine this (the TypeSymbol always claims to be non-abstract).

Note that implementing this wart for equals is, oddly, a different situation. In the expression (1 max 2) == List(3, 4).map(_ + 1) the RHS has type List[Int], but for equals the RHS is inferred as Any ? (which of course conforms with the LHS). Go figure.

Anyway I thought I would throw this out as a starting point.

def apply(u: WartUniverse): u.Traverser = {
import u.universe._

val EqEqName: TermName = "$eq$eq"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you use this?

import reflect.NameTransformer
val EqEqName: TermName = NameTransformer.encode("==")

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh cool, I was wondering where that was. Will do.

@paulp
Copy link

paulp commented Feb 2, 2014

@tpolecat "the TypeSymbol always claims to be non-abstract"

At a wild guess, you are asking "isAbstract" when what you need to ask is "isDeferred".

@paulp
Copy link

paulp commented Feb 2, 2014

Also, the RHS is inferred as Any for equals because the expected type of the argument is Any. Int.== is overloaded with a bunch of non-Any types, leading it to eventually infer something more useful.

@tpolecat
Copy link
Collaborator Author

tpolecat commented Feb 2, 2014

@paulp aha isDeferred. How foolish of me. The Any thing makes sense I guess ... any way to know the underlying type before it gets smashed into Any, or is that type ever even computed?

@paulp
Copy link

paulp commented Feb 3, 2014

Never computed. It would be very wasteful to do so. The die has been cast once you are calling the Any-accepting method.

@tpolecat
Copy link
Collaborator Author

tpolecat commented Feb 3, 2014

@paulp any idea why the RHS of == has a reasonable inferred type? Artifact of precedence rules?

@paulp
Copy link

paulp commented Feb 3, 2014

It's the overloading of ==, which causes a whole different code path to be taken. You can see the same behavior in equals if you overload it. Add a "def equals(x: Bippy): Boolean = true" and it will infer the same type for both.

val result = WartTestTraverser(Equal) {
1 == 1.0 // can't do anything about widening here
1 equals 1.0 // but we can catch it here
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The numeric widening thing is nutty. Here equals triggers the wart and == doesn't.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's because it isn't being widened. It is calling the overload which accepts a Double.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I'll update the test name.

@tpolecat
Copy link
Collaborator Author

tpolecat commented Feb 3, 2014

So, to summarize, I'm still puzzled but equals triggers the wart almost as often as == so I went ahead and added it.

@tpolecat
Copy link
Collaborator Author

tpolecat commented Feb 3, 2014

I have narrowed my confusion to this case, which does not involve overloading.

Vector(1,2,3) == List(1,2,3).map(_ + 1)     // This triggers the wart (RHS is List[Int])
Vector(1,2,3) equals List(1,2,3).map(_ + 1) // This does not (RHS is Any)

@paulp
Copy link

paulp commented Feb 3, 2014

Believe it or not that does involve overloading, albeit probably not intentionally. The == defined in AnyRef overloads the == defined in Any rather than overriding it, because the parameter type is an Any in Any and an AnyRef in AnyRef.

The method equals also exists in both Any and AnyRef, but the one in AnyRef overrides the one in Any.

I've never noticed this before; it should be considered a bug, especially since in normal code you can't overload those signatures since the methods would have the same type after erasure. Should be considered a bug - don't know if it will be.

Oh, and keep in mind that it's the overloaded/buggy version which infers an accurate type, thereby avoiding recognition through your proxy indicator of Any-inference. You realize the 'Any' which is being inferred is the second type argument to map, right? (Which is itself another bug: see https://issues.scala-lang.org/browse/SI-6155 for some discussion.)

@paulp
Copy link

paulp commented Feb 3, 2014

See https://issues.scala-lang.org/browse/SI-8219 for yet more.

@retronym
Copy link
Collaborator

retronym commented Feb 3, 2014

Analysis of the accidental overloading is included in: scala/scala#3460

Why does overloading actually influence this?

Arguments to methods that are still overloaded after an initial round of filtering by arity* are typechecked without an expected type. The argument types are then used to further filter the candidate overloaded alternatives. Finally, static overload resolution picks the winner.

  • that's a slight simplification

A difference in type argument inference between List(1).map(_ + 1) and List(1).map(_ + 1): Any is not a bug a priori; the expected type is an input to inference.

@paulp
Copy link

paulp commented Feb 3, 2014

You can choose some word other than "bug", or you can pin it on the architecture of the collections and/or the tragedy of universal equals or any number of places, but in the end, it's just broken.

@tpolecat
Copy link
Collaborator Author

tpolecat commented Feb 3, 2014

Guh. So if I'm reading this correctly it sounds like this may be the best I can do, and in fact it's only due to a bug that it works as well as it does.

@paulp
Copy link

paulp commented Feb 3, 2014

You got it.

@ClaireNeveu
Copy link
Collaborator

Closing this since we now have the Equals wart.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants