-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
restrict traverse_ and friends to require Unit #4352
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
Changes from all commits
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 |
|---|---|---|
|
|
@@ -137,7 +137,7 @@ trait VectorInstances extends cats.kernel.instances.VectorInstances { | |
| /** | ||
| * This avoids making a very deep stack by building a tree instead | ||
| */ | ||
| override def traverse_[G[_], A, B](fa: Vector[A])(f: A => G[B])(implicit G: Applicative[G]): G[Unit] = { | ||
| override def traverse_[G[_], A](fa: Vector[A])(f: A => G[Unit])(implicit G: Applicative[G]): G[Unit] = { | ||
| // the cost of this is O(size) | ||
| // c(n) = 1 + 2 * c(n/2) | ||
| // invariant: size >= 1 | ||
|
|
@@ -162,8 +162,8 @@ trait VectorInstances extends cats.kernel.instances.VectorInstances { | |
| // traversing fa, which we will do fully | ||
| // in all cases. | ||
| Eval.always { | ||
| val gb = f(a) | ||
| G.void(gb) | ||
| val fb = f(a) | ||
| G.void(fb) | ||
|
Contributor
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 comment, this |
||
| } | ||
| } | ||
|
|
||
|
|
||
| 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
Author
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.
Member
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]])
Author
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
Member
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.
Author
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
Member
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.
Author
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?
Contributor
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.
Contributor
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
Member
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]]. | ||
|
|
||
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.
we don't need this now.
fb: G[Unit]so the void is a no-op.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.
We absolutely do, or the change is not compatible. See #4352 (comment).
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 don't follow you. My claim is that
f(a): G[Unit]already. WhileG.void(f(a))is stillG[Unit], it isn't needed and is likely to be wasteful.In fact, unconditionally calling
voidinternal to a method is a code smell that should have alerted us the type was poor to begin with.That said, there could be ergonomic reasons to not have the users required to call this void.
Uh oh!
There was an error while loading. Please reload this page.
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.
Except, it's not really :) sure, that's what the current signature claims, and a signature of
G[Unit]is binary-compatible with a signature ofG[A]for an unbounded type parameterA(due to type erasure).But Cats still has to retain compatibility with calling code that was passing e.g. a
G[String]there. Otherwise we will get class cast exceptions, exactly as demonstrated in #4352 (comment).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.
@johnynek just to spell it out: once the MIMA test was added, returning
f(a)without void caused it to break.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.
Sorry to be pedantic :) actually, that's not a MiMa test. MiMa compares the binary-compatibility of signatures. MiMa can only detect linking errors but not other errors such as class casting exceptions. In this case there is no linking error, because due to type erasure the method signature is the same.
What I added was a runtime-compatibility test. Cats has an internal test project, that is compiled against Cats 2.0.0 but run against the latest development version of Cats. It shows that code compiled with an old version of Cats, that is not passing
G[Unit]to these methods will encounter a class cast exception if we rewrite the code here to assume we are indeed getting aG[Unit].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.
thank you for the explanation!
I'd love to understand the team's thoughts and @armanbilge 's in particular on:
a) what's your evaluation of the problem? to be precise: is there value, if we can do it in a binary-compatible way, in alerting users to the code-smell of calling
traverse_and friends with non-Unit values?b) what do you think of the approach to that problem of adding aliases with the correct signature and deprecating the original methods?
Uh oh!
There was an error while loading. Please reload this page.
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 think there is value. See this issue I opened proposing a lint for
flatTap, which currently suffers from a similar problem..flatTapwith non-Unitresult typelevel-scalafix#43The linting route offers a safe way to opt-in to this change.
However, there are different perspectives. Quoting @SystemFw from the Discord discussion linked in that issue.
Uh oh!
There was an error while loading. Please reload this page.
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.
In fact, I can make the same exact point here. The whole point of
traverse_is the avoid the boilerplate of callingvoid, so if I have to dolist.traverse_(a => f(a).void), why don't I just dolist.traverse(f).void? (And if the answer is, to avoid allocating the list, if you really care about that there is stillfoldMapA)That being said, I don't feel strongly about this change :)