Canonicalization overhaul #1368

jcollins-g · 2017-04-07T17:04:56Z

The giant canonicalization overhaul is shaped up well enough I'd prefer to try to submit a PR before improving it further. There are definitely still some known issues, but I'm now reasonably satisfied it introduces no new problems.

This is big. Some tips on reviewing: Changes to ModelElement and the new mixin class, Inheritable, in lib/src/model.dart are the most important, especially any method with the word "canonical" in its name. If you understand the changes there, most of the rest of what you see can be understood as downstream consequences. I've tried to apply comments and inline documentation liberally, please feel free to ask for more where it isn't clear.

…nonicalization where possible

…nal seem fixed

…his for real.

…nonicalization where possible

…nal seem fixed

…his for real.

…rrect now

…date test docs

…anceChain

…s to work here at least

…mentors-and-file-overwriting

…e-deps had us throwing half the SDK docs into there.

…SDK again.

jcollins-g · 2017-04-07T20:06:42Z

lib/src/model.dart

+  /// clean that up.
+  factory ModelElement.from(Element e, Library library,
+      {Class enclosingClass}) {
+    if (library != null &&


get rid of this breakpoint

jcollins-g · 2017-04-07T20:07:39Z

lib/src/model.dart

+        else
+          newModelElement = new Operator.inherited(e, enclosingClass, library);
+      }
+      if (library != null &&


delete breakpoint

devoncarew · 2017-04-10T15:46:26Z

Taking a first pass through today -

devoncarew

One pass through:

the canonicalization work is excellent; this will be a night and day difference for exports from a correctness POV
the single factory ctor of model elements will really help the code quality, instead of having construction logic spread throughout the codebase
we'll need to examine the use of asserts going forward, and make sure that there aren't any asserts that would be better handled as user facing errors

Given the scope of the fixes, this would make a good line in the sand for a 1.0 version. Perhaps we should do some light dartlang.org / css upgrades to give the generated html a slight facelift at the same time?

devoncarew · 2017-04-10T15:49:08Z

bin/dartdoc.dart

@@ -41,7 +41,7 @@ main(List<String> arguments) async {

  Directory sdkDir = getSdkDir();
  if (sdkDir == null) {
-    print("Error: unable to locate the Dart SDK.");
+    stderr.write(" Error: unable to locate the Dart SDK.");


+1 to using stderr here

devoncarew · 2017-04-10T19:59:15Z

lib/src/html/html_generator_instance.dart

-    _writeFile(filename, content);
+    // If you see this assert, we're probably being called to build non-canonical
+    // docs somehow.  Check data.self.isCanonical and callers for bugs.
+    assert(!_writtenFiles.contains(fullName));


Given these new asserts, we should make sure that whenever we run dartdoc during development (or on our CI) we run in checked mode. I believe there are some cases where the grind script is not run in checked mode.

Done. Grind script now always runs dartdoc in checked mode.

devoncarew · 2017-04-11T04:20:39Z

lib/src/model.dart

+            break;
+          }
+        }
+        if (definingEnclosingElement.isCanonical)


nit: we should have braces around single line if statements

devoncarew · 2017-04-11T14:26:16Z

lib/src/model.dart

    Map<String, ExecutableElement> imap =
-        manager.getMembersInheritedFromInterfaces(element);
+        library.inheritanceManager.getMembersInheritedFromInterfaces(element);

    _inheritedProperties = [];
    List<ExecutableElement> vs = [];


I believe vs here stands for values. If so, perhaps we should rename the variable for understandability?

devoncarew · 2017-04-11T14:27:37Z

lib/src/model.dart

@@ -610,13 +683,15 @@ class Class extends ModelElement implements EnclosedElement {
    return _operators;
  }

-  List<Operator> get operatorsForPages => _genPageOperators;
+  List<Operator> get operatorsForPages =>
+      _genPageOperators.toList(growable: false);


I'm curious, is there a reason why you're setting growable to false?

devoncarew · 2017-04-11T14:41:55Z

pubspec.yaml

@@ -19,9 +19,9 @@ dependencies:
  mustache4dart: ^1.0.9
  package_config: '>=0.1.5 <2.0.0'
  path: ^1.3.0
-  quiver: '>=0.18.0 <0.25.0'
-  resource: '>=1.1.0 <3.0.0'
+  resource: ^1.1.0


resource is on 2.1.2; are we compatible with that?

Not sure. I just didn't see the reason for the ceiling so removed it (if we can cross one major version, likely we can cross others).

I can bump to ^2.1.0, add the ceiling, or both. What do you think?

I'd just check that the sdk deps is bringing in a 2.0 version, and then bump our use up to ^2.0.0.

devoncarew · 2017-04-11T14:43:06Z

test/model_test.dart

-      expect(ExtendingClass.supertype.name, equals('BaseClass'));
-      expect(
-          ExtendingClass.supertype.element.library.name, equals('two_exports'));
+      expect(ExtendingClass.superChain[0].name, equals('BaseClass'));


I tend to use foo.first instead of foo[0] in many cases - it's a bit more semantic; YMMV.

devoncarew · 2017-04-11T14:44:37Z

tool/grind.dart

@@ -2,7 +2,7 @@
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.

-import 'dart:async' show Future;
+import 'dart:async' show Future, Stream;


you could also just drop the show clause here...

devoncarew · 2017-04-11T14:45:34Z

tool/grind.dart

@@ -259,6 +265,34 @@ int _findCount(String str, String match) {
  return count;
 }

+Stream<FileSystemEntity> dirContents(String dir) {
+  var lister = new Directory(dir).list(recursive: true);


return immediately instead of creating a local var?

devoncarew · 2017-04-11T14:45:54Z

tool/grind.dart

+  return lister;
+}
+
+_doFileCheck(String origin, Set<String> visited, bool error) {


jcollins-g · 2017-04-11T16:09:26Z

Tagging more issues this solves:

partial fix for: #1239, #1197
fixes: #589, #1090

jcollins-g · 2017-04-11T17:00:50Z

On general comments:

Re: canonicalization, thanks. Working with the code like this makes more sense to me than it did before this, and hopefully other developers will have the same experience.

Single factory constructor for ModelElements is functionally correct but I am thinking of restructuring it so that it's possible to construct elements of a known type without harmful consequences. I couldn't think of a way to do this quickly that was actually clearer (or small) so put that off for now. This is definitely better than before; in general I want less constructory logic outside of the classes we're instantiating and there's more to do there with Package in particular.

Yes on asserts; for a while I didn't even realize Dart asserts weren't valid outside of checked mode because IntelliJ was helpfully defaulting to having that on. Eventually I want all asserts to be errors, possibly fatal ones.

jcollins-g · 2017-04-11T17:12:13Z

As for 1.0, there are a few things I'd like to nail down before that for correctness. Created dartdoc 1.0 to track it.

keertip

Some initial comments, review in progress. Like the direction of the PR, great to see dartdoc get some tlc.

keertip · 2017-04-11T20:22:53Z

bin/dartdoc.dart

@@ -41,7 +41,7 @@ main(List<String> arguments) async {

  Directory sdkDir = getSdkDir();
  if (sdkDir == null) {
-    print("Error: unable to locate the Dart SDK.");
+    stderr.write(" Error: unable to locate the Dart SDK.");


keertip · 2017-04-11T20:26:43Z

lib/dartdoc.dart

-    package.allModelElements.forEach((m) => m.documentation);
+    // TODO(jcollins-g): move index building into a cached-on-demand generation
+    // like most other bits in [Package].
+    package.allCanonicalModelElements.forEach((m) => m.documentation);


Eliminates repeats? Were we looking at documentation for elements that were not documented?

keertip · 2017-04-11T20:51:06Z

lib/dartdoc.dart

@@ -279,13 +286,40 @@ class DartDoc {
    // Use the includeExternals.
    for (Source source in context.librarySources) {
      LibraryElement library = context.computeLibraryElement(source);
+      if (library == null) {


Are you seeing use cases when the analyzer returns null for LibraryElement . Pretty sure it should not do so. Could you check with @bwilkerson on this?

Poked at this and I'm pretty sure this was an intermediate debugging step that got forgotten. Ran through my cases without it and saw no issues.

keertip · 2017-04-11T20:57:32Z

lib/src/html/html_generator_instance.dart

@@ -95,54 +97,81 @@ class HtmlGeneratorInstance implements HtmlOptions {
      generateLibrary(package, lib);

      for (var clazz in lib.allClasses) {
+        // TODO(jcollins-g): consider refactor so that only the canonical
+        // ModelElements show up in these lists
+        if (!clazz.isCanonical) continue;


So essentially there is a always one library that a class is canonical to (not sure if I got the lingo right here) and that's the only place it should be documented? If a class is not canonical, don't document it here, as it will be documented in the canonical library.

jcollins-g · 2017-04-12T15:18:49Z

I seem to be unable to reply to comments once they are "out of date" with github's interface. But, @devoncarew, I believe all your comments are addressed. resource is 1.1.0 in the bleeding edge SDK.

Working on @keertip's now.

devoncarew · 2017-04-12T17:36:25Z

I seem to be unable to reply to comments once they are "out of date" with github's interface.

You should be able to; perhaps it's not easy to reply to the review header?

I believe all your comments are addressed. resource is 1.1.0 in the bleeding edge SDK.

lgtm for my part! It'll be great to have these fixes.

keertip

The changes look good. Like the canonicalization changes, the logic makes sense, especially with the flutter use case. Looked through the changes in model.dart, nothing really stood out as a no no. The ones that I had some doubts about already had a todo, or else there was documentation!

Dartdoc needed this overhaul, as it has been in maintenance mode for a very long time and has organically grown to add support for the more complex use cases. Good to see this happen. Hoping to see a 1.0 release soon!

keertip · 2017-04-13T00:13:22Z

lib/src/model.dart

@@ -143,7 +195,7 @@ class Accessor extends ModelElement
            ? t.getGetter(element.name)
            : t.getSetter(element.name);
        if (accessor != null) {
-          return new Accessor(accessor, library);
+          return new ModelElement.from(accessor, library);


👍 Great to have just one way of creating ModelElement

keertip · 2017-04-13T00:29:09Z

lib/src/model.dart

    }

    var classContent = isDeprecated ? 'class="deprecated" ' : '';
    return '<a ${classContent}href="${href}">$name</a>';
  }

  // TODO(keertip): consolidate all the find library methods
+  // This differs from package.findOrCreateLibraryFor in a small way,


Is the TODO still relevant? Also maybe change the names of the methods so that they don't sound so similar.

I think the TODO is still relevant; the way these functions are reusing parts of each other's functionality suggests that the lines are wrong between them, even if it isn't obvious yet exactly how to draw them.

Renamed this method and tweaked comments in a couple related places to improve clarity.

keertip · 2017-04-13T15:12:35Z

lib/src/model.dart

+  /// clean that up.
+  factory ModelElement.from(Element e, Library library,
+      {Class enclosingClass}) {
+    Tuple3<Element, Library, Class> key =


Yes, this is what was needed. Knew that dartdoc was likely creating ModelElements for the same element multiple times, but this was just a todo, until now!

Yep. It turns out you just can't build things like canonicalization or most of the other things we'd like to do (like machine-readable public API descriptions) without this.

jcollins-g added 30 commits March 27, 2017 14:12

fix key for _implementors, stop overwriting files all the time

47966ee

Intermediate attempt to resolve duplicates and add some debugging.

3bc180e

Implemented by and a few other things still broken, but closer.

3016e8b

Fixed Implemented By again with a refactor. Still need to condense ca…

a7a9ce6

…nonicalization where possible

Some duplicate libraries or something going on here.

6154a18

Things are working better now. Edge cases with fields and dart:_inter…

687cf72

…nal seem fixed

Field inheritance still broken for getters/setters. May have to fix t…

5c03e6d

…his for real.

Fix up brokenness from merges

531a559

fix key for _implementors, stop overwriting files all the time

add09d1

Intermediate attempt to resolve duplicates and add some debugging.

692b727

Implemented by and a few other things still broken, but closer.

9d4ebf9

Fixed Implemented By again with a refactor. Still need to condense ca…

ab1da77

…nonicalization where possible

Some duplicate libraries or something going on here.

ed7af68

Things are working better now. Edge cases with fields and dart:_inter…

c9dc37f

…nal seem fixed

Field inheritance still broken for getters/setters. May have to fix t…

a8e7a18

…his for real.

Fix up brokenness from merges

8a80a18

Another incremental step; field inheritance canonicalization seems co…

8edf848

…rrect now

merge fixups

aecb075

Fix enclosingCanonicalElement detection on deep inheritance trees, up…

34b0645

…date test docs

More bugs fixed with a new inheritance chain builder in Class.inherit…

06f65ba

…anceChain

Generating test package docs with no broken links and everything seem…

da0b42a

…s to work here at least

all the tests pass. but we still can't build SDK docs.

55b6c8a

Down to one broken sdk link

8cb927e

Reintroduce workaround for missing names in some instances

88a5bc2

Fix linkage for class members

167bffd

Merge branch 'master' of github.com:dart-lang/dartdoc into 1334-imple…

1a5c085

…mentors-and-file-overwriting

Display warnings and re-fix Implemented By tracking

91bfdbe

Clean up generated test package docs. New semantics for --auto-includ…

f31199a

…e-deps had us throwing half the SDK docs into there.

And add back all the missing hashCode, etc now that we're hiding the …

c3b1ee9

…SDK again.

dartfmt

189b069

jcollins-g commented Apr 7, 2017

View reviewed changes

jcollins-g added 2 commits April 10, 2017 07:31

Remove breakpoints

c79c0e7

Update changelog to reflect additional bugs this fixes

6cdf4f6

devoncarew requested review from devoncarew and keertip April 10, 2017 15:46

devoncarew reviewed Apr 11, 2017

View reviewed changes

jcollins-g added 2 commits April 11, 2017 09:40

First wave review comments

4179bf9

dartfmt

849a3ec

jcollins-g mentioned this pull request Apr 11, 2017

link to identifiers in sdk on api.dartlang.org from third-party package #739

Closed

keertip reviewed Apr 11, 2017

View reviewed changes

Drop null check that can't happen for library

3d0ee6e

jcollins-g mentioned this pull request Apr 12, 2017

Change the order of groups on dartdoc pages #1323

Closed

keertip approved these changes Apr 13, 2017

View reviewed changes

jcollins-g mentioned this pull request Apr 13, 2017

Nameless members on Flutter BlockBody docs #1367

Closed

Rename one of the library find methods for clarity, add some comments

edd9f07

jcollins-g merged commit 47b60b4 into master Apr 13, 2017

jcollins-g deleted the 1334-implementors-and-file-overwriting branch April 13, 2017 20:58

jcollins-g mentioned this pull request Apr 24, 2017

Many broken links in flutter post 0.10.0 #1380

Closed

jcollins-g mentioned this pull request May 1, 2017

Canonicalization overhaul part two #1390

Merged

jcollins-g mentioned this pull request May 17, 2017

macro implementation doesn't guarantee detection of templates before attempting to apply them #1429

Closed

jcollins-g mentioned this pull request Jun 13, 2017

Canonicalization scorer #1455

Merged

jcollins-g mentioned this pull request Oct 18, 2017

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

Merged

parlough mentioned this pull request Feb 2, 2021

Fix most current canonical score lints #2496

Merged

Canonicalization overhaul #1368

Canonicalization overhaul #1368

Conversation

jcollins-g commented Apr 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devoncarew commented Apr 10, 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

Choose a reason for hiding this comment

devoncarew Apr 11, 2017 • edited by jcollins-g Loading

Choose a reason for hiding this comment

devoncarew Apr 11, 2017 • edited by jcollins-g 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

jcollins-g commented Apr 11, 2017

jcollins-g commented Apr 11, 2017

jcollins-g commented Apr 11, 2017

keertip 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

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 Apr 12, 2017

devoncarew commented Apr 12, 2017

keertip 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devoncarew Apr 11, 2017 •

edited by jcollins-g

Loading

devoncarew Apr 11, 2017 •

edited by jcollins-g

Loading