Linter: Avoid private types in public API

[goderbauer]

Public API should not reference any private types that may not be accessible to the caller.

Examples

typedef _Foo = int Function(String);

class _Bar { }

// lint warning on the next line that _Bar and _Foo are private types while doSomething is public API.
void doSomthing(_Bar bar, _Foo foo) {
  // ...
}

/cc @pq @Hixie @mit-mit @munificent @devoncarew

[bwilkerson]

I have a prototype of this already implemented. Just haven't had time to polish it. I'm happy to commit it if someone else wants to take it over before I get back to it.

[devoncarew]

We do see this issue come up in dartdoc - it needs to account for documenting APIs when there a private type in the inheritance hierarchy for something it is documenting.

cc @jcollins-g

[bwilkerson]

Ok, I remember now why I hadn't finished it. I'd gotten to the point where I needed to make some decisions about what did and didn't constitute a violation, and was planning on opening an issue to discuss it. Now one's been opened for me. :-)

I think that by "private type" we mean any reference to a type whose name is private (_Private) or that's not visible from a public library (that part is likely to make this lint intractable unless we figure out how to get the exported name space from all public libraries in an efficient way) whether as the type directly or as a type argument to the type (List<_Private>).

But, given that definition, here are the places I can think of where we might want to generate a lint on a type if it's a private type:

• the return type of a function or method,
• the type of any parameter of a function or method,
• the bound of a type parameter to any function, method, class, mixin, extension's extended type, or type alias,
• the type of any top level variable or field,
• any type used in the declaration of a type alias (typedef F = _Private Function();),
• any extends, implements, with or on clause.

So, does the definition of "private type" sound right, and which of these uses do we want to guard against?

[Hixie]

FWIW, I would define private as "has a leading underscore", and would care about everything you list except for "extends", "implements", and "with" (i _would_ care about "on"). I can justify myself if anyone disagrees. :-)

[goderbauer]

+1 to what Hixie said.

[bwilkerson]

I would define private as "has a leading underscore" ...

I understand the Flutter package structure guidelines, but those guidelines are not universally followed. I'm concerned that by using that definition we're conflating library private and the more common definition of package private, and hence making this lint far less useful for non-Flutter packages.

That said, I don't have a good solution for efficiently implementing the pub package conventions around package private code, so I'm willing to implement this rule with the more restricted definition of private as a first cut.

[Hixie]

IMHO "package private" (but not underscored) identifiers can still be used by developers, and as such should be documented, even if doing so is considered bad form and requires a "src" import. This is why I'd not count them against this lint. That said, I don't object to defining private as "not exported by this package". In fact, that might be better, since it would encourage people who write APIs that use types from other packages to reexport those types (Flutter fails to do this reliably and a lint to force us to do it right would be very welcome).

[jcollins-g]

If the computation proves to be too expensive for the analyzer to implement in an efficient way, we could consider using the already computationally expensive implementation in dartdoc that computes the public API of a package. All that CPU time is going into building these data structures anyway...

I am not sure about defining private as "not exported by this package". First, I doubt we would want that to be literally true or we'd reexport dart:core in most packages. Flutter itself consists of a set of packages, presumably you don't want this to be enforced between all of them? In the case of people using or extending Flutter, I doubt that they should be reexporting any used Flutter types just because they're not in the Dart SDK. There are probably other situations where people are extending or adding features to a commonly used package where this would lead to repetitive boilerplate. I would also argue that the repetitive boilerplate in question doesn't add value -- these types are still defined in the packages you're reexporting them from and it's basically implementing in text what the analyzer will tell you anyway. Plus IMO it obfuscates the originating source of the types further and may make it harder to understand who owns what when reading code as imports won't show the originating package of the types in question.

I would recommend instead defining private as "defined in, but not exported by this package outside of lib/src". That's the general assumption dartdoc goes by when deciding to document something. If you stick to that, you will not require excessive boilerplate or complicated exceptions to the original rule to avoid it, nor risk using types that are not part of Flutter or its dependencies public APIs. That (edit: latter bit) seems like the main thing you want to avoid here?

[Hixie]

I am not sure about defining private as "not exported by this package". First, I doubt we would want that to be literally true or we'd reexport dart:core in most packages.

dart:core is implicitly exported by everything, effectively (since it's implicitly imported by everything).

Flutter itself consists of a set of packages, presumably you don't want this to be enforced between all of them?

Actually Flutter was the main use case I had in mind here. In general, if you import flutter/material, you don't want all of flutter/rendering, but you _do_ want the APIs that flutter/rendering introduces which are then exposed in flutter/material's API. Same with dart:ui. For example, I want to be able to use Color without explicitly importing dart:ui, because the higher layers expose it in their APIs. But I don't want dart:ui's TextStyle to be exported, because the higher layers don't use that in their API (they have their own TextStyle in fact).

[jcollins-g]

I am not sure about defining private as "not exported by this package". First, I doubt we would want that to be literally true or we'd reexport dart:core in most packages.

dart:core is implicitly exported by everything, effectively (since it's implicitly imported by everything).

True enough, bad example. The other examples still apply, however...

Flutter itself consists of a set of packages, presumably you don't want this to be enforced between all of them?

Actually Flutter was the main use case I had in mind here. In general, if you import flutter/material, you don't want all of flutter/rendering, but you _do_ want the APIs that flutter/rendering introduces which are then exposed in flutter/material's API. Same with dart:ui. For example, I want to be able to use Color without explicitly importing dart:ui, because the higher layers expose it in their APIs. But I don't want dart:ui's TextStyle to be exported, because the higher layers don't use that in their API (they have their own TextStyle in fact).

So in other words the obfuscation of imports is a feature in your view, not a bug, and the other solutions to this problem (selectively importing Color directly) are explicitly what you're trying to avoid.

Well, in that case such a definition would be necessary, but it would be likely a very Flutter-specific lint unless we decide the entire ecosystem should be encouraged to be built like this. I personally dislike this approach -- I prefer it to be more explicit when my types are coming from dependencies -- but that doesn't necessarily mean it isn't a good idea. I don't feel like I have full information on how people prefer to create package interfaces.

[Hixie]

I'd much rather people be able to say:

import 'package:flutter/material.dart';

...than:

import 'dart:async'; // for Future
import 'dart:ui'; // for Color, Canvas, FontWeight, etc
import 'vector_math/vector_math_64.dart'; // for Matrix4
import 'characters/characters.dart'; // for Characters
import 'package:flutter/foundation.dart'; // for ChangeNotifier, etc
import 'package:flutter/painting.dart'; // for AssetImage, etc
import 'package:flutter/services.dart'; // for Brightness, etc
import 'package:flutter/rendering.dart'; // for MainAxisAlignment, CustomPainter, etc
import 'package:flutter/physics.dart'; // for Simulation, etc
import 'package:flutter/gestures.dart'; // for TapDownDetails
import 'package:flutter/widgets.dart'; // for runApp, Column, etc
import 'package:flutter/material.dart';