Correct a regression for a package with a specified set of included libraries (#3825)

Correct a regression for a package with a specified set of included libraries

In 12d271a we change what libraries can count
as a 'canonical library', such that when a set of libraries is specified with
the 'include' option, any library not in the 'include' list is not canonical.
This breaks a few things.

The fix is to add all libraries to the `_allLibraries` field, not taking the
'include' list into consideration. Then when we decide what libraries are
considered to be part of a package, we filter down to 'included' libraries.

Author: srawlins

lib/src/model/category.dart

@@ -21,7 +21,7 @@ class Category
         TopLevelContainer,
         Indexable
     implements Documentable {
-  /// All libraries in [libraries] must come from [_package].
+  /// All libraries in [libraries] must come from [package].
   @override
   final Package package;
 

lib/src/model/library.dart

@@ -5,8 +5,6 @@
 import 'package:analyzer/dart/element/element.dart';
 import 'package:analyzer/dart/element/scope.dart';
 import 'package:analyzer/source/line_info.dart';
-// ignore: implementation_imports
-import 'package:analyzer/src/generated/sdk.dart' show SdkLibrary;
 import 'package:dartdoc/src/model/comment_referable.dart';
 import 'package:dartdoc/src/model/model.dart';
 import 'package:dartdoc/src/package_meta.dart' show PackageMeta;
@@ -107,13 +105,17 @@ class Library extends ModelElement
   CompilationUnitElement get compilationUnitElement =>
       element.definingCompilationUnit;
 
-  SdkLibrary? get _sdkLib =>
-      packageGraph.sdkLibrarySources[element.librarySource];
-
   @override
+
+  /// Whether this library is considered "public."
+  ///
+  /// A library is considered public if it:
+  /// * is an SDK library and it is documented and it is not internal, or
+  /// * is found in a package's top-level 'lib' directory, and
+  ///   not found in it's 'lib/src' directory, and it is not excluded.
   bool get isPublic {
     if (!super.isPublic) return false;
-    final sdkLib = _sdkLib;
+    final sdkLib = packageGraph.sdkLibrarySources[element.librarySource];
     if (sdkLib != null && (sdkLib.isInternal || !sdkLib.isDocumented)) {
       return false;
     }

lib/src/model/nameable.dart

@@ -30,8 +30,7 @@ mixin Nameable {
   ///
   /// A "package-public" element satisfies the following requirements:
   /// * is not documented with the `@nodoc` directive,
-  /// * for a library, is found in a package's top-level 'lib' directory, and
-  ///   not found in it's 'lib/src' directory,
+  /// * for a library, adheres to the documentation for [Library.isPublic],
   /// * for a library member, is in a _public_ library's exported namespace, and
   ///   is not privately named, nor an unnamed extension,
   /// * for a container (class, enum, extension, extension type, mixin) member,

lib/src/model/package_builder.dart

@@ -286,10 +286,8 @@ class PubPackageBuilder implements PackageBuilder {
         if (processedLibraries.contains(resolvedLibrary.element)) {
           continue;
         }
-        if (addingSpecials || _shouldIncludeLibrary(resolvedLibrary.element)) {
-          addLibrary(resolvedLibrary);
-          processedLibraries.add(resolvedLibrary.element);
-        }
+        addLibrary(resolvedLibrary);
+        processedLibraries.add(resolvedLibrary.element);
       }
       files.addAll(newFiles);
       if (!addingSpecials) {
@@ -329,10 +327,6 @@ class PubPackageBuilder implements PackageBuilder {
     }
   }
 
-  /// Whether [libraryElement] should be included in the libraries-to-document.
-  bool _shouldIncludeLibrary(LibraryElement libraryElement) =>
-      _config.include.isEmpty || _config.include.contains(libraryElement.name);
-
   /// Returns all top level library files in the 'lib/' directory of the given
   /// package root directory.
   ///

lib/src/model/package_graph.dart

@@ -107,11 +107,17 @@ class PackageGraph with CommentReferable, Nameable {
           'that `$dartOrFlutter analyze` completes without errors.');
     }
     var package = Package.fromPackageMeta(packageMeta, this);
-    var lib = Library.fromLibraryResult(resolvedLibrary, this, package);
-    package.libraries.add(lib);
-    _allLibraries[libraryElement.source.fullName] = lib;
+    var library = Library.fromLibraryResult(resolvedLibrary, this, package);
+    if (_shouldIncludeLibrary(libraryElement)) {
+      package.libraries.add(library);
+    }
+    _allLibraries[libraryElement.source.fullName] = library;
   }
 
+  /// Whether [libraryElement] should be included in the libraries-to-document.
+  bool _shouldIncludeLibrary(LibraryElement libraryElement) =>
+      config.include.isEmpty || config.include.contains(libraryElement.name);
+
   /// Adds [resolvedLibrary] as a special library to the package graph, which
   /// adds the library to [_allLibraries], but does not add it to any [Package]'s
   /// list of libraries.
@@ -346,10 +352,10 @@ class PackageGraph with CommentReferable, Nameable {
   /// [libraries].
   ///
   /// Keyed by `LibraryElement.source.fullName` to resolve different URIs
-  /// referring to the same location, to the same [Library].  This isn't how Dart
-  /// works internally, but Dartdoc pretends to avoid documenting or duplicating
-  /// data structures for the same "library" on disk based on how it is
-  /// referenced.  We can't use [Source] as a key due to differences in the
+  /// referring to the same location, and hence to the same [Library]. This
+  /// isn't how Dart works internally, but Dartdoc pretends to avoid documenting
+  /// or duplicating data structures for the same library on disk based on how
+  /// it is referenced. We can't use [Source] as a key due to differences in the
   /// [TimestampedData] timestamps.
   ///
   /// This mapping must be complete before [initializePackageGraph] is called.
@@ -556,14 +562,19 @@ class PackageGraph with CommentReferable, Nameable {
     }
   }
 
-  int _lastSizeOfAllLibraries = 0;
+  int _previousSizeOfAllLibraries = 0;
 
   /// A mapping from a [LibraryElement] to all of the [Library]s that export it,
   /// which is created if it is not yet populated.
   Map<LibraryElement, Set<Library>> get libraryExports {
-    // Table must be reset if we're still in the middle of adding libraries.
-    if (_allLibraries.keys.length != _lastSizeOfAllLibraries) {
-      _lastSizeOfAllLibraries = _allLibraries.keys.length;
+    // The map must be reset if we're still in the middle of adding libraries
+    // (though this shouldn't happen).
+    if (_allLibraries.keys.length != _previousSizeOfAllLibraries) {
+      assert(
+        _previousSizeOfAllLibraries == 0,
+        'Re-entered `libraryExports` while building all libraries',
+      );
+      _previousSizeOfAllLibraries = _allLibraries.keys.length;
       _libraryExports = {};
       for (var library in publicLibraries) {
         _tagExportsFor(library, library.element);

test/element_type_test.dart

@@ -28,15 +28,9 @@ void main() async {
   late FakePackageConfigProvider packageConfigProvider;
   late String packagePath;
 
-  Future<void> setUpPackage(
-    String name, {
-    String? pubspec,
-    String? analysisOptions,
-  }) async {
+  Future<void> setUpPackage(String name) async {
     packagePath = await d.createPackage(
       name,
-      pubspec: pubspec,
-      analysisOptions: analysisOptions,
       resourceProvider: resourceProvider,
     );
 

test/options_test.dart

@@ -28,12 +28,14 @@ class OptionsTest extends DartdocTestBase {
   static const packageName = 'test_package';
 
   Future<void> createPackage({
+    String? pubspec,
     String? dartdocOptions,
     List<d.Descriptor> libFiles = const [],
     List<d.Descriptor> files = const [],
   }) async {
     packagePath = await d.createPackage(
       packageName,
+      pubspec: pubspec,
       dartdocOptions: dartdocOptions,
       libFiles: libFiles,
       files: files,
@@ -181,6 +183,51 @@ class Baz {}
         orderedEquals(['library_1', 'library_2']));
   }
 
+  void test_includeOption_referringToNotIncluded() async {
+    await createPackage(
+      pubspec: '''
+name: options
+version: 0.0.1
+environment:
+  sdk: '>=3.4.0 <4.0.0'
+dependencies:
+  http:
+    path: vendor/http
+''',
+      dartdocOptions: '''
+dartdoc:
+  include: ["library_1"]
+''',
+      libFiles: [
+        d.file('library_1.dart', '''
+library library_1;
+import 'package:http/http.dart' as http;
+class Foo {
+  http.Client? client;
+}
+'''),
+      ],
+      files: [
+        d.dir('vendor', [
+          d.dir('http', [
+            d.dir('lib', [d.file('http.dart', 'class Client {}')])
+          ])
+        ])
+      ],
+    );
+    final packageGraph = await bootBasicPackage(
+      packagePath,
+      packageMetaProvider,
+      packageConfigProvider,
+    );
+    expect(packageGraph.localPublicLibraries.map((l) => l.name),
+        orderedEquals(['library_1']));
+    final foo =
+        packageGraph.localPackages.first.libraries.first.classes.named('Foo');
+    // The name is not linked, but also does not error.
+    expect(foo.allFields.first.modelType.linkedName, 'Client?');
+  }
+
   void test_includeCommandLineOption_overridesOptionsFileOption() async {
     await createPackage(
       dartdocOptions: '''

test/src/test_descriptor_utils.dart

@@ -48,7 +48,10 @@ Future<String> createPackage(
     final dependencies = parsedYaml['dependencies'] as Map;
     for (var dep in dependencies.keys) {
       // This only accepts 'path' deps.
-      final depConfig = dependencies[dep] as Map;
+      final depConfig = dependencies[dep];
+      if (depConfig is! Map) {
+        throw StateError('dep in pubspec must be a Map, but is: "$depConfig"');
+      }
       final pathDep = depConfig['path'];
 
       packagesInfo.writeln(''',{