Use analyzer line number library (#2034)

* Fix line number cache and add tests

* dartfmt

* remove accidental minus 0

* Review comments and lint fixup for modern Dart

* Change approach -- delete dartdoc's line number cache implementation

* Cleanups, add more tests

* Check for synthetic constructors instead of default

* Add testing for a completely empty library file.

* Review comments

Author: jcollins-g

lib/src/line_number_cache.dart

@@ -771,8 +771,11 @@ String _linkDocReference(String codeRef, Warnable warnable,
     }
   } else {
     if (result.warn) {
+      // Avoid claiming documentation is inherited when it comes from the
+      // current element.
       warnable.warn(PackageWarning.unresolvedDocReference,
-          message: codeRef, referredFrom: warnable.documentationFrom);
+          message: codeRef,
+          referredFrom: warnable.documentationIsLocal ? null : warnable.documentationFrom);
     }
     return '<code>${htmlEscape.convert(codeRef)}</code>';
   }

lib/src/markdown_processor.dart

@@ -25,6 +25,7 @@ import 'package:analyzer/dart/element/type.dart';
 import 'package:analyzer/dart/element/visitor.dart';
 import 'package:analyzer/file_system/file_system.dart' as file_system;
 import 'package:analyzer/file_system/physical_file_system.dart';
+import 'package:analyzer/source/line_info.dart';
 import 'package:analyzer/src/context/builder.dart';
 import 'package:analyzer/src/dart/analysis/byte_store.dart';
 import 'package:analyzer/src/dart/analysis/driver.dart';
@@ -50,7 +51,6 @@ import 'package:crypto/crypto.dart';
 import 'package:dartdoc/src/dartdoc_options.dart';
 import 'package:dartdoc/src/element_type.dart';
 import 'package:dartdoc/src/io_utils.dart';
-import 'package:dartdoc/src/line_number_cache.dart';
 import 'package:dartdoc/src/logging.dart';
 import 'package:dartdoc/src/markdown_processor.dart' show Documentation;
 import 'package:dartdoc/src/model_utils.dart' as utils;
@@ -1390,6 +1390,17 @@ class Constructor extends ModelElement
       ConstructorElement element, Library library, PackageGraph packageGraph)
       : super(element, library, packageGraph, null);
 
+  @override
+  CharacterLocation get characterLocation {
+    if (element.isSynthetic) {
+      // Make warnings for a synthetic constructor refer to somewhere reasonable
+      // since a synthetic constructor has no definition independent of the
+      // parent class.
+      return enclosingElement.characterLocation;
+    }
+    return super.characterLocation;
+  }
+
   @override
   // TODO(jcollins-g): Revisit this when dart-lang/sdk#31517 is implemented.
   List<TypeParameter> get typeParameters =>
@@ -2057,7 +2068,7 @@ class Field extends ModelElement
 }
 
 /// Mixin for top-level variables and fields (aka properties)
-abstract class GetterSetterCombo implements ModelElement {
+mixin GetterSetterCombo on ModelElement {
   Accessor get getter;
 
   Iterable<Accessor> get allAccessors sync* {
@@ -2114,6 +2125,20 @@ abstract class GetterSetterCombo implements ModelElement {
     return const HtmlEscape(HtmlEscapeMode.unknown).convert(result);
   }
 
+  @override
+  CharacterLocation get characterLocation {
+    // Handle all synthetic possibilities.  Ordinarily, warnings for
+    // explicit setters/getters will be handled by those objects, but
+    // if a warning comes up for an enclosing synthetic field we have to
+    // put it somewhere.  So pick an accessor.
+    if (element.isSynthetic) {
+      if (hasExplicitGetter) return getter.characterLocation;
+      if (hasExplicitSetter) return setter.characterLocation;
+      assert(false, 'Field and accessors can not all be synthetic');
+    }
+    return super.characterLocation;
+  }
+
   String get constantValue => linkifyConstantValue(constantValueBase);
 
   String get constantValueTruncated =>
@@ -2367,6 +2392,18 @@ class Library extends ModelElement with Categorization, TopLevelContainer {
 
   List<Class> get allClasses => _allClasses;
 
+  @override
+  CharacterLocation get characterLocation {
+    if (element.nameOffset == -1) {
+      assert(isAnonymous, 'Only anonymous libraries are allowed to have no declared location');
+      return CharacterLocation(1, 1);
+    }
+    return super.characterLocation;
+  }
+
+  @override
+  CompilationUnitElement get compilationUnitElement => (element as LibraryElement).definingCompilationUnit;
+
   @override
   Iterable<Class> get classes {
     return _allClasses
@@ -2917,6 +2954,19 @@ class Method extends ModelElement
     }).toList();
   }
 
+  @override
+  CharacterLocation get characterLocation {
+    if (enclosingElement is Enum && name == 'toString') {
+      // The toString() method on Enums is special, treated as not having
+      // a definition location by the analyzer yet not being inherited, either.
+      // Just directly override our location with the Enum definition --
+      // this is OK because Enums can not inherit from each other nor
+      // have their definitions split between files.
+      return enclosingElement.characterLocation;
+    }
+    return super.characterLocation;
+  }
+
   @override
   ModelElement get enclosingElement {
     if (_enclosingClass == null) {
@@ -3078,7 +3128,7 @@ abstract class Privacy {
 /// ModelElement will reference itself as part of the "wrong" [Library]
 /// from the public interface perspective.
 abstract class ModelElement extends Canonicalization
-    with Privacy, Warnable, Nameable, SourceCodeMixin, Indexable
+    with Privacy, Warnable, Locatable, Nameable, SourceCodeMixin, Indexable
     implements Comparable, Documentable {
   final Element _element;
 
@@ -3715,8 +3765,8 @@ abstract class ModelElement extends Canonicalization
   @override
   String get location {
     // Call nothing from here that can emit warnings or you'll cause stack overflows.
-    if (lineAndColumn != null) {
-      return "(${path.toUri(sourceFileName)}:${lineAndColumn.item1}:${lineAndColumn.item2})";
+    if (characterLocation != null) {
+      return "(${path.toUri(sourceFileName)}:${characterLocation.toString()})";
     }
     return "(${path.toUri(sourceFileName)})";
   }
@@ -3751,19 +3801,25 @@ abstract class ModelElement extends Canonicalization
 
   String get sourceFileName => element.source.fullName;
 
-  Tuple2<int, int> _lineAndColumn;
-  bool _isLineNumberComputed = false;
+  CharacterLocation _characterLocation;
+  bool _characterLocationIsSet = false;
 
   @override
-  Tuple2<int, int> get lineAndColumn {
-    // TODO(jcollins-g): implement lineAndColumn for explicit fields
-    if (!_isLineNumberComputed) {
-      _lineAndColumn = lineNumberCache.lineAndColumn(
-          element.source.fullName, element.nameOffset);
+  CharacterLocation get characterLocation {
+    if (!_characterLocationIsSet) {
+      LineInfo lineInfo = compilationUnitElement.lineInfo;
+      _characterLocationIsSet = true;
+      assert(element.nameOffset >= 0, 'Invalid location data for element: $fullyQualifiedName');
+      assert(lineInfo != null, 'No lineInfo data available for element: $fullyQualifiedName');
+      if (element.nameOffset >= 0) {
+        _characterLocation = lineInfo?.getLocation(element.nameOffset);
+      }
     }
-    return _lineAndColumn;
+    return _characterLocation;
   }
 
+  CompilationUnitElement get compilationUnitElement => element.getAncestor((e) => e is CompilationUnitElement);
+
   bool get hasAnnotations => annotations.isNotEmpty;
 
   @override
@@ -4191,8 +4247,8 @@ abstract class ModelElement extends Canonicalization
                 warn(PackageWarning.toolError, message: message),
             content: basicMatch[2],
             environment: {
-              'SOURCE_LINE': lineAndColumn?.item1?.toString(),
-              'SOURCE_COLUMN': lineAndColumn?.item2?.toString(),
+              'SOURCE_LINE': characterLocation?.lineNumber.toString(),
+              'SOURCE_COLUMN': characterLocation?.columnNumber.toString(),
               'SOURCE_PATH': (sourceFileName == null ||
                       package?.packagePath == null)
                   ? null
@@ -5153,13 +5209,6 @@ class PackageGraph {
           warnable = warnable.enclosingElement;
         }
       }
-      if (warnable is Accessor) {
-        // This might be part of a Field, if so, assign this warning to the field
-        // rather than the Accessor.
-        if ((warnable as Accessor).enclosingCombo != null) {
-          warnable = (warnable as Accessor).enclosingCombo;
-        }
-      }
     } else {
       // If we don't have an element, we need a message to disambiguate.
       assert(message != null);
@@ -5224,9 +5273,6 @@ class PackageGraph {
         break;
       case PackageWarning.unresolvedDocReference:
         warningMessage = "unresolved doc reference [${message}]";
-        if (referredFrom == null) {
-          referredFrom = warnable.documentationFrom;
-        }
         referredFromPrefix = 'in documentation inherited from';
         break;
       case PackageWarning.unknownMacro:
@@ -5273,14 +5319,14 @@ class PackageGraph {
     List<String> messageParts = [warningMessage];
     if (warnable != null) {
       messageParts
-          .add("${warnablePrefix} ${warnableName}: ${warnable.location ?? ''}");
+          .add("$warnablePrefix $warnableName: $warnable.location");
     }
     if (referredFrom != null) {
       for (Locatable referral in referredFrom) {
         if (referral != warnable) {
           var referredFromStrings = _safeWarnableName(referral);
           messageParts.add(
-              "${referredFromPrefix} ${referredFromStrings}: ${referral.location ?? ''}");
+              "$referredFromPrefix $referredFromStrings: ${referral.location}");
         }
       }
     }
@@ -5957,6 +6003,7 @@ abstract class MarkdownFileDocumentation
 class Category extends Nameable
     with
         Warnable,
+        Locatable,
         Canonicalization,
         MarkdownFileDocumentation,
         LibraryContainer,
@@ -6510,7 +6557,7 @@ class Parameter extends ModelElement implements EnclosedElement {
 abstract class SourceCodeMixin implements Documentable {
   ModelNode get modelNode;
 
-  Tuple2<int, int> get lineAndColumn;
+  CharacterLocation get characterLocation;
 
   Element get element;
 

lib/src/model.dart

@@ -88,7 +88,7 @@ class SourceLinker {
       excludes: config.linkToSourceExcludes,
       // TODO(jcollins-g): disallow defaulting?  Some elements come back without
       // a line number right now.
-      lineNumber: element.lineAndColumn?.item1 ?? 1,
+      lineNumber: element.characterLocation?.lineNumber ?? 1,
       sourceFileName: element.sourceFileName,
       revision: config.linkToSourceRevision,
       root: config.linkToSourceRoot,

lib/src/source_linker.dart

@@ -208,6 +208,9 @@ abstract class Warnable implements Canonicalization {
 abstract class Locatable {
   List<Locatable> get documentationFrom;
 
+  /// True if documentationFrom contains only one item, [this].
+  bool get documentationIsLocal => documentationFrom.length == 1 && identical(documentationFrom.first, this);
+
   String get fullyQualifiedName;
 
   String get href;

lib/src/warnings.dart

@@ -56,7 +56,7 @@ void main() {
       expect(outputLines, contains(matches('^  warning:')));
       expect(outputLines.last, matches(r'^found \d+ warnings and \d+ errors'));
       expect(outputDir.listSync(), isEmpty);
-    });
+    }, timeout: Timeout.factor(2));
 
     test('running --quiet is quiet and does generate docs', () async {
       Directory outputDir =

test/dartdoc_integration_test.dart

@@ -258,7 +258,7 @@ void main() {
         expect(p.name, 'test_package');
         expect(p.hasDocumentationFile, isTrue);
         // Total number of public libraries in test_package.
-        expect(packageGraph.defaultPackage.publicLibraries, hasLength(14));
+        expect(packageGraph.defaultPackage.publicLibraries, hasLength(15));
         expect(packageGraph.localPackages.length, equals(1));
       });
 
@@ -327,7 +327,7 @@ void main() {
       PackageGraph p = results.packageGraph;
       expect(p.defaultPackage.name, 'test_package');
       expect(p.defaultPackage.hasDocumentationFile, isTrue);
-      expect(p.localPublicLibraries, hasLength(13));
+      expect(p.localPublicLibraries, hasLength(14));
       expect(p.localPublicLibraries.map((lib) => lib.name).contains('fake'),
           isFalse);
     });