Impact of adding private members

[eernstg]

Consider the following program consisting of two libraries:

// --- Library 'n045lib.dart' (version 3).

abstract class A {
  int get _private;
}

void foo(A a) => print(a._private);

// --- Library 'n045.dart'

import 'n045lib.dart';

class B extends A {}

void main() {
  foo(B());
}

This program is accepted by the analyzer as well as the common front end, and it throws at run time because the given instance of B does not have an implementation of _private.

I have argued (e.g., here) that this situation should be subject to a warning (or an error). However, the counterargument has been raised (e.g., here) that it is more important to avoid that the addition of a private declaration causes code in other libraries to fail.

// --- Library 'n045lib.dart', version 1.

abstract class A {}

void foo(A a) {}

// --- Library 'n045lib.dart', version 2.

abstract class A {
  int get _private;
}

void foo(A a) {}

// --- Library 'n045lib.dart', version 3 again.

abstract class A {
  int get _private;
}

void foo(A a) => print(a._private);

Surely, B should not be an error with version 1. According to the "addition of private members is non-breaking" principle, B should not be an error with version 2, either. Surely, then, version 3 cannot make B an error.

So we accept that B does not have an implementation of _private because it would be worse to report an error for B based on such updates of the library that declares A.

However, if we maintain that the addition of a private member should not cause an error to occur in a different library then we should also accept the following program with no compile-time errors:

// --- Library 'n046lib.dart', version 1.

class A {}

abstract class B extends A {
  int _private([int _]);
}

void foo(B b) => print(b._private());

// --- Library 'n046lib.dart', version 2.

class A {
  int _private() => 0;
}

abstract class B extends A {
  int _private([int _]);
}

void foo(B b) => print(b._private());

// --- Library 'n046.dart'.

import 'n046lib.dart';

class C extends B {}
 
void main() {
  foo(C());
}

The analyzer again accepts both versions of the code, but the common front end reports an error for version 2, where we have added the private declaration A._private:

n046.dart:3:7: Error: The implementation of '_private' in the non-abstract class 'C' does not conform to its interface.
class C extends B {}
      ^
n046lib.dart:2:7: Context: The method 'A._private' has fewer positional arguments than those of overridden method 'B._private'.
  int _private() => 0;
      ^
n046lib.dart:6:7: Context: This is the overridden method ('_private').
  int _private([int _]);
      ^

Again, we'd incur a run-time failure because the instance of C does not have an implementation of _private (and the one which is declared in A can not be used because it does not have the required signature). We aren't able to run it, though, because of the error.

We should at least address the discrepancy such that all tools will agree on the existence/non-existence of a compile-time error.

• Option 1: Make CFE stop reporting this error, it is always OK for a class declared in a library L to have no implementation for some private members in a different library L2.
• Option 2: Drop the principle about "adding a private member doesn't break code in other libraries" and report the error in all tools (this would affect both 'n045.dart' and 'n046.dart').

@dart-lang/language-team, WDYT?

[lrhn]

• Option 3: Give C a noSuchMethod-thrower for every inaccessible member of its interface that it doesn't have a valid implementation of.

That's what we do if there is no implementation.

It shouldn't make any difference whether there is an insufficient implementation or no implementation of a member that the C class author should never be aware of at all. They should be able to proceed, and leave it up to the author of the inaccessible members to use them safely.

[eernstg]

(My 'Option 1' includes giving C a noSuchMethod-thrower. I don't we have any other choice.)

[escamoteur]

Why should Dart allow a private member without implementation in an abstract class at all? It can nowhere be implemented.

[FMorschel]

Why should Dart allow a private member without implementation in an abstract class at all? It can nowhere be implemented.

It can be implemented on other classes inside the same library/file.

[eernstg]

@escamoteur wrote:

Why should Dart allow a private member without implementation in an abstract class at all?

Agreeing with @FMorschel, here's a bit of motivation:

We may want to use an abstract declaration of a private member _m for most of the same reasons that we may want to use an abstract declaration for a public one. For instance, this will force all concrete subtypes to have some implementation, which might then be specific and appropriate. If we use a concrete declaration that doesn't do anything then we might forget to override it in some concrete subtype, and it would then spuriously forget to do its job.

abstract class A { int get _m; } // Better than `=> -1;`, where -1 is never a valid result.
class B1 extends A { get _m => 1; }
class B2 extends A { get _m => 2; }

void foo(A a) => print(a._m);

However, this will allow a subclass of A in some other library to become a run-time failure factory: It doesn't have an implementation of _m, and there's no error or warning at class B extends/implements A {} in that other library.

"OK, then A should be final or sealed."

Right, but we need to remember that, and we need to keep track of all subtypes that do not have an implementation of one or more private members (consider abstract class B3 extends A {}) because they would be equally productive run-time failure factories.

"OK, but then we just rename A into _A, and then no accessible class has an unimplemented interface member."

First, we still need to keep an eye on classes like B3.

Next, it could be inconvenient because we might well want A to be used as a type in client code, and even if we don't want to make B1 and B2 private, they can't be used in the same way as A because A is a supertype of both B1 and B2.

All in all, I don't think it's a safe bet that we can just make it an error for a class which is capable of having a subtype in a different library to have one or more private members in their interface that do not have an implementation. I think there will be cases where that kind of situation is exactly what you want and need, and hence we might want to protect ourselves against those unimplemented private members in some other way.

[escamoteur]

don't you think that if Dart gets a real protected access modifier that the need to be able to implement a library private member would no longer be needed?

[eernstg]

That doesn't sound particularly likely...

Library scoped privacy is aligned with the fundamental compilation granularity of Dart. (OK, cyclic imports can create small sets of libraries that always have to be compiled together, and some Dart compilers do whole-program optimization, but otherwise it's not unreasonable to say that we compile one library at a time).

protected is (presumably?) never limited to a scope which is a single library or a single package, it's associated with the subclass relationship. Every class (except Null) is ultimately a subclass of Object, and this means that every subclass chain (that includes any non-system classes) will cross both library and package boundaries.

This means that, as far as I can see, it isn't possible to use protected to obtain compilation stability (as in "I can change this or that, and it won't give rise to a recompilation ... and possibly breakage ... outside ").

I tend to think that library privacy makes sense from one more point of view: We're generally working on libraries, and whenever you can change anything in a given library, you can also change everything in that library. (You may break clients, unless it's guaranteed that they don't rely on the things that you're changing, but you have the physical ability to make the changes).

This means that private properties of a library are "your own ..private.. property" when you are working on a library. That would not be true at all if there could be clients anywhere out there who are depending on your protected members.

[escamoteur]

But you have the same problem with normal members that get overridden by deriving classes outside of a library unless its a sealed class.
Not sure why it is then such a big advantage to have the library as the scope. Especially a the analyzer informs you when some interface has changed.

An additional private modifier that enforces this privacy would add to compilation stability.
And this privacy implements one of the fundamental OO principle of encapsulation.

To me expressing privacy based on the location of a class, meaning in which files they are makes moving classes to different files difficult and it is more a privacy by convention but by clear statements in the class declaration.
A lot of people myself included put classes into separate files for better organisation. Having to maintain part-of statements is tedious.

[lrhn]

(My 'Option 1' includes giving C a noSuchMethod-thrower.)

Well, doh. Then "Option 1" it is!

[lrhn]

"OK, but then we just rename A into _A, and then no accessible class has an unimplemented interface member."

typedef A = _A; 😛

Defining whether a class is accessible is not completely trivial. Probably not impossible, but it's one of those cases where if you forget to dot one i or cross one t, then you have a soundness problem, which may be escalated to a security issue depending on how deep in the compiler the soundness assumption is relied on.
Best way to win is not to play.

[eernstg]

typedef A = _A;

Exactly, that was my point. I just mentioned that A could have a subclass B3 that leaks enough to create the problem, but a type alias will do just fine as well, and there will be even more ways to do it. Quite tricky.

So my proposal for the best way to play is that we lint every class that has a noSuchMethod thrower. Such a lint is guaranteed to cover all cases, and it is not hard to implement (the compiler obviously knows exactly when it is generating a noSuchMethod thrower).

There may be other ways to detect the issue locally. E.g., dart-lang/linter#4975 aims to flag every private member invocation which is potentially going to invoke a noSuchMethod thrower. However, again, that's tricky.

In the meantime, let's return to the topic of this issue: The CFE reports a compile-time error about n046lib.dart during compilation of n046.dart because C (declared in n046.dart) doesn't have an implementation of _private (private to n046lib.dart).

Should the CFE stop reporting that error (and, of course, generate a noSuchMethod thrower instead)? This basically means that the error would go away, and the lint would (optionally) replace it.

[eernstg]

@escamoteur wrote, about different kinds of access control (_..., this private, protected):

But you have the same problem with normal members that get overridden by deriving classes outside of a library unless its a sealed class.

I'm not 100% sure what this problem is the same as. ;-)

Anyway, if we only have protected, and protected implies that code in different libraries (and different packages) can depend on every protected member, then there is no privacy. Certain things are impossible (because it's an error), but no element of your code is private. This essentially means that every change is breaking.

That's the reason why I don't think protected can ever replace library-based privacy. Being private to this is a similar mechanism: It would be possible to use such members in different libraries, different packages. In other words, private to this implies that a certain discipline is enforced, but it does not provide privacy. (Perhaps it should have a different name, like "owned by this", but I don't think it's too bad to call it "private to this".)

An additional private modifier that enforces this privacy would add to compilation stability.

That would be a completely different kind of compilation stability, or at least I can't see how it would ever be possible to avoid recompiling. Let's say that library L contains a declaration that has a member m which is private to this. If we change anything about m then we have to recompile every library that has L as an import, directly or indirectly.

And this privacy implements one of the fundamental OO principle of encapsulation.

I agree on that one! It does constrain the complexity of client code that they can't access this particular member in any way. Every usage of said member must be in the code of the class that declares this member, or in the class of a subclass thereof.

However, it's still possible for the value of a member which is private to this to be reachable in any number of other ways.

dynamic d;

class A {
  final this.list = <int>[];
  A() { d = list; } // OK, we can access `list` here.
}

void main() {
  d.add(24);
}

To me expressing privacy based on the location of a class, meaning in which files they are makes moving classes to different files difficult

Sure, but the point is that as long as class C occurs in library L, every access to every private member that C has must necessarily occur in L. This makes it easier to understand what we're doing with all those private members.

If you move C to some other library then those private accesses are of course broken, and you may have to perform some major refactorings in order to repair the functionality again.

But then again, just don't move that class! ;-) If you insist on getting access to that class from some other library then let that other library export the class from L.

and it is more a privacy by convention but by clear statements in the class declaration.

Privacy in Dart is one of the strictly maintained properties of the language. You can't break the privacy rules.

[leafpetersen]

However, if we maintain that the addition of a private member should not cause an error to occur in a different library

This principle, while a nice thing to aim for as a guideline, is not in any way shape or form universally true in Dart, and I think never has been (even, I think, in Dart 1). Counter-example:

//lib.dart
class A {
  int _x = 3;
}

mixin M {
//  String _x() => "hello";
}

//main.dart
import "lib.dart";

class C extends A with M {}

void main() {
}

This program is fine, but if you uncomment the line in M (that is, add a private method in a library) you will get a new error in a different library.

I think my preference is to do nothing here (that is, ask the analyzer to start reporting this error).

Is there any good reason we allow classes to exist with concrete methods which do not satisfy their interface? That does seem like the other obvious path here: just make B an error. Is there a good reason for a user to want to write that code?

[eernstg]

This principle [..] is not [..] universally true in Dart,

Indeed. We should land this one, finally: #1626. Note that it includes cases that would work if we hadn't decided that we will report an error for mixin applications where "unrelated" concrete members are brought together (for example, they might have exactly the same signature and everything would just work, except that it is an error when they are private).

This is clearly an example of a situation where the addition of a private member with an existing name will introduce an error in a different library.

This particular error arises at a mixin application, and nowhere else.

Perhaps we can just maintain that it is safe to add a member with a fresh private name: If it is not a compile-time error locally in that library L then it is also not a compile-time error in a library that imports L.

Is there any good reason we allow classes to exist with concrete methods which do not satisfy their interface? That does seem like the other obvious path here: just make B an error. Is there a good reason for a user to want to write that code?

Well, that's what I've been saying for years, "just report those errors!". In this case it's an incorrect override. In other cases we've mentioned, the given class does not have an implementation of a private member. The effect is the same: If we try to execute that member at run time it will have to throw. One is not better or worse than the other.

But I keep getting pushback based on arguments like "we cannot introduce compile-time errors in importing libraries because we have changed private declarations in this library". I'd say "Yes, we can!" 😁