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

scalap 2.13.x regression with constructor parameters #11747

Open
Shadowfiend opened this issue Sep 23, 2019 · 2 comments

Comments

@Shadowfiend
Copy link

commented Sep 23, 2019

While trying to make lift-json tests pass in Scala 2.13, I discovered a difference in the behavior of scalap between Scala 2.12 and 2.13. In particular, the constructor MethodSignature of a class has children for its parameters in Scala 2.12, but seems to lack these children in Scala 2.13.

I've created a minimal reproduction at https://github.com/Shadowfiend/scala-2-13-scalap-primitive-parameter-detection . While I'm running the example above on OpenJDK 11.0.2, the related test failure is playing out on OpenJDK 8, 11, and 12.

For simplicity, here is the test code:

case class Test(name: String)

class Booyan(base: Int)
class Booyak(base: String)
class Booyap(base: java.lang.Object)

object Test extends App {
  printClass(classOf[Booyan])
  printClass(classOf[Booyak])
  printClass(classOf[Booyap])

  def printClass(clazz: Class[_]) = {
    println(
      ScalaSigParser
        .parse(clazz)
        .get
        .symbols
        .collect {
          case initSymbol: MethodSymbol if initSymbol.name == "<init>" =>
            initSymbol.name + ": " + initSymbol.children
        }
        .mkString(" - ", "\n - ", "")
    )
  }
}

And here is the output on 2.12.9 vs 2.13.1:

# 2.12.9
 - <init>: List(MethodSymbol(base, owner=2, flags=2000, info=15 ,None))
 - <init>: List(MethodSymbol(base, owner=2, flags=2000, info=15 ,None))
 - <init>: List(MethodSymbol(base, owner=2, flags=2000, info=15 ,None))
# 2.13.1
 - <init>: List()
 - <init>: List()
 - <init>: List()
@som-snytt

This comment has been minimized.

Copy link

commented Sep 23, 2019

@Shadowfiend

This comment has been minimized.

Copy link
Author

commented Sep 23, 2019

If this is intentional, I should be able to update our code accordingly. In fact I'll probably do that anyway to get things out the door.

I'll say though, it's weird to filter children on anything other than what the parent is… Without a lot of context on my end, it feels like pickling should use a purpose-added variant instead and children should preserve its original behavior.

EDIT to say: the above opinion aside, I'm fine if the decision is to close this issue if the change is as intended. Thanks for the super-quick pinpoint @som-snytt !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.