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

Case class copy and apply inherit access modifiers from constructor [ci: last-only] #7702

Open
wants to merge 1 commit into
base: 2.13.x
from

Conversation

Projects
None yet
3 participants
@Jasper-M
Copy link
Member

Jasper-M commented Jan 31, 2019

I guess this'll have to go under a flag, unless someone wants to merge this into RC1 after all...
Also happy to accept pointers on a better way to get the mods of the primary constructor from a ClassDef without getting cyclic reference errors.

@scala-jenkins scala-jenkins added this to the 2.13.1 milestone Jan 31, 2019

@Jasper-M Jasper-M force-pushed the Jasper-M:topic/7884 branch from dec3d17 to b4630df Jan 31, 2019

@Jasper-M

This comment has been minimized.

Copy link
Member Author

Jasper-M commented Feb 1, 2019

The 3 failing tests all look expected to me.

@Jasper-M Jasper-M changed the title [WIP] Case class copy and apply inherit access modifiers from constructor Case class copy and apply inherit access modifiers from constructor [ci: last-only] Feb 6, 2019

@Jasper-M

This comment has been minimized.

Copy link
Member Author

Jasper-M commented Feb 6, 2019

Added some changes to bring the PR in line with lampepfl/dotty#5830 and fixes for corresponding dotty issues lampepfl/dotty#5857 and lampepfl/dotty#5859.

@Jasper-M

This comment has been minimized.

Copy link
Member Author

Jasper-M commented Feb 6, 2019

@adriaanm All those private constructors in pos/userdefined_apply.scala, are they there to avoid that calls to apply get transformed to calls to the constructor? In which case it would be fine to make them protected instead?

@adriaanm

This comment has been minimized.

Copy link
Member

adriaanm commented Feb 6, 2019

@adriaanm All those private constructors in pos/userdefined_apply.scala, are they there to avoid that calls to apply get transformed to calls to the constructor? In which case it would be fine to make them protected instead?

IIRC it's to control who can create an instance of a class -- if this is what you want, a private constructor is useless if your apply is public, so basically the same as what your PR offers by default :-)

@Jasper-M Jasper-M force-pushed the Jasper-M:topic/7884 branch from 82362e2 to 282f75b Feb 6, 2019

@Jasper-M

This comment has been minimized.

Copy link
Member Author

Jasper-M commented Feb 6, 2019

@adriaanm Err... Not following 100% what you're saying. Some of those tests seem to rely specifically on the generated apply not being private.

But anyway I think this PR is ready for review and discussion on how to proceed (SIP or no SIP, -Xsource flag or no flag, ...).

Status of this PR right now:

  • private and protected, both qualified and unqualified, get inherited by the copy method from the constructor
  • (un)qualified private gets inherited by the apply method from the constructor
  • (un)qualified protected constructor results in normal public apply method, as seen in dotty
  • case classes with a (un)qualified private constructor will have a companion object that does not extend a FunctionN trait.

I reviewed all failing tests and the bugs they are defending against. In all cases the right thing to do seemed to me turning private constructors into protected. That way apply is still public and calls to apply will not get rewritten into calls to the constructor.

@Jasper-M Jasper-M force-pushed the Jasper-M:topic/7884 branch from 282f75b to 9b3a2cb Feb 7, 2019

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