Fix line number cache

lib/src/line_number_cache.dart

@@ -8,6 +8,7 @@ import 'dart:collection';
 import 'dart:io';
 
 import 'package:dartdoc/src/tuple.dart';
+import 'package:path/path.dart' as pathLib;
 
 String _getNewlineChar(String contents) {
   if (contents.contains("\r\n")) {
@@ -22,12 +23,14 @@ String _getNewlineChar(String contents) {
 SplayTreeMap<int, int> _createLineNumbersMap(String contents) {
   var newlineChar = _getNewlineChar(contents);
   var offset = 0;
-  var lineNumber = 0;
+  var lineNumber = 1;
   var result = new SplayTreeMap<int, int>();
 
   do {
     result[offset] = lineNumber;
-    offset = contents.indexOf(newlineChar, offset + 1);
+    offset = (offset + 1 <= contents.length)
+        ? contents.indexOf(newlineChar, offset + 1)
+        : -1;
     lineNumber += 1;
   } while (offset != -1);
 
@@ -36,22 +39,19 @@ SplayTreeMap<int, int> _createLineNumbersMap(String contents) {
 
 final LineNumberCache lineNumberCache = new LineNumberCache();
 
-// TODO(kevmoo): this could use some testing
 class LineNumberCache {
-  final Map<String, String> __fileContents = <String, String>{};
   final Map<String, SplayTreeMap<int, int>> _lineNumbers =
       <String, SplayTreeMap<int, int>>{};
 
   Tuple2<int, int> lineAndColumn(String file, int offset) {
+    file = pathLib.canonicalize(file);
     var lineMap = _lineNumbers.putIfAbsent(
-        file, () => _createLineNumbersMap(_fileContents(file)));
+        file, () => _createLineNumbersMap(new File(file).readAsStringSync()));
     var lastKey = lineMap.lastKeyBefore(offset);
     if (lastKey != null) {
-      return new Tuple2(lineMap[lastKey] + 1, offset - lastKey);
+      return new Tuple2(lineMap[lastKey], offset - lastKey);
+    } else {
+      return new Tuple2(lineMap[0], offset - 0);
     }
-    return null;
   }
-
-  String _fileContents(String file) =>
-      __fileContents.putIfAbsent(file, () => new File(file).readAsStringSync());
 }

lib/src/model.dart

@@ -4701,9 +4701,10 @@ class PackageGraph {
     Set<ModelElement> precachedElements = new Set();
 
     Iterable<Future> precacheOneElement(ModelElement m) sync* {
-      for (ModelElement d in m.documentationFrom.where((d) => d.documentationComment != null)) {
-        if (needsPrecacheRegExp.hasMatch(d.documentationComment)
-            && !precachedElements.contains(d)) {
+      for (ModelElement d
+          in m.documentationFrom.where((d) => d.documentationComment != null)) {
+        if (needsPrecacheRegExp.hasMatch(d.documentationComment) &&
+            !precachedElements.contains(d)) {
           precachedElements.add(d);
           yield d._precacheLocalDocs();
         }

test/line_number_cache_test.dart

@@ -0,0 +1,65 @@
+// 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.cache_test;
+
+import 'dart:io';
+
+import 'package:dartdoc/src/line_number_cache.dart';
+import 'package:path/path.dart' as pathLib;
+import 'package:test/test.dart';
+import 'package:dartdoc/src/tuple.dart';
+
+void main() {
+  group('Verify basic line cache behavior', () {
+    Directory _tempDir;
+
+    setUp(() {
+      _tempDir = Directory.systemTemp.createTempSync('line_number_cache');
+    });
+
+    tearDown(() {
+      _tempDir.deleteSync(recursive: true);
+    });
+
+    test('validate empty file behavior', () {
+      File emptyFile = File(pathLib.join(_tempDir.path, 'empty_file'))
+        ..writeAsStringSync('');
+      LineNumberCache cache = LineNumberCache();
+      expect(cache.lineAndColumn(emptyFile.path, 0), equals(Tuple2(1, 0)));
+    });
+
+    test('single line without newline', () {
+      File singleLineWithoutNewline =
+          File(pathLib.join(_tempDir.path, 'single_line'))
+            ..writeAsStringSync('a single line');
+      LineNumberCache cache = LineNumberCache();
+      expect(cache.lineAndColumn(singleLineWithoutNewline.path, 2),
+          equals(Tuple2(1, 2)));
+      expect(cache.lineAndColumn(singleLineWithoutNewline.path, 0),
+          equals(Tuple2(1, 0)));
+    });
+
+    test('multiple line without trailing newline', () {
+      File multipleLine = File(pathLib.join(_tempDir.path, 'multiple_line'))
+        ..writeAsStringSync('This is the first line\nThis is the second line');
+      LineNumberCache cache = LineNumberCache();
+      expect(cache.lineAndColumn(multipleLine.path, 60), equals(Tuple2(2, 38)));
+      expect(cache.lineAndColumn(multipleLine.path, 30), equals(Tuple2(2, 8)));
+      expect(cache.lineAndColumn(multipleLine.path, 5), equals(Tuple2(1, 5)));
+      expect(cache.lineAndColumn(multipleLine.path, 0), equals(Tuple2(1, 0)));
+    });
+
+    test('multiple lines with trailing newline', () {
+      File multipleLine = File(pathLib.join(_tempDir.path, 'multiple_line'))
+        ..writeAsStringSync(
+            'This is the first line\nThis is the second line\n');
+      LineNumberCache cache = LineNumberCache();
+      expect(cache.lineAndColumn(multipleLine.path, 60), equals(Tuple2(3, 14)));
+      expect(cache.lineAndColumn(multipleLine.path, 30), equals(Tuple2(2, 8)));
+      expect(cache.lineAndColumn(multipleLine.path, 5), equals(Tuple2(1, 5)));
+      expect(cache.lineAndColumn(multipleLine.path, 0), equals(Tuple2(1, 0)));
+    });
+  });
+}

test/model_test.dart

@@ -88,33 +88,47 @@ void main() {
   // when the feature is enabled by default.
   group('Experiments', () {
     Library main;
-    TopLevelVariable aComplexSet, inferredTypeSet, specifiedSet, untypedMap, typedSet;
+    TopLevelVariable aComplexSet,
+        inferredTypeSet,
+        specifiedSet,
+        untypedMap,
+        typedSet;
 
     setUpAll(() {
-      main = packageGraphExperiments.libraries.firstWhere((lib) => lib.name == 'main');
+      main = packageGraphExperiments.libraries
+          .firstWhere((lib) => lib.name == 'main');
       aComplexSet = main.constants.firstWhere((v) => v.name == 'aComplexSet');
-      inferredTypeSet = main.constants.firstWhere((v) => v.name == 'inferredTypeSet');
+      inferredTypeSet =
+          main.constants.firstWhere((v) => v.name == 'inferredTypeSet');
       specifiedSet = main.constants.firstWhere((v) => v.name == 'specifiedSet');
       untypedMap = main.constants.firstWhere((v) => v.name == 'untypedMap');
       typedSet = main.constants.firstWhere((v) => v.name == 'typedSet');
     });
 
     test('Set literals test', () {
       expect(aComplexSet.modelType.name, equals('Set'));
-      expect(aComplexSet.modelType.typeArguments.map((a) => a.name).toList(), equals(['AClassContainingLiterals']));
-      expect(aComplexSet.constantValue, equals('const {const AClassContainingLiterals(3, 5)}'));
+      expect(aComplexSet.modelType.typeArguments.map((a) => a.name).toList(),
+          equals(['AClassContainingLiterals']));
+      expect(aComplexSet.constantValue,
+          equals('const {const AClassContainingLiterals(3, 5)}'));
       expect(inferredTypeSet.modelType.name, equals('Set'));
-      expect(inferredTypeSet.modelType.typeArguments.map((a) => a.name).toList(), equals(['num']));
+      expect(
+          inferredTypeSet.modelType.typeArguments.map((a) => a.name).toList(),
+          equals(['num']));
       expect(inferredTypeSet.constantValue, equals('const {1, 2.5, 3}'));
       expect(specifiedSet.modelType.name, equals('Set'));
-      expect(specifiedSet.modelType.typeArguments.map((a) => a.name).toList(), equals(['int']));
+      expect(specifiedSet.modelType.typeArguments.map((a) => a.name).toList(),
+          equals(['int']));
       expect(specifiedSet.constantValue, equals('const {}'));
       expect(untypedMap.modelType.name, equals('Map'));
-      expect(untypedMap.modelType.typeArguments.map((a) => a.name).toList(), equals(['dynamic', 'dynamic']));
+      expect(untypedMap.modelType.typeArguments.map((a) => a.name).toList(),
+          equals(['dynamic', 'dynamic']));
       expect(untypedMap.constantValue, equals('const {}'));
       expect(typedSet.modelType.name, equals('Set'));
-      expect(typedSet.modelType.typeArguments.map((a) => a.name).toList(), equals(['String']));
-      expect(typedSet.constantValue, matches(new RegExp(r'const <String>\s?{}')));
+      expect(typedSet.modelType.typeArguments.map((a) => a.name).toList(),
+          equals(['String']));
+      expect(typedSet.constantValue,
+          matches(new RegExp(r'const <String>\s?{}')));
     });
   });
 
@@ -128,7 +142,8 @@ void main() {
     Method invokeToolNonCanonical, invokeToolNonCanonicalSubclass;
     Method invokeToolPrivateLibrary, invokeToolPrivateLibraryOriginal;
     Method invokeToolParentDoc, invokeToolParentDocOriginal;
-    final RegExp packageInvocationIndexRegexp = new RegExp(r'PACKAGE_INVOCATION_INDEX: (\d+)');
+    final RegExp packageInvocationIndexRegexp =
+        new RegExp(r'PACKAGE_INVOCATION_INDEX: (\d+)');
 
     setUpAll(() {
       _NonCanonicalToolUser = fakeLibrary.allClasses
@@ -165,13 +180,16 @@ void main() {
       packageGraph.allLocalModelElements.forEach((m) => m.documentation);
     });
 
-    test('invokes tool when inherited documentation is the only means for it to be seen', () {
+    test(
+        'invokes tool when inherited documentation is the only means for it to be seen',
+        () {
       // Verify setup of the test is correct.
       expect(invokeToolParentDoc.isCanonical, isTrue);
       expect(invokeToolParentDoc.documentationComment, isNull);
       // Error message here might look strange due to toString() on Methods, but if this
       // fails that means we don't have the correct invokeToolParentDoc instance.
-      expect(invokeToolParentDoc.documentationFrom, contains(invokeToolParentDocOriginal));
+      expect(invokeToolParentDoc.documentationFrom,
+          contains(invokeToolParentDocOriginal));
       // Tool should be substituted out here.
       expect(invokeToolParentDoc.documentation, isNot(contains('{@tool')));
     });
@@ -187,18 +205,20 @@ void main() {
             equals(packageInvocationIndexRegexp
                 .firstMatch(invokeToolNonCanonicalSubclass.documentation)
                 .group(1)));
-        expect(invokeToolPrivateLibrary.documentation, isNot(contains('{@tool')));
+        expect(
+            invokeToolPrivateLibrary.documentation, isNot(contains('{@tool')));
         expect(
             invokeToolPrivateLibraryOriginal.documentation, contains('{@tool'));
       });
 
       test('Documentation borrowed from implementer case', () {
-        expect(packageInvocationIndexRegexp
-            .firstMatch(invokeToolParentDoc.documentation)
-            .group(1),
-        equals(packageInvocationIndexRegexp
-            .firstMatch(invokeToolParentDocOriginal.documentation)
-            .group(1)));
+        expect(
+            packageInvocationIndexRegexp
+                .firstMatch(invokeToolParentDoc.documentation)
+                .group(1),
+            equals(packageInvocationIndexRegexp
+                .firstMatch(invokeToolParentDocOriginal.documentation)
+                .group(1)));
       });
     });
 
@@ -3042,8 +3062,10 @@ String topLevelFunction(int param1, bool param2, Cool coolBeans,
     });
 
     test('PRETTY_COLORS', () {
-      expect(prettyColorsConstant.constantValue, matches(new RegExp(
-          r"const <String>\s?\[COLOR_GREEN, COLOR_ORANGE, 'blue'\]")));
+      expect(
+          prettyColorsConstant.constantValue,
+          matches(new RegExp(
+              r"const <String>\s?\[COLOR_GREEN, COLOR_ORANGE, 'blue'\]")));
     });
 
     test('MY_CAT is linked', () {

test/src/utils.dart

@@ -29,7 +29,8 @@ PackageGraph testPackageGraphSdk;
 
 final Directory testPackageBadDir = new Directory('testing/test_package_bad');
 final Directory testPackageDir = new Directory('testing/test_package');
-final Directory testPackageExperimentsDir = new Directory('testing/test_package_experiments');
+final Directory testPackageExperimentsDir =
+    new Directory('testing/test_package_experiments');
 final Directory testPackageMinimumDir =
     new Directory('testing/test_package_minimum');
 final Directory testPackageWithEmbedderYaml =
@@ -82,8 +83,8 @@ void init({List<String> additionalArguments}) async {
 
   testPackageGraphExperiments = await bootBasicPackage(
       'testing/test_package_experiments', [],
-      additionalArguments: additionalArguments + ['--enable-experiment', 'set-literals']
-  );
+      additionalArguments:
+          additionalArguments + ['--enable-experiment', 'set-literals']);
 
   testPackageGraphSmall = await bootBasicPackage(
       'testing/test_package_small', [],

testing/test_package/lib/fake.dart

@@ -1032,7 +1032,8 @@ abstract class ImplementingClassForTool {
 }
 
 /// The method [invokeToolParentDoc] gets its documentation from an interface class.
-abstract class CanonicalPrivateInheritedToolUser implements ImplementingClassForTool {
+abstract class CanonicalPrivateInheritedToolUser
+    implements ImplementingClassForTool {
   @override
   void invokeToolParentDoc() {
     print('hello, tool world');

testing/test_package_experiments/lib/main.dart

@@ -10,7 +10,8 @@ class AClassContainingLiterals {
   const AClassContainingLiterals(this.value1, this.value2);
 
   @override
-  bool operator==(Object other) => other is AClassContainingLiterals && value1 == other.value1;
+  bool operator ==(Object other) =>
+      other is AClassContainingLiterals && value1 == other.value1;
 }
 
-const aComplexSet = {AClassContainingLiterals(3, 5)};
+const aComplexSet = {AClassContainingLiterals(3, 5)};