Skip to content

Commit

Permalink
Refactor special case for how canonical enclosing containers are calc…
Browse files Browse the repository at this point in the history
…ulated

There is some complicated logic in the calculations for determining a canonical
enclosing container, when we find ourselves examining a "hidden" interface. The
list of "hidden" interfaces, which we should not consider when determining a
canonical enclosing container, is exactly one: `Interceptor`. But it's
important that it stay buried.

The logic was hard to read, so first, I split out a complicated if-condition
into a few local variables. The logic also needs to track the "current"
container in the inheritance list, and the "previous", and the "previous
visible." This was complicated when using a for-in loop. I changed it to use a
standard for loop with indices.

Then I saw that we only ever find outself asking a container for it's
"memberByExample" in the case when we're looking for one of Object's members
(`toString`, `noSuchMethod`, `operator ==`, `hashCode`, and `runtimeType`. We
can inline the logic to fetch a member by example. This allows us to delete a
late field on every Container.

Also, the package graph `_invisibleAnnotations` and `_inheritThrough` fields
had very complicated initializers which I simplified with if-elements.
  • Loading branch information
srawlins committed Nov 11, 2024
1 parent 73344a2 commit 6051dd2
Show file tree
Hide file tree
Showing 4 changed files with 55 additions and 67 deletions.
13 changes: 6 additions & 7 deletions lib/src/model/comment_referable.dart
Original file line number Diff line number Diff line change
Expand Up @@ -223,14 +223,13 @@ extension on Scope {
/// A set of utility methods for helping build
/// [CommentReferable.referenceChildren] out of collections of other
/// [CommentReferable]s.
extension CommentReferableEntryGenerators on Iterable<CommentReferable> {
extension CommentReferableEntryGenerators<T extends CommentReferable>
on Iterable<T> {
/// Creates reference entries for this Iterable.
///
/// If there is a conflict with [referable], the included [MapEntry] uses
/// [referable]'s [CommentReferable.referenceName] as a prefix.
Map<String, CommentReferable> explicitOnCollisionWith(
CommentReferable referable) =>
{
Map<String, T> explicitOnCollisionWith(CommentReferable referable) => {
for (var r in this)
if (r.referenceName == referable.referenceName)
'${referable.referenceName}.${r.referenceName}': r
Expand All @@ -239,13 +238,13 @@ extension CommentReferableEntryGenerators on Iterable<CommentReferable> {
};

/// A mapping from each [CommentReferable]'s name to itself.
Map<String, CommentReferable> get asMapByName => {
Map<String, T> get asMapByName => {
for (var r in this) r.referenceName: r,
};

/// Returns all values not of this type.
List<CommentReferable> whereNotType<T>() => [
List<T> whereNotType<U>() => [
for (var referable in this)
if (referable is! T) referable,
if (referable is! U) referable,
];
}
27 changes: 0 additions & 27 deletions lib/src/model/container.dart
Original file line number Diff line number Diff line change
Expand Up @@ -156,33 +156,6 @@ abstract class Container extends ModelElement
late final Set<Element> _allElements =
allModelElements.map((e) => e.element).toSet();

late final Map<String, List<ModelElement>> _membersByName = () {
var membersByName = <String, List<ModelElement>>{};
for (var element in allModelElements) {
membersByName.putIfAbsent(element.name, () => []).add(element);
}
return membersByName;
}();

/// Given a [ModelElement] that is a member of some other class, returns
/// the member of this class that has the same name and runtime type.
///
/// This enables object substitution for canonicalization, such as Interceptor
/// for Object.
T memberByExample<T extends ModelElement>(T example) {
// [T] is insufficiently specific to disambiguate between different
// subtypes of [Inheritable] or other mixins/implementations of
// [ModelElement] via [Iterable.whereType].
var possibleMembers = _membersByName[example.name]!
.where((e) => e.runtimeType == example.runtimeType);
if (example is Accessor) {
possibleMembers = possibleMembers
.where((e) => example.isGetter == (e as Accessor).isGetter);
}
assert(possibleMembers.length == 1);
return possibleMembers.first as T;
}

bool get hasPublicStaticFields => staticFields.any((e) => e.isPublic);

List<Field> get publicStaticFieldsSorted =>
Expand Down
53 changes: 35 additions & 18 deletions lib/src/model/inheritable.dart
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import 'package:analyzer/dart/element/element.dart';
import 'package:collection/collection.dart' show IterableExtension;
import 'package:dartdoc/src/model/attribute.dart';
import 'package:dartdoc/src/model/comment_referable.dart';
import 'package:dartdoc/src/model/model.dart';
import 'package:dartdoc/src/special_elements.dart';

Expand Down Expand Up @@ -55,22 +56,40 @@ mixin Inheritable on ContainerMember {
var searchElement = element.declaration;
// TODO(jcollins-g): generate warning if an inherited element's definition
// is in an intermediate non-canonical class in the inheritance chain?
Container? previous;
Container? previousNonSkippable;
Container? found;
for (var c in _inheritance.reversed) {
// Filter out mixins.
if (c.containsElement(searchElement)) {
if ((packageGraph.inheritThrough.contains(previous) &&
c != definingEnclosingContainer) ||
(packageGraph.inheritThrough.contains(c) &&
c == definingEnclosingContainer)) {
return previousNonSkippable!
.memberByExample(this)
.canonicalEnclosingContainer;
var reverseInheritance = _inheritance.reversed.toList();
for (var i = 0; i < reverseInheritance.length; i++) {
var container = reverseInheritance[i];
if (container.containsElement(searchElement)) {
var previousIsHiddenAndNotDefining = i > 0 &&
_isHiddenInterface(reverseInheritance[i - 1]) &&
container != definingEnclosingContainer;
var thisIsHiddenAndDefining = _isHiddenInterface(container) &&
container == definingEnclosingContainer;
// If the previous container in the search is one of the "hidden"
// interfaces, and it's not this member's defining container, OR if
// this container in the search is one of the "hidden" interfaces,
// and it is also this member's defining container, then we can just
// immediately return the canonical enclosing container of the
// overridden member in the previous, non-hidden container in the
// inheritance.
if (previousIsHiddenAndNotDefining || thisIsHiddenAndDefining) {
var previousVisible = reverseInheritance
.take(i)
.lastWhere((e) => !_isHiddenInterface(e));
var membersInPreviousVisible = previousVisible.allModelElements
.where((e) => e.name == name)
.whereType<Inheritable>()
.whereNotType<Field>();
assert(
membersInPreviousVisible.length == 1,
'found multiple members named "$name" in '
'"${previousVisible.name}": '
'${membersInPreviousVisible.toList()}');
return membersInPreviousVisible.first.canonicalEnclosingContainer;
}
var canonicalContainer =
packageGraph.findCanonicalModelElementFor(c) as Container?;
var canonicalContainer = packageGraph
.findCanonicalModelElementFor(container) as Container?;
// TODO(jcollins-g): invert this lookup so traversal is recursive
// starting from the ModelElement.
if (canonicalContainer != null) {
Expand All @@ -80,10 +99,6 @@ mixin Inheritable on ContainerMember {
break;
}
}
previous = c;
if (!packageGraph.inheritThrough.contains(c)) {
previousNonSkippable = c;
}
}
if (found != null) {
return found;
Expand All @@ -96,6 +111,8 @@ mixin Inheritable on ContainerMember {
return super.computeCanonicalEnclosingContainer();
}

bool _isHiddenInterface(Container? c) => packageGraph.isHiddenInterface(c);

/// A roughly ordered list of this element's enclosing container's inheritance
/// chain.
///
Expand Down
29 changes: 14 additions & 15 deletions lib/src/model/package_graph.dart
Original file line number Diff line number Diff line change
Expand Up @@ -706,29 +706,28 @@ class PackageGraph with CommentReferable, Nameable {
?.linkedName ??
'Object';

/// The set of [Class]es which should _not_ be presented as implementers.
/// The set of [Class]es which should _not_ be considered the canonical
/// enclosing container of any container member.
///
/// Add classes here if they are similar to Interceptor in that they are to be
/// ignored even when they are the implementers of [Inheritable]s, and the
/// class these inherit from should instead claim implementation.
late final Set<Class> inheritThrough = () {
var interceptorSpecialClass = specialClasses[SpecialClass.interceptor];
if (interceptorSpecialClass == null) {
return const <Class>{};
}
late final Set<Class> _inheritThrough = {
if (specialClasses[SpecialClass.interceptor] case var interceptor?)
interceptor,
};

return {interceptorSpecialClass};
}();
/// Whether [c] is a "hidden" interface.
///
/// A hidden interface should never be considered the canonical enclosing
/// container of a container member.
bool isHiddenInterface(Container? c) => _inheritThrough.contains(c);

/// The set of [Class] objects that are similar to 'pragma' in that we should
/// never count them as documentable annotations.
late final Set<Class> _invisibleAnnotations = () {
var pragmaSpecialClass = specialClasses[SpecialClass.pragma];
if (pragmaSpecialClass == null) {
return const <Class>{};
}
return {pragmaSpecialClass};
}();
late final Set<Class> _invisibleAnnotations = {
if (specialClasses[SpecialClass.pragma] case var pragma?) pragma,
};

bool isAnnotationVisible(Class class_) =>
!_invisibleAnnotations.contains(class_);
Expand Down

0 comments on commit 6051dd2

Please sign in to comment.