-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Prevent Chain
instances being backed by mutable or lazy Seq
#4169
Conversation
Just to clarify: what are the race conditions you mentioned? |
case s: ImSeq[A] => fromImmutableSeq(s) // pay O(1) not O(N) cost | ||
case s: MutSeq[A] => fromMutableSeq(s) | ||
case notSeq => | ||
fromImmutableSeq(notSeq.toVector) // toSeq could return a Stream, creating potential race conditions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's add a more complete comment here. I don't know the race conditions you are referring to, and future comment readers might also be confused.
I think toVector
is fine even if there is no race conditions because Vector is very memory efficient (due to internally using Array).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking this!
Nice safety fix.
s match { | ||
case mut: MutSeq[A] => fromMutableSeq(mut) | ||
case imm: ImSeq[A] => fromImmutableSeq(imm) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not an objection but rather just a suggestion about the style.
I personally don't like such renames on imports, because they tend to introduce new not-well known names. Moreover, Scala Docs suggests a convention about it:
https://docs.scala-lang.org/overviews/collections/overview.html
I.e. you could import it in this way:
import scala.collection.immutable
import scala.collection.mutable
Then use it in the code:
s match {
case seq: mutable.Seq[A] => fromMutableSeq(mut)
case seq: immutable.Seq[A] => fromImmutableSeq(imm)
}
That looks way clearer and easier to read, doesn't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, I very much feel the same way about this.
private def fromImmutableSeq[A](s: ImSeq[A]): Chain[A] = { | ||
if (s.isEmpty) nil | ||
else if (s.lengthCompare(1) == 0) one(s.head) | ||
else Wrap(s) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This implementation is identical in both Scala 2.12 and 2.13+ versions. I wonder – does it make sense to make it shared to avoid unnecessary duplication?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eh, I don't think it's worth the mental indirection when reading the code. It's just a few lines, it's fine.
def fromSeq[A](s: Seq[A]): Chain[A] = | ||
s match { | ||
case mut: MutSeq[A] => fromMutableSeq(mut) | ||
case imm: ImSeq[A] => fromImmutableSeq(imm) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually, Seq
isn't sealed, so in principle there could be other subclasses.
Can we instead do:
s match {
case imm: ImSeq[A] => fromImmutableSeq(imm)
case _ =>
if (s.isEmpty) Empty
else if (s.lengthCompare(1) == 0) Singleton(s.head)
else Wrap(s.toVector)
}
@bplommer polite bump, seems just a few fixes and then this is mergeable :) thanks! |
Completely forgot about this! Will do soon :) |
This addresses two issues:
Chain.Wrap
to wrapimmutable.Seq
instead ofSeq
, which until 2.13 could be mutable.IterableOnce
, explicitly convert toVector
. Previous use of.iterator.toSeq
can return a lazy Seq, creating race conditions - in 2.12 it returnsStream
. In 2.13 it returnsList
, but this isn't guaranteed by the return type.I haven't attempted to prevent
Chain
from wrapping aLazyList
orStream
passed directly to the constructor.