Full typedef support in dartdoc.

Full typedef support in dartdoc.

Committed: https://code.google.com/p/dart/source/detail?r=11024

[Johnni Winther, 2012-08-17 13:54:27]

https://chromiumcodereview.appspot.com/10823390/diff/2001/tests/compiler/dart...
File tests/compiler/dart2js/mirrors_test.dart (right):

https://chromiumcodereview.appspot.com/10823390/diff/2001/tests/compiler/dart...
tests/compiler/dart2js/mirrors_test.dart:32: var compilation = new
Compilation.library([inputPath], libPath);
This will make the path used by dartdoc tested within unittests. Hopefully we'll
then catch dartdoc breaks earlier.

[Johnni Winther, 2012-08-21 07:57:54]

PTAL

[Lasse Reichstein Nielsen, 2012-08-21 08:22:47]

lgtm

[ahe, 2012-08-21 08:50:28]

lgtm

http://codereview.chromium.org/10823390/diff/2001/pkg/dartdoc/dartdoc.dart
File pkg/dartdoc/dartdoc.dart (right):

http://codereview.chromium.org/10823390/diff/2001/pkg/dartdoc/dartdoc.dart#ne...
pkg/dartdoc/dartdoc.dart:795: assert(type is TypedefMirror);
This is one of the many places where an assertion is inferior.

If dart2js adds another kind of type, dartdoc simply ignores it in production
mode and crashes with an assertion error in developer mode.

It would be much better to do something like this:

} else if (type is TypedefMirror) {
...
} else {
throw new StateError("internal error: unknown type $type.");

http://codereview.chromium.org/10823390/diff/2001/pkg/dartdoc/mirrors/dart2js...
File pkg/dartdoc/mirrors/dart2js_mirror.dart (right):

http://codereview.chromium.org/10823390/diff/2001/pkg/dartdoc/mirrors/dart2js...
pkg/dartdoc/mirrors/dart2js_mirror.dart:852: _typedef.unalias(system.compiler),
Why do you unalias here?

http://codereview.chromium.org/10823390/diff/2001/tests/compiler/dart2js/mirr...
File tests/compiler/dart2js/mirrors_test.dart (right):

http://codereview.chromium.org/10823390/diff/2001/tests/compiler/dart2js/mirr...
tests/compiler/dart2js/mirrors_test.dart:32: var compilation = new
Compilation.library([inputPath], libPath);
On 2012/08/17 13:54:27, Johnni Winther wrote:
> This will make the path used by dartdoc tested within unittests. Hopefully
we'll
> then catch dartdoc breaks earlier.

I was surprised that you didn't land this change by itself.

http://codereview.chromium.org/10823390/diff/2001/tests/compiler/dart2js/mirr...
tests/compiler/dart2js/mirrors_test.dart:557:
Expect.isNotNull(funcTypedefTypeVariables, "Type variables is null");
The String argument to isNotNull is virtually pointless in this case.

http://codereview.chromium.org/10823390/diff/2001/tests/compiler/dart2js/mirr...
tests/compiler/dart2js/mirrors_test.dart:559: "Unexpected type variable count");
Ditto.

http://codereview.chromium.org/10823390/diff/2001/tests/compiler/dart2js/mirr...
tests/compiler/dart2js/mirrors_test.dart:562:
Expect.isNotNull(funcTypedefDefinition, "Typedef definition is null");
Ditto.

http://codereview.chromium.org/10823390/diff/2001/tests/compiler/dart2js/mirr...
tests/compiler/dart2js/mirrors_test.dart:564: "Unexpected typedef definition");
Ditto.

[Johnni Winther, 2012-08-21 09:30:27]

https://chromiumcodereview.appspot.com/10823390/diff/2001/pkg/dartdoc/dartdoc...
File pkg/dartdoc/dartdoc.dart (right):

https://chromiumcodereview.appspot.com/10823390/diff/2001/pkg/dartdoc/dartdoc...
pkg/dartdoc/dartdoc.dart:795: assert(type is TypedefMirror);
On 2012/08/21 08:50:28, ahe wrote:
> This is one of the many places where an assertion is inferior.
> 
> If dart2js adds another kind of type, dartdoc simply ignores it in production
> mode and crashes with an assertion error in developer mode.
> 
> It would be much better to do something like this:
> 
> } else if (type is TypedefMirror) {
> ...
> } else {
> throw new StateError("internal error: unknown type $type.");

Done.

https://chromiumcodereview.appspot.com/10823390/diff/2001/pkg/dartdoc/mirrors...
File pkg/dartdoc/mirrors/dart2js_mirror.dart (right):

https://chromiumcodereview.appspot.com/10823390/diff/2001/pkg/dartdoc/mirrors...
pkg/dartdoc/mirrors/dart2js_mirror.dart:852: _typedef.unalias(system.compiler),
On 2012/08/21 08:50:28, ahe wrote:
> Why do you unalias here?

Duh! Should be _typedef.element.alias.

https://chromiumcodereview.appspot.com/10823390/diff/2001/tests/compiler/dart...
File tests/compiler/dart2js/mirrors_test.dart (right):

https://chromiumcodereview.appspot.com/10823390/diff/2001/tests/compiler/dart...
tests/compiler/dart2js/mirrors_test.dart:557:
Expect.isNotNull(funcTypedefTypeVariables, "Type variables is null");
On 2012/08/21 08:50:28, ahe wrote:
> The String argument to isNotNull is virtually pointless in this case.

Done.

https://chromiumcodereview.appspot.com/10823390/diff/2001/tests/compiler/dart...
tests/compiler/dart2js/mirrors_test.dart:559: "Unexpected type variable count");
On 2012/08/21 08:50:28, ahe wrote:
> Ditto.

Done.

https://chromiumcodereview.appspot.com/10823390/diff/2001/tests/compiler/dart...
tests/compiler/dart2js/mirrors_test.dart:562:
Expect.isNotNull(funcTypedefDefinition, "Typedef definition is null");
On 2012/08/21 08:50:28, ahe wrote:
> Ditto.

Done.

https://chromiumcodereview.appspot.com/10823390/diff/2001/tests/compiler/dart...
tests/compiler/dart2js/mirrors_test.dart:564: "Unexpected typedef definition");
On 2012/08/21 08:50:28, ahe wrote:
> Ditto.

Done.

[ahe, 2012-08-21 10:17:27]

Still LGTM