Some classes don't list their subclasses

[Hixie]

Seen on the Flutter documentation. It used to be that the widgets library version of RenderObjectWidget didn't have subclasses listed, but the material version did. Now, however, it's even worse, because since we (correctly) don't include the material version, the list is entirely unavailable.

e.g.: http://docs.flutter.io/flutter/widgets/RenderObjectWidget-class.html

[sethladd]

For completeness, what would you expect to see?

[Hixie]

Well for example InkResponse has a line that says "Implemented by InkWell TableRowInkWell":
https://docs.flutter.io/flutter/material/InkResponse-class.html

I would expect that on the page of any class with subclasses.

[Hixie]

Related bugs that might be easy to fix at the same time: #1069, #901

This actually might be a dupe of 1069, I'm not quite sure.

[sethladd]

Yeah, this is an important one to fix.

[jcollins-g]

If I understand correctly, any class that extends/implements another class in the same library should be mentioned in the parent class doc under "Implemented by:". A specific example for RenderObjectWidget would be the subclasses MultiRenderObjectWidget and LayoutBuilder. It looks like the code tries to do this already; I'll investigate why these cases get dropped.

[sethladd]

I'll investigate why these cases get dropped.

Thanks for taking a look!

[jcollins-g]

Swung back around to this and I think I've found the root of the problem. The _implementors map is sometimes accessed with Class objects that are different than the ones used to build the map. This should be fairly straightforward to correct.

[jcollins-g]

Actually, it's weirder than that. While processing the material library, the class html file for RenderObjectWidget in *widgets* gets written, with the correct "Implemented by" section. But then as the code continues to process the widgets library, the html file is overwritten with the wrong Implemented by section. Not clear yet to me what causes this.

[jcollins-g]

dartdoc does the equivalent of:

for library in package: library.compute_and_write_some_docs()

Chatted with @keertip and it seems recent fixes to put classes from other libraries in the same package that are imported in the right place has resulted in a situation where multiple overwrites of the same file can occur.

For example:

If library B defines class q, and library A imports class q, then the for loop above will write out B/q-class.html twice, once while processing library A, and once for library B. I've verified this using breakpoints on dartdoc while traversing flutter.

Where this is relevant for #1334 is that the table built up for determining what subclasses exist for a particular class is library specific. So if a class in A has a class r that subclasses class q, then executing the for loop above generated B/q-class.html twice: once, with the subclass r listed in "Implemented by:", and once without it. Depending on the order in which the libraries are processed, you'll get a different final version of B/q-class.html. The bug for #1334 was noticed because of the ordering (since widget.dart is alphabetically later than other libraries).

I will see whether this can be fixed with some sort of bandaid or if a larger refactoring is called for.

[jcollins-g]

Fixing up the key to the implementors map seems to be good enough, and I'm convinced that it's the right way to handle it. (For a while I was concerned that these needed to link to the canonical ModelElements but it actually makes sense for them to refer to the ModelElements for each library).

This works for the simple example, RenderObjectWidget:

As well as the StatefulWidget class, which has many implementors spread across different libraries. I verified links go to the right places, too.

Cleaning up the change and will upload a PR soon.

[jcollins-g]

It turns out this isn't quite enough to guarantee correct behavior due to the file overwriting. So I've altered the template writing to never write files multiple times. That will make element collision issues more obvious in the future. Also, we now only write documentation for the canonical version of classes.

This seems to correct a host of minor issues, such as the wrong library appearing in the top navigation bar for typedefs like AnimationStatusListener (should be flutter > animation > AnimationStatusListener, not flutter > widgets > AnimationStatusListener). And it speeds up documentation generation by 25% for flutter.

[jcollins-g]

Examples of the minor problems the anti-rewriting change solved for AnimationStatusListener. Note particularly the correction of what library the class is in in the navigation bar (it was always in the right place on the filesystem), and the change in the sidebars to put the right links in.

Before (correct screenshot now):

After:

[Hixie]

I'm hoping those aren't supposed to be the same class. :-)

This looks fantastic! Nice work.

[keertip]

Looks good!

[devoncarew]

@jcollins-g, thanks for the analysis and for digging into this; very nice to have it fixed!

[Sfshaza]

Yes, thanks so much, @jcollins-g!

[jcollins-g]

Unfortunately, during the cleanup I found more problems related to #1341. The tree construction recently added to support "isCanonical" seems to be wrong, and may be unnecessary. I'm currently investigating that.

[jcollins-g]

Hixie: Sadly, they are supposed to be the same class. There are bits in dartdoc that seem to obfuscate problems with the object model and the various trees and tables. (eg. If we can't find a canonical library for a ModelElement, just jam in whatever library element we have. Write out library data with a mishmash of canonical and non-canonical libraries. The above discussed file overwriting that changes things depending on the order the libraries are parsed in, etc). This investigation has been a progressive removing of said crutches as I discover them, then fixing all the quite nontrivial consequences when I do that.

It's all quite understandable in old software of course, but it means that this fix will take a bit longer. But the better news is, when I get done, we shouldn't have any more trouble in this area.

[sethladd]

Thank you @jcollins-g for working through this!

[Hixie]

Are you sure? Those two screenshots are of the AlwaysStoppedAnimation<T> class and the AnimationStatusListener typedef. If these are supposed to be the same class then things are definitely going more poorly than I understood! :-)

[jcollins-g]

Oh, oops, uploaded the wrong snapshot. Will fix that when I get back to my desk. :-)

[jcollins-g]

As I learn more about this problem and the codebase, I've come up with a more coherent question to ask regarding how flutter wants to document classes and other identifiers. One problem here is language: at various places different people and parts of Dart call a library each "part" within a file, the namespace declared with "library foo", the top level library in lib that exports files in lib/src in sequence and is the entry point for Dartdoc to create a unit of documentation, and even an entire pub package.

For the purpose of this question, a "library" is the top level library that is a unit of documentation generation inside Dartdoc. Dartdoc used to simply document any identifier exported within a package as part of libraries referencing them. However, flutter's structure means that many duplicate and confusing documents get created when dartdoc does this, making it hard to tell where identifiers "come from", due to reexporting. A discussion on #1158 covers some of the issues.

#1319 tried to address this and cut down on the number of duplicates, but file overwriting and some generous defaulting cloaked other bugs like this one and #1341.

The graph structure built by @astashov doesn't quite do it because sometimes there are exports we do want to treat as canonical, and the defaulting @astashov used wasn't quite enough to get all cases because you have to know exactly where to break the chain rather than assume that there's only one reexport.

Which boils down to the same question @astashov asked but received an partial response for, which is, how do we know which "library" to treat as canonical for a given identifier? Since it turns out not to be as simple as backing one up the export chain as the example given in #1158, we have to come up with a new definition.

I've attempted to come up with a set of rules that is both computable and matches the "I know it when I see it" sense of what should belong where I've inferred from the bugs and discussion on this issue.

The old way Dartdoc did this:

• An identifier exported within a package should be documented in any libraries referencing it.

New exceptions to this rule:

• If an identifier is referenced in one library and exported in another, it should only be documented by the exporting library. e.g. widget's ScrollPosition class should not be documented by material, where it is referenced. (We have duplicate docs still in some cases #1341)
• If an identifier is exported by more than one library, it should only be documented with the "canonical" library. (In Flutter docs, classes are listed multiple times #1158)
• A library is the canonical library for an identifier iff that identifier was not exported through referencing the public interface of another library (Some re-exported symbols have the wrong library links #1345, this bug).
• A "public interface" for a library is accessed through import/export "package:flutter/foo.dart" and similar. A "private interface" is referenced with a relative path to a dart file, e.g. "foo.dart" from something in lib/src.

So, if I implement this, the only way a reexported symbol from flutter library will be documented is if it was exported from private to public, not on future reexports between public libraries. This provides a way for flutter to have some control over what is and isn't "public" and therefore documented, and seems reasonably intuitive in practice from inspection -- flutter already uses public interfaces where it seems the intention is not to "take ownership" of the symbol from the other library.

Comments?

[jcollins-g]

Screenshots now correct in earlier comment around file rewriting.

[Hixie]

My ideal approach would be as follows:

For each package, determine the top-level libraries and the internal libraries. Top-level libraries are determined by some heuristic that prefers:

• those with a library keyword.
• those in the lib/ directory, not in the lib/src directory.

For each package, determine the set of packages imported by this package. All packages, whether directly imported or imported by indirect reference, are to be included in the documentation.

For each identifier, determine the package that actually declares the identifier. If it's re-exported by another package, that other package is ignored for the purposes of this identifier.

For each identifier in a package, determine the canonical top-level library for that identifier, by some heuristic that prefers:

• top-level libraries that import or export other top-level libraries should not be canonical for identifiers defined by those other libraries, unless there's a reference loop which would make it unclear who actually exports it. (In other words, attempt to create a tree, and the lowest level of the tree that introduces the identifier is the canonical source of that identifier.)
• if an identifier is introduced by a library A, and reexported by a library B, and B itself is reexported by a library C (which does not directly export A), then the canonical library is the first top-level library in the list A, B, C. (This is another approach to creating a tree and picking the lowest level as the canonical source.)
• if the previous heuristics aren't sufficient, prefer the top-level library whose name (e.g. the rendering in package:flutter/rendering.dart) matches a component of the path to the library that introduces the identifier (e.g. the rendering in flutter/lib/src/rendering/object.dart).

Only document the identifier in its canonical top-level library.

For bonus points, the page for the identifier could also list other top-level packages that reexport the identifier, as "Also export from: package:flutter/widget.dart, package:flutter/material.dart", or some such.

I would expect the above to result in more packages being listed in the dartdocs. For example, I would expect to see vector_math appear in the list if you follow through the steps above.

[jcollins-g]

@Hixie

Do you literally mean "all packages imported" rather than exported? Typically we don't document imported identifiers AFAICT.

Otherwise, your heuristics seem very close to what I was planning to implement. I plan to break reference loops using some variant of the private/public distinction I previously mentioned. I think that will also obviate the need for the path matching, but more testing is required; if not the path matching isn't an unreasonable fallback.

I like the idea of an "also exported from" for classes to point to other libraries in the same package. I'll see what I can do if I can get the rest of this working.

[Hixie]

I meant all the imported packages. Eventually we're going to make the Flutter SDK hermetic, shipping a fixed set of packages with fixed versions, and it'll be good to document all those packages together.

If you want to start with just documenting the things that are accessible via the packages being passed to the dartdoc tool, that's fine too.

The end result that I care about is I want to see vector_math be the package that documents Matrix4. Right now it's widgets, which is one of the places we reexport Matrix4 from. I don't mind if the vector_math documentation doesn't include things that aren't reexported, though that might be weird.

[jcollins-g]

In order for this canonicalization to be consistent, it seems I had to implement what you suggested as a side effect. Canonicalization means that documenting Matrix4 inside of widgets is no longer allowed without extreme ugliness to keep that exception. So, to support doing it at all I had to fix the --auto-include-dependencies flag to work better with the current structure of Dart packages.

The result will be something like this when --auto-include-dependencies is passed in:

It's part of package "flutter" because we're documenting it from there, so that's a little odd, but given that the versions of those packages selected are dependent on the flutter package it's not entirely nonsensical.

Without --auto-include-dependencies, Matrix4 will simply not be documented at all and links to it in docs will be dropped. This is a big enough behavior change I'm going to suggest incrementing the dartdoc minor version.

[Hixie]

From Flutter's perspective, it's fine if it says "flutter" at the top, because the docs are all in the context of flutter. So SGTM. :-)

[jcollins-g]

The fixing of canonicalization is requiring major surgery inside of dartdoc, including:

• Making the various element caching schemes actually function at all layers, and all instantiations of ModelElements use them
• Handling split field (getter with no setter or the reverse) inheritance at least a little better
• Detecting canonicalization of inherited fields through complex inheritance chains, esp. when not using recursive dependency documentation generation
• Correctly handling recursive dependency documentation generation

The existing data structures just weren't designed for most of this, and so the change continues to grow. Frustratingly, all the changes are linked together, because changing one thing here without changing the others generates broken docs. I'll continue to update this branch with my work in progress.

[sethladd]

Thanks for the update. Sorry to hear so much surgery is required, but glad you're on the case and improving the tool!

[jcollins-g]

Branch is getting close. I'm now working on re-fixing the original issue here with the modifications to the data model.

[jcollins-g]

The new canonicalization fixes have a couple of implications when --auto-include-dependencies is not passed on the command line.

First, there is the possibility that if a package reexports a class from outside the package from multiple top level libraries, dartdoc will be unable to determine which of those top level libraries should be considered the "real" one and own it. Matrix4 has this problem in flutter since it is directly exported at the top level in two libraries, and will display a warning something like this:

Warning: ambiguous reexport of Matrix4 (package:flutter/material.dart), canonical candidates: (rendering, widgets)

In this case, dartdoc will just pick the first one (rendering), claim that it is canonical for Matrix4, link all references to that one, and display the warning.

The second possibility already occurred in some cases in the previous version (though now we will warn about it), which is that classes that are referenced but never exported in a package won't have links to them. Dartdoc will now show a warning like this when that happens:

Warning: no canonical library found for VMServiceClient (package:vm_service_client/vm_service_client.dart), not linking

Both of these cases go away if you add --auto-include-dependencies (or --include the relevant libraries), in which case the identifiers will get linked to their appropriate places.

[jcollins-g]

And I just realized there's a third possibility we'll warn similarly as to number 2 but was previously hidden by the canonicalization bugs. If a reexported class from another package, like Matrix4, refers to classes in parameters that aren't themselves reexported by the package, we won't be able to link to them. IMO this is actually a problem with the package; if you want to make Matrix4 part of the interface to flutter you should probably be reexporting all the identifiers its interface depends on. Dartdoc will now alert you to this:

Warning: no canonical library found for Vector3 (package:vector_math/vector_math_64.dart), not linking.

With --auto-include-dependencies, though, at least it will link to the right library so the user will know what to import themselves.

[jcollins-g]

The ginormous branch is nearing readiness to mail out a PR for; have to write changelogs, update dartdoc's minor version to 0.10, and write a handful of tests.

In the meantime: flutter-demo-docs.tar.gz

This is a generated set of flutter docs with the latest dartdoc on my branch. Also included is "output.txt" which is output from the bot script. Note in particular some of the new warnings; they seem to be legitimate problems with what dartdoc is trying to analyze that were either ignored or hidden by bugs before. Some of them may be fixable with improvements to the bot script -- and you'll see details on what they mean in the above comments.

@Hixie, @sethladd, if you have time to download and browse the docs for structural problems like the original problem that spawned this work, please do so -- would appreciate the feedback.

[Hixie]

Looks like the majority of the warnings are because we don't yet set --auto-include-dependencies? We should just set that, right?

[jcollins-g]

Possibly... trying that seems to generate some confusing results, including having two copies of the SDK documented. I've got some ideas for fixing up the bot script to arrange the libraries in a more flutter-like way to avoid this but haven't gotten to implementing that yet.