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: flutter/flutter#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 flutter/flutter#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)
dart-lang/linter#1      A.toString (package:hello_world/a.dart:6:26)

[FMorschel]

Not showing the error but the actual "implementation":

bug.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

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 dart-lang/language#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)
dart-lang/linter#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 .