Prevent private or non-canonical elements from ever generating links (#1589)

* First version

* dartfmt

* Add a test

Author: jcollins-g

lib/src/element_type.dart

@@ -21,6 +21,8 @@ class ElementType extends Privacy {
 
   DartType get type => _type;
 
+  Library get library => element.library;
+
   bool get isDynamic => _type.isDynamic;
 
   bool get isFunctionType => (_type is FunctionType);

lib/src/model.dart

@@ -128,7 +128,13 @@ abstract class Inheritable implements ModelElement {
   ModelElement get definingEnclosingElement =>
       _memoizer.memoized(_definingEnclosingElement);
 
-  ModelElement _canonicalEnclosingElement() {
+  @override
+  ModelElement _canonicalModelElement() {
+    return canonicalEnclosingElement?.allCanonicalModelElements
+        ?.firstWhere((m) => m.name == name, orElse: () => null);
+  }
+
+  Class _canonicalEnclosingElement() {
     Class canonicalEnclosingClass;
     Element searchElement = element;
     if (isInherited) {
@@ -180,7 +186,7 @@ abstract class Inheritable implements ModelElement {
     return canonicalEnclosingClass;
   }
 
-  ModelElement get canonicalEnclosingElement =>
+  Class get canonicalEnclosingElement =>
       _memoizer.memoized(_canonicalEnclosingElement);
 
   List<Class> get inheritance {
@@ -608,6 +614,7 @@ class Class extends ModelElement
   @override
   ModelElement get enclosingElement => library;
 
+  @override
   String get fileName => "${name}-class.html";
 
   String get fullkind {
@@ -654,8 +661,11 @@ class Class extends ModelElement
 
   @override
   String get href {
-    if (canonicalLibrary == null) return null;
-    return '${canonicalLibrary.dirName}/$fileName';
+    if (!identical(canonicalModelElement, this))
+      return canonicalModelElement?.href;
+    assert(canonicalLibrary != null);
+    assert(canonicalLibrary == library);
+    return '${library.dirName}/$fileName';
   }
 
   /// Returns all the implementors of this class.
@@ -1132,8 +1142,11 @@ class Constructor extends ModelElement
 
   @override
   String get href {
-    if (canonicalLibrary == null) return null;
-    return '${canonicalLibrary.dirName}/${_constructor.enclosingElement.name}/$name.html';
+    if (!identical(canonicalModelElement, this))
+      return canonicalModelElement?.href;
+    assert(canonicalLibrary != null);
+    assert(canonicalLibrary == library);
+    return '${enclosingElement.library.dirName}/${enclosingElement.name}/$name.html';
   }
 
   @override
@@ -1346,9 +1359,12 @@ class EnumField extends Field {
 
   @override
   String get href {
-    if (canonicalLibrary == null || canonicalEnclosingElement == null)
-      return null;
-    return '${canonicalEnclosingElement.library.dirName}/${(canonicalEnclosingElement as Class).fileName}';
+    if (!identical(canonicalModelElement, this))
+      return canonicalModelElement?.href;
+    assert(!(canonicalLibrary == null || canonicalEnclosingElement == null));
+    assert(canonicalLibrary == library);
+    assert(canonicalEnclosingElement == enclosingElement);
+    return '${enclosingElement.library.dirName}/${(enclosingElement as Class).fileName}';
   }
 
   @override
@@ -1422,19 +1438,12 @@ class Field extends ModelElement
 
   @override
   String get href {
-    String retval;
-    if (canonicalLibrary == null) return null;
-    if (enclosingElement is Class) {
-      if (canonicalEnclosingElement == null) return null;
-      retval =
-          '${canonicalEnclosingElement.canonicalLibrary.dirName}/${canonicalEnclosingElement.name}/$_fileName';
-    } else if (enclosingElement is Library) {
-      retval = '${canonicalLibrary.dirName}/$_fileName';
-    } else {
-      throw new StateError(
-          '$name is not in a class or library, instead it is a ${enclosingElement.element}');
-    }
-    return retval;
+    if (!identical(canonicalModelElement, this))
+      return canonicalModelElement?.href;
+    assert(canonicalLibrary != null);
+    assert(canonicalEnclosingElement == enclosingElement);
+    assert(canonicalLibrary == library);
+    return '${enclosingElement.library.dirName}/${enclosingElement.name}/$fileName';
   }
 
   @override
@@ -1494,7 +1503,8 @@ class Field extends ModelElement
 
   FieldElement get _field => (element as FieldElement);
 
-  String get _fileName => isConst ? '$name-constant.html' : '$name.html';
+  @override
+  String get fileName => isConst ? '$name-constant.html' : '$name.html';
 
   String _sourceCode() {
     // We could use a set to figure the dupes out, but that would lose ordering.
@@ -1906,6 +1916,7 @@ class Library extends ModelElement {
 
   Iterable<Class> get publicExceptions => filterNonPublic(exceptions);
 
+  @override
   String get fileName => '$dirName-library.html';
 
   List<ModelFunction> _functions() {
@@ -1943,8 +1954,9 @@ class Library extends ModelElement {
 
   @override
   String get href {
-    if (canonicalLibrary == null) return null;
-    return '${canonicalLibrary.dirName}/$fileName';
+    if (!identical(canonicalModelElement, this))
+      return canonicalModelElement?.href;
+    return '${library.dirName}/$fileName';
   }
 
   InheritanceManager _inheritanceManager() => new InheritanceManager(element);
@@ -2275,18 +2287,19 @@ class Method extends ModelElement
     return _enclosingClass;
   }
 
-  String get fileName => "${name}.html";
-
   String get fullkind {
     if (_method.isAbstract) return 'abstract $kind';
     return kind;
   }
 
   @override
   String get href {
-    if (canonicalLibrary == null || canonicalEnclosingElement == null)
-      return null;
-    return '${canonicalEnclosingElement.canonicalLibrary.dirName}/${canonicalEnclosingElement.name}/${fileName}';
+    if (!identical(canonicalModelElement, this))
+      return canonicalModelElement?.href;
+    assert(!(canonicalLibrary == null || canonicalEnclosingElement == null));
+    assert(canonicalLibrary == library);
+    assert(canonicalEnclosingElement == enclosingElement);
+    return '${enclosingElement.library.dirName}/${enclosingElement.name}/${fileName}';
   }
 
   @override
@@ -2693,6 +2706,20 @@ abstract class ModelElement extends Canonicalization
   bool get canHaveParameters =>
       element is ExecutableElement || element is FunctionTypedElement;
 
+  ModelElement _canonicalModelElement() {
+    Class preferredClass;
+    if (enclosingElement is Class) {
+      preferredClass = enclosingElement;
+    }
+    return package.findCanonicalModelElementFor(element,
+        preferredClass: preferredClass);
+  }
+
+  // Returns the canonical ModelElement for this ModelElement, or null
+  // if there isn't one.
+  ModelElement get canonicalModelElement =>
+      _memoizer.memoized(_canonicalModelElement);
+
   // TODO(jcollins-g): untangle when mixins can call super
   @override
   List<ModelElement> get documentationFrom =>
@@ -2888,6 +2915,8 @@ abstract class ModelElement extends Canonicalization
     return '';
   }
 
+  String get fileName => "${name}.html";
+
   /// Returns the fully qualified name.
   ///
   /// For example: libraryName.className.methodName
@@ -3559,12 +3588,13 @@ class ModelFunctionTyped extends ModelElement
   @override
   ModelElement get enclosingElement => library;
 
-  String get fileName => "$name.html";
-
   @override
   String get href {
-    if (canonicalLibrary == null) return null;
-    return '${canonicalLibrary.dirName}/$fileName';
+    if (!identical(canonicalModelElement, this))
+      return canonicalModelElement?.href;
+    assert(canonicalLibrary != null);
+    assert(canonicalLibrary == library);
+    return '${library.dirName}/$fileName';
   }
 
   @override
@@ -4182,6 +4212,9 @@ class Package extends Canonicalization with Nameable, Warnable, Memoizeable {
     if (e is PropertyAccessorElement) {
       searchElement = e.variable;
     }
+    if (e is GenericFunctionTypeElement) {
+      searchElement = e.enclosingElement;
+    }
 
     for (Library library in publicLibraries) {
       if (library.modelElementsMap.containsKey(searchElement)) {
@@ -4211,6 +4244,10 @@ class Package extends Canonicalization with Nameable, Warnable, Memoizeable {
   /// Tries to find a canonical ModelElement for this element.  If we know
   /// this element is related to a particular class, pass preferredClass to
   /// disambiguate.
+  ///
+  /// This doesn't know anything about [Package.inheritThrough] and probably
+  /// shouldn't, so using it with [Inheritable]s without special casing is
+  /// not advised.
   ModelElement findCanonicalModelElementFor(Element e, {Class preferredClass}) {
     assert(allLibrariesAdded);
     Library lib = findCanonicalLibraryFor(e);
@@ -4295,7 +4332,9 @@ class Package extends Canonicalization with Nameable, Warnable, Memoizeable {
       }
 
       assert(matches.length <= 1);
-      if (!matches.isEmpty) modelElement = matches.first;
+      if (matches.isNotEmpty) {
+        modelElement = matches.first;
+      }
     } else {
       if (lib != null) {
         Accessor getter;
@@ -4431,19 +4470,7 @@ class Parameter extends ModelElement implements EnclosedElement {
 
   @override
   String get href {
-    if (canonicalLibrary == null) return null;
-    var p = _parameter.enclosingElement;
-
-    if (p is FunctionElement) {
-      return '${canonicalLibrary.dirName}/${p.name}.html';
-    } else {
-      // TODO: why is this logic here?
-      var name = Operator.friendlyNames.containsKey(p.name)
-          ? Operator.friendlyNames[p.name]
-          : p.name;
-      return '${canonicalLibrary.dirName}/${p.enclosingElement.name}/' +
-          '${name}.html#${htmlId}';
-    }
+    throw new StateError('href not implemented for parameters');
   }
 
   @override
@@ -4636,8 +4663,11 @@ class TopLevelVariable extends ModelElement
 
   @override
   String get href {
-    if (canonicalLibrary == null) return null;
-    return '${canonicalLibrary.dirName}/$_fileName';
+    if (!identical(canonicalModelElement, this))
+      return canonicalModelElement?.href;
+    assert(canonicalLibrary != null);
+    assert(canonicalLibrary == library);
+    return '${library.dirName}/$fileName';
   }
 
   @override
@@ -4664,7 +4694,8 @@ class TopLevelVariable extends ModelElement
     return docs;
   }
 
-  String get _fileName => isConst ? '$name-constant.html' : '$name.html';
+  @override
+  String get fileName => isConst ? '$name-constant.html' : '$name.html';
 
   TopLevelVariableElement get _variable => (element as TopLevelVariableElement);
 }
@@ -4678,8 +4709,6 @@ class Typedef extends ModelElement
   @override
   ModelElement get enclosingElement => library;
 
-  String get fileName => '$name.html';
-
   @override
   String get nameWithGenerics => '$name${super.genericParameters}';
 
@@ -4697,8 +4726,11 @@ class Typedef extends ModelElement
 
   @override
   String get href {
-    if (canonicalLibrary == null) return null;
-    return '${canonicalLibrary.dirName}/$fileName';
+    if (!identical(canonicalModelElement, this))
+      return canonicalModelElement?.href;
+    assert(canonicalLibrary != null);
+    assert(canonicalLibrary == library);
+    return '${library.dirName}/$fileName';
   }
 
   // Food for mustache.
@@ -4730,8 +4762,11 @@ class TypeParameter extends ModelElement {
 
   @override
   String get href {
-    if (canonicalLibrary == null) return null;
-    return '${canonicalLibrary.dirName}/${_typeParameter.enclosingElement.name}/$name';
+    if (!identical(canonicalModelElement, this))
+      return canonicalModelElement?.href;
+    assert(canonicalLibrary != null);
+    assert(canonicalLibrary == library);
+    return '${enclosingElement.library.dirName}/${enclosingElement.name}/$name';
   }
 
   @override

lib/src/model_utils.dart

@@ -70,6 +70,13 @@ bool hasPrivateName(Element e) {
   if (e.name.startsWith('_')) {
     return true;
   }
+  // GenericFunctionTypeElements have the name we care about in the enclosing
+  // element.
+  if (e is GenericFunctionTypeElement) {
+    if (e.enclosingElement.name.startsWith('_')) {
+      return true;
+    }
+  }
   if (e is LibraryElement &&
       (e.identifier.startsWith('dart:_') ||
           ['dart:nativewrappers'].contains(e.identifier))) {

test/model_test.dart

@@ -1889,6 +1889,7 @@ String topLevelFunction(int param1, bool param2, Cool coolBeans,
   group('Constant', () {
     TopLevelVariable greenConstant,
         cat,
+        customClassPrivate,
         orangeConstant,
         prettyColorsConstant,
         deprecated;
@@ -1906,6 +1907,7 @@ String topLevelFunction(int param1, bool param2, Cool coolBeans,
       deprecated =
           exLibrary.constants.firstWhere((c) => c.name == 'deprecated');
       Class Dog = exLibrary.allClasses.firstWhere((c) => c.name == 'Dog');
+      customClassPrivate = fakeLibrary.constants.firstWhere((c) => c.name == 'CUSTOM_CLASS_PRIVATE');
       aStaticConstField =
           Dog.allFields.firstWhere((f) => f.name == 'aStaticConstField');
       aName = Dog.allFields.firstWhere((f) => f.name == 'aName');
@@ -1920,6 +1922,10 @@ String topLevelFunction(int param1, bool param2, Cool coolBeans,
       expect(aStaticConstField.constantValue, '"A Constant Dog"');
     });
 
+    test('privately constructed constants are unlinked', () {
+      expect(customClassPrivate.constantValue, 'const _APrivateConstClass()');
+    });
+
     test('has a fully qualified name', () {
       expect(greenConstant.fullyQualifiedName, 'ex.COLOR_GREEN');
     });

testing/test_package/lib/fake.dart

@@ -160,6 +160,12 @@ class ConstantClass {
   ConstantClass.notConstant(this.value);
 }
 
+class _APrivateConstClass {
+  const _APrivateConstClass();
+}
+
+const _APrivateConstClass CUSTOM_CLASS_PRIVATE = const _APrivateConstClass();
+
 // No dart docs on purpose. Also, a non-primitive const class.
 const ConstantClass CUSTOM_CLASS = const ConstantClass('custom');
 

testing/test_package_docs/fake/AClassUsingASuperMixin-class.html

@@ -71,6 +71,7 @@ <h5>fake library</h5>
 
       <li class="section-title"><a href="fake/fake-library.html#constants">Constants</a></li>
       <li><a href="fake/CUSTOM_CLASS-constant.html">CUSTOM_CLASS</a></li>
+      <li><a href="fake/CUSTOM_CLASS_PRIVATE-constant.html">CUSTOM_CLASS_PRIVATE</a></li>
       <li><a class="deprecated" href="fake/DOWN-constant.html">DOWN</a></li>
       <li><a href="fake/greatAnnotation-constant.html">greatAnnotation</a></li>
       <li><a href="fake/greatestAnnotation-constant.html">greatestAnnotation</a></li>

testing/test_package_docs/fake/AMixinCallingSuper-class.html

@@ -71,6 +71,7 @@ <h5>fake library</h5>
 
       <li class="section-title"><a href="fake/fake-library.html#constants">Constants</a></li>
       <li><a href="fake/CUSTOM_CLASS-constant.html">CUSTOM_CLASS</a></li>
+      <li><a href="fake/CUSTOM_CLASS_PRIVATE-constant.html">CUSTOM_CLASS_PRIVATE</a></li>
       <li><a class="deprecated" href="fake/DOWN-constant.html">DOWN</a></li>
       <li><a href="fake/greatAnnotation-constant.html">greatAnnotation</a></li>
       <li><a href="fake/greatestAnnotation-constant.html">greatestAnnotation</a></li>