From 0577ca1d20388bc13ed132bea587991766e3d431 Mon Sep 17 00:00:00 2001 From: Sam Rawlins Date: Mon, 19 Aug 2024 12:59:16 -0700 Subject: [PATCH] Improve logic of calculating a "fully qualified name" (#3836) For improvements that I'm working on in another CL, it made sense to tidy up this code. The `_buildFullyQualifiedName` function was unnecessarily recursive and complex. And then the `_fullyQualifiedNameWithoutLibrary` function (a) has a complex name and (b) also worked _further_ to do undo some work done in `_buildFullyQualifiedName`. This CL tidies these two "fields". * Combine `fullyQualifiedName` and `_buildFullyQualifiedName` into one getter (removing one field). * Combine `fullyQualifiedNameWithoutLibrary` and `_fullyQualifiedNameWithoutLibrary` into one late final field, remove recursion, and simplify name to `qualifiedName`. Also mark as `@visibleForOverriding`. It should be private but because of the mixin hierarchy, it is declared in one file, and defined in another. * Improve documentation. --- .../templates.runtime_renderers.dart | 71 ++++++------------- lib/src/model/documentation_comment.dart | 7 +- lib/src/model/model_element.dart | 30 ++++---- lib/src/model/nameable.dart | 12 +++- test/constant_values_test.dart | 7 +- test/constructors_test.dart | 19 +++++ test/end2end/model_test.dart | 38 ---------- test/extension_types_test.dart | 12 ++-- test/extensions_test.dart | 20 ++++-- test/type_parameter_test.dart | 9 +++ test/typedef_test.dart | 5 +- 11 files changed, 107 insertions(+), 123 deletions(-) diff --git a/lib/src/generator/templates.runtime_renderers.dart b/lib/src/generator/templates.runtime_renderers.dart index 28dca9890b..83df2b9adb 100644 --- a/lib/src/generator/templates.runtime_renderers.dart +++ b/lib/src/generator/templates.runtime_renderers.dart @@ -3974,30 +3974,6 @@ class _Renderer_DocumentationComment getters: _invisibleGetters['Documentation']!); }, ), - 'fullyQualifiedNameWithoutLibrary': Property( - getValue: (CT_ c) => c.fullyQualifiedNameWithoutLibrary, - renderVariable: - (CT_ c, Property self, List remainingNames) { - if (remainingNames.isEmpty) { - return self.getValue(c).toString(); - } - var name = remainingNames.first; - var nextProperty = - _Renderer_String.propertyMap().getValue(name); - return nextProperty.renderVariable( - self.getValue(c) as String, - nextProperty, - [...remainingNames.skip(1)]); - }, - isNullValue: (CT_ c) => - c.fullyQualifiedNameWithoutLibrary == null, - renderValue: (CT_ c, RendererBase r, - List ast, StringSink sink) { - _render_String(c.fullyQualifiedNameWithoutLibrary!, ast, - r.template, sink, - parent: r); - }, - ), 'hasDocumentationComment': Property( getValue: (CT_ c) => c.hasDocumentationComment, renderVariable: (CT_ c, Property self, @@ -10313,29 +10289,6 @@ class _Renderer_ModelElement extends RendererBase { parent: r); }, ), - 'fullyQualifiedNameWithoutLibrary': Property( - getValue: (CT_ c) => c.fullyQualifiedNameWithoutLibrary, - renderVariable: - (CT_ c, Property self, List remainingNames) { - if (remainingNames.isEmpty) { - return self.getValue(c).toString(); - } - var name = remainingNames.first; - var nextProperty = - _Renderer_String.propertyMap().getValue(name); - return nextProperty.renderVariable( - self.getValue(c) as String, - nextProperty, - [...remainingNames.skip(1)]); - }, - isNullValue: (CT_ c) => false, - renderValue: (CT_ c, RendererBase r, - List ast, StringSink sink) { - _render_String(c.fullyQualifiedNameWithoutLibrary, ast, - r.template, sink, - parent: r); - }, - ), 'hasAnnotations': Property( getValue: (CT_ c) => c.hasAnnotations, renderVariable: (CT_ c, Property self, @@ -10783,6 +10736,28 @@ class _Renderer_ModelElement extends RendererBase { parent: r, getters: _invisibleGetters['Context']!); }, ), + 'qualifiedName': Property( + getValue: (CT_ c) => c.qualifiedName, + renderVariable: + (CT_ c, Property self, List remainingNames) { + if (remainingNames.isEmpty) { + return self.getValue(c).toString(); + } + var name = remainingNames.first; + var nextProperty = + _Renderer_String.propertyMap().getValue(name); + return nextProperty.renderVariable( + self.getValue(c) as String, + nextProperty, + [...remainingNames.skip(1)]); + }, + isNullValue: (CT_ c) => false, + renderValue: (CT_ c, RendererBase r, + List ast, StringSink sink) { + _render_String(c.qualifiedName, ast, r.template, sink, + parent: r); + }, + ), 'sourceCode': Property( getValue: (CT_ c) => c.sourceCode, renderVariable: @@ -15888,11 +15863,11 @@ const _invisibleGetters = { 'documentationLocal', 'element', 'elementDocumentation', - 'fullyQualifiedNameWithoutLibrary', 'hasDocumentationComment', 'hasNodoc', 'needsPrecache', 'pathContext', + 'qualifiedName', 'sourceFileName' }, 'Element': { diff --git a/lib/src/model/documentation_comment.dart b/lib/src/model/documentation_comment.dart index 7f0047244a..35dbb5ef95 100644 --- a/lib/src/model/documentation_comment.dart +++ b/lib/src/model/documentation_comment.dart @@ -142,7 +142,10 @@ mixin DocumentationComment String? get sourceFileName; - String? get fullyQualifiedNameWithoutLibrary; + /// The name of this element, qualified with any enclosing element(s), up to + /// but not including an enclosing library. + @visibleForOverriding + String get qualifiedName; p.Context get pathContext; @@ -279,7 +282,7 @@ mixin DocumentationComment 'PACKAGE_PATH': package.packagePath, 'PACKAGE_NAME': package.name, 'LIBRARY_NAME': library?.fullyQualifiedName, - 'ELEMENT_NAME': fullyQualifiedNameWithoutLibrary, + 'ELEMENT_NAME': qualifiedName, 'INVOCATION_INDEX': invocationIndex.toString(), 'PACKAGE_INVOCATION_INDEX': (package.toolInvocationIndex++).toString(), }; diff --git a/lib/src/model/model_element.dart b/lib/src/model/model_element.dart index d0b9b719f5..753adaed66 100644 --- a/lib/src/model/model_element.dart +++ b/lib/src/model/model_element.dart @@ -5,6 +5,7 @@ /// The models used to represent Dart code. library; +import 'dart:collection'; import 'dart:convert'; import 'package:analyzer/dart/element/element.dart'; @@ -571,18 +572,21 @@ abstract class ModelElement /// documented. String get filePath; - /// Returns the fully qualified name. - /// - /// For example: 'libraryName.className.methodName' @override - late final String fullyQualifiedName = _buildFullyQualifiedName(this, name); - - late final String _fullyQualifiedNameWithoutLibrary = - fullyQualifiedName.replaceFirst('${library.fullyQualifiedName}.', ''); + String get fullyQualifiedName => + this is Library ? qualifiedName : '${library.name}.$qualifiedName'; @override - String get fullyQualifiedNameWithoutLibrary => - _fullyQualifiedNameWithoutLibrary; + late final String qualifiedName = () { + var enclosingElement = this.enclosingElement; + + var result = name; + while (enclosingElement != null && enclosingElement is! Library) { + result = '${enclosingElement.name}.$result'; + enclosingElement = enclosingElement.enclosingElement; + } + return result; + }(); @override String get sourceFileName => element.source!.fullName; @@ -783,14 +787,6 @@ abstract class ModelElement @override String toString() => '$runtimeType $name'; - String _buildFullyQualifiedName(ModelElement e, String fullyQualifiedName) { - final enclosingElement = e.enclosingElement; - return enclosingElement == null - ? fullyQualifiedName - : _buildFullyQualifiedName( - enclosingElement, '${enclosingElement.name}.$fullyQualifiedName'); - } - @internal @override CommentReferable get definingCommentReferable { diff --git a/lib/src/model/nameable.dart b/lib/src/model/nameable.dart index 27c3134a36..b8229554ff 100644 --- a/lib/src/model/nameable.dart +++ b/lib/src/model/nameable.dart @@ -16,8 +16,16 @@ import 'package:dartdoc/src/model/package_graph.dart'; mixin Nameable { String get name; - /// A qualified name, mostly for use in the web search functionality, and for - /// warnings printed in the terminal; not for display use in rendered HTML. + /// A "fully" qualified name, mostly for use in the web search functionality, + /// and for warnings printed in the terminal; not for display use in rendered + /// HTML. + /// + /// "Fully" means the name is qualified through the library. For example, a + /// method named 'baz' in a class named 'Bar' in a library named 'foo' has a + /// fully qualified name of 'foo.Bar.baz'. + /// + /// As dartdoc can document multiple packages at once, note that such + /// qualifying names may not be unique across all documented packages. String get fullyQualifiedName => name; /// The name to use as text in the rendered documentation. diff --git a/test/constant_values_test.dart b/test/constant_values_test.dart index c5cb1fbcd3..69f8d81713 100644 --- a/test/constant_values_test.dart +++ b/test/constant_values_test.dart @@ -10,7 +10,7 @@ import 'src/utils.dart'; void main() { defineReflectiveSuite(() { - defineReflectiveTests(ConstantValuesWithConstructorTearoffsTest); + defineReflectiveTests(ConstantValuesTest); if (namedArgumentsAnywhereAllowed) { defineReflectiveTests(ConstantValuesWithNamedArgumentsAnywhereTest); } @@ -22,9 +22,9 @@ void main() { // test_reflective_loader migration. @reflectiveTest -class ConstantValuesWithConstructorTearoffsTest extends DartdocTestBase { +class ConstantValuesTest extends DartdocTestBase { @override - String get libraryName => 'constructor_tearoffs'; + String get libraryName => 'constant_values'; void test_nonGenericFunctionReference() async { var library = await bootPackageWithLibrary(''' @@ -32,6 +32,7 @@ void func() {} const aFunc = func; '''); var aFuncConstant = library.constants.named('aFunc'); + expect(aFuncConstant.fullyQualifiedName, equals('constant_values.aFunc')); expect(aFuncConstant.constantValue, equals('func')); } diff --git a/test/constructors_test.dart b/test/constructors_test.dart index d6e9978a61..81f744910b 100644 --- a/test/constructors_test.dart +++ b/test/constructors_test.dart @@ -28,6 +28,8 @@ abstract final class C { '''); var c = library.classes.named('C').constructors.first; expect(c.name, equals('C')); + // TODO(srawlins): This should be `constructors.C.new`. + expect(c.fullyQualifiedName, 'constructors.C.C'); expect(c.isPublic, isTrue); expect(c.documentationAsHtml, '

Constructor.

'); } @@ -41,6 +43,8 @@ abstract final class C { '''); var c = library.classes.named('C').constructors.first; expect(c.name, equals('C')); + // TODO(srawlins): This should be `constructors.C.new`. + expect(c.fullyQualifiedName, 'constructors.C.C'); expect(c.isPublic, isFalse); expect(c.documentationAsHtml, '

Constructor.

'); } @@ -54,6 +58,8 @@ abstract interface class C { '''); var c = library.classes.named('C').constructors.first; expect(c.name, equals('C')); + // TODO(srawlins): This should be `constructors.C.new`. + expect(c.fullyQualifiedName, 'constructors.C.C'); expect(c.isPublic, isFalse); expect(c.documentationAsHtml, '

Constructor.

'); } @@ -67,6 +73,7 @@ class C { '''); var c = library.classes.named('C').constructors.first; expect(c.name, equals('C._')); + expect(c.fullyQualifiedName, 'constructors.C._'); expect(c.isPublic, isFalse); expect(c.documentationAsHtml, '

Constructor.

'); } @@ -103,6 +110,7 @@ class C { '''); var c = library.classes.named('C').constructors.first; expect(c.name, equals('C.named')); + expect(c.fullyQualifiedName, 'constructors.C.named'); expect(c.isPublic, isTrue); expect(c.documentationAsHtml, '

Constructor.

'); } @@ -129,6 +137,8 @@ class C { '''); var c = library.classes.named('C').constructors.first; expect(c.name, equals('C')); + // TODO(srawlins): This should be `constructors.C.new`. + expect(c.fullyQualifiedName, 'constructors.C.C'); expect(c.isPublic, isTrue); expect(c.documentationAsHtml, '

Constructor.

'); } @@ -156,6 +166,7 @@ enum E { '''); var e = library.enums.named('E').constructors.first; expect(e.name, equals('E.named')); + expect(e.fullyQualifiedName, 'constructors.E.named'); expect(e.isPublic, isFalse); expect(e.documentationAsHtml, '

Constructor.

'); } @@ -170,6 +181,8 @@ enum E { '''); var e = library.enums.named('E').constructors.first; expect(e.name, equals('E')); + // TODO(srawlins): This should be `constructors.E.new`. + expect(e.fullyQualifiedName, 'constructors.E.E'); expect(e.isPublic, isFalse); expect(e.documentationAsHtml, '

Constructor.

'); } @@ -184,6 +197,7 @@ extension type ET(int it) { var etNamed = library.extensionTypes.named('ET').constructors.named('ET.named'); expect(etNamed.name, equals('ET.named')); + expect(etNamed.fullyQualifiedName, 'constructors.ET.named'); expect(etNamed.isPublic, isTrue); expect(etNamed.documentationAsHtml, '

Constructor.

'); } @@ -195,6 +209,7 @@ extension type ET.named(int it) {} var etNamed = library.extensionTypes.named('ET').constructors.named('ET.named'); expect(etNamed.name, equals('ET.named')); + expect(etNamed.fullyQualifiedName, 'constructors.ET.named'); expect(etNamed.isPublic, isTrue); } @@ -204,6 +219,8 @@ extension type ET(int it) {} '''); var et = library.extensionTypes.named('ET').constructors.named('ET'); expect(et.name, equals('ET')); + // TODO(srawlins): This should be `constructors.ET.new`. + expect(et.fullyQualifiedName, 'constructors.ET.ET'); expect(et.isPublic, isTrue); } @@ -216,6 +233,8 @@ extension type ET.named(int it) { '''); var etNamed = library.extensionTypes.named('ET').constructors.named('ET'); expect(etNamed.name, equals('ET')); + // TODO(srawlins): This should be `constructors.ET.new`. + expect(etNamed.fullyQualifiedName, 'constructors.ET.ET'); expect(etNamed.isPublic, isTrue); expect(etNamed.documentationAsHtml, '

Constructor.

'); } diff --git a/test/end2end/model_test.dart b/test/end2end/model_test.dart index 9fbe1c7271..5af9cef3e6 100644 --- a/test/end2end/model_test.dart +++ b/test/end2end/model_test.dart @@ -1923,10 +1923,6 @@ void main() async { CatString = exLibrary.classes.named('CatString'); }); - test('has a fully qualified name', () { - expect(Apple.fullyQualifiedName, 'ex.Apple'); - }); - test('does have a line number and column', () { expect(Apple.characterLocation, isNotNull); }); @@ -2984,10 +2980,6 @@ void main() async { 'DocumentThisExtensionOnce')); }); - test('has a fully qualified name', () { - expect(ext.fullyQualifiedName, 'ex.AppleExtension'); - }); - test('does have a line number and column', () { expect(ext.characterLocation, isNotNull); }); @@ -3069,10 +3061,6 @@ void main() async { doAComplicatedThing = fakeLibrary.functions.named('doAComplicatedThing'); }); - test('has a fully qualified name', () { - expect(thisIsAsync.fullyQualifiedName, 'fake.thisIsAsync'); - }); - test('does have a line number and column', () { expect(thisIsAsync.characterLocation, isNotNull); }); @@ -3370,10 +3358,6 @@ String? topLevelFunction(int param1, bool param2, Cool coolBeans, 'bool Function<T4>(String, T1, T4)')); }); - test('has a fully qualified name', () { - expect(m1.fullyQualifiedName, 'ex.B.m1'); - }); - test('has abstract kind', () { expect(abstractMethod.fullkind, 'abstract method'); }); @@ -3725,10 +3709,6 @@ String? topLevelFunction(int param1, bool param2, Cool coolBeans, equals('Getter doc for explicitGetterImplicitSetter')); }); - test('has a fully qualified name', () { - expect(lengthX.fullyQualifiedName, 'fake.WithGetterAndSetter.lengthX'); - }); - test('has extended documentation', () { expect(lengthX.oneLineDoc, equals('Returns a length.')); expect(lengthX.documentation, contains('the fourth dimension')); @@ -4014,10 +3994,6 @@ String? topLevelFunction(int param1, bool param2, Cool coolBeans, equals('Setter docs should be shown.')); }); - test('has a fully qualified name', () { - expect(justGetter.fullyQualifiedName, 'fake.justGetter'); - }); - test('type arguments are correct', () { var modelType = mapWithDynamicKeys.modelType as ParameterizedElementType; expect(modelType.typeArguments, hasLength(2)); @@ -4097,10 +4073,6 @@ String? topLevelFunction(int param1, bool param2, Cool coolBeans, expect(customClassPrivate.constantValue, 'const _APrivateConstClass()'); }); - test('has a fully qualified name', () { - expect(greenConstant.fullyQualifiedName, 'ex.COLOR_GREEN'); - }); - test('has the correct kind', () { expect(greenConstant.kind, equals(Kind.topLevelConstant)); }); @@ -4192,11 +4164,6 @@ String? topLevelFunction(int param1, bool param2, Cool coolBeans, 'ConstructorTester<A, B>.fromSomething'); }); - test('has a fully qualified name', () { - expect( - appleConstructorFromString.fullyQualifiedName, 'ex.Apple.fromString'); - }); - test('has a line number and column', () { expect(appleDefaultConstructor.characterLocation, isNotNull); expect(syntheticConstructor.characterLocation, isNotNull); @@ -4388,11 +4355,6 @@ String? topLevelFunction(int param1, bool param2, Cool coolBeans, ); }); - test('has a fully qualified name', () { - expect(processMessage.fullyQualifiedName, 'ex.processMessage'); - expect(generic.fullyQualifiedName, 'fake.NewGenericTypedef'); - }); - test('has enclosing element', () { expect(processMessage.enclosingElement.name, equals(exLibrary.name)); expect(generic.enclosingElement.name, equals(fakeLibrary.name)); diff --git a/test/extension_types_test.dart b/test/extension_types_test.dart index 769726e2a9..4fa1acc268 100644 --- a/test/extension_types_test.dart +++ b/test/extension_types_test.dart @@ -40,8 +40,10 @@ extension type ET(int it) implements num { class C {} '''); + var et = library.extensionTypes.named('ET'); + expect(et.fullyQualifiedName, 'extension_types.ET'); expect( - library.extensionTypes.named('ET').documentationAsHtml, + et.documentationAsHtml, '

Doc referring to ' 'C.

', ); @@ -57,12 +59,10 @@ extension type ET(int it) { class C {} '''); + var et = library.extensionTypes.named('ET'); + expect(et.fullyQualifiedName, 'extension_types.ET'); expect( - library.extensionTypes - .named('ET') - .instanceMethods - .named('m') - .documentationAsHtml, + et.instanceMethods.named('m').documentationAsHtml, '

Doc referring to ' 'C.

', ); diff --git a/test/extensions_test.dart b/test/extensions_test.dart index 391b984895..0540e6a9a0 100644 --- a/test/extensions_test.dart +++ b/test/extensions_test.dart @@ -31,8 +31,10 @@ extension Ex on int {} var f() {} '''); + var f = library.functions.named('f'); + expect(f.fullyQualifiedName, 'extension_methods.f'); expect( - library.functions.named('f').documentationAsHtml, + f.documentationAsHtml, contains('Ex'), ); } @@ -45,10 +47,12 @@ extension E on int { } '''); + var f = library.extensions.first.instanceFields.first; + expect(f.fullyQualifiedName, 'extension_methods.E.f'); // We are primarily testing that dartdoc does not crash when trying to // resolve an unknown reference, from the position of an extension member. expect( - library.extensions.first.instanceFields.first.documentationAsHtml, + f.documentationAsHtml, contains('

Text NotFound text.

'), ); } @@ -63,8 +67,10 @@ extension Ex on int { var f() {} '''); + var f = library.functions.named('f'); + expect(f.fullyQualifiedName, 'extension_methods.f'); expect( - library.functions.named('f').documentationAsHtml, + f.documentationAsHtml, contains('Ex.m'), ); } @@ -79,8 +85,10 @@ extension Ex on int { var f() {} '''); + var f = library.functions.named('f'); + expect(f.fullyQualifiedName, 'extension_methods.f'); expect( - library.functions.named('f').documentationAsHtml, + f.documentationAsHtml, contains('Ex.b'), ); } @@ -95,8 +103,10 @@ extension Ex on int { var f() {} '''); + var f = library.functions.named('f'); + expect(f.fullyQualifiedName, 'extension_methods.f'); expect( - library.functions.named('f').documentationAsHtml, + f.documentationAsHtml, contains('Ex.b'), ); } diff --git a/test/type_parameter_test.dart b/test/type_parameter_test.dart index d5d02ee320..9a9779dc2d 100644 --- a/test/type_parameter_test.dart +++ b/test/type_parameter_test.dart @@ -19,6 +19,15 @@ class TypeParameterTest extends DartdocTestBase { @override String get libraryName => 'type_parameters'; + void test_name() async { + var library = await bootPackageWithLibrary(''' +void f(int p) {} +'''); + var typeParameter = library.functions.named('f').typeParameters.first; + expect(typeParameter.name, equals('T')); + expect(typeParameter.fullyQualifiedName, equals('type_parameters.f.T')); + } + void test_referenced() async { var library = await bootPackageWithLibrary(''' /// Text [T]. diff --git a/test/typedef_test.dart b/test/typedef_test.dart index 2e26170a50..0c0a7b35ad 100644 --- a/test/typedef_test.dart +++ b/test/typedef_test.dart @@ -36,6 +36,7 @@ typedef T = C; '''); final tTypedef = library.typedefs.named('T'); expect(tTypedef.nameWithGenerics, 'T'); + expect(tTypedef.fullyQualifiedName, equals('typedefs.T')); expect(tTypedef.genericParameters, ''); expect(tTypedef.aliasedType, isA()); } @@ -96,8 +97,8 @@ Line _two_.'''); var library = await bootPackageWithLibrary(''' typedef Cb2 = T Function(T); '''); - final cb2Typedef = library.typedefs.named('Cb2'); - + var cb2Typedef = library.typedefs.named('Cb2'); + expect(cb2Typedef.fullyQualifiedName, equals('typedefs.Cb2')); expect( cb2Typedef.nameWithGenerics, 'Cb2<T>',