From c821eb3fce067fcbeca733c8fa06b1788c739e77 Mon Sep 17 00:00:00 2001 From: Sam Rawlins Date: Tue, 27 Feb 2024 17:44:38 -0800 Subject: [PATCH 1/2] Simplify InheritingContainer.allModelElements and _inheritedElements --- lib/src/model/inheriting_container.dart | 73 +++++++++++++------------ 1 file changed, 38 insertions(+), 35 deletions(-) diff --git a/lib/src/model/inheriting_container.dart b/lib/src/model/inheriting_container.dart index 5ca54ac93a..ab5f8f743e 100644 --- a/lib/src/model/inheriting_container.dart +++ b/lib/src/model/inheriting_container.dart @@ -103,15 +103,10 @@ abstract class InheritingContainer extends Container .asLanguageFeatureSet(const LanguageFeatureRendererHtml()) .toList(); - late final List _allModelElements = () { - _inheritedElementsCache = _inheritedElements; - var result = [ - ...super.allModelElements, - ...typeParameters, - ]; - _inheritedElementsCache = null; - return result; - }(); + late final List _allModelElements = [ + ...super.allModelElements, + ...typeParameters, + ]; Iterable get inheritedMethods { var methodNames = declaredMethods.map((m) => m.element.name).toSet(); @@ -148,55 +143,63 @@ abstract class InheritingContainer extends Container late final List publicSuperChain = model_utils.filterNonPublic(superChain).toList(growable: false); - List? _inheritedElementsCache; - List get _inheritedElements { - if (_inheritedElementsCache != null) return _inheritedElementsCache!; + /// A list of the inherited executable elements, one element per inherited + /// `Name`. + /// + /// In this list, elements that are "closer" in the inheritance chain to + /// _this_ element are preferred over elements that are further away. In the + /// case of ties, concrete inherited elements are prefered to non-concrete + /// ones. + late final List _inheritedElements = () { if (element is ClassElement && (element as ClassElement).isDartCoreObject) { return const []; } - final concreteInheritenceMap = + var concreteInheritanceMap = packageGraph.inheritanceManager.getInheritedConcreteMap2(element); - final inheritenceMap = + var inheritanceMap = packageGraph.inheritanceManager.getInheritedMap2(element); - List? inheritanceChainElements; + var inheritanceChainElements = + inheritanceChain.map((c) => c.element).toList(growable: false); - final combinedMap = { - for (final name in concreteInheritenceMap.keys) - name.name: concreteInheritenceMap[name]!, + // A combined map of names to inherited _concrete_ Elements, and other + // inherited Elements. + var combinedMap = { + for (var MapEntry(:key, :value) in concreteInheritanceMap.entries) + key.name: value, }; - for (final name in inheritenceMap.keys) { - final inheritenceElement = inheritenceMap[name]!; - final combinedMapElement = combinedMap[name.name]; + for (var MapEntry(key: name, value: inheritedElement) + in inheritanceMap.entries) { + var combinedMapElement = combinedMap[name.name]; if (combinedMapElement == null) { - combinedMap[name.name] = inheritenceElement; + combinedMap[name.name] = inheritedElement; continue; } - // Elements in the inheritance chain starting from `this.element` down to, - // but not including, [Object]. - inheritanceChainElements ??= - inheritanceChain.map((c) => c.element).toList(growable: false); - final enclosingElement = - inheritenceElement.enclosingElement as InterfaceElement; + // Elements in the inheritance chain starting from `this.element` up to, + // but not including, `Object`. + var enclosingElement = + inheritedElement.enclosingElement as InterfaceElement; assert(inheritanceChainElements.contains(enclosingElement) || enclosingElement.isDartCoreObject); - // If the concrete object from - // [InheritanceManager3.getInheritedConcreteMap2] is farther from this - // class in the inheritance chain than the one provided by + // If the concrete object from `getInheritedConcreteMap2` is farther in + // the inheritance chain from this class than the one provided by // `inheritedMap2`, prefer `inheritedMap2`. This correctly accounts for // intermediate abstract classes that have method/field implementations. - if (inheritanceChainElements.indexOf( - combinedMapElement.enclosingElement as InterfaceElement) < + var enclosingElementFromCombined = + combinedMapElement.enclosingElement as InterfaceElement; + if (inheritanceChainElements.indexOf(enclosingElementFromCombined) < inheritanceChainElements.indexOf(enclosingElement)) { - combinedMap[name.name] = inheritenceElement; + combinedMap[name.name] = inheritedElement; } } + // Finally, return all of the elements ultimately collected in the combined + // map. return combinedMap.values.toList(growable: false); - } + }(); /// All fields defined on this container, _including inherited fields_. late List allFields = () { From 5fdfee55da7e3a4773209bcc5dccac6cafe36614 Mon Sep 17 00:00:00 2001 From: Sam Rawlins Date: Wed, 28 Feb 2024 08:26:46 -0800 Subject: [PATCH 2/2] feedback --- lib/src/model/inheriting_container.dart | 31 ++++++++++++++++--------- lib/src/model/mixin.dart | 4 +--- 2 files changed, 21 insertions(+), 14 deletions(-) diff --git a/lib/src/model/inheriting_container.dart b/lib/src/model/inheriting_container.dart index ab5f8f743e..a8bc4acbfc 100644 --- a/lib/src/model/inheriting_container.dart +++ b/lib/src/model/inheriting_container.dart @@ -151,12 +151,17 @@ abstract class InheritingContainer extends Container /// case of ties, concrete inherited elements are prefered to non-concrete /// ones. late final List _inheritedElements = () { - if (element is ClassElement && (element as ClassElement).isDartCoreObject) { + if (element case ClassElement classElement + when classElement.isDartCoreObject) { return const []; } + // The mapping of all of the inherited element names to their _concrete_ + // implementation element. var concreteInheritanceMap = packageGraph.inheritanceManager.getInheritedConcreteMap2(element); + // The mapping of all inherited element names to the nearest inherited + // element that they resolve to. var inheritanceMap = packageGraph.inheritanceManager.getInheritedMap2(element); @@ -184,10 +189,11 @@ abstract class InheritingContainer extends Container assert(inheritanceChainElements.contains(enclosingElement) || enclosingElement.isDartCoreObject); - // If the concrete object from `getInheritedConcreteMap2` is farther in - // the inheritance chain from this class than the one provided by - // `inheritedMap2`, prefer `inheritedMap2`. This correctly accounts for - // intermediate abstract classes that have method/field implementations. + // If the concrete element from `getInheritedConcreteMap2` is farther in + // the inheritance chain from this class than the (non-concrete) one + // provided by `getInheritedMap2`, prefer the latter. This correctly + // accounts for intermediate abstract classes that have method/field + // implementations. var enclosingElementFromCombined = combinedMapElement.enclosingElement as InterfaceElement; if (inheritanceChainElements.indexOf(enclosingElementFromCombined) < @@ -314,12 +320,13 @@ abstract class InheritingContainer extends Container bool get hasPublicSuperChainReversed => publicSuperChainReversed.isNotEmpty; - /// Not the same as [superChain] as it may include mixins. + /// A sorted list of [element]'s inheritance chain, including interfaces and + /// mixins. /// - /// It's really not even the same as ordinary Dart inheritance, either, + /// Note: this list is really not even the same as ordinary Dart inheritance, /// because we pretend that interfaces are part of the inheritance chain /// to include them in the set of things we might link to for documentation - /// purposes in abstract classes. + /// purposes. List get inheritanceChain; @visibleForTesting @@ -382,6 +389,8 @@ abstract class InheritingContainer extends Container Iterable get publicSuperChainReversed => publicSuperChain.reversed; + /// The chain of super-types, starting with [supertype], up to, but not + /// including, `Object`. List get superChain { var typeChain = []; var parent = supertype; @@ -389,12 +398,12 @@ abstract class InheritingContainer extends Container typeChain.add(parent); final parentType = parent.type; if (parentType is! InterfaceType) { - throw StateError('ancestor of $this is $parent with model element ' - '${parent.modelElement}'); + throw StateError("ancestor of '$this' is '$parent' with model element " + "'${parent.modelElement}'"); } var superclass = parentType.superclass; - // Avoid adding [Object] to the [superChain] ([_supertype] already has + // Avoid adding `Object` to the `superChain` (`_supertype` already has // this check). if (superclass == null || superclass.superclass == null) { break; diff --git a/lib/src/model/mixin.dart b/lib/src/model/mixin.dart index ade838bf30..563f7b8de3 100644 --- a/lib/src/model/mixin.dart +++ b/lib/src/model/mixin.dart @@ -29,14 +29,12 @@ class Mixin extends InheritingContainer with TypeImplementing { @override late final List inheritanceChain = [ this, - - // Mix-in interfaces come before other interfaces. ...superclassConstraints.modelElements.expandInheritanceChain, for (var container in superChain.modelElements) ...container.inheritanceChain, - // Interfaces need to come last, because classes in the superChain might + // Interfaces need to come last, because classes in the `superChain` might // implement them even when they aren't mentioned. ...interfaceElements.expandInheritanceChain, ];