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

patmat strips unused bindings, but affects semantics #5024

Closed
scabug opened this issue Sep 23, 2011 · 17 comments
Closed

patmat strips unused bindings, but affects semantics #5024

scabug opened this issue Sep 23, 2011 · 17 comments
Assignees
Milestone

Comments

@scabug
Copy link

scabug commented Sep 23, 2011

The pattern matcher strips unused bindings, which is good, but this alters the behavior w.r.t #1503, which is not so good. I need to record what bindings were stripped and type the RHS accordingly.

@scabug
Copy link
Author

scabug commented Sep 23, 2011

Imported From: https://issues.scala-lang.org/browse/SI-5024?orig=1
Reporter: @paulp

@scabug
Copy link
Author

scabug commented Sep 23, 2011

@acruise said:
It's also a bit annoying for debugging, when I need to assign a bound value to a useless val just to inspect it. ;)

@scabug
Copy link
Author

scabug commented Sep 23, 2011

@paulp said:
I don't understand; this issue is only about the type of the right hand side of the match. Fixing it will not change what data is available where.

@scabug
Copy link
Author

scabug commented Dec 5, 2012

@retronym said:
Is this still relevant under virpatmat?

@scabug
Copy link
Author

scabug commented Dec 5, 2012

@paulp said:
Possibly not - in fact with a less horrendous pattern matcher it may not even be necessary/desirable to strip the bindings. But more importantly, I just discovered #1503 isn't fixed at all.

@scabug
Copy link
Author

scabug commented Jan 23, 2013

@adriaanm said:
if this should be fixed, please rephrase so that it becomes clear what's expected
too much time has gone by to figure out what to do based on the comments

@scabug
Copy link
Author

scabug commented Jan 24, 2013

@paulp said:
This sequence went "true, true, false" when this ticket was opened. Now it goes "true, true, true". Unfortunately it's supposed to go "true, false, false". So the bug has moved - it is no longer that unused bindings are stripped, but that they don't appear to be considered at all, at least not based on this example. I know that #1503 remains unfixed so it's not like they were being adequately considered before either.

scala> val s: Seq[Nothing] = Vector.empty
s: Seq[Nothing] = Vector()

scala> s match { case Nil => true ; case _ => false }
res0: Boolean = true

scala> s match { case x@Nil => true ; case _ => false }
res1: Boolean = true

scala> s match { case x@Nil => {x; true} ; case _ => false }
<console>:9: warning: a pure expression does nothing in statement position; you may be omitting necessary parentheses
              s match { case x@Nil => {x; true} ; case _ => false }
                                       ^
res2: Boolean = true

@scabug
Copy link
Author

scabug commented Jan 24, 2013

@paulp said:

scala> s match { case x@Nil => {x.::(""); true} ; case _ => false }
java.lang.ClassCastException: scala.collection.immutable.Vector cannot be cast to scala.collection.immutable.List
	at .<init>(<console>:9)
	at .<clinit>(<console>)
	at .<init>(<console>:7)
	at .<clinit>(<console>)
	at $print(<console>)

That's a regression from -Xoldpatmat:

scala> val s: Seq[Nothing] = Vector.empty
s: Seq[Nothing] = Vector()

scala> s match { case x@Nil => {x.::(""); true} ; case _ => false }
res0: Boolean = false

There's a test ostensibly for #1503, but it's not a good test.

@scabug
Copy link
Author

scabug commented Feb 1, 2013

@adriaanm said:
I fail to see why x.::("") should type check at all.
All we know is x: Seq[Nothing] and x == Nil.
The latter does not entail any type information.

(By the de facto semantics of a constant pattern, == is used for comparison and not eq.)

Thus x: Seq[Nothing] does not have the member ::.

@scabug
Copy link
Author

scabug commented Feb 1, 2013

@paulp said:
That's the point, it shouldn't typecheck. We know that with fairly high confidence due to the ClassCastException it is presently throwing after it successfully typechecks.

@scabug
Copy link
Author

scabug commented Feb 1, 2013

@adriaanm said:
So then we can fix this and #1503, right?

We know that if this variable is used under this unsound assumption, the code will always fail at run-time.
If the variable does not occur, or its type need not be the unsound type, it will continue to work.

I don't see how this is going to break existing code that worked at run-time -- do we care about time bombs that type checked but could never work?

Lest you think I'm being overly defensive, I'm reacting to https://issues.scala-lang.org/browse/SI-1503?focusedCommentId=43564&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-43564

@scabug
Copy link
Author

scabug commented Feb 1, 2013

@paulp said:
Changing this would cause weaker types to be inferred for case bodies, which would cause weaker types for matches, which would cause weaker types for expressions, which would cause weaker types for methods. This in turn would impact everything: implicit resolution, overloading selection, overrides, conformance, on and on. All of that happens regardless of whether the potentially unsound case body is ever reached at runtime. It could be the world's least sound case body, but if the pattern never matches then it is a tree falling in a forest where everyone has lost their ears to the plague and doesn't care about trees. At least, it is until we make it not compile anymore.

None of that means we shouldn't fix it, and I don't have any reason to believe it's going to hit that many places in the wild, I'm just answering the question.

If you're thinking on this matter, this would be a great time to look at the ticket I was bugging you about a month ago, #785. And at my implementation of much of what's in it, paulp/scala@4bd8ad6ae5 .

@scabug
Copy link
Author

scabug commented Feb 1, 2013

@paulp said:
And on #1503, I don't have a clear picture of this even being a distinct bug from #1503. It was in the old matcher because during a period of implementation desperation I wrote code to strip unused bindings. But from a theoretical standpoint there's no reason to do that.

@scabug
Copy link
Author

scabug commented Feb 1, 2013

@adriaanm said:
I have to run, but the only code ant strap.done could find that's affected by my patch is in GenTypes. I had to turn it into:

      case tpe : NoType.type =>
        reifyMirrorObject(tpe)
      case tpe : NoPrefix.type =>
        reifyMirrorObject(tpe)

Proposed patch: https://github.com/adriaanm/scala/commits/ticket-5024

I'm not sure we should do this in 2.10.1, but slightly in favor.

@scabug
Copy link
Author

scabug commented Feb 2, 2013

@adriaanm said:
probably too big a change for 2.10.1

@scabug
Copy link
Author

scabug commented Oct 15, 2013

@gkossakowski said:
Unassigning and rescheduling to M7 as previous deadline was missed.

@scabug
Copy link
Author

scabug commented Jan 28, 2014

@adriaanm said:
consolidating with #1503

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