SI-8346 Re-established soundness of toSet (element type widening) #4703
Conversation
|
I anticipate binary compatibility issues with this patch, since I wrote it without any reference to how it was done before, and I may have chosen to make alterations in different spots. Will modify ones MIMA tells me what's wrong. (If it tells me what's wrong.) @SethTisue - why didn't the reversion of the previous commit complain? It couldn't have been binary compatible, could it? (Or did it complain, and did we ignore it?) |
| @@ -53,6 +53,7 @@ self => | |||
| val map = self.rangeImpl(from, until) | |||
| new map.DefaultKeySortedSet | |||
| } | |||
| override def toSet[C >: A] = Set.empty[C] ++ this | |||
|
The change is binary compatible because all the overrides introduced have the exact same signature in bytecode as the method that is being overridden, and all the overrides are in classes (none in a trait). |
|
@lrytz - Ah, right. Hm, does that mean that our bincompat story will be a lot better once traits are mostly-to-entirely implemented with default methods? That would be really nice. |
toSet needs to rebuild some child classes, but not others, as toSet is allowed to widen element types (which the invariant Set normally cannot do), and some sets rely upon their invariance. Thus, sets that rely upon their invariance now rebuild themselves into a generic set upon toSet, while those that do not just sit there. Note: there was a similar patch previously that fixed the same problem, but this is a reimplementation to circumvent license issues. Note: the newBuilder method was benchmarked as (surprisingly!) the most efficient way to create small sets, so it is used where sets may need to be rebuild.
|
@lrytz - Fixed the things you commented on, and changed the default implementation to use Set.newBuilder which was, surprisingly to me, almost always the most efficient in microbenchmarks. |
|
LGTM, thanks! |
SI-8346 Re-established soundness of toSet (element type widening)
toSet needs to rebuild some child classes, but not others, as toSet is
allowed to widen element types (which the invariant Set normally cannot do),
and some sets rely upon their invariance. Thus, sets that rely upon their
invariance now rebuild themselves into a generic set upon toSet, while those
that do not just sit there.
Note: there was a similar patch previously that fixed the same problem, but
this is a reimplementation to circumvent license issues.