Adds limited support for private classes.

Adds limited support for private classes.

Subtype/superclass quantification can include private classes, even
though this may not be needed (or even intended) by the programmer who
wrote the capabilities, or the programmer who use them via an import.
This CL ensures that we do not any more generate type expressions that
fail to compile because they attempt to access a private class in a
different library.

We keep the private classes in place because they are needed during
traversals of superclass chains (they should not fail when superclass
quantification is enabled), and they do have `declarations` and other
features as expected for any class mirror.

It is quite common that a given "official" class `C` is used by
clients (in type annotations etc.), but they will actually be using
instances of "secret" subclasses, possibly private, often because
they have obtained those objects using factory constructors (a good
example is `List`). This means that it is very useful to be able to
obtain an instance mirror for such an instance, and also to do
`.type` and get a class mirror for it.

That feature can only be supported if we can recognize that a given
object is an instance of a specific private class, and we do not
have a maintainable way to do that. So we leave out that bit for now.

Fixes the code generation problem reported in issue #43.

R=sigurdm@google.com

Committed: https://github.com/dart-lang/reflectable/commit/2d9b6f5584b921183faec6d2d5b8d09265175bd0

[eernst, 2015-10-15 11:32:44]

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

[eernst, 2015-10-15 11:32:45]

Private classes, limitedly! ;)

[sigurdm, 2015-10-15 12:30:58]

Write about the other refactorings in the cl dscription, or move them to a
separate cl.

https://codereview.chromium.org/1391013008/diff/1/reflectable/lib/src/reflect...
File reflectable/lib/src/reflectable_mirror_based.dart (right):

https://codereview.chromium.org/1391013008/diff/1/reflectable/lib/src/reflect...
reflectable/lib/src/reflectable_mirror_based.dart:343: if
(impliesCorrespondingSetter(reflectable.capabilities) &&
I think the changes related to getter/setters ideally belong in a separate CL.

https://codereview.chromium.org/1391013008/diff/1/reflectable/lib/src/reflect...
File reflectable/lib/src/reflectable_transformer_based.dart (right):

https://codereview.chromium.org/1391013008/diff/1/reflectable/lib/src/reflect...
reflectable/lib/src/reflectable_transformer_based.dart:113: if (result == null
&& "$type".startsWith("_")) {
Abstract startsWith("_") to isPrivateName()

https://codereview.chromium.org/1391013008/diff/1/reflectable/lib/src/reflect...
reflectable/lib/src/reflectable_transformer_based.dart:1272: // For
mixin-applications we need to construct objects that represents their
I think this comment needs to be updated.

https://codereview.chromium.org/1391013008/diff/1/reflectable/lib/src/reflect...
reflectable/lib/src/reflectable_transformer_based.dart:1283: bool
correspondsTo(Type type) {
I think this support for reflecting on instances of private classes will be very
shaky:
* Type.toString() is not well defined.
* Even given the current behavior of Type.toString() there is no qualified name,
so distinguishing two private classes with the same name cannot be done in
general.

We should try to write up a DEP to allow back-door access to privates.

https://codereview.chromium.org/1391013008/diff/1/reflectable/lib/src/transfo...
File reflectable/lib/src/transformer_implementation.dart (right):

https://codereview.chromium.org/1391013008/diff/1/reflectable/lib/src/transfo...
reflectable/lib/src/transformer_implementation.dart:1042: String uriCode =
'Uri.parse(r"reflectable://$library")';
Is this any better?

https://codereview.chromium.org/1391013008/diff/1/test_reflectable/test/priva...
File test_reflectable/test/private_class_test.dart (right):

https://codereview.chromium.org/1391013008/diff/1/test_reflectable/test/priva...
test_reflectable/test/private_class_test.dart:44: InstanceMirror instanceMirror
= privacyReflectable.reflect(object);
What about other actions on private class-mirrors?

* invoking static functions
* invoking constructors

Without a backdoor the support for private access is necessarily very sketchy.
Maybe it is better to just warn up front (perhaps with logger.fine) and then
ignore the private classes.

https://codereview.chromium.org/1391013008/diff/1/test_reflectable/test/priva...
test_reflectable/test/private_class_test.dart:44: InstanceMirror instanceMirror
= privacyReflectable.reflect(object);
I think the right way to allow reflection on private sub-classes is to make
support for admitSubtypesCapability that uses is-checks (in some well-defined
order) to choose the classmirror to return.

[eernst, 2015-10-15 12:51:05]

On 2015/10/15 12:30:58, sigurdm wrote:
> Write about the other refactorings in the cl dscription, or move them to a
> separate cl.
> 
>
https://codereview.chromium.org/1391013008/diff/1/reflectable/lib/src/reflect...
> File reflectable/lib/src/reflectable_mirror_based.dart (right):
> 
>
https://codereview.chromium.org/1391013008/diff/1/reflectable/lib/src/reflect...
> reflectable/lib/src/reflectable_mirror_based.dart:343: if
> (impliesCorrespondingSetter(reflectable.capabilities) &&
> I think the changes related to getter/setters ideally belong in a separate CL.
> 
>
https://codereview.chromium.org/1391013008/diff/1/reflectable/lib/src/reflect...
> File reflectable/lib/src/reflectable_transformer_based.dart (right):
> 
>
https://codereview.chromium.org/1391013008/diff/1/reflectable/lib/src/reflect...
> reflectable/lib/src/reflectable_transformer_based.dart:113: if (result == null
> && "$type".startsWith("_")) {
> Abstract startsWith("_") to isPrivateName()
> 
>
https://codereview.chromium.org/1391013008/diff/1/reflectable/lib/src/reflect...
> reflectable/lib/src/reflectable_transformer_based.dart:1272: // For
> mixin-applications we need to construct objects that represents their
> I think this comment needs to be updated.
> 
>
https://codereview.chromium.org/1391013008/diff/1/reflectable/lib/src/reflect...
> reflectable/lib/src/reflectable_transformer_based.dart:1283: bool
> correspondsTo(Type type) {
> I think this support for reflecting on instances of private classes will be
very
> shaky:
> * Type.toString() is not well defined.
> * Even given the current behavior of Type.toString() there is no qualified
name,
> so distinguishing two private classes with the same name cannot be done in
> general.
> 
> We should try to write up a DEP to allow back-door access to privates.
> 
>
https://codereview.chromium.org/1391013008/diff/1/reflectable/lib/src/transfo...
> File reflectable/lib/src/transformer_implementation.dart (right):
> 
>
https://codereview.chromium.org/1391013008/diff/1/reflectable/lib/src/transfo...
> reflectable/lib/src/transformer_implementation.dart:1042: String uriCode =
> 'Uri.parse(r"reflectable://$library")';
> Is this any better?
> 
>
https://codereview.chromium.org/1391013008/diff/1/test_reflectable/test/priva...
> File test_reflectable/test/private_class_test.dart (right):
> 
>
https://codereview.chromium.org/1391013008/diff/1/test_reflectable/test/priva...
> test_reflectable/test/private_class_test.dart:44: InstanceMirror
instanceMirror
> = privacyReflectable.reflect(object);
> What about other actions on private class-mirrors?
> 
> * invoking static functions
> * invoking constructors
> 
> Without a backdoor the support for private access is necessarily very sketchy.
> Maybe it is better to just warn up front (perhaps with logger.fine) and then
> ignore the private classes.
> 
>
https://codereview.chromium.org/1391013008/diff/1/test_reflectable/test/priva...
> test_reflectable/test/private_class_test.dart:44: InstanceMirror
instanceMirror
> = privacyReflectable.reflect(object);
> I think the right way to allow reflection on private sub-classes is to make
> support for admitSubtypesCapability that uses is-checks (in some well-defined
> order) to choose the classmirror to return.

Further review responses upcoming.

[eernst, 2015-10-15 13:46:12]

Review response. Main thing: Support for `reflect` on instances of private
classes was deleted, and the test was adjusted to expect that `canReflect`
returns false in those cases.

https://codereview.chromium.org/1391013008/diff/1/reflectable/lib/src/reflect...
File reflectable/lib/src/reflectable_mirror_based.dart (right):

https://codereview.chromium.org/1391013008/diff/1/reflectable/lib/src/reflect...
reflectable/lib/src/reflectable_mirror_based.dart:343: if
(impliesCorrespondingSetter(reflectable.capabilities) &&
On 2015/10/15 12:30:58, sigurdm wrote:
> I think the changes related to getter/setters ideally belong in a separate CL.

Agreed in meat space to not separate them this time.

https://codereview.chromium.org/1391013008/diff/1/reflectable/lib/src/reflect...
File reflectable/lib/src/reflectable_transformer_based.dart (right):

https://codereview.chromium.org/1391013008/diff/1/reflectable/lib/src/reflect...
reflectable/lib/src/reflectable_transformer_based.dart:113: if (result == null
&& "$type".startsWith("_")) {
On 2015/10/15 12:30:58, sigurdm wrote:
> Abstract startsWith("_") to isPrivateName()

This section deleted: We do not support reflecting on private classes at this
point after all.

https://codereview.chromium.org/1391013008/diff/1/reflectable/lib/src/reflect...
reflectable/lib/src/reflectable_transformer_based.dart:1272: // For
mixin-applications we need to construct objects that represents their
On 2015/10/15 12:30:58, sigurdm wrote:
> I think this comment needs to be updated.

Done.

https://codereview.chromium.org/1391013008/diff/1/reflectable/lib/src/reflect...
reflectable/lib/src/reflectable_transformer_based.dart:1283: bool
correspondsTo(Type type) {
On 2015/10/15 12:30:58, sigurdm wrote:
> I think this support for reflecting on instances of private classes will be
very
> shaky:
> * Type.toString() is not well defined.
> * Even given the current behavior of Type.toString() there is no qualified
name,
> so distinguishing two private classes with the same name cannot be done in
> general.
> 
> We should try to write up a DEP to allow back-door access to privates.

Deleted, too.

https://codereview.chromium.org/1391013008/diff/1/reflectable/lib/src/transfo...
File reflectable/lib/src/transformer_implementation.dart (right):

https://codereview.chromium.org/1391013008/diff/1/reflectable/lib/src/transfo...
reflectable/lib/src/transformer_implementation.dart:1042: String uriCode =
'Uri.parse(r"reflectable://$library")';
On 2015/10/15 12:30:58, sigurdm wrote:
> Is this any better?

We cannot implement a map from `Uri` to `LibraryMirror` when they all have the
same uri (`null`). This makes them distinct as far as they have distinct library
directives, and then we can implement `Reflectable.libraries` (which was
missing).

But they are still potentially identical for different libraries (e.g., two
libraries with no library directive).

Adjusted to include the `libraries.indexOf(library)` for disambiguation.

https://codereview.chromium.org/1391013008/diff/1/test_reflectable/test/priva...
File test_reflectable/test/private_class_test.dart (right):

https://codereview.chromium.org/1391013008/diff/1/test_reflectable/test/priva...
test_reflectable/test/private_class_test.dart:44: InstanceMirror instanceMirror
= privacyReflectable.reflect(object);
On 2015/10/15 12:30:58, sigurdm wrote:
> I think the right way to allow reflection on private sub-classes is to make
> support for admitSubtypesCapability that uses is-checks (in some well-defined
> order) to choose the classmirror to return.

For now, I've changed the test to check `canReflect` and see that it says
`false` on the private classes.

https://codereview.chromium.org/1391013008/diff/1/test_reflectable/test/priva...
test_reflectable/test/private_class_test.dart:44: InstanceMirror instanceMirror
= privacyReflectable.reflect(object);
On 2015/10/15 12:30:58, sigurdm wrote:
> What about other actions on private class-mirrors?
> 
> * invoking static functions
> * invoking constructors
> 
> Without a backdoor the support for private access is necessarily very sketchy.
> Maybe it is better to just warn up front (perhaps with logger.fine) and then
> ignore the private classes.

Now you won't get a mirror on them, so we can revisit that later when they
return.

[sigurdm, 2015-10-15 13:52:44]

LGTM

[eernst, 2015-10-15 14:14:43]

Committed patchset #5 (id:80001) manually as
2d9b6f5584b921183faec6d2d5b8d09265175bd0 (presubmit successful).