Implement ParameterMirror.{isFinal,hasDefaultValue,defaultValue}.

Implement ParameterMirror.{isFinal,hasDefaultValue,defaultValue}.

Fixes issue 12196 and partially addresses issue 12430.

BUG=
R=ahe@google.com, asiva@google.com, gbracha@google.com

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

[Michael Lippautz (Google), 2013-08-21 20:37:36]

Right now we don't have the required information stored somewhere, so we reparse
the function declaration (only up to and including formal parameters).

Siva: The information is generated eagerly by the mirrors library when creating
the list of all parameter mirrors, since I think that reparsing once is better
than lazily reparsing for every parameter. Opinion?

Gilad: Changing the mirrors API as discussed. (ParameterMirror.defaultValue
returns an InstanceMirror)

Peter: I added a new test file, since we dart2js doesn't support this right now.
Since dart2js answers false for .hasDefaultValue the old test is not affected by
the stringify change.

https://codereview.chromium.org/23020025/diff/10001/runtime/lib/mirrors.cc
File runtime/lib/mirrors.cc (right):

https://codereview.chromium.org/23020025/diff/10001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:32: // Note a "raw type" is not the same as a RawType.
Moving the throw stuff to the top of the file, since the functions are
self-contained and needed by the rest.

[gbracha, 2013-08-21 23:02:11]

API lgtm

[rmacnak, 2013-08-23 23:15:27]

https://codereview.chromium.org/23020025/diff/10001/runtime/lib/mirrors.cc
File runtime/lib/mirrors.cc (right):

https://codereview.chromium.org/23020025/diff/10001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:192: args.SetAt(6, is_final.value() ? Bool::True() :
Bool::False());
is_final is already a handle, so you can pass it to SetAt directly.

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

https://codereview.chromium.org/23020025/diff/10001/runtime/lib/mirrors_impl....
runtime/lib/mirrors_impl.dart:1103: bool get hasDefaultValue =>
!(_defaultValueReflectee == null);
!=

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

https://codereview.chromium.org/23020025/diff/10001/sdk/lib/mirrors/mirrors.d...
sdk/lib/mirrors/mirrors.dart:1108: * A mirror on the default value for this
parameter, if it exists.
And do what if it doesn't?

https://codereview.chromium.org/23020025/diff/10001/tests/lib/mirrors/paramet...
File tests/lib/mirrors/parameter_dart_test.dart (right):

https://codereview.chromium.org/23020025/diff/10001/tests/lib/mirrors/paramet...
tests/lib/mirrors/parameter_dart_test.dart:23: B.corge({int x: 3 * 17, String
str: "bar"});
Include tests of parameters of getters and setters.

[Michael Lippautz (Google), 2013-08-24 00:17:20]

https://codereview.chromium.org/23020025/diff/10001/runtime/lib/mirrors.cc
File runtime/lib/mirrors.cc (right):

https://codereview.chromium.org/23020025/diff/10001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:192: args.SetAt(6, is_final.value() ? Bool::True() :
Bool::False());
On 2013/08/23 23:15:27, Ryan Macnak wrote:
> is_final is already a handle, so you can pass it to SetAt directly.

Done.

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

https://codereview.chromium.org/23020025/diff/10001/runtime/lib/mirrors_impl....
runtime/lib/mirrors_impl.dart:1103: bool get hasDefaultValue =>
!(_defaultValueReflectee == null);
On 2013/08/23 23:15:27, Ryan Macnak wrote:
> !=

Done.

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

https://codereview.chromium.org/23020025/diff/10001/sdk/lib/mirrors/mirrors.d...
sdk/lib/mirrors/mirrors.dart:1108: * A mirror on the default value for this
parameter, if it exists.
On 2013/08/23 23:15:27, Ryan Macnak wrote:
> And do what if it doesn't?

Done.

https://codereview.chromium.org/23020025/diff/10001/tests/lib/mirrors/paramet...
File tests/lib/mirrors/parameter_dart_test.dart (right):

https://codereview.chromium.org/23020025/diff/10001/tests/lib/mirrors/paramet...
tests/lib/mirrors/parameter_dart_test.dart:23: B.corge({int x: 3 * 17, String
str: "bar"});
On 2013/08/23 23:15:27, Ryan Macnak wrote:
> Include tests of parameters of getters and setters.

Done.

[siva, 2013-08-26 05:05:42]

https://codereview.chromium.org/23020025/diff/18001/runtime/lib/mirrors.cc
File runtime/lib/mirrors.cc (right):

https://codereview.chromium.org/23020025/diff/18001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:82: }
What about args 3 and 4, you should document those.

https://codereview.chromium.org/23020025/diff/18001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:118: }
Not sure if it is worth having so many overloaded ThrowNoSuchMethod methods. Why
not get rid of the last two and at the invocation sites
just use AbstractType::Handle(...) or Instance::null_instance() parameters.

https://codereview.chromium.org/23020025/diff/18001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:177: const Array& param_descriptor = Array::Cast(result);
We normally use the Cast operator only after an if statement which
confirms the type is an Array, e.g:
if (result.IsArray()) {
  const Array& param_descriptor = Array::Cast(result);
}

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

https://codereview.chromium.org/23020025/diff/18001/runtime/lib/mirrors_impl....
runtime/lib/mirrors_impl.dart:1099: }
Maybe write as:

if (!isOptional) {
  return false;
}
if (_defaultValue == null) {
  _defaultValue = reflect(_defaultValueReflectee);
}
return _defaultValue;

https://codereview.chromium.org/23020025/diff/18001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/23020025/diff/18001/runtime/vm/parser.cc#newc...
runtime/vm/parser.cc:770: Array& param_descriptor =
Array::Handle(Array::New(param_cnt * 2));
Array::Handle(isolate, ...) since you are using that pattern in this function

https://codereview.chromium.org/23020025/diff/18001/runtime/vm/parser.cc#newc...
runtime/vm/parser.cc:783: isolate->set_long_jump_base(base);
why is isolate->set_long_jump_base(base) not done in the success case?

[hausner, 2013-08-26 15:40:51]

https://codereview.chromium.org/23020025/diff/18001/runtime/vm/parser.h
File runtime/vm/parser.h (right):

https://codereview.chromium.org/23020025/diff/18001/runtime/vm/parser.h#newco...
runtime/vm/parser.h:147: static RawObject* ParseFunctionParameters(const
Function& func);
Please add a comment that describes what the function returns.

[Michael Lippautz (Google), 2013-08-26 19:45:56]

https://codereview.chromium.org/23020025/diff/18001/runtime/lib/mirrors.cc
File runtime/lib/mirrors.cc (right):

https://codereview.chromium.org/23020025/diff/18001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:82: }
On 2013/08/26 05:05:43, siva wrote:
> What about args 3 and 4, you should document those.

Done.

https://codereview.chromium.org/23020025/diff/18001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:118: }
On 2013/08/26 05:05:43, siva wrote:
> Not sure if it is worth having so many overloaded ThrowNoSuchMethod methods.
Why
> not get rid of the last two and at the invocation sites
> just use AbstractType::Handle(...) or Instance::null_instance() parameters.

Done.

https://codereview.chromium.org/23020025/diff/18001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:177: const Array& param_descriptor = Array::Cast(result);
On 2013/08/26 05:05:43, siva wrote:
> We normally use the Cast operator only after an if statement which
> confirms the type is an Array, e.g:
> if (result.IsArray()) {
>   const Array& param_descriptor = Array::Cast(result);
> }

result is either an error (which checked above) or an array. Cast() would
already include the assert call that checks for IsArray().

const Array& param_descriptor;
if (result.IsArray()) {
param_descriptor ^= result;
} else {
UNREACHABLE();
}

This looks strange?

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

https://codereview.chromium.org/23020025/diff/18001/runtime/lib/mirrors_impl....
runtime/lib/mirrors_impl.dart:1099: }
On 2013/08/26 05:05:43, siva wrote:
> Maybe write as:
> 
> if (!isOptional) {
>   return false;
> }
> if (_defaultValue == null) {
>   _defaultValue = reflect(_defaultValueReflectee);
> }
> return _defaultValue;

Rewrote using this pattern, with the difference that it returns null in the case
that this is not an optional parameter. (Conform to the spec.)

https://codereview.chromium.org/23020025/diff/18001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/23020025/diff/18001/runtime/vm/parser.cc#newc...
runtime/vm/parser.cc:770: Array& param_descriptor =
Array::Handle(Array::New(param_cnt * 2));
On 2013/08/26 05:05:43, siva wrote:
> Array::Handle(isolate, ...) since you are using that pattern in this function

Done.

https://codereview.chromium.org/23020025/diff/18001/runtime/vm/parser.cc#newc...
runtime/vm/parser.cc:783: isolate->set_long_jump_base(base);
On 2013/08/26 05:05:43, siva wrote:
> why is isolate->set_long_jump_base(base) not done in the success case?

Done.

https://codereview.chromium.org/23020025/diff/18001/runtime/vm/parser.h
File runtime/vm/parser.h (right):

https://codereview.chromium.org/23020025/diff/18001/runtime/vm/parser.h#newco...
runtime/vm/parser.h:147: static RawObject* ParseFunctionParameters(const
Function& func);
On 2013/08/26 15:40:51, hausner wrote:
> Please add a comment that describes what the function returns.

Done.

[siva, 2013-08-27 00:31:01]

LGTM with one comment.

https://codereview.chromium.org/23020025/diff/18001/runtime/lib/mirrors.cc
File runtime/lib/mirrors.cc (right):

https://codereview.chromium.org/23020025/diff/18001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:177: const Array& param_descriptor = Array::Cast(result);
Ok.

On 2013/08/26 19:45:57, Michael Lippautz wrote:
> On 2013/08/26 05:05:43, siva wrote:
> > We normally use the Cast operator only after an if statement which
> > confirms the type is an Array, e.g:
> > if (result.IsArray()) {
> >   const Array& param_descriptor = Array::Cast(result);
> > }
> 
> result is either an error (which checked above) or an array. Cast() would
> already include the assert call that checks for IsArray().
> 
> const Array& param_descriptor;
> if (result.IsArray()) {
> param_descriptor ^= result;
> } else {
> UNREACHABLE();
> }
> 
> This looks strange?

https://codereview.chromium.org/23020025/diff/26001/runtime/lib/mirrors.cc
File runtime/lib/mirrors.cc (right):

https://codereview.chromium.org/23020025/diff/26001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:165: args.SetAt(1, name);
document here as to why arg 2 is not set.

[ahe, 2013-08-27 16:48:38]

Provided you don't reduce test coverage of dart2js, all the Dart code LGTM!

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

https://codereview.chromium.org/23020025/diff/31001/runtime/lib/mirrors_impl....
runtime/lib/mirrors_impl.dart:1107: InstanceMirror _defaultValue = null;
No need to initialize to null.

https://codereview.chromium.org/23020025/diff/31001/tests/lib/lib.status
File tests/lib/lib.status (right):

https://codereview.chromium.org/23020025/diff/31001/tests/lib/lib.status#newc...
tests/lib/lib.status:26: mirrors/parameter_dart_test: Fail # Issue 6490
This means dart2js gets less test coverage. Please either create a new test, or
add a bool like isDart2js in mirrors_test.dart.

[Michael Lippautz (Google), 2013-08-27 17:30:15]

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

https://codereview.chromium.org/23020025/diff/31001/runtime/lib/mirrors_impl....
runtime/lib/mirrors_impl.dart:1107: InstanceMirror _defaultValue = null;
On 2013/08/27 16:48:38, ahe wrote:
> No need to initialize to null.

Done.

https://codereview.chromium.org/23020025/diff/31001/tests/lib/lib.status
File tests/lib/lib.status (right):

https://codereview.chromium.org/23020025/diff/31001/tests/lib/lib.status#newc...
tests/lib/lib.status:26: mirrors/parameter_dart_test: Fail # Issue 6490
On 2013/08/27 16:48:38, ahe wrote:
> This means dart2js gets less test coverage. Please either create a new test,
or
> add a bool like isDart2js in mirrors_test.dart.

As discussed "offline", renaming files appropriately. dart2js still gets the
same coverage as before, since tests are spread across 2 files:
* tests/lib/mirrors/param_test.dart (full version)
* tests/lib/mirrors/param_dart2js_test.dart (trimmed down version for dart2js)

[Michael Lippautz (Google), 2013-08-27 17:31:37]

Committed patchset #10 manually as r26730 (presubmit successful).

[ahe, 2013-08-27 17:33:06]

https://codereview.chromium.org/23020025/diff/31001/tests/lib/lib.status
File tests/lib/lib.status (right):

https://codereview.chromium.org/23020025/diff/31001/tests/lib/lib.status#newc...
tests/lib/lib.status:26: mirrors/parameter_dart_test: Fail # Issue 6490
On 2013/08/27 17:30:15, Michael Lippautz wrote:
> On 2013/08/27 16:48:38, ahe wrote:
> > This means dart2js gets less test coverage. Please either create a new test,
> or
> > add a bool like isDart2js in mirrors_test.dart.
> 
> As discussed "offline", renaming files appropriately. dart2js still gets the
> same coverage as before, since tests are spread across 2 files:
> * tests/lib/mirrors/param_test.dart (full version)
> * tests/lib/mirrors/param_dart2js_test.dart (trimmed down version for dart2js)

Thank you. I had overlooked that the test was a copy.