SI-7459 Handle pattern binders used as prefixes in TypeTrees.#4122
Merged
adriaanm merged 1 commit intoscala:2.11.xfrom Dec 18, 2014
Merged
SI-7459 Handle pattern binders used as prefixes in TypeTrees.#4122adriaanm merged 1 commit intoscala:2.11.xfrom
adriaanm merged 1 commit intoscala:2.11.xfrom
Conversation
Member
Author
|
PLS REBUILD ALL |
|
(kitty-note-to-self: ignore 62323336) |
Member
Author
|
PLS REBUILD ALL |
|
(kitty-note-to-self: ignore 62667904) |
Member
Author
|
Dang, the new tests I added crash in the |
91e2dbf to
f633fe3
Compare
f633fe3 to
559eb52
Compare
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`.
559eb52 to
5b8327b
Compare
Contributor
There was a problem hiding this comment.
Could we push this exception (!(isPastTyper && sym.isSynthetic) && ...) into isVolatile (... isVolatile(tp.underlying) && (sym.hasVolatileType || !sym.isStable))?
Contributor
There was a problem hiding this comment.
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.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Match translation was incorrect for:
We would end up with Types in TypeTrees referring to the wrong
symbols, e.g:
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:
bindSubPatsto traverse types, as well as terms,in search of references to bound symbols
Substitutionto reuseTree#substituteSymbolsratherthan the home-brew substitution from
Trees toTrees, if thetotrees are allIdentsI 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
substituteSymbolsin
caseDefsafter we abandon that approach if we detect thepatterns 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:.. 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.