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

Weird behavior with default arguments on traits when using FSC #5167

Closed
scabug opened this issue Nov 8, 2011 · 13 comments
Closed

Weird behavior with default arguments on traits when using FSC #5167

scabug opened this issue Nov 8, 2011 · 13 comments
Assignees
Milestone

Comments

@scabug
Copy link

scabug commented Nov 8, 2011

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?

@scabug
Copy link
Author

scabug commented Nov 8, 2011

Imported From: https://issues.scala-lang.org/browse/SI-5167?orig=1
Reporter: Ryan Hendrickson (ryan.hendrickson_bwater)
Affected Versions: 2.9.1, 2.9.2
Other Milestones: 2.10.0
See #3649
Attachments:

@scabug
Copy link
Author

scabug commented Jan 23, 2012

Pavel Fatin (pavel.fatin) said:
The problem can be reproduced in command line (i.e. outside of IntelliJ IDEA).

@scabug
Copy link
Author

scabug commented Feb 22, 2012

Barry Kaplan (memelet1) said:
I've just been pounding my head on this one. The only solution I have found was to define the method directly in SadObject. When I try the fourth bullet I get a completely different error, but my method is a bit different:

trait SadTrait {
  def buggyMethod(arg1: String, argWithDefault: String = "default") = ...
}

class SadClass extends SadTrait {
  buggyMethod(arg1 = "foo")
}

The error is:

error: not found: value arg1
Error occurred in an application involving default arguments.
arg1 =

Invoking the method without then named arg seems to work.

@scabug
Copy link
Author

scabug commented Feb 22, 2012

Barry Kaplan (memelet1) said (edited on Feb 22, 2012 4:01:36 AM UTC):
Actually simply removing the name for the argument does not work. I needed to also do what's in bullet 4, ie:

trait SadTrait {
  def buggyMethod(arg1: String, argWithDefault: String = "default") = {
    val arg1_alias = arg1
    val argWithDefault_alias = argWithDefault
    // then use those aliases ...
}

@scabug
Copy link
Author

scabug commented Jun 1, 2012

@OlegYch said:
same happens with overloaded methods without default arguments and an implicit parameter:

trait a {
  def notified(title: String, layout: Component, width: Int, height: Int)(implicit app: com.vaadin.Application) {}
  def notified(title: String, layout: Component, width: Int, height: Int, action: () => Unit)(implicit app: com.vaadin.Application) {}
}
class b extends a {
    notified("", new VerticalLayout, width = 380, height = 430, action = () => ())
}
error: overloaded method value notified with alternatives:
(title: String,layout: com.vaadin.ui.Component,width$3: Int,height$3: Int,action$1: () => Unit)(implicit app$3: com.vaadin.Application)Unit <and>
(title: String,layout: com.vaadin.ui.Component,width: Int,height: Int)(implicit app: com.vaadin.Application)Unit
cannot be applied to (java.lang.String, vaadin.scala.VerticalLayout, width: Int, height: Int, action: () => Unit)
Error occurred in an application involving default arguments.
notified("", new VerticalLayout, width = 380, height = 430, action = () => ())

although it's really flaky, i tried to define a methods with same signatures but with a different name and wasn't able to reproduce

@scabug
Copy link
Author

scabug commented Jun 1, 2012

@retronym said (edited on Jun 1, 2012 7:20:45 PM UTC):
Under scalac -Xresident:

~/code/scratch/20120601 JAVA_OPTS="-Xdebug -Xrunjdwp:transport=dt_socket,server=y,suspend=y,address=5006" ~/code/scala/build/quick/bin/scalac -Xresident -classpath out -d out 
Listening for transport dt_socket at address: 5006

nsc> t5167_1.scala

nsc> t5167_2.scala
t5167_2.scala:5: error: not enough arguments for method buggyMethod: (argWithDefault$1: String)Unit
    SadObject.buggyMethod()
                         ^

The type of the invoked method shows up as:

(argWithDefault$1: String)Unit

The symbol of the parameter of SadTrait#buggyMethod is now owned by the corresponding method in the trait implementation class.

(qual, name, member(qual, name).owner, member(qual, name).info.params.head.owner.owner)
   = (SadObject,buggyMethod,trait SadTrait,class SadTrait$class)

It then tries and fails to find the default getter argWithDefault$1$1.

@scabug
Copy link
Author

scabug commented Jun 1, 2012

@som-snytt said:
Coincidentally, I was just looking at this, because of similarity to a previous bug fix, perhaps. I was guessing transformNamedApplication is confusing lambdalift, based on the weird renaming of the term.

It's not flaky or hard to reproduce, so it seems assailable.

I also have to catch up on the last few episodes of Fringe.

@scabug
Copy link
Author

scabug commented Jun 1, 2012

@som-snytt said:
So, just to confirm, -Yshow-syms shows that lambdalift does the rename. I'm so glad Paul mentioned -Yshow-syms. It really shows syms.

@scabug
Copy link
Author

scabug commented Jun 2, 2012

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

@scabug
Copy link
Author

scabug commented Jun 2, 2012

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

@scabug
Copy link
Author

scabug commented Jun 2, 2012

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

@scabug
Copy link
Author

scabug commented Jun 3, 2012

@retronym said:
scala/scala#659

@scabug
Copy link
Author

scabug commented Jun 3, 2012

@retronym said:

When these get renamed later in erasure...

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

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

No branches or pull requests

2 participants