Skip to content

Simplify InheritingContainer.allModelElements and _inheritedElements #3689

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Feb 28, 2024
Merged

Simplify InheritingContainer.allModelElements and _inheritedElements #3689

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c821eb3
Simplify InheritingContainer.allModelElements and _inheritedElements
srawlins Feb 28, 2024
5fdfee5
feedback
srawlins Feb 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 38 additions & 35 deletions lib/src/model/inheriting_container.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,15 +103,10 @@ abstract class InheritingContainer extends Container
.asLanguageFeatureSet(const LanguageFeatureRendererHtml())
.toList();

late final List<ModelElement> _allModelElements = () {
_inheritedElementsCache = _inheritedElements;
var result = [
...super.allModelElements,
...typeParameters,
];
_inheritedElementsCache = null;
return result;
}();
late final List<ModelElement> _allModelElements = [
...super.allModelElements,
...typeParameters,
];

Iterable<Method> get inheritedMethods {
var methodNames = declaredMethods.map((m) => m.element.name).toSet();
Expand Down Expand Up @@ -148,55 +143,63 @@ abstract class InheritingContainer extends Container
late final List<DefinedElementType> publicSuperChain =
model_utils.filterNonPublic(superChain).toList(growable: false);

List<ExecutableElement>? _inheritedElementsCache;
List<ExecutableElement> 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<ExecutableElement> _inheritedElements = () {
if (element is ClassElement && (element as ClassElement).isDartCoreObject) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

    if (element case ClassElement classElement
        when classElement.isDartCoreObject) {
      return const <ExecutableElement>[];
    }

Maybe? Instead of casting.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it

return const <ExecutableElement>[];
}

final concreteInheritenceMap =
var concreteInheritanceMap =
packageGraph.inheritanceManager.getInheritedConcreteMap2(element);
final inheritenceMap =
var inheritanceMap =
packageGraph.inheritanceManager.getInheritedMap2(element);

List<InterfaceElement>? inheritanceChainElements;
var inheritanceChainElements =
inheritanceChain.map((c) => c.element).toList(growable: false);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also these three maps above are named so similarly. Can we add a comment on what the difference is between them?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugh, the doc comment on inheritanceChain only describes what it is not ("Not the same as [superChain] as it may include mixins."). OK so I've updated that comment. And some more. And then introduced some comments above these declarations.

I cannot think of better names; they're already long and I think making them longer would hurt readability. But I'm open to new name ideas.

FWIW I don't know why this algorithm was chosen for _inheritedElements, but I'm strongly preferring to leave it alone; it is likely depended on by Flutter. It might be documented in tests.


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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which is inheritedMap2? Is this comment outdated or am I missing something?
Or does this just mean inheritanceMap and "the one provided" == inheritedElement?
Maybe fix the comments to be a tad more clear.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, wrong name. I am referring to the getInheritedMap2 method. Updated.

// 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<Field> allFields = () {
Expand Down