Skip to content

Commit 3b59e42

Browse files
committed
Fix many library sidebars, and search
The library sidebars on container (e.g. Class), top-level variable, top-level function, and typedefs were totally busted, as a result of d629e1e. Fixes #3859 and fixes #3861. The fix is to allow a Library's `canonicalModelElement` to be it's canonical library, instead of `null`. This involves adding a few checks for "am I calculating something for a Library?" and moving some "is this library considered public" logic out of `Element.hasPrivateName` and into `ModelElement.isPublic`. That corrects the issue of an exported Class's (for example) enclosing element's URI, for the search results. Once all of that is fixed, we also correct the `aboveSidebarPath` implementation for a few elements, to point to the _canonical_ library's sidebar path. This fix highlights an issue with wildcard-variables, so one test is newly marked as failing.
1 parent 46564a6 commit 3b59e42

10 files changed

+84
-32
lines changed

lib/src/model/container.dart

+1-1
Original file line numberDiff line numberDiff line change
@@ -238,7 +238,7 @@ abstract class Container extends ModelElement
238238
String get sidebarPath;
239239

240240
@override
241-
String get aboveSidebarPath => library.sidebarPath;
241+
String get aboveSidebarPath => canonicalLibraryOrThrow.sidebarPath;
242242

243243
@override
244244
String get belowSidebarPath => sidebarPath;

lib/src/model/model_element.dart

+17
Original file line numberDiff line numberDiff line change
@@ -394,6 +394,22 @@ abstract class ModelElement
394394
!(enclosingElement as Extension).isPublic) {
395395
return false;
396396
}
397+
398+
if (element case LibraryElement(:var identifier, :var source)) {
399+
// Private Dart SDK libraries are not public.
400+
if (identifier.startsWith('dart:_') ||
401+
identifier.startsWith('dart:nativewrappers/') ||
402+
'dart:nativewrappers' == identifier) {
403+
return false;
404+
}
405+
// Package-private libraries are not public.
406+
var elementUri = source.uri;
407+
if (elementUri.scheme == 'package' &&
408+
elementUri.pathSegments[1] == 'src') {
409+
return false;
410+
}
411+
}
412+
397413
return !element.hasPrivateName && !hasNodoc;
398414
}();
399415

@@ -518,6 +534,7 @@ abstract class ModelElement
518534
final candidateLibraries = thisAndExported.where((l) {
519535
if (!l.isPublic) return false;
520536
if (l.package.documentedWhere == DocumentLocation.missing) return false;
537+
if (this is Library) return true;
521538
var lookup = l.element.exportNamespace.definedNames[topLevelElementName];
522539
return topLevelElement ==
523540
(lookup is PropertyAccessorElement ? lookup.variable2 : lookup);

lib/src/model/model_function.dart

+1-1
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,7 @@ class ModelFunctionTyped extends ModelElement with TypeParameters {
5151
String get filePath => '${canonicalLibrary?.dirName}/$fileName';
5252

5353
@override
54-
String get aboveSidebarPath => enclosingElement.sidebarPath;
54+
String get aboveSidebarPath => canonicalLibraryOrThrow.sidebarPath;
5555

5656
@override
5757
String? get belowSidebarPath => null;

lib/src/model/package_graph.dart

+3
Original file line numberDiff line numberDiff line change
@@ -195,6 +195,7 @@ class PackageGraph with CommentReferable, Nameable {
195195
// that might not _have_ a canonical [ModelElement], too.
196196
if (element.isCanonical ||
197197
element.canonicalModelElement == null ||
198+
element is Library ||
198199
element.enclosingElement!.isCanonical) {
199200
for (var d in element.documentationFrom
200201
.where((d) => d.hasDocumentationComment)) {
@@ -771,6 +772,8 @@ class PackageGraph with CommentReferable, Nameable {
771772
if (canonicalClass != null) preferredClass = canonicalClass;
772773
}
773774
var lib = modelElement.canonicalLibrary;
775+
if (modelElement is Library) return lib;
776+
774777
if (lib == null && preferredClass != null) {
775778
lib = preferredClass.canonicalLibrary;
776779
}

lib/src/model/top_level_variable.dart

+1-1
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ class TopLevelVariable extends ModelElement
4545
String get filePath => '${canonicalLibraryOrThrow.dirName}/$fileName';
4646

4747
@override
48-
String get aboveSidebarPath => enclosingElement.sidebarPath;
48+
String get aboveSidebarPath => canonicalLibraryOrThrow.sidebarPath;
4949

5050
@override
5151
String? get belowSidebarPath => null;

lib/src/model/typedef.dart

+1-1
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ abstract class Typedef extends ModelElement
3535
String get filePath => '${canonicalLibraryOrThrow.dirName}/$fileName';
3636

3737
@override
38-
String get aboveSidebarPath => enclosingElement.sidebarPath;
38+
String get aboveSidebarPath => canonicalLibraryOrThrow.sidebarPath;
3939

4040
@override
4141
String? get belowSidebarPath => null;

lib/src/model_utils.dart

-13
Original file line numberDiff line numberDiff line change
@@ -72,19 +72,6 @@ extension ElementExtension on Element {
7272
return true;
7373
}
7474
}
75-
if (self is LibraryElement) {
76-
if (self.identifier.startsWith('dart:_') ||
77-
self.identifier.startsWith('dart:nativewrappers/') ||
78-
'dart:nativewrappers' == self.identifier) {
79-
return true;
80-
}
81-
var elementUri = self.source.uri;
82-
// TODO(jcollins-g): Implement real cross package detection.
83-
if (elementUri.scheme == 'package' &&
84-
elementUri.pathSegments[1] == 'src') {
85-
return true;
86-
}
87-
}
8875
return false;
8976
}
9077
}

test/dartdoc_test_base.dart

+12-8
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,9 @@ abstract class DartdocTestBase {
3636

3737
String get linkPrefix => '$placeholder$libraryName';
3838

39+
String get dartAsyncUrlPrefix =>
40+
'https://api.dart.dev/stable/3.2.0/dart-async';
41+
3942
String get dartCoreUrlPrefix => 'https://api.dart.dev/stable/3.2.0/dart-core';
4043

4144
String get sdkConstraint => '>=3.3.0 <4.0.0';
@@ -99,20 +102,21 @@ analyzer:
99102
/// Creates a single library named [libraryName], with optional preamble
100103
/// [libraryPreamble]. Optionally, pass [extraFiles] such as
101104
/// `dartdoc_options.yaml`.
102-
Future<Library> bootPackageWithLibrary(String libraryContent,
103-
{String libraryPreamble = '',
104-
Iterable<d.Descriptor> extraFiles = const [],
105-
List<String> additionalArguments = const []}) async {
105+
Future<Library> bootPackageWithLibrary(
106+
String libraryContent, {
107+
String libraryFilePath = 'lib/library.dart',
108+
String libraryPreamble = '',
109+
Iterable<d.Descriptor> extraFiles = const [],
110+
List<String> additionalArguments = const [],
111+
}) async {
106112
return (await bootPackageFromFiles([
107-
d.dir('lib', [
108-
d.file('lib.dart', '''
113+
d.file(libraryFilePath, '''
109114
$libraryPreamble
110115
library $libraryName;
111116
112117
$libraryContent
113118
'''),
114-
]),
115-
...extraFiles
119+
...extraFiles,
116120
], additionalArguments: additionalArguments))
117121
.libraries
118122
.named(libraryName);

test/libraries_test.dart

+40-2
Original file line numberDiff line numberDiff line change
@@ -5,13 +5,19 @@
55
import 'package:analyzer/file_system/memory_file_system.dart';
66
import 'package:dartdoc/src/model/model.dart';
77
import 'package:test/test.dart';
8+
import 'package:test_reflective_loader/test_reflective_loader.dart';
89

10+
import 'dartdoc_test_base.dart';
911
import 'src/test_descriptor_utils.dart' as d;
1012
import 'src/utils.dart';
1113

12-
// TODO(srawlins): Migrate to test_reflective_loader tests.
13-
1414
void main() {
15+
defineReflectiveSuite(() {
16+
defineReflectiveTests(LibrariesTest);
17+
});
18+
19+
// TODO(srawlins): Migrate to test_reflective_loader tests.
20+
1521
test('A named library', () async {
1622
var packageMetaProvider = testPackageMetaProvider;
1723

@@ -169,3 +175,35 @@ A doc comment.
169175
'${htmlBasePlaceholder}dart-async/dart-async-library.html');
170176
}, onPlatform: {'windows': Skip('Test does not work on Windows (#2446)')});
171177
}
178+
179+
@reflectiveTest
180+
class LibrariesTest extends DartdocTestBase {
181+
@override
182+
String get libraryName => 'libraries';
183+
184+
void test_publicLibrary() async {
185+
var library = await bootPackageWithLibrary(
186+
'var x = 1;',
187+
libraryFilePath: 'lib/library.dart',
188+
);
189+
190+
expect(library.qualifiedName, 'libraries');
191+
expect(library.href, '${placeholder}libraries/libraries-library.html');
192+
}
193+
194+
void test_exportedLibrary() async {
195+
var library = await bootPackageWithLibrary(
196+
'var x = 1;',
197+
libraryFilePath: 'lib/src/library.dart',
198+
extraFiles: [
199+
d.dir('lib', [
200+
d.file('public.dart', '''
201+
export 'src/library.dart';
202+
''')
203+
]),
204+
],
205+
);
206+
expect(library.qualifiedName, 'libraries');
207+
expect(library.href, '${placeholder}public/public-library.html');
208+
}
209+
}

test/prefixes_test.dart

+8-5
Original file line numberDiff line numberDiff line change
@@ -29,11 +29,14 @@ int x = 0;
2929
''',
3030
additionalArguments: ['--link-to-remote'],
3131
);
32-
var f = library.properties.named('x');
33-
// There is no link, but also no wrong link or crash.
34-
expect(f.documentationAsHtml, '<p>Text <code>async</code>.</p>');
32+
var x = library.properties.named('x');
33+
expect(
34+
x.documentationAsHtml,
35+
'<p>Text <a href="$dartAsyncUrlPrefix/dart-async-library.html">async</a>.</p>',
36+
);
3537
}
3638

39+
@FailingTest(issue: 'https://github.com/dart-lang/dartdoc/issues/3769')
3740
void test_referenced_wildcard() async {
3841
var library = await bootPackageWithLibrary(
3942
'''
@@ -44,8 +47,8 @@ int x = 0;
4447
''',
4548
additionalArguments: ['--link-to-remote'],
4649
);
47-
var f = library.properties.named('x');
50+
var x = library.properties.named('x');
4851
// There is no link, but also no wrong link or crash.
49-
expect(f.documentationAsHtml, '<p>Text <code>_</code>.</p>');
52+
expect(x.documentationAsHtml, '<p>Text <code>_</code>.</p>');
5053
}
5154
}

0 commit comments

Comments
 (0)