language icon indicating copy to clipboard operation
language copied to clipboard

Do implementers have to implement private members?

Open wytesk133 opened this issue 5 years ago • 10 comments

On DartPad, the following code does not compile

class A {
  int thisIsPublic;
  int _thisIsPrivate;
}

class B implements A {
  @override
  int thisIsPublic;
}

The error is The non-abstract class 'B' is missing implementations for these members: - A._thisIsPrivate.

Do implementers have to implement private members?

wytesk133 avatar Jun 04 '20 10:06 wytesk133

The dart docs says so yes:

// A person. The implicit interface contains greet().
class Person {
  // In the interface, but visible only in this library.
  final _name;

(https://dart.dev/guides/language/language-tour#implicit-interfaces)

Note "visible only in this library", is you do class B implements A in another library (or even just another file) you don't have to implement the private members.

renefloor avatar Jun 04 '20 11:06 renefloor

Thanks. Those details are very important to keep in mind

pedromassango avatar Jun 04 '20 11:06 pedromassango

This approach doesn't seem safe to me. Consider:

library a;

import 'b.dart';

void main() {
  B()._thisIsPrivate; // NoSuchMethodError: Class 'B' has no instance getter '_thisIsPrivate'.
}

abstract class A {
  int thisIsPublic;
  int _thisIsPrivate;
}
library b;

import 'a.dart';

class B implements A {
  @override
  int thisIsPublic;
}

Now is too late to change this, but just for discussion matters, why not demanding implementations from different libraries to implement private members? Of course it would prevent implementations of classes with private members, but isn't safety more important?

ghost avatar Jun 04 '20 14:06 ghost

@wytesk133 wrote:

Do implementers have to implement private members?

Yes, the basic rule is that it is an error if a concrete subtype does not have an implementation of a member, private or public.

However, Dart used to be a much more dynamically checked language than it is today, and the treatment of missing private members across different libraries is a corner of the language where sound static checking has not (yet) been introduced, so there is indeed a potential for an error at run time which is not flagged at compile-time, as shown in @hugocbpassos' example here.

It wouldn't be difficult at all to flag this error at compile-time, but it is a breaking change, which is the main reason why it hasn't been done.

It would actually be a very good candidate for a lint, cf. https://github.com/dart-lang/sdk/issues/58179.

eernstg avatar Jun 04 '20 14:06 eernstg

@lrhn pointed out in dart-lang/sdk#57805:

class Foo {  // using today's syntax, so no "open" yet
  final String _foo;
  Foo(String foo) : _foo = foo;
  int get hashCode => _foo.hashCode;
  bool operator==(Object other) => other is Foo && _foo == other._foo;
}
class MyFoo implements Foo { }

void main() {
  Foo foo = Foo("h");
  Foo foo2 = Foo("h");
  MyFoo myFoo1 = MyFoo();
  MyFoo myFoo2 = MyFoo();

  print(foo1 == foo2);  // true
  print(myFoo1 == myFoo2);  // false
  print(myFoo1 == foo1);  // false
  /// Unhandled Exception. NoSuchMethodError: `MyFoo` has no instance getter `_foo`
  print(foo1 == myFoo1); 
}

I'm curious how has this not been a bigger issue yet? I'd definitely be supportive of at least a lint to catch this.

Levi-Lesches avatar Jul 04 '21 03:07 Levi-Lesches

The behavior has been consistent since the dawn of Dart (at last since the removal of interface declarations), and I am genuinely impressed that it hasn't been a real issue in practice.

What likely happens is that classes are either intended to be implemented or not. If they are, the author won't write the unsafe == operator above. If they are not, if anyone tries anyway, and things fail, they'll just silently stop trying. The users will learn that "FooBar must be extended, otherwise it doesn't work". I know we have a few such classes in the dart:io library, likely around sockets, and I probably some in the main platform libraries too, like Symbol and Type. You can implement the interface, it just won't work with any of the code expecting platform sockets, symbols or types. So people learn that and just don't implement them.

lrhn avatar Jul 05 '21 08:07 lrhn

This issue can also be fixed by disallowing private members to be accessed outside of the class that declares them (#2918), but this looks like a much less breaking change.

Levi-Lesches avatar Mar 15 '23 19:03 Levi-Lesches

That won't fix the problem for:

class Optimist {
  final int _value;
  Optimist(this.value);
  Optimist operator+(Optimist other) => Optimist(_value, other._value);
}

The other._value is the risky one, but it occurs smack in the middle of the declaring class.

The issue is not the location, the issue is that Optimist is used as an interface in other._value, but as a self-access in _value (aka. this._value)

lrhn avatar Mar 15 '23 19:03 lrhn

Wow, that was fast. Guess this issue is still needed then since it would force any implementer of Pessimist to override _value.

What if that proposal did restrict access to anything other than a self-access (this._private)? EDIT: I added that into the proposal.

Levi-Lesches avatar Mar 15 '23 23:03 Levi-Lesches

The core of the issue here is that in the following code, B is an acceptable replacement for A, but C is not.

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

The reason for this is that A and B might have some private members that C does not, meaning they don't truly share the same interface and are thus not truly interchangeable. No one is debating that the author of this file doesn't make a direct choice when choosing to extend or implement, but I wouldn't agree that they're choosing to make C a "lesser" version of A. In other words, I don't think the author of B and C is deliberately choosing this behavior:

void test(A a) { /* Use some private members of A */ }
void main() {
  test(A());  // OK
  test(B());  // OK
  test(C());  // Compiles, but can lead to runtime errors
}

If either this proposal or dart-lang/linter#2918 is accepted, then there will no longer be any difference. It will break some (inherently unsafe) code, but will ultimately lead to safer usage.

Levi-Lesches avatar Mar 16 '23 02:03 Levi-Lesches