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

Restrict visibility of copy and apply #5472

Merged
merged 4 commits into from Nov 21, 2018

Conversation

Projects
None yet
9 participants
@odersky
Contributor

odersky commented Nov 19, 2018

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 added some commits Nov 19, 2018

Restrict visibility of copy and apply
Make the synthesized copy and apply methods of a case class have the
same visibility as its private constructor.
Fix tests and bootstrapped code
There were two occurrences where we exploited that a case class with a private
constructor still had a public apply method. I believe both were mistakes.
@odersky

This comment has been minimized.

@allanrenucci

This comment has been minimized.

Member

allanrenucci commented Nov 19, 2018

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

@odersky

This comment has been minimized.

Contributor

odersky commented Nov 19, 2018

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

This comment has been minimized.

Member

allanrenucci commented Nov 20, 2018

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

This comment has been minimized.

julienrf commented Nov 20, 2018

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

@smarter

This comment has been minimized.

Member

smarter commented Nov 20, 2018

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

@adriaanm

This comment has been minimized.

Contributor

adriaanm commented Nov 20, 2018

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

This comment has been minimized.

Contributor

odersky commented Nov 20, 2018

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.

julienrf added a commit to julienrf/scala that referenced this pull request Nov 20, 2018

Make NextOfKin’s constructor private[Tombstone]
This change is required if we want to change the visibility
of the synthetic `apply` method in case classes companion to
match the visibility of its case class constructor.

See also discussion in lampepfl/dotty#5472.
@odersky

This comment has been minimized.

Contributor

odersky commented Nov 20, 2018

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

@smarter

This comment has been minimized.

Member

smarter commented Nov 20, 2018

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

This comment has been minimized.

Contributor

adriaanm commented Nov 20, 2018

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

julienrf added a commit to scala/scala that referenced this pull request Nov 21, 2018

Make NextOfKin’s constructor private[Tombstone]
This change is required if we want to change the visibility
of the synthetic `apply` method in case classes companion to
match the visibility of its case class constructor.

See also discussion in lampepfl/dotty#5472.
@odersky

This comment has been minimized.

Contributor

odersky commented Nov 21, 2018

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

This comment has been minimized.

odd commented Nov 21, 2018

@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

This comment has been minimized.

Contributor

odersky commented Nov 21, 2018

@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

This comment has been minimized.

Contributor

adriaanm commented Nov 21, 2018

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

This comment has been minimized.

Member

SethTisue commented Nov 21, 2018

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

This comment has been minimized.

Contributor

dwijnand commented Nov 21, 2018

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

This comment has been minimized.

Member

smarter commented Nov 21, 2018

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

This comment has been minimized.

Contributor

dwijnand commented Nov 21, 2018

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

This comment has been minimized.

Member

smarter commented Nov 21, 2018

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

@odersky odersky force-pushed the dotty-staging:align-case-class-methods branch from 2db2df4 to d657bd7 Nov 21, 2018

@dwijnand

This comment has been minimized.

Contributor

dwijnand commented Nov 21, 2018

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.

@odersky odersky merged commit 69ef081 into lampepfl:master Nov 21, 2018

2 checks passed

CLA User signed CLA
Details
continuous-integration/drone/pr the build was successful
Details

@allanrenucci allanrenucci deleted the dotty-staging:align-case-class-methods branch Nov 21, 2018

@odd

This comment has been minimized.

odd commented Nov 23, 2018

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

This comment has been minimized.

propensive commented Nov 23, 2018

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?

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