Implement reflective access to static methods.

Implement reflective access to static methods.

BUG=http://dartbug.com/11745
R=ngeoffray@google.com

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

[ngeoffray, 2013-07-17 07:35:57]

https://codereview.chromium.org/19391004/diff/1/dart/sdk/lib/_internal/compil...
File dart/sdk/lib/_internal/compiler/implementation/js/nodes.dart (right):

https://codereview.chromium.org/19391004/diff/1/dart/sdk/lib/_internal/compil...
dart/sdk/lib/_internal/compiler/implementation/js/nodes.dart:521: /// Wrap a
CodeBuffer as an expression.
This feels very workaround-ish. Add a TODO that this should eventually be
removed?

https://codereview.chromium.org/19391004/diff/1/dart/sdk/lib/_internal/compil...
File dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart
(right):

https://codereview.chromium.org/19391004/diff/1/dart/sdk/lib/_internal/compil...
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:142: *
Later, this code is emitted when the class or library is emitted.
Would there be a way to just produce them when we produce the class?

https://codereview.chromium.org/19391004/diff/1/dart/sdk/lib/_internal/compil...
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:3505: for
(var b in elementBuffers[element]) {
Add a TODO to remove this once you implement it for non-instantiated classes and
native classes?

https://codereview.chromium.org/19391004/diff/1/dart/sdk/lib/_internal/compil...
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:3551:
compiler.internalError('Pending statics (see above).');
Is that an internal error, or a not yet implemented error? I'd prefer using the
latter to avoid confusing people in the team seeing a crash report.

https://codereview.chromium.org/19391004/diff/1/dart/sdk/lib/_internal/compil...
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:3614: if
(owner == null) {
This looks too defensive.

[ahe, 2013-07-17 07:55:27]

Nicolas, please review my replies and let me know if you disagree.

https://codereview.chromium.org/19391004/diff/1/dart/sdk/lib/_internal/compil...
File dart/sdk/lib/_internal/compiler/implementation/js/nodes.dart (right):

https://codereview.chromium.org/19391004/diff/1/dart/sdk/lib/_internal/compil...
dart/sdk/lib/_internal/compiler/implementation/js/nodes.dart:521: /// Wrap a
CodeBuffer as an expression.
On 2013/07/17 07:35:57, ngeoffray wrote:
> This feels very workaround-ish. Add a TODO that this should eventually be
> removed?

Agreed.  The intent is to make it easier to migrate to full AST based solution.

https://codereview.chromium.org/19391004/diff/1/dart/sdk/lib/_internal/compil...
File dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart
(right):

https://codereview.chromium.org/19391004/diff/1/dart/sdk/lib/_internal/compil...
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:142: *
Later, this code is emitted when the class or library is emitted.
On 2013/07/17 07:35:57, ngeoffray wrote:
> Would there be a way to just produce them when we produce the class?

Yes. I hope to clean this up.

However, there are, AFAIK, some important ordering issues related to bound
closures. So AFAIK, this isn't trivial.

I'll add a TODO.

https://codereview.chromium.org/19391004/diff/1/dart/sdk/lib/_internal/compil...
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:3505: for
(var b in elementBuffers[element]) {
On 2013/07/17 07:35:57, ngeoffray wrote:
> Add a TODO to remove this once you implement it for non-instantiated classes
and
> native classes?

I'm not sure this code would go away. Consider it an assertion.

https://codereview.chromium.org/19391004/diff/1/dart/sdk/lib/_internal/compil...
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:3551:
compiler.internalError('Pending statics (see above).');
On 2013/07/17 07:35:57, ngeoffray wrote:
> Is that an internal error, or a not yet implemented error? I'd prefer using
the
> latter to avoid confusing people in the team seeing a crash report.

It is an internal error.

If a static is "pending" it means that no code is generated for it.  This is an
error as we have not been able to eliminate the method using tree shaking, which
means that it might be called.

https://codereview.chromium.org/19391004/diff/1/dart/sdk/lib/_internal/compil...
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:3614: if
(owner == null) {
On 2013/07/17 07:35:57, ngeoffray wrote:
> This looks too defensive.

At first sight, yes. But notice that if we don't check for null, we simply store
null in elementBuffers.

When I was debugging, it wasn't clear if that was the problem I had, so I wanted
to be sure. I prefer to keep the check.

[karlklose, 2013-07-17 08:01:40]

DBC:

https://codereview.chromium.org/19391004/diff/1/dart/sdk/lib/_internal/compil...
File dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart
(right):

https://codereview.chromium.org/19391004/diff/1/dart/sdk/lib/_internal/compil...
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:3501: var
sortedElements = Elements.sortedByPosition(elementBuffers.keys);
Type this variable (and also b in l. 3505)?

https://codereview.chromium.org/19391004/diff/1/dart/sdk/lib/_internal/compil...
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:3615:
throw 'Bad owner of $element';
Could you call internalError instead?

[ngeoffray, 2013-07-17 08:08:42]

LGTM

https://codereview.chromium.org/19391004/diff/1/dart/sdk/lib/_internal/compil...
File dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart
(right):

https://codereview.chromium.org/19391004/diff/1/dart/sdk/lib/_internal/compil...
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:142: *
Later, this code is emitted when the class or library is emitted.
On 2013/07/17 07:55:27, ahe wrote:
> On 2013/07/17 07:35:57, ngeoffray wrote:
> > Would there be a way to just produce them when we produce the class?
> 
> Yes. I hope to clean this up.
> 
> However, there are, AFAIK, some important ordering issues related to bound
> closures. So AFAIK, this isn't trivial.
> 
> I'll add a TODO.

I agree it's not trivial.

https://codereview.chromium.org/19391004/diff/1/dart/sdk/lib/_internal/compil...
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:3505: for
(var b in elementBuffers[element]) {
On 2013/07/17 07:55:27, ahe wrote:
> On 2013/07/17 07:35:57, ngeoffray wrote:
> > Add a TODO to remove this once you implement it for non-instantiated classes
> and
> > native classes?
> 
> I'm not sure this code would go away. Consider it an assertion.

OK. So after reading the code again, I realize I misunderstood something. I
thought this was a way for you to know which static was not correctly
implemented (because it's on a native class, abstract, ....). It actually isn't.

So this check feels like a validation thing that we could do after doing the
emission. Would it make sense to add a validate method after emission that would
a) Make sure the internal data structure in the emitter make sense (like you're
doing here)
b) potentially do a run on the CodeBuffer to ensure it's valid syntax (but
that's long term).

[ahe, 2013-07-17 08:31:24]

Thank you, Karl and Nicolas!

https://codereview.chromium.org/19391004/diff/1/dart/sdk/lib/_internal/compil...
File dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart
(right):

https://codereview.chromium.org/19391004/diff/1/dart/sdk/lib/_internal/compil...
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:3501: var
sortedElements = Elements.sortedByPosition(elementBuffers.keys);
On 2013/07/17 08:01:40, karlklose wrote:
> Type this variable (and also b in l. 3505)?

Done.

https://codereview.chromium.org/19391004/diff/1/dart/sdk/lib/_internal/compil...
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:3505: for
(var b in elementBuffers[element]) {
On 2013/07/17 08:08:42, ngeoffray wrote:
> On 2013/07/17 07:55:27, ahe wrote:
> > On 2013/07/17 07:35:57, ngeoffray wrote:
> > > Add a TODO to remove this once you implement it for non-instantiated
classes
> > and
> > > native classes?
> > 
> > I'm not sure this code would go away. Consider it an assertion.
> 
> OK. So after reading the code again, I realize I misunderstood something. I
> thought this was a way for you to know which static was not correctly
> implemented (because it's on a native class, abstract, ....). It actually
isn't.
> 
> So this check feels like a validation thing that we could do after doing the
> emission. Would it make sense to add a validate method after emission that
would
> a) Make sure the internal data structure in the emitter make sense (like
you're
> doing here)
> b) potentially do a run on the CodeBuffer to ensure it's valid syntax (but
> that's long term).

If feel that would be easier to accomplish once I switch to generating statics
as part of the class as you suggested above at line 142.

https://codereview.chromium.org/19391004/diff/1/dart/sdk/lib/_internal/compil...
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:3615:
throw 'Bad owner of $element';
On 2013/07/17 08:01:40, karlklose wrote:
> Could you call internalError instead?

Done.

[ahe, 2013-07-17 08:33:41]

Committed patchset #2 manually as r25085 (presubmit successful).