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

Regression involving macros, resetLocalAttrs, and singleton types

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: Scala 2.10.2-RC1
    • Fix Version/s: Scala 2.10.2-RC2
    • Component/s: Macros
    • Labels:
      None

      Description

      The following REPL transcript works in 2.10.1, but not 2.10.2-RC1:

      scala> final case class CV(p: Int = 3, g: Int = 2)
      defined class CV
      
      scala> import scala.reflect._,macros._, scala.language.experimental.macros
      import scala.reflect._
      import macros._
      import scala.language.experimental.macros
      
      scala> def macroImpl[T: c.WeakTypeTag](c: Context)(t: c.Expr[T]): c.Expr[List[T]] = {
           |   val r = c.universe.reify { List(t.splice) }
           |   c.Expr[List[T]]( c.resetLocalAttrs(r.tree) )
           | }
      macroImpl: [T](c: scala.reflect.macros.Context)(t: c.Expr[T])(implicit evidence$1: c.WeakTypeTag[T])c.Expr[List[T]]
      
      scala> def demo[T](t: T): List[T] = macro macroImpl[T]
      demo: [T](t: T)List[T]
      
      scala> demo { val d = 4; CV(g = d); "a" }
      <console>:19: error: type mismatch;
       found   : d.type (with underlying type Int)
       required: d.type
                    demo { val d = 4; CV(g = d); "a" }
                                             ^
      

      The code compiles in 2.10.2-RC1 when one of:

      • resetLocalAttrs is removed
      • named/default arguments aren't used
      • CV(g = d: Int) is used

        Issue Links

          Activity

          Hide
          Paul Phillips added a comment -

          It's 83c9c764b5. Pulled back too far.

          Show
          Paul Phillips added a comment - It's 83c9c764b5. Pulled back too far.
          Hide
          Jason Zaugg added a comment -

          We generated a ValDef whose tpt TypeTree has no original; this contains a reference to the symbol for `d`. resetAttrs and the retypecheck assigns a new symbol for d and leaves a the reference to the prior symbol dangling.

          I think I'll revert the fix altogether on 2.10.x, and refine it on master to only use the stable type for the named arguments if needed to conform to the formal parameter type.

          The real bug is the resetAttrs concept.

          Show
          Jason Zaugg added a comment - We generated a ValDef whose tpt TypeTree has no original; this contains a reference to the symbol for `d`. resetAttrs and the retypecheck assigns a new symbol for d and leaves a the reference to the prior symbol dangling. I think I'll revert the fix altogether on 2.10.x, and refine it on master to only use the stable type for the named arguments if needed to conform to the formal parameter type. The real bug is the resetAttrs concept.
          Hide
          Grzegorz Kossakowski added a comment -

          Jason, I found your last statement intriguing. Could you elaborate why you find resetAttrs a bad idea and how would you address use case resetAttrs tries to serve?

          Show
          Grzegorz Kossakowski added a comment - Jason, I found your last statement intriguing. Could you elaborate why you find resetAttrs a bad idea and how would you address use case resetAttrs tries to serve?
          Hide
          Jason Zaugg added a comment - - edited

          It has been a constant source of bugs. No one can really say with great certainty what it does. To maintain the invariant that typecheck(reset(typecheck(t)) =~= typecheck(t), all sorts of finicky tricks like TypeTree#original are needed, and they don't always suffice. See https://groups.google.com/d/msg/scala-internals/t6cpY7fGng4/0CIfXj1e-UEJ for a recent example.

          I'm exploring ways that macros can avoid the need to use resetAttrs, e.g. if they just need to splice a tree into a new context, we just need to expose `changeOwner` transformations, see: https://issues.scala-lang.org/browse/SI-5797?focusedCommentId=63969&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-63969. Once we get through the current crop of releases, I'm going to finish my resetAttrs-free version of scala-async.

          See also:

          https://issues.scala-lang.org/browse/SI-6187
          https://issues.scala-lang.org/browse/SI-5464
          ...

          Show
          Jason Zaugg added a comment - - edited It has been a constant source of bugs. No one can really say with great certainty what it does. To maintain the invariant that typecheck(reset(typecheck(t)) =~= typecheck(t) , all sorts of finicky tricks like TypeTree#original are needed, and they don't always suffice. See https://groups.google.com/d/msg/scala-internals/t6cpY7fGng4/0CIfXj1e-UEJ for a recent example. I'm exploring ways that macros can avoid the need to use resetAttrs, e.g. if they just need to splice a tree into a new context, we just need to expose `changeOwner` transformations, see: https://issues.scala-lang.org/browse/SI-5797?focusedCommentId=63969&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-63969 . Once we get through the current crop of releases, I'm going to finish my resetAttrs-free version of scala-async. See also: https://issues.scala-lang.org/browse/SI-6187 https://issues.scala-lang.org/browse/SI-5464 ...
          Hide
          Paul Phillips added a comment -

          I'm surprised that comment requires elaboration. reset* was sure to be a nightmare from the beginning. Do people not notice that almost every new mechanism which comes along is a means of avoiding, undoing, freezing, thawing, or reconstructing the mutation performed by earlier mechanisms? Instead of turtles all the way down, it's mutants all the way down.

          The compiler as it is will never be robust. As long as methods with names like "resetFoo" are piled on top of the problem rather than doing something about the root cause, this will never change.

          Show
          Paul Phillips added a comment - I'm surprised that comment requires elaboration. reset* was sure to be a nightmare from the beginning. Do people not notice that almost every new mechanism which comes along is a means of avoiding, undoing, freezing, thawing, or reconstructing the mutation performed by earlier mechanisms? Instead of turtles all the way down, it's mutants all the way down. The compiler as it is will never be robust. As long as methods with names like "resetFoo" are piled on top of the problem rather than doing something about the root cause, this will never change.
          Hide
          Grzegorz Kossakowski added a comment -

          The only reason I asked for elaboration is to hear from people that fully understand it so I don't need to guess.

          I agree that trying to reset mutable state in case of ad-hoc caching mechanisms we have in the compiler is not a good idea. I just wanted to know what breaks exactly in this particular case.

          Thanks to both Jason and Paul.

          Show
          Grzegorz Kossakowski added a comment - The only reason I asked for elaboration is to hear from people that fully understand it so I don't need to guess. I agree that trying to reset mutable state in case of ad-hoc caching mechanisms we have in the compiler is not a good idea. I just wanted to know what breaks exactly in this particular case. Thanks to both Jason and Paul.
          Hide
          Paul Phillips added a comment -

          I don't mean to discourage questions. It's just that at this point I've wasted an awful lot of my life on issues which (it is increasingly clear to me) are completely unnecessary. And there's not really any end in sight.

          Show
          Paul Phillips added a comment - I don't mean to discourage questions. It's just that at this point I've wasted an awful lot of my life on issues which (it is increasingly clear to me) are completely unnecessary. And there's not really any end in sight.
          Show
          Jason Zaugg added a comment - https://github.com/scala/scala/pull/2601
          Hide
          Mark Harrah added a comment -

          Thanks!

          Show
          Mark Harrah added a comment - Thanks!

            People

            • Assignee:
              Jason Zaugg
              Reporter:
              Mark Harrah
            • Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development