Disallow private members from being accessed without `this`

[Levi-Lesches]

The main problem, illustrated by dart-lang/sdk#57710, is that this code results in a runtime error and not a compiler error

// a.dart
class A { int _a = 1; }
void test(A a) => print(a._a);  // <-- relies on [A] having a private member [_a]

// b.dart
import "a.dart";
class B implements A { } 
void main() => test(B());  // Compiles with runtime error

This error occurs because declaring test in the same library as A allows us to access A._a, but doesn't guarantee that all As will have a member named _a -- for example, B does not. dart-lang/sdk#57710 forces B to provide its own (ie, override) _a, but I have a different solution: disallow this entirely. Another example:

// a.dart
class Value { 
  int _value = 1;
  void _print() => print(_a);  // (1)
  int add(Value other) => this._value + other._value;  // (2) and (3)
}

int addValues(Value a, Value b) => a._value + b._value;  // (3)

// b.dart
import "a.dart";
class Value2 implements Value {
  @override
  int add(Value other) => -1;  // some custom override not involving [_value]
} 

void main() {
  Value1().add(Value2());  // Error: Value2 does not have member `_value`
  Value2().add(Value1());  // Ok
  addValues(Value1(), Value2());  // Error: Value2 does not have member `_value`
}

In total, there are three few ways to access a private member:

1. In a public method, using this: is always safe since, if the class is implemented, the method will be reimplemented
2. In a private method, using this: is always safe since it is impossible to call the method if the class is implemented
3. From any other location, or when not using this: is unsafe since the object may be an implements subclass

It's a breaking change, but disallowing option 3 eliminates runtime errors in exchange for safer code. The root of the issue is that test is separated from A but depends on an intimate knowledge of A and should thus be a part of it. The above example can be rewritten as:

// a.dart
class A { 
  int _a = 1;
  void test() => print(_a); 
}

// b.dart
class B implements A {
  /// we must implement [test] such that it doesn't depend on [A._a]
  @override
  void test() => print(1);
}
void main() => B().test();

Overall, if either dart-lang/sdk#57710 or this proposal is accepted, it will reduce the difference between implementing and extending (see dart-lang/sdk#57805), which can also simplify the Class Modifiers proposal.

I think this is a lot more breaking than dart-lang/sdk#57710 though, and so it's probably fine if this one is dropped in favor of that. In general, dart-lang/sdk#57710 favors a less breaking approach while this issue favors a design that more cleanly establishes what is and isn't private.

[TimWhiting]

If A is declared like this and the test method is public, then the author of A should probably put the 'base' modifier on the class requiring extension and disallowing implementing. This situation could be an error, (when a public member of a library accesses a private member of a class that can be implemented).

[Levi-Lesches]

You're right that disallowing implementation will avoid this bug, but the point of this issue is to show how the current behavior is broken, with the hope that fixing it will bring implements closer to extends. In other words, if this buggy behavior is no longer possible, there won't be as much of a difference between base and interface in the class modifiers proposal.

But anyway, with the current behavior in mind,

If A is declared like this and the test method is public, then the author of A should probably put the 'base' modifier on the class requiring extension and disallowing implementing.

The point is that if test is a public method of A, not a random function elsewhere, then implementing would be okay, since B would have to override A.test with an implementation that makes sense, and doesn't rely on A._a. That's why defining test outside of A is so bad -- it relies on information about an instance of A it can't guarantee.

[TimWhiting]

Ahh, you are right, this isn't a problem with methods, so its a bad interaction between library based privacy and class implementation.

[Levi-Lesches]

As @lrhn pointed out, in the following example, it is not enough to allow _value because it occurs in its declaring class. We must prevent other._value as well:

// a.dart
class Optimist {
  final int _value;
  Optimist(this.value);
  // This private member is referenced inside its declaring class, but can still cause an error
  Optimist operator+(Optimist other) => Optimist(_value, other._value); 
}

// b.dart
class Pessimist implements Optimist { }
void main() => Optimist() + Pessimist();  // error

So I added a new explanation to the top comment and I think we should eliminate option 3.

---

Overall, the core of the problem is that in the following code, A and B have members that C do not:

class A { } 
// --------
class B extends A { }
class C implements A { } 

and while there are pros and cons to extends vs implements, most devs operate under the assumption that implements lets them override everything, but in reality it actually hides members that might cause runtime errors later.

[lrhn]

What you are defining here is non-interface methods.

Rather than conflating it with privacy, consider it a separate kind of declarations (which may be distinguished using separate name syntax or by using modifiers at the declaration).

An abstract member declaration introduces a member into the declaration's interface without adding an implementation to the class.
This would be the opposite, a declaration which introduces a member to the class, but does not add it to the interface.

So, as a strawman, let's say such declarations are introduced using names starting with two underscores.
A non-interface member cannot be invoked on a variable only typed by the interface. That means it can only be invoked on this or super, because all other instances are only known by their interface.
(I guess invoking a generative constructor can safely be assumed to be an instance of that class, but that may not be useful.)

Then you cannot write:

class Optimist {
  final int __value;
  Optimist(int value) : __value = value;
  Optimist operator+(Optimist other) => this.__value + other.__value; // INVALID
}

because other.__value tries to do a typed access to __value on the interface Optimist which doesn't have a __value member.

We can say that dynamic invocation, (other as dynamic).__value, doesn't work if the target is a non-interface method. Then it really can only be invoked using this or super.

Using private naming isn't necessarily the best choice.
If we used a modifier instead, then we can have non-library-private non-interface members.
Let's use protected 😉.

base class StringBuilder {
  protected final List<String> chunks = [];
  void add(String string) {
    chunks.add(string);
  } 
  String toString() {
    if (chunks.isEmpty) return "";
    if (chunks.length == 1) return chunks.first;
    var result = chunks.join();
    chunks..clear()..add(result);
    return result;
  }
}
// Maybe in a different library
class StringWriter extends StringBuilder {
  void addAll(Iterable<String> strings, [String separator = ""]) {
    if (separator.isEmpty) { 
      super.chunks.addAll(strings);  // Can access `chunks` on this/super.
    } else {
      super.chunks.addSeparated(strings, separator); // Some extension method.
    }
  }
}

void main() {
  var writer = StringWriter(); 
  if (writer.chunks.isEmpty) { // INVALID
    // ...
  }
}

So, a different concept, more general than just avoiding unsafe private member access.
A protected private-named member would then be both subclass only and library only.

(If this was possible, I'd probably start making all my private members protected too. But you don't have to,
if you want a number of different private classes with no shared superclass in the same library to share the same private interface name, you can.)

[Levi-Lesches]

Let's use protected 😉.

I fully agree that what I'm describing here is more in line with protected than library-private, but that's part of the point! This issue is not a proposal for a new feature, it's a request to make an existing feature safer. The way code private members are currently implemented allows for very unsafe behavior and is currently a motivating factor behind making the class-modifiers feature more complicated. At the very basic level, the code samples at the top of this issue should not compile.

Rewriting the existing behavior to be more like protected while keeping it scoped to the library level is a compromise to minimize breakage while locking down unsafe code. In other words, it won't allow code at the top of this issue to compile while also not breaking people's conceptual understanding of private members as library-scoped. If we can make protected its own feature to eventually replace library privacy, that will be great too. Not to mention, it'll get rid of the @visibleForOverriding and @protected annotations from package:meta.

[lrhn]

Library privacy solves multiple problems.

The primary problem is having "private" names that cannot conflict with names from other libraries.
It's not so much that others cannot access the members with private name, as much as that the there cannot be a conflict with that name either.

Going for protected instead will not solve that.
If you have variables that others cannot access, and you can only access through this, is still a problem if a subclass cannot declare a member with the same name. Library private names solve that.

They just don't work as well with implements from other libraries. But then, nothing does, not protected either, not without base forcing you to inherit the implementation.

[munificent]

In general, yes, it's nice if we can use static analysis to completely eliminate a class of runtime failures. At the same time, static analysis does have a cost:

1. It makes the language more complex for users to learn and makes code more complex to read and understand.
2. Users have to spent time deciding how to use the static analysis feature as they write their code.
3. It takes time to design and implement, which could be spent on other features.
4. Since static analysis is conservative, it often prohibits some programs that would not fail at runtime because it's unable to prove that it won't fail.

Those mean that it's not necessarily to the right call to make something a compile time error. Null safety is a good example of this trade-off: Even before safety, users were generally able to write robust Dart code that didn't fail with null reference errors at runtime. When they moved to null safety, they were forced to start thinking about where to use ? and late. And some programs that are dynamically fine have compile errors that necessitate them using ! or copying values to local variables and other patterns.

With null safety, we believe that null reference errors are a large enough source of user pain that it's worth the cost. And we also believe that the annotations on which variables are nullable helps users better reason about the invariants in their code.

With this issue, Dart has always had this bug where a library can receive an instance of one of its own types and try to access a private member on it. That access will fail if the object is actually an instance of a class outside of the library implementing the library's interface. But, in practice, I almost never hear about users running into bugs because of this. On the other hand, it's very useful and convenient to be able to access private members on instances other than this. Implementing equality is the classic example going all the way back to Smalltalk, but it comes up frequently.

I think class modifiers will solve the problem for the (likely small number of) users that care about this particular invariant. But I don't think it would be a net benefit if we made a more sweeping change to the language to completely eliminate it. The runtime errors are just not a big source of pain, and the constraints you would have to fit within to eliminate them are.

[eernstg]

One requirement which has been noted for the base mechanism is that private members must be implemented. The same is true for the proposal in this issue:

// a.dart

abstract class A {
  abstract int _a;
  void test() => print(this._a); // Not safe.
}

import 'a.dart';

class B extends A {}
void main() => B().test();  // Compiles, but throws at run time

The example above is just the most blatant one. There are various ways to end up in a situation where a given private member like A._a is not implemented in a given subtype D of A declared in the same library, and we must then know for sure that it is impossible to create a subclass of D in another library.

Of course, dart-lang/sdk#58179 will flag class B (and every other class which will cause this problem), but in order to ensure that there will not be any such class it is necessary to enforce something about private members being implemented (there are probably a bunch of different ways to do this).