[Request] Warn when using private members of the same interface but a different instance

[jellynoone]

Consider the following example:

/// lib1.dart
class Addable {
  final int _value;

  Addable(this._value);

  operator +(Addable addable) {
    return Addable(_value + addable._value);
  }
}

/// lib2.dart
import 'lib1.dart';

class DecoratedAddable implements Addable {
  final Addable _decorated;

  DecoratedAddable(this._decorated);

  @override
  operator +(Addable addable) {
    print('Adding');
    return _decorated + addable;
  }
}

/// main.dart
import 'lib1.dart';
import 'lib2.dart';

void main() {
  Addable(1) + DecoratedAddable(Addable(2));
}

Running dart run main.dart issues the expected error:

Unhandled exception:
NoSuchMethodError: Class 'DecoratedAddable' has no instance getter '_value'.
Receiver: Instance of 'DecoratedAddable'
Tried calling: _value
#0      Object.noSuchMethod (dart:core-patch/object_patch.dart:38:5)
dart-lang/sdk#57147      DecoratedAddable._value (lib1.dart:2:13)
dart-lang/sdk#57148      Addable.+ (lib1.dart:7:37)
dart-lang/sdk#57149      main (main.dart:5:14)
dart-lang/sdk#57150      _delayEntrypointInvocation.<anonymous closure> (dart:isolate-patch/isolate_patch.dart:297:19)
dart-lang/sdk#57151      _RawReceivePortImpl._handleMessage (dart:isolate-patch/isolate_patch.dart:192:12)

My desire is if the analyzer could warn whenever you are using a private member of a different instance of a class which can be implemented by a foreign library. In this case, if a warning could be issue in Addable when performing: Addable(_value + addable._value); that addable._value is unsafe since the passed in Addable may be a different implementation.

Of course if Addable was private this shouldn't warn since we can be certain all the fields will be implemented.

This issue has been on my mind for quite some time since it requires you to be constantly focused on what you're doing with your classes.

To illustrate this point further. Here is at least one example of this issue in the sdk, in the Duration class:

sdk/sdk/lib/core/duration.dart

Lines 193 to 201 in edb7fb4

	Duration operator +(Duration other) {
	return Duration._microseconds(_duration + other._duration);
	}
	/// Subtracts [other] from this Duration and
	/// returns the difference as a new Duration object.
	Duration operator -(Duration other) {
	return Duration._microseconds(_duration - other._duration);
	}

and some more unsafe usages at other operators:

sdk/sdk/lib/core/duration.dart

Lines 223 to 233 in edb7fb4

	/// Whether this [Duration] is shorter than [other].
	bool operator <(Duration other) => this._duration < other._duration;
	/// Whether this [Duration] is longer than [other].
	bool operator >(Duration other) => this._duration > other._duration;
	/// Whether this [Duration] is shorter than or equal to [other].
	bool operator <=(Duration other) => this._duration <= other._duration;
	/// Whether this [Duration] is longer than or equal to [other].
	bool operator >=(Duration other) => this._duration >= other._duration;

While the fix is trivial here, as is, if a client wanted to, for some reason, decorate the Duration class, and through implements rather than extend, these methods would fail.

Another way to safe-guard against this issue, would be either that no public class that defines a private field / method can be implemented by a different library only extended. I assume this would require some specification changes.

[lrhn]

To me, that sounds more like a lint than a warning, because there are risks of false positives.

The Duration examples are known, and we should just fix them (use .inMicroseconds instead)

[bwilkerson]

... warn whenever you are using a private member of a different instance of a class which can be implemented by a foreign library.

I suspect that this approach would result in a lot of false positives, because I think there are plenty of times when it's valid to write a class (with private members) that you don't intend other users to implement.

... no public class that defines a private field / method can be implemented by a different library only extended.

But this approach, with a bit of refinement, would be less prone to false positives. I think it would also put the onus on the person attempting to implement a class rather than the person writing the class, which seems better to me.

The one refinement that comes to mind is that it's only a problem if the private members of the class being implemented are being accessed on some object other than this. There might be other refinements that I haven't thought of yet that would also reduce the number of false positives.

[lrhn]

... no public class that defines a private field / method can be implemented by a different library only extended.

But this approach, with a bit of refinement, would be less prone to false positives. I think it would also put the onus on the
person attempting to implement a class rather than the person writing the class, which seems better to me.

The problem here is that having a library private member should be invisible outside of that library. It's not part of the DartDoc and not discoverable, and you are free to change it between versions with nobody being able to tell the difference.

Also, it would introduce a class without any implementable interface. That'd be great, but we may want to introduce that feature a little more formally, as something you can request, and not just as a side-effect of having private instance members.

The current behavior is permissive. Not surprising, since it has been with us since Dart 0.
Any restriction we add is going to prohibit some existing use-case, unless we make it so complicated it's not going to be explainable.

Basically: It's OK to call private members on objects if we know that the run-time instance of the object has the member.
We can know that in a number of ways:

• Call on a known class (this and super invocations in class declarations, this invocations on mixins if declared in the mixin).
• Interface invocations if the interface doesn't leak from this library (so no other-library implementation is possible).
• Interface invocations if the instances are not parameters of public methods of escaping objects. (We created the objects, and we know it.)

There is no good solution because a super-class having a member that a sub-class don't is completely contrary to object oriented design. We could make them all non-virtual, but we do rely on overriding private members in private subclasses.

[eernstg]

I think the constraint that a given member should only be accessed on this should be considered separately. I created #58722 to propose support for that kind of constraint.

[jellynoone]

After posting this issue I've search around the language, linter and the sdk repositories to realise, my request here is more or less trying to solve the inability to explicitly forbid implementing a class which #46331, linter-3353, and language-704 propose to solve.

And those would probably be a less false positive way of dealing with the issue described here.