Escape type parameters inside comment references (#1740)

* Escape type parameters and other things inside comment references

* Rebuild package docs

Author: jcollins-g

lib/src/markdown_processor.dart

@@ -154,9 +154,8 @@ final RegExp _hide_schemes = new RegExp('^(http|https)://');
 
 class MatchingLinkResult {
   final ModelElement element;
-  final String label;
   final bool warn;
-  MatchingLinkResult(this.element, this.label, {this.warn: true});
+  MatchingLinkResult(this.element, {this.warn: true});
 }
 
 class IterableBlockParser extends md.BlockParser {
@@ -244,13 +243,12 @@ MatchingLinkResult _getMatchingLinkElement(
   // By debugging inspection, it seems correct to not warn when we don't have
   // CommentReferences; there's actually nothing that needs resolving in
   // that case.
-  if (commentRefs == null)
-    return new MatchingLinkResult(null, null, warn: false);
+  if (commentRefs == null) return new MatchingLinkResult(null, warn: false);
 
   if (!codeRef.contains(isConstructor) &&
       codeRef.contains(notARealDocReference)) {
     // Don't waste our time on things we won't ever find.
-    return new MatchingLinkResult(null, null, warn: false);
+    return new MatchingLinkResult(null, warn: false);
   }
 
   ModelElement refModelElement;
@@ -283,20 +281,20 @@ MatchingLinkResult _getMatchingLinkElement(
     // TODO(jcollins-g): remove squelching of non-canonical warnings here
     //                   once we no longer process full markdown for
     //                   oneLineDocs (#1417)
-    return new MatchingLinkResult(null, null, warn: element.isCanonical);
+    return new MatchingLinkResult(null, warn: element.isCanonical);
   }
 
   // Ignore all parameters.
   if (refModelElement is Parameter || refModelElement is TypeParameter)
-    return new MatchingLinkResult(null, null, warn: false);
+    return new MatchingLinkResult(null, warn: false);
 
   // There have been places in the code which helpfully cache entities
   // regardless of what package they are associated with.  This assert
   // will protect us from reintroducing that.
   assert(refModelElement == null ||
       refModelElement.packageGraph == element.packageGraph);
   if (refModelElement != null) {
-    return new MatchingLinkResult(refModelElement, null);
+    return new MatchingLinkResult(refModelElement);
   }
   // From this point on, we haven't been able to find a canonical ModelElement.
   if (!refModelElement.isCanonical) {
@@ -306,15 +304,15 @@ MatchingLinkResult _getMatchingLinkElement(
     }
     // Don't warn about doc references because that's covered by the no
     // canonical library found message.
-    return new MatchingLinkResult(null, null, warn: false);
+    return new MatchingLinkResult(null, warn: false);
   }
   // We should never get here unless there's a bug in findCanonicalModelElementFor.
   // findCanonicalModelElementFor(searchElement, preferredClass: preferredClass)
   // should only return null if ModelElement.from(searchElement, refLibrary)
   // would return a non-canonical element.  However, outside of checked mode,
   // at least we have a canonical element, so proceed.
   assert(false);
-  return new MatchingLinkResult(refModelElement, null);
+  return new MatchingLinkResult(refModelElement);
 }
 
 /// Given a set of commentRefs, return the one whose name matches the codeRef.
@@ -725,7 +723,6 @@ String _linkDocReference(
   MatchingLinkResult result;
   result = _getMatchingLinkElement(codeRef, warnable, commentRefs);
   final ModelElement linkedElement = result.element;
-  final String label = result.label ?? codeRef;
   if (linkedElement != null) {
     var classContent = '';
     if (linkedElement.isDeprecated) {
@@ -734,16 +731,16 @@ String _linkDocReference(
     // This would be linkedElement.linkedName, but link bodies are slightly
     // different for doc references.
     if (linkedElement.href == null) {
-      return '<code>${htmlEscape.convert(label)}</code>';
+      return '<code>${htmlEscape.convert(codeRef)}</code>';
     } else {
-      return '<a ${classContent}href="${linkedElement.href}">$label</a>';
+      return '<a ${classContent}href="${linkedElement.href}">${htmlEscape.convert(codeRef)}</a>';
     }
   } else {
     if (result.warn) {
       warnable.warn(PackageWarning.unresolvedDocReference,
           message: codeRef, referredFrom: warnable.documentationFrom);
     }
-    return '<code>${htmlEscape.convert(label)}</code>';
+    return '<code>${htmlEscape.convert(codeRef)}</code>';
   }
 }
 

test/model_test.dart

@@ -927,6 +927,18 @@ void main() {
             contains(
                 '<a href="fake/NAME_SINGLEUNDERSCORE-constant.html">NAME_SINGLEUNDERSCORE</a>'));
       });
+
+      test('correctly escapes type parameters in links', () {
+        expect(
+            docsAsHtml,
+            contains(
+                '<a href="fake/ExtraSpecialList-class.html">ExtraSpecialList&lt;Object&gt;</a>'));
+      });
+
+      test('correctly escapes type parameters in broken links', () {
+        expect(docsAsHtml,
+            contains('<code>ThisIsNotHereNoWay&lt;MyType&gt;</code>'));
+      });
     });
 
     test('multi-underscore names in brackets do not become italicized', () {

testing/test_package/lib/fake.dart

@@ -744,6 +744,10 @@ class BaseForDocComments {
   /// Reference to a bracket operator within this class [operator []] xxx
   ///
   /// Reference to a bracket operator in another class [SpecialList.operator []] xxx
+  ///
+  /// Reference containing a type parameter [ExtraSpecialList<Object>]
+  ///
+  /// Reference to something that doesn't exist containing a type parameter [ThisIsNotHereNoWay<MyType>]
   String doAwesomeStuff(int value) => null;
 
   void anotherMethod() {}

testing/test_package_docs/fake/BaseForDocComments/doAwesomeStuff.html

@@ -91,6 +91,8 @@ <h1>doAwesomeStuff method</h1>
 the name <a href="two_exports/BaseClass-class.html">BaseClass</a> xx</p>
 <p>Reference to a bracket operator within this class <a href="fake/BaseForDocComments/operator_get.html">operator []</a> xxx</p>
 <p>Reference to a bracket operator in another class <code>SpecialList.operator []</code> xxx</p>
+<p>Reference containing a type parameter <a href="fake/ExtraSpecialList-class.html">ExtraSpecialList&lt;Object&gt;</a></p>
+<p>Reference to something that doesn't exist containing a type parameter <code>ThisIsNotHereNoWay&lt;MyType&gt;</code></p>
     </section>