Throw when reflecting on elements not covered by a `MirrorsUsed` annotation.

Throw when reflecting on elements not covered by a `MirrorsUsed` annotation.

BUG=
R=ngeoffray@google.com

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

[ahe, 2013-08-26 20:41:14]

https://codereview.chromium.org/23045004/diff/6001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/js_backend/backend.dart (right):

https://codereview.chromium.org/23045004/diff/6001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:1539: bool
isAccessibleByReflection(Element element) {
I'm not sure this method is different from isNeededForReflection.

However, it could be a problem that this returns true when
hasInsufficientMirrorsUsed.

The problem is that "isTreeShakingDisabled" means: someone is using reflection
to invoke methods. (Or actually, if isTreeShakingDisabled is false, no methods
are invoked reflectively).

This is important as dart:mirrors may be imported to access
MirrorSystem.getName. But that shouldn't change how methods are generated.

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

https://codereview.chromium.org/23045004/diff/6001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1272: if
(reflectionName != null && backend.isAccessibleByReflection(member)) {
I thought we determined that not having a reflectionName isn't sufficient, and
that you planned to use lack of metadata.

https://codereview.chromium.org/23045004/diff/6001/tests/compiler/dart2js_ext...
File tests/compiler/dart2js_extra/mirror_invalid_field_access2_test.dart
(right):

https://codereview.chromium.org/23045004/diff/6001/tests/compiler/dart2js_ext...
tests/compiler/dart2js_extra/mirror_invalid_field_access2_test.dart:10:
@MirrorsUsed(targets: 'C.foo')
What happens if you define a class D and add it to targets:

class D {
  var foo;
  var bar;
}

In this case, I think the test will fail because mangledNames will contain an
entry for D.bar, which will be mistaken for the one for C.bar.

[karlklose, 2013-08-28 15:12:34]

PTAL.

[ngeoffray, 2013-08-29 07:45:49]

LGTM

https://codereview.chromium.org/23045004/diff/12002/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/js_backend/backend.dart (right):

https://codereview.chromium.org/23045004/diff/12002/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:1559: /// is
in the set of elements covered by a `MirrorsUsed` annotation.
Please also add a comment for isNeededForReflection. It is not obvious what the
difference is between the 2.

https://codereview.chromium.org/23045004/diff/12002/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart (right):

https://codereview.chromium.org/23045004/diff/12002/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:341:
js('prototype["${namer.getterPrefix}" + accessorName].\$reflectable ='
Please move $reflectable to the namer.

https://codereview.chromium.org/23045004/diff/12002/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:342: '
reflectable')
Only add it if reflectable is true?

https://codereview.chromium.org/23045004/diff/12002/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:355: '
reflectable')
ditto.

https://codereview.chromium.org/23045004/diff/12002/sdk/lib/_internal/lib/js_...
File sdk/lib/_internal/lib/js_helper.dart (right):

https://codereview.chromium.org/23045004/diff/12002/sdk/lib/_internal/lib/js_...
sdk/lib/_internal/lib/js_helper.dart:152: _getCachedInvocation(Object object) {
Don't you check already at all call sites of this method?

https://codereview.chromium.org/23045004/diff/12002/sdk/lib/_internal/lib/js_...
sdk/lib/_internal/lib/js_helper.dart:173: if (JS('bool', r'#.$reflectable ==
false', method)) {
Use JS_GET_NAME.

https://codereview.chromium.org/23045004/diff/12002/sdk/lib/_internal/lib/js_...
sdk/lib/_internal/lib/js_helper.dart:174: throw new UnsupportedError('reflection
on ${memberName}');
Maybe add a bit more explanation in the error message. Isn't it only because of
@MirrorsUsed?

https://codereview.chromium.org/23045004/diff/12002/sdk/lib/_internal/lib/js_...
File sdk/lib/_internal/lib/js_mirrors.dart (right):

https://codereview.chromium.org/23045004/diff/12002/sdk/lib/_internal/lib/js_...
sdk/lib/_internal/lib/js_mirrors.dart:278: throw new
UnsupportedError('reflection on ${memberName}');
Better error message.

https://codereview.chromium.org/23045004/diff/12002/sdk/lib/_internal/lib/js_...
sdk/lib/_internal/lib/js_mirrors.dart:1020: throw new
UnsupportedError('reflection on ${memberName}');
Ditto.

https://codereview.chromium.org/23045004/diff/12002/sdk/lib/_internal/lib/js_...
sdk/lib/_internal/lib/js_mirrors.dart:1073: if (code == 45) {
Put 45 in a variable, like int reflectionMarker = 45;

https://codereview.chromium.org/23045004/diff/12002/sdk/lib/_internal/lib/js_...
sdk/lib/_internal/lib/js_mirrors.dart:1271: canInvokeReflectively() {
bool canInvokeReflectively() {

https://codereview.chromium.org/23045004/diff/12002/sdk/lib/_internal/lib/js_...
sdk/lib/_internal/lib/js_mirrors.dart:1272: return JS('bool', r'#.$reflectable
== true', _jsFunction);
JS_GET_NAME.

https://codereview.chromium.org/23045004/diff/12002/tests/compiler/dart2js_ex...
File tests/compiler/dart2js_extra/mirror_invalid_field_access2_test.dart
(right):

https://codereview.chromium.org/23045004/diff/12002/tests/compiler/dart2js_ex...
tests/compiler/dart2js_extra/mirror_invalid_field_access2_test.dart:29:
Expect.throws(() => mirror.setField(const Symbol('bar'),  2));
I'd check the error.

[karlklose, 2013-08-29 15:33:37]

Thank you, Nicolas.

https://codereview.chromium.org/23045004/diff/12002/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/js_backend/backend.dart (right):

https://codereview.chromium.org/23045004/diff/12002/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:1559: /// is
in the set of elements covered by a `MirrorsUsed` annotation.
On 2013/08/29 07:45:49, ngeoffray wrote:
> Please also add a comment for isNeededForReflection. It is not obvious what
the
> difference is between the 2.

Done.

https://codereview.chromium.org/23045004/diff/12002/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart (right):

https://codereview.chromium.org/23045004/diff/12002/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:341:
js('prototype["${namer.getterPrefix}" + accessorName].\$reflectable ='
On 2013/08/29 07:45:49, ngeoffray wrote:
> Please move $reflectable to the namer.

Done.

https://codereview.chromium.org/23045004/diff/12002/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:342: '
reflectable')
On 2013/08/29 07:45:49, ngeoffray wrote:
> Only add it if reflectable is true?

Done.

https://codereview.chromium.org/23045004/diff/12002/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:355: '
reflectable')
On 2013/08/29 07:45:49, ngeoffray wrote:
> ditto.

Done.

https://codereview.chromium.org/23045004/diff/12002/sdk/lib/_internal/lib/js_...
File sdk/lib/_internal/lib/js_helper.dart (right):

https://codereview.chromium.org/23045004/diff/12002/sdk/lib/_internal/lib/js_...
sdk/lib/_internal/lib/js_helper.dart:152: _getCachedInvocation(Object object) {
No, the other checks are for static functions, which use a different lookup.

https://codereview.chromium.org/23045004/diff/12002/sdk/lib/_internal/lib/js_...
sdk/lib/_internal/lib/js_helper.dart:173: if (JS('bool', r'#.$reflectable ==
false', method)) {
On 2013/08/29 07:45:49, ngeoffray wrote:
> Use JS_GET_NAME.

Done.

https://codereview.chromium.org/23045004/diff/12002/sdk/lib/_internal/lib/js_...
sdk/lib/_internal/lib/js_helper.dart:174: throw new UnsupportedError('reflection
on ${memberName}');
On 2013/08/29 07:45:49, ngeoffray wrote:
> Maybe add a bit more explanation in the error message. Isn't it only because
of
> @MirrorsUsed?

Done.

https://codereview.chromium.org/23045004/diff/12002/sdk/lib/_internal/lib/js_...
File sdk/lib/_internal/lib/js_mirrors.dart (right):

https://codereview.chromium.org/23045004/diff/12002/sdk/lib/_internal/lib/js_...
sdk/lib/_internal/lib/js_mirrors.dart:278: throw new
UnsupportedError('reflection on ${memberName}');
On 2013/08/29 07:45:49, ngeoffray wrote:
> Better error message.

Done.

https://codereview.chromium.org/23045004/diff/12002/sdk/lib/_internal/lib/js_...
sdk/lib/_internal/lib/js_mirrors.dart:1020: throw new
UnsupportedError('reflection on ${memberName}');
On 2013/08/29 07:45:49, ngeoffray wrote:
> Ditto.

Done.

https://codereview.chromium.org/23045004/diff/12002/sdk/lib/_internal/lib/js_...
sdk/lib/_internal/lib/js_mirrors.dart:1073: if (code == 45) {
On 2013/08/29 07:45:49, ngeoffray wrote:
> Put 45 in a variable, like int reflectionMarker = 45;

Done.

https://codereview.chromium.org/23045004/diff/12002/sdk/lib/_internal/lib/js_...
sdk/lib/_internal/lib/js_mirrors.dart:1271: canInvokeReflectively() {
On 2013/08/29 07:45:49, ngeoffray wrote:
> bool canInvokeReflectively() {

Done.

https://codereview.chromium.org/23045004/diff/12002/sdk/lib/_internal/lib/js_...
sdk/lib/_internal/lib/js_mirrors.dart:1272: return JS('bool', r'#.$reflectable
== true', _jsFunction);
On 2013/08/29 07:45:49, ngeoffray wrote:
> JS_GET_NAME.

Done.

https://codereview.chromium.org/23045004/diff/12002/tests/compiler/dart2js_ex...
File tests/compiler/dart2js_extra/mirror_invalid_field_access2_test.dart
(right):

https://codereview.chromium.org/23045004/diff/12002/tests/compiler/dart2js_ex...
tests/compiler/dart2js_extra/mirror_invalid_field_access2_test.dart:29:
Expect.throws(() => mirror.setField(const Symbol('bar'),  2));
On 2013/08/29 07:45:49, ngeoffray wrote:
> I'd check the error.

Done.

[karlklose, 2013-08-29 15:55:51]

Committed patchset #9 manually as r26874 (presubmit successful).