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

Weird behavior with default arguments on traits when using FSC

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: Scala 2.9.1, Scala 2.9.2
    • Fix Version/s: Scala 2.10.0-M3, Scala 2.10.0
    • Component/s: Misc Compiler
    • Environment:

      Windows XP x64 SP2; JDK 1.6.0_21; IntelliJ IDEA 10.5.2; Scala Plugin for IntelliJ 0.4.1491

      Description

      Okay, this is a weird one. I suspect it's a bug against FSC, but honestly it could be the IntelliJ Scala plugin's fault; I just don't know.

      In one file, in src:

      package compilerbug
      
      trait SadTrait {
        def buggyMethod(argWithDefault: String = "default") {
          for (i <- 0 to 1) {
            val _ = argWithDefault
          }
        }
      }
      
      object SadObject extends SadTrait
      

      In another file, in test:

      package compilerbug
      
      class TestClass {
        def repro() {
          SadObject.buggyMethod()
        }
      }
      

      With FSC disabled, this compiles fine. With FSC on, this complains:

      error: not enough arguments for method buggyMethod: (argWithDefault$1: String)Unit
      SadObject.buggyMethod()
      

      This appears to be a minimal repro to me; the problem goes away if:

      • TestClass lives in src as well
      • argWithDefault is not used in the for loop
      • the outside of the for loop is commented out leaving a bare val _ = argWithDefault
      • Inside buggyMethod, I let val argAlias = argWithDefault, and then use argAlias in the for loop
      • buggyMethod is defined directly on SadObject rather than in SadTrait

      Fortunately, the fourth bullet point is a pretty general workaround for this problem, so I'm not blocked on it... but weird, right?

        Issue Links

          Activity

          Hide
          Jason Zaugg added a comment -

          It's a head scratcher. I've attached a screenshot from the debugger that shows the type history of the method, second time around.

          TypeHistory(posterasure:3,(argWithDefault: String)Unit,TypeHistory(tailcalls:3,(argWithDefault: String)Unit,TypeHistory(namer:5,(argWithDefault$1: String)Unit,null)))
          
          Show
          Jason Zaugg added a comment - It's a head scratcher. I've attached a screenshot from the debugger that shows the type history of the method, second time around. TypeHistory(posterasure:3,(argWithDefault: String)Unit,TypeHistory(tailcalls:3,(argWithDefault: String)Unit,TypeHistory(namer:5,(argWithDefault$1: String)Unit,null)))
          Hide
          Jason Zaugg added a comment -

          Righto. AddInterfaces#implMethodDef appears culpable. It cut/pastes the method from the trait to the implementation class, without creating new symbols for the parameters. When these get renamed later in erasure, the new names become visible in the type history of the original method at the typer phase of the next run. Or something along those lines.

            private def implMethodDef(tree: Tree): Tree = (
              implMethodMap get tree.symbol
                      map (impl => new ChangeOwnerAndReturnTraverser(tree.symbol, impl)(tree setSymbol impl))
                getOrElse abort("implMethod missing for " + tree.symbol)
            )
          
          Show
          Jason Zaugg added a comment - Righto. AddInterfaces#implMethodDef appears culpable. It cut/pastes the method from the trait to the implementation class, without creating new symbols for the parameters. When these get renamed later in erasure, the new names become visible in the type history of the original method at the typer phase of the next run. Or something along those lines. private def implMethodDef(tree: Tree): Tree = ( implMethodMap get tree.symbol map (impl => new ChangeOwnerAndReturnTraverser(tree.symbol, impl)(tree setSymbol impl)) getOrElse abort("implMethod missing for " + tree.symbol) )
          Hide
          Jason Zaugg added a comment -

          Here's a proposed fix:

          https://github.com/retronym/scala/compare/ticket/5167

          I haven't run the tests to see if it breaks anything else, yet.

          Open questions:

          • Do we have a facility in partest to test this?
          • should we clone symbols in tparams and tpt too?
          • performance: should we fuse this substitution together with ChangeOwnerAndReturnTraverser?
          Show
          Jason Zaugg added a comment - Here's a proposed fix: https://github.com/retronym/scala/compare/ticket/5167 I haven't run the tests to see if it breaks anything else, yet. Open questions: Do we have a facility in partest to test this? should we clone symbols in tparams and tpt too? performance: should we fuse this substitution together with ChangeOwnerAndReturnTraverser ?
          Show
          Jason Zaugg added a comment - https://github.com/scala/scala/pull/659
          Hide
          Jason Zaugg added a comment -

          When these get renamed later in erasure...

          That wasn't accurate: AddInterfaces is actually a part of the erasure phase.

          Show
          Jason Zaugg added a comment - When these get renamed later in erasure... That wasn't accurate: AddInterfaces is actually a part of the erasure phase.

            People

            • Assignee:
              Jason Zaugg
              Reporter:
              Ryan Hendrickson
            • Votes:
              7 Vote for this issue
              Watchers:
              10 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development