Do not depend on FunctionType being a subclass of ParameterizedType

[jcollins-g]

BREAKING CHANGE: Changing of modelType's ElementType and its subclasses may break custom templates if they depend on certain otherwise unused bits, like returnElement.
BREAKING CHANGE: Implicit futures (the type returned from async functions with no declared return type) will no longer be documented as Future, but instead will be documented as dynamic, more in line with the language specification but maybe not user expectations.

The upcoming PR, https://dart-review.googlesource.com/c/sdk/+/201520, makes FunctionType no longer a subclass of ParameterizedType which has a lot of implications in the parallel structure in Dartdoc's element_type.dart. This deals with those issues in a backwards compatible way.

Did some refactoring along the way -- more functionality has been extracted into mixins and the overall structure and usage of types is cleaner with less repetitive code.

[coveralls]

Coverage decreased (-0.3%) to 58.002% when pulling 7b2d063 on jcollins-g:functiontype-remove-members into e58ae4e on dart-lang:master.

[jcollins-g]

+@srawlins for general dartdoc, +@scheglov for analyzer compatibility

[scheglov]

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.

[jcollins-g]

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.

[scheglov]

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 :-)

[scheglov]

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?

[jcollins-g]

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.

[jcollins-g]

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.

[scheglov]

Same here, maybe - don't re-request, provide required data.

[jcollins-g]

This could be a good idea for the renderers -- I don't like that they require passing in the higher level objects myself. Will look into that later on -- it will be painful to do that now due to the class structure.