Re: nestmates spec open issues

[David Holmes]

Hi John,

On 26/10/2017 2:03 PM, John Rose wrote:

On Oct 25, 2017, at 8:54 PM, David Holmes  wrote:

Though I still feel uncomfortable lying about the nest-host. I don't see what 
usecases would be served by doing that. Other than informational uses, I can't 
see a reason to actually identify the nest-host which would not be impacted by 
lying about that identity. The only truly useful query is for nestmate access 
and that doesn't need to expose the identity of the nest-host and 
conservatively rejects access if anything goes wrong.


I'm sympathetic to this, but that implies that getNestHost should return null
rather than this for a non-nesty class.  I think it's more valuable that 
getNestHost
return 'this' for non-nesty classes, and therefore also for broken-nest classes.


Ok. The JVMS is proposed to state:

"A class or interface without a NestHost attribute belongs to the nest 
hosted by itself."


That covers top-level classes and all types compiled with an earlier 
version of javac. If we consider a class with an unresolvable, or 
otherwise invalid, NestHost attribute, as-if it had no NestHost 
attribute, then returning "this" is the natural response for that case.



Bottom line for my personal preferences:

- hasNestMateAccess: never throws, always returns true or false

Yes!


Okay.


- getNestHost(): throw if the host isn't there or else a membership validation 
check fails

See above.


getNestHost() won't throw, as above.

Suggestion: also add getValidatedNestHost() that can throw?


- getNestMembers(): throw if any nest member isn't there or a membership 
validation check fails

As I said before, I don't mind this choice.  Especially if we have 
hasNestMateAccess.


Okay. getNestMembers() will throw.

Now the question is what to throw? Dan's spec change for the access 
checking allows resolution/linkage errors (and VME) to pass through, and 
specifies IllegalAccessError for an invalid nest membership claim. The 
IAE makes sense in the context of an access check - though could also be 
ICCE.


But in the context of the reflection API, getNestMembers or 
getValidatedNestHost, IllegalAccessError doesn't really make sense - 
we're not trying to access anything. ICCE seems more fitting there.


Thanks,
David


Cheers,
David

Re: nestmates spec open issues

[David Holmes]

On 27/10/2017 12:02 PM, John Rose wrote:
On Oct 25, 2017, at 11:11 PM, David Holmes > wrote:


Please don't throw IAE that case.  I don't want to bikeshed exception 
types,

but I think we have a strong precedent for using ICCE when encountering
a questionable classfile configuration (one that shouldn't have come 
out of

javac).  Note also the ICCE <: VME, so it just gets passed through.


You must have missed Dan's update regarding access checks. He's 
already proposed to throw IAE - and I've implemented it. :)


http://cr.openjdk.java.net/~dlsmith/nestmates.html



Actually, IllegalAccessError is not too terrible, although
I'd prefer ICCE for reasons stated.

I read IAE as "IllegalArgumentException", which explains
the warmth of my response.  Why did I think you meant
IllegalArgumentException?  *That* is unexplainable.


Sorry - it never occurred to me that IAE was an overloaded acronym. :)

David



— John

Re: nestmates spec open issues

[David Holmes]

On 26/10/2017 1:11 PM, John Rose wrote:

On Oct 25, 2017, at 8:07 PM, John Rose  wrote:


On Oct 25, 2017, at 3:33 PM, Remi Forax > wrote:


getClasses() throws an exception but getAnnotations() skips unavailable 
annotations.

that said, i'm not against throwing in this case.


I'm not against throwing either, but I think scrubbing (like annotations)
is a little better, because it is more robust about retaining partial results.


Aren't annotations a different situation because annotations are well 
known to potentially not be available at runtime? A missing annotation 
does not necessarily reflect an error in the runtime environment of an 
application. Also AFAIK see from the jdk/java/lang/annotation/Missing/ 
test the throw/no-throw behaviour depends on exactly how the missing 
annotation is used.



Partial results are important if you are just asking about one or two
classes and don't care about their other nestmates.


How do you know to only ask about "one or two classes"? Sorry I'm not 
understanding the potential usecase here.If you've already identified 
specific classes of interest then you can call getNestHost on them.



And even if H.getNestMembers() throws because of some missing N1,
we have to be careful to allow N2.getNestHost() to return H, if we can
validate that N2 points to H and vice versa.  We don't want the validated
relation between N2 and H to be collateral damage to a failure of N1.


Absolutely not. The two queries would be completely unrelated in that 
regard.



(If H goes missing there is nothing we can do with N1 and N2, except
to assign each to its own 1-nest.  Which is OK with me.)


Then presumably we should do the same at the VM level instead of 
allowing resolution related exceptions to propagate as currently 
proposed? We could just throw IAE as we do if a nest-host validation 
check fails. Though a case can still be made to allow VME's to pass through.


Though I still feel uncomfortable lying about the nest-host. I don't see 
what usecases would be served by doing that. Other than informational 
uses, I can't see a reason to actually identify the nest-host which 
would not be impacted by lying about that identity. The only truly 
useful query is for nestmate access and that doesn't need to expose the 
identity of the nest-host and conservatively rejects access if anything 
goes wrong.


Bottom line for my personal preferences:

- hasNestMateAccess: never throws, always returns true or false
- getNestHost(): throw if the host isn't there or else a membership 
validation check fails
- getNestMembers(): throw if any nest member isn't there or a membership 
validation check fails


Cheers,
David

Re: nestmates spec open issues

[John Rose]

On Oct 25, 2017, at 8:07 PM, John Rose  wrote:
> 
> On Oct 25, 2017, at 3:33 PM, Remi Forax  > wrote:
>> 
>> getClasses() throws an exception but getAnnotations() skips unavailable 
>> annotations.
>> 
>> that said, i'm not against throwing in this case.
> 
> I'm not against throwing either, but I think scrubbing (like annotations)
> is a little better, because it is more robust about retaining partial results.
> Partial results are important if you are just asking about one or two
> classes and don't care about their other nestmates.

And even if H.getNestMembers() throws because of some missing N1,
we have to be careful to allow N2.getNestHost() to return H, if we can
validate that N2 points to H and vice versa.  We don't want the validated
relation between N2 and H to be collateral damage to a failure of N1.

(If H goes missing there is nothing we can do with N1 and N2, except
to assign each to its own 1-nest.  Which is OK with me.)

Re: nestmates spec open issues

[John Rose]

On Oct 25, 2017, at 3:33 PM, Remi Forax  wrote:
> 
> getClasses() throws an exception but getAnnotations() skips unavailable 
> annotations.
> 
> that said, i'm not against throwing in this case.

I'm not against throwing either, but I think scrubbing (like annotations)
is a little better, because it is more robust about retaining partial results.
Partial results are important if you are just asking about one or two
classes and don't care about their other nestmates.

Re: nestmates spec open issues

[Remi Forax]

getClasses() throws an exception but getAnnotations() skips unavailable 
annotations.

that said, i'm not against throwing in this case.

Rémi

- Mail original -
> De: "David Holmes" <david.hol...@oracle.com>
> À: "Valhalla Expert Group Observers" 
> <valhalla-spec-observ...@openjdk.java.net>, "John Rose" 
> <john.r.r...@oracle.com>,
> "Brian Goetz" <brian.go...@oracle.com>
> Cc: "valhalla-spec-experts" <valhalla-spec-experts@openjdk.java.net>
> Envoyé: Mercredi 25 Octobre 2017 23:52:31
> Objet: Re: nestmates spec open issues

> On 26/10/2017 2:51 AM, John Rose wrote:
>> On Oct 25, 2017, at 8:39 AM, Brian Goetz <brian.go...@oracle.com> wrote:
>>>
>>> John Rose proposed:
>>>> a) Class.getnestHost() - defaults to itself if there is a resolution error
>>>
>>> (or if there is no Nest attribute)
>> 
>> (yes)
>> 
>>>> b) Class.getNestMembers() -  returns full nest, fallback of self if any
>>>> resolution errors including it lists a nestHost that
>>>>  does not list it.
>>>
>>> For consistency with the classfile representation, this should probably 
>>> omit the
>>> host class.
>> 
>> (yes, that's my preference, although the other way is not too terrible)
>> 
>>>
>>>> [editor notes:
>>>> -  full statically defined nest from classfile attribute? As distinct from 
>>>> full
>>>> dynamically
>>>> currently loaded nest - right?
>>>
>>> I would prefer that this return only the static members, which is consistent
>>> with the design center for reflection -- reflection over classfiles.
>> 
>> yes yes yes; the reflection should reflect what's in the class-file (or a
>> resolvable subset), not everything in the VM's knowledge
>> 
>> I like Remi's idea of scrubbing the list (of reflected nestmates) of bad 
>> actors
>> rather than clearing the list if there are *any* bad actors.
>> 
>> Reminder:  bad actors are an edge case, not a normal case.
>> Question:  what do we do in the exactly parallel case for getInnerClasses?  
>> Do
>> we scrub bad actors?  Nullify the result?  Throw?  (Probably throw.)
> 
> Throw.
> 
> David
> 
>> — John

Re: nestmates spec open issues

[David Holmes]

On 26/10/2017 2:51 AM, John Rose wrote:

On Oct 25, 2017, at 8:39 AM, Brian Goetz  wrote:


John Rose proposed:

a) Class.getnestHost() - defaults to itself if there is a resolution error


(or if there is no Nest attribute)


(yes)


b) Class.getNestMembers() -  returns full nest, fallback of self if any 
resolution errors including it lists a nestHost that
 does not list it.


For consistency with the classfile representation, this should probably omit 
the host class.


(yes, that's my preference, although the other way is not too terrible)




[editor notes:
-  full statically defined nest from classfile attribute? As distinct from full 
dynamically
currently loaded nest - right?


I would prefer that this return only the static members, which is consistent 
with the design center for reflection -- reflection over classfiles.


yes yes yes; the reflection should reflect what's in the class-file (or a 
resolvable subset), not everything in the VM's knowledge

I like Remi's idea of scrubbing the list (of reflected nestmates) of bad actors 
rather than clearing the list if there are *any* bad actors.

Reminder:  bad actors are an edge case, not a normal case.
Question:  what do we do in the exactly parallel case for getInnerClasses?  Do 
we scrub bad actors?  Nullify the result?  Throw?  (Probably throw.)


Throw.

David


— John

Re: nestmates spec open issues

[John Rose]

On Oct 25, 2017, at 8:39 AM, Brian Goetz  wrote:
> 
> John Rose proposed:
>> a) Class.getnestHost() - defaults to itself if there is a resolution error
> 
> (or if there is no Nest attribute)

(yes)

>> b) Class.getNestMembers() -  returns full nest, fallback of self if any 
>> resolution errors including it lists a nestHost that
>> does not list it.
> 
> For consistency with the classfile representation, this should probably omit 
> the host class.

(yes, that's my preference, although the other way is not too terrible)

> 
>> [editor notes:
>> -  full statically defined nest from classfile attribute? As distinct from 
>> full dynamically
>> currently loaded nest - right?
> 
> I would prefer that this return only the static members, which is consistent 
> with the design center for reflection -- reflection over classfiles.

yes yes yes; the reflection should reflect what's in the class-file (or a 
resolvable subset), not everything in the VM's knowledge

I like Remi's idea of scrubbing the list (of reflected nestmates) of bad actors 
rather than clearing the list if there are *any* bad actors.

Reminder:  bad actors are an edge case, not a normal case.
Question:  what do we do in the exactly parallel case for getInnerClasses?  Do 
we scrub bad actors?  Nullify the result?  Throw?  (Probably throw.)

— John

Re: nestmates spec open issues

[Brian Goetz]

John Rose proposed:

a) Class.getnestHost() - defaults to itself if there is a resolution error


(or if there is no Nest attribute)

b) Class.getNestMembers() -  returns full nest, fallback of self if 
any resolution errors including it lists a nestHost that

    does not list it.


For consistency with the classfile representation, this should probably 
omit the host class.



[editor notes:
-  full statically defined nest from classfile attribute? As distinct 
from full dynamically

currently loaded nest - right?


I would prefer that this return only the static members, which is 
consistent with the design center for reflection -- reflection over 
classfiles.

Re: nestmates spec open issues

[Remi Forax]

> De: "Karen Kinnear" <karen.kinn...@oracle.com>
> À: "valhalla-spec-experts" <valhalla-spec-experts@openjdk.java.net>
> Envoyé: Mercredi 25 Octobre 2017 16:45:51
> Objet: nestmates spec open issues

> ...
> 2. Core reflection APIs [ https://bugs.openjdk.java.net/browse/JDK-8188075 |
> https://bugs.openjdk.java.net/browse/JDK-8188075 ]

> John Rose proposed:
> a) Class.getnestHost() - defaults to itself if there is a resolution error
> b) Class.getNestMembers() - returns full nest, fallback of self if any
> resolution errors including it lists a nestHost that
> does not list it.

> [editor notes:
> - full statically defined nest from classfile attribute? As distinct from full
> dynamically
> currently loaded nest - right?
> - looks like John is proposing resolving the names, not just returning 
> strings,
> and skipping any resolution failures

I do not like swallowing exceptions, it always make things harder to debug, so 
counter proposition: 

Class Class.getNestHost() 
if there is no attribute, return this, throw aTypeNotPresentException if either 
the NestHost can not be loaded or the NestHost doesn't list this as its member. 

Class[] Class.getNestMembers() 
scrub all members that are either not found or or not valid (not the same 
NestHost) from the array and always add this to the array. 

... 

> thanks,
> Karen

cheers, 
Rémi