Skip to content

Handle reexports of extensions in dartdoc #2045

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Oct 23, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/src/html/html_generator_instance.dart
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ class HtmlGeneratorInstance {
}
}

for (var extension in filterNonPublic(lib.extensions)) {
for (var extension in filterNonDocumented(lib.extensions)) {
generateExtension(_packageGraph, lib, extension);

for (var constant in filterNonDocumented(extension.constants)) {
Expand Down
56 changes: 32 additions & 24 deletions lib/src/model.dart
Original file line number Diff line number Diff line change
Expand Up @@ -2415,8 +2415,6 @@ class Library extends ModelElement with Categorization, TopLevelContainer {
return _allOriginalModelElementNames;
}

List<Class> get allClasses => _allClasses;

@override
CharacterLocation get characterLocation {
if (element.nameOffset == -1) {
Expand All @@ -2430,19 +2428,30 @@ class Library extends ModelElement with Categorization, TopLevelContainer {
CompilationUnitElement get compilationUnitElement => (element as LibraryElement).definingCompilationUnit;

@override
Iterable<Class> get classes {
return _allClasses
.where((c) => !c.isErrorOrException)
.toList(growable: false);
}
Iterable<Class> get classes => allClasses.where((c) => !c.isErrorOrException);

@override
Iterable<Extension> get extensions {
if (_extensions != null) return _extensions;
_extensions = _libraryElement.definingCompilationUnit.extensions
.map((e) => ModelElement.from(e, this, packageGraph) as Extension)
.toList(growable: false)
..sort(byName);
if (_extensions == null) {
// De-dupe extensions coming from multiple exported libraries at once.
Set<ExtensionElement> extensionElements = Set();
extensionElements.addAll(_libraryElement.definingCompilationUnit.extensions);
for (CompilationUnitElement cu in _libraryElement.parts) {
extensionElements.addAll(cu.extensions);
}
for (LibraryElement le in _libraryElement.exportedLibraries) {
extensionElements.addAll(le.definingCompilationUnit.extensions
.where((t) => _exportedNamespace.definedNames.values.contains(t.name)));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm worried this might not do the right thing. Consider files foo.dart and bar.dart, each of which defines a distinct extension called E. Then if we have a library like this:

export 'foo.dart' show E;
export 'bar.dart' hide E;

Then I think both Es will get included in extensionElements, because both of them have a name that matches a name that exists in _exportedNamespace.

The best correct approach I can currently think of is:

  • Expose an analyzer API to allow clients to figure out which elements are actually exported by an ExportElement (e.g. we could expose dartdoc to filter a set of elements via namespace combinators (we could move NamespaceBuilder._applyCombinators to ExportElement and re-name it appropriately)
  • Change this code to iterate through the export elements rather than the exported libraries, so that it can use the newly exposed API to figure out exactly which extensions are shown.

It's ok with me if you want to file an issue and address this in a later PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I'm seeing this now that I'm looking more closely at the library scope code in the analyzer. It looks like this has always been wrong in the class implementation. The "LibraryExportScope" I mentioned in chat the other day is probably the right solution for this, too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix for this is taking a while, I think I will go ahead and deal with this in a followup PR.

}

extensionElements.addAll(_exportedNamespace.definedNames.values
.whereType<ExtensionElement>());

_extensions = extensionElements
.map((e) => ModelElement.from(e, this, packageGraph) as Extension)
.toList(growable: false)
..sort(byName);
}
return _extensions;
}

Expand Down Expand Up @@ -2628,10 +2637,10 @@ class Library extends ModelElement with Categorization, TopLevelContainer {

@override
List<Class> get exceptions {
return _allClasses
.where((c) => c.isErrorOrException)
.toList(growable: false)
..sort(byName);
if (_exceptions == null) {
_exceptions = allClasses.where((c) => c.isErrorOrException).toList(growable: false);
}
return _exceptions;
}

@override
Expand Down Expand Up @@ -2752,24 +2761,23 @@ class Library extends ModelElement with Categorization, TopLevelContainer {
return _typedefs;
}

List<Class> get _allClasses {
List<Class> get allClasses {
if (_classes != null) return _classes;

// De-dupe classes coming from multiple exported libraries at once.
Set<ClassElement> types = Set();
types.addAll(_libraryElement.definingCompilationUnit.types);
for (CompilationUnitElement cu in _libraryElement.parts) {
types.addAll(cu.types);
}
for (LibraryElement le in _libraryElement.exportedLibraries) {
types.addAll(le.definingCompilationUnit.types
.where((t) => _exportedNamespace.definedNames.values.contains(t.name))
.toList());
.where((t) => _exportedNamespace.definedNames.values.contains(t.name)));
}

types.addAll(_exportedNamespace.definedNames.values
.where((e) => e is ClassElement && !e.isMixin)
.cast<ClassElement>()
.where((element) => !element.isEnum));
.whereType<ClassElement>()
.where((e) => !e.isMixin && !e.isEnum));

_classes = types
.map((e) => ModelElement.from(e, this, packageGraph) as Class)
Expand All @@ -2783,7 +2791,7 @@ class Library extends ModelElement with Categorization, TopLevelContainer {
LibraryElement get _libraryElement => (element as LibraryElement);

Class getClassByName(String name) {
return _allClasses.firstWhere((it) => it.name == name, orElse: () => null);
return allClasses.firstWhere((it) => it.name == name, orElse: () => null);
}

List<TopLevelVariable> _getVariables() {
Expand Down Expand Up @@ -5026,7 +5034,7 @@ class PackageGraph {
documentedPackages.toList().forEach((package) {
package._libraries.sort((a, b) => compareNatural(a.name, b.name));
package._libraries.forEach((library) {
library._allClasses.forEach(_addToImplementors);
library.allClasses.forEach(_addToImplementors);
});
});
_implementors.values.forEach((l) => l.sort());
Expand Down
16 changes: 16 additions & 0 deletions test/model_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -2114,11 +2114,20 @@ void main() {

group('Extension', () {
Extension ext, fancyList;
Extension documentOnceReexportOne, documentOnceReexportTwo;
Library reexportOneLib, reexportTwoLib;
Class extensionReferencer;
Method doSomeStuff, doStuff, s;
List<Extension> extensions;

setUpAll(() {
reexportOneLib = packageGraph.libraries
.firstWhere((lib) => lib.name == 'reexport_one');
reexportTwoLib = packageGraph.libraries
.firstWhere((lib) => lib.name == 'reexport_two');
documentOnceReexportOne = reexportOneLib.extensions.firstWhere((e) => e.name == 'DocumentThisExtensionOnce');
documentOnceReexportTwo = reexportTwoLib.extensions.firstWhere((e) => e.name == 'DocumentThisExtensionOnce');

ext = exLibrary.extensions.firstWhere((e) => e.name == 'AppleExtension');
extensionReferencer = exLibrary.classes.firstWhere((c) => c.name == 'ExtensionReferencer');
fancyList = exLibrary.extensions.firstWhere((e) => e.name == 'FancyList');
Expand All @@ -2129,6 +2138,12 @@ void main() {
extensions = exLibrary.publicExtensions.toList();
});

test('basic canonicalization for extensions', () {
expect(documentOnceReexportOne.isCanonical, isFalse);
expect(documentOnceReexportOne.href, equals(documentOnceReexportTwo.href));
expect(documentOnceReexportTwo.isCanonical, isTrue);
});

// TODO(jcollins-g): implement feature and update tests
test('documentation links do not crash in base cases', () {
packageGraph.packageWarningCounter.hasWarning(doStuff, PackageWarning.notImplemented,
Expand All @@ -2143,6 +2158,7 @@ void main() {
expect(extensionReferencer.documentationAsHtml, contains('<code>_Shhh</code>'));
expect(extensionReferencer.documentationAsHtml, contains('<a href="ex/FancyList.html">FancyList</a>'));
expect(extensionReferencer.documentationAsHtml, contains('<a href="ex/AnExtension/call.html">AnExtension.call</a>'));
expect(extensionReferencer.documentationAsHtml, contains('<a href="reexport_two/DocumentThisExtensionOnce.html">DocumentThisExtensionOnce</a>'));
});

test('has a fully qualified name', () {
Expand Down
1 change: 1 addition & 0 deletions testing/test_package/lib/example.dart
Original file line number Diff line number Diff line change
Expand Up @@ -677,4 +677,5 @@ extension on Object {

/// This class has nothing to do with [_Shhh], [FancyList], or [AnExtension.call],
/// but should not crash because we referenced them.
/// We should be able to find [DocumentThisExtensionOnce], too.
class ExtensionReferencer {}
1 change: 1 addition & 0 deletions testing/test_package/lib/reexport_two.dart
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
/// {@canonicalFor reexport.somelib.SomeClass}
/// {@canonicalFor reexport.somelib.AUnicornClass}
/// {@canonicalFor something.ThatDoesntExist}
/// {@canonicalFor reexport.somelib.DocumentThisExtensionOnce}
/// {@category Unreal}
library reexport_two;

Expand Down
16 changes: 16 additions & 0 deletions testing/test_package/lib/src/somelib.dart
Original file line number Diff line number Diff line change
Expand Up @@ -7,3 +7,19 @@ class SomeOtherClass {}
class YetAnotherClass {}

class AUnicornClass {}


/// A private extension.
extension _Unseen on Object {
void doYouSeeMe() { }
}

/// An extension without a name
extension on List {
void somethingNew() { }
}

/// [_Unseen] is not seen, but [DocumentMe] is.
extension DocumentThisExtensionOnce on String {
String get reportOnString => '$this is wonderful';
}