SI-8331 make sure type select & applied type doesn't match terms #3594

Merged
merged 2 commits into from Mar 10, 2014

Conversation

Projects
None yet
4 participants
Contributor

densh commented Feb 28, 2014

Due to tree re-use it used to be the fact that type quasiquotes could
match term trees. This commit makes sure selections and applied type and
type applied are all non-overlapping between q and tq.

review @retronym

densh added this to the 2.11.0-RC2 milestone Feb 28, 2014

retronym was assigned by densh Feb 28, 2014

@retronym retronym and 1 other commented on an outdated diff Feb 28, 2014

...flect/scala/reflect/internal/ReificationSupport.scala
@@ -219,7 +231,10 @@ trait ReificationSupport { self: SymbolTable =>
case UnApply(treeInfo.Unapplied(Select(fun, nme.unapply)), pats) =>
Some((fun, pats :: Nil))
case treeInfo.Applied(fun, targs, argss) =>
- Some((SyntacticTypeApplied(fun, targs), argss))
+ if (fun.isTerm)
+ Some((SyntacticTypeApplied(fun, targs), argss))
+ else
+ Some((SyntacticAppliedType(fun, targs), argss))
@retronym

retronym Feb 28, 2014

Owner

You could tighten up the if else here to avoid some nanoscale duplication

@densh

densh Feb 28, 2014

Contributor

Good idea.

@densh

densh Feb 28, 2014

Contributor

Fixed.

Owner

retronym commented Feb 28, 2014

Could you elaborate on how tree reuse leads to the bug, perhaps with an example?

Contributor

densh commented Feb 28, 2014

Sure. Lets say we have a user that has no idea about internal organization of trees but have read a quasiquote guide and decides to write a tree transformer that wraps all term selections into a magical call:

class MagicTransformer extends Transformer {
  override transform(tree: Tree) = tree match {
    case q"$qual.$name" => 
      val transformedQual = transform(qual)
      q"magic($transformedQual, ${name.toString})"
    case _ => super.transform(tree)
  }
}

This code might look correct at first but if we look at the desugaring of the pattern we'll see:

case Select(qual, name) =>

Which will match both term and type selections. Definitely not something one would expect when they probably wanted to match just terms (due to usage of q"" rather than tq"")

Similarly with type application / applied type. One would probably expect q"" to only match type applications and tq"" to only match applied types.

@densh densh added tested and removed tested labels Feb 28, 2014

Member

xeno-by commented Mar 1, 2014

Needs to be rebased.

Member

xeno-by commented Mar 1, 2014

Can something be done here for idents as well?

@xeno-by xeno-by and 1 other commented on an outdated diff Mar 1, 2014

...flect/scala/reflect/internal/ReificationSupport.scala
case TypeApply(fun, targs) => Some((fun, targs))
+ case _ if tree.isTerm => Some((tree, Nil))
+ case _ => None
+ }
+ }
+
+ object SyntacticAppliedType extends SyntacticTypeAppliedExtractor {
@xeno-by

xeno-by Mar 1, 2014

Member

I think this would use a comment that explains its purpose.

@densh

densh Mar 2, 2014

Contributor

Fixed.

@xeno-by xeno-by and 1 other commented on an outdated diff Mar 1, 2014

...flect/scala/reflect/internal/ReificationSupport.scala
@@ -219,7 +231,10 @@ trait ReificationSupport { self: SymbolTable =>
case UnApply(treeInfo.Unapplied(Select(fun, nme.unapply)), pats) =>
Some((fun, pats :: Nil))
case treeInfo.Applied(fun, targs, argss) =>
- Some((SyntacticTypeApplied(fun, targs), argss))
+ val inbrackets =
@xeno-by

xeno-by Mar 1, 2014

Member

inbrackets? What does this name mean?

@densh

densh Mar 2, 2014

Contributor

fun[targs], targs are in brackets.

@xeno-by

xeno-by Mar 2, 2014

Member

But isn't inbrackets going to contain the entire fun[targs] thing?

@densh

densh Mar 2, 2014

Contributor

That's just a temp variable name that's better than tree.

@xeno-by

xeno-by Mar 2, 2014

Member

Don't think it's better given that it's misleading. Could you rename?

@densh

densh Mar 2, 2014

Contributor

Into tree?

@densh

densh Mar 2, 2014

Contributor

Fixed.

@xeno-by

xeno-by Mar 2, 2014

Member

Very funny. Not.

Contributor

densh commented Mar 2, 2014

Can something be done here for idents as well?

Idents are handled by Liftable. You can't fix it without redesigning the api, possibly by splitting into multiple type classes for terms, types and patterns.

@densh densh added tested and removed tested labels Mar 2, 2014

@xeno-by xeno-by and 1 other commented on an outdated diff Mar 3, 2014

...flect/scala/reflect/internal/ReificationSupport.scala
@@ -223,7 +241,10 @@ trait ReificationSupport { self: SymbolTable =>
case UnApply(treeInfo.Unapplied(Select(fun, nme.unapply)), pats) =>
Some((fun, pats :: Nil))
case treeInfo.Applied(fun, targs, argss) =>
- Some((SyntacticTypeApplied(fun, targs), argss))
+ val `fun[targs]` =
@xeno-by

xeno-by Mar 3, 2014

Member

result? callee?

@densh

densh Mar 3, 2014

Contributor

Done.

densh removed the tested label Mar 3, 2014

Member

xeno-by commented Mar 3, 2014

LGTM

Member

xeno-by commented Mar 4, 2014

@adriaanm This pull request isn't binary compatible with RC1. Can we merge nonetheless, because: a) it is definitely useful, b) the incompatibility consists in scala.reflect.api#internal and affects only quasiquotes?

Contributor

densh commented Mar 6, 2014

ping @retronym.

Member

xeno-by commented Mar 6, 2014

We need to rebuild this pull request in order to expose and fix the binary incompat that it introduces.

Owner

retronym commented Mar 7, 2014

LGTM

Denys Shabalin added some commits Feb 25, 2014

Denys Shabalin SI-8331 make sure type select & applied type doesn't match terms
Due to tree re-use it used to be the fact that type quasiquotes could
match term trees. This commit makes sure selections and applied type and
type applied are all non-overlapping between q and tq.
67d175f
Denys Shabalin Address pull request feedback
1. Tighten up the if else to avoid duplication
2. Add doc comments
1b5a34b
Contributor

densh commented Mar 9, 2014

Rebased to uncover bincompat issues. Added proposed changes to whitelists.

@densh densh added tested and removed tested labels Mar 9, 2014

Owner

adriaanm commented Mar 10, 2014

LGTM, as discussed during scala meeting

@adriaanm adriaanm added a commit that referenced this pull request Mar 10, 2014

@adriaanm adriaanm Merge pull request #3594 from densh/si/8331
SI-8331 make sure type select & applied type doesn't match terms
51a413e

@adriaanm adriaanm merged commit 51a413e into scala:master Mar 10, 2014

1 check passed

default pr-scala Took 72 min.
Details

@adriaanm adriaanm modified the milestone: 2.11.0-RC3, 2.11.0-RC2 Mar 18, 2014

scabug referenced this pull request in scala/bug Apr 7, 2017

Closed

Type quasiquote may match term trees #8331

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