Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

SI-6455 no longer rewrite .withFilter to .filter #3566

Merged
merged 2 commits into from

3 participants

@adriaanm
Owner

This has been deprecated for two major releases now,
and during that time caused plenty of harm (see also SI-7239).

Time to retire, rewrite.

What say ye, @gkossakowski, @Ichoran & @retronym?

@retronym
Owner

The std lib doesn't even fully comply iirc. Was it Either? Maybe scalacheck too?

@adriaanm
Owner

Urgh. I admit this is a bit of an eager submission. Deprecation fatigue has set in.

@adriaanm
Owner

I'll back off for RC1 if we're indeed this badly broken. That'll teach us ignoring -deprecation.

@adriaanm adriaanm added the tested label
@adriaanm
Owner

Let's see if I can get https://jenkins-dbuild.typesafe.com:8499/job/Community-2.11.x-manual/ to inspire confidence on this one.

@adriaanm
Owner

The only class I found that defines filter but not withFilter is Try, but @jorgeortiz85 is on the case!

@adriaanm
Owner

I decided to follow the pattern implemented in Option. I manually inspected Scalacheck (and it's compiled by the IDE validation) -- it seems to support withFilter where filter is.

@adriaanm adriaanm removed the tested label
@adriaanm
Owner

https://jenkins-dbuild.typesafe.com:8499/job/Community-2.11.x-manual/1/console is building the head of this PR, according to the log:
"uri" : "git://github.com/scala/scala.git#bc2a1c1d4862bac5c47345b95d8ce486042fe083"

@adriaanm adriaanm added the tested label
@adriaanm
Owner

Dbuild failed again. I'll defer to 2.12. Should've done this sooner in the cycle.

@adriaanm adriaanm closed this
@adriaanm adriaanm reopened this
@adriaanm adriaanm removed the tested label
@adriaanm
Owner

I weakened it to make -Xfuture enforce the deprecation.

@adriaanm adriaanm added the tested label
@gkossakowski gkossakowski commented on the diff
src/library/scala/util/Try.scala
((5 lines not shown))
+ * if the predicate is not satisfied.
+ *
+ * Note: unlike filter, withFilter does not create a new Try.
+ * Instead, it restricts the domain of subsequent
+ * `map`, `flatMap`, `foreach`, and `withFilter` operations.
+ *
+ * As Try is a one-element collection, this may be a bit overkill,
+ * but it's consistent with withFilter on Option and the other collections.
+ *
+ * @param p the predicate used to test elements.
+ * @return an object of class `WithFilter`, which supports
+ * `map`, `flatMap`, `foreach`, and `withFilter` operations.
+ * All these operations apply to those elements of this Try
+ * which satisfy the predicate `p`.
+ */
+ @inline final def withFilter(p: T => Boolean): WithFilter = new WithFilter(p)
@gkossakowski Owner

What does @inline achieve here? We won't get rid of the passed closure but just inline the constructor call. I don't think it's worth it (JVM handles this much better than we do).

@adriaanm Owner

Could be, this was copy pasted from Option. Looks like I forgot to note that in the comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
adriaanm added some commits
@adriaanm adriaanm SI-6455 under -Xfuture, don't rewrite .withFilter to .filter
This has been deprecated for two major releases now,
but `filter`'s still preferred over `withFilter` in the wild.
62560b1
@adriaanm adriaanm SI-6455 util.Try supports withFilter 12dc4a2
@adriaanm
Owner

test added, comment for Try's withFilter test clarified

@adriaanm adriaanm added tested and removed tested labels
@gkossakowski

LGTM

The @inline is still there but it won't harm us too much.

@gkossakowski gkossakowski merged commit 1ea43ff into scala:master
@adriaanm adriaanm deleted the adriaanm:t6455 branch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Feb 25, 2014
  1. @adriaanm

    SI-6455 under -Xfuture, don't rewrite .withFilter to .filter

    adriaanm authored
    This has been deprecated for two major releases now,
    but `filter`'s still preferred over `withFilter` in the wild.
  2. @adriaanm
This page is out of date. Refresh to see the latest.
View
2  src/compiler/scala/tools/nsc/typechecker/Typers.scala
@@ -4714,7 +4714,7 @@ trait Typers extends Adaptations with Tags with TypersTracking with PatternTyper
UnstableTreeError(qualTyped)
)
val tree1 = name match {
- case nme.withFilter => tryWithFilterAndFilter(tree, qualStableOrError)
+ case nme.withFilter if !settings.future => tryWithFilterAndFilter(tree, qualStableOrError)
case _ => typedSelect(tree, qualStableOrError, name)
}
def sym = tree1.symbol
View
29 src/library/scala/util/Try.scala
@@ -111,6 +111,35 @@ sealed abstract class Try[+T] {
*/
def filter(p: T => Boolean): Try[T]
+ /** Creates a non-strict filter, which eventually converts this to a `Failure`
+ * if the predicate is not satisfied.
+ *
+ * Note: unlike filter, withFilter does not create a new Try.
+ * Instead, it restricts the domain of subsequent
+ * `map`, `flatMap`, `foreach`, and `withFilter` operations.
+ *
+ * As Try is a one-element collection, this may be a bit overkill,
+ * but it's consistent with withFilter on Option and the other collections.
+ *
+ * @param p the predicate used to test elements.
+ * @return an object of class `WithFilter`, which supports
+ * `map`, `flatMap`, `foreach`, and `withFilter` operations.
+ * All these operations apply to those elements of this Try
+ * which satisfy the predicate `p`.
+ */
+ @inline final def withFilter(p: T => Boolean): WithFilter = new WithFilter(p)
@gkossakowski Owner

What does @inline achieve here? We won't get rid of the passed closure but just inline the constructor call. I don't think it's worth it (JVM handles this much better than we do).

@adriaanm Owner

Could be, this was copy pasted from Option. Looks like I forgot to note that in the comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+
+ /** We need a whole WithFilter class to honor the "doesn't create a new
+ * collection" contract even though it seems unlikely to matter much in a
+ * collection with max size 1.
+ */
+ class WithFilter(p: T => Boolean) {
+ def map[U](f: T => U): Try[U] = Try.this filter p map f
+ def flatMap[U](f: T => Try[U]): Try[U] = Try.this filter p flatMap f
+ def foreach[U](f: T => U): Unit = Try.this filter p foreach f
+ def withFilter(q: T => Boolean): WithFilter = new WithFilter(x => p(x) && q(x))
+ }
+
/**
* Applies the given function `f` if this is a `Failure`, otherwise returns this if this is a `Success`.
* This is like `flatMap` for the exception.
View
4 test/files/neg/t6455.check
@@ -0,0 +1,4 @@
+t6455.scala:5: error: value withFilter is not a member of object O
+ O.withFilter(f => true)
+ ^
+one error found
View
1  test/files/neg/t6455.flags
@@ -0,0 +1 @@
+-Xfuture
View
6 test/files/neg/t6455.scala
@@ -0,0 +1,6 @@
+object O { def filter(p: Int => Boolean): O.type = this }
+
+class Test {
+ // should not compile because we no longer rewrite withFilter => filter under -Xfuture
+ O.withFilter(f => true)
+}
View
2  test/files/scalacheck/quasiquotes/TypecheckedProps.scala
@@ -44,7 +44,7 @@ object TypecheckedProps extends QuasiquoteProperties("typechecked") {
val enums = fq"foo <- new Foo" :: fq"if foo != null" :: Nil
val body = q"foo"
val q"$_; for(..$enums1) yield $body1" = typecheck(q"""
- class Foo { def map(f: Any => Any) = this; def filter(cond: Any => Boolean) = this }
+ class Foo { def map(f: Any => Any) = this; def withFilter(cond: Any => Boolean) = this }
for(..$enums) yield $body
""")
assert(enums1 ≈ enums)
View
35 test/junit/scala/util/TryTest.scala
@@ -0,0 +1,35 @@
+package scala.util
+
+import org.junit.runner.RunWith
+import org.junit.runners.JUnit4
+import org.junit.Test
+import org.junit.Assert._
+
+/* Test Try's withFilter method, which was added along with the -Xfuture fix for SI-6455 */
+@RunWith(classOf[JUnit4])
+class TryTest {
+ @Test
+ def withFilterFail(): Unit = {
+ val fail = for (x <- util.Try(1) if x > 1) yield x
+ assert(fail.isFailure)
+ }
+
+ @Test
+ def withFilterSuccess(): Unit = {
+ val success1 = for (x <- util.Try(1) if x >= 1) yield x
+ assertEquals(success1, util.Success(1))
+ }
+
+ @Test
+ def withFilterFlatMap(): Unit = {
+ val successFlatMap = for (x <- util.Try(1) if x >= 1; y <- util.Try(2) if x < y) yield x
+ assertEquals(successFlatMap, util.Success(1))
+ }
+
+ @Test
+ def withFilterForeach(): Unit = {
+ var ok = false
+ for (x <- util.Try(1) if x == 1) ok = x == 1
+ assert(ok)
+ }
+}
Something went wrong with that request. Please try again.