canonicalization overhaul part 3: Types, Templates, and Public/Private #1524

jcollins-g · 2017-10-18T16:29:36Z

Going to split this out and clean up some of the architectural duct tape here, but creating a PR for reference so @devoncarew and @keertip can see where this is going.

Investigating #1512 resulted in the detection of a regression in #1514, related to the generic type support recently added in #1506 and related PRs. It turns out this is badly broken -- as indicated in #1514 there are some highly visible docs that have confusing types listed since it's normal for different classes type parameters to be T.

Fixing this pulled on some threads related once again to canonicalization introduced in #1319, with refinements in #1368 and #1390. In order to resolve types correctly, dartdoc's ElementType class has to be instantiated properly, with the right ModelElement and type. To do that, ModelElements have to keep track of the original Member objects that contain type resolution information from the analyzer. And finally, in order to handle referring to elements defined in non-public libraries but reexported elsewhere, we need to have a better handle on what is public vs. non-public, and canonical vs. not-canonical.

The original pre-canonicalization dartdoc documented everything it saw if it was public, and didn't document it if it wasn't public. This made sense since dartdoc didn't try to dedupe as requested in issue #1090, so resolved types could always be displayed. Dartdoc also didn't need to distinguish between Member and Element objects and always used them the same, because issues related to generic types hadn't yet revealed problems like dart-lang/sdk#29600.

However, after canonicalization, there were various hacks that tried to keep type resolution mostly functional, though resulting in some inconsistent behavior noted in #1512. Still, until we added support for anonymous functions & working generic types, these hacks were good enough. But the whole mess finally came to a head there and it became impossible to fix #1514 without either breaking the function support or reworking the base data structures to include more information, including information about non-public ModelElements contained in classes, libraries, etc.

Adding non-public items meant fixing the private/public distinction inside ModelElements. This now results in a new property, isDocumented, and new hasFoo / allFoo equivalents explicitly filtering for public items if needed. (hasPublicFoo, allPublicFoo) with the default being that all elements are included. Fixing the templates for that was virtually impossible with the old version of mustache4dart which silently failed, but the new version obeys the "fail on missing" flag so I upgraded that and then promptly discovered a pile of existing template bugs as well, fixing those.

Now that all objects are included so that type resolution works and the isDocumented call checks canonicalization, warnings became more likely to happen on non-documented elements because we traverse them now. So introduced some infinite-recursion prevention in the warning system and added isDocumented checks for a couple of the non-canonicalization-related warnings typically generated by non-public elements. This results in a net win, hiding many warnings we had that were nonsensical from before this change (partial fix for #1412) as well as squelching new warnings generated by the type resolution fixes.

The planned breakout of this change will be to submit the pieces in the following order if I can:

Warnings improvement
Public/private distinction, mustache4dart upgrade, and template fixes
ModelElement instantiation and Member tracking
Type resolution fixes

Disentangling them along these lines will take a bit but should make it easier to review.

kevmoo · 2017-10-18T18:02:09Z

lib/src/html/html_generator_instance.dart

+        if (property.name == 'topLevelVariable')
+          1+1;
+        if (property.linkedReturnType == '')
+          1+1;


kevmoo · 2017-10-18T18:02:23Z

lib/src/html/html_generator_instance.dart

-        if (!property.isCanonical) continue;
+      for (var property in lib.properties.where((p) => p.isDocumented)) {
+        if (property.name == 'topLevelVariable')
+          1+1;


kevmoo · 2017-10-18T18:02:43Z

lib/src/markdown_processor.dart

@@ -390,6 +390,7 @@ Element _findRefElementInLibrary(String codeRef, ModelElement element,
  assert(element != null);
  assert(element.package.allLibrariesAdded);

+


??? Dartfmt won't like this...

jcollins-g · 2017-10-18T19:16:42Z

To be clear, this is WIP and not intended to land. Mostly this will be reference material for the breakout pieces as I submit those.

devoncarew · 2017-10-19T16:45:57Z

Taking a spin through now just to get familiar -

devoncarew

I took a pass through; looking forward to the sub-PRs. Also, good to hear that the new mustache4dart version does template validation :)

devoncarew · 2017-10-19T19:10:37Z

lib/src/html/html_generator_instance.dart

@@ -96,86 +96,78 @@ class HtmlGeneratorInstance implements HtmlOptions {

    generatePackage();

-    for (var lib in package.libraries) {
+    for (var lib in package.libraries.where((l) => l.isDocumented)) {


Consider putting isDocumented onto Documentable, and having a utility method that would filter a list of Documentable objects to just the ones that were documented.

(I see a lot of .where((o) => o.isDocumented below)

kevmoo · 2017-10-26T20:12:01Z

Ran this through the SDK – just curious. Sooo much better. Love seeing all of random toString and hashCode members gone. Also great to see links to Stream<List> in generics. Very nice.

Two things I noticed.

Some private members show up

Compare https://api.dartlang.org/be/151323/dart-io/FileSystemEvent-class.html CONSTANTS to

Saw a few cases where Implements links are dead

Look at https://api.dartlang.org/be/151323/dart-io/ContentType-class.html
See how the HeaderValue is live? It's not w/ this PR

I have no clue if you're already aware of these – sorry for the noise if you are.

Otherwise, super excited to see this change get in!

kevmoo · 2017-11-08T19:44:32Z

lib/src/warnings.dart

@@ -256,12 +256,14 @@ class PackageWarningCounter {
  }

  int get errorCount {
+    if (_warningCounts.isEmpty) return 0;
    return _warningCounts.keys
        .map((w) => options.asErrors.contains(w) ? _warningCounts[w] : 0)
        .reduce((a, b) => a + b);


using fold here would let you avoid the extra case

kevmoo · 2017-11-08T19:44:50Z

lib/src/warnings.dart

    return _warningCounts.keys
        .map((w) => options.asErrors.contains(w) ? _warningCounts[w] : 0)
        .reduce((a, b) => a + b);
  }

  int get warningCount {
+    if (_warningCounts.isEmpty) return 0;
    return _warningCounts.keys
        .map((w) =>
            options.asWarnings.contains(w) && !options.asErrors.contains(w)


Ditto below – replace reduce with fold...at least I think

jcollins-g · 2017-11-08T20:32:07Z

With the exception of flutter's dartdoc script needing to be updated for new parameters and code cleanup, I've finished making the part 3 overhaul function for Flutter, Angular, and the Dart SDK with head and stable versions of analyzer, as well as addressing the issues @kevmoo mentioned and many more.

If it wasn't a breaking change before, it definitely is now. New flags and new naming of anonymous libraries make this a big change.

In order to fix the complex packages without losing functionality, several more changes were required.

Bizarre mid-word hyperlinking issue #1535 had to be addressed. Lots of previously unlinkable things becoming linkable in the overhaul resulted in this bug getting worse, hitting cases where it was generating invalid HTML in the summary case when the string was truncated.
angular_components contains multiple public libraries named the same, breaking dartdoc assumptions #1526 had to be dealt with. Anonymous libraries with the same base name inside the same package is now a thing in Angular. Dartdoc now places anonymous libraries on disk in such a way to avoid collisions, but keeps the user-visible names of the libraries as they were.
Basic package boundary awareness has been added to dartdoc. Dartdoc now knows a little bit about package boundaries in canonicalization, and --auto-include-dependencies now traverses package rather than library dependencies. A new --exclude-packages flag allows for the exclusion of packages when using --auto-include-dependencies. This was necessary to squelch bogus warnings and make it possible to discern real bugs in the overhaul vs. noise.
Package boundary awareness means fewer ambiguous doc reference warnings when using --auto-include-dependencies. dartdoc now knows to stay local to the package when it is ambiguous which symbol a reference links to.
Package boundary awareness means all useless "no canonical found" warnings have been silenced at last. There's only one left now in the SDK that seems to be legitimate (a public class AttributeClassSet inherits from a impl class that's only in dart:html_common).

Code here still needs to be cleaned up, including removing my persistent breakpoints (the "1+1"s), and refactoring package construction so that the tests can use the same code as the mainline dartdoc for dependency recursion.

Additional pieces for the planned split in addition to what I already indicated:

Bizarre mid-word hyperlinking issue #1535
Disk layout of anonymous libraries
Some aspects of package boundary awareness

kevmoo · 2017-11-08T20:50:18Z

FWIW, I went through the cases I'd seen. All cleared up. Everything looks so much better. Awesome work!

devoncarew · 2017-11-08T21:16:18Z

@jcollins-g, just to confirm:

The planned breakout of this change will be to submit the pieces in the following order if I can:

Still planned (the-breakup-into-pieces)?

jcollins-g · 2017-11-08T21:40:02Z

@devoncarew Yes. There will be a couple more pieces now, sadly, but that is still planned.

jcollins-g · 2017-11-09T16:53:34Z

Oh, and for those perusing the code, there's been an additional change here that I was trying to avoid but it was just getting too hard to deal with the cut-and-pastiness of Dartdoc. (Not anyone's fault -- this is what happens when a small tool grows without enough focused time on updating the design to match).

I've begun selectively factoring out common sections of code and interfaces into mixins and abstract classes, hopefully in a way that makes some sense. There's now a Canonicalization class for adding Dartdoc canonicalization support, a Privacy class for elements that support isPublic and its friends, a TypeParameters class for elements with type parameters, and so on.

This hasn't yet been addressed systematically because I was really trying to just fix one bug here and I'm only factoring out code that I'm touching. But hopefully I will have time to follow up and address the larger structural changes Dartdoc needs.

kevmoo · 2017-11-09T16:58:04Z

I've begun selectively factoring out common sections of code and interfaces...

🎉 Yay for maintainability!

jcollins-g · 2017-11-13T17:28:20Z

Starting the breakout now, first piece will be some of the separable warnings changes.

* first piece * Finish warnings breakout * dartfmt * Review comment

jcollins-g · 2017-11-27T23:10:42Z

I've managed to break out a few pieces, but disentangling changes is starting to become difficult. It's getting very hard to identify non-mutually-dependent pieces of any size. A few things I thought might be separable, like elements of the private/public distinction, can't be extracted without dragging most or all over the overhaul along and still have dartdoc pass tests and generate valid docs. So, we're going to try reviewing this as it is now. Open for comments!

…arams

devoncarew · 2017-11-28T23:32:44Z

lib/dartdoc.dart

-import 'package:analyzer/source/sdk_ext.dart';
-import 'package:analyzer/src/context/builder.dart';
-import 'package:analyzer/src/dart/sdk/sdk.dart';
+import 'package:analyzer/analyzer.dart';


That's convenient :)

devoncarew · 2017-11-28T23:36:49Z

lib/dartdoc.dart

+          .where((_Error error) => error.isError)
+          .toList()
+            ..sort();
+      // TODO(jcollins-g): Why does the SDK have analysis errors?  Annotations


Perhaps also track as an issue?

done, and issue added to relevant TODOs.

devoncarew · 2017-11-28T23:39:50Z

lib/src/element_type.dart

@@ -58,7 +71,10 @@ class ElementType {
    return _linkedName;
  }

-  String get name => _type.name;
+  String get name {
+    if (_type.name == null) return _type.element.name;


or, return _type.name ?? _type.element.name;?

or, if we're going to get fancy: String get name => _type.name ?? _type.element.name;

Done. :-)

devoncarew · 2017-11-28T23:44:30Z

lib/src/model.dart

-  Accessor(PropertyAccessorElement element, Library library)
-      : super(element, library);
+  Accessor(
+      PropertyAccessorElement element, Library library, Member originalMember)


I see some subclasses pass null in for originalMember; perhaps make it an [optional] param?

dart-lang/sdk#15101 makes this very tricky. For now, I prefer the simpler approach of passing nulls rather than the suggested workaround, as it is easier to understand at a glance what the code is doing. Added a TODO.

devoncarew · 2017-11-28T23:45:53Z

lib/src/model.dart

@@ -2278,7 +2510,12 @@ abstract class ModelElement extends Nameable
  // WARNING: putting anything into the body of this seems
  // to lead to stack overflows. Need to make a registry of ModelElements
  // somehow.
-  ModelElement(this._element, this._library);
+  ModelElement(this._element, this._library, this._originalMember) {}


ditto re: possibly making _originalMember optional

devoncarew · 2017-11-28T23:50:51Z

lib/src/model.dart

+  Iterable<TopLevelVariable> get publicConstants => filterNonPublic(constants);
+
+  String _dirName;
+  String get dirName {


Thinking about a pattern for this; not sure this helps, but here's one idea:

String get dirName { return _dirName ?? (_dirName = () { return ... ; // init code... }()); }

Yeah, not sure I like that one -- it is far more complex than the original even if it does save us a bit of vertical space. But let's keep thinking on it!

devoncarew · 2017-11-28T23:55:31Z

lib/src/model.dart

@@ -4548,3 +4864,313 @@ class TypeParameter extends ModelElement {
  @override
  String toString() => element.name;
 }
+
+/// Everything you need to instantiate a Package object for documenting.
+class PackageBuilder {


+1, this, class Canonicalization, class Privacy - I think the modeling improvements here will really help.

devoncarew · 2017-11-28T23:57:18Z

lib/src/model.dart

+  AnalysisContext get context {
+    if (_context == null) {
+      // TODO(jcollins-g): fix this so it actually obeys analyzer options files.
+      var options = new AnalysisOptionsImpl()..enableAssertInitializer = true;


@bwilkerson - Janice has changed dartdoc so that it only creates an analysis context in one place now. We should look into unifying with how the analysis server creates contexts - we have several outstanding issues as dartdoc doesn't pick up the analysis options settings from projects.

devoncarew · 2017-11-28T23:59:52Z

test/src/utils.dart

+          [],
+          [],
+          true,
+          withAutoIncludedDependencies)


a trailing comma here (and some other places with less than ideal formatting) may make dartfmt format this better

(I find that trailing commas can make things this split lines dartfmt better)

Apparently not in this case -- the addition of the method call on the end seems to make this extra-ugly.

Package bootBasicPackage( String dirPath, List<String> excludes, bool withAutoIncludedDependencies) { Directory dir = new Directory(dirPath); return new PackageBuilder( dir, excludes, [], sdkDir, new PackageMeta.fromDir(new Directory(dirPath)), [], [], true, withAutoIncludedDependencies, ) .buildPackage(); }

…arams

jcollins-g · 2017-11-30T17:40:23Z

Disabling dartdoc's flutter travis for now, since they need to pass different parameters to dartdoc for the new version to work. I'm checking Flutter manually for now.

kevmoo · 2017-11-30T18:01:58Z

Well done, @jcollins-g !!!

googlebot added the cla: yes Google CLA check succeeded. label Oct 18, 2017

kevmoo reviewed Oct 18, 2017

View reviewed changes

devoncarew reviewed Oct 19, 2017

View reviewed changes

jcollins-g mentioned this pull request Oct 25, 2017

angular_components contains multiple public libraries named the same, breaking dartdoc assumptions #1526

Closed

jcollins-g mentioned this pull request Nov 3, 2017

Bizarre mid-word hyperlinking issue #1535

Closed

jcollins-g force-pushed the fix-type-params branch from ad24a5a to 966f85b Compare November 8, 2017 19:07

kevmoo reviewed Nov 8, 2017

View reviewed changes

jcollins-g force-pushed the fix-type-params branch from 9245296 to 835570a Compare November 13, 2017 16:59

This was referenced Nov 15, 2017

Warnings enhancements from #1524 #1539

Merged

Update mustache4dart and clean up templates #1540

Merged

Dartdoc does not accept super calls from mixins #1541

Closed

jcollins-g force-pushed the fix-type-params branch from 89c7834 to ff8af5f Compare November 20, 2017 16:19

jcollins-g added a commit that referenced this pull request Nov 20, 2017

Warnings enhancements from #1524 (#1539)

56a0148

* first piece * Finish warnings breakout * dartfmt * Review comment

jcollins-g force-pushed the fix-type-params branch from ff8af5f to a6fad37 Compare November 20, 2017 16:48

jcollins-g force-pushed the fix-type-params branch 4 times, most recently from e44655e to b16f863 Compare November 27, 2017 18:30

jcollins-g mentioned this pull request Nov 27, 2017

Fix truncation in middle of links for constants #1544

Merged

jcollins-g added 2 commits November 28, 2017 07:49

Flatten changes one last time

b3ef88d

Merge branch 'master' of github.com:dart-lang/dartdoc into fix-type-p…

971153c

…arams

jcollins-g force-pushed the fix-type-params branch from df7c56a to 971153c Compare November 28, 2017 16:06

devoncarew approved these changes Nov 29, 2017

View reviewed changes

jcollins-g added 2 commits November 30, 2017 07:31

Merge branch 'master' of github.com:dart-lang/dartdoc into fix-type-p…

e1e06cd

…arams

Fix tests post analyzer update

7a134cc

jcollins-g mentioned this pull request Nov 30, 2017

Find out root cause of spurious analysis errors in SDK when running dartdoc #1547

Open

jcollins-g added 2 commits November 30, 2017 09:27

Review comments

93a6d87

Disable travis temporarily

eedcc55

jcollins-g merged commit 07a4f4c into master Nov 30, 2017

jcollins-g deleted the fix-type-params branch November 30, 2017 17:58

This was referenced Nov 30, 2017

Private superclasses lead to dead references #1476

Closed

DartDoc for Dart SDK shows internal interfaces. #1173

Closed

jcollins-g mentioned this pull request Mar 12, 2018

API docs sidebar has undesired entries flutter/flutter#15290

Closed

jcollins-g mentioned this pull request Apr 4, 2018

dartdoc's unit test setup diverges significantly from actual usage #1420

Closed

		@@ -390,6 +390,7 @@ Element _findRefElementInLibrary(String codeRef, ModelElement element,
		assert(element != null);
		assert(element.package.allLibrariesAdded);

canonicalization overhaul part 3: Types, Templates, and Public/Private #1524

canonicalization overhaul part 3: Types, Templates, and Public/Private #1524

Conversation

jcollins-g commented Oct 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcollins-g commented Oct 18, 2017

devoncarew commented Oct 19, 2017

devoncarew left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevmoo commented Oct 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcollins-g commented Nov 8, 2017 • edited Loading

kevmoo commented Nov 8, 2017

devoncarew commented Nov 8, 2017

jcollins-g commented Nov 8, 2017

jcollins-g commented Nov 9, 2017

kevmoo commented Nov 9, 2017

jcollins-g commented Nov 13, 2017

jcollins-g commented Nov 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcollins-g commented Nov 30, 2017

kevmoo commented Nov 30, 2017

jcollins-g commented Oct 18, 2017 •

edited

Loading

jcollins-g commented Nov 8, 2017 •

edited

Loading

jcollins-g commented Nov 27, 2017 •

edited

Loading