Restrict visibility of copy and apply

[odersky]

Make the synthesized copy and apply methods of a case class have the
same visibility as its private constructor.

I believe it would make sense to spec it that way.

[odersky]

This was prompted by https://contributors.scala-lang.org/t/removing-copy-operation-from-case-classes-with-private-constructors/2605.

[allanrenucci]

I suspect the change to apply might break a lot of code. I'll run this patch through the community build

[odersky]

I suspect the change to apply might break a lot of code. I'll run this patch through the community build

That would be good to know, yes.

[allanrenucci]

It breaks the 2.13 std lib in one place: https://github.com/scala/scala/blob/b846ae5dd600456dd9f4da089bddea8ea5fbb58b/src/library/scala/collection/immutable/VectorMap.scala#L213

I am not sure what was the intention of the author here. cc/ @odd

[julienrf]

I guess NextOfKin’s constructor should be private[Tombstone] instead.

[smarter]

@adriaanm Opinions on whether this change could also make it to 2.13 or 2.14 ?

[adriaanm]

We could do something for 2.13.x (x > 0) under -Xsource to experiment with the community build. We're kind of maxed out for the next few weeks, but happy to accept PRs sooner.

[odersky]

I guess NextOfKin’s constructor should be private[Tombstone] instead.

I agree. Exactly the same mistake was found in dotty's Interactive object. So I my tentative conclusion is that any errors caused by this change are true errors that should be fixed.

@julienrf @odd Can one of you fix VectorMap, then we can merge this PR.

[odersky]

Thanks for the quick fix @julienrf. Can you merge this PR once the 2.13.x fix is in?

[smarter]

Hmm, If this doesn't get into 2.13, it's going to make cross-compilation harder, since getting your code to compile with Dotty might require a non-binary-compatible change :/.

[adriaanm]

I'll discuss with the team to see what we can do here.

[odersky]

A possible option would be to not do this under Scala-2 mode. Let's see what Scala 2.14 does.

But so far my understanding is that the cases where it matters would be rare

• there are not that many classes with private constructors
• I don't think there's a known or recommended pattern to combine a private constructor with a public apply or copy method. That defeats the purpose.
• So I believe that any violations of the rules are accidental, and errors.

[odd]

@allanrenucci @julienrf Indeed, the intention was to only allow direct calls to the constructor of NextOfKin from inside Tombstone.apply so it should have been private[Tombstone] all along.

[odersky]

@allanrenucci @julienrf Indeed, the intention was to only allow direct calls to the constructor of NextOfKin from inside Tombstone.apply so it should have been private[Tombstone] all along.

So, arguably, the current behavior is surprising and could be a security risk [i.e. nobody thought that apply would still be public even if the constructor was private]. One more reason to fix it.

[adriaanm]

So, I don't think we can change this in 2.13 (at least not by default), both because of how close we are to RC1 and due to the unknown amount of breakage this would cause.

Cross-compilation should be achievable by explicitly writing the private apply method, right? Since these cases are rare, that doesn't sound like a big burden for added security.

[SethTisue]

I don't think there's a known or recommended pattern to combine a private constructor with a public apply or copy method

My informal impression is that a private constructor usually arises when someone is trying to lock down a case class to prevent invalid instances from being constructed. You make the constructor private, and you write a custom public apply method that validates the arguments before calling the private constructor.

Can I assume that combination (private constructor, custom public apply) will remain supported?

[dwijnand]

You make the constructor private, and you write a custom public apply method that validates the arguments before calling the private constructor.

What happens if the arguments aren't valid?

The only use-case I can think of for having a public apply that doesn't throw exceptions and has the same return type (Foo, not Option[Foo]) is for memoisation.

Also, it would be nice if a private constructor didn't become public in the bytecode and therefore public to Java users (scala/bug#6882).

[smarter]

Also, it would be nice if a private constructor didn't become public in the bytecode and therefore public to Java users

https://openjdk.java.net/jeps/181 should make this much easier for Java 11+.

[dwijnand]

See also scala/bug#7085 (comment), Java has a strategy for this for inner classes already:

Another tricky aspect of Java's approach to access is constructors. It creates 'access constructors' with an extra dummy parameter. The type of the parameter was used to uniquely distinguish the constructors, and was chosen to be the type of the inner class that needed to access the constructor.

[smarter]

I know, but I don't think it's worth changing things now just for Java 8.

[dwijnand]

I assume that in whichever version of Scala targets Java 11+ we could change that implementation detail, like when we changed the encoding of functions into lambdas.

[odd]

The only use-case I can think of for having a public apply that doesn't throw exceptions and has the same return type (Foo, not Option[Foo]) is for memoisation.

A public user defined apply method is also useful for normalizing parameters before passing them on to the private constructor (especially so if the class is a case class and you want the normalized value to be used in the auto generated equals, hashCode etc).

[propensive]

In the longer term, can copy be implemented as an extension method with Scala 3's metaprogramming facilities? Would that be able to take into account the visibility modifiers on the constructor?