Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
scabug opened this issue Feb 24, 2013 · 8 comments
Open
Milestone

Comments

@scabug
Copy link

scabug commented Feb 24, 2013

"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:

retronym/scala@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.

@scabug
Copy link
Author

scabug commented Feb 24, 2013

Imported From: https://issues.scala-lang.org/browse/SI-7176?orig=1
Reporter: @retronym
Affected Versions: 2.9.2, 2.10.0

@scabug
Copy link
Author

scabug commented Feb 24, 2013

@paulp said:
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 #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."

@scabug
Copy link
Author

scabug commented Feb 24, 2013

@retronym said:
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.

@scabug
Copy link
Author

scabug commented Feb 24, 2013

@paulp said:
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.

@scabug
Copy link
Author

scabug commented Feb 24, 2013

@retronym said:
Who would have thought that this could go wrong? scala/scala@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.

@scabug
Copy link
Author

scabug commented Feb 24, 2013

@xeno-by said:
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

@scabug
Copy link
Author

scabug commented Feb 24, 2013

@retronym said:
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.

@scabug
Copy link
Author

scabug commented Mar 10, 2013

@retronym said (edited on Mar 10, 2013 9:37:22 PM UTC):
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

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

No branches or pull requests

2 participants