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

c.i.Set#toSet is unsound, warns against overriding with sound version #8346

Closed
scabug opened this issue Feb 26, 2014 · 10 comments
Closed

c.i.Set#toSet is unsound, warns against overriding with sound version #8346

scabug opened this issue Feb 26, 2014 · 10 comments

Comments

@scabug
Copy link

scabug commented Feb 26, 2014

3cc99d7 deprecated overriding the casting Set#toSet introduced in #3953. This cast is demonstrably unsafe, so at least overriding should not be deprecated, unless the types of operations like + change. I imagine the cast should be overridden in the collections library where it is unsound, or moved to leaves where it's known to be sound.

scala> collection.immutable.SortedSet(1,2).toSet[Any] + "hi"
java.lang.ClassCastException: java.lang.String cannot be cast to java.lang.Integer
  at scala.runtime.BoxesRunTime.unboxToInt(BoxesRunTime.java:105)
  at scala.math.Ordering$Int$.compare(Ordering.scala:256)
  at scala.collection.immutable.RedBlackTree$.upd(RedBlackTree.scala:154)
  at scala.collection.immutable.RedBlackTree$.update(RedBlackTree.scala:66)
  at scala.collection.immutable.TreeSet.$plus(TreeSet.scala:117)
  at scala.collection.immutable.TreeSet.$plus(TreeSet.scala:53)
  ... 40 elided

The above CCE occurs in 2.10.3, 2.10.4-RC3, and 2.11.0-M8. The deprecation warning occurs in 2.11.0-M8.

@scabug
Copy link
Author

scabug commented Feb 26, 2014

Imported From: https://issues.scala-lang.org/browse/SI-8346?orig=1
Reporter: Stephen Compall (s11001001)
Affected Versions: 2.10.3, 2.10.4-RC3, 2.11.0-M8
See #3953

@scabug
Copy link
Author

scabug commented Mar 3, 2014

@Ichoran said:
Ugh. The problem is not with the deprecation of overriding, it's with the covariance-friendly form of toSet that does not play well with typeclass-style restrictions. I seriously doubt there is a solution here that is less drastic than "rewrite the whole collections library". I'll look into it more, though.

@scabug
Copy link
Author

scabug commented Mar 4, 2014

Stephen Compall (s11001001) said:
Set#toSet could just do what Seq#toSet does; the former is no more evil than the other. And HashSet#toSet, specifically, could be where the casting version goes, and likewise to any other Set subclasses that are representationally covariant. Regardless, the override deprecation goes away in this scheme.

@scabug
Copy link
Author

scabug commented Mar 4, 2014

@Ichoran said:
It's pretty obvious that Seq#toSet is going to recreate the entire collection. It's much less obvious that Set#toSet will. That SortedSet.toSet is broken is evidence of that. I think the solution is to add a protected abstract method that you override to indicate whether it can be covariant or not. Not pretty, but less bad I guess than the alternatives.

@scabug
Copy link
Author

scabug commented May 24, 2014

Stephen Compall (s11001001) said:
scala/scala#3791

@scabug
Copy link
Author

scabug commented May 29, 2014

@Ichoran said:
Fixed by scala/scala#3791

@scabug
Copy link
Author

scabug commented Aug 18, 2015

@adriaanm said:
Reopened by scala/scala#4693 since Stephen will not sign our CLA.

@scabug
Copy link
Author

scabug commented Aug 18, 2015

@adriaanm said:
Rex, please forget everything you know about the old PR and create a new one at some point...

@scabug
Copy link
Author

scabug commented Aug 18, 2015

Stephen Compall (s11001001) said:
Or implement based on [#comment-68560], having forgotten what the patch looks like.

@scabug
Copy link
Author

scabug commented Aug 22, 2015

@Ichoran said:
scala/scala#4703

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

No branches or pull requests

2 participants