Skip to content

Improve couldApplyTo and add tests #2062

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 10 commits into from
Nov 8, 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
29 changes: 27 additions & 2 deletions lib/src/element_type.dart
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,11 @@ class TypeParameterElementType extends DefinedElementType {
}
return _nameWithGenerics;
}

@override
ClassElement get _boundClassElement => interfaceType.element;
@override
InterfaceType get interfaceType => (type as TypeParameterType).bound;
}

/// An [ElementType] associated with an [Element].
Expand Down Expand Up @@ -311,6 +316,26 @@ abstract class DefinedElementType extends ElementType {
}
return _typeArguments;
}

/// By default, the bound is the type of the declared class.
ClassElement get _boundClassElement => (element.element as ClassElement);
Class get boundClass =>
ModelElement.fromElement(_boundClassElement, packageGraph);
InterfaceType get interfaceType => type;

InterfaceType _instantiatedType;

/// Return this type, instantiated to bounds if it isn't already.
DartType get instantiatedType {
if (_instantiatedType == null) {
if (!interfaceType.typeArguments.every((t) => t is InterfaceType)) {
_instantiatedType = packageGraph.typeSystem.instantiateToBounds(interfaceType);
} else {
_instantiatedType = interfaceType;
}
}
return _instantiatedType;
}
}

/// Any callable ElementType will mix-in this class, whether anonymous or not.
Expand Down Expand Up @@ -446,8 +471,8 @@ class CallableGenericTypeAliasElementType extends ParameterizedElementType
@override
ElementType get returnType {
if (_returnType == null) {
_returnType = ElementType.from(
type.returnType, library, packageGraph, this);
_returnType =
ElementType.from(type.returnType, library, packageGraph, this);
}
return _returnType;
}
Expand Down
24 changes: 14 additions & 10 deletions lib/src/markdown_processor.dart
Original file line number Diff line number Diff line change
Expand Up @@ -206,11 +206,13 @@ MatchingLinkResult _getMatchingLinkElement(
if (refModelElement == null && element is ModelElement) {
Container preferredClass = _getPreferredClass(element);
if (preferredClass is Extension) {
element.warn(PackageWarning.notImplemented, message: 'Comment reference resolution inside extension methods is not yet implemented');
element.warn(PackageWarning.notImplemented,
message:
'Comment reference resolution inside extension methods is not yet implemented');
} else {
refModelElement =
_MarkdownCommentReference(codeRef, element, commentRefs, preferredClass)
.computeReferredElement();
refModelElement = _MarkdownCommentReference(
codeRef, element, commentRefs, preferredClass)
.computeReferredElement();
}
}

Expand Down Expand Up @@ -649,8 +651,8 @@ class _MarkdownCommentReference {
Inheritable overriddenElement =
(element as Inheritable).overriddenElement;
while (overriddenElement != null) {
tryClasses.add(
(element as Inheritable).overriddenElement.enclosingElement);
tryClasses
.add((element as Inheritable).overriddenElement.enclosingElement);
overriddenElement = overriddenElement.overriddenElement;
}
}
Expand All @@ -669,7 +671,7 @@ class _MarkdownCommentReference {

if (results.isEmpty && realClass != null) {
for (Class superClass
in realClass.publicSuperChain.map((et) => et.element as Class)) {
in realClass.publicSuperChain.map((et) => et.element)) {
if (!tryClasses.contains(superClass)) {
_getResultsForClass(superClass);
}
Expand Down Expand Up @@ -719,12 +721,12 @@ class _MarkdownCommentReference {
// TODO(jcollins-g): get rid of reimplementation of identifier resolution
// or integrate into ModelElement in a simpler way.
List<Class> superChain = [tryClass];
superChain.addAll(tryClass.interfaces.map((t) => t.element as Class));
superChain.addAll(tryClass.interfaces.map((t) => t.element));
// This seems duplicitous with our caller, but the preferredClass
// hint matters with findCanonicalModelElementFor.
// TODO(jcollins-g): This makes our caller ~O(n^2) vs length of superChain.
// Fortunately superChains are short, but optimize this if it matters.
superChain.addAll(tryClass.superChain.map((t) => t.element as Class));
superChain.addAll(tryClass.superChain.map((t) => t.element));
for (final c in superChain) {
_getResultsForSuperChainElement(c, tryClass);
if (results.isNotEmpty) break;
Expand Down Expand Up @@ -778,7 +780,9 @@ String _linkDocReference(String codeRef, Warnable warnable,
// current element.
warnable.warn(PackageWarning.unresolvedDocReference,
message: codeRef,
referredFrom: warnable.documentationIsLocal ? null : warnable.documentationFrom);
referredFrom: warnable.documentationIsLocal
? null
: warnable.documentationFrom);
}
return '<code>${htmlEscape.convert(codeRef)}</code>';
}
Expand Down
74 changes: 44 additions & 30 deletions lib/src/model.dart
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
library dartdoc.models;

import 'dart:async';
import 'dart:collection' show UnmodifiableListView;
import 'dart:collection' show HashSet, UnmodifiableListView;
import 'dart:convert';
import 'dart:io';

Expand All @@ -22,7 +22,6 @@ import 'package:analyzer/dart/ast/ast.dart'
InstanceCreationExpression;
import 'package:analyzer/dart/element/element.dart';
import 'package:analyzer/dart/element/type.dart';
import 'package:analyzer/dart/element/type_system.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';
Expand All @@ -36,7 +35,6 @@ import 'package:analyzer/src/dart/element/element.dart';
import 'package:analyzer/src/dart/element/inheritance_manager3.dart';
import 'package:analyzer/src/dart/element/member.dart'
show ExecutableMember, Member, ParameterMember;
import 'package:analyzer/src/dart/element/type.dart' show InterfaceTypeImpl;
import 'package:analyzer/src/dart/sdk/sdk.dart';
import 'package:analyzer/src/generated/engine.dart';
import 'package:analyzer/src/generated/java_io.dart';
Expand Down Expand Up @@ -805,7 +803,8 @@ class Class extends Container
_potentiallyApplicableExtensions = utils
.filterNonDocumented(packageGraph.extensions)
.where((e) => e.couldApplyTo(this))
.toList(growable: false);
.toList(growable: false)
..sort(byName);
}
return _potentiallyApplicableExtensions;
}
Expand Down Expand Up @@ -1344,26 +1343,44 @@ class Extension extends Container
ElementType.from(_extension.extendedType, library, packageGraph);
}

/// Returns [true] if there is an instantiation of [c] to which this extension
/// could be applied.
bool couldApplyTo(Class c) =>
_couldApplyTo(extendedType.type, c.element, packageGraph.typeSystem);

static bool _couldApplyTo(
DartType extendedType, ClassElement element, Dart2TypeSystem typeSystem) {
InterfaceTypeImpl classInstantiated =
typeSystem.instantiateToBounds(element.thisType);
classInstantiated = element.instantiate(
typeArguments: classInstantiated.typeArguments.map((a) {
if (a.isDynamic) {
return typeSystem.typeProvider.neverType;
}
return a;
}).toList(),
nullabilitySuffix: classInstantiated.nullabilitySuffix);

return (classInstantiated.element == extendedType.element) ||
typeSystem.isSubtypeOf(classInstantiated, extendedType);
bool couldApplyTo(Class c) => _couldApplyTo(c.modelType);

/// Return true if this extension could apply to [t].
bool _couldApplyTo(DefinedElementType t) {
return t.instantiatedType == extendedType.instantiatedType ||
(t.instantiatedType.element == extendedType.instantiatedType.element &&
isSubtypeOf(t)) ||
isBoundSupertypeTo(t);
}

/// The instantiated to bounds [extendedType] of this extension is a subtype of
/// [t].
bool isSubtypeOf(DefinedElementType t) => packageGraph.typeSystem
.isSubtypeOf(extendedType.instantiatedType, t.instantiatedType);

bool isBoundSupertypeTo(DefinedElementType t) =>
_isBoundSupertypeTo(t.type, HashSet());

/// Returns true if at least one supertype (including via mixins and
/// interfaces) is equivalent to or a subtype of [extendedType] when
/// instantiated to bounds.
bool _isBoundSupertypeTo(
InterfaceType superType, HashSet<InterfaceType> visited) {
ClassElement superClass = superType?.element;
if (visited.contains(superType)) return false;
visited.add(superType);
if (superClass == extendedType.type.element &&
(superType == extendedType.instantiatedType ||
packageGraph.typeSystem
.isSubtypeOf(superType, extendedType.instantiatedType))) {
return true;
}
List<InterfaceType> supertypes = [];
ClassElementImpl.collectAllSupertypes(supertypes, superType, null);
for (InterfaceType toVisit in supertypes) {
if (_isBoundSupertypeTo(toVisit, visited)) return true;
}
return false;
}

@override
Expand Down Expand Up @@ -1418,7 +1435,7 @@ class Extension extends Container
}

@override
DefinedElementType get modelType => super.modelType;
ParameterizedElementType get modelType => super.modelType;

List<ModelElement> _allModelElements;

Expand Down Expand Up @@ -5062,13 +5079,11 @@ class PackageGraph {
package._libraries.sort((a, b) => compareNatural(a.name, b.name));
package._libraries.forEach((library) {
library.allClasses.forEach(_addToImplementors);
// TODO(jcollins-g): Use a better data structure.
_extensions.addAll(library.extensions);
});
});
_implementors.values.forEach((l) => l.sort());
allImplementorsAdded = true;
_extensions.sort(byName);
allExtensionsAdded = true;

// We should have found all special classes by now.
Expand Down Expand Up @@ -5179,8 +5194,7 @@ class PackageGraph {
final Map<String, List<Class>> _implementors = Map();

/// A list of extensions that exist in the package graph.
// TODO(jcollins-g): Consider implementing a smarter structure for this.
final List<Extension> _extensions = List();
final List<Extension> _extensions = [];

/// PackageMeta for the default package.
final PackageMeta packageMeta;
Expand Down Expand Up @@ -5212,7 +5226,7 @@ class PackageGraph {
/// TODO(brianwilkerson) Replace the driver with the session.
final AnalysisDriver driver;
final AnalysisSession session;
final TypeSystem typeSystem;
final Dart2TypeSystem typeSystem;
final DartSdk sdk;

Map<Source, SdkLibrary> _sdkLibrarySources;
Expand Down
37 changes: 37 additions & 0 deletions test/model_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -2199,6 +2199,43 @@ void main() {
orderedEquals([uphill]));
});

test('applicableExtensions include those from implements & mixins', () {
Extension extensionCheckLeft,
extensionCheckRight,
extensionCheckCenter,
extensionCheckImplementor2,
onNewSchool,
onOldSchool;
Class implementor, implementor2, school;
Extension getExtension(String name) =>
fakeLibrary.extensions.firstWhere((e) => e.name == name);
Class getClass(String name) =>
fakeLibrary.classes.firstWhere((e) => e.name == name);
extensionCheckLeft = getExtension('ExtensionCheckLeft');
extensionCheckRight = getExtension('ExtensionCheckRight');
extensionCheckCenter = getExtension('ExtensionCheckCenter');
extensionCheckImplementor2 = getExtension('ExtensionCheckImplementor2');
onNewSchool = getExtension('OnNewSchool');
onOldSchool = getExtension('OnOldSchool');

implementor = getClass('Implementor');
implementor2 = getClass('Implementor2');
school = getClass('School');

expect(
implementor.potentiallyApplicableExtensions,
orderedEquals([
extensionCheckCenter,
extensionCheckImplementor2,
extensionCheckLeft,
extensionCheckRight
]));
expect(implementor2.potentiallyApplicableExtensions,
orderedEquals([extensionCheckImplementor2, extensionCheckLeft]));
expect(school.potentiallyApplicableExtensions,
orderedEquals([onNewSchool, onOldSchool]));
});

test('type parameters and bounds work with applicableExtensions', () {
expect(
superMegaTron.potentiallyApplicableExtensions, orderedEquals([leg]));
Expand Down
43 changes: 43 additions & 0 deletions testing/test_package/lib/fake.dart
Original file line number Diff line number Diff line change
Expand Up @@ -1018,6 +1018,49 @@ class AnotherExtended<T extends BaseTest> extends BaseTest {}

class BigAnotherExtended extends AnotherExtended<SubclassBaseTest> {}

extension ExtensionCheckLeft on Implemented1 {
int get left => 3;
}

extension ExtensionCheckRight on Implemented2 {
int get right => 4;
}

extension ExtensionCheckCenter on BaseExtended {
bool get center => true;
}

class BaseExtended {}

class Implemented1 {}

class Implemented2 {}

class Implementor extends BaseExtended
implements Implemented1, Implemented2, Implementor2 {}

extension ExtensionCheckImplementor2 on Implementor2 {
int get test => 1;
}

class Implementor2 implements Implemented1 {}

class OldSchoolMixin {
aThing() {}
}

mixin NewSchoolMixin {}

extension OnNewSchool on NewSchoolMixin {
String get bar => 'hello';
}

extension OnOldSchool on OldSchoolMixin {
int get foo => 4;
}

class School with OldSchoolMixin, NewSchoolMixin {}

//
//
//
Expand Down