Skip to content

Private class constructor is public at the bytecode level #12711

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

Closed
julienrf opened this issue Jan 10, 2023 · 10 comments
Closed

Private class constructor is public at the bytecode level #12711

julienrf opened this issue Jan 10, 2023 · 10 comments
Labels

Comments

@julienrf
Copy link

julienrf commented Jan 10, 2023

Classes with private constructors are actually public at the bytecode level, this creates two issues:

  • it can be called from Java code
  • MiMa reports error if its signature changes

Reproduction steps

// Foo.scala
package bug

class Foo private (x: Int)

object Foo {
  def apply(x: Int) = new Foo(x)
}
// Bar.java
package bug

class Bar {
  public void bar() {
    Foo foo = new Foo(42);
  }
}

Output

The code compiles.

Expectation

The Java compilation should fail.

Possible solution

In a discussion, @smarter suggested to emit the constructor as ACC_SYNTHETIC to make it non-accessible from Java, and ignored by mima (see lightbend-labs/mima#92). Note that that change would still be binary compatible with possible existing Java code that would call such constructors (but it would be source incompatible).

Related Scala 3 issue: scala/scala3#16651

@lrytz
Copy link
Member

lrytz commented Jan 11, 2023

Marking the constructor synthetic could break other tools.

IMO this should stay as is in bytecode, it would be good to improve MiMa to understand this pattern.

@julienrf
Copy link
Author

Marking the constructor synthetic could break other tools.

Do you see a “good” reason to rely on this breach?

@lrytz
Copy link
Member

lrytz commented Jan 11, 2023

I'm not saying having the constructor public is good, we should keep things private in bytecode where possible - but there are many cases where we can't. For example anything private[x] is public in bytecode, but MiMa knows how to handle that.

Marking something that's not synthetic as synthetic might break tools that take information from the bytecode.

@SethTisue
Copy link
Member

SethTisue commented Jan 11, 2023

I agree with Lukas. It's actually inherent and normal — though unfortunate! — that we have to mark things in bytecode as public even when they aren't public in the source.

The callable-from-Java thing is unfortunate but it's a much bigger problem than just for constructors, so I don't think it's that helpful or important to fix it just for constructors, especially when the "fix" involves lying by marking actual user code as synthetic.

As for MiMa, being smarter about this is MiMa's responsibility.

@sjrd
Copy link
Member

sjrd commented Jan 11, 2023

The IMO significant difference about constructors is that they keep their name (obviously). For other typical use cases, names are mangled with the fully qualified name of their owner and/or package, and so it's much more obvious that you're not supposed to call them even if they show up.

@sjrd
Copy link
Member

sjrd commented Jan 11, 2023

especially when the "fix" involves lying by marking actual user code as synthetic.

FWIW, my proposed fix was to make them JVM-package-private instead.

@lrytz
Copy link
Member

lrytz commented Jan 11, 2023

For other typical use cases, names are mangled with the fully qualified name

Not always though, for example private[x] members

@lrytz
Copy link
Member

lrytz commented Jan 12, 2023

... also, name-mangling private members that are made public is required to avoid accidental name clashes. I'm not sure if name mangling would have been introduced only to hide private definitions from Java.

@sjrd
Copy link
Member

sjrd commented Jan 12, 2023

I see. Then I guess the least controversial way forward is indeed to change MiMa.

@julienrf
Copy link
Author

Thank you for the discussion, I’ve followed-up by creating lightbend-labs/mima#738

@julienrf julienrf closed this as not planned Won't fix, can't repro, duplicate, stale Jan 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants