Combine the two implementations of "library canonicalization" (#3781)

I was shocked to find that we had two mechanisms for determining an element's
"canonical library": `ModelElement.canonicalLibrary` and a
`PackageGraph._findCanonicalLibraryFor`. They were not quite the same
implementation, but the differences turn out to be uninteresting; the two
implementations can be combined. This change removes
`PackageGraph._findCanonicalLibraryFor`. This implementation was only used in
the package graph to find the canonical element for a given element. That code
can now just use `ModelElement.canonicalLibrary`. It requires a fair bit of
refactoring all over:

* `PackageGraph.findCanonicalModelElementFor` previously accepted an analyzer
  Element, instead of a ModelElement, but almost all of the call sites used
  `foo.element` or some other expression that could easily be replaced with an
  access to a ModelElement instead of the correlated Element.
* Document `ModelElement.definingLibrary`, `ModelElement.canonicalLibrary`,
  `ModelElement._searchForCanonicalLibrary`.
* Simplify `ContainerMember.computeCanonicalEnclosingContainer` to stop
  unnecessarily special-casing extensions and extension types.
* Simplify lookup component of `ModelElement._searchForCanonicalLibrary`.
* Simplity `ContainerMember.referenceGrandparentOverrides` to not use `sync*`.
* Move the top-level `findCanonicalFor` function out of `model_utils.dart` and
  into `inheriting_container.dart`, the only code that uses it anymore.

Author: srawlins

lib/src/element_type.dart

@@ -301,8 +301,7 @@ abstract class DefinedElementType extends ElementType {
   @override
   bool get isPublic {
     var canonicalClass =
-        packageGraph.findCanonicalModelElementFor(modelElement.element) ??
-            modelElement;
+        packageGraph.findCanonicalModelElementFor(modelElement) ?? modelElement;
     return canonicalClass.isPublic;
   }
 

lib/src/generator/templates.runtime_renderers.dart

@@ -3424,7 +3424,7 @@ class _Renderer_ContainerMember extends RendererBase<ContainerMember> {
                   renderVariable: (CT_ c, Property<CT_> self,
                           List<String> remainingNames) =>
                       self.renderSimpleVariable(
-                          c, remainingNames, 'Iterable<Library>'),
+                          c, remainingNames, 'List<Library>'),
                   renderIterable: (CT_ c, RendererBase<CT_> r,
                       List<MustachioNode> ast, StringSink sink) {
                     return c.referenceGrandparentOverrides.map((e) =>

lib/src/model/container_member.dart

@@ -40,16 +40,8 @@ mixin ContainerMember on ModelElement {
     return canonicalEnclosingContainer;
   }();
 
-  Container? computeCanonicalEnclosingContainer() {
-    final enclosingElement = this.enclosingElement;
-    return switch (enclosingElement) {
-      Extension() =>
-        packageGraph.findCanonicalModelElementFor(enclosingElement.element),
-      ExtensionType() =>
-        packageGraph.findCanonicalModelElementFor(enclosingElement.element),
-      _ => packageGraph.findCanonicalModelElementFor(element.enclosingElement),
-    } as Container?;
-  }
+  Container? computeCanonicalEnclosingContainer() =>
+      packageGraph.findCanonicalModelElementFor(enclosingElement) as Container?;
 
   @override
   // TODO(jcollins-g): dart-lang/dartdoc#2693.
@@ -63,13 +55,12 @@ mixin ContainerMember on ModelElement {
       ];
 
   @override
-  Iterable<Library> get referenceGrandparentOverrides sync* {
-    // TODO(jcollins-g): split Field documentation up between accessors
-    // and resolve the pieces with different scopes.  dart-lang/dartdoc#2693.
-    // Until then, just pretend we're handling this correctly.
-    yield (documentationFrom.first as ModelElement).definingLibrary;
-    // TODO(jcollins-g): Wean users off of depending on canonical library
-    // resolution. dart-lang/dartdoc#2696
-    if (canonicalLibrary != null) yield canonicalLibrary!;
-  }
+  List<Library> get referenceGrandparentOverrides =>
+      // TODO(jcollins-g): split Field documentation up between accessors
+      // and resolve the pieces with different scopes.  dart-lang/dartdoc#2693.
+      // Until then, just pretend we're handling this correctly.
+      [
+        (documentationFrom.first as ModelElement).definingLibrary,
+        (packageGraph.findCanonicalModelElementFor(this) ?? this).library,
+      ];
 }

lib/src/model/inheritable.dart

@@ -70,8 +70,8 @@ mixin Inheritable on ContainerMember {
                 .memberByExample(this)
                 .canonicalEnclosingContainer;
           }
-          var canonicalContainer = packageGraph
-              .findCanonicalModelElementFor(c.element) as Container?;
+          var canonicalContainer =
+              packageGraph.findCanonicalModelElementFor(c) as Container?;
           // TODO(jcollins-g): invert this lookup so traversal is recursive
           // starting from the ModelElement.
           if (canonicalContainer != null) {
@@ -100,7 +100,7 @@ mixin Inheritable on ContainerMember {
       }
     } else if (definingEnclosingContainer is! Extension) {
       // TODO(jcollins-g): factor out extension logic into [Extendable].
-      return packageGraph.findCanonicalModelElementFor(element.enclosingElement)
+      return packageGraph.findCanonicalModelElementFor(enclosingElement)
           as Container?;
     }
     return super.computeCanonicalEnclosingContainer();

lib/src/model/inheriting_container.dart

@@ -405,18 +405,28 @@ abstract class InheritingContainer extends Container {
       } else {
         var implementers = packageGraph.implementers[implementer];
         if (implementers != null) {
-          model_utils.findCanonicalFor(implementers).forEach(addToResult);
+          _findCanonicalFor(implementers).forEach(addToResult);
         }
       }
     }
 
     var immediateImplementers = packageGraph.implementers[this];
     if (immediateImplementers != null) {
-      model_utils.findCanonicalFor(immediateImplementers).forEach(addToResult);
+      _findCanonicalFor(immediateImplementers).forEach(addToResult);
     }
     return result.toList(growable: false)..sort(byName);
   }
 
+  /// Finds canonical classes for all classes in the iterable, if possible.
+  /// If a canonical class can not be found, returns the original class.
+  Iterable<InheritingContainer> _findCanonicalFor(
+      Iterable<InheritingContainer> containers) {
+    return containers.map((container) {
+      var canonical = packageGraph.findCanonicalModelElementFor(container);
+      return canonical as InheritingContainer? ?? container;
+    });
+  }
+
   bool get hasPublicInterfaces => publicInterfaces.isNotEmpty;
 
   List<InheritingContainer> get interfaceElements => [

lib/src/model/model_element.dart

@@ -439,17 +439,23 @@ abstract class ModelElement
       ExtensionType() => enclosingElement,
       _ => null,
     };
-    return packageGraph.findCanonicalModelElementFor(element,
+    return packageGraph.findCanonicalModelElementFor(this,
         preferredClass: preferredClass);
   }();
 
   bool get hasSourceHref => sourceHref.isNotEmpty;
 
   late final String sourceHref = SourceLinker.fromElement(this).href();
 
+  /// The library in which [element] is declared.
+  ///
+  /// This is different from [library] if this [ModelElement] is derived from
+  /// a library that exports [element] from another library.
   Library get definingLibrary =>
       getModelForElement(element.library!) as Library;
 
+  /// A public, documented library which exports this [ModelElement], ideally in
+  /// [definingLibrary]'s package.
   late final Library? canonicalLibrary = () {
     if (element.hasPrivateName) {
       // Privately named elements can never have a canonical library.
@@ -480,6 +486,8 @@ abstract class ModelElement
     return null;
   }();
 
+  /// Searches [PackageGraph.libraryExports] for a public, documented library
+  /// which exports this [ModelElement], ideally in [definingLibrary]'s package.
   Library? _searchForCanonicalLibrary() {
     var thisAndExported = packageGraph.libraryExports[definingLibrary.element];
     if (thisAndExported == null) {
@@ -494,17 +502,20 @@ abstract class ModelElement
         topLevelElement.enclosingElement != null) {
       topLevelElement = topLevelElement.enclosingElement!;
     }
+    var topLevelElementName = topLevelElement.name;
+    if (topLevelElementName == null) {
+      // Any member of an unnamed extension is not public, and has no
+      // canonical library.
+      return null;
+    }
 
     final candidateLibraries = thisAndExported
         .where((l) =>
             l.isPublic && l.package.documentedWhere != DocumentLocation.missing)
         .where((l) {
-      var lookup =
-          l.element.exportNamespace.definedNames[topLevelElement.name!];
-      return switch (lookup) {
-        PropertyAccessorElement() => topLevelElement == lookup.variable2,
-        _ => topLevelElement == lookup,
-      };
+      var lookup = l.element.exportNamespace.definedNames[topLevelElementName];
+      return topLevelElement ==
+          (lookup is PropertyAccessorElement ? lookup.variable2 : lookup);
     }).toList(growable: true);
 
     // Avoid claiming canonicalization for elements outside of this element's
@@ -612,8 +623,8 @@ abstract class ModelElement
 
   bool get hasParameters => parameters.isNotEmpty;
 
-  /// If [canonicalLibrary] (or [canonicalEnclosingElement], for [Inheritable]
-  /// subclasses) is null, this is null.
+  /// If [canonicalLibrary] (or [Inheritable.canonicalEnclosingContainer], for
+  /// [Inheritable] subclasses) is `null`, this is `null`.
   @override
   String? get href {
     if (!identical(canonicalModelElement, this)) {

lib/src/model/package_graph.dart

@@ -708,121 +708,108 @@ class PackageGraph with CommentReferable, Nameable {
     return buffer.toString();
   }
 
-  final Map<Element, Library?> _canonicalLibraryFor = {};
-
-  /// Tries to find a top level library that references this element.
-  Library? _findCanonicalLibraryFor(Element e) {
-    assert(allLibrariesAdded);
-    if (_canonicalLibraryFor.containsKey(e)) {
-      return _canonicalLibraryFor[e];
-    }
-    _canonicalLibraryFor[e] = null;
-
-    var searchElement = switch (e) {
-      PropertyAccessorElement() => e.variable2,
-      GenericFunctionTypeElement() => e.enclosingElement,
-      _ => e,
-    };
-    if (searchElement == null) return null;
-    for (var library in publicLibraries) {
-      var modelElements = library.modelElementsMap[searchElement];
-      if (modelElements != null) {
-        if (modelElements.any((element) => element.isCanonical)) {
-          return _canonicalLibraryFor[e] = library;
-        }
-      }
-    }
-    return _canonicalLibraryFor[e];
-  }
-
-  /// Tries to find a canonical [ModelElement] for this [e].  If we know this
-  /// element is related to a particular class, pass a [preferredClass] to
-  /// disambiguate.
+  /// Tries to find a canonical [ModelElement] for this [modelElement].  If we
+  /// know this element is related to a particular class, pass a
+  /// [preferredClass] to disambiguate.
   ///
   /// This doesn't know anything about [PackageGraph.inheritThrough] and
   /// probably shouldn't, so using it with [Inheritable]s without special casing
   /// is not advised.
-  ModelElement? findCanonicalModelElementFor(Element? e,
+  ///
+  /// This can return `null` in a few ways: if [modelElement] is `null`, or if
+  /// it has no canonical library, or if a possible canonical model element has
+  /// a `false` value for `isCanonical`.
+  ModelElement? findCanonicalModelElementFor(ModelElement? modelElement,
       {Container? preferredClass}) {
     assert(allLibrariesAdded);
-    if (e == null) return null;
+    if (modelElement == null) return null;
+    var element = modelElement.element;
     if (preferredClass != null) {
       var canonicalClass =
-          findCanonicalModelElementFor(preferredClass.element) as Container?;
+          findCanonicalModelElementFor(preferredClass) as Container?;
       if (canonicalClass != null) preferredClass = canonicalClass;
     }
-    var lib = _findCanonicalLibraryFor(e);
+    var lib = modelElement.canonicalLibrary;
     if (lib == null && preferredClass != null) {
-      lib = _findCanonicalLibraryFor(preferredClass.element);
+      lib = preferredClass.canonicalLibrary;
     }
     // For elements defined in extensions, they are canonical.
-    var enclosingElement = e.enclosingElement;
+    var enclosingElement = element.enclosingElement;
     if (enclosingElement is ExtensionElement) {
       lib ??= getModelForElement(enclosingElement.library) as Library?;
       // TODO(keertip): Find a better way to exclude members of extensions
       // when libraries are specified using the "--include" flag.
       if (lib != null && lib.isDocumented) {
-        return getModelFor(e, lib);
+        return getModelFor(element, lib);
       }
     }
     // TODO(jcollins-g): The data structures should be changed to eliminate
     // guesswork with member elements.
-    var declaration = e.declaration;
-    ModelElement? modelElement;
+    var declaration = element.declaration;
+    ModelElement? canonicalModelElement;
     if (declaration != null &&
-        (e is ClassMemberElement || e is PropertyAccessorElement)) {
-      e = declaration;
-      modelElement = _findCanonicalModelElementForAmbiguous(e, lib,
+        (element is ClassMemberElement || element is PropertyAccessorElement)) {
+      modelElement = getModelForElement(declaration);
+      element = modelElement.element;
+      canonicalModelElement = _findCanonicalModelElementForAmbiguous(
+          modelElement, lib,
           preferredClass: preferredClass as InheritingContainer?);
     } else {
       if (lib != null) {
-        if (e is PropertyInducingElement) {
-          var getter = e.getter != null ? getModelFor(e.getter!, lib) : null;
-          var setter = e.setter != null ? getModelFor(e.setter!, lib) : null;
-          modelElement = getModelForPropertyInducingElement(e, lib,
+        if (element is PropertyInducingElement) {
+          var getter =
+              element.getter != null ? getModelFor(element.getter!, lib) : null;
+          var setter =
+              element.setter != null ? getModelFor(element.setter!, lib) : null;
+          canonicalModelElement = getModelForPropertyInducingElement(
+              element, lib,
               getter: getter as Accessor?, setter: setter as Accessor?);
         } else {
-          modelElement = getModelFor(e, lib);
+          canonicalModelElement = getModelFor(element, lib);
         }
       }
-      assert(modelElement is! Inheritable);
-      if (modelElement != null && !modelElement.isCanonical) {
-        modelElement = null;
+      assert(canonicalModelElement is! Inheritable);
+      if (canonicalModelElement != null && !canonicalModelElement.isCanonical) {
+        canonicalModelElement = null;
       }
     }
     // Prefer fields and top-level variables.
-    if (e is PropertyAccessorElement && modelElement is Accessor) {
-      modelElement = modelElement.enclosingCombo;
+    if (element is PropertyAccessorElement &&
+        canonicalModelElement is Accessor) {
+      canonicalModelElement = canonicalModelElement.enclosingCombo;
     }
-    return modelElement;
+    return canonicalModelElement;
   }
 
-  ModelElement? _findCanonicalModelElementForAmbiguous(Element e, Library? lib,
+  ModelElement? _findCanonicalModelElementForAmbiguous(
+      ModelElement modelElement, Library? lib,
       {InheritingContainer? preferredClass}) {
+    var element = modelElement.element;
     var candidates = <ModelElement>{};
     if (lib != null) {
       var constructedWithKey = allConstructedModelElements[
-          ConstructedModelElementsKey(e, lib, null)];
+          ConstructedModelElementsKey(element, lib, null)];
       if (constructedWithKey != null) {
         candidates.add(constructedWithKey);
       }
       var constructedWithKeyWithClass = allConstructedModelElements[
-          ConstructedModelElementsKey(e, lib, preferredClass)];
+          ConstructedModelElementsKey(element, lib, preferredClass)];
       if (constructedWithKeyWithClass != null) {
         candidates.add(constructedWithKeyWithClass);
       }
       if (candidates.isEmpty) {
         candidates = {
-          ...?allInheritableElements[InheritableElementsKey(e, lib)]
+          ...?allInheritableElements[InheritableElementsKey(element, lib)]
               ?.where((me) => me.isCanonical),
         };
       }
     }
 
-    var canonicalClass = findCanonicalModelElementFor(e.enclosingElement);
+    var canonicalClass =
+        findCanonicalModelElementFor(modelElement.enclosingElement);
     if (canonicalClass is InheritingContainer) {
       candidates.addAll(canonicalClass.allCanonicalModelElements
-          .where((m) => m.element == e));
+          .where((m) => m.element == element));
     }
 
     var matches = {...candidates.where((me) => me.isCanonical)};

lib/src/model_utils.dart

@@ -52,16 +52,6 @@ Iterable<T> filterHasCanonical<T extends ModelElement>(
   return maybeHasCanonicalItems.where((me) => me.canonicalModelElement != null);
 }
 
-/// Finds canonical classes for all classes in the iterable, if possible.
-/// If a canonical class can not be found, returns the original class.
-Iterable<InheritingContainer> findCanonicalFor(
-    Iterable<InheritingContainer> containers) {
-  return containers.map((c) =>
-      c.packageGraph.findCanonicalModelElementFor(c.element)
-          as InheritingContainer? ??
-      c);
-}
-
 extension ElementExtension on Element {
   bool get hasPrivateName {
     final name = this.name;