Scala Programming Language
  1. Scala Programming Language
  2. SI-7176

dynamic, implicit views, tree checkers, ..., rely on in-place type checking

    Details

    • Type: Bug Bug
    • Status: Open
    • Priority: Minor Minor
    • Resolution: Unresolved
    • Affects Version/s: Scala 2.9.2, Scala 2.10.0
    • Fix Version/s: Backlog
    • Component/s: None
    • Labels:
      None

      Description

      "What is the typer contract?"

      https://groups.google.com/d/topic/scala-internals/jeixpDwxTt8/discussion

      This patch to return new tree instances from Typer#typed:

      https://github.com/retronym/scala/compare/scala:2.10.x...retronym:topic/tree-clone

      Leads to these failing tests:

      https://scala-webapps.epfl.ch/jenkins/job/scala-checkin-manual/830/console

      Use of Tree#== is the first culprit.

        Issue Links

          Activity

          Hide
          Paul Phillips added a comment -

          Most calls to == come by way of pattern matching on trees, because patterns like EmptyTree call ==. In my opinion this is a specification bug (see SI-785) and we would be better off in numerous ways if it would call eq.

          In fact this is a perfect example of the problem with having different semantics for objects and vals. EmptyTree used to be an object, and in those days it would call eq. At some point during reflection changes it became "val EmptyTree", as did NoSymbol. I'm sure there's zero chance anyone was thinking "and part of this change will be upending the semantics of pattern matching on these things."

          Show
          Paul Phillips added a comment - Most calls to == come by way of pattern matching on trees, because patterns like EmptyTree call ==. In my opinion this is a specification bug (see SI-785 ) and we would be better off in numerous ways if it would call eq. In fact this is a perfect example of the problem with having different semantics for objects and vals. EmptyTree used to be an object, and in those days it would call eq. At some point during reflection changes it became "val EmptyTree", as did NoSymbol. I'm sure there's zero chance anyone was thinking "and part of this change will be upending the semantics of pattern matching on these things."
          Hide
          Jason Zaugg added a comment -

          Wait, but for Tree that's the same thing, right?

          // Tree 
          override def equals(that: Any) = this eq that.asInstanceOf[AnyRef]
          

          It happens to be useful in this case that the calls go through == so we have somewhere to intercept/log/analyse.

          Show
          Jason Zaugg added a comment - Wait, but for Tree that's the same thing, right? // Tree override def equals(that: Any) = this eq that.asInstanceOf[AnyRef] It happens to be useful in this case that the calls go through == so we have somewhere to intercept/log/analyse.
          Hide
          Paul Phillips added a comment -

          Yes, I was just mentioning it because it makes the traffic through Tree#== much higher than it would otherwise be. So I'm not sure it's so useful, because the ones arising from pattern matches are the ones we want to ignore.

          Show
          Paul Phillips added a comment - Yes, I was just mentioning it because it makes the traffic through Tree#== much higher than it would otherwise be. So I'm not sure it's so useful, because the ones arising from pattern matches are the ones we want to ignore.
          Hide
          Jason Zaugg added a comment -

          Who would have thought that this could go wrong? https://github.com/scala/scala/commit/86397c9

          // typedCompoundTypeTree
          newTyper(context.make(templ, self.typeSymbol, decls)).typedRefinement(templ)
          
              def typedRefinement(templ: Template) {
                val stats = templ.body
                namer.enterSyms(stats)
          
                // need to delay rest of typedRefinement to avoid cyclic reference errors
                unit.toCheck += { () =>
                  val stats1 = typedStats(stats, NoSymbol)
                  // this code kicks in only after typer, so `stats` will never be filled in time
                  // as a result, most of compound type trees with non-empty stats will fail to reify
                  // todo. investigate whether something can be done about this
                  val att = templ.attachments.get[CompoundTypeTreeOriginalAttachment].getOrElse(CompoundTypeTreeOriginalAttachment(Nil, Nil))
                  templ.removeAttachment[CompoundTypeTreeOriginalAttachment]
                  templ updateAttachment att.copy(stats = stats1)
                  for (stat <- stats1 if stat.isDef) {
                    val member = stat.symbol
                    if (!(context.owner.ancestors forall
                          (bc => member.matchingSymbol(bc, context.owner.thisType) == NoSymbol))) {
                            member setFlag OVERRIDE
                          }
                  }
                }
              }
          

          The lowest friction solution for this would be to make CompoundTypeTreeOriginalAttachment mutable. We could then attach it to templ immediately. The deferred code would operate on that attachment, which would have been passed to the copy of tree.

          15% less hacky, we could avoid registering this with `unit.toCheck`, and instead have a standard ToCheck(() => Unit) attachment. At the end of each typing a compilation unit, we would traverse the final tree and execute these blocks of code.

          Or find some other way to break the cycles.

          Show
          Jason Zaugg added a comment - Who would have thought that this could go wrong? https://github.com/scala/scala/commit/86397c9 // typedCompoundTypeTree newTyper(context.make(templ, self.typeSymbol, decls)).typedRefinement(templ) def typedRefinement(templ: Template) { val stats = templ.body namer.enterSyms(stats) // need to delay rest of typedRefinement to avoid cyclic reference errors unit.toCheck += { () => val stats1 = typedStats(stats, NoSymbol) // this code kicks in only after typer, so `stats` will never be filled in time // as a result, most of compound type trees with non-empty stats will fail to reify // todo. investigate whether something can be done about this val att = templ.attachments.get[CompoundTypeTreeOriginalAttachment].getOrElse(CompoundTypeTreeOriginalAttachment(Nil, Nil)) templ.removeAttachment[CompoundTypeTreeOriginalAttachment] templ updateAttachment att.copy(stats = stats1) for (stat <- stats1 if stat.isDef) { val member = stat.symbol if (!(context.owner.ancestors forall (bc => member.matchingSymbol(bc, context.owner.thisType) == NoSymbol))) { member setFlag OVERRIDE } } } } The lowest friction solution for this would be to make CompoundTypeTreeOriginalAttachment mutable. We could then attach it to templ immediately. The deferred code would operate on that attachment, which would have been passed to the copy of tree. 15% less hacky, we could avoid registering this with `unit.toCheck`, and instead have a standard ToCheck(() => Unit) attachment. At the end of each typing a compilation unit, we would traverse the final tree and execute these blocks of code. Or find some other way to break the cycles.
          Hide
          Eugene Burmako added a comment -

          Reify also relies on some trees to be typechecked in place. Since we cannot easily reify typetrees, we have to roll them back to originals, but then we need the originals to be symful, which either requires that they are typechecked in place or needs complex bookkeeping. This is explained in details in https://github.com/scalamacros/kepler/blob/88b2915790a6a2ccfa490de6e36aa355148a42b2/src/compiler/scala/reflect/reify/phases/Reshape.scala#L130

          Show
          Eugene Burmako added a comment - Reify also relies on some trees to be typechecked in place. Since we cannot easily reify typetrees, we have to roll them back to originals, but then we need the originals to be symful, which either requires that they are typechecked in place or needs complex bookkeeping. This is explained in details in https://github.com/scalamacros/kepler/blob/88b2915790a6a2ccfa490de6e36aa355148a42b2/src/compiler/scala/reflect/reify/phases/Reshape.scala#L130
          Hide
          Jason Zaugg added a comment -

          Next cab off the rank: when we search for an implicit view that provided some member that matches the arguments, the types of those arguments are looked up in context.tree.

              /** Try to apply an implicit conversion to `qual` so that it contains
               *  a method `name`. If that's ambiguous try taking arguments into
               *  account using `adaptToArguments`.
               */
              def adaptToMemberWithArgs(tree: Tree, qual: Tree, name: Name, mode: Int, reportAmbiguous: Boolean, saveErrors: Boolean): Tree = {
                def onError(reportError: => Tree): Tree = {
                  context.tree match {
                    case Apply(tree1, args) if (tree1 eq tree) && args.nonEmpty =>
                      silent(_.typedArgs(args, mode)) match {
                        case SilentResultValue(xs) =>
                          val args = xs.asInstanceOf[List[Tree]]
                          if (args exists (_.isErrorTyped))
                            reportError
                          else
                            adaptToArguments(qual, name, args, WildcardType, reportAmbiguous, saveErrors)
                        case _            =>
                          reportError
                      }
                    case _ =>
                      reportError
                  }
                }
                silent(_.adaptToMember(qual, HasMember(name), false)) match {
                    case SilentResultValue(res) => res
                    case SilentTypeError(err) => onError({if (reportAmbiguous) { context.issue(err) }; setError(tree)})
                }
              }
          

          But if typed doesn't operate in-place, that tree contains untyped arguments.

          Show
          Jason Zaugg added a comment - Next cab off the rank: when we search for an implicit view that provided some member that matches the arguments, the types of those arguments are looked up in context.tree . /** Try to apply an implicit conversion to `qual` so that it contains * a method `name`. If that's ambiguous try taking arguments into * account using `adaptToArguments`. */ def adaptToMemberWithArgs(tree: Tree, qual: Tree, name: Name, mode: Int, reportAmbiguous: Boolean, saveErrors: Boolean): Tree = { def onError(reportError: => Tree): Tree = { context.tree match { case Apply(tree1, args) if (tree1 eq tree) && args.nonEmpty => silent(_.typedArgs(args, mode)) match { case SilentResultValue(xs) => val args = xs.asInstanceOf[List[Tree]] if (args exists (_.isErrorTyped)) reportError else adaptToArguments(qual, name, args, WildcardType, reportAmbiguous, saveErrors) case _ => reportError } case _ => reportError } } silent(_.adaptToMember(qual, HasMember(name), false)) match { case SilentResultValue(res) => res case SilentTypeError(err) => onError({if (reportAmbiguous) { context.issue(err) }; setError(tree)}) } } But if typed doesn't operate in-place, that tree contains untyped arguments.
          Hide
          Jason Zaugg added a comment - - edited

          Turns out my canary was placed on the wrong part of the coal mine. By changing it to duplicate the trees after type checking, all of the problems with inspecting context.tree evaporated. Thanks for Lukas and Eugene for helping out with this.

          That did leave a few legitimate failures, such as neg/t5696.scala and pos/t5720-ownerous.scala.

          The updated patch:

          @@ -5615,7 +5618,7 @@ trait Typers extends Modes with Adaptations with Tags {
                   }
          
                   val alreadyTyped = tree.tpe ne null
          -        var tree1: Tree = if (alreadyTyped) tree else {
          +        val tree11: Tree = if (alreadyTyped) tree else {
                     printTyping(
                       ptLine("typing %s: pt = %s".format(ptTree(tree), ptPlugins),
                         "undetparams"      -> context.undetparams,
          @@ -5628,6 +5631,7 @@ trait Typers extends Modes with Adaptations with Tags {
                     )
                     typed1(tree, mode, dropExistential(ptPlugins))
                   }
          +        val tree1 = tree11.shallowDuplicate
          
          Show
          Jason Zaugg added a comment - - edited Turns out my canary was placed on the wrong part of the coal mine. By changing it to duplicate the trees after type checking, all of the problems with inspecting context.tree evaporated. Thanks for Lukas and Eugene for helping out with this. That did leave a few legitimate failures, such as neg/t5696.scala and pos/t5720-ownerous.scala. The updated patch: @@ -5615,7 +5618,7 @@ trait Typers extends Modes with Adaptations with Tags { } val alreadyTyped = tree.tpe ne null - var tree1: Tree = if (alreadyTyped) tree else { + val tree11: Tree = if (alreadyTyped) tree else { printTyping( ptLine("typing %s: pt = %s".format(ptTree(tree), ptPlugins), "undetparams" -> context.undetparams, @@ -5628,6 +5631,7 @@ trait Typers extends Modes with Adaptations with Tags { ) typed1(tree, mode, dropExistential(ptPlugins)) } + val tree1 = tree11.shallowDuplicate

            People

            • Assignee:
              Unassigned
              Reporter:
              Jason Zaugg
            • Votes:
              0 Vote for this issue
              Watchers:
              7 Start watching this issue

              Dates

              • Created:
                Updated:

                Development