Implement newInstance in the transformer.

Implement newInstance in the transformer.

I implement named and optional arguments by copying the code generating the default values. This is not perfect.

This applies on top of https://codereview.chromium.org/1182083002

BUG=
R=eernst@google.com

Committed: https://github.com/dart-lang/reflectable/commit/4e682d506bd14c5ab5df3763639a717ba4ed0b75

[sigurdm, 2015-06-16 12:57:10]

Patchset #1 (id:1) has been deleted

[sigurdm, 2015-06-16 12:57:59]

sigurdm@google.com changed reviewers:
+ eernst@google.com

[eernst, 2015-06-18 13:35:41]

LGTM, along with a couple of remarks: It would be nice to get documentation for
the 'xString' methods where x is some mirror method name or similar; and I think
it is justified that constructors in abstract classes are omitted in
'memberStrings', but I tried to retrace the reason for this in order to see why;
finally, there seems to be two capabilities for the same concept.

https://codereview.chromium.org/1181413005/diff/20001/reflectable/lib/src/tra...
File reflectable/lib/src/transformer_implementation.dart (right):

https://codereview.chromium.org/1181413005/diff/20001/reflectable/lib/src/tra...
reflectable/lib/src/transformer_implementation.dart:213: String get
newInstanceString {
I've been looking for a general hint (some DartDoc comment, presumably) about
the general idea behind xString for various x. Is it a factorization that
includes both (1) obtaining various data from the Element model about the shape
of the target program, and also (2) producing a string that represents the same
semantics in generated code? I guess these two elements were often inlined in
code for larger tasks, so the factorization looks fine in itself.

On the other hand, it does look somewhat "impure" that the data extraction and
the string construction are packaged together. Would it be a meaningless further
factorization to express them separately? (I realize that it may well be a bit
inconvenient, because the two are quite intertwined.)

Also, it is tempting to think that some of the string construction tasks can be
obtained by using some 'prettyprint' feature from the Element world, if there is
such a thing. (Copying an excerpt from the original source would surely not be a
safe bet, because that might be formatted in ways that make no sense when moved
into an entirely different context).

Is there a natural place where we can add a few lines of text explaining the
situation?

https://codereview.chromium.org/1181413005/diff/20001/reflectable/lib/src/tra...
reflectable/lib/src/transformer_implementation.dart:239: }).join(", ");
    Maybe
  we can sell this style of
  indentation to the rest of the
world

rather
  than this boring
one?

https://codereview.chromium.org/1181413005/diff/20001/reflectable/lib/src/tra...
reflectable/lib/src/transformer_implementation.dart:935: }
This one would be new, but still related to the treatment of `abstract`
features.

But it's not obvious to me that `isAbstract` justifies omission.


An element.dart `ClassElement` represents a class C, so `newInstanceString`
would describe constructors producing instances of C, and this would make them
part of the "members strings" of the static mirror class under construction.

We should be able to obtain a class mirror for an abstract class, because users
can use reflectType to directly request such a mirror.

It is possible to invoke a constructor for an abstract class, but it's a runtime
error, Spec. 1.9, Sect. 16.12.1, so presumably it's OK to leave it as
'NoSuchCapability' rather than reproducing the original runtime error (didn't
check which one that is).

On the other hand, it would make sense to include constructors in abstract
classes for informational purposes (when using reflection to "browse the code"),
and such constructors are of course useful in general because they can factor
out common pieces of code which would otherwise occur in many duplicates in the
constructors of concrete subclasses.

Do we have a place where the constructors in abstract classes are available for
"browsing"? Would be ClassMirror.declarations (checking: yes, that's
_everything_, all regular (instance or static) methods, getters, setters,
fields, constructors, and even type variables).

OK: So we should actually drop constructors of abstract classes here.

https://codereview.chromium.org/1181413005/diff/20001/reflectable/lib/src/tra...
reflectable/lib/src/transformer_implementation.dart:1057: return
invokeConstructorsCapability;
I believe this is the same thing as the capability
NewInstanceCapability([RegExp]) and the instance newInstanceCapability. So we
need to eliminate the one that we end up not using.

(PS: `newInstanceCapability` was named this way because it controls the
availability of `newInstance` on mirrors, just like `metadataCapability`
controls the avail. of `metadata`, etc., so `invokeConstructor` may be more
intuitive, but `newInstance` is probably more consistent.)

https://codereview.chromium.org/1181413005/diff/20001/test_reflectable/.status
File test_reflectable/.status (right):

https://codereview.chromium.org/1181413005/diff/20001/test_reflectable/.statu...
test_reflectable/.status:3: test/new_instance_test: RuntimeError #
dartbug.com/19635
Maybe separate the tests into the cases that will work and the cases affected by
dartbug.com/19635, such that we can run as much as possible?

https://codereview.chromium.org/1181413005/diff/20001/test_reflectable/test/n...
File test_reflectable/test/new_instance_default_values_test.dart (right):

https://codereview.chromium.org/1181413005/diff/20001/test_reflectable/test/n...
test_reflectable/test/new_instance_default_values_test.dart:8: library
reflectable.test.to_be_transformed.new_instance_test;
Looks like the separation I suggested in .status has already happened, just
missing an update to this lib.name.

Actually, I thought we got rid of all occurrences of 'to_be_transformed' a long
time ago, when that directory ceased to exist.

[sigurdm, 2015-06-19 07:50:54]

I am not doing everything you asked in the comments (as detailed in my
responses), so PTAL at my responses before I land this.

https://codereview.chromium.org/1181413005/diff/20001/reflectable/lib/src/tra...
File reflectable/lib/src/transformer_implementation.dart (right):

https://codereview.chromium.org/1181413005/diff/20001/reflectable/lib/src/tra...
reflectable/lib/src/transformer_implementation.dart:213: String get
newInstanceString {
On 2015/06/18 13:35:41, eernst wrote:
> I've been looking for a general hint (some DartDoc comment, presumably) about
> the general idea behind xString for various x. Is it a factorization that
> includes both (1) obtaining various data from the Element model about the
shape
> of the target program, and also (2) producing a string that represents the
same
> semantics in generated code? I guess these two elements were often inlined in
> code for larger tasks, so the factorization looks fine in itself.

Your intuition is right. There is no exact defined semantics of the -String
methods, but something along those lines.

> On the other hand, it does look somewhat "impure" that the data extraction and
> the string construction are packaged together. Would it be a meaningless
further
> factorization to express them separately? (I realize that it may well be a bit
> inconvenient, because the two are quite intertwined.)
> 
> Also, it is tempting to think that some of the string construction tasks can
be
> obtained by using some 'prettyprint' feature from the Element world, if there
is
> such a thing. (Copying an excerpt from the original source would surely not be
a
> safe bet, because that might be formatted in ways that make no sense when
moved
> into an entirely different context).
> 
> Is there a natural place where we can add a few lines of text explaining the
> situation?
> 

Because I am going to refactor how we represent the data - I will leave this
uncommented for now, and try to document better how the new approach works.

https://codereview.chromium.org/1181413005/diff/20001/reflectable/lib/src/tra...
reflectable/lib/src/transformer_implementation.dart:239: }).join(", ");
On 2015/06/18 13:35:41, eernst wrote:
>     Maybe
>   we can sell this style of
>   indentation to the rest of the
> world
> 
> rather
>   than this boring
> one?

?

https://codereview.chromium.org/1181413005/diff/20001/reflectable/lib/src/tra...
reflectable/lib/src/transformer_implementation.dart:935: }
On 2015/06/18 13:35:41, eernst wrote:
> This one would be new, but still related to the treatment of `abstract`
> features.
> 
> But it's not obvious to me that `isAbstract` justifies omission.
> 
> 
> An element.dart `ClassElement` represents a class C, so `newInstanceString`
> would describe constructors producing instances of C, and this would make them
> part of the "members strings" of the static mirror class under construction.
> 
> We should be able to obtain a class mirror for an abstract class, because
users
> can use reflectType to directly request such a mirror.
> 
> It is possible to invoke a constructor for an abstract class, but it's a
runtime
> error, Spec. 1.9, Sect. 16.12.1, so presumably it's OK to leave it as
> 'NoSuchCapability' rather than reproducing the original runtime error (didn't
> check which one that is).
> 
> On the other hand, it would make sense to include constructors in abstract
> classes for informational purposes (when using reflection to "browse the
code"),
> and such constructors are of course useful in general because they can factor
> out common pieces of code which would otherwise occur in many duplicates in
the
> constructors of concrete subclasses.
> 
> Do we have a place where the constructors in abstract classes are available
for
> "browsing"? Would be ClassMirror.declarations (checking: yes, that's
> _everything_, all regular (instance or static) methods, getters, setters,
> fields, constructors, and even type variables).
> 
> OK: So we should actually drop constructors of abstract classes here.

You cannot invoke abstract cosntructors directly, therefore we do not generate a
custom newInstance method.

https://codereview.chromium.org/1181413005/diff/20001/reflectable/lib/src/tra...
reflectable/lib/src/transformer_implementation.dart:1057: return
invokeConstructorsCapability;
On 2015/06/18 13:35:41, eernst wrote:
> I believe this is the same thing as the capability
> NewInstanceCapability([RegExp]) and the instance newInstanceCapability. So we
> need to eliminate the one that we end up not using.
> 
> (PS: `newInstanceCapability` was named this way because it controls the
> availability of `newInstance` on mirrors, just like `metadataCapability`
> controls the avail. of `metadata`, etc., so `invokeConstructor` may be more
> intuitive, but `newInstance` is probably more consistent.)

This was based on the previous incantation of capabilities. Should be updated in
the rebase.

https://codereview.chromium.org/1181413005/diff/20001/test_reflectable/.status
File test_reflectable/.status (right):

https://codereview.chromium.org/1181413005/diff/20001/test_reflectable/.statu...
test_reflectable/.status:3: test/new_instance_test: RuntimeError #
dartbug.com/19635
On 2015/06/18 13:35:41, eernst wrote:
> Maybe separate the tests into the cases that will work and the cases affected
by
> dartbug.com/19635, such that we can run as much as possible?

I don't want to spend too much time on supporting the non-transformed program in
dart2js. The dart:mirror implementation is very limited, and I don't see any
payoff.

https://codereview.chromium.org/1181413005/diff/20001/test_reflectable/test/n...
File test_reflectable/test/new_instance_default_values_test.dart (right):

https://codereview.chromium.org/1181413005/diff/20001/test_reflectable/test/n...
test_reflectable/test/new_instance_default_values_test.dart:8: library
reflectable.test.to_be_transformed.new_instance_test;
On 2015/06/18 13:35:41, eernst wrote:
> Looks like the separation I suggested in .status has already happened, just
> missing an update to this lib.name.
No - this is an example of a default value that we do not handle in the
transform

Fixed the lib name.
> 
> Actually, I thought we got rid of all occurrences of 'to_be_transformed' a
long
> time ago, when that directory ceased to exist.

I think they lived on in the library names.
This can be changed in a seperate cl.

[eernst, 2015-06-19 09:22:21]

LGTM, with a couple of remarks about `newInstanceString`: Suggested small
adjustment of terminology, and commented on the 'faithful default values' issue.

https://codereview.chromium.org/1181413005/diff/60001/reflectable/lib/src/tra...
File reflectable/lib/src/transformer_implementation.dart (right):

https://codereview.chromium.org/1181413005/diff/60001/reflectable/lib/src/tra...
reflectable/lib/src/transformer_implementation.dart:256: int positionalCount =
type.normalParameterTypes.length;
I suspect that `normalParameterTypes` delivers information about the required
positional parameters (which is the phrase used by the spec), so maybe we should
use that terminology in order to disambiguate; maybe it's worth paying the
verbosity cost of something like `requiredPositionalCount` to make that
explicit?

https://codereview.chromium.org/1181413005/diff/60001/reflectable/lib/src/tra...
reflectable/lib/src/transformer_implementation.dart:257: int optionalCount =
type.optionalParameterTypes.length;
Similarly, since both named parameters and (some) positional parameters can be
optional, we might help readers of this code by using `optionalPositionalCount`
here.

https://codereview.chromium.org/1181413005/diff/60001/reflectable/lib/src/tra...
reflectable/lib/src/transformer_implementation.dart:266: defaultValueCode ==
null ? "" : " = $defaultValueCode";
Wouldn't it be better to have the '.. faithfully' TODO right here, such that
future maintainers wouldn't have to recover the info about where the crucial
point is? Or do we have any other location where it is necessary to ensure that
a piece of code related to this issue will actually have the intended semantics
when inserted into a generated context?

Maybe one simple step we could take already would be to recognize some "typical"
literals (integers, strings, maybe also lists/maps of same); do we have
information which makes it easy to spot them? We could then throw a
`UnimplementedError` or a warning for everything else.

https://codereview.chromium.org/1181413005/diff/60001/reflectable/lib/src/tra...
reflectable/lib/src/transformer_implementation.dart:570: if (method.isAbstract)
return false;
Maybe this one was reintroduced spuriously by git?

https://codereview.chromium.org/1181413005/diff/60001/reflectable/lib/src/tra...
reflectable/lib/src/transformer_implementation.dart:988: }
Maybe this one was reintroduced spuriously by git?

[sigurdm, 2015-06-19 10:52:00]

https://codereview.chromium.org/1181413005/diff/60001/reflectable/lib/src/tra...
File reflectable/lib/src/transformer_implementation.dart (right):

https://codereview.chromium.org/1181413005/diff/60001/reflectable/lib/src/tra...
reflectable/lib/src/transformer_implementation.dart:256: int positionalCount =
type.normalParameterTypes.length;
On 2015/06/19 09:22:21, eernst wrote:
> I suspect that `normalParameterTypes` delivers information about the required
> positional parameters (which is the phrase used by the spec), so maybe we
should
> use that terminology in order to disambiguate; maybe it's worth paying the
> verbosity cost of something like `requiredPositionalCount` to make that
> explicit?

Done.

https://codereview.chromium.org/1181413005/diff/60001/reflectable/lib/src/tra...
reflectable/lib/src/transformer_implementation.dart:257: int optionalCount =
type.optionalParameterTypes.length;
On 2015/06/19 09:22:21, eernst wrote:
> Similarly, since both named parameters and (some) positional parameters can be
> optional, we might help readers of this code by using
`optionalPositionalCount`
> here.

Done.

https://codereview.chromium.org/1181413005/diff/60001/reflectable/lib/src/tra...
reflectable/lib/src/transformer_implementation.dart:266: defaultValueCode ==
null ? "" : " = $defaultValueCode";
On 2015/06/19 09:22:21, eernst wrote:
> Wouldn't it be better to have the '.. faithfully' TODO right here, such that
> future maintainers wouldn't have to recover the info about where the crucial
> point is? Or do we have any other location where it is necessary to ensure
that
> a piece of code related to this issue will actually have the intended
semantics
> when inserted into a generated context?
Done
> 
> Maybe one simple step we could take already would be to recognize some
"typical"
> literals (integers, strings, maybe also lists/maps of same); do we have
> information which makes it easy to spot them? We could then throw a
> `UnimplementedError` or a warning for everything else.

Added TODO for this.

https://codereview.chromium.org/1181413005/diff/60001/reflectable/lib/src/tra...
reflectable/lib/src/transformer_implementation.dart:570: if (method.isAbstract)
return false;
On 2015/06/19 09:22:21, eernst wrote:
> Maybe this one was reintroduced spuriously by git?

Done.

https://codereview.chromium.org/1181413005/diff/60001/reflectable/lib/src/tra...
reflectable/lib/src/transformer_implementation.dart:988: }
On 2015/06/19 09:22:21, eernst wrote:
> Maybe this one was reintroduced spuriously by git?

No this one is good. We do not want to be able to invoke constructors on
abstract classes.

[sigurdm, 2015-06-19 10:52:17]

Committed patchset #4 (id:80001) manually as
4e682d506bd14c5ab5df3763639a717ba4ed0b75 (presubmit successful).