Skip to content

Do not depend on FunctionType being a subclass of ParameterizedType #2665

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.

Sign up for GitHub

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
Jun 3, 2021
Merged

Do not depend on FunctionType being a subclass of ParameterizedType #2665

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
4a0d9c6
First try
jcollins-g Jun 2, 2021
31fa623
retire the feature
jcollins-g Jun 2, 2021
6e045ef
refactor
jcollins-g Jun 2, 2021
e95dbd4
finish excising implicit future
jcollins-g Jun 2, 2021
97525d5
cleanup
jcollins-g Jun 2, 2021
4a2f667
Fix recursive parameter search (flutter test failure)
jcollins-g Jun 2, 2021
c9b40ee
this seems to work
jcollins-g Jun 3, 2021
04334b2
DefinedElementType back in visibleTypes
jcollins-g Jun 3, 2021
2a440a0
Comments and type cleanup
jcollins-g Jun 3, 2021
7b2d063
Merge branch 'master' into functiontype-remove-members
jcollins-g Jun 3, 2021
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
229 changes: 92 additions & 137 deletions lib/src/element_type.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,15 @@ abstract class ElementType extends Privacy with CommentReferable, Nameable {

factory ElementType.from(
DartType f, Library library, PackageGraph packageGraph,
[ElementType returnedFrom]) {
{ElementType returnedFrom}) {
if (f.element == null ||
f.element.kind == ElementKind.DYNAMIC ||
f.element.kind == ElementKind.NEVER) {
if (f is FunctionType) {
if (f.aliasElement != null) {
return AliasedFunctionTypeElementType(
Copy link
Contributor

Choose a reason for hiding this comment

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

Null safety suggestion: make aliasElement and aliasArguments required parameters of AliasedFunctionTypeElementType. It is nice to use the type system to prove that some data is available. Here, we have already proved that aliasElement is available. But then we drop this proof, and force AliasedFunctionTypeElementType to implicitly require that aliasElement and aliasArguments are not null.

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 TL;DR is no, not doing that, because dartdoc isn't written this way in general and to introduce a contradictory paradigm would be confusing unless it propagated throughout the entire codebase, which is a lot of work. Making the requirement more explicit within the constructor instead.

However, this probably deserves more explanation (at least I would want more if I were you) so here we go. This includes a lot of background, you may know some already, but for posterity...

First, the primary purpose of this if statement is not to prove that aliasElement is available, but rather to select the appropriate constructor if it is available. Dartdoc models "how to structure and render code in documentation" within the class structure and type system, vs the analyzer's "how to represent the code to answer questions around static analysis". An ancient choice in dartdoc on how to do this is as an overlay to the analyzer, essentially creating a custom view of the element model. This is done with one-to-one, many-to-one, or one-to-many dartdoc classes for a given analyzer class. This makes some sense, since how you view the code in documentation is closely related to its structure as written, which is closely related to how it is represented in the analyzer.

I am not sure if the original author of dartdoc was conscious of this design choice and all its implications, and I am pretty sure they didn't think of NNBD when making this choice! But it is kind of baked in to the entire piece of software and has been extended by myself and others since then with varying levels of consistency. What is happening in this PR is not that we're proving that aliasElement is present, we're using its presence to decide which object to construct (AliasedFunctionTypeElementType or FunctionTypeElementType), and therefore, eventually, how we will render the FunctionType. It just sort of accidentally proves that it is available along the way. Essentially, the code I wrote just wishes those getters didn't exist in analyzer types where we don't care about them, but because they do, we do this little dance.

This is consistent with dartdoc's design choice of basing every object off a corresponding analyzer object. Leading me to the second part, around making dartdoc internally consistent. We can argue about whether the design choice is a good idea -- if I was building this thing from scratch I might make different choices, but, given where we are... Were I to pass aliasElement and aliasArguments through to the constructor here, we'd be doing something that isn't typically done in the rest of dartdoc. We try not to pass things that can be acquired directly from the object we are representing, though sometimes for convenience we do pass the Dartdoc equivalent of those objects to constructors.

In addition to the design inconsistency, a second consequence is now you have the wrong way to access aliasElement (type.aliasElement) and the right way (analyzerAliasElement?, since we already have aliasElement that is a Dartdoc object) with no way to enforce which one you are using.

While maybe we are smart enough to always use the right way of accessing members of DartType I do not trust myself a few months from now to reread this code and use the right one all the time. In most cases getters in dartdoc like aliasElement are converted Dartdoc objects rather than the analyzer objects, and you can see me doing that in this PR. In addition, not all used getters of type are represented, should we also pass element? element.name? (Remember, even name is the name as Dartdoc sees it, which is occasionally different from the analyzer's view in Dartdoc objects. element is actually slated for demolition in ElementType and I don't want to keep it around -- again, trying to reduce inconsistency here rather than increase it).

We could also fix that "multiple paths" problem by eliminating the DartType entirely from ElementType constructors and always passing everything we use in or otherwise discarding the original object. This however is an even more jarring design choice given the rest of Dartdoc. At this point I'd rather just redesign/reimplement the whole program because that will be enormous and maybe not worth the effort. I certainly wouldn't want to introduce this without converting most of dartdoc to use this style. And that would involve a lot of boilerplate code plus make debugging more difficult (i.e. where did this name/element/aliasElement come from in the analyzer, which is a question I ask quite often in diagnosing problems).

So given these issues, I will resist passing more parameters that can be acquired via getters in the represented analyzer object unless there's a case compelling enough to overcome the long term maintenance problems that can create. Or, if there's a lot of time to restructure dartdoc around this. Maybe a better path will become more clear after Dartdoc is converted to NNBD, which if it isn't going to be done this quarter, will be a P1 next quarter.

Copy link
Contributor

Choose a reason for hiding this comment

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

Wow, this is a very thorough explanation, thank you for it.

I appreciate the intention to keep code consistent, and was merely sharing my view on writing more null safe code. But DartDoc is not null safe, so it might be premature to do anyway :-)

f, library, packageGraph, returnedFrom);
}
return FunctionTypeElementType(f, library, packageGraph, returnedFrom);
}
return UndefinedElementType(f, library, packageGraph, returnedFrom);
Expand All @@ -50,10 +54,13 @@ abstract class ElementType extends Privacy with CommentReferable, Nameable {
var isGenericTypeAlias = f.aliasElement != null && f is! InterfaceType;
if (f is FunctionType) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, this is probably what you mean about being backward compatible.
Do we need this though? Why not check for any FunctionType, with or without element and handle as FunctionTypeElementType or AliasedFunctionTypeElementType, and never as CallableElementType?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

CallableElementType is still needed before both dartdoc and analyzer publish -- after that I agree it is no longer reachable and we can jettison that case. Adding some comments to explain.

You're also seeing me remove some leftovers from a previous "be compatible with old and new analyzer" iteration, it's now down to an assert rather than branching into the old CallableGenericTypeAliasElementType -- now this is handled elsewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, perhaps you meant, why is CallableElementType needed ever? Because that case uses the DefinedElementType base class instead of UndefinedElementType, and that has implications for some of the other getters in ElementType. It wouldn't work to treat the two cases the same.

assert(f is ParameterizedType);
if (isGenericTypeAlias) {
return CallableGenericTypeAliasElementType(
f, library, packageGraph, element, returnedFrom);
}
// This is an indication we have an extremely out of date analyzer....
assert(
!isGenericTypeAlias, 'should never occur: out of date analyzer?');
// And finally, delete this case and its associated class
// after https://dart-review.googlesource.com/c/sdk/+/201520
// is in all published versions of analyzer this version of dartdoc
// is compatible with.
return CallableElementType(
f, library, packageGraph, element, returnedFrom);
} else if (isGenericTypeAlias) {
Expand Down Expand Up @@ -93,11 +100,10 @@ abstract class ElementType extends Privacy with CommentReferable, Nameable {
return '';
}

/// An unmodifiable list of this element type's parameters.
List<Parameter> get parameters;

DartType get instantiatedType;

Iterable<ElementType> get typeArguments;

bool isBoundSupertypeTo(ElementType t);
bool isSubtypeOf(ElementType t);

Expand All @@ -117,33 +123,20 @@ class UndefinedElementType extends ElementType {
@override
Element get element => null;

String _linkedName;

@override
bool get isPublic => true;

@override
String get name {
if (isImpliedFuture) return 'Future';
if (type.isVoid) return 'void';
assert({'Never', 'void', 'dynamic'}.contains(type.element.name),
'Unrecognized type for UndefinedElementType: ${type.toString()}');
return type.element.name;
}

/// Returns true if this type is an implied `Future`.
bool get isImpliedFuture => (type.isDynamic &&
returnedFrom != null &&
returnedFrom is DefinedElementType &&
(returnedFrom as DefinedElementType).modelElement.isAsynchronous);

@override
String get nameWithGenerics => '$name$nullabilitySuffix';

@override
String get nullabilitySuffix =>
isImpliedFuture && library.isNullSafety ? '?' : super.nullabilitySuffix;

/// Assume that undefined elements don't have useful bounds.
@override
DartType get instantiatedType => type;
Expand All @@ -158,45 +151,22 @@ class UndefinedElementType extends ElementType {
String get linkedName => name;

@override
Map<String, CommentReferable> get referenceChildren => {};
Iterable<ElementType> get typeArguments => [];

@override
Iterable<CommentReferable> get referenceParents => [];
Map<String, CommentReferable> get referenceChildren => {};

@override
// TODO(jcollins-g): remove the need for an empty list here.
List<Parameter> get parameters => [];
Iterable<CommentReferable> get referenceParents => [];
}

/// A FunctionType that does not have an underpinning Element.
class FunctionTypeElementType extends UndefinedElementType
with CallableElementTypeMixin {
FunctionTypeElementType(DartType f, Library library,
with Rendered, Callable {
FunctionTypeElementType(FunctionType f, Library library,
PackageGraph packageGraph, ElementType returnedFrom)
: super(f, library, packageGraph, returnedFrom);

@override
List<Parameter> get parameters => type.parameters
.map((p) => ModelElement.from(p, library, packageGraph) as Parameter)
.toList(growable: false);

@override
ElementType get returnType =>
ElementType.from(type.returnType, library, packageGraph, this);

@override
String get linkedName {
_linkedName ??= _renderer.renderLinkedName(this);
return _linkedName;
}

String _nameWithGenerics;
@override
String get nameWithGenerics {
_nameWithGenerics ??= _renderer.renderNameWithGenerics(this);
return _nameWithGenerics;
}

/// An unmodifiable list of this function element's type parameters.
List<TypeParameter> get typeFormals => type.typeFormals
.map((p) => ModelElement.from(p, library, packageGraph) as TypeParameter)
Expand All @@ -205,39 +175,42 @@ class FunctionTypeElementType extends UndefinedElementType
@override
String get name => 'Function';

ElementTypeRenderer<FunctionTypeElementType> get _renderer =>
@override
ElementTypeRenderer get _renderer =>
packageGraph.rendererFactory.functionTypeElementTypeRenderer;
}

class ParameterizedElementType extends DefinedElementType {
class AliasedFunctionTypeElementType extends FunctionTypeElementType
with Aliased {
AliasedFunctionTypeElementType(FunctionType f, Library library,
PackageGraph packageGraph, ElementType returnedFrom)
: super(f, library, packageGraph, returnedFrom) {
assert(type.aliasElement != null);
assert(type.aliasArguments != null);
}

@override
ElementTypeRenderer<AliasedFunctionTypeElementType> get _renderer =>
packageGraph.rendererFactory.aliasedFunctionTypeElementTypeRenderer;
}

class ParameterizedElementType extends DefinedElementType with Rendered {
ParameterizedElementType(ParameterizedType type, Library library,
PackageGraph packageGraph, ModelElement element, ElementType returnedFrom)
: super(type, library, packageGraph, element, returnedFrom);

String _linkedName;
@override
String get linkedName {
_linkedName ??= _renderer.renderLinkedName(this);
return _linkedName;
}

String _nameWithGenerics;
@override
String get nameWithGenerics {
_nameWithGenerics ??= _renderer.renderNameWithGenerics(this);
return _nameWithGenerics;
}

ElementTypeRenderer<ParameterizedElementType> get _renderer =>
packageGraph.rendererFactory.parameterizedElementTypeRenderer;
}

class AliasedElementType extends ParameterizedElementType {
AliasedElementType(ParameterizedType type, Library library,
PackageGraph packageGraph, ModelElement element, ElementType returnedFrom)
: super(type, library, packageGraph, element, returnedFrom) {
assert(type.aliasElement != null);
}
/// A [ElementType] whose underlying type was referrred to by a type alias.
mixin Aliased implements ElementType {
@override
String get name => type.aliasElement.name;

@override
bool get isTypedef => true;

ModelElement _aliasElement;
ModelElement get aliasElement => _aliasElement ??=
Expand All @@ -249,6 +222,26 @@ class AliasedElementType extends ParameterizedElementType {
.map((f) => ElementType.from(f, library, packageGraph))
.toList(growable: false);

Iterable<ElementType> _typeArguments;
@override
Iterable<ElementType> get typeArguments =>
_typeArguments ??= (type as ParameterizedType)
.typeArguments
.map((f) => ElementType.from(f, library, packageGraph))
.toList(growable: false);
}

class AliasedElementType extends ParameterizedElementType with Aliased {
AliasedElementType(ParameterizedType type, Library library,
PackageGraph packageGraph, ModelElement element, ElementType returnedFrom)
: super(type, library, packageGraph, element, returnedFrom) {
assert(type.aliasElement != null);
}

/// Parameters, if available, for the underlying typedef.
List<Parameter> get aliasedParameters =>
modelElement.isCallable ? modelElement.parameters : [];

@override
ElementTypeRenderer<AliasedElementType> get _renderer =>
packageGraph.rendererFactory.aliasedElementTypeRenderer;
Expand Down Expand Up @@ -304,24 +297,8 @@ abstract class DefinedElementType extends ElementType {
return canonicalClass?.isPublic ?? false;
}

@override
bool get isTypedef =>
modelElement is Typedef || modelElement is ModelFunctionTypedef;

@override
List<Parameter> get parameters =>
modelElement.isCallable ? modelElement.parameters : [];

ModelElement get returnElement => modelElement;
ElementType _returnType;
ElementType get returnType {
_returnType ??= ElementType.from(type, library, packageGraph, this);
return _returnType;
}

Iterable<ElementType> _typeArguments;

/// An unmodifiable list of this element type's parameters.
@override
Iterable<ElementType> get typeArguments =>
_typeArguments ??= (type as ParameterizedType)
.typeArguments
Expand Down Expand Up @@ -383,38 +360,46 @@ abstract class DefinedElementType extends ElementType {
modelElement.referenceParents;
}

/// Any callable ElementType will mix-in this class, whether anonymous or not.
mixin CallableElementTypeMixin implements ElementType {
@override
// TODO(jcollins-g): remove after dart-lang/dartdoc#2648 is fixed.
String get linkedName;

ModelElement get returnElement => returnType is DefinedElementType
? (returnType as DefinedElementType).modelElement
: null;
/// Any callable ElementType will mix-in this class, whether anonymous or not,
/// unless it is an alias reference.
mixin Callable implements ElementType {
List<Parameter> get parameters => type.parameters
.map((p) => ModelElement.from(p, library, packageGraph) as Parameter)
.toList(growable: false);

ElementType _returnType;
ElementType get returnType {
_returnType ??=
ElementType.from(type.returnType, library, packageGraph, this);
_returnType ??= ElementType.from(type.returnType, library, packageGraph);
return _returnType;
}

@override
FunctionType get type => _type;
}

Iterable<ElementType> _typeArguments;
Iterable<ElementType> get typeArguments =>
_typeArguments ??= type.aliasArguments
?.map((f) => ElementType.from(f, library, packageGraph))
?.toList() ??
[];
/// This [ElementType] uses an [ElementTypeRenderer] to generate
/// some of its parameters.
mixin Rendered implements ElementType {
String _linkedName;
@override
String get linkedName {
_linkedName ??= _renderer.renderLinkedName(this);
return _linkedName;
}

String _nameWithGenerics;
@override
String get nameWithGenerics {
_nameWithGenerics ??= _renderer.renderNameWithGenerics(this);
return _nameWithGenerics;
}

ElementTypeRenderer<ElementType> get _renderer;
}

/// A callable type that may or may not be backed by a declaration using the generic
/// function syntax.
class CallableElementType extends ParameterizedElementType
with CallableElementTypeMixin {
class CallableElementType extends DefinedElementType with Rendered, Callable {
CallableElementType(FunctionType t, Library library,
PackageGraph packageGraph, ModelElement element, ElementType returnedFrom)
: super(t, library, packageGraph, element, returnedFrom);
Expand All @@ -428,39 +413,9 @@ class CallableElementType extends ParameterizedElementType
packageGraph.rendererFactory.callableElementTypeRenderer;
}

/// Types backed by a [GenericTypeAliasElement] that may or may not be callable.
abstract class GenericTypeAliasElementTypeMixin {}

/// A non-callable type backed by a [GenericTypeAliasElement].
class GenericTypeAliasElementType extends TypeParameterElementType
with GenericTypeAliasElementTypeMixin {
class GenericTypeAliasElementType extends TypeParameterElementType {
GenericTypeAliasElementType(TypeParameterType t, Library library,
PackageGraph packageGraph, ModelElement element, ElementType returnedFrom)
: super(t, library, packageGraph, element, returnedFrom);
}

/// A Callable generic type alias that may or may not have a name.
class CallableGenericTypeAliasElementType extends ParameterizedElementType
with CallableElementTypeMixin, GenericTypeAliasElementTypeMixin {
CallableGenericTypeAliasElementType(FunctionType t, Library library,
PackageGraph packageGraph, ModelElement element, ElementType returnedFrom)
: super(t, library, packageGraph, element, returnedFrom);

ModelElement _returnElement;
@override
ModelElement get returnElement {
_returnElement ??= ModelElement.fromElement(
type.aliasElement.enclosingElement, packageGraph);
return _returnElement;
}

@override
ElementType get returnType {
_returnType ??=
ElementType.from(type.returnType, library, packageGraph, this);
return _returnType;
}

@override
DartType get instantiatedType => type;
}
3 changes: 2 additions & 1 deletion lib/src/generator/templates.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ import 'package:path/path.dart' as path show Context;

const _visibleTypes = {
Annotation,
CallableElementTypeMixin,
Callable,
Category,
Class,
Constructor,
Expand All @@ -50,6 +50,7 @@ const _visibleTypes = {
Enum,
Extension,
FeatureSet,
FunctionTypeElementType,
LanguageFeature,
Library,
LibraryContainer,
Expand Down
Loading