Align source mirrors with runtime mirrors.

Align source mirrors with runtime mirrors.

BUG=
R=floitsch@google.com

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

[floitsch, 2014-01-28 19:21:52]

FYI.
I still have to get a better feeling of the overall change (the big picture).
Will try to do this tomorrow.

https://codereview.chromium.org/119913002/diff/20001/pkg/docgen/lib/docgen.dart
File pkg/docgen/lib/docgen.dart (right):

https://codereview.chromium.org/119913002/diff/20001/pkg/docgen/lib/docgen.da...
pkg/docgen/lib/docgen.dart:679: // a filter. Issue(#9590).
Either rewrite TODO, or just do the switch.

https://codereview.chromium.org/119913002/diff/20001/pkg/docgen/lib/docgen.da...
pkg/docgen/lib/docgen.dart:750: /// Returns a map of [Generic] objects
constructed from the class mirror.
from the type mirror.

https://codereview.chromium.org/119913002/diff/20001/sdk/lib/_internal/compil...
File
sdk/lib/_internal/compiler/implementation/mirrors/dart2js_instance_mirrors.dart
(right):

https://codereview.chromium.org/119913002/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/mirrors/dart2js_instance_mirrors.dart:167:
List<String> get _list {
Why not call this one "keys" ?

https://codereview.chromium.org/119913002/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/mirrors/dart2js_instance_mirrors.dart:169:
_listCache = new List<String>(_constant.keys.entries.length);
nit: I would prefer to use a temporary variable here.

https://codereview.chromium.org/119913002/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/mirrors/dart2js_instance_mirrors.dart:182:
Iterable<String> get keys {
nit: one line with =>

https://codereview.chromium.org/119913002/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/mirrors/dart2js_instance_mirrors.dart:241:
with ObjectMirrorMixin, InstanceMirrorMixin
indent by 4.

https://codereview.chromium.org/119913002/diff/20001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirrors.dart
(right):

https://codereview.chromium.org/119913002/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirrors.dart:55:
MethodMirror _convertElementMethodToMethodMirror(Dart2JsDeclarationMirror
library,
long line.

https://codereview.chromium.org/119913002/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirrors.dart:82: var
parent = owner;
You could also write:
if (owner != null) {
  Dart2JsDeclarationMirror parent = owner;
  return ...;
}
return _simpleNameString;

https://codereview.chromium.org/119913002/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirrors.dart:364: void
_ensureDeclarations() {
The function seems to be used only once.
Inline?

https://codereview.chromium.org/119913002/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirrors.dart:411:
Dart2JsDeclarationMirror container = library;
nit:
I spent a few minutes trying to understand why you needed a type-cast, before I
realized that you could overwrite the variable 2 lines below.

Dart2JsDeclarationMirror container;
if (element.getEnclosingClass() != null) {
  container = system ...;
} else {
  container = library;
}

https://codereview.chromium.org/119913002/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirrors.dart:423:
Iterable<DeclarationMirror> members =
fits on one line?

https://codereview.chromium.org/119913002/diff/20001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/mirrors/dart2js_type_mirrors.dart
(right):

https://codereview.chromium.org/119913002/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/mirrors/dart2js_type_mirrors.dart:127:
TypeSourceMirror createInstantiation(List<TypeSourceMirror> newTypeArguments) {
long line.

https://codereview.chromium.org/119913002/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/mirrors/dart2js_type_mirrors.dart:328:
FunctionType get _type => super._type;
can't you just remove the function?

https://codereview.chromium.org/119913002/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/mirrors/dart2js_type_mirrors.dart:418:
int get hashCode => 13 * _element.hashCode;
I'm not sure I understand.

operator==:
Afaict there is only one class that has isVoid => true. Therefore operator
==(other) could just be `other is Dart2JsVoidMirror`.

hashCode:
The operator== only looks at "other.isVoid". So I'm assuming that the voidType
(or at least its element) is always having the same hashCode. So this should be
a constant value. Couldn't we just return a random value here?

https://codereview.chromium.org/119913002/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/mirrors/dart2js_type_mirrors.dart:453:
int get hashCode => 13 * _element.hashCode;
same as for void type mirror.

https://codereview.chromium.org/119913002/diff/20001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/mirrors/mirrors_util.dart
(right):

https://codereview.chromium.org/119913002/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/mirrors/mirrors_util.dart:383: result
= mirror.declarations[id];
You are already returning from the middle of the second if-block.
Make it more explicit that you don't expect to reach the bottom of the function
by returning immediately here too.

if (mirror is LibraryMirror) return mirror.declarations[id];
if (mirror is MethodMirror) {
  return mirror.parameters.firstWhere(...);
}
// Try member lookup.
...

https://codereview.chromium.org/119913002/diff/20001/sdk/lib/_internal/dartdo...
File sdk/lib/_internal/dartdoc/lib/dartdoc.dart (right):

https://codereview.chromium.org/119913002/diff/20001/sdk/lib/_internal/dartdo...
sdk/lib/_internal/dartdoc/lib/dartdoc.dart:1502: } else if (member is
MethodMirror) {
nit.
I would prefer:
MethodMirror method = member;
...

https://codereview.chromium.org/119913002/diff/20001/sdk/lib/_internal/dartdo...
sdk/lib/_internal/dartdoc/lib/dartdoc.dart:1539: } else if (member is
VariableMirror) {
ditto.

https://codereview.chromium.org/119913002/diff/20001/sdk/lib/_internal/dartdo...
sdk/lib/_internal/dartdoc/lib/dartdoc.dart:1561: } else {
ditto.

https://codereview.chromium.org/119913002/diff/20001/sdk/lib/_internal/dartdo...
sdk/lib/_internal/dartdoc/lib/dartdoc.dart:1645: assert(false);
ditto.

https://codereview.chromium.org/119913002/diff/20001/tests/utils/source_mirro...
File tests/utils/source_mirrors_test.dart (right):

https://codereview.chromium.org/119913002/diff/20001/tests/utils/source_mirro...
tests/utils/source_mirrors_test.dart:1: // Copyright (c) 2012, the Dart project
authors.  Please see the AUTHORS file
update date.

https://codereview.chromium.org/119913002/diff/20001/tools/dom/docs/lib/docs....
File tools/dom/docs/lib/docs.dart (right):

https://codereview.chromium.org/119913002/diff/20001/tools/dom/docs/lib/docs....
tools/dom/docs/lib/docs.dart:106: print('  member: $memberMirror');
debug print?

[floitsch, 2014-01-28 19:54:54]

I tried to apply the CL to my local checkout, and it failed.
Could you please rebase.

[floitsch, 2014-01-30 18:18:19]

LGTM.

https://chromiumcodereview.appspot.com/119913002/diff/60001/sdk/lib/_internal...
File
sdk/lib/_internal/compiler/implementation/mirrors/dart2js_instance_mirrors.dart
(right):

https://chromiumcodereview.appspot.com/119913002/diff/60001/sdk/lib/_internal...
sdk/lib/_internal/compiler/implementation/mirrors/dart2js_instance_mirrors.dart:72:
final Dart2JsMirrorSystem mirrors;
Why do we call this "mirrors" and not "mirrorSystem" ?

https://chromiumcodereview.appspot.com/119913002/diff/60001/sdk/lib/_internal...
File
sdk/lib/_internal/compiler/implementation/mirrors/dart2js_member_mirrors.dart
(right):

https://chromiumcodereview.appspot.com/119913002/diff/60001/sdk/lib/_internal...
sdk/lib/_internal/compiler/implementation/mirrors/dart2js_member_mirrors.dart:105:
} else if (realName == 'unary-') {
It seems hackish to have string constants here.

https://chromiumcodereview.appspot.com/119913002/diff/60001/sdk/lib/_internal...
File sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirrors.dart
(right):

https://chromiumcodereview.appspot.com/119913002/diff/60001/sdk/lib/_internal...
sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirrors.dart:418: var
method = _convertElementMethodToMethodMirror(
Why the change (removing the type "MethodMirror"). Afaict
_convertElementMethodToMethodMirror always returns a Dart2JsMethodMirror.

[Emily Fortuna, 2014-01-30 19:33:10]

This is an epic CL, Johnni! 
Thank you for all of your hard work!

https://chromiumcodereview.appspot.com/119913002/diff/60001/pkg/docgen/lib/do...
File pkg/docgen/lib/docgen.dart (right):

https://chromiumcodereview.appspot.com/119913002/diff/60001/pkg/docgen/lib/do...
pkg/docgen/lib/docgen.dart:127:
mirrorToDocgen[dart2js_util.qualifiedNameOf(mirror)];
I liked that qualified name was a property on the mirror. what's the rationale
for this change?

https://chromiumcodereview.appspot.com/119913002/diff/60001/pkg/docgen/lib/do...
pkg/docgen/lib/docgen.dart:1124: var exportedClasses =
_addAll(exported['classes'],
perhaps instead exportedClasses could just be an Iterable, and then we can look
up the name on an as-needed basis instead of having the _addAll helper function.

https://chromiumcodereview.appspot.com/119913002/diff/60001/pkg/docgen/lib/do...
pkg/docgen/lib/docgen.dart:1792: return
'${owner.docName}.${dart2js_util.nameOf(mirror.owner)}-${dart2js_util.nameOf(mirror)}';
80 char

https://chromiumcodereview.appspot.com/119913002/diff/60001/sdk/lib/_internal...
File sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirrors.dart
(right):

https://chromiumcodereview.appspot.com/119913002/diff/60001/sdk/lib/_internal...
sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirrors.dart:55:
MethodMirror _convertElementMethodToMethodMirror(Dart2JsDeclarationMirror
library,
80 char. :-P

https://chromiumcodereview.appspot.com/119913002/diff/60001/sdk/lib/_internal...
File sdk/lib/_internal/compiler/implementation/mirrors/mirrors_util.dart
(right):

https://chromiumcodereview.appspot.com/119913002/diff/60001/sdk/lib/_internal...
sdk/lib/_internal/compiler/implementation/mirrors/mirrors_util.dart:15: String
nameOf(DeclarationMirror mirror) =>
I'd like to cast a vote in favor of the old way we had accessing these. I liked
the properties -- it seemed less verbose.

[Johnni Winther, 2014-01-31 14:16:44]

Committed patchset #4 manually as r32195 (presubmit successful).

[Emily Fortuna, 2014-01-31 21:53:48]

I still have a few questions about the API I'm hoping to hear back about...

https://chromiumcodereview.appspot.com/119913002/diff/60001/pkg/docgen/lib/do...
File pkg/docgen/lib/docgen.dart (right):

https://chromiumcodereview.appspot.com/119913002/diff/60001/pkg/docgen/lib/do...
pkg/docgen/lib/docgen.dart:127:
mirrorToDocgen[dart2js_util.qualifiedNameOf(mirror)];
On 2014/01/30 19:33:10, Emily Fortuna wrote:
> I liked that qualified name was a property on the mirror. what's the rationale
> for this change?

Johnni, can you please explain this new API?

https://chromiumcodereview.appspot.com/119913002/diff/60001/pkg/docgen/lib/do...
pkg/docgen/lib/docgen.dart:1124: var exportedClasses =
_addAll(exported['classes'],
On 2014/01/30 19:33:10, Emily Fortuna wrote:
> perhaps instead exportedClasses could just be an Iterable, and then we can
look
> up the name on an as-needed basis instead of having the _addAll helper
function.

bump?

[Johnni Winther, 2014-02-03 08:22:48]

Forgot to publish my comments.

@efortuna: Ping me again if the answers aren't satifactory.

https://codereview.chromium.org/119913002/diff/60001/pkg/docgen/lib/docgen.dart
File pkg/docgen/lib/docgen.dart (right):

https://codereview.chromium.org/119913002/diff/60001/pkg/docgen/lib/docgen.da...
pkg/docgen/lib/docgen.dart:127:
mirrorToDocgen[dart2js_util.qualifiedNameOf(mirror)];
On 2014/01/30 19:33:10, Emily Fortuna wrote:
> I liked that qualified name was a property on the mirror. what's the rationale
> for this change?

The [qualifiedName] property is still there but the type has changed from String
to Symbol. The [qualifiedNameOf] is a helper to get the String version.

Maybe you can do with using [Symbol] for keys instead of [String] but I made to
conversion to ensure semantic equivalence.

https://codereview.chromium.org/119913002/diff/60001/pkg/docgen/lib/docgen.da...
pkg/docgen/lib/docgen.dart:1124: var exportedClasses =
_addAll(exported['classes'],
On 2014/01/30 19:33:10, Emily Fortuna wrote:
> perhaps instead exportedClasses could just be an Iterable, and then we can
look
> up the name on an as-needed basis instead of having the _addAll helper
function.

Sounds like a good I idea. I'll leave that up to you, though.

https://codereview.chromium.org/119913002/diff/60001/pkg/docgen/lib/docgen.da...
pkg/docgen/lib/docgen.dart:1792: return
'${owner.docName}.${dart2js_util.nameOf(mirror.owner)}-${dart2js_util.nameOf(mirror)}';
On 2014/01/30 19:33:10, Emily Fortuna wrote:
> 80 char

Done.

https://codereview.chromium.org/119913002/diff/60001/sdk/lib/_internal/compil...
File
sdk/lib/_internal/compiler/implementation/mirrors/dart2js_instance_mirrors.dart
(right):

https://codereview.chromium.org/119913002/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/mirrors/dart2js_instance_mirrors.dart:72:
final Dart2JsMirrorSystem mirrors;
On 2014/01/30 18:18:20, floitsch wrote:
> Why do we call this "mirrors" and not "mirrorSystem" ?

No good reason. Changed.

https://codereview.chromium.org/119913002/diff/60001/sdk/lib/_internal/compil...
File
sdk/lib/_internal/compiler/implementation/mirrors/dart2js_member_mirrors.dart
(right):

https://codereview.chromium.org/119913002/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/mirrors/dart2js_member_mirrors.dart:105:
} else if (realName == 'unary-') {
On 2014/01/30 18:18:20, floitsch wrote:
> It seems hackish to have string constants here.

A general problem for this method. Added a TODO.

https://codereview.chromium.org/119913002/diff/60001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirrors.dart
(right):

https://codereview.chromium.org/119913002/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirrors.dart:55:
MethodMirror _convertElementMethodToMethodMirror(Dart2JsDeclarationMirror
library,
On 2014/01/30 19:33:10, Emily Fortuna wrote:
> 80 char. :-P

Done.

https://codereview.chromium.org/119913002/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirrors.dart:418: var
method = _convertElementMethodToMethodMirror(
On 2014/01/30 18:18:20, floitsch wrote:
> Why the change (removing the type "MethodMirror"). Afaict
> _convertElementMethodToMethodMirror always returns a Dart2JsMethodMirror.

Done.

https://codereview.chromium.org/119913002/diff/60001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/mirrors/mirrors_util.dart
(right):

https://codereview.chromium.org/119913002/diff/60001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/mirrors/mirrors_util.dart:15: String
nameOf(DeclarationMirror mirror) =>
On 2014/01/30 19:33:10, Emily Fortuna wrote:
> I'd like to cast a vote in favor of the old way we had accessing these. I
liked
> the properties -- it seemed less verbose.

I comes from the change to use Symbol which doesn't fit with docgen/dartdoc that
needs strings. I added these helper methods because we have a problem with
symbols of private names and I wanted to all source mirror usages statically
visible.