diff --git a/lib/src/line_number_cache.dart b/lib/src/line_number_cache.dart deleted file mode 100644 index 0f5e3b9f2e..0000000000 --- a/lib/src/line_number_cache.dart +++ /dev/null @@ -1,57 +0,0 @@ -// Copyright (c) 2015, 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; - -import 'dart:collection'; -import 'dart:io'; - -import 'package:dartdoc/src/tuple.dart'; - -String _getNewlineChar(String contents) { - if (contents.contains("\r\n")) { - return "\r\n"; - } else if (contents.contains("\r")) { - return "\r"; - } else { - return "\n"; - } -} - -SplayTreeMap _createLineNumbersMap(String contents) { - var newlineChar = _getNewlineChar(contents); - var offset = 0; - var lineNumber = 0; - var result = SplayTreeMap(); - - do { - result[offset] = lineNumber; - offset = contents.indexOf(newlineChar, offset + 1); - lineNumber += 1; - } while (offset != -1); - - return result; -} - -final LineNumberCache lineNumberCache = LineNumberCache(); - -// TODO(kevmoo): this could use some testing -class LineNumberCache { - final Map __fileContents = {}; - final Map> _lineNumbers = - >{}; - - Tuple2 lineAndColumn(String file, int offset) { - var lineMap = _lineNumbers.putIfAbsent( - file, () => _createLineNumbersMap(_fileContents(file))); - var lastKey = lineMap.lastKeyBefore(offset); - if (lastKey != null) { - return Tuple2(lineMap[lastKey] + 1, offset - lastKey); - } - return null; - } - - String _fileContents(String file) => - __fileContents.putIfAbsent(file, () => File(file).readAsStringSync()); -} diff --git a/lib/src/markdown_processor.dart b/lib/src/markdown_processor.dart index 486334bf57..bda5eee254 100644 --- a/lib/src/markdown_processor.dart +++ b/lib/src/markdown_processor.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 '${htmlEscape.convert(codeRef)}'; } diff --git a/lib/src/model.dart b/lib/src/model.dart index c44a220345..3167ac7d17 100644 --- a/lib/src/model.dart +++ b/lib/src/model.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 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 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 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 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 _lineAndColumn; - bool _isLineNumberComputed = false; + CharacterLocation _characterLocation; + bool _characterLocationIsSet = false; @override - Tuple2 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 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 get lineAndColumn; + CharacterLocation get characterLocation; Element get element; diff --git a/lib/src/source_linker.dart b/lib/src/source_linker.dart index 7a2def8384..992e5fe626 100644 --- a/lib/src/source_linker.dart +++ b/lib/src/source_linker.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, diff --git a/lib/src/warnings.dart b/lib/src/warnings.dart index 1b96459dc8..97542bfc20 100644 --- a/lib/src/warnings.dart +++ b/lib/src/warnings.dart @@ -208,6 +208,9 @@ abstract class Warnable implements Canonicalization { abstract class Locatable { List 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; diff --git a/test/dartdoc_integration_test.dart b/test/dartdoc_integration_test.dart index 16eb6f1c0a..8d7c138dd7 100644 --- a/test/dartdoc_integration_test.dart +++ b/test/dartdoc_integration_test.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 = diff --git a/test/dartdoc_test.dart b/test/dartdoc_test.dart index c47ade0f5e..078d3e8d70 100644 --- a/test/dartdoc_test.dart +++ b/test/dartdoc_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); }); diff --git a/test/model_test.dart b/test/model_test.dart index 4398476726..4944c17a8f 100644 --- a/test/model_test.dart +++ b/test/model_test.dart @@ -494,7 +494,7 @@ void main() { expect( packageGraph .localPackages.first.defaultCategory.publicLibraries.length, - equals(7)); + equals(8)); }); test('Verify libraries with multiple categories show up in multiple places', @@ -518,7 +518,7 @@ void main() { expect( packageGraph .localPackages.first.defaultCategory.publicLibraries.length, - equals(7)); + equals(8)); }); }); @@ -588,7 +588,7 @@ void main() { }); test('libraries', () { - expect(packageGraph.localPublicLibraries, hasLength(12)); + expect(packageGraph.localPublicLibraries, hasLength(13)); expect(interceptorsLib.isPublic, isFalse); }); @@ -603,7 +603,7 @@ void main() { Package package = packageGraph.localPackages.first; expect(package.name, 'test_package'); - expect(package.publicLibraries, hasLength(12)); + expect(package.publicLibraries, hasLength(13)); }); test('multiple packages, sorted default', () { @@ -752,6 +752,10 @@ void main() { expect(exLibrary.name, 'ex'); }); + test('has a line number and column', () { + expect(exLibrary.characterLocation, isNotNull); + }); + test('packageName', () { expect(exLibrary.packageName, 'test_package'); }); @@ -1734,6 +1738,10 @@ void main() { .firstWhere((f) => f.name == 'overrideByModifierClass'); }); + test('does have a line number and column', () { + expect(GenericMixin.characterLocation, isNotNull); + }); + test(('Verify mixin member is available in findRefElementCache'), () { expect(packageGraph.findRefElementCache['GenericMixin.mixinMember'], isNotEmpty); @@ -1866,6 +1874,10 @@ void main() { expect(Apple.fullyQualifiedName, 'ex.Apple'); }); + test('does have a line number and column', () { + expect(Apple.characterLocation, isNotNull); + }); + test('we got the classes we expect', () { expect(Apple.name, equals('Apple')); expect(B.name, equals('B')); @@ -1892,7 +1904,7 @@ void main() { }); test('correctly finds all the classes', () { - expect(classes, hasLength(30)); + expect(classes, hasLength(31)); }); test('abstract', () { @@ -2115,6 +2127,10 @@ void main() { expect(ext.fullyQualifiedName, 'ex.AppleExtension'); }); + test('does have a line number and column', () { + expect(ext.characterLocation, isNotNull); + }); + test('has enclosing element', () { expect(ext.enclosingElement.name, equals(exLibrary.name)); }); @@ -2173,9 +2189,11 @@ void main() { group('Enum', () { Enum animal; + Method animalToString; setUpAll(() { animal = exLibrary.enums.firstWhere((e) => e.name == 'Animal'); + animalToString = animal.allInstanceMethods.firstWhere((m) => m.name == 'toString'); /// Trigger code reference resolution animal.documentationAsHtml; @@ -2185,6 +2203,12 @@ void main() { expect(animal.fullyQualifiedName, 'ex.Animal'); }); + test('toString() method location is handled specially', () { + expect(animalToString.characterLocation, isNotNull); + expect(animalToString.characterLocation.toString(), + equals(animal.characterLocation.toString())); + }); + test('has enclosing element', () { expect(animal.enclosingElement.name, equals(exLibrary.name)); }); @@ -2269,6 +2293,10 @@ void main() { expect(thisIsAsync.fullyQualifiedName, 'fake.thisIsAsync'); }); + test('does have a line number and column', () { + expect(thisIsAsync.characterLocation, isNotNull); + }); + test('has enclosing element', () { expect(f1.enclosingElement.name, equals(exLibrary.name)); }); @@ -2554,6 +2582,10 @@ String topLevelFunction(int param1, bool param2, Cool coolBeans, .singleWhere((m) => m.name == 'getAFunctionReturningBool'); }); + test('does have a line number and column', () { + expect(abstractMethod.characterLocation, isNotNull); + }); + test('verify parameter types are correctly displayed', () { expect( getAFunctionReturningVoid.linkedReturnType, @@ -2805,6 +2837,14 @@ String topLevelFunction(int param1, bool param2, Cool coolBeans, .firstWhere((f) => f.name == 'covariantSetter'); }); + test('Fields always have line and column information', () { + expect(implicitGetterExplicitSetter.characterLocation, isNotNull); + expect(explicitGetterImplicitSetter.characterLocation, isNotNull); + expect(explicitGetterSetter.characterLocation, isNotNull); + expect(constField.characterLocation, isNotNull); + expect(aProperty.characterLocation, isNotNull); + }); + test('covariant fields are recognized', () { expect(covariantField.isCovariant, isTrue); expect(covariantField.featuresAsString, contains('covariant')); @@ -3308,7 +3348,9 @@ String topLevelFunction(int param1, bool param2, Cool coolBeans, Constructor appleDefaultConstructor, constCatConstructor; Constructor appleConstructorFromString; Constructor constructorTesterDefault, constructorTesterFromSomething; - Class apple, constCat, constructorTester, referToADefaultConstructor; + Constructor syntheticConstructor; + Class apple, constCat, constructorTester, referToADefaultConstructor, + withSyntheticConstructor; setUpAll(() { apple = exLibrary.classes.firstWhere((c) => c.name == 'Apple'); constCat = exLibrary.classes.firstWhere((c) => c.name == 'ConstantCat'); @@ -3325,6 +3367,8 @@ String topLevelFunction(int param1, bool param2, Cool coolBeans, .firstWhere((c) => c.name == 'ConstructorTester.fromSomething'); referToADefaultConstructor = fakeLibrary.classes .firstWhere((c) => c.name == 'ReferToADefaultConstructor'); + withSyntheticConstructor = exLibrary.classes.firstWhere((c) => c.name == 'WithSyntheticConstructor'); + syntheticConstructor = withSyntheticConstructor.defaultConstructor; }); test('calculates comment references to classes vs. constructors correctly', @@ -3351,6 +3395,18 @@ String topLevelFunction(int param1, bool param2, Cool coolBeans, appleConstructorFromString.fullyQualifiedName, 'ex.Apple.fromString'); }); + test('has a line number and column', () { + expect(appleDefaultConstructor.characterLocation, isNotNull); + expect(syntheticConstructor.characterLocation, isNotNull); + // The default constructor should not reference the class for location + // because it is not synthetic. + expect(appleDefaultConstructor.characterLocation.toString(), + isNot(equals(apple.characterLocation.toString()))); + // A synthetic class should reference its parent. + expect(syntheticConstructor.characterLocation.toString(), + equals(withSyntheticConstructor.characterLocation.toString())); + }); + test('has enclosing element', () { expect(appleDefaultConstructor.enclosingElement.name, equals(apple.name)); }); diff --git a/testing/test_package/lib/completely_empty_lib.dart b/testing/test_package/lib/completely_empty_lib.dart new file mode 100644 index 0000000000..e69de29bb2 diff --git a/testing/test_package/lib/example.dart b/testing/test_package/lib/example.dart index 4cba5f5112..7274be9313 100644 --- a/testing/test_package/lib/example.dart +++ b/testing/test_package/lib/example.dart @@ -619,6 +619,9 @@ abstract class HtmlInjection { void injectHtmlFromTool(); } +/// Just a class with a synthetic constructor. +class WithSyntheticConstructor {} + /// Extension on a class defined in the package extension AnExtension on WithGeneric { int call(String s) => 0;