Skip to content

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

Merged
adriaanm merged 1 commit intoscala:2.11.xfrom
retronym:ticket/7459-2
Dec 18, 2014
Merged

SI-7459 Handle pattern binders used as prefixes in TypeTrees.#4122
adriaanm merged 1 commit intoscala:2.11.xfrom
retronym:ticket/7459-2

Conversation

@retronym
Copy link
Member

@retronym retronym commented Nov 9, 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.

I also had to add an escape hatch for the "type selection from
volatile type" check in the type checker. Without this, the
translation of pos/t7459c.scala:

case <synthetic> val x1: _$1 = (null: Test.Mirror[_]).universe;
case5(){
  if (x1.isInstanceOf[Test.JavaUniverse])
    {
      <synthetic> val x2: _$1 with Test.JavaUniverse = (x1.asInstanceOf[_$1 with Test.JavaUniverse]: _$1 with Test.JavaUniverse);
      matchEnd4({
        val ju1: Test.JavaUniverse = x2;
        val f: () => x2.Type = (() => (null: x2.TypeTag[Nothing]).tpe);

.. triggers that error at x2.TypeTag.

Review by @adriaanm. I think we can do better for 2.12 as we discussed in #3490.
But we should consider booking this progress.

@scala-jenkins scala-jenkins added this to the 2.11.5 milestone Nov 9, 2014
@retronym
Copy link
Member Author

retronym commented Nov 9, 2014

PLS REBUILD ALL

@scala-jenkins
Copy link

(kitty-note-to-self: ignore 62323336)
🐱 Roger! Rebuilding pr-scala for 91e2dbf. 🚨

@retronym
Copy link
Member Author

PLS REBUILD ALL

@scala-jenkins
Copy link

(kitty-note-to-self: ignore 62667904)
🐱 Roger! Rebuilding pr-scala for 91e2dbf. 🚨

@retronym
Copy link
Member Author

Dang, the new tests I added crash in the icode phase with -optimize enabled (even with all its component flags disabled):

% qscalac -Ydead-code -Yinline:false -Yclosure-elim:false -Yinline-handlers:false -Yconst-opt:false test/files/run/t7459b.scala

% qscalac -optimize -Ydead-code:false -Yinline:false -Yclosure-elim:false -Yinline-handlers:false -Yconst-opt:false test/files/run/t7459b.scala
error:
  symbol value tttt does not exist in LM.<init>
     while compiling: test/files/run/t7459b.scala
        during phase: icode
     library version: version 2.11.5-20141110-000629-91e2dbf786
    compiler version: version 2.11.5-20141110-000629-91e2dbf786
  reconstructed args: -optimise

  last tree to typer: Ident(n)
       tree position: line 15 of test/files/run/t7459b.scala
            tree tpe: LM
              symbol: value n
   symbol definition: n: LM (a TermSymbol)
      symbol package: <empty>
       symbol owners: value n -> constructor CC -> class CC
           call site: class Test$delayedInit$body in package <empty>

== Source file context for tree position ==

    12   new LM().g(new CC(new LM()))
    13 }
    14 case class CC(n: LM)
    15
error: scala.reflect.internal.FatalError:
  symbol value tttt does not exist in LM.<init>
     while compiling: test/files/run/t7459b.scala

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
  - extends `substIdentsForTrees` to substitute selections like
    `x1.caseField` into types.

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.

I also had to add an escape hatch for the "type selection from
volatile type" check in the type checker. Without this, the
translation of `pos/t7459c.scala`:

    case <synthetic> val x1: _$1 = (null: Test.Mirror[_]).universe;
    case5(){
      if (x1.isInstanceOf[Test.JavaUniverse])
        {
          <synthetic> val x2: _$1 with Test.JavaUniverse = (x1.asInstanceOf[_$1 with Test.JavaUniverse]: _$1 with Test.JavaUniverse);
          matchEnd4({
            val ju1: Test.JavaUniverse = x2;
            val f: () => x2.Type = (() => (null: x2.TypeTag[Nothing]).tpe);

.. triggers that error at `x2.TypeTag`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we push this exception (!(isPastTyper && sym.isSynthetic) && ...) into isVolatile (... isVolatile(tp.underlying) && (sym.hasVolatileType || !sym.isStable))?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ping @retronym -- I'll merge by end of my day unless you'd like to take this on for extra credit

adriaanm added a commit that referenced this pull request Dec 18, 2014
SI-7459 Handle pattern binders used as prefixes in TypeTrees.
@adriaanm adriaanm merged commit ecc6369 into scala:2.11.x Dec 18, 2014
retronym added a commit to retronym/scala that referenced this pull request Jan 29, 2015
The pattern matcher needs to substitute references to bound
variables with references to either a) synthetic temporary vals,
or to b) selections. The latter occurs under -optimize to avoid
to be frugal with local variable slots.

For instance:

```
def test(s: Some[String]) = s match {
  case Some(elem) => elem.length
}
```

Is translated to:

```
def test(s: Some[String]): Int = {
  case <synthetic> val x1: Some[String] = s;
  case4(){
    if (x1.ne(null))
      matchEnd3(x1.x.length())
    else
      case5()
  };
  case5(){
    matchEnd3(throw new MatchError(x1))
  };
  matchEnd3(x: Int){
    x
  }
}
```

However, for a long time this translation failed to consider
references to the binder in types. scala#4122 tried to address this
by either using standard substitution facilities where available
(references to temp vals), and by expanding the patmat's
home grown substitution to handle the more complex case of
referencing a selection.

However, this left the tree in an incoherent state; while it
patched up the `.tpe` field of `Tree`s, it failed to modify the
info of `Symbol`-s.

This led to a crash in the later uncurry phase under
`-Ydelambdafy:method`.

This commit modifies the info of such symbols to get rid of stray
refeferences to the pattern binder symbols.
retronym added a commit to retronym/scala that referenced this pull request Jan 29, 2015
The pattern matcher needs to substitute references to bound
variables with references to either a) synthetic temporary vals,
or to b) selections. The latter occurs under -optimize to avoid
to be frugal with local variable slots.

For instance:

```
def test(s: Some[String]) = s match {
  case Some(elem) => elem.length
}
```

Is translated to:

```
def test(s: Some[String]): Int = {
  case <synthetic> val x1: Some[String] = s;
  case4(){
    if (x1.ne(null))
      matchEnd3(x1.x.length())
    else
      case5()
  };
  case5(){
    matchEnd3(throw new MatchError(x1))
  };
  matchEnd3(x: Int){
    x
  }
}
```

However, for a long time this translation failed to consider
references to the binder in types. scala#4122 tried to address this
by either using standard substitution facilities where available
(references to temp vals), and by expanding the patmat's
home grown substitution to handle the more complex case of
referencing a selection.

However, this left the tree in an incoherent state; while it
patched up the `.tpe` field of `Tree`s, it failed to modify the
info of `Symbol`-s.

This led to a crash in the later uncurry phase under
`-Ydelambdafy:method`.

This commit modifies the info of such symbols to get rid of stray
refeferences to the pattern binder symbols.
retronym added a commit to retronym/scala that referenced this pull request Jan 29, 2015
The pattern matcher needs to substitute references to bound
variables with references to either a) synthetic temporary vals,
or to b) selections. The latter occurs under -optimize to avoid
to be frugal with local variable slots.

For instance:

```
def test(s: Some[String]) = s match {
  case Some(elem) => elem.length
}
```

Is translated to:

```
def test(s: Some[String]): Int = {
  case <synthetic> val x1: Some[String] = s;
  case4(){
    if (x1.ne(null))
      matchEnd3(x1.x.length())
    else
      case5()
  };
  case5(){
    matchEnd3(throw new MatchError(x1))
  };
  matchEnd3(x: Int){
    x
  }
}
```

However, for a long time this translation failed to consider
references to the binder in types. scala#4122 tried to address this
by either using standard substitution facilities where available
(references to temp vals), and by expanding the patmat's
home grown substitution to handle the more complex case of
referencing a selection.

However, this left the tree in an incoherent state; while it
patched up the `.tpe` field of `Tree`s, it failed to modify the
info of `Symbol`-s.

This led to a crash in the later uncurry phase under
`-Ydelambdafy:method`.

This commit modifies the info of such symbols to get rid of stray
refeferences to the pattern binder symbols.
albsadowski pushed a commit to albsadowski/scala that referenced this pull request Feb 17, 2015
The pattern matcher needs to substitute references to bound
variables with references to either a) synthetic temporary vals,
or to b) selections. The latter occurs under -optimize to avoid
to be frugal with local variable slots.

For instance:

```
def test(s: Some[String]) = s match {
  case Some(elem) => elem.length
}
```

Is translated to:

```
def test(s: Some[String]): Int = {
  case <synthetic> val x1: Some[String] = s;
  case4(){
    if (x1.ne(null))
      matchEnd3(x1.x.length())
    else
      case5()
  };
  case5(){
    matchEnd3(throw new MatchError(x1))
  };
  matchEnd3(x: Int){
    x
  }
}
```

However, for a long time this translation failed to consider
references to the binder in types. scala#4122 tried to address this
by either using standard substitution facilities where available
(references to temp vals), and by expanding the patmat's
home grown substitution to handle the more complex case of
referencing a selection.

However, this left the tree in an incoherent state; while it
patched up the `.tpe` field of `Tree`s, it failed to modify the
info of `Symbol`-s.

This led to a crash in the later uncurry phase under
`-Ydelambdafy:method`.

This commit modifies the info of such symbols to get rid of stray
refeferences to the pattern binder symbols.
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