Implement topLevelMembers, staticMembers, instanceMembers in the VM.

Implement topLevelMembers, staticMembers, instanceMembers in the VM.

BUG=http://dartbug.com/14632
R=asiva@google.com, gbracha@google.com

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

[gbracha, 2013-11-12 21:01:45]

tests lgtm

[rmacnak, 2013-11-13 02:08:01]

Added a test for the properties of the synthetic members.

[gbracha, 2013-11-14 19:21:20]

lgtm with comments

https://codereview.chromium.org/67203002/diff/200001/sdk/lib/mirrors/mirrors....
File sdk/lib/mirrors/mirrors.dart (right):

https://codereview.chromium.org/67203002/diff/200001/sdk/lib/mirrors/mirrors....
sdk/lib/mirrors/mirrors.dart:919: */
Returns true if the reflectee is synthetic, and returns false otherwise.

A reflectee is synthetic if it is its a getter or setter implicitly introduced
for a field or if it is a constructor that was implicitly introduced as a
default constructor or as part of a mixin application.

https://codereview.chromium.org/67203002/diff/200001/tests/lib/mirrors/synthe...
File tests/lib/mirrors/synthetic_accessor_properties_test.dart (right):

https://codereview.chromium.org/67203002/diff/200001/tests/lib/mirrors/synthe...
tests/lib/mirrors/synthetic_accessor_properties_test.dart:100:
Expect.equals(reflectClass(C), mm.returnType);
Should be Type

[rmacnak, 2013-11-14 22:19:02]

https://codereview.chromium.org/67203002/diff/200001/sdk/lib/mirrors/mirrors....
File sdk/lib/mirrors/mirrors.dart (right):

https://codereview.chromium.org/67203002/diff/200001/sdk/lib/mirrors/mirrors....
sdk/lib/mirrors/mirrors.dart:919: */
On 2013/11/14 19:21:21, gbracha wrote:
> Returns true if the reflectee is synthetic, and returns false otherwise.
> 
> A reflectee is synthetic if it is its a getter or setter implicitly introduced
> for a field or if it is a constructor that was implicitly introduced as a
> default constructor or as part of a mixin application.

Done.

https://codereview.chromium.org/67203002/diff/200001/tests/lib/mirrors/synthe...
File tests/lib/mirrors/synthetic_accessor_properties_test.dart (right):

https://codereview.chromium.org/67203002/diff/200001/tests/lib/mirrors/synthe...
tests/lib/mirrors/synthetic_accessor_properties_test.dart:100:
Expect.equals(reflectClass(C), mm.returnType);
On 2013/11/14 19:21:21, gbracha wrote:
> Should be Type

Done.

[rmacnak, 2013-11-26 17:59:43]

Resync'd. +Siva

[gbracha, 2013-11-26 18:35:14]

Still lgtm with one more tweak to dartdoc (see comment)

https://codereview.chromium.org/67203002/diff/300001/sdk/lib/mirrors/mirrors....
File sdk/lib/mirrors/mirrors.dart (right):

https://codereview.chromium.org/67203002/diff/300001/sdk/lib/mirrors/mirrors....
sdk/lib/mirrors/mirrors.dart:925: * for a field or if it is a constructor that
was implicitly introduced as a
or Type.

[rmacnak, 2013-11-26 19:27:07]

https://codereview.chromium.org/67203002/diff/300001/sdk/lib/mirrors/mirrors....
File sdk/lib/mirrors/mirrors.dart (right):

https://codereview.chromium.org/67203002/diff/300001/sdk/lib/mirrors/mirrors....
sdk/lib/mirrors/mirrors.dart:925: * for a field or if it is a constructor that
was implicitly introduced as a
On 2013/11/26 18:35:14, gbracha wrote:
> or Type.

Done.

[ahe, 2013-12-06 12:20:40]

I have some concerns about inventing synthetic getters for classes and typedefs.

https://codereview.chromium.org/67203002/diff/320001/runtime/lib/mirrors_impl...
File runtime/lib/mirrors_impl.dart (right):

https://codereview.chromium.org/67203002/diff/320001/runtime/lib/mirrors_impl...
runtime/lib/mirrors_impl.dart:203: String get source => '<synthetic code>';
I'm worried that returning illegal syntax leads to bugs. Can we return null or
throw an exception?

https://codereview.chromium.org/67203002/diff/320001/runtime/lib/mirrors_impl...
runtime/lib/mirrors_impl.dart:227: class _SyntheticTypeGetter extends
_SyntheticAccessor {
I want to make sure that we're building a mirror API for Dart, not Newspeak.

If I understand this correctly, this used to create a getter corresponding to a
typedef or a class. That's how Dart works.

https://codereview.chromium.org/67203002/diff/320001/sdk/lib/_internal/lib/js...
File sdk/lib/_internal/lib/js_mirrors.dart (right):

https://codereview.chromium.org/67203002/diff/320001/sdk/lib/_internal/lib/js...
sdk/lib/_internal/lib/js_mirrors.dart:1746: bool get isSynthetic => false;
Could you add a TODO here? I don't think this correct in all situations.

[rmacnak, 2013-12-06 19:41:35]

https://codereview.chromium.org/67203002/diff/320001/runtime/lib/mirrors_impl...
File runtime/lib/mirrors_impl.dart (right):

https://codereview.chromium.org/67203002/diff/320001/runtime/lib/mirrors_impl...
runtime/lib/mirrors_impl.dart:227: class _SyntheticTypeGetter extends
_SyntheticAccessor {
On 2013/12/06 12:20:40, ahe wrote:
> I want to make sure that we're building a mirror API for Dart, not Newspeak.
> 
> If I understand this correctly, this used to create a getter corresponding to
a
> typedef or a class. That's how Dart works.

Right, the intention of instance/static/topLevelMembers is to represent the API
of an object. In a top-level scope one can say SomeType and it behaves like the
library had a getter for the type. SomeType() even fails with a NSM on Type for
call(). The story holds for library prefixes as well. Currently
LibraryMirror.getField(#SomeType) doesn't answer the type, but consistency
suggests it should.

[rmacnak, 2013-12-06 22:14:04]

Removed topLevelMembers for later.

[rmacnak, 2013-12-07 02:09:05]

https://codereview.chromium.org/67203002/diff/320001/runtime/lib/mirrors_impl...
File runtime/lib/mirrors_impl.dart (right):

https://codereview.chromium.org/67203002/diff/320001/runtime/lib/mirrors_impl...
runtime/lib/mirrors_impl.dart:203: String get source => '<synthetic code>';
On 2013/12/06 12:20:40, ahe wrote:
> I'm worried that returning illegal syntax leads to bugs. Can we return null or
> throw an exception?

Done.

https://codereview.chromium.org/67203002/diff/320001/sdk/lib/_internal/lib/js...
File sdk/lib/_internal/lib/js_mirrors.dart (right):

https://codereview.chromium.org/67203002/diff/320001/sdk/lib/_internal/lib/js...
sdk/lib/_internal/lib/js_mirrors.dart:1746: bool get isSynthetic => false;
On 2013/12/06 12:20:40, ahe wrote:
> Could you add a TODO here? I don't think this correct in all situations.

Done.

[siva, 2013-12-11 00:54:43]

lgtm

https://chromiumcodereview.appspot.com/67203002/diff/360001/runtime/lib/mirro...
File runtime/lib/mirrors_impl.dart (right):

https://chromiumcodereview.appspot.com/67203002/diff/360001/runtime/lib/mirro...
runtime/lib/mirrors_impl.dart:579: return _cachedStaticMembers = result;
Would it be more readable to write this as
if (_cachedStaticMembers == null) {
  var result = ...
  ....
  ....
  _cachedStaticMembers = result;
}
return _cachedStaticMembers;

https://chromiumcodereview.appspot.com/67203002/diff/360001/runtime/lib/mirro...
runtime/lib/mirrors_impl.dart:605: return _cachedInstanceMembers = result;
Ditto.

https://chromiumcodereview.appspot.com/67203002/diff/360001/sdk/lib/_internal...
File sdk/lib/_internal/lib/js_mirrors.dart (right):

https://chromiumcodereview.appspot.com/67203002/diff/360001/sdk/lib/_internal...
sdk/lib/_internal/lib/js_mirrors.dart:1822: // TODO(ahe): This might not be true
for all cases.
Usually it is preferable to have an issue number in the TODO instead of an ldap,
ensures that it is tracked.

[rmacnak, 2013-12-11 01:17:10]

https://chromiumcodereview.appspot.com/67203002/diff/360001/runtime/lib/mirro...
File runtime/lib/mirrors_impl.dart (right):

https://chromiumcodereview.appspot.com/67203002/diff/360001/runtime/lib/mirro...
runtime/lib/mirrors_impl.dart:579: return _cachedStaticMembers = result;
On 2013/12/11 00:54:43, siva wrote:
> Would it be more readable to write this as
> if (_cachedStaticMembers == null) {
>   var result = ...
>   ....
>   ....
>   _cachedStaticMembers = result;
> }
> return _cachedStaticMembers;

It's about the same either way, but your suggestion is more consistent with the
rest of the library.

https://chromiumcodereview.appspot.com/67203002/diff/360001/sdk/lib/_internal...
File sdk/lib/_internal/lib/js_mirrors.dart (right):

https://chromiumcodereview.appspot.com/67203002/diff/360001/sdk/lib/_internal...
sdk/lib/_internal/lib/js_mirrors.dart:1822: // TODO(ahe): This might not be true
for all cases.
On 2013/12/11 00:54:43, siva wrote:
> Usually it is preferable to have an issue number in the TODO instead of an
ldap,
> ensures that it is tracked.

Added issue number from the failing tests.

[rmacnak, 2013-12-11 02:08:22]

Committed patchset #11 manually as r31056 (presubmit successful).