Improve support for package-with-macro-application

The previous fix is no good; it was concerned with _augmentations_, assuming
they cannot be read from disk. This is not the case. Only a macro-generated
augmentation cannot be read from disk.

Instead we change tactics and say we _can_ include source code from a macro
(maybe we shouldn't), and just ask analyzer for all source files, from the
AnalysisContext.

Other changes to facilitate this:

* Do not dispose the AnalysisContextCollection right away; we should wait until
  all docs are generated.
* Pass the AnalysisContext to ModelNode, and no longer pass the
  ResourceProvider.
* Remove the file cache thing in model_utils; instead ModelNode.sourceCode only
  relies on the AnalysisContext. Theoretically there is already caching over
  that at analyzer? But I benchmarked with Flutter and saw no time-to-document
  or max-RSS changes.
* Do not ask the AnalysisContextCollection for the AnalysisContext _for every
  file_! Cache it. To facilitate that, change `PubPackageBuilder()` into a
  factory.
* Sort the fields instantiated by the `PackageGraph.uninitialized` constructor
  above it.
* In `addFilesReferencedBy`, also look at augmentation imports.

Author: srawlins

lib/src/dartdoc.dart

@@ -217,6 +217,7 @@ class Dartdoc {
     if (config.showStats) {
       logInfo(runtimeStats.buildReport());
     }
+    await packageBuilder.dispose();
     return DartdocResults(config.topLevelPackageMeta, packageGraph, _outputDir);
   }
 

lib/src/model/model_node.dart

@@ -4,24 +4,24 @@
 
 import 'dart:convert';
 
+import 'package:analyzer/dart/analysis/analysis_context.dart';
+import 'package:analyzer/dart/analysis/results.dart';
 import 'package:analyzer/dart/ast/ast.dart';
 import 'package:analyzer/dart/element/element.dart';
-import 'package:analyzer/file_system/file_system.dart';
-import 'package:dartdoc/src/model_utils.dart' as model_utils;
 import 'package:meta/meta.dart';
 
 /// Stripped down information derived from [AstNode] containing only information
 /// needed to resurrect the source code of [element].
 class ModelNode {
   final Element _element;
-  final ResourceProvider _resourceProvider;
+  final AnalysisContext _analysisContext;
   final int _sourceEnd;
   final int _sourceOffset;
 
   factory ModelNode(
-      AstNode? sourceNode, Element element, ResourceProvider resourceProvider) {
+      AstNode? sourceNode, Element element, AnalysisContext analysisContext) {
     if (sourceNode == null) {
-      return ModelNode._(element, resourceProvider,
+      return ModelNode._(element, analysisContext,
           sourceEnd: -1, sourceOffset: -1);
     } else {
       // Get a node higher up the syntax tree that includes the semicolon.
@@ -32,12 +32,12 @@ class ModelNode {
         assert(sourceNode is FieldDeclaration ||
             sourceNode is TopLevelVariableDeclaration);
       }
-      return ModelNode._(element, resourceProvider,
+      return ModelNode._(element, analysisContext,
           sourceEnd: sourceNode.end, sourceOffset: sourceNode.offset);
     }
   }
 
-  ModelNode._(this._element, this._resourceProvider,
+  ModelNode._(this._element, this._analysisContext,
       {required int sourceEnd, required int sourceOffset})
       : _sourceEnd = sourceEnd,
         _sourceOffset = sourceOffset;
@@ -46,14 +46,21 @@ class ModelNode {
 
   /// The text of the source code of this node, stripped of the leading
   /// indentation, and stripped of the doc comments.
-  late final String sourceCode = _isSynthetic
-      ? ''
-      : model_utils
-          .getFileContentsFor(_element, _resourceProvider)
-          .substringFromLineStart(_sourceOffset, _sourceEnd)
-          .stripIndent
-          .stripDocComments
-          .trim();
+  late final String sourceCode = () {
+    if (_isSynthetic) return '';
+
+    var path = _element.source?.fullName;
+    if (path == null) return '';
+
+    var fileResult = _analysisContext.currentSession.getFile(path);
+    if (fileResult is! FileResult) return '';
+
+    return fileResult.content
+        .substringFromLineStart(_sourceOffset, _sourceEnd)
+        .stripIndent
+        .stripDocComments
+        .trim();
+  }();
 }
 
 @visibleForTesting

lib/src/model/package_builder.dart

@@ -4,6 +4,7 @@
 
 import 'dart:async';
 
+import 'package:analyzer/dart/analysis/analysis_context.dart';
 import 'package:analyzer/dart/analysis/context_root.dart';
 import 'package:analyzer/dart/analysis/results.dart';
 import 'package:analyzer/dart/ast/ast.dart';
@@ -36,6 +37,8 @@ import 'package:path/path.dart' as p show Context;
 abstract class PackageBuilder {
   // Builds package graph to be used by documentation generator.
   Future<PackageGraph> buildPackageGraph();
+
+  Future<void> dispose();
 }
 
 /// A package builder that understands pub package format.
@@ -44,12 +47,49 @@ class PubPackageBuilder implements PackageBuilder {
   final PackageMetaProvider _packageMetaProvider;
   final PackageConfigProvider _packageConfigProvider;
 
-  PubPackageBuilder(
+  final AnalysisContextCollectionImpl _contextCollection;
+  final AnalysisContext _analysisContext;
+
+  factory PubPackageBuilder(
+    DartdocOptionContext config,
+    PackageMetaProvider packageMetaProvider,
+    PackageConfigProvider packageConfigProvider, {
+    @visibleForTesting bool skipUnreachableSdkLibraries = false,
+  }) {
+    var contextCollection = AnalysisContextCollectionImpl(
+      includedPaths: [config.inputDir],
+      // TODO(jcollins-g): should we pass excluded directories here instead
+      // of handling it ourselves?
+      resourceProvider: packageMetaProvider.resourceProvider,
+      sdkPath: config.sdkDir,
+      updateAnalysisOptions2: ({
+        required AnalysisOptionsImpl analysisOptions,
+        required ContextRoot contextRoot,
+        required DartSdk sdk,
+      }) =>
+          analysisOptions
+            ..warning = false
+            ..lint = false,
+    );
+    return PubPackageBuilder._(
+      config,
+      packageMetaProvider,
+      packageConfigProvider,
+      contextCollection,
+      analysisContext: contextCollection.contextFor(config.inputDir),
+      skipUnreachableSdkLibraries: skipUnreachableSdkLibraries,
+    );
+  }
+
+  PubPackageBuilder._(
     this._config,
     this._packageMetaProvider,
-    this._packageConfigProvider, {
-    @visibleForTesting bool skipUnreachableSdkLibraries = false,
-  }) : _skipUnreachableSdkLibraries = skipUnreachableSdkLibraries;
+    this._packageConfigProvider,
+    this._contextCollection, {
+    required AnalysisContext analysisContext,
+    required bool skipUnreachableSdkLibraries,
+  })  : _analysisContext = analysisContext,
+        _skipUnreachableSdkLibraries = skipUnreachableSdkLibraries;
 
   @override
   Future<PackageGraph> buildPackageGraph() async {
@@ -77,6 +117,7 @@ class PubPackageBuilder implements PackageBuilder {
       _sdk,
       _embedderSdkUris.isNotEmpty,
       _packageMetaProvider,
+      _analysisContext,
     );
     await _getLibraries(newGraph);
     runtimeStats.endPerfTask();
@@ -93,6 +134,12 @@ class PubPackageBuilder implements PackageBuilder {
     return newGraph;
   }
 
+  @override
+  Future<void> dispose() async {
+    // Shutdown macro support.
+    await _contextCollection.dispose();
+  }
+
   late final DartSdk _sdk = _packageMetaProvider.defaultSdk ??
       FolderBasedDartSdk(
           _resourceProvider, _resourceProvider.getFolder(_config.sdkDir));
@@ -132,22 +179,6 @@ class PubPackageBuilder implements PackageBuilder {
 
   late final Map<String, List<Folder>> _packageMap;
 
-  late final _contextCollection = AnalysisContextCollectionImpl(
-    includedPaths: [_config.inputDir],
-    // TODO(jcollins-g): should we pass excluded directories here instead of
-    // handling it ourselves?
-    resourceProvider: _resourceProvider,
-    sdkPath: _config.sdkDir,
-    updateAnalysisOptions2: ({
-      required AnalysisOptionsImpl analysisOptions,
-      required ContextRoot contextRoot,
-      required DartSdk sdk,
-    }) =>
-        analysisOptions
-          ..warning = false
-          ..lint = false,
-  );
-
   List<String> get _sdkFilesToDocument => [
         for (var sdkLib in _sdk.sdkLibraries)
           _sdk.mapDartUri(sdkLib.shortName)!.fullName,
@@ -245,6 +276,8 @@ class PubPackageBuilder implements PackageBuilder {
         }
         var resolvedLibrary = await _resolveLibrary(file);
         if (resolvedLibrary == null) {
+          // `file` did not resolve to a _library_; could be a part, an
+          // augmentation, or some other invalid result.
           _knownParts.add(file);
           continue;
         }
@@ -459,8 +492,6 @@ class PubPackageBuilder implements PackageBuilder {
       specialFiles.difference(files),
       addingSpecials: true,
     );
-    // Shutdown macro support.
-    await _contextCollection.dispose();
   }
 
   /// Throws an exception if any configured-to-be-included files were not found
@@ -511,22 +542,29 @@ class DartDocResolvedLibrary {
 extension on Set<String> {
   /// Adds [element]'s path and all of its part files' paths to `this`, and
   /// recursively adds the paths of all imported and exported libraries.
-  void addFilesReferencedBy(LibraryElement? element) {
-    if (element != null) {
-      var path = element.source.fullName;
-      if (add(path)) {
-        for (var import in element.libraryImports) {
-          addFilesReferencedBy(import.importedLibrary);
-        }
-        for (var export in element.libraryExports) {
-          addFilesReferencedBy(export.exportedLibrary);
-        }
+  void addFilesReferencedBy(LibraryOrAugmentationElement? element) {
+    if (element == null) return;
+
+    var path = element.source?.fullName;
+    if (path == null) return;
+
+    if (add(path)) {
+      for (var import in element.libraryImports) {
+        addFilesReferencedBy(import.importedLibrary);
+      }
+      for (var export in element.libraryExports) {
+        addFilesReferencedBy(export.exportedLibrary);
+      }
+      if (element is LibraryElement) {
         for (var part in element.parts
             .map((e) => e.uri)
             .whereType<DirectiveUriWithUnit>()) {
           add(part.source.fullName);
         }
       }
+      for (var augmentation in element.augmentationImports) {
+        addFilesReferencedBy(augmentation.importedAugmentation);
+      }
     }
   }
 }

lib/src/model/package_graph.dart

@@ -4,6 +4,7 @@
 
 import 'dart:collection';
 
+import 'package:analyzer/dart/analysis/analysis_context.dart';
 import 'package:analyzer/dart/ast/ast.dart';
 import 'package:analyzer/dart/element/element.dart';
 import 'package:analyzer/file_system/file_system.dart';
@@ -32,11 +33,29 @@ import 'package:dartdoc/src/warnings.dart';
 import 'package:meta/meta.dart';
 
 class PackageGraph with CommentReferable, Nameable {
+  /// Dartdoc's configuration flags.
+  final DartdocOptionContext config;
+
+  final bool hasEmbedderSdk;
+
+  /// [PackageMeta] provider for building [PackageMeta]s.
+  final PackageMetaProvider packageMetaProvider;
+
+  final InheritanceManager3 inheritanceManager = InheritanceManager3();
+
+  final AnalysisContext analysisContext;
+
+  /// PackageMeta for the default package.
+  final PackageMeta packageMeta;
+
+  final Map<Source?, SdkLibrary> sdkLibrarySources;
+
   PackageGraph.uninitialized(
     this.config,
     DartSdk sdk,
     this.hasEmbedderSdk,
     this.packageMetaProvider,
+    this.analysisContext,
   )   : packageMeta = config.topLevelPackageMeta,
         sdkLibrarySources = {
           for (var lib in sdk.sdkLibraries) sdk.mapDartUri(lib.shortName): lib
@@ -46,10 +65,6 @@ class PackageGraph with CommentReferable, Nameable {
     Package.fromPackageMeta(packageMeta, this);
   }
 
-  final InheritanceManager3 inheritanceManager = InheritanceManager3();
-
-  final Map<Source?, SdkLibrary> sdkLibrarySources;
-
   void dispose() {
     // Clear out any cached tool snapshots and temporary directories.
     // TODO(jcollins-g): Consider ownership change for these objects
@@ -260,7 +275,7 @@ class PackageGraph with CommentReferable, Nameable {
       for (var field in fields) {
         var element = field.declaredElement!;
         _modelNodes.putIfAbsent(
-            element, () => ModelNode(field, element, resourceProvider));
+            element, () => ModelNode(field, element, analysisContext));
       }
       return;
     }
@@ -269,13 +284,13 @@ class PackageGraph with CommentReferable, Nameable {
       for (var field in fields) {
         var element = field.declaredElement!;
         _modelNodes.putIfAbsent(
-            element, () => ModelNode(field, element, resourceProvider));
+            element, () => ModelNode(field, element, analysisContext));
       }
       return;
     }
     var element = declaration.declaredElement!;
     _modelNodes.putIfAbsent(
-        element, () => ModelNode(declaration, element, resourceProvider));
+        element, () => ModelNode(declaration, element, analysisContext));
   }
 
   ModelNode? getModelNodeFor(Element element) => _modelNodes[element];
@@ -338,23 +353,12 @@ class PackageGraph with CommentReferable, Nameable {
   /// A list of extensions that exist in the package graph.
   final List<Extension> _extensions = [];
 
-  /// PackageMeta for the default package.
-  final PackageMeta packageMeta;
-
   /// Name of the default package.
   String get defaultPackageName => packageMeta.name;
 
-  /// Dartdoc's configuration flags.
-  final DartdocOptionContext config;
-
-  /// PackageMeta Provider for building [PackageMeta]s.
-  final PackageMetaProvider packageMetaProvider;
-
   late final Package defaultPackage =
       Package.fromPackageMeta(packageMeta, this);
 
-  final bool hasEmbedderSdk;
-
   bool get hasFooterVersion => !config.excludeFooterVersion;
 
   @override

lib/src/model/source_code_mixin.dart

@@ -13,8 +13,7 @@ mixin SourceCode implements Documentable {
 
   Element? get element;
 
-  bool get hasSourceCode =>
-      config.includeSource && !element.isAugmentation && sourceCode.isNotEmpty;
+  bool get hasSourceCode => config.includeSource && sourceCode.isNotEmpty;
 
   Library? get library;
 
@@ -23,18 +22,3 @@ mixin SourceCode implements Documentable {
     return modelNode == null ? '' : modelNode.sourceCode;
   }
 }
-
-extension on Element? {
-  /// Whether `this` is an augmentation method or property.
-  ///
-  /// This property should only be referenced for elements whose source code we
-  /// may wish to refer to.
-  bool get isAugmentation {
-    final self = this;
-    return switch (self) {
-      ExecutableElement() => self.augmentationTarget != null,
-      PropertyInducingElement() => self.augmentationTarget != null,
-      _ => false,
-    };
-  }
-}