Skip to content
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

deprecate fallback to filter when withFilter is not found in for-comprehension desugaring #6455

Closed
scabug opened this issue Sep 30, 2012 · 18 comments

Comments

@scabug
Copy link

scabug commented Sep 30, 2012

See the error message below? I use filter but it's not defined: I see, it seems I'm a bad kid. But... where is the call to filter? Is the compiler really complaining on the call to withFilter (retorical question, answer: yes)?

[error] /Users/pgiarrusso/Documents/Research/Sorgenti/linqonsteroids/src/main/scala/ivm/optimization/SubquerySharing.scala:372: value filter is not a member of ivm.expressiontree.Lifting.TypeMappingApp[Traversable,TupleTAnd,AnyRef,T]
[error]             Some(ConstByIdentity(t.asInstanceOf[TypeMapping[Traversable, TupleTAnd, AnyRef]], cTag, tTag).get[T](clazz) /*map(_._1) */withFilter Fun.makefun(cond, fx).f)
[error]                                                                                                                                       ^

I did not believe my eyes (I was also tired, so that was a good idea), but after a couple of minutes I realized I should trust them more than my computer.

Grepping withFilter on the compiler reveals the following wonder in Typers:

            val tree1 = // temporarily use `filter` and an alternative for `withFilter`
              if (name == nme.withFilter)
                silent(_ => typedSelect(tree, qual1, name)) match {
                  case SilentResultValue(result) =>
                    result
                  case _ =>
                    silent(_ => typed1(Select(qual1, nme.filter) setPos tree.pos, mode, pt)) match {
[...]

I know this was intended. It's even documented somewhere, IIRC about the desugaring of for comprehensions (where it makes total sense). But whenever I call withFilter? Also, this was introduced IIRC in 2.8.
Could we please, at the very least, deprecate this aliasing in 2.10, so that it can be removed in 2.11? Or improve the error message?

@scabug
Copy link
Author

scabug commented Sep 30, 2012

Imported From: https://issues.scala-lang.org/browse/SI-6455?orig=1
Reporter: @Blaisorblade
Affected Versions: 2.9.2, 2.10.0
See #6571
Duplicates #2700

@scabug
Copy link
Author

scabug commented Oct 1, 2012

@retronym said:
The same question was posed leading up to 2.9, to which Martin replied:

I agree. We intend to keep it deprecated until the next major release after 2.9.

https://groups.google.com/forum/#!msg/scala-internals/-A0dXo3k0Lk/0CNXgzWo02QJ

@scabug
Copy link
Author

scabug commented Dec 12, 2012

@retronym said:
Duplicate of #2700, removal scheduled for 2.12 in #6571

@scabug
Copy link
Author

scabug commented Dec 12, 2012

@retronym said:
Reopening as #6571 is a metabug pointing to this one.

@scabug
Copy link
Author

scabug commented Mar 15, 2013

@adriaanm said:
Un-assigning to foster work stealing, as announced in https://groups.google.com/forum/?fromgroups=#!topic/scala-internals/o8WG4plpNkw

@scabug
Copy link
Author

scabug commented Jul 10, 2013

@adriaanm said:
Unassigning and rescheduling to M6 as previous deadline was missed.

@scabug
Copy link
Author

scabug commented Dec 11, 2013

@adriaanm said:
Simon, would you like to take a stab at this? Please assign back to me if not.

@scabug
Copy link
Author

scabug commented Feb 10, 2014

@adriaanm said:
Since 2.11.0-RC1 is one week away, pushing all non-blockers without PR to 2.11.1-RC1. Please undo the change if I missed work in progress.

@scabug
Copy link
Author

scabug commented Feb 10, 2014

@Blaisorblade said:
Adriaan, this is a request for deprecation, so IIRC it can only be done 2.11.0-RC1 or 2.12.0-RC1. Or did I miss something?
Shouldn't this be a one-liner adding a deprecation warning in the right location?

@scabug
Copy link
Author

scabug commented Feb 10, 2014

@adriaanm said:
Thanks for pointing it out, bulk changes FTW!

@scabug
Copy link
Author

scabug commented Feb 10, 2014

@adriaanm said:
Simon, please assign to me if you're busy this week. Thanks!

@scabug
Copy link
Author

scabug commented Feb 10, 2014

@soc said (edited on Feb 10, 2014 1:48:39 PM UTC):
I'd prefer not deprecating it. Over the mid-term, I think it makes more sense to deprecate withFilter instead as everything seems to be moving towards non-eager evaluation anyway, and with a non-eager filter, withFilter is pointless.

@scabug
Copy link
Author

scabug commented Feb 10, 2014

@Blaisorblade said:
Simon, I think my description must have mislead you, because I hold you in esteem but I really don't see your point. We're not talking of deprecating the filter or withFilter method, or anything like that.

I think Scalac should never rewrite "foo.filter(arg)" to "foo.withFilter(arg)" — but Scalac does exactly this today, under certain conditions, in a half-hearted and undocumented way. This was designed for for-comprehensions, where it can make sense, but it also catches explicit calls to filter, since the difference is already lost when the rewrite happens (because parsing and some desugarings are fused together, as Paul Phillips complains about).

To solve this without breaking code, the transformation should emit a deprecation warning in 2.11, explaining that this transformation will stop happening in 2.12; Martin and Adriaan agreed on this course of action.

I am convinced that your argument has no relevance on whether this transformation should be deprecated. I do see that for lazy views, you might want to define withFilter to be the same as filter, but you can do so in your library, you don't need the compiler doing that for you. That's because we all want as much behavior as possible to be outside the compiler and in the library.

And even if the transformation is kept in, then somebody should improve both the implementation (look at the error message above more closely to see why) and the documentation (this is not in the SLS).

@scabug
Copy link
Author

scabug commented Feb 10, 2014

@soc said:
Hi Paolo,

could be that I'm completely missing the point ... I'm incredibly tired right now, but I re-read the ticket (and #2700) and now I'm even more confused.
So the compiler is complaining that filter is missing, despite the code not actually using filter, but withFilter? Is this the problem?
If yes, then I think this should be fixed ... feel free to reassign it to you, I'm pretty much drowning in tickets and other life-related things currently.

@scabug
Copy link
Author

scabug commented Feb 10, 2014

@Blaisorblade said:
Simon: you got it right.

Adriaan: reassigning to you per your wish, since Simon is busy.

@scabug
Copy link
Author

scabug commented Feb 10, 2014

@adriaanm said:
Thanks for the clarifications, guys!

@scabug
Copy link
Author

scabug commented Feb 20, 2014

@adriaanm said (edited on Feb 20, 2014 6:33:16 AM UTC):
scala/scala#3566

Perhaps a bit eager, but let's see what the build says.

@scabug
Copy link
Author

scabug commented Feb 24, 2014

@adriaanm said:
I weakened the PR to enforce the deprecation (by not doing the rewrite) under -Xfuture.

@scabug scabug closed this as completed Feb 25, 2014
@scabug scabug added this to the 2.11.0-RC1 milestone Apr 7, 2017
Blaisorblade added a commit to dotty-staging/dotty that referenced this issue May 30, 2018
This was dropped for Scala 2 in scala/scala#5252 for
2.12.0-RC1, as discussed for instance in
scala/bug#6455 and
scala/bug#8339.
Blaisorblade added a commit to dotty-staging/dotty that referenced this issue May 30, 2018
This was dropped for Scala 2 in scala/scala#5252 for 2.12.0-RC1, as discussed
for instance in scala/bug#6455 and scala/bug#8339.

Beyond this code, also remove `MaybeFilter` attachment and attending code
introduced in fcea3d5.

A caveat is that, unexpectedly, this code was added in 2016-08-24, after
scala/scala#5252 was merged, as if the decision was reversed; but scala#1461 says:

> Also, I went through tests in pending/pos and reclassified as far as possible.

So I assume this just happened because the Scala 2 PR didn't remove our pending
test.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants