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

SI-7859 Value classes may wrap a non-public member #2965

Merged
merged 1 commit into from
Oct 3, 2013

Conversation

retronym
Copy link
Member

We allow value class constructors to be non-public, so to be regular,
we should also allow the same for the param accessor.

This commit uses the 'makeNotPrivate' machinery to ensure that
the backend can generate the requisite unboxing calls.

This commit:

  • refactors the code that enforced the restrictions, improving
    a few error messages and positions. The remaining restrictions
    needed some rewording in light of this change.

  • allows value classes to have non-public, val parameters.
    private[this] / protected[this] are still disallowed as value
    classes don't have a concept of this, and because trying to
    accomdate then would complicate the implementation.

    This means that class C(x: Int) extends AnyVal is not allowed,
    the user still must write private val x: Int or val x: Int.

  • Outlaw class C()()(val x: Int) extends AnyVal to curtail any
    bugs that might lurk in such a formulation.

The tests:

  • Show that the privacy is respected in the typer phase, under
    joint and separate compilation. We don't want a repeat performance
    of SI-6601.
  • Show that code that needs compiler-generated unboxes works under
    both compilation scenarios
  • Checks that the remaining restrictions are enforced and well
    communicated.

Review by @gkossakowski

@retronym
Copy link
Member Author

Updating the documentation: scala/docs.scala-lang#250

@gkossakowski
Copy link
Member

I must say that I'm not entirely happy with increased use of makeNotPrivate but I won't block this PR on that basis.

The problem with makeNotPrivate is:

  • it hurts Java interoperability because we expose private members to Java classes
  • there's one name space for instance, public members of a class so in order to avoid clashes we have to mangle names
  • public names look confusing in tools like debuggers

I'd vastly prefer if we followed what javac does which is emitting static accessor methods marked as synthetic methods (so debuggers and other tools ignore them). This would address all three points. The second point is addressed because static names sit in class name space (so they are not inherited).

I mention this only because I'm uneasy to see yet another use of suboptimal (in my opinion) mechanism.

I'll review this PR on Monday or Tuesday.

@retronym
Copy link
Member Author

Just to link things together, here's @gkossakowski's ticket proposing a more Java-esque approach to access: https://issues.scala-lang.org/browse/SI-7085

@gkossakowski
Copy link
Member

@retronym: thanks, I totally forgot that this is being tracked in JIRA. I must develop the habit of searching JIRA before I repeat myself again.

@retronym
Copy link
Member Author

PLS SYNC

@retronym
Copy link
Member Author

Just spotted the test failures. Will investigate.

We allow value class constructors to be non-public, so to be regular,
we should also allow the same for the param accessor.

This commit uses the 'makeNotPrivate' machinery to ensure that
the backend can generate the requisite unboxing calls.

This commit:

  - refactors the code that enforced the restrictions, improving
    a few error messages and positions. The remaining restrictions
    needed some rewording in light of this change.
  - allows value classes to have non-public, val parameters.
    private[this] / protected[this] are still disallowed as value
    classes don't have a concept of `this`, and because trying to
    accomdate then would complicate the implementation.

    This means that `class C(x: Int) extends AnyVal` is not allowed,
    the user still must write `private val x: Int` or `val x: Int`.
  - Outlaw `class C()()(val x: Int) extends AnyVal` to curtail any
    bugs that might lurk in such a formulation.

The tests:

  - Show that the privacy is respected in the typer phase, under
    joint and separate compilation. We don't want a repeat performance
    of SI-6601.
  - Show that code that needs compiler-generated unboxes works under
    both compilation scenarios
  - Checks that the remaining restrictions are enforced and well
    communicated.
@retronym
Copy link
Member Author

Pushed an updated check file.

@retronym
Copy link
Member Author

retronym commented Oct 3, 2013

ping @gkossakowski

@gkossakowski
Copy link
Member

LGTM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants