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

Infer private[this] #7411

Merged
merged 10 commits into from Oct 21, 2019

Conversation

@odersky
Copy link
Contributor

odersky commented Oct 12, 2019

Infer private[this] if all accesses are via this, even if it is not explicitly specified.

Deprecate usage of private[this] and protected[this] under -strict (i.e. for 3.1.).

The main reasons for dropping private[this] are:

  • it is syntactically an irregular case. A pair of brackets usually encloses a type, but this is a value.
  • its effect over private is purely local and can be easily inferred
  • it leads to bike shedding: should I use private or private[this]? One is shorter but the other might be more efficient.

protected[this] by now influences compiler decisions in no way at all. Its delta over protected is small. protected by itself is niche (e.g. Don Syme has argued eloquently why it is too niche for F#), so having two variants of it looks like overkill.

odersky added 2 commits Oct 12, 2019
The reason is that some of these fields might be converted later to private[this]
fields which do not get  setter. Instead, generate setters for private vars
that are not private[this] in phase Getters.
The plan is that by then we know when a members is private[this].
@odersky odersky force-pushed the dotty-staging:drop-local branch from 6a317a5 to 31228c0 Oct 13, 2019
odersky added 6 commits Oct 13, 2019
Make private term members Local if they are accessed only from
their owner's this-type.
Don't emit signatures for private fields of primitive type
Add docs to state that they will be dropped.
@odersky odersky marked this pull request as ready for review Oct 13, 2019
@odersky

This comment has been minimized.

Copy link
Contributor Author

odersky commented Oct 13, 2019

test performance please

@dotty-bot

This comment has been minimized.

Copy link

dotty-bot commented Oct 13, 2019

performance test scheduled: 1 job(s) in queue, 0 running.

@dotty-bot

This comment has been minimized.

Copy link

dotty-bot commented Oct 13, 2019

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/7411/ to see the changes.

Benchmarks is based on merging with master (0f7a85b)

@pshirshov

This comment has been minimized.

Copy link

pshirshov commented Oct 13, 2019

While I welcome removal of this syntax, I don't thin that the semantic is unneccessary. It's very convenient to be able to enforce stricter rules. Also I wouldn't call protected modifier a niche thing - more access control is always better.

To be quiet honest I think we need lot more flexible access restriction system, so we may specifiy complex predicates on who may and may not access an entity.

Could you consider keeping the functionality under different syntax please?.. Maybe not a modifer but an annotation?..

@odersky

This comment has been minimized.

Copy link
Contributor Author

odersky commented Oct 14, 2019

Maybe not a modifer but an annotation?..

That's just a cop out. Seriously, I have not seen a convincing argument why distinctions between private/protected and private/protected[this] are necessary. And unnecessary distinctions just lead to bikeshedding and complexity.

One could argue that the default is wrong and it should always be [this]. We already had this discussion, and I believe that the reasons for keeping plain private and protected were convincing, for algorithmic reasons as well as backwards compatibility.

@anatoliykmetyuk anatoliykmetyuk self-requested a review Oct 15, 2019
@anatoliykmetyuk anatoliykmetyuk self-assigned this Oct 15, 2019
@anatoliykmetyuk anatoliykmetyuk merged commit 0bf296e into lampepfl:master Oct 21, 2019
2 checks passed
2 checks passed
CLA User signed CLA
Details
continuous-integration/drone/pr Build is passing
Details
@anatoliykmetyuk anatoliykmetyuk deleted the dotty-staging:drop-local branch Oct 21, 2019
@anatoliykmetyuk anatoliykmetyuk added this to the 0.20 Tech Preview milestone Oct 31, 2019
@neko-kai

This comment has been minimized.

Copy link

neko-kai commented Oct 31, 2019

@odersky
protected[this] and [this] access modifiers in general are required for variance in path-dependent types, e.g.

trait ProtectedThisVarianceOk[-A, +B] {
  protected[this] type AliasA = A
  protected[this] type AliasB = B
}

trait ProtectedVarianceIsNotOk[-A, +B] {
  // Error:(16, 18) contravariant type A occurs in invariant position in type A of type AliasA
  //  protected type AliasA = A
  protected type AliasA = A
  protected type AliasB = B
}

Scala 2 allows declaration of AliasA because it's not a visible type member of ProtectedThisVarianceOk – as an internal-only alias its aliasing of contravariant A cannot break the type's outer visible variance.

IMHO removing protected[this] means fundamentally gimping path-dependency.

@odersky

This comment has been minimized.

Copy link
Contributor Author

odersky commented Nov 2, 2019

Treating protected[this] specially for variance is unfortunately unsound (scala/bug#7093). Nothing changes for private[this]; the [this] is simply inferred.

@neko-kai

This comment has been minimized.

Copy link

neko-kai commented Nov 3, 2019

Sorry, I was not aware. It still feels like a bad blow for cake & PDT; possibly less dramatic if variant aliases are out of the picture anyway... Removal of private[this] is of course strictly an improvement (if someone needs a not-this private for reflective field access, they can use private[MyClass] for the same effect)

@ryanberckmans

This comment has been minimized.

Copy link

ryanberckmans commented Nov 23, 2019

Here is a proposal to retain private[this] in Scala 3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.