Maybe warn about unsafe private member user based on class modifiers.

[lrhn]

If code uses a private member on a non-this object that is supplied through a public API, like:

class MyBox<T> {
  final T _value;
  MyBox(T value) : _value = value;
  bool operator==(Object other) => other is MyBox && _value == other._value;
}

where the other._value assumes that the incoming MyBox object has this MyBox class in its superclass chain, then it breaks if someone, somewhere (in a different library), does implements MyBox. That other class will automatically get a throwing implementation of get _value.

With class modifiers, we may want to give a warning (directly or by a lint) about this design, because there is now a way to show intent about whether it's OK to implement the class.

If a library

• Accesses a private member as an interface member access, o._foo (not as this._foo, or super._foo outside of mixins),
• Any declaration containing (declaring or inheriting) _foo is public,
• and that declaration doesn't prevent implementation (subtypes or is itself a final or base declarations)

then the class leaves itself open to this kind of failure.

The workaround is either to declare the class base, or to rewrite the method (and maybe API, if necessary) to not depend on directly accessing private members.

Should probably be a lint. There is no real risk to others if you only do this inside internal code.

[eernstg]

💯 😄

[keertip]

/cc @scheglov @bwilkerson

[bwilkerson]

I'm not certain that I understand the problem. If a class C implements MyBox, then C is required to define (or inherit) a concrete implementation of every public member of MyBox. In the example above, that's ==.

I'm not seeing how the defined or inherited implementation of == could reference MyBox._value without also having access to the field.

[scheglov]

A subtype of MyBox will inherit implementation of == from Object. Although I don't think that the issue is the ==.

The warning looks reasonable to me. I don't know how many pre-existing violations we will find though.

I don't understand the motivation description for it to be a lint: "There is no real risk to others if you only do this inside internal code." What do you mean under "internal code"? Google3?

[bwilkerson]

A subtype of MyBox will inherit implementation of == from Object.

I understand that. But the implementation from Object won't try to invoke _value, and it's an attempt to invoke _value in a class that has no getter named _value that causes the code to break. So where's the problem? (Sorry if I'm being dense.)

[scheglov]

This program crashes:

import 'test4.dart';

class MyBox2 implements MyBox {
  int _value = 0;
}

void main() {
  MyBox(0) == MyBox2();
}

class MyBox {
  final int _value;
  MyBox(this._value);
  bool operator ==(Object other) => other is MyBox && _value == other._value;
}

Because MyBox.== can only access MyBox._value, but not MyBox2._value. And MyBox2._value does not implement MyBox._value. It does not have to, but it also cannot.

Unhandled exception:
NoSuchMethodError: Class 'MyBox2' has no instance getter '_value'.
Receiver: Instance of 'MyBox2'
Tried calling: _value
#0      MyBox2._value (file:///Users/scheglov/dart/test/bin/test.dart:3:7)
#1      MyBox.== (file:///Users/scheglov/dart/test/bin/test4.dart:4:71)
#2      main (file:///Users/scheglov/dart/test/bin/test.dart:8:12)

[eernstg]

[Edit: OK, this point was already made in the comment above, just as I was writing this one. ;-]

The problem is that other._value may not exist (we don't even need a type argument, so I took it out):

// Library 'a.dart'.

class MyBox {
  final int _value;
  MyBox(this._value);
  bool operator ==(Object other) => other is MyBox && _value == other._value;
}

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

class ForeignBox implements MyBox {} // No compile-time error.

void main() {
  MyBox(1) == ForeignBox(); // Throws.
}

[bwilkerson]

Ok, thanks. I don't know why I didn't see that before.

I don't understand the motivation description for it to be a lint: "There is no real risk to others if you only do this inside internal code." What do you mean under "internal code"? Google3?

I won't try to guess at someone else's meaning, but my argument for making it a lint is that it isn't an error until a subclass is created, so in cases where you don't expect a subclass to be created it would be a false positive. We could limit the diagnostic to only being produced in non-src libraries in a publishable package, but that feels like too many restrictions. I'd rather just make it a lint.

But that does raise the question of whether we might also want a warning on the use-side of the relationship (for example, if we ever see such a subclass being defined, or an instance of such a class being passed in to a method that references a private member). It's good to warn the author of the superclass that the superclass could be misused, but if they ignore the warning it might be good to warn the consumer of the superclass that it isn't safe to do so.

[eernstg]

we might also want a warning on the use-side

Certainly! Whenever a class declaration C has one or more implicitly generated throwing stubs for private methods of another library L, it makes sense to report that situation.

It may be possible to use an instance of C safely, but it's not obvious at all how to do that (basically, that object must never be the value of any expression in L).

So it's basically guaranteed that there are no false positives.

However, it would be nice if the community as a whole could get a habit of always using base or final on every class which is accessible from other libraries and contains private members. If that's almost always true then we will almost never have a case where those throwing stubs are needed, and this handles the problem at the root.

[lrhn]

Having throwing stubs for other-library interfaces is working as intended, not something I'd warn about (you also cannot do anything about it).

The case I do want to warn about is:

• A public interface (for some definition of "public" that makes sense, including not being private or final)
• which has a private member, say _privateMember
• and is not marked base (or sealed with a base supertype)
• And where code in the same library does o._privateMember
• where o is not an object that is known to inherit implementation for _privateMember.
  • A this._privateMember or super._privateMember (not in a mixin) or x._privateMember where we know the actual type of x, because we just created it in the line above, can all be safe.
  • A parameter is the typical case, but an instance field initialized with a constructor parameter should also count.

I'd give the warning at the o._privateMember:

You're calling _privateMember on an object which may be implementing TheType without having a valid implementation of _privateMember. Consider making TheType base to ensure a concrete implementation on all TheType subtypes.

(or something to that effect.)

Using all available analyzer information to avoid false positives is fine. Any heuristic to decide that the type is not actually public (say, never exposed by package's public API) is fine.

So, this should give a warning:

class MyInt {
  final int _value;
  MyInt(int value) : _value = value;
  MyInt operator+(MyInt other) => MyInt(_value + other._value);
  int toInt() => _value;
}

because other._value is unsafe. Changing MyInt to base class MyInt would solve it, as would changing + to:

  MyInt operator+(MyInt other) => MyInt(_value + other.toInt());

(BigInt will choose the former approach and prevent implementation. Duration was fixed the latter way, to use the public .asMilliseconds/.asMicroseconds instead of accessing a private member.)

[eernstg]

Agreed, we can use all available information to avoid false positives with any diagnostic. However, the core of the issue is still the simple fact that a class has a throwing instance member which was generated implicitly, and it's in general an undecidable task to tell whether or not that might be executed, so a warning is warranted, in general.

Relying on the fact that there are no invocations of _privateMember on any other receiver than this is safe, but also very brittle (you'd have a rule like "never write o._privateMember unless you already have an invocation like that somewhere in this library).

I hope we'll get to a place where no such classes exist, because the only classes with private members are (1) base or final (including implicitly so), or (2) classes that are private and not leaked.