There's cake!

[Reinier Zwitserloot]

On Tue, Nov 27, 2012 at 12:10 AM, Alex Buckley <alex.buckley at oracle.com>wrote:

>
> - It still requires the containing annotation type to be marked
> (@ContainerFor(A.class)) for the benefit of legacy clients.
>
>
Correct, but the key difference is that you don't _NEED_ to make a
containing annotation type in the first place! In my proposal, containers
as a concept exists only as a way to grandfather in existing containers.
It's not something anybody building a new library should even touch. Misc8
_REQUIRES_ the creation of one (by hand!). Forever. Like Paul said, forcing
the use of containers forever from here on out seems very shortsighted,
especially for a feature (annotations) that isn't very old.

My proposal is roughly equivalent for existing libraries, and much simpler
for new ones.


> - It still requires a sensible relationship between the containing and
> repeatable annotation types. (For example, it would be silly to allow
> getAnnotation(FooContainer.**class) to succeed auto-magically if
> FooContainer.class truly exists and has @Retention(SOURCE).)
>

Why not? If FooContainer is _explicitly_ marked to be the container for
@Foo, then I don't see any issue with x.getAnnotation(FooContainer.class)
resulting in a generated FooContainer instance that is simply an aggregator
for all the @Foos on 'x', even if FooContainer somehow ends up with
@Retention(SOURCE). Alternatively, Marking an annotation declaration with
both source retention and @ContainerFor can be considered a compiler error;
if you want I can write up a spec that just makes a heads-or-tails call on
these issues, because there are pros and cons for either scenario. I
thought it would be more fruitful to consider the basics first before
delving in such detail.

 The relation is that FooContainer is marked @ContainerFor(Foo.class). No
further relations are necessary.


>
> - It still requires "look through" by the core reflection and language
> model APIs when presented with legacy class files that physically have
> container annotations.
>

That is mostly correct; my proposal still requires look-through. It
requires less of that, though: The code remains unchanged (other than
adapting to be able to handle multiple annotations of the same type in the
class file format itself), but, if the result is not found (i.e., return
null in the current implementation), instead check if the asked-for
annotation has a @ContainerFor annotation, and if so, create a proxy for
that if there is at least one base annotation present.

As far as implementing goes, my proposal does not significantly cut down on
effort. The aim is to turn all this extra effort into something that is
relevant only for the current crop of containers. Any new APIs (and any
existing libraries that deprecate their containers) don't even need to know
any of this functionality to cater to backwards compatibility exists.

As I said, my proposal is roughly equivalent for existing libraries, and
much simpler for new ones. So far, we're still on things that are only
relevant for existing libraries. For new libraries, there's just a simple
one-liner added: Instead of the process: "No annotations found, so, return
null", we get: "No annotations found, so, does this annotation declaration
have @ContainerFor on it? No? Then, return null". I can't imagine this is
going to be a problem.


>
> - It has the same compatibility impact at runtime for the common case
> (Example 1.2-1), namely the exposure of multiple annotations of the same
> type.
>
>
the same... for existing libraries! For _new_ libraries, this complexity
goes away entirely, because _there is no container_. at all. No container
means no 1.2-1. My proposal is roughly equivalent for existing libraries,
and much simpler for new ones.

(Frankly, I expect the last point to cause far more teeth-gnashing than the
> container/containee relationship, since only the API owner sets that
> relationship whereas untold numbers of clients are affected by behavioral
> incompatibility. We considered having the SE 7 reflection methods behave
> identically in SE 8 to SE 7, but decided instead to make the methods "do
> the right thing" in SE 8 and beyond - the language allows multiple
> annotations of the same type, so reflection should too.)
>

I'm not sure I follow. There are two base cases:

* A library featuring repeatable annotations is new or aggressively
deprecates and as such has no container whatsoever. As this can only
possibly work in JavaSE8+, the library only runs on JavaSE8+, and thus uses
.getAnnotations(Base.class). There are no weird corner cases when both the
container and the base are present - my proposal has vastly simplified
matters.

* A library featuring repeatable annotations decides to use a container.
There is only one reason to do this: This library already had a container,
i.e., this library was written to run on JavaSE7. Any simplification of the
API in JavaSE8 is irrelevant; in fact, changing the answers that the
existing reflection API calls give, even if the new answers are easier to
work with, can only serve to make life harder for the developers of this
library: They _HAVE TO_ run on JavaSE7, and thus they _HAVE TO_ handle the
answers JavaSE7 can give. This includes answering with a container if you
call .getAnnotations() and a container is explicitly present. If a library
decides to migrate such that it is no longer compatible with JavaSE7, it
can do so very easily: Get rid of the container, and use
.getAnnotations(Base.class).

My proposal is in all cases easier for both library authors and end users
compared to Misc 8(for the 'new library' case, significantly so).


> All of the above makes me think there is very little simplification in
> your proposal. You've just changed the "opt in" flag from @ContainedBy(...)
> to @Repeatable. And there are two downsides:
>

I'm very confused now. My proposal completely eliminates the need for the
container, and with it, the considerable complexity that that entails, for
all users. That seems like quite a big deal to me!

New library authors get the benefit of no containers immediately. Existing
libraries that already have containers can give their end users a nice bit
of syntax sugar immediately without having to change their code at all, and
can deprecate their container whenever their schedule allows them to do so.
In the worst case scenario, where there is no plan to ever migrate away
from the container, my proposal is not better, but also not worse, than
Misc8.


>
> - Since the Foo type is just "repeatable", there is no control over
> _where_ @Foo may be repeated. It is an important aspect of the Oracle
> proposal that the annotation type owner controls the kinds of declarations
> (classes, fields, etc) on which their annotation may occur more than once
> (via @Target on the containing annotation type).
>

What are the use cases for this? If there are valid use cases, @Repeatable
can also be granted the ElementType parameter similar to @Target. This is
another detail I intentionally left out, as it is trivially solvable and
does not seem pertinent to the central idea of what I'm proposing. At any
rate, listing the types where repeatability is allowed directly on the base
annotation seems a lot nicer compared to having to look up the container
and then checking the @Target on that.


>
> - Your reflection behavior is to synthesize a container annotation only if
> the client asks for one and there's at least one base annotation present. A
> legacy client may legitimately expect to find @FooContainer with an _empty_
> value(), but no such annotation will be synthesized. I also suspect the
> rules for container synthesis are more complex than presently thought.
>

Good point; as @ContainerFor must be explicitly added, the API for
.getAnnotation(SomeContainer.class) should be defined as returning a
synthesized SomeContainer instance with an empty value() if no base
annotations are found at all.

I don't follow how container synthesis is more complex compared to the
Misc8 proposal; after all, Misc8 synthesizes containers in exactly the same
way; it just does so at compile time, and not at run time. However, because
it requires asking for one 'by name', so to speak, the container class is
guaranteed present at runtime. I don't understand why this would be more
complicated compared to doing the same thing at compile time. The empty
file issue can be worked around. The simplest way is to state that
@FooContainer will be synthesized with _empty_ value() if it is not present
but FooContainer (the declaration) is marked with @ContainerFor. The
library has to explicitly add the @ContainerFor, after all.

Every method in the annotation declaration returns its default (as defaults
must be present for everything except the value() method). What
complications do you foresee in returning defaults? The reflective API can
already do this with .getDefaultValue() on the j.l.r.Method object right
now. The value() method needs to return a synthesized array which is the
exact same as the output of .getAnnotations(Base.class). What other
complications do you suspect?


>
> The current design has been publicly specified for over three months and
> the implementation is pretty much complete, so it's a bit late to redesign
> things.
>

Repeatable annotations is a proposal that avoids a slight bit of
boilerplate. It does not enable anything that cannot be done right now with
containers. It does not strike me, in the slightest, as a crucial aspect of
java8 whose lack will be sorely missed. With a proposal on the table that
at least appears to result in a markedly less complicated end result (in
that the entire concept of 'containers' can just go away for everyone that
hasn't already built one), it seems crazy to me to continue just because of
time pressure.


> An orthogonal issue is reflection over a mix of base and container
> annotations. There is a deliberate policy laid out in section 1.2 that
> @Foo(1) on the subclass hides @Foo(0) (whether contained or not) on the
> superclass. Under a different policy, where no Foo hiding occurs, we would
> still "look through" @FooContainer and return @Foo(0) from the superclass +
> @Foo(1) from the subclass. I am surprised you think that getAnnotations()
> in SE 8 should ever return a container. Again, Oracle is happy to propose
> that SE 7 methods like getAnnotations() change behavior if it's "the right
> thing" going forward.
>
>
You're surprised I'm in favour of not changing the behaviour of existing
API? That's unexpected.

Any library that has containers in the first place is designed to work with
JavaSE7, and JavaSE7 will return containers when calling .getAnnotations().
We can't go back in time and change JavaSE7's spec. Not changing spec is a
superior solution for these libraries.

Any library that is not designed to work with JavaSE7 will (with my
proposal) not use containers. Without containers, .getAnnotations()
couldn't possibly return one. Not even introducing the notion of containers
in the first place is a superior solution for these libraries.

 -- Reinier Zwitserloot