Skip to content

Commit

Permalink
Refactor canonicalization more
Browse files Browse the repository at this point in the history
  • Loading branch information
srawlins committed Oct 11, 2023
1 parent 2943545 commit 99c317a
Show file tree
Hide file tree
Showing 15 changed files with 75 additions and 72 deletions.
5 changes: 3 additions & 2 deletions lib/src/generator/templates.aot_renderers_for_html.dart
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import 'dart:convert';

import 'package:dartdoc/src/generator/template_data.dart';
import 'package:dartdoc/src/model/accessor.dart';
import 'package:dartdoc/src/model/canonicalization.dart';
import 'package:dartdoc/src/model/category.dart';
import 'package:dartdoc/src/model/class.dart';
import 'package:dartdoc/src/model/constructor.dart';
Expand All @@ -36,7 +37,6 @@ import 'package:dartdoc/src/model/operator.dart';
import 'package:dartdoc/src/model/package.dart';
import 'package:dartdoc/src/model/top_level_variable.dart';
import 'package:dartdoc/src/model/typedef.dart';
import 'package:dartdoc/src/warnings.dart';

String renderCategory(CategoryTemplateData context0) {
final buffer = StringBuffer();
Expand Down Expand Up @@ -3323,7 +3323,8 @@ String _deduplicated_lib_templates_html__head_html(TemplateDataBase context0) {
return buffer.toString();
}

String _deduplicated_lib_templates_html__documentation_html(Warnable context0) {
String _deduplicated_lib_templates_html__documentation_html(
Canonicalization context0) {
final buffer = StringBuffer();
if (context0.hasDocumentation == true) {
buffer.writeln();
Expand Down
5 changes: 3 additions & 2 deletions lib/src/generator/templates.aot_renderers_for_md.dart
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import 'dart:convert';

import 'package:dartdoc/src/generator/template_data.dart';
import 'package:dartdoc/src/model/accessor.dart';
import 'package:dartdoc/src/model/canonicalization.dart';
import 'package:dartdoc/src/model/category.dart';
import 'package:dartdoc/src/model/class.dart';
import 'package:dartdoc/src/model/constructor.dart';
Expand All @@ -35,7 +36,6 @@ import 'package:dartdoc/src/model/operator.dart';
import 'package:dartdoc/src/model/package.dart';
import 'package:dartdoc/src/model/top_level_variable.dart';
import 'package:dartdoc/src/model/typedef.dart';
import 'package:dartdoc/src/warnings.dart';

String renderCategory(CategoryTemplateData context0) {
final buffer = StringBuffer();
Expand Down Expand Up @@ -1761,7 +1761,8 @@ String _deduplicated_lib_templates_md__head_md(TemplateDataBase context0) {
return buffer.toString();
}

String _deduplicated_lib_templates_md__documentation_md(Warnable context0) {
String _deduplicated_lib_templates_md__documentation_md(
Canonicalization context0) {
final buffer = StringBuffer();
if (context0.hasDocumentation == true) {
buffer.writeln();
Expand Down
1 change: 0 additions & 1 deletion lib/src/generator/templates.runtime_renderers.dart
Original file line number Diff line number Diff line change
Expand Up @@ -4003,7 +4003,6 @@ class _Renderer_Documentable extends RendererBase<Documentable> {
_propertyMapCache.putIfAbsent(
CT_,
() => {
..._Renderer_Nameable.propertyMap<CT_>(),
'config': Property(
getValue: (CT_ c) => c.config,
renderVariable: (CT_ c, Property<CT_> self,
Expand Down
61 changes: 47 additions & 14 deletions lib/src/model/canonicalization.dart
Original file line number Diff line number Diff line change
Expand Up @@ -3,54 +3,87 @@
// BSD-style license that can be found in the LICENSE file.

import 'package:dartdoc/src/model/model.dart';
import 'package:dartdoc/src/warnings.dart';

/// Classes extending this class have canonicalization support in Dartdoc.
abstract mixin class Canonicalization implements Locatable, Documentable {
///
/// This provides heuristic scoring to determine which library a human likely
/// considers this element to be primarily 'from', and therefore, canonical.
/// Still warn if the heuristic isn't very confident.
abstract mixin class Canonicalization
implements Locatable, Documentable, Warnable {
bool get isCanonical;

/// Pieces of the location, split to remove 'package:' and slashes.
Set<String> get locationPieces;

List<ScoredCandidate> scoreCanonicalCandidates(Iterable<Library> libraries) {
return libraries.map(_scoreElementWithLibrary).toList(growable: false)
Library calculateCanonicalCandidate(Iterable<Library> libraries) {
var scoredCandidates = libraries
.map((library) => _scoreElementWithLibrary(
library, fullyQualifiedName, locationPieces))
.toList(growable: false)
..sort();

final librariesByScore = scoredCandidates.map((s) => s.library).toList();
var secondHighestScore =
scoredCandidates[scoredCandidates.length - 2].score;
var highestScore = scoredCandidates.last.score;
var confidence = highestScore - secondHighestScore;
final canonicalLibrary = librariesByScore.last;

if (confidence < config.ambiguousReexportScorerMinConfidence) {
var libraryNames = librariesByScore.map((l) => l.name);
var message = '$libraryNames -> ${canonicalLibrary.name} '
'(confidence ${confidence.toStringAsPrecision(4)})';
warn(PackageWarning.ambiguousReexport,
message: message, extendedDebug: scoredCandidates.map((s) => '$s'));
}

return canonicalLibrary;
}

ScoredCandidate _scoreElementWithLibrary(Library library) {
var scoredCandidate = ScoredCandidate(library);
static _ScoredCandidate _scoreElementWithLibrary(Library library,
String elementQualifiedName, Set<String> elementLocationPieces) {
var scoredCandidate = _ScoredCandidate(library);

// Large boost for `@canonicalFor`, essentially overriding all other
// concerns.
if (library.canonicalFor.contains(fullyQualifiedName)) {
if (library.canonicalFor.contains(elementQualifiedName)) {
scoredCandidate._alterScore(5.0, _Reason.canonicalFor);
}

// Penalty for deprecated libraries.
if (library.isDeprecated) {
scoredCandidate._alterScore(-1.0, _Reason.deprecated);
}

// Give a big boost if the library has the package name embedded in it.
if (library.package.namePieces.intersection(library.namePieces).isEmpty) {
scoredCandidate._alterScore(1.0, _Reason.packageName);
}

// Give a tiny boost for libraries with long names, assuming they're
// more specific (and therefore more likely to be the owner of this symbol).
scoredCandidate._alterScore(
.01 * library.namePieces.length, _Reason.longName);

// If we don't know the location of this element (which shouldn't be
// possible), return our best guess.
assert(locationPieces.isNotEmpty);
if (locationPieces.isEmpty) return scoredCandidate;
assert(elementLocationPieces.isNotEmpty);
if (elementLocationPieces.isEmpty) return scoredCandidate;

// The more pieces we have of the location in our library name, the more we
// should boost our score.
scoredCandidate._alterScore(
library.namePieces.intersection(locationPieces).length.toDouble() /
locationPieces.length.toDouble(),
library.namePieces.intersection(elementLocationPieces).length.toDouble() /
elementLocationPieces.length.toDouble(),
_Reason.sharedNamePart,
);

// If pieces of location at least start with elements of our library name,
// boost the score a little bit.
var scoreBoost = 0.0;
for (var piece in locationPieces.expand((item) => item.split('_'))) {
for (var piece in elementLocationPieces.expand((item) => item.split('_'))) {
for (var namePiece in library.namePieces) {
if (piece.startsWith(namePiece)) {
scoreBoost += 0.001;
Expand All @@ -64,7 +97,7 @@ abstract mixin class Canonicalization implements Locatable, Documentable {

/// This class represents the score for a particular element; how likely
/// it is that this is the canonical element.
class ScoredCandidate implements Comparable<ScoredCandidate> {
class _ScoredCandidate implements Comparable<_ScoredCandidate> {
final List<(_Reason, double)> _reasons = [];

final Library library;
Expand All @@ -73,7 +106,7 @@ class ScoredCandidate implements Comparable<ScoredCandidate> {
/// is the intended canonical Library.
double score = 0.0;

ScoredCandidate(this.library);
_ScoredCandidate(this.library);

void _alterScore(double scoreDelta, _Reason reason) {
score += scoreDelta;
Expand All @@ -83,7 +116,7 @@ class ScoredCandidate implements Comparable<ScoredCandidate> {
}

@override
int compareTo(ScoredCandidate other) => score.compareTo(other.score);
int compareTo(_ScoredCandidate other) => score.compareTo(other.score);

@override
String toString() {
Expand Down
4 changes: 2 additions & 2 deletions lib/src/model/documentable.dart
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import 'model.dart';

/// Bridges the gap between model elements and packages,
/// both of which have documentation.
abstract class Documentable extends Nameable {
mixin Documentable on Nameable {
String? get documentation;

String get documentationAsHtml;
Expand Down Expand Up @@ -77,5 +77,5 @@ mixin MarkdownFileDocumentation implements Documentable, Canonicalization {
String get location => '(${documentationFile?.path})';

@override
Set<String> get locationPieces => <String>{location};
Set<String> get locationPieces => {location};
}
7 changes: 2 additions & 5 deletions lib/src/model/documentation.dart
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
import 'package:dartdoc/src/markdown_processor.dart';
import 'package:dartdoc/src/model/model.dart';
import 'package:dartdoc/src/render/documentation_renderer.dart';
import 'package:dartdoc/src/warnings.dart';
import 'package:markdown/markdown.dart' as md;

class Documentation {
Expand Down Expand Up @@ -74,10 +73,8 @@ class Documentation {
if (text == null || text.isEmpty) {
return const [];
}
showWarningsForGenericsOutsideSquareBracketsBlocks(
text, _element as Warnable);
var document =
MarkdownDocument.withElementLinkResolver(_element as Warnable);
showWarningsForGenericsOutsideSquareBracketsBlocks(text, _element);
var document = MarkdownDocument.withElementLinkResolver(_element);
return document.parseMarkdownText(text, processFullText: processFullText);
}

Expand Down
4 changes: 3 additions & 1 deletion lib/src/model/documentation_comment.dart
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import 'package:analyzer/dart/element/element.dart';
import 'package:args/args.dart';
import 'package:crypto/crypto.dart' as crypto;
import 'package:dartdoc/src/model/canonicalization.dart';
import 'package:dartdoc/src/model/documentable.dart';
import 'package:dartdoc/src/model/documentation.dart';
import 'package:dartdoc/src/model/inheritable.dart';
Expand Down Expand Up @@ -36,7 +37,8 @@ final _htmlInjectRegExp = RegExp(r'<dartdoc-html>([a-f0-9]+)</dartdoc-html>');
///
/// [_processCommentWithoutTools] and [processComment] are the primary
/// entrypoints.
mixin DocumentationComment on Documentable, Warnable, Locatable, SourceCode {
mixin DocumentationComment
on Canonicalization, Documentable, Warnable, Locatable, SourceCode {
@override
Element get element;

Expand Down
1 change: 0 additions & 1 deletion lib/src/model/enum.dart
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,6 @@ class EnumField extends Field {
if (!identical(canonicalModelElement, this)) {
return canonicalModelElement?.href;
}
assert(!(canonicalLibrary == null || canonicalEnclosingContainer == null));
assert(canonicalLibrary == library);
assert(canonicalEnclosingContainer == enclosingElement);
// TODO(jcollins-g): EnumField should not depend on enclosingElement, but
Expand Down
44 changes: 9 additions & 35 deletions lib/src/model/model_element.dart
Original file line number Diff line number Diff line change
Expand Up @@ -384,10 +384,6 @@ abstract class ModelElement extends Canonicalization
// Stub for mustache.
Iterable<Category?> get displayedCategories => const [];

Set<Library>? get _exportedInLibraries {
return library.packageGraph.libraryExports[element.library!];
}

@override
late final ModelNode? modelNode = packageGraph.getModelNodeFor(element);

Expand Down Expand Up @@ -487,7 +483,7 @@ abstract class ModelElement extends Canonicalization
return null;
}

// This is not accurate if we are constructing the Package.
// This is not accurate if we are still constructing the Package.
assert(packageGraph.allLibrariesAdded);

var definingLibraryIsLocalPublic =
Expand All @@ -512,8 +508,7 @@ abstract class ModelElement extends Canonicalization
}();

Library? _searchForCanonicalLibrary() {
var thisAndExported = definingLibrary._exportedInLibraries;

var thisAndExported = packageGraph.libraryExports[definingLibrary.element];
if (thisAndExported == null) {
return null;
}
Expand All @@ -533,10 +528,10 @@ abstract class ModelElement extends Canonicalization
.where((l) {
var lookup =
l.element.exportNamespace.definedNames[topLevelElement.name!];
if (lookup is PropertyAccessorElement) {
lookup = lookup.variable;
}
return topLevelElement == lookup;
return switch (lookup) {
PropertyAccessorElement() => topLevelElement == lookup.variable,
_ => topLevelElement == lookup,
};
}).toList(growable: true);

// Avoid claiming canonicalization for elements outside of this element's
Expand All @@ -559,29 +554,9 @@ abstract class ModelElement extends Canonicalization
return candidateLibraries.single;
}

// Start with our top-level element.
var warnable = ModelElement._fromElement(topLevelElement, packageGraph);
// Heuristic scoring to determine which library a human likely
// considers this element to be primarily 'from', and therefore,
// canonical. Still warn if the heuristic isn't that confident.
var scoredCandidates =
warnable.scoreCanonicalCandidates(candidateLibraries);
final librariesByScore = scoredCandidates.map((s) => s.library).toList();
var secondHighestScore =
scoredCandidates[scoredCandidates.length - 2].score;
var highestScore = scoredCandidates.last.score;
var confidence = highestScore - secondHighestScore;
final canonicalLibrary = librariesByScore.last;

if (confidence < config.ambiguousReexportScorerMinConfidence) {
var libraryNames = librariesByScore.map((l) => l.name);
var message = '$libraryNames -> ${canonicalLibrary.name} '
'(confidence ${confidence.toStringAsPrecision(4)})';
warnable.warn(PackageWarning.ambiguousReexport,
message: message, extendedDebug: scoredCandidates.map((s) => '$s'));
}

return canonicalLibrary;
var topLevelModelElement =
ModelElement._fromElement(topLevelElement, packageGraph);
return topLevelModelElement.calculateCanonicalCandidate(candidateLibraries);
}

@override
Expand Down Expand Up @@ -684,7 +659,6 @@ abstract class ModelElement extends Canonicalization
if (!identical(canonicalModelElement, this)) {
return canonicalModelElement?.href;
}
assert(canonicalLibrary != null);
assert(canonicalLibrary == library);
var packageBaseHref = package.baseHref;
return '$packageBaseHref$filePath';
Expand Down
1 change: 0 additions & 1 deletion lib/src/model/model_function.dart
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,6 @@ class ModelFunctionTyped extends ModelElement
if (!identical(canonicalModelElement, this)) {
return canonicalModelElement?.href;
}
assert(canonicalLibrary != null);
assert(canonicalLibrary == library);
return '${package.baseHref}$filePath';
}
Expand Down
1 change: 0 additions & 1 deletion lib/src/model/package.dart
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,6 @@ class Package extends LibraryContainer
// object that contains them.
Map<String?, Set<String>> usedAnimationIdsByHref = {};

/// Pieces of the location, split to remove 'package:' and slashes.
@override
Set<String> get locationPieces => const {};

Expand Down
1 change: 0 additions & 1 deletion lib/src/model/top_level_variable.dart
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,6 @@ class TopLevelVariable extends ModelElement
if (!identical(canonicalModelElement, this)) {
return canonicalModelElement?.href;
}
assert(canonicalLibrary != null);
assert(canonicalLibrary == library);
return '${package.baseHref}$filePath';
}
Expand Down
1 change: 0 additions & 1 deletion lib/src/model/typedef.dart
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,6 @@ abstract class Typedef extends ModelElement
if (!identical(canonicalModelElement, this)) {
return canonicalModelElement?.href;
}
assert(canonicalLibrary != null);
assert(canonicalLibrary == library);
return '${package.baseHref}$filePath';
}
Expand Down
7 changes: 4 additions & 3 deletions lib/src/validator.dart
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import 'dart:convert';
import 'package:analyzer/file_system/file_system.dart';
import 'package:dartdoc/src/dartdoc_options.dart';
import 'package:dartdoc/src/logging.dart';
import 'package:dartdoc/src/model/canonicalization.dart';
import 'package:dartdoc/src/model/model_element.dart';
import 'package:dartdoc/src/model/package_graph.dart';
import 'package:dartdoc/src/runtime_stats.dart';
Expand Down Expand Up @@ -214,9 +215,9 @@ class Validator {
}) {
// Ordinarily this would go in [Package.warn], but we don't actually know
// what [ModelElement] to warn on yet.
Warnable? warnOnElement;
final referredFromElements = <Warnable>{};
Set<Warnable>? warnOnElements;
ModelElement? warnOnElement;
final referredFromElements = <Canonicalization>{};
Set<ModelElement>? warnOnElements;

// Make all paths relative to origin.
if (path.isWithin(origin, warnOn)) {
Expand Down
4 changes: 2 additions & 2 deletions lib/src/warnings.dart
Original file line number Diff line number Diff line change
Expand Up @@ -296,9 +296,9 @@ const Map<PackageWarning, PackageWarningDefinition> packageWarningDefinitions =
defaultWarningMode: PackageWarningMode.ignore),
};

/// Something that package warnings can be called on. Optionally associated
/// Something that package warnings can be reported on. Optionally associated
/// with an analyzer [element].
mixin Warnable implements Canonicalization, CommentReferable {
mixin Warnable implements CommentReferable, Documentable, Locatable {
Element? get element;

void warn(
Expand Down

0 comments on commit 99c317a

Please sign in to comment.