Skip to content

SI-7459 Handle pattern binders used as prefixes in TypeTrees.#3490

Closed
retronym wants to merge 2 commits intoscala:masterfrom
retronym:ticket/7459
Closed

SI-7459 Handle pattern binders used as prefixes in TypeTrees.#3490
retronym wants to merge 2 commits intoscala:masterfrom
retronym:ticket/7459

Conversation

@retronym
Copy link
Member

@retronym retronym commented Feb 8, 2014

Match translation was incorrect for:

case t => new t.C
case D(t) => new d.C

We would end up with Types in TypeTrees referring to the wrong symbols, e.g:

// t7459a.scala
((x0$1: this.LM) => {
  case <synthetic> val x1: this.LM = x0$1;
  case4(){
   matchEnd3(new tttt.Node[Any]())
  };
  matchEnd3(x: Any){
    x
}

Or:
// t7459b.scala
((x0$1: CC) => {
case val x1: CC = x0$1;
case4(){
if (x1.ne(null))
matchEnd3(new tttt.NodeAny)
else
case5()
};

This commit:

  • Changes bindSubPats to traverse types, as well as terms,
    in search of references to bound symbols
  • Changes Substitution to reuse Tree#substituteSymbols rather
    than the home-brew substitution from Trees to Trees, if the
    to trees are all Idents

I had to dance around the awkward handling of "swatches" (exception
handlers that can be implemented with JVM native type switches) by
duplicating trees to avoid seeing the results of substituteSymbols
in caseDefs after we abandon that approach if we detect the
patterns are too complex late in the game.

@adriaanm
Copy link
Contributor

adriaanm commented Feb 8, 2014

Looks like an unhappy owner chain:

[quick.library] class SerialVersionUID(value: Long) extends scala.annotation.ClassfileAnnotation
[quick.library]       ^
[quick.library] error: java.lang.IllegalArgumentException: Could not find proxy for val s: scala.util.Success in List(value s, value $anonfun, method fallbackTo, trait Future, package concurrent, package scala, package <root>) (currentOwner= method apply )
[quick.library]     at scala.tools.nsc.transform.LambdaLift$LambdaLifter.scala$tools$nsc$transform$LambdaLift$LambdaLifter$$searchIn$1(LambdaLift.scala:317)

@retronym
Copy link
Member Author

retronym commented Feb 8, 2014

And now for the other last 10%:

import scala.concurrent._
import scala.util._

class Test {
  (null: Any) match {
    case s @ Some(_) => ???
    case f @ _ => 
      () => f
      ???
  }
}

@retronym retronym closed this Feb 8, 2014
Match translation was incorrect for:

    case t => new t.C
    case D(t) => new d.C

We would end up with Types in TypeTrees referring to the wrong symbols, e.g:

    // t7459a.scala
    ((x0$1: this.LM) => {
      case <synthetic> val x1: this.LM = x0$1;
      case4(){
       matchEnd3(new tttt.Node[Any]())
      };
      matchEnd3(x: Any){
        x
    }

Or:
    // t7459b.scala
     ((x0$1: CC) => {
       case <synthetic> val x1: CC = x0$1;
       case4(){
         if (x1.ne(null))
           matchEnd3(new tttt.Node[Any]())
         else
           case5()
       };

This commit:

  - Changes `bindSubPats` to traverse types, as well as terms,
    in search of references to bound symbols
  - Changes `Substitution` to reuse `Tree#substituteSymbols` rather
    than the home-brew substitution from `Tree`s to `Tree`s, if the
    `to` trees are all `Ident`s

I had to dance around the awkward handling of "swatches" (exception
handlers that can be implemented with JVM native type switches) by
duplicating trees to avoid seeing the results of `substituteSymbols`
in `caseDefs` after we abandon that approach if we detect the
patterns are too complex late in the game.
@retronym retronym reopened this Feb 8, 2014
@retronym
Copy link
Member Author

retronym commented Feb 9, 2014

Life wasn't meant to be easy. Closing for now.

The difficulty arises from using Tree#substituteSymbols (which is accurate but destroys the original tree) in the environment of the pattern matcher which seems to substitute into trees more than once. (e.g. during analysis and again during codegen.)

Here's another messy swing at it which also gets all the new tests passing. https://github.com/retronym/scala/compare/ticket/7459-reloaded

@retronym retronym closed this Feb 9, 2014
@adriaanm
Copy link
Contributor

adriaanm commented Feb 9, 2014

I was also thinking that we should remove the mutation during analysis. It's another instance of jumping in head first to figure out how deep the pool is -- assuming we can abort mid-flight before we hit the bottom.

@retronym
Copy link
Member Author

retronym commented Feb 9, 2014

That's a dangerous habit in our swimming pool. I was wrangling similar problems in typers recently when it bailed out of names/defaults and left a trail of destruction in the owners of locally defined symbols.

@retronym
Copy link
Member Author

retronym commented Feb 9, 2014

My big find in that latest branch, BTW, was using substituteTypes to subst binder.accessor.type in where needed.

Although it would probably be cleaner to just emit eager binders for anything used in a single type. I got lost trying to implement that, though.

@retronym
Copy link
Member Author

retronym commented Feb 9, 2014

Anyway, this change can land in 2.11.1, so lets refocus on the others for now.

@adriaanm
Copy link
Contributor

I'll put my speedos back in the drawer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants