Prefer package-local canonicalization and warn on reexported private API across packages (#1684)
diff --git a/lib/src/model.dart b/lib/src/model.dart
index 78c5a26..3e4c982 100644
--- a/lib/src/model.dart
+++ b/lib/src/model.dart
@@ -2941,6 +2941,21 @@
if (topLevelElement == lookup) return true;
return false;
}).toList();
+
+ // Avoid claiming canonicalization for elements outside of this element's
+ // defining package.
+ // TODO(jcollins-g): Make the else block unconditional.
+ if (!candidateLibraries.isEmpty &&
+ !candidateLibraries
+ .any((l) => l.package == definingLibrary.package)) {
+ warn(PackageWarning.reexportedPrivateApiAcrossPackages,
+ message: definingLibrary.package.fullyQualifiedName,
+ referredFrom: candidateLibraries);
+ } else {
+ candidateLibraries
+ .removeWhere((l) => l.package != definingLibrary.package);
+ }
+
// Start with our top-level element.
ModelElement warnable =
new ModelElement.fromElement(topLevelElement, packageGraph);
@@ -2972,7 +2987,8 @@
} else {
_canonicalLibrary = definingLibrary;
}
- if (this is Inheritable) {
+ // Only pretend when not linking to remote packages.
+ if (this is Inheritable && !config.linkToRemote) {
if ((this as Inheritable).isInherited &&
_canonicalLibrary == null &&
packageGraph.publicLibraries.contains(library)) {
@@ -4054,8 +4070,9 @@
case PackageWarning.noCanonicalFound:
// Fix these warnings by adding libraries with --include, or by using
// --auto-include-dependencies.
- // TODO(jcollins-g): add a dartdoc flag to enable external website linking for non-canonical elements, using .packages for versioning
- // TODO(jcollins-g): support documenting multiple packages at once and linking between them
+ // TODO(jcollins-g): pipeline references through linkedName for error
+ // messages and warn for non-public canonicalization
+ // errors.
warningMessage =
"no canonical library found for ${warnableName}, not linking";
break;
@@ -4081,6 +4098,10 @@
warningMessage =
"--package-order gives invalid package name: '${message}'";
break;
+ case PackageWarning.reexportedPrivateApiAcrossPackages:
+ warningMessage =
+ "private API of ${message} is reexported by libraries in other packages: ";
+ break;
case PackageWarning.unresolvedDocReference:
warningMessage = "unresolved doc reference [${message}]";
if (referredFrom == null) {
diff --git a/lib/src/warnings.dart b/lib/src/warnings.dart
index 58589ed..2e89a2b 100644
--- a/lib/src/warnings.dart
+++ b/lib/src/warnings.dart
@@ -54,6 +54,10 @@
PackageWarning.packageOrderGivesMissingPackageName,
"category-order-gives-missing-package-name",
"The category-order flag on the command line was given the name of a nonexistent package"),
+ PackageWarning.reexportedPrivateApiAcrossPackages: const PackageWarningHelpText(
+ PackageWarning.reexportedPrivateApiAcrossPackages,
+ "reexported-private-api-across-packages",
+ "One or more libraries reexports private API members from outside its own package"),
PackageWarning.unresolvedDocReference: const PackageWarningHelpText(
PackageWarning.unresolvedDocReference,
"unresolved-doc-reference",
@@ -113,6 +117,7 @@
noCanonicalFound,
noLibraryLevelDocs,
packageOrderGivesMissingPackageName,
+ reexportedPrivateApiAcrossPackages,
unresolvedDocReference,
unknownMacro,
brokenLink,
diff --git a/tool/grind.dart b/tool/grind.dart
index 195c58c..abec203 100644
--- a/tool/grind.dart
+++ b/tool/grind.dart
@@ -33,6 +33,10 @@
}
}
+/// The pub cache inherited by grinder.
+final String defaultPubCache =
+ Platform.environment['PUB_CACHE'] ?? resolveTildePath('~/.pub-cache');
+
/// Run no more than the number of processors available in parallel.
final MultiFutureTracker testFutures = new MultiFutureTracker(
Platform.environment.containsKey('TRAVIS')
@@ -183,11 +187,11 @@
class WarningsCollection {
final String tempDir;
- final Map<String, int> _warningKeyCounts;
+ final Map<String, int> warningKeyCounts;
final String branch;
final String pubCachePath;
WarningsCollection(this.tempDir, this.pubCachePath, this.branch)
- : this._warningKeyCounts = new Map() {}
+ : this.warningKeyCounts = new Map() {}
static const String kPubCachePathReplacement = '_xxxPubDirectoryxxx_';
static const String kTempDirReplacement = '_xxxTempDirectoryxxx_';
@@ -208,8 +212,8 @@
void add(String text) {
String key = _toKey(text);
- _warningKeyCounts.putIfAbsent(key, () => 0);
- _warningKeyCounts[key]++;
+ warningKeyCounts.putIfAbsent(key, () => 0);
+ warningKeyCounts[key]++;
}
/// Output formatter for comparing warnings. [this] is the original.
@@ -220,19 +224,19 @@
Set<String> onlyCurrent = new Set();
Set<String> identical = new Set();
Set<String> allKeys = new Set.from([]
- ..addAll(_warningKeyCounts.keys)
- ..addAll(current._warningKeyCounts.keys));
+ ..addAll(warningKeyCounts.keys)
+ ..addAll(current.warningKeyCounts.keys));
for (String key in allKeys) {
- if (_warningKeyCounts.containsKey(key) &&
- !current._warningKeyCounts.containsKey(key)) {
+ if (warningKeyCounts.containsKey(key) &&
+ !current.warningKeyCounts.containsKey(key)) {
onlyOriginal.add(key);
- } else if (!_warningKeyCounts.containsKey(key) &&
- current._warningKeyCounts.containsKey(key)) {
+ } else if (!warningKeyCounts.containsKey(key) &&
+ current.warningKeyCounts.containsKey(key)) {
onlyCurrent.add(key);
- } else if (_warningKeyCounts.containsKey(key) &&
- current._warningKeyCounts.containsKey(key) &&
- _warningKeyCounts[key] != current._warningKeyCounts[key]) {
+ } else if (warningKeyCounts.containsKey(key) &&
+ current.warningKeyCounts.containsKey(key) &&
+ warningKeyCounts[key] != current.warningKeyCounts[key]) {
quantityChangedOuts.add(key);
} else {
identical.add(key);
@@ -253,7 +257,7 @@
printBuffer.writeln('*** $title : Identical warning quantity changed');
for (String key in quantityChangedOuts) {
printBuffer.writeln(
- "* Appeared ${_warningKeyCounts[key]} times in $branch, ${current._warningKeyCounts[key]} in ${current.branch}:");
+ "* Appeared ${warningKeyCounts[key]} times in $branch, ${current.warningKeyCounts[key]} in ${current.branch}:");
printBuffer.writeln(current._fromKey(key));
}
}
@@ -813,26 +817,38 @@
pluginPackageDocsDir.path, 8005, 'serve-dartdoc-flutter-plugin-docs');
}
-@Task('Build docs for a package that requires flutter with remote linking')
-buildDartdocFlutterPluginDocs() async {
+Future<WarningsCollection> _buildDartdocFlutterPluginDocs() async {
FlutterRepo flutterRepo = await FlutterRepo.fromExistingFlutterRepo(
await cleanFlutterRepo, 'docs-flutter-plugin');
- await flutterRepo.launcher.runStreamed(
- Platform.resolvedExecutable,
- [
- '--checked',
- pathLib.join(Directory.current.path, 'bin', 'dartdoc.dart'),
- '--link-to-remote',
- '--output',
- pluginPackageDocsDir.path
- ],
- workingDirectory: pluginPackage.path);
+ return jsonMessageIterableToWarnings(
+ await flutterRepo.launcher.runStreamed(
+ Platform.resolvedExecutable,
+ [
+ '--checked',
+ pathLib.join(Directory.current.path, 'bin', 'dartdoc.dart'),
+ '--json',
+ '--link-to-remote',
+ '--output',
+ pluginPackageDocsDir.path
+ ],
+ workingDirectory: pluginPackage.path),
+ pluginPackageDocsDir.path,
+ defaultPubCache,
+ 'HEAD');
+}
+
+@Task('Build docs for a package that requires flutter with remote linking')
+buildDartdocFlutterPluginDocs() async {
+ await _buildDartdocFlutterPluginDocs();
}
@Task('Verify docs for a package that requires flutter with remote linking')
-@Depends(buildDartdocFlutterPluginDocs)
testDartdocFlutterPlugin() async {
+ WarningsCollection warnings = await _buildDartdocFlutterPluginDocs();
+ if (!warnings.warningKeyCounts.isEmpty) {
+ fail('No warnings should exist in : ${warnings.warningKeyCounts}');
+ }
// Verify that links to Dart SDK and Flutter SDK go to the flutter site.
expectFileContains(
pathLib.join(
