Calculate what extensions could apply with an extension tree #2061
Conversation
| /// instantiation of that type where the extension would be applicable if | ||
| /// made accessible in the namespace per: | ||
| /// https://github.com/dart-lang/language/blob/master/accepted/2.6/static-extension-members/feature-specification.md#implicit-extension-member-invocation | ||
| class ExtensionNode { |
There was a problem hiding this comment.
I'm not convinced that a tree structure is adequate, given that it appears to follow the type hierarchy but the type hierarchy is a graph structure. If the declarations look like
class A {}
class B extends A {}
then, as I understand it, there would be a node for A and a node for B that is a child of the node for A. But if the declarations look like
class A {}
class B {}
class C implements A, B {}
then there would be nodes for A, B, and C, and C would need to be a child of both the node for A and the node for B, which a tree doesn't allow.
In analyzer we just compute a list of the extensions visible within a single library, then recompute the set of applicable extensions based on a given type without trying to build a graph of extensions. I think that's a cleaner approach, but ymmv.
|
|
||
| /// The set of seen [ModelElement]s backing [extendedType]s in this tree. | ||
| /// Only valid at the root. | ||
| Set<ModelElement> _seen; |
There was a problem hiding this comment.
I'm not sure what you mean by "Only valid at the root.". If you mean that only the node at the root of the tree will have a value for this field and other nodes will have null values, then consider either creating a subclass such as ExtensionRootNode that's only used for the root node that defines this field, or create an ExtensionTree that holds both this field and the root node. The space savings will probably be minimal, but it will allow you to make this a non-nullable field in the future. Ditto for _genericsInserted.
| Set<ModelElement> _genericsInserted; | ||
|
|
||
| /// The type all [extensions] are extending. | ||
| final DefinedElementType extendedType; |
There was a problem hiding this comment.
For the purposes of dartdoc, could this just be the element associated with the extended type?
|
|
||
| /// The [PackageGraph] associated with the tree; must be the same for all | ||
| /// nodes. | ||
| final PackageGraph packageGraph; |
There was a problem hiding this comment.
If you do have a tree / root node, then this field can be moved there as well and a getter added to walk up the parent chain to find it. That would ensure that it's the same for all nodes.
|
|
||
| /// Merge from [other] node into [this], unconditionally. | ||
| void _merge(ExtensionNode other) { | ||
| //assert(other.extendedType.type == extendedType.type); |
| documentedPackages.toList().forEach((package) { | ||
| package._libraries.forEach((library) { | ||
| for (Extension e in library.extensions) { | ||
| ExtensionNode returned = _extensions.addExtension(e); |
There was a problem hiding this comment.
Does this need to filter out private extensions?
|
withdrawing for now. @bwilkerson is right in that a tree structure is likely inadequate. |
More groundwork for #2021 and #2033.
This is more accurate than #2050 and should have more scalable performance. In particular it fixes some bugs in the tests that were verifying incorrect output.