False positive from library_private_types_in_public_api

[eernstg]

Consider the following program:

// ----- Library 'public.dart'.

class _Private {
  final int i;
  const _Private(this.i);
}

extension type const Public._(_Private it) implements _Private { // LINT            <----
  const Public.small() : this._(const _Private(10));
  const Public.large() : this._(const _Private(100));
  factory Public.negative(int i) {
    if (i >= 0) throw ArgumentError("Must be negative");
    return Public._(_Private(i));
  }
}

// ----- Some other library
import 'public.dart';

// Can create instances of `_Private` using the "public face" `Public`,
// but only in very specific ways.
// Can use `Public` as the static type of instances of `_Private`.

void main() {
  const c1 = Public.small();
  const c2 = Public.large();
  var v1 = Public.negative(-1);
  Public v2 = c1;
  print(v2.i); // '10'.
}

The program gives rise to a diagnostic message from library_private_types_in_public_api at the formal parameter it of the primary constructor Public._:

Invalid use of a private type in a public API.

However, a private constructor should probably not be considered to be a public API.

Also, the overall purpose of the code seems legitimate (that is, providing access for other libraries to a private class as a type, and providing controlled access to the creation of instances of the private class), which could serve as an additional argument why the lint should not flag this kind of situation.

[pq]

@bwilkerson

[bwilkerson]

However, a private constructor should probably not be considered to be a public API.

I agree. A private constructor is not public API.

However, as I understand it, the representation field (_Private it) is treated like a final instance field with the same name and type. If the name of the field is not private, then the induced getter is public API with a private return type. I believe that that's the public API being reported by the lint, not the constructor.

... providing access for other libraries to a private class as a type ...

I believe that's exactly the kind of thing the lint is designed to prevent. If you really want the other libraries to use instances of a class then that class shouldn't be private.

[eernstg]

the representation field (_Private it) is treated like a final instance field

Aha, that was the reason. Good catch, thanks!

I just changed it to _it (which wasn't important for the example), and the lint message went away.

OK, so this entire issue was a false positive. ;-)
Closing.

[eernstg]

By the way, I wanted to comment on one more thing:

If you really want the other libraries to use instances of a class then that class shouldn't be private.

I don't think we should adopt that as a general rule. The point is that we may derive some expressive power from giving selective access to a private type. For example, here is a design that allows us to restrict the usage of a given class.

Here's the straightforward version:

// --- Library 'lib.dart'.

class C<X> {
  void Function(X) fun;
  C(this.fun);
}

// --- Library 'main.dart'.

void main() {
  C<int> c = C((i) => print(i.isEven));
  c.fun(42); // OK.

  C<num> badC = c; // OK!
  badC.fun(3.14); // Throws; even `badC.fun` would throw.
}

We can use a private class and a public "face" for that class (a type alias) to tighten the too-permissive typing:

// --- Library 'lib.dart'.

typedef Inv<X> = X Function(X);
typedef C<X> = _C<X, Inv<X>>;

class _C<X, Invariance extends Inv<X>> {
  void Function(X) fun;
  _C(this.fun);
}

// --- Library 'main.dart'.
import 'lib.dart';

void main() {
  C<int> c = C((i) => print(i.isEven));
  c.fun(42); // OK.

  // C<num> badC = c; // Compile-time error.
  // badC.fun(3.14); // So we never even get to run this.

  C<num> badC = c as dynamic; // Compiles, but throws.
}

This means that it is a built-in (and strictly enforced) property of C<num> that it isn't a supertype of C<int>, and this means that we don't have to worry about having covariant subtyping of an instance of type C, it just won't happen.

In contrast, if we insist that _C must be public then we can't enforce that there always is the relationship between the type arguments of _C that if the first one is T then the second one is exactly T Function(T). If we don't have that relationship then we don't get the extra type safety. For instance, a _C<int, Never> would be assignable to a variable of type _C<num, S> for any S.

Extension types can be used to do similar things, and many other things that are also essentially about providing constrained/selective access to a given type (and that type is only protected against unlimited access if it is private).

[bwilkerson]

Ok. I don't have any problem with your example [1]. If you added something like

extension type const Public._(C it) implements C {}

everything would be fine because C is a public type.

But if you wrote

extension type const Public._(_C it) implements _C {}

I'd expect the lint to produce a diagnostic because then you're exposing the private type instead of the public face you defined.

[1] I never would have thought of an example like that, hence my not mentioning it as a possible solution. Also, wouldn't this be better supported if we just implemented the variance language feature rather than require code like this?

[eernstg]

Also, wouldn't this be better supported if we just implemented the variance language feature rather than require code like this?

I'd love to have some version of dart-lang/language#524 in the language, and it's worth noting that a real language mechanism would enable an improved user experience (because the emulation gives rise to error messages that reveal the underlying private type and talk about the phantom type parameter that users shouldn't otherwise need to know about).

However, phantom type parameters can do many other things. Having built-in support for statically checked variance will just eliminate the need for one particular use case. Here is another one:

abstract class Unit {}
abstract class Meter implements Unit {}
abstract class Inch implements Unit {}

extension type Length<X extends Unit>(int value) {
  Length<X> operator +(Length<X> other) => Length<X>(value + other.value);
}

extension on int {
  Length<Meter> get asMeter => Length<Meter>(this);
  Length<Inch> get asInch => Length<Inch>(this);
}

Length<X> add<X extends Unit>(Length<X> a, Length<X> b) {
  // Note that the units are consistent, but we do not
  // depend on compile-time knowledge about _which_ unit
  // is being used.
  return a + b;
}

void main() {
  var x1 = 10.asMeter;
  var x2 = 5.asMeter;
  var x3 = add(x1, x2);
  assert(x3 == 15);
  x3.expectStaticType<Exactly<Length<Meter>>>;
  
  var y1 = 10.asInch;
  var y2 = add(y1, y1);
  assert(y2 == 20);
  y2.expectStaticType<Exactly<Length<Inch>>>;
  
  // y2 = x3; // Compile-time error.
}

// Static type probing helpers.

typedef Exactly<X> = X Function(X);

extension<X> on X {
  X expectStaticType<Y extends Exactly<X>>() => this;
}

By the way, we'd need to use (or emulate) invariance for the type parameter of Length in order to make this safe. Otherwise we can call add<Unit>(a, b) where a and b have different units. We would know that something is wrong because the returned result wouldn't be assignable to Length<Meter> (or indeed Length<U> for any value of U which is a proper subtype of Unit), but we could be calling a method with return type void. There's nothing special about this, the usual technique just works; but it does make the code a bit less readable, so let's just pretend that we have made that type parameter invariant.

The point is that phantom type parameters can be used to maintain a kind of (very simple) type system extensions, and compile-time-only mechanisms like extension types can be a convenient vehicle for implementing those type system extensions on top of existing classes. The result is that we can maintain a slightly more disciplined style of programming without paying anything for it at run time.