Add warnings for "free-hanging" generics in docs [#1250] (#1313)

* Add warnings for "free-hanging" generics in docs [#1250]

@Hixie asked for warnings when the generics are "free-hanging", and not
wrapped into [] block (like `Apple<Cat>`, but not `[Apple<Cat>]`). This
commit adds those warnings. We try to find the tags, which are not
wrapped into square brackets, which are not from the whitelist of
possible HTML tags.

Also, now output all the warnings to the console:

* When there's a missing reference in the comments
* When there're ambiguous reference in the comments
* When there're "free-hanging" generics

But that generates TONS AND TONS of warnings when you e.g. generate Dart
SDK docs or Flutter docs. So, I've added the flag `--show-warnings`,
without it we don't show the warnings.

* Address feedback

Author: astashov

bin/dartdoc.dart

@@ -140,6 +140,7 @@ main(List<String> arguments) async {
   initializeConfig(
       addCrossdart: addCrossdart,
       examplePathPrefix: args['example-path-prefix'],
+      showWarnings: args['show-warnings'],
       includeSource: includeSource,
       inputDir: inputDir,
       sdkVersion: sdk.sdkVersion);
@@ -176,6 +177,8 @@ ArgParser _createArgsParser() {
       defaultsTo: false);
   parser.addFlag('sdk-docs',
       help: 'Generate ONLY the docs for the Dart SDK.', negatable: false);
+  parser.addFlag('show-warnings',
+      help: 'Display warnings.', negatable: false);
   parser.addFlag('show-progress',
       help: 'Display progress indications to console stdout', negatable: false);
   parser.addOption('sdk-readme',

lib/dartdoc.dart

@@ -64,12 +64,14 @@ Future<List<Generator>> initGenerators(String url, List<String> headerFilePaths,
 void initializeConfig(
     {Directory inputDir,
     String sdkVersion,
+    bool showWarnings: false,
     bool addCrossdart: false,
     String examplePathPrefix,
     bool includeSource: true}) {
   setConfig(
       inputDir: inputDir,
       sdkVersion: sdkVersion,
+      showWarnings: showWarnings,
       addCrossdart: addCrossdart,
       examplePathPrefix: examplePathPrefix,
       includeSource: includeSource);

lib/src/config.dart

@@ -9,21 +9,23 @@ import 'dart:io';
 class Config {
   final Directory inputDir;
   final bool addCrossdart;
+  final bool showWarnings;
   final String examplePathPrefix;
   final bool includeSource;
   final String sdkVersion;
   Config._(
-      this.inputDir, this.addCrossdart, this.examplePathPrefix, this.includeSource, this.sdkVersion);
+      this.inputDir, this.showWarnings, this.addCrossdart, this.examplePathPrefix, this.includeSource, this.sdkVersion);
 }
 
 Config _config;
 Config get config => _config;
 
 void setConfig(
     {Directory inputDir,
+    bool showWarnings: false,
     String sdkVersion,
     bool addCrossdart: false,
     String examplePathPrefix,
     bool includeSource: true}) {
-  _config = new Config._(inputDir, addCrossdart, examplePathPrefix, includeSource, sdkVersion);
+  _config = new Config._(inputDir, showWarnings, addCrossdart, examplePathPrefix, includeSource, sdkVersion);
 }

lib/src/markdown_processor.dart

@@ -6,6 +6,7 @@
 library dartdoc.markdown_processor;
 
 import 'dart:convert';
+import 'dart:math';
 
 import 'package:analyzer/dart/ast/ast.dart';
 import 'package:analyzer/dart/element/element.dart'
@@ -14,8 +15,23 @@ import 'package:html/parser.dart' show parse;
 import 'package:markdown/markdown.dart' as md;
 
 import 'model.dart';
-
-const bool _emitWarning = false;
+import 'reporting.dart';
+
+const validHtmlTags = const [
+  "a", "abbr", "address", "area", "article", "aside", "audio", "b",
+  "bdi", "bdo", "blockquote", "br", "button", "canvas", "caption",
+  "cite", "code", "col", "colgroup", "data", "datalist", "dd", "del", "dfn",
+  "div", "dl", "dt", "em", "fieldset", "figcaption", "figure",
+  "footer", "form", "h1", "h2", "h3", "h4", "h5", "h6", "header", "hr",
+  "i", "iframe", "img", "input", "ins", "kbd", "keygen", "label",
+  "legend", "li", "link", "main", "map", "mark", "meta", "meter", "nav",
+  "noscript", "object", "ol", "optgroup", "option", "output", "p", "param",
+  "pre", "progress", "q", "s", "samp",
+  "script", "section", "select", "small", "source", "span", "strong", "style",
+  "sub", "sup", "table", "tbody", "td", "template", "textarea", "tfoot", "th",
+  "thead", "time", "title", "tr", "track", "u", "ul", "var", "video", "wbr"
+];
+final nonHTMLRegexp = new RegExp("</?(?!(${validHtmlTags.join("|")})[> ])\\w+[> ]");
 
 // We don't emit warnings currently: #572.
 const List<String> _oneLinerSkipTags = const ["code", "pre"];
@@ -137,9 +153,8 @@ MatchingLinkResult _findRefElementInLibrary(String codeRef, ModelElement element
   } else if (result.length == 1) {
     return new MatchingLinkResult(result.values.first, result.values.first.name);
   } else {
-    // TODO: add --fatal-warning, which would make the app crash in case of ambiguous references
-    print(
-        "Ambiguous reference to [${codeRef}] in '${element.fullyQualifiedName}' (${element.sourceFileName}:${element.lineNumber}). " +
+    warning(
+        "Ambiguous reference to [${codeRef}] in ${_elementLocation(element)}. " +
             "We found matches to the following elements: ${result.keys.map((k) => "'${k}'").join(", ")}");
     return new MatchingLinkResult(null, null);
   }
@@ -165,23 +180,75 @@ String _linkDocReference(String reference, ModelElement element, NodeList<Commen
     // different for doc references. sigh.
     return '<a ${classContent}href="${linkedElement.href}">$label</a>';
   } else {
-    if (_emitWarning) {
-      // TODO: add --fatal-warning, which would make the app crash in case of ambiguous references
-      print("  warning: unresolved doc reference '$reference' (in $element)");
-    }
+    warning("unresolved doc reference '$reference' (in ${_elementLocation(element)}");
     return '<code>${HTML_ESCAPE.convert(label)}</code>';
   }
 }
 
+String _elementLocation(ModelElement element) {
+  while ((element.element.documentationComment == null || element.element.documentationComment == "")
+      && element.overriddenElement != null) {
+    element = element.overriddenElement;
+  }
+  return "'${element.fullyQualifiedName}' (${element.sourceFileName}:${element.lineNumber})";
+}
+
 String _renderMarkdownToHtml(String text, [ModelElement element]) {
   md.Node _linkResolver(String name) {
     NodeList<CommentReference> commentRefs = _getCommentRefs(element);
     return new md.Text(_linkDocReference(name, element, commentRefs));
   }
 
+  _showWarningsForGenericsOutsideSquareBracketsBlocks(text, element);
   return md.markdownToHtml(text, inlineSyntaxes: _markdown_syntaxes, linkResolver: _linkResolver);
 }
 
+// Generics should be wrapped into `[]` blocks, to avoid handling them as HTML tags
+// (like, [Apple<int>]). @Hixie asked for a warning when there's something, that looks
+// like a non HTML tag (a generic?) outside of a `[]` block.
+// https://github.com/dart-lang/dartdoc/issues/1250#issuecomment-269257942
+void _showWarningsForGenericsOutsideSquareBracketsBlocks(String text, [ModelElement element]) {
+  List<int> tagPositions = findFreeHangingGenericsPositions(text);
+  if (tagPositions.isNotEmpty) {
+    tagPositions.forEach((int position) {
+      String errorMessage = "Generic type handled as HTML";
+      if (element != null) {
+        errorMessage += " in ${_elementLocation(element)}";
+      }
+      errorMessage += " - '${text.substring(max(position - 20, 0), min(position + 20, text.length))}'";
+      warning(errorMessage);
+    });
+  }
+}
+
+List<int> findFreeHangingGenericsPositions(String string) {
+  int currentPosition = 0;
+  int squareBracketsDepth = 0;
+  List<int> results = [];
+  while (true) {
+    final int nextOpenBracket = string.indexOf("[", currentPosition);
+    final int nextCloseBracket = string.indexOf("]", currentPosition);
+    final int nextNonHTMLTag = string.indexOf(nonHTMLRegexp, currentPosition);
+    final Iterable<int> nextPositions = [nextOpenBracket, nextCloseBracket, nextNonHTMLTag].where((p) => p != -1);
+    if (nextPositions.isNotEmpty) {
+      final minPos = nextPositions.reduce(min);
+      if (nextOpenBracket == minPos) {
+        squareBracketsDepth += 1;
+      } else if (nextCloseBracket == minPos) {
+        squareBracketsDepth = max(squareBracketsDepth - 1, 0);
+      } else if (nextNonHTMLTag == minPos) {
+        if (squareBracketsDepth == 0) {
+          results.add(minPos);
+        }
+      }
+      currentPosition = minPos + 1;
+    } else {
+      break;
+    }
+  }
+  return results;
+}
+
 class Documentation {
   final String raw;
   final String asHtml;

lib/src/reporting.dart

@@ -0,0 +1,16 @@
+// Copyright (c) 2016, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+library reporting;
+
+import 'config.dart';
+import 'dart:io';
+
+void warning(String message) {
+  // TODO: Could handle fatal warnings here, or print to stderr, or remember
+  // that we had at least one warning, and exit with non-null exit code in this case.
+  if (config != null && config.showWarnings) {
+    stderr.writeln("warning: ${message}");
+  }
+}

test/all.dart

@@ -13,6 +13,7 @@ import 'model_utils_test.dart' as model_utils_tests;
 import 'package_meta_test.dart' as package_meta_tests;
 import 'resource_loader_test.dart' as resource_loader_tests;
 import 'template_test.dart' as template_tests;
+import 'markdown_processor_test.dart' as markdown_processor_tests;
 
 void main() {
   compare_output_tests.main();
@@ -24,4 +25,5 @@ void main() {
   package_meta_tests.main();
   resource_loader_tests.main();
   template_tests.main();
+  markdown_processor_tests.main();
 }

test/markdown_processor_test.dart

@@ -0,0 +1,27 @@
+// Copyright (c) 2016, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+library dartdoc.markdown_processor_test;
+
+import 'package:dartdoc/src/markdown_processor.dart';
+import 'package:test/test.dart';
+
+void main() {
+  group('findFreeHangingGenericsPositions()', () {
+    test('returns empty array if all the generics are in []', () {
+      final string = "One two [three<four>] [[five<six>] seven eight";
+      expect(findFreeHangingGenericsPositions(string), equals([]));
+    });
+
+    test('returns positions of generics outside of []', () {
+      final string = "One two<int> [[three<four>] five<six>] seven<eight>";
+      expect(findFreeHangingGenericsPositions(string), equals([7, 44]));
+    });
+
+    test('ignores HTML tags', () {
+      final string = "One two<int> foo<pre> [[three<four>] five<six>] bar</pre> seven<eight>";
+      expect(findFreeHangingGenericsPositions(string), equals([7, 63]));
+    });
+  });
+}