[type-annos-observers] Comments on the Nov 7 specification

[Alex Buckley]

Experts,

Werner Dietl sent the comments below about the spec in Mike's mail 
"Improving the format of type annotation attributes" of 11/7/12.

Most are useful clarifications to the spec, but one comment proposes an 
array rather than a tree to represent the hierarchical location of a 
type annotation in a compound type. Each level in the tree becomes the 
succeeding array entry.

The proposal has a union with a context-sensitive single member that 
would break pack200, so here is a slightly modified version:

struct type_path {
   u1              path_length;
   type_path_entry path[path_length];
}

struct type_path_entry {
   u1 type_path_kind;
     // 0: annotation is deeper in this array type
     // 1: annotation is deeper in this nested type
     // 2: annotation is on the bound of this wildcard type arg
     // 3: annotation is on the i'th type arg of this parameterized type
   u1 type_argument_index;
     // 0: ignore me
     // non-0: the 1st, 2nd, etc type arg of this parameterized type
}

I think this is fine. A further improvement would be to drop the 
type_argument_index item and encode the i'th type arg into the 
type_path_kind item. Namely, type_path_kind values >=3 represent the 
type_path_kind-2'th type arg of the current parameterized type. This 
trick is used in the stack_map_frame structure (JVMS 4.7.4).

Alex

**********

page 16:
- The values for "path_type" are not specified. Something like Fig. 1
might be overkill, but I don't just want to assume that they are 0 - 4.

- I'm not quite sure I see the advantage of the nested structure.
Couldn't we use an array instead? We are simply specifying a sequence
of steps to follow, not a complicated tree.

struct type_path_entry {
   u1 path_type;
   union {
     // empty for most, only type_argument_path has info
     u1 type_argument_index;
   }
}

struct type_path {
   u1 path_length;
   type_path_entry [ path_length ];
}

The path_length could be used instead of or in addition to ending each
path with empty_path - like we can have either C-style null-terminated
strings or Pascal-style strings with length
I would prefer using path_length without empty_path.

Then instead of:

type_argument_path { 1, type_argument_path { 0, empty path } }

we would have:

length: 2;
elements: type_argument_path, 1, type_argument_path, 0;

I think the resulting sequence of bytes would be the same (if
path_length is left off), but I would find the presentation as array a
lot simpler.
Am I missing what the advantage of this nested presentation is? If so,
I might be implementing it wrong


- On a related note, I find the representation of nested types weird. It
basically is "go one step up in the nesting". Similarly,
type_argument_path could be a "go one step right in the type
arguments" operation.
I find the solution for type arguments nicer and would suggest that we use:

   union {
     // empty for most
     u1 type_argument_index;
     u1 outer_index;
   }

The location for @M in the first example was:

inner_type_path { inner_type_path { inner_type_path { empty_path } } }

and would now simply be:

length: 1;
elements: inner_type_path, 3;

This is similar to the old way of counting, just ignoring any
array/type argument confusion and just counting the nesting. 0 is the
main modifier and left off, 1 is the first enclosing, etc. This way
the index corresponds to the number of "inner_type_path" elements in
the complicated/current way. We could use 0-based instead.


page 14:
- Section 3.3.6: I'm a bit amazed that the "throws_type_index" is a
u2, but the "method_parameter_index" is a u1. Can there really be so
many more exceptions than parameters?

- I note that annotations in the signature are stored in the
"method_info structure", whereas annotations in method bodies are
stored in "a Code attribute".
Is this an intentional difference? Or is this something that wasn't
updated yet? If it's intentional, I would find a heads-up useful,
maybe together with Fig. 1 that discusses the different categories.


page 3:
- I would put "for casts" and "for type tests" together. I would
mention that there are no runtime checks for these.

- In "for constructor invocation results":
In
   myVar . new @Tainted NestedClass
add "()" at the end.

- Point 4 already talks about receivers, before point 5 introduced the
concept and syntax for that. Maybe the order should be switched?

page 5:
- The last sentence of point 5 is the first to mention TYPE_USE. Would
it make sense to introduce the new ElementType constants earlier?

- Point 6 could mention that it uses ElementType.TYPE_PARAMETER

page 10:
- "Annotations that target instructions are _are_ those..."

- In "How Java SE 7 stores annotations" we are reminded that there are
both Runtime[In]VisibleParameterAnnotations and 
Runtime[In]VisibleAnnotations.
Would it help to highlight that in JSR 308 we do not add
Runtime[In]VisibleParameterTypeAnnotations and instead store such
annotations with the method?

page 11:
- The last paragraph of Section 3.1 should also have a reference to
Section 3.4 and give a similar overview as for the other sections.

page 12:
- Last sentence of Section 3.3 refers to generic type arguments and
arrays only. It should have the complete list with nested types, etc.

- Copy & paste mistake: Section 3.3.1 mentions
"type_parameter_bound_target" and "bound".

page 13:
- Section 3.3.2 could also mention that the index is 0-based, like
earlier and later subsections do (redundantly, I agree).

**********