Parameters with function types supported + Added features in mirrors implementation

Parameters with function types supported + Added features in mirrors implementation

VariableListElement.computeType now handles function types.
Default value and function typed parameters supported in the mirror system.

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

[floitsch, 2012-07-17 18:53:04]

LGTM.
But I would appreciate if Karl or Peter would do another review-round at some
point.

https://chromiumcodereview.appspot.com/10779010/diff/1/lib/compiler/implement...
File lib/compiler/implementation/elements/elements.dart (right):

https://chromiumcodereview.appspot.com/10779010/diff/1/lib/compiler/implement...
lib/compiler/implementation/elements/elements.dart:507: * Function signature for
variable of a function type. The signature is kept
for variables with a function type.

https://chromiumcodereview.appspot.com/10779010/diff/1/lib/compiler/implement...
lib/compiler/implementation/elements/elements.dart:508: * in order to provide
full information about parameter names through the
remove "in order".

https://chromiumcodereview.appspot.com/10779010/diff/1/lib/compiler/implement...
lib/compiler/implementation/elements/elements.dart:531: var node =
parseNode(compiler);
s/var/Node.

https://chromiumcodereview.appspot.com/10779010/diff/1/lib/compiler/implement...
lib/compiler/implementation/elements/elements.dart:536: var link =
node.definitions.nodes;
s/var/Link

https://chromiumcodereview.appspot.com/10779010/diff/1/lib/compiler/implement...
lib/compiler/implementation/elements/elements.dart:538: link.head is
FunctionExpression &&
.asFunctionExpression() != null

https://chromiumcodereview.appspot.com/10779010/diff/1/lib/compiler/implement...
lib/compiler/implementation/elements/elements.dart:540: {
move into previous line.

https://chromiumcodereview.appspot.com/10779010/diff/1/lib/compiler/implement...
lib/compiler/implementation/elements/elements.dart:541: var functionExpression =
link.head;
s/var/FunctionExpression

https://chromiumcodereview.appspot.com/10779010/diff/1/lib/compiler/implement...
lib/compiler/implementation/elements/elements.dart:544: functionSignature =
compiler.resolveFunctionExpression(
nit: I would prefer a new line after the "=" (if it fits).

https://chromiumcodereview.appspot.com/10779010/diff/1/lib/dartdoc/dartdoc.dart
File lib/dartdoc/dartdoc.dart (right):

https://chromiumcodereview.appspot.com/10779010/diff/1/lib/dartdoc/dartdoc.da...
lib/dartdoc/dartdoc.dart:1102: if (paramName.startsWith('_') &&
It is probably not that easy. You need to check that
- there is no other parameter with the shortened name.
- adapt the links accordingly.

In any case I would prefer this to be in a separate CL.

https://chromiumcodereview.appspot.com/10779010/diff/1/lib/dartdoc/mirrors/da...
File lib/dartdoc/mirrors/dart2js_mirror.dart (right):

https://chromiumcodereview.appspot.com/10779010/diff/1/lib/dartdoc/mirrors/da...
lib/dartdoc/mirrors/dart2js_mirror.dart:46: system, method, link.head, true));
indent by 4.

https://chromiumcodereview.appspot.com/10779010/diff/1/lib/dartdoc/mirrors/da...
lib/dartdoc/mirrors/dart2js_mirror.dart:52: Dart2JsParameterMirror
_convertVariableElementToParameterMirror(
alternatively you could make the unnamed Dart2JsParameterMirror constructor to
be a factory that specializes if it sees a FieldParameterElement.

https://chromiumcodereview.appspot.com/10779010/diff/1/lib/dartdoc/mirrors/mi...
File lib/dartdoc/mirrors/mirrors.dart (right):

https://chromiumcodereview.appspot.com/10779010/diff/1/lib/dartdoc/mirrors/mi...
lib/dartdoc/mirrors/mirrors.dart:409: * constructor, that is, if it is of the
form [:this.x:] where [:x:] is a
... constructor. That is ...

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

https://chromiumcodereview.appspot.com/10779010/diff/1/tests/compiler/dart2js...
tests/compiler/dart2js/mirrors_test.dart:485: // TODO(johnniwinther): Should
return the function type E func(F f):
Remove TODO.

https://chromiumcodereview.appspot.com/10779010/diff/1/tests/compiler/dart2js...
tests/compiler/dart2js/mirrors_test.dart:487:
Expect.isNotNull(method3Parameter1type, "Parameter type of 'func1' is null");
Where does the "func1" come from?

https://chromiumcodereview.appspot.com/10779010/diff/1/tests/compiler/dart2js...
tests/compiler/dart2js/mirrors_test.dart:493: "Parameters of 'func1' is null");
nit: align.

https://chromiumcodereview.appspot.com/10779010/diff/1/tests/compiler/dart2js...
tests/compiler/dart2js/mirrors_test.dart:503: "Return type of 'func1' is not a
F");
That's not the return type.

[Johnni Winther, 2012-07-18 13:32:50]

PTAL

https://chromiumcodereview.appspot.com/10779010/diff/1/lib/compiler/implement...
File lib/compiler/implementation/elements/elements.dart (right):

https://chromiumcodereview.appspot.com/10779010/diff/1/lib/compiler/implement...
lib/compiler/implementation/elements/elements.dart:507: * Function signature for
variable of a function type. The signature is kept
On 2012/07/17 18:53:04, floitsch wrote:
> for variables with a function type.

Done.

https://chromiumcodereview.appspot.com/10779010/diff/1/lib/compiler/implement...
lib/compiler/implementation/elements/elements.dart:508: * in order to provide
full information about parameter names through the
On 2012/07/17 18:53:04, floitsch wrote:
> remove "in order".

Done.

https://chromiumcodereview.appspot.com/10779010/diff/1/lib/compiler/implement...
lib/compiler/implementation/elements/elements.dart:531: var node =
parseNode(compiler);
On 2012/07/17 18:53:04, floitsch wrote:
> s/var/Node.

Done.

https://chromiumcodereview.appspot.com/10779010/diff/1/lib/compiler/implement...
lib/compiler/implementation/elements/elements.dart:536: var link =
node.definitions.nodes;
On 2012/07/17 18:53:04, floitsch wrote:
> s/var/Link

Done.

https://chromiumcodereview.appspot.com/10779010/diff/1/lib/compiler/implement...
lib/compiler/implementation/elements/elements.dart:538: link.head is
FunctionExpression &&
On 2012/07/17 18:53:04, floitsch wrote:
> .asFunctionExpression() != null

Done.

https://chromiumcodereview.appspot.com/10779010/diff/1/lib/compiler/implement...
lib/compiler/implementation/elements/elements.dart:540: {
On 2012/07/17 18:53:04, floitsch wrote:
> move into previous line.

Done.

https://chromiumcodereview.appspot.com/10779010/diff/1/lib/compiler/implement...
lib/compiler/implementation/elements/elements.dart:541: var functionExpression =
link.head;
On 2012/07/17 18:53:04, floitsch wrote:
> s/var/FunctionExpression

Done.

https://chromiumcodereview.appspot.com/10779010/diff/1/lib/compiler/implement...
lib/compiler/implementation/elements/elements.dart:544: functionSignature =
compiler.resolveFunctionExpression(
On 2012/07/17 18:53:04, floitsch wrote:
> nit: I would prefer a new line after the "=" (if it fits).

Done.

https://chromiumcodereview.appspot.com/10779010/diff/1/lib/dartdoc/dartdoc.dart
File lib/dartdoc/dartdoc.dart (right):

https://chromiumcodereview.appspot.com/10779010/diff/1/lib/dartdoc/dartdoc.da...
lib/dartdoc/dartdoc.dart:1102: if (paramName.startsWith('_') &&
On 2012/07/17 18:53:04, floitsch wrote:
> It is probably not that easy. You need to check that
> - there is no other parameter with the shortened name.
> - adapt the links accordingly.
> 
> In any case I would prefer this to be in a separate CL.

Left out for now.

https://chromiumcodereview.appspot.com/10779010/diff/1/lib/dartdoc/mirrors/da...
File lib/dartdoc/mirrors/dart2js_mirror.dart (right):

https://chromiumcodereview.appspot.com/10779010/diff/1/lib/dartdoc/mirrors/da...
lib/dartdoc/mirrors/dart2js_mirror.dart:46: system, method, link.head, true));
On 2012/07/17 18:53:04, floitsch wrote:
> indent by 4.

Done.

https://chromiumcodereview.appspot.com/10779010/diff/1/lib/dartdoc/mirrors/da...
lib/dartdoc/mirrors/dart2js_mirror.dart:52: Dart2JsParameterMirror
_convertVariableElementToParameterMirror(
On 2012/07/17 18:53:04, floitsch wrote:
> alternatively you could make the unnamed Dart2JsParameterMirror constructor to
> be a factory that specializes if it sees a FieldParameterElement.

Done.

[floitsch, 2012-07-19 08:45:00]

Still LGTM.

https://chromiumcodereview.appspot.com/10779010/diff/6001/lib/compiler/implem...
File lib/compiler/implementation/elements/elements.dart (right):

https://chromiumcodereview.appspot.com/10779010/diff/6001/lib/compiler/implem...
lib/compiler/implementation/elements/elements.dart:507: * Function signature for
variable with a function type. The signature is kept
variable*s* or "a variable"

https://chromiumcodereview.appspot.com/10779010/diff/6001/lib/dartdoc/mirrors...
File lib/dartdoc/mirrors/dart2js_mirror.dart (right):

https://chromiumcodereview.appspot.com/10779010/diff/6001/lib/dartdoc/mirrors...
lib/dartdoc/mirrors/dart2js_mirror.dart:580:
Dart2JsParameterMirror.normal(Dart2JsMirrorSystem system,
Unless you think that people should use this constructor directly, maybe make it
private.
If you leave it public, add a comment explaining the difference.

[Johnni Winther, 2012-07-19 13:12:10]

https://chromiumcodereview.appspot.com/10779010/diff/1/lib/dartdoc/mirrors/mi...
File lib/dartdoc/mirrors/mirrors.dart (right):

https://chromiumcodereview.appspot.com/10779010/diff/1/lib/dartdoc/mirrors/mi...
lib/dartdoc/mirrors/mirrors.dart:409: * constructor, that is, if it is of the
form [:this.x:] where [:x:] is a
On 2012/07/17 18:53:04, floitsch wrote:
> ... constructor. That is ...

Done.

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

https://chromiumcodereview.appspot.com/10779010/diff/1/tests/compiler/dart2js...
tests/compiler/dart2js/mirrors_test.dart:485: // TODO(johnniwinther): Should
return the function type E func(F f):
On 2012/07/17 18:53:04, floitsch wrote:
> Remove TODO.

Done.

https://chromiumcodereview.appspot.com/10779010/diff/1/tests/compiler/dart2js...
tests/compiler/dart2js/mirrors_test.dart:487:
Expect.isNotNull(method3Parameter1type, "Parameter type of 'func1' is null");
On 2012/07/17 18:53:04, floitsch wrote:
> Where does the "func1" come from?
func1 is the name of the first parameter of method3.

https://chromiumcodereview.appspot.com/10779010/diff/1/tests/compiler/dart2js...
tests/compiler/dart2js/mirrors_test.dart:493: "Parameters of 'func1' is null");
On 2012/07/17 18:53:04, floitsch wrote:
> nit: align.

Done.

https://chromiumcodereview.appspot.com/10779010/diff/1/tests/compiler/dart2js...
tests/compiler/dart2js/mirrors_test.dart:503: "Return type of 'func1' is not a
F");
On 2012/07/17 18:53:04, floitsch wrote:
> That's not the return type.
Fixed.

https://chromiumcodereview.appspot.com/10779010/diff/6001/lib/dartdoc/mirrors...
File lib/dartdoc/mirrors/dart2js_mirror.dart (right):

https://chromiumcodereview.appspot.com/10779010/diff/6001/lib/dartdoc/mirrors...
lib/dartdoc/mirrors/dart2js_mirror.dart:580:
Dart2JsParameterMirror.normal(Dart2JsMirrorSystem system,
On 2012/07/19 08:45:00, floitsch wrote:
> Unless you think that people should use this constructor directly, maybe make
it
> private.
> If you leave it public, add a comment explaining the difference.

I actually asked whether private constructors existed and he said 'no'! I'll
make it private.

[ahe, 2012-08-02 19:20:25]

https://chromiumcodereview.appspot.com/10779010/diff/15001/lib/compiler/imple...
File lib/compiler/implementation/elements/elements.dart (right):

https://chromiumcodereview.appspot.com/10779010/diff/15001/lib/compiler/imple...
lib/compiler/implementation/elements/elements.dart:511: FunctionSignature
functionSignature;
We plan to turn this into a type, right?

https://chromiumcodereview.appspot.com/10779010/diff/15001/lib/compiler/imple...
lib/compiler/implementation/elements/elements.dart:534: } else {
We are already changing this, right? This looks scary and a potential for
infinite recursion.