Skip to content

Commit

Permalink
Improve support for package-with-macro-application (#3687)
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@srawlins
srawlins committed Feb 28, 2024
1 parent b28ee1f commit c1c0854
Show file tree
Hide file tree
Showing 8 changed files with 117 additions and 108 deletions.
1 change: 1 addition & 0 deletions lib/src/dartdoc.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,7 @@ class Dartdoc {
if (config.showStats) {
logInfo(runtimeStats.buildReport());
}
await packageBuilder.dispose();
return DartdocResults(config.topLevelPackageMeta, packageGraph, _outputDir);
}

Expand Down
1 change: 1 addition & 0 deletions lib/src/generator/templates.runtime_renderers.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16851,6 +16851,7 @@ const _invisibleGetters = {
'allLibraries',
'allLibrariesAdded',
'allLocalModelElements',
'analysisContext',
'breadcrumbName',
'config',
'dartCoreObject',
Expand Down
37 changes: 22 additions & 15 deletions lib/src/model/model_node.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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;
Expand All @@ -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
Expand Down
102 changes: 70 additions & 32 deletions lib/src/model/package_builder.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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.
Expand All @@ -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 {
Expand All @@ -68,6 +108,7 @@ class PubPackageBuilder implements PackageBuilder {
_sdk,
_embedderSdkUris.isNotEmpty,
_packageMetaProvider,
_analysisContext,
);
await _getLibraries(newGraph);
runtimeStats.endPerfTask();
Expand All @@ -84,6 +125,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));
Expand Down Expand Up @@ -123,22 +170,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,
Expand Down Expand Up @@ -236,6 +267,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;
}
Expand Down Expand Up @@ -450,8 +483,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
Expand Down Expand Up @@ -502,22 +533,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);
}
}
}
}
40 changes: 22 additions & 18 deletions lib/src/model/package_graph.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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;
}
Expand All @@ -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];
Expand Down Expand Up @@ -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
Expand Down
18 changes: 1 addition & 17 deletions lib/src/model/source_code_mixin.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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,
};
}
}
Loading

0 comments on commit c1c0854

Please sign in to comment.