Lint request: `avoid_private_access_on_open_classes`

[matanlurey]

In Dart, private members cannot be implemented outside of their declaring library, and as such, using implements A where A has a private member that is accessed outside of it's class can easily create an invalid class state; other types assume subtype A will always have a private member that it cannot have by definition.

An example described by @jakemac53:

// a.dart
class A {
  int get _private => 0;
}

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

// b.dart
import 'a.dart';

// OK, no hints/lints/compile issues.
class B implements A {
  // Even if this was commented-back in it would not help.
  // int get _private => 0;
}

I'd like to suggest a lint (that hopefully we add to recommended sets as it matures), which I'm naming avoid_private_access_on_open_classes (but the name is not important to me, only the semantics), it would flag private members accessed outside of their declaring class or sub-classes declared in the declaring library:

class A {
  int get _private => 0;

  @override
  // OK
  String toString() => 'A: $_private';
}

class B extends A {
  @override
  // OK
  String toString() => 'B: $_private';
}

void foo(A a) {
  // LINT: avoid_private_access_on_open_classes
  print(a._private);
}

class C {
  final A a;
  C(this.a);

  @override
  // LINT: avoid_private_access_on_open_classes
  String toString() => 'C: ${a._private}';
}

final class D {
  int get _private => 0;
}

void d(D d) {
  // OK, class is final.
  print(d._private);
}

base class E {
  int get _private => 0;
}

void e(E e) {
  // OK, class is base.
  print(e._private);
}

abstract class F {
  int get _private;

 @override
 // LINT: _private is not implemented, and F is open.
 String toString() => 'F: $_private';
}

The error message/quick-fixes could suggest:

1. Making A final
2. Making A base
3. Making _private public
4. Silencing the lint with an // ignore if the developer knows this type is not truly API public

---

User-land issue: https://github.com/flutter/flutter/issues/148692.

[jakemac53]

Note that even within the same class, it isn't always safe, at least if the member is abstract:

abstract class A {
  int get _x;

  @override
  String toString() => '$_x'; // This should also lint, less clear what the lint should suggest.
}

There are probably other complicated cases to think through as well.

Honestly, I would explore whether triggering the lint on any class with private state is an option. That would be the safest thing to do.

Or, if this is the only case we can come up with, then we should extend the lint to check for it.

[matanlurey]

abstract class A {
  int get _x;

  @override
  String toString() => '$_x'; // This should also lint, less clear what the lint should suggest.
}

Interesting, I guess I don't quite understand this case?

I don't see any potential for runtime exceptions?

Honestly, I would explore whether triggering the lint on any class with private state is an option.

If we can find concrete cases where bad behavior is possible, I'd be supportive - I'd be worried about creating a lint so strict that it invalidates entire codebases that are otherwise running OK.

[jakemac53]

I don't see any potential for runtime exceptions?

You can extend A outside this library afaik, and will get the runtime error on calls to toString().

[jakemac53]

Although I realize now the only safe thing to do would be to require final then?

[srawlins]

It took me a few readings to get the motivation, but I think I get it. The key that I was missing is that this access is OK on final classes, because we know the runtime type (the exhaustive list anyhow). But for an open class, anyone could have claimed to implement it, and you know that they didn't really implement it.

[matanlurey]

Yup you got it!

[FMorschel]

Just to show the importance of this lint, there are probably some other Flutter APIs which are being used this way.

As I had already filed https://github.com/flutter/flutter/issues/148033. There are probably more "controllers" and similar classes that currently have that issue.

For these cases, another solution would be creating a wrapper class or something similar that would implement that private functionality. That class could potentially be private and that would be fine, I think.

[matanlurey]

I don't see any potential for runtime exceptions?

You can extend A outside this library afaik, and will get the runtime error on calls to toString().

@jakemac53 I think that's fine, right? The A#toString() call accesses A#_private, which is fine?

[jakemac53]

@jakemac53 I think that's fine, right? The A#toString() call accesses A#_private, which is fine?

It isn't fine, because A is abstract and has no implementation of _private, but B is still allowed to extend it from outside the library, so you end up with a runtime stack trace in A#toString():

Unhandled exception:
NoSuchMethodError: Class 'B' has no instance getter '_private'.
Receiver: Instance of 'B'
Tried calling: _private
#0      B._private (package:hello_world/b.dart:8:7)
#1      A.toString (package:hello_world/a.dart:6:26)

[FMorschel]

Not showing the error but the actual "implementation":

bug.dart

```dart import 'other.dart'; void main() { final a = A(); final b = B(); final c = C(); print(a.toString()); print(b.toString()); print(c.toString()); } class A { int get _private => 0; @override String toString() => '$runtimeType: $_private'; } ```

other.dart

```dart import 'bug.dart'; class B implements A {} class C extends A {} ```

Output:

A: 0
Instance of 'B'
C: 0

Exited.

Edit

This is exactly why I'm waiting for https://github.com/dart-lang/language/issues/3748.

[matanlurey]

@jakemac53 I think that's fine, right? The A#toString() call accesses A#_private, which is fine?

It isn't fine, because A is abstract and has no implementation of _private, but B is still allowed to extend it from outside the library, so you end up with a runtime stack trace in A#toString():

Unhandled exception:
NoSuchMethodError: Class 'B' has no instance getter '_private'.
Receiver: Instance of 'B'
Tried calling: _private
#0      B._private (package:hello_world/b.dart:8:7)
#1      A.toString (package:hello_world/a.dart:6:26)

Ohhhhhh, ok sorry I was stuck/rat-holed on my original example. You mean:

// This is an impossible case, other classes outside of this class can't implement it.
abstract class A {
  int get _private;
}

I wonder if this should be a second lint, or this lint should be generalized as avoid_errenous_private_on_open_classes (better names appreciated).

+1, we should also cover that use case; added in the OP as abstract class F.

[eernstg]

@matanlurey, this is great! I'd love to see improvements in the static support for detecting and avoiding these privacy related run-time failures.

However, it isn't quite sufficient to do the things you mentioned.

@jakemac53 already mentioned that an abstract declaration isn't safe. Here are some other things to be aware of:

Safe call sites

We have two invocations marked 'OK'. Consider the following variant:

// --- Library 'n044lib.dart'.

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

  @override
  String toString() => 'A: $_private()';
}

abstract class B extends A {
  @override
  // OK
  String toString() => 'B: $_private()';
}

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

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

void useIt1(Danger1 danger1) => danger1._private(0);
void useIt2(Danger2 danger2) => danger2._private(0);

void main() {} // Allow the CFE to process this library.

This library is accepted by the analyzer as well as the CFE. It may be used as follows:

import 'n044lib.dart';

class Danger1Sub extends Danger1 {}
class Danger2Sub extends Danger2 {}

void main() {
  try {
    Danger1Sub().toString(); // Would throw in `A.toString`.
  } catch (_) {
    print('Caught 1');
  }
  try {
    Danger2Sub().toString(); // Would throw in `B.toString`.
  } catch (_) {
    print('Caught 2');
  }
}

This program is also accepted by the analyzer.

The CFE rejects the program with a compile-time error reporting that the methods private to 'n044lib.dart' are erroneous. (I created https://github.com/dart-lang/language/issues/3826 to report the discrepancy between the analyzer and the CFE, and to discuss the situation more broadly because it amounts to a violation of a principle which has been held up as desirable.)

n044.dart:3:7: Error: The implementation of '_private' in the non-abstract class 'Danger1Sub' does not conform to its interface.
class Danger1Sub extends Danger1 {}
      ^^^^^^^^^^
n044lib.dart:2:7: Context: The method 'A._private' has fewer positional arguments than those of overridden method 'Danger1._private'.
  int _private() => 0;
      ^
n044lib.dart:15:7: Context: This is the overridden method ('_private').
  int _private([int _]);
      ^
n044.dart:4:7: Error: The implementation of '_private' in the non-abstract class 'Danger2Sub' does not conform to its interface.
class Danger2Sub extends Danger2 {}
      ^^^^^^^^^^
n044lib.dart:2:7: Context: The method 'A._private' has fewer positional arguments than those of overridden method 'Danger2._private'.
  int _private() => 0;
      ^
n044lib.dart:19:7: Context: This is the overridden method ('_private').
  int _private([int _]);
      ^

So if we maintain that the correct behavior is to not report an error when a class in a library L does not have an implementation of some members that are private to a different library L2, then the above example would demonstrate that the call sites in A.toString and B.toString aren't safe after all.

Make A final

Again, the analyzer accepts the following:

// --- Library 'n047lib.dart'.

final class A {
  num _private() => 0;

  @override
  String toString() => 'A: $_private()';
}

abstract base class B extends A {
  int _private();
}

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

// --- Library 'n047.dart'.
import 'n047lib.dart';

base class C extends B {}

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

The CFE reports an error (like before, it reports that there is no implementation of _private), and it can again be argued that it should not report this error. This shows that the invocation in A.toString as well as the invocation in foo are unsafe.

Make A base

This case is subsumed by the previous one, because the constraints introduced by making a class base is a subset of the constraints for final. Hence, making A base is going to have at least as many loopholes as making a final.

Make _private public

This would work. It might be inconvenient in many situations for other reasons, but it would definitely eliminate the run-time failures that are specific to private members.

So what?

The analyzer follows the principle that no errors/warnings are reported when a concrete class does not have an implementation of one or more private members from another library. With that, it's actually quite tricky to make these examples truly safe. A single keyword like final or base won't do it, we'd have to perform some other investigations of the entire library that declares those private members.

If we do change the CFE such that it behaves like the analyzer then the run-time failures are real. We can run the examples shown above, they will pass muster during static analysis, and they'll throw at run time because some private member does not have an implementation.

So we're safe for now, but only because the CFE is reporting some compile-time errors that it shouldn't (at least according to some people ;-).

It is probably not an option to keep both the analyzer and the CFE unchanged because they disagree on whether or not there is a compile-time error.

The core question is the following: Is it possible to flag every unsafe private member invocation, with a reasonably small number of false positives? Or perhaps it's enough to flag "most" unsafe private member invocations, because this is better than nothing? (We don't have to insist on soundness here, we have a history of allowing this particular kind of no-such-method error to occur in programs that do not elicit any diagnostics.)

We would need a notion of a "class/mixin/... that doesn't leak as a supertype", which means that no subtype can be created in a different library. We would also consider at least one additional kind of type as safe:

• Include each class or mixin which is private, whose non-bottom subtypes are all private (necessarily to the same library), and where none of the types are made available to other libraries as type aliases. They are non-leaking.
• Also include each class which is final or sealed and whose non-bottom subtypes are all final or sealed as well (these types can be any mixture of private and public). They are non-leaking as supertypes.
• Finally, include every class/mixin which is base and where every leaking subtype has an implementation of every private member. These types are leaking as supertypes, but they are still safe because all private members of a concrete class will be implemented, even concrete classes that are created in other libraries.

Invocations of a private member on a receiver with one of these kinds of types would be safe.

[bwilkerson]

Is it possible to flag every unsafe private member invocation, with a reasonably small number of false positives?

I would postulate that compile-time errors should have zero false positives. It's fine if the language forbids a construct that could be given a runtime semantic, but if the construct is allowed by the language then we can't report it as an error.

Warnings need to be similarly free of false positives, though we do currently have a few violations of the policy.

We could, however, create a lint for this if we think we'd enable it in the core lint set (and it doesn't require whole-program analysis). But for a lint we don't need to flag every violation in order for it to be reasonable, so there might be a tradeoff between completeness and the number of false positives.

[eernstg]

I would postulate that compile-time errors should have zero false positives ... if the construct is allowed by the language then we can't report it as an error.

I thought lints would freely report situations that aren't compile-time errors, that's basically all they can do?

In any case, we can under-approximate and over-approximate (obtaining a guarantee that no non-problems are reported, perhaps failing to detect some actual problems, respectively obtaining a guarantee that no problems remain unreported, perhaps reporting some situations that aren't actual problems). We just need to choose an approach which is in line with existing policies.

[bwilkerson]

if the construct is allowed by the language then we can't report it as an error.

I thought lints would freely report situations that aren't compile-time errors, that's basically all they can do?

Sorry for the confusion. What I meant by "error" is "compile-time error". I try to always use "error" that way, to use "warning" to mean a non-error that is enabled by default, and "lint" to mean a non-error that is disabled by default. I use "diagnostic" to mean any of the above.

Somehow I'd gotten the impression that you were proposing that this be an error. I'm not sure how, and you clearly labeled the issue as a "Lint request", so I should have realized that that's not what you had in mind.

We just need to choose an approach which is in line with existing policies.

It's a bit fuzzy, but we don't allow false positives on lints unless it's not possible to avoid false positives without introducing false negatives AND the risks to a user of having false negatives is high enough to justify the annoyance of false positives. The latter is, of course, purely a judgement call, and not one we always get right.

[eernstg]

What I meant by "error" ... "warning" ... "lint" ...

Very good, we agree completely on that! (Except that I've probably used "warning" less precisely now and then.)

I used the word 'error' a number of times because I was reporting that the tools currently emit some compile-time errors in situations relevant to this issue (especially the CFE).

If it is decided that the CFE should stop reporting these errors (in particular, in response to "this class does not have an implementation of a member which is private to a different library") then I'd very much support the introduction of a lint for the same thing. After all, if my app just crashed with a noSuchMethod exception then I am not going to be a huge lot more happy even if they tell me "but the non-existing thing was a private method!" ;-)

The lint proposed in this issue is somewhat different: It attempts to make a distinction between safe and unsafe invocations of private members. That's a tricky exercise (e.g., because it involves precise knowledge about which classes are "leaking" to other libraries), but it might be possible.