Flag concrete classes with unimplemented private members

[eernstg]

Cf. dart-lang/language#1006 (comment).

The static checks on a Dart program do not call for errors in a special case where a member is unimplemented in a concrete class, namely the case where no implementation can be written because the declaration needs to be in one library, but its name is private to another library:

// Library 'lib.dart'.
class A { int _x; }
void foo(A a) => print(a._x);

// Library 'main.dart'.
import 'lib.dart';
class B implements A {}
void main() => foo(B());

This soundness issue is ancient, of course, but it was never made a plain compile-time error to have this situation, presumably because it would break too much existing code.

However, we could allow developers to avoid the issue by using a lint, and then we may be in a better position to make it a proper compile-time error later on.

[bwilkerson]

What is the probability (or our best guess at the probability) that a concrete class missing an implementation of a private method would not be a problem? That is, how likely are false positives for a lint like this? My guess is that it's nearly zero, but there might be cases that I'm not thinking of.

If the probability is low enough, should we consider making this a hint instead? (A hint is effectively a lint that's enabled by default.)

[eernstg]

It would be safe if the private declaration (like A in the example) is in library L and instances of the incomplete class (like B, in library L2) are never passed to code in L.

For instance, if L provides a set of classes (A, A2, .. Ak) then L2 could provide a wholesale replacement (B <: A, B2 <: A2, ...), and then a client might be able to use B... objects exclusively, with some code that expects A... objects. But that wouldn't be easy to do, because all instance creations must be changed to deliver B... objects. So this could probably only be done in a setting with a pervasive use of dependency injection.

I actually think it would be pretty difficult to use those classes with missing private method implementations in a safe manner..

[fzyzcjy]

Hi, is this possible to workaround? This causes problems in delegating/proxy:

// Library 'lib.dart'.
class A { int _x; void publicMethod(); }
void foo(A a) => print(a._x);

// Library 'main.dart'.
import 'lib.dart';
class ProxyOfA implements A {
  final A inner;
  ProxyOfA(this.inner);
  void publicMethod() => inner.publicMethod(); // the proxy will delegate all methods to the inner
}
void main() => foo(B());

Then it throws :(

[fzyzcjy]

IMHO "delegate/proxy" is a very standard design pattern

[eernstg]

Proxy is indeed well-known, but you can't violate privacy in Dart. So if you wish to create a proxy that declares a getter/setter pair for _x which is a name that is private to a library L, then your proxy declaration (or some superclass thereof) must be in L.

This issue is about getting a heads-up from the linter about the situation where a member is unimplemented (and this can't be resolved in the current library because it has a name which is private to another library), and there is no connection to any features that would allow us to violate privacy.

However, you can of course install a "publicizer" using a mixin declared in the library that contains the private name:

// Library 'lib.dart'.
class A {
  int _x = 0;
  void publicMethod() {}
}

mixin APublicizer implements A {
  abstract int public_x;
  int get _x => public_x;
  set _x(int value) => public_x = value;
}

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

// Library 'main.dart'.
import 'lib.dart';

class ProxyOfA with APublicizer implements A {
  final A inner;
  int public_x = 1; // Stand-in for `_x` of 'lib.dart'.
  ProxyOfA(this.inner);
  void publicMethod() =>
      inner.publicMethod(); // the proxy will delegate all methods to the inner
}

void main() => foo(ProxyOfA(A()));

[fzyzcjy]

Thanks! Indeed I want to proxy some Flutter classes in order to have extra behavior, so cannot define a proxy in the same library :/

[eernstg]

Yeah, it's difficult: We want strict mechanisms because they provide firm guarantees. And then I need to break the rules, just this one time. 😄

[fzyzcjy]

Miss the good old days when using Python/Java/Kotlin/... 😄 (But Dart is great!)

[lrhn]

I'd be wary about this lint.

We have so far allowed people to define private members on their public interfaces knowing that they won't interfere with external implementations. Introducing a lint which makes this a problem for the external implementation is putting the onus of handling the issue on the wrong person, the one who can't possibly do anything about it anyway, other than not implement the interface (or add an ignore, which nobody else will see).

We have not said that you should only have private members on public interfaces that are not intended to be implemented.
Therefore we can't expect people to have done so.
(We just added a private member on the Socket class because we knew it wouldn't affect anyone, and as long as we keep track of only using our own implementations internally, it's safe too.)

I'd rather warn at the point where you have a public interface with a private member. That's a lot harder because all classes, including implementation classes, have interfaces. At least it would put the ignore at the point where it matters, where the people potentially calling the member can see that they need to be aware that it might not be implemented.

In the long run, if/when we get sealed types and extend-only classes, it'll make more sense to worry about private members of non-sealed interfaces.

[eernstg]

We have so far allowed people to define private members on their
public interfaces knowing that they won't interfere with external
implementations

It is true that we don't emit any compile-time errors for this situation, but it will cause scenarios like the example in this issue to occur: We have a class A declared in library L with a private member _m and a class B declared in library L2 with implements A, and then we use an instance of B where an A is expected. Then _m is called, and an exception is thrown.

If _m wasn't declared in A yesterday, but it was added today, then this is definitely interfering with our class B. I think it is reasonable to at least have a lint for this, such that this kind of interference doesn't come as a complete surprise.

as long as we keep track of only using our own implementations internally, it's safe too.

One way to ensure that is to only put private members in private and non-leaking classes, and then, in internal code, to test that we indeed have an instance of that private class before any private members are invoked. (This is of course necessary unless we perform the invocation dynamically).

However, even that isn't trivial: If we want to have a private member in a non-leaf class (because it's part of the interface of several different classes, some of which have their own implementation), then we'll end up with something like two separate class hierarchies, one public hierarchy with only public members, and another hierarchy consisting of private classes "hanging off of" the public hierarchy:

class PublicA { void foo() {}}
class PublicB1 implements PublicA {}
class PublicB2 implements PublicA { void foo() {...}}

class _PrivateA implements PublicA { void _foo() {}}
class _PrivateB1 extends _PrivateA implements PublicB1 { void _foo() {...}}
class _PrivateB2 extends _PrivateA implements PublicB2 {}

Each public class would probably have redirecting constructors such that new PublicA(...) actually yields an instance of _PrivateA. The code (in all classes mentioned above) would presumably have many occurrences of type tests where a given PublicX is tested with if (myPublicX is _PrivateX) ....

So neither the author of the private member nor the author of the subtype-that-doesn't-have-some-private-methods will have an easy time dealing with this problem. But allowing it to happen silently is definitely not going to help.

[Levi-Lesches]

@lrhn

We have so far allowed people to define private members on their public interfaces knowing that they won't interfere with external implementations. Introducing a lint which makes this a problem for the external implementation is putting the onus of handling the issue on the wrong person, the one who can't possibly do anything about it anyway, other than not implement the interface (or add an ignore, which nobody else will see).
...
I'd rather warn at the point where you have a public interface with a private member. That's a lot harder because all classes, including implementation classes, have interfaces. At least it would put the ignore at the point where it matters, where the people potentially calling the member can see that they need to be aware that it might not be implemented.

Would dart-lang/language#2918 put the lint/warning/error in the right place? In other words:

class A { int _a = 1; }  // lrhn: hint here
void test(A a) => print(a._a);  // dart-lang/linter#2918: hint here
// ---------------------
class B implements A { }  // this issue: hint here

As discussed, the author of A might want private members for its own internal use, so it's not always correct to hint there. We can hint at B, but if there is no test function, then there's nothing wrong with B. Only the test function is inherently unsafe and should always be hinted.