-
-
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
restrict traverse_ and friends to require Unit #4352
Changes from 1 commit
a9a0879
85e3d15
9f60b5c
b5ddca0
d35c6ea
b96d971
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -47,7 +47,8 @@ private[syntax] trait FoldableSyntaxBinCompat1 { | |
} | ||
|
||
final class NestedFoldableOps[F[_], G[_], A](private val fga: F[G[A]]) extends AnyVal { | ||
def sequence_(implicit F: Foldable[F], G: Applicative[G]): G[Unit] = F.sequence_(fga) | ||
def sequence_(implicit F: Foldable[F], G: Applicative[G], ev: A <:< Unit): G[Unit] = | ||
F.sequence_(fga.asInstanceOf[F[G[Unit]]]) | ||
Comment on lines
+50
to
+51
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this one is also causing binary compatibility issues, and in this case I don't see how this function can still exist in its current form (i.e. without said evidence) -- I guess it would have to move or something and that would for sure break binary compatibility? not sure if there is a fix here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, we cannot change the signature of this method. What we can do is add a new ops class: final class NestedUnitFoldableOps[F[_], G[_]](private val fgu: F[G[Unit]]) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. so this one has to change then to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems so. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That still allows people to call There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
honestly, this is my feeling about this PR as a whole. It's obvious at the moment we don't even have existing test coverage to safely make this change. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if we can't change signatures, what about adding new methods with an appropriate signature and deprecating the old ones? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I recall I did something similar – i.e. was tuning a method with a very subtle change in its signature while preserving its name: #3997. Not exactly the same though, but there are some similarities apparent.
Personally, I don't think it is a good idea, especially because the old methods do not have such bugs that would hinder their usage. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Do you mean, there's no tests for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
||
/** | ||
* @see [[Foldable.foldK]]. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -44,13 +44,11 @@ trait NonEmptyTraverseTests[F[_]] extends TraverseTests[F] with ReducibleTests[F | |
ArbYB: Arbitrary[Y[B]], | ||
ArbYC: Arbitrary[Y[C]], | ||
ArbFB: Arbitrary[F[B]], | ||
ArbFM: Arbitrary[F[M]], | ||
ArbXM: Arbitrary[X[M]], | ||
ArbYM: Arbitrary[Y[M]], | ||
ArbFGA: Arbitrary[F[G[A]]], | ||
ArbGU: Arbitrary[G[Unit]], | ||
ArbFGU: Arbitrary[F[G[Unit]]], | ||
ArbFXM: Arbitrary[F[X[M]]], | ||
ArbGB: Arbitrary[G[B]], | ||
ArbGM: Arbitrary[G[M]], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These changes will also cause compatibility problems. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I assumed the correct answer here would be to leave the prior implicit params alone and just add mine at the end of arglist. the mima check is still complaining in 2.12 and I'm not sure how to resolve it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unfortunately we cannot change these signatures at all: no changes, no additions, no removals. It's not specific to Scala 2.12, that one probably just happened to fail first. See #4324 (comment) for a possible strategy. |
||
CogenA: Cogen[A], | ||
CogenB: Cogen[B], | ||
CogenC: Cogen[C], | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -35,8 +35,8 @@ trait ReducibleTests[F[_]] extends FoldableTests[F] { | |
def reducible[G[_]: Applicative, A: Arbitrary, B: Arbitrary](implicit | ||
ArbFA: Arbitrary[F[A]], | ||
ArbFB: Arbitrary[F[B]], | ||
ArbFGA: Arbitrary[F[G[A]]], | ||
ArbGB: Arbitrary[G[B]], | ||
ArbFGU: Arbitrary[F[G[Unit]]], | ||
ArbGU: Arbitrary[G[Unit]], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here. |
||
CogenA: Cogen[A], | ||
CogenB: Cogen[B], | ||
EqG: Eq[G[Unit]], | ||
|
@@ -58,8 +58,8 @@ trait ReducibleTests[F[_]] extends FoldableTests[F] { | |
forAll(laws.reduceRightConsistentWithReduceRightOption[A] _), | ||
"reduce consistent with reduceLeft" -> | ||
forAll(laws.reduceReduceLeftConsistent[B] _), | ||
"nonEmptyTraverse_ consistent with traverse_" -> forAll(laws.traverseConsistent[G, A, B] _), | ||
"nonEmptySequence_ consistent with sequence_" -> forAll(laws.sequenceConsistent[G, A] _), | ||
"nonEmptyTraverse_ consistent with traverse_" -> forAll(laws.traverseConsistent[G, A] _), | ||
"nonEmptySequence_ consistent with sequence_" -> forAll(laws.sequenceConsistent[G] _), | ||
"size consistent with reduceMap" -> forAll(laws.sizeConsistent[A] _) | ||
) | ||
} | ||
|
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.
I'm afraid this change is not compatible. Although we can change the signature, we need to keep the current implementation. See the test I added in 41439fc.
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.
Or possibly, this implementation could work:
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.
yikes -- neither
G.void
nor even the original implementation seems to make this go away, at least on my local machine. Does that mean the change can't be made with binary compatibility at all?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.
Damn, really? I thought it should be possible, if done carefully. But maybe not.
Also, maybe it's not worth the risk since clearly this is not easy to reason about.
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.
pushed my branch with (credited) breaking test, and attempt to fix it by restoring original implementations. It didn't work. Wondering if:
a) there is some path to binary compatibility that I don't understand;
b) is it better to add new functions (
traverseEach
,sequenceEach
, maybetraverseEachM
for monads) and possibly deprecate the unrestricted variants (traverse_
andsequence_
)?happy to proceed however y'all think best. many thanks @armanbilge for spotting this issue
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.
btw: what is the motivation for making this change?
foldMapA is relatively rarely used, I think. While we have worked out many stack overflow issues around traverse and sequence. Touching these implementations has an excellent chance of introducing stack overflow errors for users I think.
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.
the motivation was that the implementation is identical so why not reduce repetition; but since it broke binary compatibility (see Arman's added MIMA test, now added to the PR) I reverted it.