Re: RFR: JDK-8274848: LambdaMetaFactory::metafactory on REF_invokeSpecial impl method has incorrect behavior [v4]

[Dan Smith]

On Thu, 28 Oct 2021 20:26:47 GMT, Mandy Chung  wrote:

>> Classes compiled prior to the nestmate support will generate 
>> `REF_invokeSpecial` if the implementation method is a private instance 
>> method.   Since a lambda proxy class is a hidden class, a nestmate of the 
>> host class, it can invoke the private implementation method but it has to 
>> use `REF_invokeVirtual` or `REF_invokeInterface`.   In order to support the 
>> old classes running on the new runtime, LMF implementation adjusts the 
>> reference kind from `REF_invokeSpecial` to 
>> `REF_invokeVirtual/REF_invokeInterface`. 
>> 
>> This PR fixes the check properly to ensure the adjustment is only made if 
>> the implementation method is private method in the host class.
>
> Mandy Chung has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   convert to TestNG test

Marked as reviewed by dlsmith (Committer).

Ahhh, much nicer. Thanks for updating the test.

-

PR: https://git.openjdk.java.net/jdk/pull/5901

Re: RFR: JDK-8274848: LambdaMetaFactory::metafactory on REF_invokeSpecial impl method has incorrect behavior [v2]

[Dan Smith]

On Tue, 12 Oct 2021 16:21:33 GMT, Mandy Chung  wrote:

>> Classes compiled prior to the nestmate support will generate 
>> `REF_invokeSpecial` if the implementation method is a private instance 
>> method.   Since a lambda proxy class is a hidden class, a nestmate of the 
>> host class, it can invoke the private implementation method but it has to 
>> use `REF_invokeVirtual` or `REF_invokeInterface`.   In order to support the 
>> old classes running on the new runtime, LMF implementation adjusts the 
>> reference kind from `REF_invokeSpecial` to 
>> `REF_invokeVirtual/REF_invokeInterface`. 
>> 
>> This PR fixes the check properly to ensure the adjustment is only made if 
>> the implementation method is private method in the host class.
>
> Mandy Chung has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   remove filelist which was added accidentally

Source changes look good.

The test seems like way too much overhead for this small thing. Looks like a 
lot of the ASM code is just to verify that javac generates the test case you 
expect? I'd suggest invoking the LMF API directly instead, testing both private 
and non-private `invokespecial` MethodHandles, just making sure the rules can 
be used without error.

-

PR: https://git.openjdk.java.net/jdk/pull/5901

Integrated: 8268192: LambdaMetafactory with invokespecial causes VerificationError

[Dan Smith]

On Mon, 7 Jun 2021 23:58:44 GMT, Dan Smith  wrote:

> Small bug fix.

This pull request has now been integrated.

Changeset: 58ba48b7
Author:    Dan Smith 
URL:   
https://git.openjdk.java.net/jdk/commit/58ba48b7b88eff359683aa3271c48b18f1973282
Stats: 27 lines in 3 files changed: 7 ins; 12 del; 8 mod

8268192: LambdaMetafactory with invokespecial causes VerificationError

Reviewed-by: psandoz, mchung

-

PR: https://git.openjdk.java.net/jdk/pull/4403

Re: RFR: 8268192: LambdaMetafactory with invokespecial causes VerificationError [v2]

[Dan Smith]

On Tue, 8 Jun 2021 02:23:09 GMT, liach  
wrote:

>> Dan Smith has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Clean up validation of implKind REF_invokeSpecial
>
> src/java.base/share/classes/java/lang/invoke/InnerClassLambdaMetafactory.java 
> line 191:
> 
>> 189: useImplMethodHandle = 
>> (Modifier.isProtected(implInfo.getModifiers()) &&
>> 190:!VerifyAccess.isSamePackage(implClass, 
>> implInfo.getDeclaringClass())) ||
>> 191:implKind == H_INVOKESPECIAL;
> 
> Won't this make regular private instance method calls use condy as well, as 
> they are invokespecial?

See this code from the `AbstractValidatingLambdaMetafactory` constructor:


case REF_invokeSpecial:
// JDK-8172817: should use referenced class here, but we don't 
know what it was
this.implClass = implInfo.getDeclaringClass();
this.implIsInstanceMethod = true;

// Classes compiled prior to dynamic nestmate support invokes a 
private instance
// method with REF_invokeSpecial.
//
// invokespecial should only be used to invoke private nestmate 
constructors.
// The lambda proxy class will be defined as a nestmate of 
targetClass.
// If the method to be invoked is an instance method of 
targetClass, then
// convert to use invokevirtual or invokeinterface.
if (targetClass == implClass && 
!implInfo.getName().equals("")) {
this.implKind = implClass.isInterface() ? 
REF_invokeInterface : REF_invokeVirtual;
} else {
this.implKind = REF_invokeSpecial;
}
break;


We turn all same-class invocations into `invokevirtual`. (And all `` 
invocations have kind `newInvokeSpecial`, mentioning them here is actually 
useless.)

-

PR: https://git.openjdk.java.net/jdk/pull/4403

Re: RFR: 8268192: LambdaMetafactory with invokespecial causes VerificationError [v2]

[Dan Smith]

On Tue, 8 Jun 2021 15:17:44 GMT, Dan Smith  wrote:

>> src/java.base/share/classes/java/lang/invoke/InnerClassLambdaMetafactory.java
>>  line 191:
>> 
>>> 189: useImplMethodHandle = 
>>> (Modifier.isProtected(implInfo.getModifiers()) &&
>>> 190:!VerifyAccess.isSamePackage(implClass, 
>>> implInfo.getDeclaringClass())) ||
>>> 191:implKind == H_INVOKESPECIAL;
>> 
>> Won't this make regular private instance method calls use condy as well, as 
>> they are invokespecial?
>
> See this code from the `AbstractValidatingLambdaMetafactory` constructor:
> 
> 
> case REF_invokeSpecial:
> // JDK-8172817: should use referenced class here, but we 
> don't know what it was
> this.implClass = implInfo.getDeclaringClass();
> this.implIsInstanceMethod = true;
> 
> // Classes compiled prior to dynamic nestmate support invokes 
> a private instance
> // method with REF_invokeSpecial.
> //
> // invokespecial should only be used to invoke private 
> nestmate constructors.
> // The lambda proxy class will be defined as a nestmate of 
> targetClass.
> // If the method to be invoked is an instance method of 
> targetClass, then
> // convert to use invokevirtual or invokeinterface.
> if (targetClass == implClass && 
> !implInfo.getName().equals("")) {
> this.implKind = implClass.isInterface() ? 
> REF_invokeInterface : REF_invokeVirtual;
> } else {
> this.implKind = REF_invokeSpecial;
> }
> break;
> 
> 
> We turn all same-class invocations into `invokevirtual`. (And all `` 
> invocations have kind `newInvokeSpecial`, mentioning them here is actually 
> useless.)

Actually, I think I'll fix up this code and the comment...

-

PR: https://git.openjdk.java.net/jdk/pull/4403

Re: RFR: 8268192: LambdaMetafactory with invokespecial causes VerificationError [v2]

[Dan Smith]

On Tue, 8 Jun 2021 15:27:04 GMT, Dan Smith  wrote:

>> Small bug fix.
>
> Dan Smith has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Clean up validation of implKind REF_invokeSpecial

src/java.base/share/classes/java/lang/invoke/AbstractValidatingLambdaMetafactory.java
 line 160:

> 158: // method with REF_invokeSpecial. Newer classes use 
> REF_invokeVirtual or
> 159: // REF_invokeInterface, and we can use that instruction 
> in the lambda class.
> 160: if (targetClass == implClass) {

The name `` can't appear here. It's only used by `newInvokeSpecial`.

src/java.base/share/classes/java/lang/invoke/InnerClassLambdaMetafactory.java 
line 189:

> 187: // to invoke directly. (javac prefers to avoid this situation by
> 188: // generating bridges in the target class)
> 189: useImplMethodHandle = 
> (Modifier.isProtected(implInfo.getModifiers()) &&

Switched from `!Modifier.isPublic` to `Modifier.isProtected`. Access errors 
from the caller Lookup should already be caught by validation when the 
`implementation` is cracked. Protected and `super` calls are the only cases 
where the access from the nestmate is not equivalent.

-

PR: https://git.openjdk.java.net/jdk/pull/4403

Re: RFR: 8268192: LambdaMetafactory with invokespecial causes VerificationError [v2]

[Dan Smith]

> Small bug fix.

Dan Smith has updated the pull request incrementally with one additional commit 
since the last revision:

  Clean up validation of implKind REF_invokeSpecial

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4403/files
  - new: https://git.openjdk.java.net/jdk/pull/4403/files/74c346a1..149fb55b

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=4403=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4403=00-01

  Stats: 8 lines in 1 file changed: 0 ins; 4 del; 4 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4403.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4403/head:pull/4403

PR: https://git.openjdk.java.net/jdk/pull/4403

RFR: 8268192: LambdaMetafactory with invokespecial causes VerificationError

[Dan Smith]

Small bug fix.

-

Commit messages:
 - Fix merge markup
 - Merge branch 'master' into 8268192
 - Merge branch '8174222' into 8268192
 - Fix stray renaming
 - Apply renamings to LambdaProxyClassArchive
 - Address reviewer comments
 - Turn back on test case that was failing
 - Merge branch '8174222' into 8268192
 - Fix accidentally commented-out parts of test
 - Merge branch '8174222' into 8268192
 - ... and 7 more: https://git.openjdk.java.net/jdk/compare/fc08af58...74c346a1

Changes: https://git.openjdk.java.net/jdk/pull/4403/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4403=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8268192
  Stats: 19 lines in 2 files changed: 7 ins; 8 del; 4 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4403.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4403/head:pull/4403

PR: https://git.openjdk.java.net/jdk/pull/4403

Integrated: 8174222: LambdaMetafactory: validate inputs and improve documentation

[Dan Smith]

On Thu, 3 Jun 2021 20:03:53 GMT, Dan Smith  wrote:

> Standardizes and better specifies the errors thrown by LambdaMetafactory, 
> including for inputs that would not be generated by javac.
> 
> Does some renaming of core parameters/fields to better reflect their purpose.
> 
> In the implementation, stops using ACC_BRIDGE for any methods, which is not a 
> good fit for what these methods do (they don't delegate to each other, but 
> all invoke the same implementation method).

This pull request has now been integrated.

Changeset: fc08af58
Author:Dan Smith 
URL:   
https://git.openjdk.java.net/jdk/commit/fc08af58cb0571ed375a7937aac7a951ba224644
Stats: 834 lines in 8 files changed: 397 ins; 33 del; 404 mod

8174222: LambdaMetafactory: validate inputs and improve documentation

Reviewed-by: mchung

-

PR: https://git.openjdk.java.net/jdk/pull/4346

Re: RFR: 8174222: LambdaMetafactory: validate inputs and improve documentation [v4]

[Dan Smith]

> Standardizes and better specifies the errors thrown by LambdaMetafactory, 
> including for inputs that would not be generated by javac.
> 
> Does some renaming of core parameters/fields to better reflect their purpose.
> 
> In the implementation, stops using ACC_BRIDGE for any methods, which is not a 
> good fit for what these methods do (they don't delegate to each other, but 
> all invoke the same implementation method).

Dan Smith has updated the pull request incrementally with two additional 
commits since the last revision:

 - Fix stray renaming
 - Apply renamings to LambdaProxyClassArchive

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4346/files
  - new: https://git.openjdk.java.net/jdk/pull/4346/files/b8b4ac14..9d722edf

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=4346=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4346=02-03

  Stats: 94 lines in 4 files changed: 0 ins; 0 del; 94 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4346.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4346/head:pull/4346

PR: https://git.openjdk.java.net/jdk/pull/4346

Re: RFR: 8174222: LambdaMetafactory: validate inputs and improve documentation [v3]

[Dan Smith]

On Fri, 4 Jun 2021 22:09:39 GMT, Mandy Chung  wrote:

> Can you also rename the parameter names in 
> `java.lang.invoke.LambdaProxyClassArchive` methods to match the new ones?

Hmm, that expands the reach of the patch a bit—into native HotSpot code—but 
I've given it a try. Let me know if it looks like I've broken something. :-)

-

PR: https://git.openjdk.java.net/jdk/pull/4346

Re: RFR: 8174222: LambdaMetafactory: validate inputs and improve documentation [v3]

[Dan Smith]

On Fri, 4 Jun 2021 22:06:49 GMT, Mandy Chung  wrote:

>> Dan Smith has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Address reviewer comments
>
> src/java.base/share/classes/java/lang/invoke/LambdaMetafactory.java line 312:
> 
>> 310:  * @return a CallSite whose target can be used to perform capture, 
>> generating
>> 311:  * instances of the interface named by {@code factoryType}
>> 312:  * @throws LambdaConversionException If {@code caller} does not 
>> have full privilege
> 
> Suggestion:
> 
>  * @throws LambdaConversionException If {@code caller} does not have 
> {@linkplain Lookup#hasFullPrivilegeAccess full privilege}

It's hard to see in the diff, but this link just appeared in the javadoc a few 
lines above. Stylistically, doesn't seem like it should be linked again.

-

PR: https://git.openjdk.java.net/jdk/pull/4346

Re: RFR: 8174222: LambdaMetafactory: validate inputs and improve documentation [v2]

[Dan Smith]

On Fri, 4 Jun 2021 06:16:45 GMT, Peter Levart  wrote:

>> Dan Smith has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Fix accidentally commented-out parts of test
>
> src/java.base/share/classes/java/lang/invoke/AbstractValidatingLambdaMetafactory.java
>  line 69:
> 
>> 67: final boolean isSerializable; // Should the returned 
>> instance be serializable
>> 68: final Class[] interfaces;  // Additional interfaces 
>> to be implemented
>> 69: final MethodType[] bridges;   // Signatures of 
>> additional methods to bridge
> 
> If you are removing ACC_BRIDGE from additional generated methods, then 
> perhaps this parameter name could also be changed? `altMethods` (as 
> alternative methods perhaps?)

Yes, it was sort of a half-done move. You're right, we should rename these 
(and, more importantly, in the public LambdaMetafactory API). Done. Also 
renamed `interfaces` --> `altInterfaces`.

-

PR: https://git.openjdk.java.net/jdk/pull/4346

Re: RFR: 8174222: LambdaMetafactory: validate inputs and improve documentation [v3]

[Dan Smith]

> Standardizes and better specifies the errors thrown by LambdaMetafactory, 
> including for inputs that would not be generated by javac.
> 
> Does some renaming of core parameters/fields to better reflect their purpose.
> 
> In the implementation, stops using ACC_BRIDGE for any methods, which is not a 
> good fit for what these methods do (they don't delegate to each other, but 
> all invoke the same implementation method).

Dan Smith has updated the pull request incrementally with one additional commit 
since the last revision:

  Address reviewer comments

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4346/files
  - new: https://git.openjdk.java.net/jdk/pull/4346/files/4b8d0dab..b8b4ac14

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=4346=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4346=01-02

  Stats: 73 lines in 4 files changed: 0 ins; 0 del; 73 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4346.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4346/head:pull/4346

PR: https://git.openjdk.java.net/jdk/pull/4346

Re: RFR: 8174222: LambdaMetafactory: validate inputs and improve documentation [v2]

[Dan Smith]

On Fri, 4 Jun 2021 01:22:05 GMT, liach  
wrote:

>> Dan Smith has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Fix accidentally commented-out parts of test
>
> src/java.base/share/classes/java/lang/invoke/LambdaMetafactory.java line 547:
> 
>> 545: throw new IllegalArgumentException("argument has wrong 
>> type");
>> 546: }
>> 547: return type.cast(result);
> 
> Can we just use a `return (T) result` as there will always be a checked cast 
> on the caller's end, and this call seems redundant? The only shortcoming is 
> that the call will raise an unchecked warning that needs to be suppressed. Or 
> is this negligible after hotspot optimization?

Eh, the scale here is quite small (never more than, say, 20 items), and an 
instanceof + a cast is a routine coding style that I'd expect to be optimized 
away. Doesn't seem worth the maintenance noise of a `@SuppressWarnings`.

-

PR: https://git.openjdk.java.net/jdk/pull/4346

Re: RFR: 8174222: LambdaMetafactory: validate inputs and improve documentation [v2]

[Dan Smith]

On Fri, 4 Jun 2021 00:08:41 GMT, Mandy Chung  wrote:

>> Dan Smith has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Fix accidentally commented-out parts of test
>
> src/java.base/share/classes/java/lang/invoke/LambdaMetafactory.java line 457:
> 
>> 455:  * @return a CallSite whose target can be used to perform capture, 
>> generating
>> 456:  * instances of the interface named by {@code factoryType}
>> 457:  * @throws LambdaConversionException If {@code caller} does not 
>> have private
> 
> One additional comment:
> 
> The lookup access has been extended since 14 to include `MODULE` and 
> `ORIGINAL` access.  
> `Lookup::hasFullPrivilegeAccess` returns true if the lookup has `PRIVATE` and 
> `MODULE` which means that this lookup is not teleported from another module 
> via `Lookup::in` and `MethodHandles::privateLookupIn`.
> 
> What privilege do you expect the `caller` lookup should have?  I believe full 
> privilege access is the appropriate privilege.  The `ORIGINAL` access is 
> stricter as the lookup must be created from the original lookup class.
> 
> [1] shows what access a `Lookup` has when being produced via different APIs.
> 
> [1] 
> https://download.java.net/java/early_access/jdk17/docs/api/java.base/java/lang/invoke/MethodHandles.Lookup.html#access-modes

Thanks, yes I think `hasFullPrivilegeAccess` is what we want. I updated the 
javadoc, along with the actual check. Thanks for catching!

> test/jdk/java/lang/invoke/lambda/MetafactoryArgValidationTest.java line 2:
> 
>> 1: /*
>> 2:  * Copyright (c) 2019, Oracle and/or its affiliates. All rights reserved.
> 
> copyright year needs update.

Fixed. And I bumped it on the other files, too.

-

PR: https://git.openjdk.java.net/jdk/pull/4346

Re: RFR: 8174222: LambdaMetafactory: validate inputs and improve documentation [v2]

[Dan Smith]

> Standardizes and better specifies the errors thrown by LambdaMetafactory, 
> including for inputs that would not be generated by javac.
> 
> Does some renaming of core parameters/fields to better reflect their purpose.
> 
> In the implementation, stops using ACC_BRIDGE for any methods, which is not a 
> good fit for what these methods do (they don't delegate to each other, but 
> all invoke the same implementation method).

Dan Smith has updated the pull request incrementally with one additional commit 
since the last revision:

  Fix accidentally commented-out parts of test

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4346/files
  - new: https://git.openjdk.java.net/jdk/pull/4346/files/81db8e5e..4b8d0dab

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=4346=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4346=00-01

  Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4346.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4346/head:pull/4346

PR: https://git.openjdk.java.net/jdk/pull/4346

Re: RFR: 8174222: LambdaMetafactory: validate inputs and improve documentation

[Dan Smith]

On Thu, 3 Jun 2021 20:03:53 GMT, Dan Smith  wrote:

> Standardizes and better specifies the errors thrown by LambdaMetafactory, 
> including for inputs that would not be generated by javac.
> 
> Does some renaming of core parameters/fields to better reflect their purpose.
> 
> In the implementation, stops using ACC_BRIDGE for any methods, which is not a 
> good fit for what these methods do (they don't delegate to each other, but 
> all invoke the same implementation method).

CSR was completed months ago: JDK-8221255. Just never got around to pushing the 
changes. Since then, InnerClassLambdaMetafactory was changed significantly, but 
the the public API is unchanged.

In testing, I discovered one issue leading to an internal error: JDK-8268192. I 
commented out my test case and will follow up on that issue separately.

-

PR: https://git.openjdk.java.net/jdk/pull/4346

RFR: 8174222: LambdaMetafactory: validate inputs and improve documentation

[Dan Smith]

Standardizes and better specifies the errors thrown by LambdaMetafactory, 
including for inputs that would not be generated by javac.

Does some renaming of core parameters/fields to better reflect their purpose.

In the implementation, stops using ACC_BRIDGE for any methods, which is not a 
good fit for what these methods do (they don't delegate to each other, but all 
invoke the same implementation method).

-

Commit messages:
 - Remove use of ACC_BRIDGE
 - Comment out failing test cases, will fix separately
 - Fixes from merge
 - Merge branch 'master' into 8174222
 - 8174222: LambdaMetafactory: validate inputs and improve documentation

Changes: https://git.openjdk.java.net/jdk/pull/4346/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4346=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8174222
  Stats: 726 lines in 4 files changed: 397 ins; 33 del; 296 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4346.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4346/head:pull/4346

PR: https://git.openjdk.java.net/jdk/pull/4346

Re: Proposal for Decimal64 and Decimal128 value-based classes

[Dan Smith]

> On Mar 31, 2021, at 3:27 PM, Maurizio Cimadamore 
>  wrote:
> 
> What I'd be curious though, is if the @ValueBased annotation could be 
> enhanced to say _which_ primitive class default you want (and then javac 
> could enforce extra checks if you pick the "val" default). If something like 
> this was feasible (cc'ing Dan), maybe some of the friction here could be 
> removed?

You mean annotate a class with "pretend this class's name represents a value 
type" and then implement the associated null checks in javac, even though we 
don't actually have value types yet?

I'd expect that to run into a number of problems related to the fact that the 
language model hasn't actually been updated to include primitive classes or 
value types yet. Plus the lack of features like reference types (Foo.ref) would 
be limiting for programmers who need them. Plus binary incompatibility—value 
types need special encoding in class files, and those class files aren't legal 
yet; when they are, you risk mismatches.

In this case I think the straightforward approach of just completing and 
delivering the Valhalla features is better than trying to spin off a small 
taste of them early.

Integrated: 8257845: Integrate JEP 390

[Dan Smith]

On Sat, 5 Dec 2020 01:46:31 GMT, Dan Smith  wrote:

> Integration of [JEP 390](https://bugs.openjdk.java.net/browse/JDK-8249100).
> 
> Development has been broken into 5 tasks, each with its own JBS issue:
> - [JDK-8252180](https://bugs.openjdk.java.net/browse/JDK-8252180): Deprecate 
> wrapper class constructors for removal (rriggs)
> - [JDK-8254047](https://bugs.openjdk.java.net/browse/JDK-8254047): Revise 
> "value-based class" & apply to wrappers (dlsmith)
> - [JDK-8252181](https://bugs.openjdk.java.net/browse/JDK-8252181):Define & 
> apply annotation jdk.internal.ValueBased (rriggs)
> - [JDK-8252183](https://bugs.openjdk.java.net/browse/JDK-8252183): Add 'lint' 
> warning for @ValueBased classes (sadayapalam)
> - [JDK-8257027](https://bugs.openjdk.java.net/browse/JDK-8257027): Diagnose 
> synchronization on @ValueBased classes (lfoltan)
> 
> All changes have been previously reviewed and integrated into the [`jep390` 
> branch](https://github.com/openjdk/valhalla/tree/jep390) of the `valhalla` 
> repository. See the subtasks of the 5 JBS issues for these changes, including 
> discussion and links to reviews. (Reviewers: mchung, dlsmith, jlaskey, 
> rriggs, lfoltan, fparain, hseigel.)
> 
> CSRs have also been completed or are nearly complete:
> 
> - [JDK-8254324](https://bugs.openjdk.java.net/browse/JDK-8254324) for wrapper 
> class constructor deprecation
> - [JDK-8254944](https://bugs.openjdk.java.net/browse/JDK-8254944) for 
> revisions to "value-based class"
> - [JDK-8256023](https://bugs.openjdk.java.net/browse/JDK-8256023) for new 
> `javac` lint warnings
> 
> Here's an overview of the files changed:
> 
> - `src/hotspot`: implementing diagnostics when `monitorenter` is applied to 
> an instance of a class tagged with `jdk.internal.ValueBased`. This enhances 
> previous work that produced such diagnostics for the primitive wrapper 
> classes.
> 
> - `src/java.base/share/classes/java/lang`: deprecating for removal the 
> wrapper class constructors; revising the definition of "value-based class" in 
> `ValueBased.html` and the description used by linking classes; applying 
> "value-based class" to the primitive wrapper classes; marking value-based 
> classes with `@jdk.internal.ValueBased`.
> 
> - `src/java.base/share/classes/java/lang/constant`: no longer designating 
> these classes as "value-based", since they rely heavily on field inheritance.
> 
> - `src/java.base/share/classes/java/time`: revising the description used to 
> link to `ValueBased.html`; marking value-based classes with 
> `@jdk.internal.ValueBased`.
> 
> - `src/java.base/share/classes/java/util`: revising the description used to 
> link to `ValueBased.html`; marking value-based classes with 
> `@jdk.internal.ValueBased`.
> 
> - `src/java.base/share/classes/jdk/internal`: define the 
> `@jdk.internal.ValueBased` annotation.
> 
> - `src/java.management.rmi`: removing uses of wrapper class constructors.
> 
> - `src/java.xml`: removing uses of wrapper class constructors.
> 
> - `src/jdk.compiler`: implementing the `synchronization` lint category, which 
> reports attempts to synchronize on classes and interfaces annotated with 
> `@jdk.internal.ValueBased`.
> 
> - `src/jdk.incubator.foreign`: revising the description used to link to 
> `ValueBased.html`. (Applying `@jdk.internal.ValueBased` would require a 
> special module export, which was not deemed necessary for an incubating API.)
> 
> - `src/jdk.internal.vm.compiler`: suppressing `javac` deprecation and 
> synchronization warnings in tests
> 
> - `src/jdk.jfr`: supplementary changes for HotSpot diagnostics
> 
> - `test`: testing new `javac` and HotSpot behavior; removing usages of 
> deprecated wrapper class constructors from tests, or suppressing deprecation 
> warnings; revising the description used to link to `ValueBased.html`.

This pull request has now been integrated.

Changeset: 48d8650a
Author:Dan Smith 
URL:   https://git.openjdk.java.net/jdk/commit/48d8650a
Stats: 902 lines in 114 files changed: 489 ins; 121 del; 292 mod

8257845: Integrate JEP 390
8254047: [JEP 390] Revise "value-based class" & apply to wrappers
8252181: [JEP 390] Define & apply annotation jdk.internal.ValueBased
8252183: [JEP 390] Add 'lint' warning for @ValueBased classes
8257027: [JEP 390] Diagnose synchronization on @ValueBased classes
8252180: [JEP 390] Deprecate wrapper class constructors for removal

Co-authored-by: Roger Riggs 
Co-authored-by: Srikanth Adayapalam 
Co-authored-by: Lois Foltan 
Reviewed-by: rriggs, hseigel, mchung, darcy

-

PR: https://git.openjdk.java.net/jdk/pull/1636

Re: RFR: 8252180: [JEP 390] Deprecate wrapper class constructors for removal

[Dan Smith]

On Sat, 5 Dec 2020 01:46:31 GMT, Dan Smith  wrote:

> Integration of [JEP 390](https://bugs.openjdk.java.net/browse/JDK-8249100).
> 
> Development has been broken into 5 tasks, each with its own JBS issue:
> - Deprecate wrapper class constructors for removal
> - Revise "value-based class" & apply to wrappers
> - Define & apply annotation jdk.internal.ValueBased
> - Add 'lint' warning for @ValueBased classes
> - Diagnose synchronization on @ValueBased classes
> 
> All changes have been previously reviewed and integrated into the [`jep390` 
> branch](https://github.com/openjdk/valhalla/tree/jep390) of the `valhalla` 
> repository. See the subtasks of the 5 JBS issues for these changes, including 
> discussion and links to reviews. (Reviewers: mchung, dlsmith, jlaskey, 
> rriggs, lfoltan, fparain, hseigel.)
> 
> CSRs have also been completed or are nearly complete:
> 
> - [JDK-8254324](https://bugs.openjdk.java.net/browse/JDK-8254324) for wrapper 
> class constructor deprecation
> - [JDK-8254944](https://bugs.openjdk.java.net/browse/JDK-8254944) for 
> revisions to "value-based class"
> - [JDK-8256023](https://bugs.openjdk.java.net/browse/JDK-8256023) for new 
> `javac` lint warnings
> 
> Here's an overview of the files changed:
> 
> - `src/hotspot`: implementing diagnostics when `monitorenter` is applied to 
> an instance of a class tagged with `jdk.internal.ValueBased`. This enhances 
> previous work that produced such diagnostics for the primitive wrapper 
> classes.
> 
> - `src/java.base/share/classes/java/lang`: deprecating for removal the 
> wrapper class constructors; revising the definition of "value-based class" in 
> `ValueBased.html` and the description used by linking classes; applying 
> "value-based class" to the primitive wrapper classes; marking value-based 
> classes with `@jdk.internal.ValueBased`.
> 
> - `src/java.base/share/classes/java/lang/constant`: no longer designating 
> these classes as "value-based", since they rely heavily on field inheritance.
> 
> - `src/java.base/share/classes/java/time`: revising the description used to 
> link to `ValueBased.html`; marking value-based classes with 
> `@jdk.internal.ValueBased`.
> 
> - `src/java.base/share/classes/java/util`: revising the description used to 
> link to `ValueBased.html`; marking value-based classes with 
> `@jdk.internal.ValueBased`.
> 
> - `src/java.base/share/classes/jdk/internal`: define the 
> `@jdk.internal.ValueBased` annotation.
> 
> - `src/java.management.rmi`: removing uses of wrapper class constructors.
> 
> - `src/java.xml`: removing uses of wrapper class constructors.
> 
> - `src/jdk.compiler`: implementing the `synchronization` lint category, which 
> reports attempts to synchronize on classes and interfaces annotated with 
> `@jdk.internal.ValueBased`.
> 
> - `src/jdk.incubator.foreign`: revising the description used to link to 
> `ValueBased.html`. (Applying `@jdk.internal.ValueBased` would require a 
> special module export, which was not deemed necessary for an incubating API.)
> 
> - `src/jdk.internal.vm.compiler`: suppressing `javac` deprecation and 
> synchronization warnings in tests
> 
> - `src/jdk.jfr`: supplementary changes for HotSpot diagnostics
> 
> - `test`: testing new `javac` and HotSpot behavior; removing usages of 
> deprecated wrapper class constructors from tests, or suppressing deprecation 
> warnings; revising the description used to link to `ValueBased.html`.

I've updated the description to provide an overview for those unfamiliar with 
this work. Reviewers are welcome, including those who have previously reviewed 
a subset of these changes.

-

PR: https://git.openjdk.java.net/jdk/pull/1636

RFR: 8252180: [JEP 390] Deprecate wrapper class constructors for removal

[Dan Smith]

Integration of JEP 390, addressing the following issues:

8252180: [JEP 390] Deprecate wrapper class constructors for removal
8254047: [JEP 390] Revise "value-based class" & apply to wrappers
8252181: [JEP 390] Define & apply annotation jdk.internal.ValueBased
8252183: [JEP 390] Add 'lint' warning for @ValueBased classes
8257027: [JEP 390] Diagnose synchronization on @ValueBased classes

See https://github.com/openjdk/valhalla/tree/jep390 for development history.

-

Commit messages:
 - Merge
 - 8257776: [valhalla:jep390] Add disclaimer about future changes to 
value-based classes
 - 8254275: [valhalla/jep390] Revise "value-based class" & apply to wrappers
 - 8252182: [JEP 390] Diagnose synchronization on @ValueBased classes
 - Merge
 - 8256663: [test] Deprecated use of new Double in jshell ImportTest
 - 8253962: Add @ValueBased to unmodifable Collection implementation classes
 - 8256002: Cleanup of Wrapper changes
 - Merge
 - 8254271: Development to deprecate wrapper class constructors for removal
 - ... and 2 more: https://git.openjdk.java.net/jdk/compare/93b6ab56...7c5e5bfe

Changes: https://git.openjdk.java.net/jdk/pull/1636/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1636=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8252180
  Stats: 902 lines in 114 files changed: 489 ins; 121 del; 292 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1636.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1636/head:pull/1636

PR: https://git.openjdk.java.net/jdk/pull/1636

Re: RFR: 8246778: Compiler implementation for Sealed Classes (Second Preview)

[Dan Smith]

On Wed, 2 Dec 2020 17:57:22 GMT, Mandy Chung  wrote:

> I suggest `Class::getPermittedSubclasses` to return a `non-null` array if 
> this `Class` is sealed, i.e. this class is derived from a `class` file with 
> the presence of `PermittedSubclasses` attribute regardless of its content 
> (the attribute could be empty or contains zero or more entries which is a 
> properly loaded permitted subtype.
> 
> If this `Class` is not sealed, `Class::getPermittedSubclasses` returns null 
> (see `Class::getRecordComponents` and some other reflection APIs as 
> precedence).

Agree, that seems reasonable. Often, methods in `Class` with an array return 
type default to an empty array, but `getRecordComponents` is a good example of 
returning `null` when an empty array is meaningful.

-

PR: https://git.openjdk.java.net/jdk/pull/1483

Re: RFR: 8174222: LambdaMetafactory: validate inputs and improve documentation

[Dan Smith]

> On Mar 21, 2019, at 8:58 AM, Brian Goetz  wrote:
> 
> +1 from me.
> 
> 
>> http://cr.openjdk.java.net/~dlsmith/8174222/webrev.00/
>> 
>> 
> 
> AbstractValidatingLMF
> -
> 
> I see you renamed most of the fields and params.  Most of these are 
> improvements, but it may be worth being more explicit in ambiguous cases.  
> For example, there are two method names, so perhaps instead of `methodName` 
> it should be `interfaceMethodName` to make it clear this is the SAM method 
> and not the implementation method.

Good idea, done.

> I presume the SecurityException is something that was always thrown, and we 
> are just documenting it now.  Should that be wrapped in a LCE instead?

Yep, it's inherent in trying to crack MethodHandles. It's unusual enough that 
it seems like it's worth passing through—it's sort of a configuration problem 
for users?

Worth thinking about how that limitation impacts the language. I didn't try to 
test it, because I don't totally get how to trigger it, but it seems like you 
could write a valid Java program with a method reference and end up triggering 
the exception at runtime? Is that bad?

> Accidentally lost the {@code ...} surrounding MethodHandle in class 
> description.

Fixed.

Re: RFR: 8174222: LambdaMetafactory: validate inputs and improve documentation

[Dan Smith]

> On Mar 20, 2019, at 11:22 PM, Mandy Chung  wrote:
> 
> On 3/20/19 9:03 PM, Dan Smith wrote:
>> http://cr.openjdk.java.net/~dlsmith/8174222/webrev.00/
> 
> AbstractValidatingLambdaMetafactory.java
> +throw new LambdaConversionException("implementation is not 
> direct or cannot be cracked");
> 
> It may help to print implementation method handle:
> 
>throw new LambdaConversionException(implementation + " is not direct or 
> cannot be cracked");

Not sure how useful the toString will be, but I suppose it's better than 
nothing. They seem to all start with "MethodHandle", so at least it will be 
clear what kind of object we're talking about. Done.

> If you mind the formatting, the text descripting @param seems 
> to be aligned with the first word in the description above it.
> I don't know if the webrev shows the whitespace properly
> you may want to check out line 90-93.

Yes, those need some extra indenting. Fixed.

> Where does SecurityException get thrown?

The call to 'revealDirect'. Added a comment.

> I think this needs a CSR.  metafactory and altMetafactory @throws 
> IAE, NPE and SecurityException.

Yes, agreed. Created here:
https://bugs.openjdk.java.net/browse/JDK-8221255

> The class description of LambdaMetafactory also promotes @implNote to the 
> spec.

Yep. Changed to an @apiNote, actually. It was wrong to treat it as an 
@implNote, because implementations don't get to decide which set of 
MethodHandles to support.

> Otherwise looks good.

Thanks!

RFR: 8174222: LambdaMetafactory: validate inputs and improve documentation

[Dan Smith]

http://cr.openjdk.java.net/~dlsmith/8174222/webrev.00/

Please review this update to java.lang.invoke.LambdaMetafactory. Adds some 
argument validation with tests; improves documentation; renames some variables.

JBS: https://bugs.openjdk.java.net/browse/JDK-8174222

—Dan

Re: RFR - JDK-8210717 String::detab, String::entab (Code Review)

[Dan Smith]

> On Sep 18, 2018, at 11:52 AM, Jim Laskey  wrote:
> 
> Please review the code for String::detab and String::entab. Used to expand 
> tabs into spaces, and spaces back to tabs.
> 
> webrev: http://cr.openjdk.java.net/~jlaskey/8210717/webrev/index.html
> jbs: https://bugs.openjdk.java.net/browse/JDK-8210717
> csr: https://bugs.openjdk.java.net/browse/JDK-8210718 
> <https://bugs.openjdk.java.net/browse/JDK-8210718>

For detab: here's an attempt at a brief formal description of the behavior, if 
it's useful.

"Replace each tab code point (U+0009) in the string with one or more space code 
points (U+0020). The number of spaces that replace a tab varies between 1 and 
{@code tabWidth}, whatever is necessary so that the last replacement space is 
the nth code point of the {@link #lines() line}, where {@code n % 
tabWidth == 0}."

---

For entab: I noticed that something like the following doesn't behave how you'd 
probably expect.

"int x = 23;".entab(4)
--> "\tint\tx =\t23;"

For other strings, the intent is ambiguous (are the first and last name two 
fields, or only one?):

"Dan Smith   daniel.smith".entab(4)
--> "Dan\tSmith\tdaniel.smith"

Seems like there are multiple possible approaches:
- Entab all spaces that align with a tab stop (current behavior)
- Only entab sequences of 2 or more spaces (implies 'tabWidth >= 2')
- Only entab prefix spaces

We can support all of these via different overloads or parameters, or we can 
choose the best one for most use cases. But I don't know that we really have 
pressing use cases—detab is the one that's easy to envision using. It might be 
best to just drop the method unless/until compelling use cases arise.

Re: JEP 293: Guidelines for JDK Command-Line Tool Options

[Dan Smith]

> On Jul 8, 2016, at 8:01 AM, Jonathan Gibbons <jonathan.gibb...@oracle.com> 
> wrote:
> 
> On 07/07/2016 11:07 PM, Dan Smith wrote:
>> A suggestion for clarifying the text: I was confused about whether "may" 
>> refers to choices that the tool developer is allowed to make, or choices 
>> that the end user is allowed to make.
>> 
>> For example: "Options can require an argument to be provided. For a 
>> long-form options, the argument may be separated from the option name by 
>> either white space or '='."
>> 
>> Is the idea that the tool should support '=' as an alias for white space, or 
>> that the tool choose which form it wants to support?  I'm guessing the 
>> latter, but I'm not totally confident in that guess.
>> 
>> —Dan
> 
> It means that, as you put it, "=" is an alias for white-space, so that you 
> can use either of the following forms:
> 
>--module-path my:module:path
>--module-path=my:module:path

Okay, good to know.  So I suggest updating the text to make clear that "may" 
means a choice the end user makes, and the tool is meant to support both.  
(Other uses of "may" or similar words are also potentially ambiguous and could 
use some extra scrutiny.)

—Dan

Re: JEP 293: Guidelines for JDK Command-Line Tool Options

[Dan Smith]

A suggestion for clarifying the text: I was confused about whether "may" refers 
to choices that the tool developer is allowed to make, or choices that the end 
user is allowed to make.

For example: "Options can require an argument to be provided. For a long-form 
options, the argument may be separated from the option name by either white 
space or '='."

Is the idea that the tool should support '=' as an alias for white space, or 
that the tool choose which form it wants to support?  I'm guessing the latter, 
but I'm not totally confident in that guess.

—Dan

Re: RFR 8159751: ObjectStreamClass: hide 'final' flag for anonymous classes

[Dan Smith]

> On Jun 23, 2016, at 3:21 PM, Roger Riggs  wrote:
>>> I would suggest javac to produce the same ACC_FINAL bits as it did before 
>>> for each -target version.
>>> (I know it looks like a hack, but a compatible one).
>>> Correct it for 9 and leave the SUID computation alone.
>>> 
>> Okay, so I interpret this to mean you think (2) is tolerable, at least where 
>> the two binaries for the anonymous class have different version numbers?
> yes, 
> 
> No change is needed to the computed SUID or ObjectStreamClass; it uses 
> whatever is in the class file.

Okay, thanks for the input.  Given the compatibility problems created by 
changing the serialization spec, my tentative plan is to close JDK-8159751 
"Won't Fix" and just address JDK-8129576 in the compiler, guarded by "-target 
9".

I'll get CCC approval for this approach before proceeding.

—Dan

Re: RFR 8159751: ObjectStreamClass: hide 'final' flag for anonymous classes

[Dan Smith]

> On Jun 23, 2016, at 2:04 PM, Roger Riggs <roger.ri...@oracle.com> wrote:
> 
> Hi Dan,
> 
> On 6/23/2016 2:53 PM, Dan Smith wrote:
>>> On Jun 23, 2016, at 11:37 AM, Roger Riggs <roger.ri...@oracle.com>
>>>  wrote:
>>> 
>>> Hi Dan,
>>> 
>>> I was concerned about inter-operation between versions because serialized 
>>> objects
>>> are passed between Java runtimes with different versions and both need to 
>>> compute
>>> the same serial version uid (if it is not explicitly declared). The older 
>>> java runtimes will
>>> compute the serial version uid without regard to your change.
>>> 
>>> If javac always turns on the ACC_FINAL bit and using -target 8 and then the 
>>> class file
>>> is executed in a Java 8 runtime, it seems like it would compute a different 
>>> value
>>> which would be in compatible change.  It might be safer to introduce this 
>>> change
>>> only with the new class file version number.
>>> 
>> Ugh, this is a good point.  Hadn't thought about that use case.
>> 
>> How should we compare these two compatibility risks?:
>> 
>> (1) Using identical class binaries on two sides of a pipe, but running 
>> different JDK versions, a class has different UIDs on the two sides.
>> 
> Go back to the rationale for not including ACC_FINAL in the modifiers used in 
> SUID?
> I'm not clear on that part (other than its a current bug, not spec compliant).
> Reflection could report final based on the 'implicitly' final spec for 
> anonymous classes.
> Or reflection needs to be classfile version aware.
> 
> It is simpler to just hash whatever bits are there and not modify them.
> It won't break because of compiler changes or re-compiles.

Rationale: javac would like to change the bits generated for anonymous inner 
classes -- specifically, by setting the ACC_FINAL bit, where it has been left 
unset in the past -- but doesn't want to disrupt clients who use serialization. 
 (Why not?  To be nice, mostly -- trying to avoid (2).)  If we can force these 
new classes to have the same UID as the old classes, then there's no problem.

Except, oops, we can't redefine the implicit UID definition without introducing 
interop problems between different JDK versions -- that's (1).

I don't think it's necessarily bad for ObjectStreamClass to manipulate the bits 
before hashing them -- we already mask out lots of flags, per the serialization 
spec -- but the problem here is that any change to the hashing behavior (for a 
class that legally exists with an older class file version) is going to 
introduce incompatibilities between different versions of the java.io API.

Changing the behavior of reflection doesn't help: you end up with the same 
problem, a single binary class having different UIDs depending on which version 
of the Java APIs is being used.  (Besides, using reflection to obfuscate the 
actual contents of a class file is something we should avoid.)

>> (2) Using class binaries compiled from different versions of javac on two 
>> sides of a pipe, but running the same JDK versions, a class has different 
>> UIDs on the two sides.
>> 
>> Your suggestion to use "-target" helps to reduce the likelihood of (1).  But 
>> there may still be classes that are broken in this scenario.  Some examples:
>> - Really old classes that had ACC_FINAL set (it has been turned on and off 
>> once or twice in javac over the years)
>> - Classes that target 9, but a bytecode rewriter converts to run on 8, 
>> without tweaking ACC_FINAL
>> 
> Not your problem, different rules apply to different class file versions, 
> tools need to respect/ deal with it.

There are no rules that say that the 'inner_class_access_flags' value for an 
anonymous inner class (as defined by Class.isAnonymousClass) must be ACC_FINAL, 
or must not be ACC_FINAL.  Both are totally fine, as far as the JVM is 
concerned.

The reason we want to change javac's behavior is because the *language 
specification* says the class is 'final', which ought to constrain the subset 
of anonymous inner classes that are generated by compilers for the Java 
language.

>> - Classes generated by other compilers (Eclipse doesn't use ACC_FINAL for 
>> anon inner classes at the moment, but what about, e.g., other languages?)
>> 
>> As for (2), fixing this javac bug without touching ObjectStreamClass will 
>> impact all Serializable anonymous inner classes -- a much wider net.  Then 
>> again, they've received fair warning: "the default serialVersionUID 
>> computation is highly sensitive to class details that may vary depending on 
>> compiler implementations"; and my u

Re: RFR 8159751: ObjectStreamClass: hide 'final' flag for anonymous classes

[Dan Smith]

> On Jun 23, 2016, at 11:37 AM, Roger Riggs  wrote:
> 
> Hi Dan,
> 
> I was concerned about inter-operation between versions because serialized 
> objects
> are passed between Java runtimes with different versions and both need to 
> compute
> the same serial version uid (if it is not explicitly declared). The older 
> java runtimes will
> compute the serial version uid without regard to your change.
> 
> If javac always turns on the ACC_FINAL bit and using -target 8 and then the 
> class file
> is executed in a Java 8 runtime, it seems like it would compute a different 
> value
> which would be in compatible change.  It might be safer to introduce this 
> change
> only with the new class file version number.

Ugh, this is a good point.  Hadn't thought about that use case.

How should we compare these two compatibility risks?:

(1) Using identical class binaries on two sides of a pipe, but running 
different JDK versions, a class has different UIDs on the two sides.

(2) Using class binaries compiled from different versions of javac on two sides 
of a pipe, but running the same JDK versions, a class has different UIDs on the 
two sides.

Your suggestion to use "-target" helps to reduce the likelihood of (1).  But 
there may still be classes that are broken in this scenario.  Some examples:
- Really old classes that had ACC_FINAL set (it has been turned on and off once 
or twice in javac over the years)
- Classes that target 9, but a bytecode rewriter converts to run on 8, without 
tweaking ACC_FINAL
- Classes generated by other compilers (Eclipse doesn't use ACC_FINAL for anon 
inner classes at the moment, but what about, e.g., other languages?)

As for (2), fixing this javac bug without touching ObjectStreamClass will 
impact all Serializable anonymous inner classes -- a much wider net.  Then 
again, they've received fair warning: "the default serialVersionUID computation 
is highly sensitive to class details that may vary depending on compiler 
implementations"; and my understanding of common practice is that users who 
serialize anonymous classes understand that they should have identical binaries 
on both sides.

So (1) is a much narrower problem, but also more serious: users can reasonably 
consider a failure to serialize/deserialize with identical class binaries to be 
a bug; they can't really complain about a new compiler version spitting out 
different bytes.

Thoughts?

—Dan

Re: RFR 8159751: ObjectStreamClass: hide 'final' flag for anonymous classes

[Dan Smith]

> On Jun 23, 2016, at 8:51 AM, Roger Riggs  wrote:
> 
> Hi Dan,
> 
> Is setting ACC_FINAL for anonymous inner classes a unique feature of the 
> latest bytecode version?
> (javac -target 9)
> 
> Was it always clear in class files with earlier bytecode versions?

This is a choice that the compiler makes, subject to the constraints of the 
Language Specification.  At the moment, ACC_FINAL does not get set*.  The 
outcome I expect from JDK-8129576 is to always set it, regardless of target 
version, since that's what the spec has always said.  I suppose we could 
consider guarding the change on target version, but anyway that wouldn't affect 
the behavior of serialization either way, since this patch causes it to always 
ignore ACC_FINAL.

(* Actually, we found a bug in which it sometimes gets set on the class's 
flags, but still remains unset in the flags provided by the InnerClasses 
attribute.  The latter is what you get from Class.getModifiers, so this bug 
hasn't affected serialization.)

—Dan

RFR 8159751: ObjectStreamClass: hide 'final' flag for anonymous classes

[Dan Smith]

Hi, making a change to serialization's computation of UID in order to avoid 
taking ACC_FINAL into account for anonymous inner classes.  Will also change 
the Serialization Specification.

https://bugs.openjdk.java.net/browse/JDK-8159751

The patch is small, inline below.  Not sure about the best testing strategy, 
but I decided to add a test that compares to a precomputed magic number.  I 
verified that this test fails after modifying javac (per JDK-8129576), but then 
succeeds after applying the change to ObjectStreamClass.  (I won't actually 
push JDK-8129576 until this is pushed, though.)

—Dan

# HG changeset patch
# Parent 7e7f37ae1a6d76c0374b0dc46709d8fde25456b1
diff -r 7e7f37ae1a6d -r f468e80bcd51 
src/java.base/share/classes/java/io/ObjectStreamClass.java
--- a/src/java.base/share/classes/java/io/ObjectStreamClass.javaTue Jun 
21 15:15:05 2016 -0700
+++ b/src/java.base/share/classes/java/io/ObjectStreamClass.javaWed Jun 
22 16:59:00 2016 -0600
@@ -1708,6 +1708,14 @@
  Modifier.INTERFACE | Modifier.ABSTRACT);
 
 /*
+ * compensate for anonymous classes which have historically
+ * not been marked ACC_FINAL
+ */
+if ((classMods & Modifier.FINAL) != 0 && cl.isAnonymousClass()) {
+classMods = classMods & ~Modifier.FINAL;
+}
+
+/*
  * compensate for javac bug in which ABSTRACT bit was set for an
  * interface only if the interface declared methods
  */
diff -r 7e7f37ae1a6d -r f468e80bcd51 
test/java/io/ObjectInputStream/AnonymousClassUID.java
--- /dev/null   Thu Jan 01 00:00:00 1970 +
+++ b/test/java/io/ObjectInputStream/AnonymousClassUID.java Wed Jun 22 
16:59:00 2016 -0600
@@ -0,0 +1,45 @@
+/*
+ * Copyright (c) 2016, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+import java.io.ObjectStreamClass;
+import java.io.Serializable;
+
+/*
+ * @test
+ * @bug 8159751
+ * @summary Ensure that an anonymous inner class has a stable UID
+ */
+public class AnonymousClassUID {
+
+public static void main(String[] args) {
+Class c = new Serializable() {
+int x = 0;
+public String m(Object arg) { return null; }
+public int m(Class arg) { return 1; }
+}.getClass();
+long uid = ObjectStreamClass.lookup(c).getSerialVersionUID();
+long expectedUID = -8213874395645447803L;
+if (uid != expectedUID) throw new AssertionError("Unexpected UID: " + 
uid);
+}
+
+}

Re: AbstractList etc. functionality as interfaces with default methods?

[Dan Smith]

 On May 14, 2015, at 7:24 AM, Remi Forax fo...@univ-mlv.fr wrote:
 
 On 05/14/2015 03:05 PM, Brian Goetz wrote:
 Not only is there a problem with modCount, but also with 
 equals/hashCode/toString.  You can’t define these Object methods in an 
 interface.
 They could be defined as static methods to delegate to. From API 
 consistency perspective, we have for example the following static methods 
 on primitive wrapper classes:
 Right.  We considered this during Lambda, but by the time we got here, we 
 concluded that this was mostly trading one downside for another.  It seemed 
 overwhelmingly likely that people would forget to override 
 equals/hashCode/toString in this case, and create collections that violated 
 the contract.
 
 
 The other problem is that it creates ambiguous method references,
 if you have a class or an interface like:
 class A {
  public static int hashCode(A a) { ... }
 }
 
 A::hashCode is ambiguous.

An easy solution to both of these problems is to use a different name.  We 
discussed this as a workaround when designing the no default 'hashCode' 
method restriction.

interface ListE {
...

/** Returns the hash code for this list.  The hash code of a list is 
defined as ... */
int hashCode();

/** Computes a hash code consistent with the specification of 'hashCode'. */
default int defaultHashCode() { ... }
}

(Details like whether it's static or instance, and what interface it lives in, 
are flexible.)

—Dan

Stream optimization suggestions

[Dan Smith]

I've submitted a couple of RFEs to support some simple optimizations on 
Streams.  I noticed both issues while writing some code that forced me to 
choose between cluttering up my straightforward, uniform Stream logic and 
eating the cost of linear operations that should be O(1).

JDK-8067969 Optimize Stream.count for SIZED Streams
JDK-8067971 Support optimizations for always-true and always-false Predicates

Certainly not high-priority, but would be nice for somebody working with 
java.util.stream to address someday.

—Dan

Re: Lambda-fied pattern matching

[Dan Smith]

 On Nov 19, 2014, at 4:10 PM, Remi Forax fo...@univ-mlv.fr wrote:
 
 and yes, please log a RFE.

RFE is here: https://bugs.openjdk.java.net/browse/JDK-8067973

Lambda-fied pattern matching

[Dan Smith]

Working with the pattern matching API, I noticed that it could be made a lot 
less clumsy with some lambdafication.

Here's the status quo:

Pattern p = Pattern.compile((\w)*, (\d)*, (\w)*);
for (String s : lines) {
Matcher m = p.matcher(str);
if (m.match(s)) {
System.out.println(m.group(1));
}
}

With a lambda-friendly API:

Pattern p = Pattern.compile(\d*, \d*, \d*);
for (String s : lines) {
p.match(str, r - System.out.println(r.group(1)));
}

The 'match' is declared as 'match(String, ConsumerMatchResult)'.  You could 
argue that the functional interface should be a Function rather than a 
Consumer; whatever.

Could also do 'matchFirst', 'matchAll' -- the latter eliminates even more 
boilerplate.

If considered useful, this could be added to String too:

str.match(\d*, \d*, \d*, r - System.out.println(r.group(1)));

Is this something that has been considered?  Should I file an RFE?

—Dan

Re: please review draft JEP: Convenience Factory Methods for Collections

[Dan Smith]

The motivation section ought to more directly survey what we have now:
- Collections.emptyList/emptySet/emptyMap
- Collections.singletonList/singleton/singletonMap
- Arrays.asList
- EnumSet.of (varargs, plus some overloads)
- Stream.empty and Stream.of (singleton plus varargs)
- Constructors that, by convention, always have a variant that constructs an 
empty collection and another to copy the contents of an existing collection

This helps to clarify that there are multiple goals:
- Provide some default methods in List/Set/Map for easier access to existing 
behavior (following the pattern of EnumSet)
- Add new functionality to create immutable Sets/Maps for 2 = n = small 
number.
- (Maybe?) varargs methods to create immutable Sets/Maps or arbitrary size.
- Provide an array-less implementation of immutable Lists for 2 = n = small 
number.

--

If a mutable collection is desired, the contents of an immutable collection 
can easily be copied into a collection type of the caller's choice. ... If 
there are sufficient use cases to support convenient creation of mutable 
collections, factory methods for particular mutable collection classes could be 
added.

Following the existing convention, it might make more sense to make these 
constructors:
ArrayList()
ArrayList(Collection? extends E)
ArrayList(E...) // this is new

--

Converting a mutable collection into an immutable one is more difficult.

In principle, if you've got a varargs List creation method, there's nothing to 
stop you from overloading it with an Collection/Iterable/Stream version.  Same 
functionality, although this might encourage users to think that larger 
collections will behave properly.  (You could pass in a large array to the 
varargs method, but presumably most users will be passing in small varargs 
lists.)

--

I was curious about other collections that might benefit.
- It turns out Queue and Deque are pretty useless as immutable data structures.
- SortedSet and SortedMap could be useful.
- Stream could probably be made consistent with the overloading scheme you 
settle on; EnumSet, too, if it's different.
- There might be some use cases for Iterator; likely doesn't carry its weight, 
though.

—Dan

On Jul 16, 2014, at 6:46 PM, Stuart Marks stuart.ma...@oracle.com wrote:

 Hi all,
 
 Please review this draft JEP for Convenience Factory Methods for Collections:
 
https://bugs.openjdk.java.net/browse/JDK-8048330
 
 Brief background: several times over the years there have been proposals to 
 add collection literals to the language. The most recent round of this was 
 in regard to JEP 186, a research JEP to explore this topic. That effort was 
 concluded by Brian Goetz, as summarized in this email:
 
http://mail.openjdk.java.net/pipermail/lambda-dev/2014-March/011938.html
 
 Essentially, the idea of adding collection literals to the language was set 
 aside in favor of adding some library APIs, not entirely unlike collection 
 literals, that make it more convenient to create collections. That's what 
 this proposal is.
 
 Share and enjoy,
 
 s'marks
 

Random access in ArrayDeque

[Dan Smith]

I noticed recently that the javac scanner is making use of ArrayList.remove(0) 
when it consumes a buffer.  Obviously this is an inefficient way to implement a 
buffer, so I thought I'd try to fix it [1].  ArrayDeque seems to provide just 
the behavior I need, with one fatal flaw: despite encoding its data with an 
array, the class exposes no random-access operations.  For lookahead, I need to 
be able to call get(int).

This seems to be a fairly common complaint [2][3].

I found an old bug requesting that ArrayDeque be enhanced to implement List 
[4], as well as a thread from 2010 that included a proof-of-concept 
ArrayDeque+List [5] and had a note from Martin Buchholz saying he regrets that 
ArrayDeque doesn't already implement List [6].

Is there any hope of ArrayDeque being enhanced in the near future to provide 
random access?  There's some risk that any such initiative would be derailed by 
a quest for an uber-collection.  I think a lot of people would be quite happy 
just to have a (trivial) 'get(int)' method added to ArrayDeque.

—Dan

[1] https://bugs.openjdk.java.net/browse/JDK-8033158
[2] http://en.wikipedia.org/wiki/Deque#Language_support
[3] https://www.google.com/#q=arraydeque+%22random+access%22
[4] https://bugs.openjdk.java.net/browse/JDK-6368844
[5] http://mail.openjdk.java.net/pipermail/core-libs-dev/2010-April/004038.html
[6] http://mail.openjdk.java.net/pipermail/core-libs-dev/2010-April/004031.html

RFR JDK-8022442: Fix unchecked warnings in HashMap

[Dan Smith]

Please review this warnings cleanup.

Bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8022442 (not yet 
visible)
Webrev: http://cr.openjdk.java.net/~dlsmith/8022442/webrev.00/

—Dan

Re: RFR JDK-8022442: Fix unchecked warnings in HashMap

[Dan Smith]

Update: some minor changes are necessary to LinkedHashMap to be compatible with 
the HashMap changes (HashMap is unchanged).

Webrev: http://cr.openjdk.java.net/~dlsmith/8022442/webrev.01/

I confirmed that the jdk_util tests pass.

—Dan

On Aug 6, 2013, at 2:11 PM, Dan Smith daniel.sm...@oracle.com wrote:

 Please review this warnings cleanup.
 
 Bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8022442 (not yet 
 visible)
 Webrev: http://cr.openjdk.java.net/~dlsmith/8022442/webrev.00/
 
 —Dan

Re: RFR JDK-8022442: Fix unchecked warnings in HashMap

[Dan Smith]

An intermediate result from an unnecessary refactoring.  Thanks for catching.

—Dan

On Aug 6, 2013, at 3:10 PM, Brent Christian brent.christ...@oracle.com wrote:

 Looks fine to me, though I'm not a Reviewer (but I think I contributed a 
 fair number of those warnings O:).
 
 One question: why was enext added?:
 
 1249  EntryK,V enext = (EntryK,V) newTable[i];
 1250  e.next = enext;
 
 Thanks,
 -Brent
 
 On 8/6/13 2:11 PM, Dan Smith wrote:
 Please review this warnings cleanup.
 
 Bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8022442 (not yet 
 visible)
 Webrev: http://cr.openjdk.java.net/~dlsmith/8022442/webrev.00/
 
 —Dan
 

Re: RFR JDK-8022442: Fix unchecked warnings in HashMap

[Dan Smith]

On Aug 6, 2013, at 2:43 PM, Remi Forax fo...@univ-mlv.fr wrote:

 On 08/06/2013 11:11 PM, Dan Smith wrote:
 Please review this warnings cleanup.
 
 Bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8022442 (not yet 
 visible)
 Webrev: http://cr.openjdk.java.net/~dlsmith/8022442/webrev.00/
 
 —Dan
 
 Hi Dan,
 I've seen that you have introduce a common super interface for entry and tree 
 node,
 I suppose that  you check that there is no performance regression.
 I wonder if an abstract class is not better than an interface because as far 
 as I know
 CHA implemented in hotspot doesn't work on interface
 (but I may be wrong, there is perhaps a special optimization for arrays).

To make sure I understand: your concern is that an aastore will be more 
expensive when assigning to a KeyValueData[] than to an Object[] (or even to 
SomeOtherClass[])?

For what it's worth, all assignments to table[i] are statically known to be 
safe.  E.g.:

EntryK,V next = (EntryK,V) e.next;
...
table[i] = next;

So surely a smart VM only does the check once?

Here are some other things that might be concerns, but don't apply here:
- interface method invocations: there are no methods in the interface to invoke
- checkcast to an interface: all the casts are to concrete classes (Entry, 
TreeBin, TreeNode)

(There are some unchecked casts from KeyValueData to KeyValueData with 
different type parameters, but I assume these don't cause any checkcasts.)

—Dan

Re: Code Review Request: More tests for 7184826: (reflect) Add support for Project Lambda concepts in core reflection

[Dan Smith]

Hi.

Per a request from Joel, I've taken a look at DefaultStaticTestData.  I don't 
really have the full context here, but I'm assuming that the annotations get 
translated into tests that guarantee 1) the result of Class.getMethods is 
exactly (no more -- excepting Object methods -- and no less) those methods 
named by MethodDesc annotations; and 2) the result of Class.getDeclaredMethods 
is exactly (no more, no less) those methods that are marked declared=YES.

The expected results seem accurate.  I would personally focus testing more on 
different inheritance shapes and less on different combinations of (unrelated) 
method declarations or presence/absence type variables (!?), but it's a valid 
test in any case.

There ought to be some testing for scenarios that javac won't generate, like 
conflicting default method declarations.

It's not clear to me why we're generating classes with lambda methods 
(TestIF16) and then not testing that reflection sees those methods.  Presumably 
they get filtered out by the testing logic. But, if so... what's the point of 
testing?  Really, I don't think lambdas belong here at all -- it's a completely 
orthogonal feature.

(Note: I'm not a Reviewer either, but don't mind providing feedback, especially 
on spec-related questions.)

—Dan

On Jul 22, 2013, at 7:12 AM, Amy Lu amy...@oracle.com wrote:

 Thank you Joel for all the valuable feedback.
 
 Test have been refactored and here comes the updated version:
 https://dl.dropboxusercontent.com/u/5812451/yl153753/7184826/webrev.01/index.html
 
 Thanks,
 Amy
 
 
 On 7/10/13 10:17 PM, Joel Borggren-Franck wrote:
 Hi Amy, Tristan,
 
 I'm not a Reviewer kind of reviewer, but I've started to look at the
 code and can sponsor this.
 
 Some comments on test/java/lang/reflect/Method/invoke/DefaultStaticTest.java:
 
 As there are a lot of non-public top-level classes perhaps this test
 should be in it own directory.
 
 It is very hard to read the data table:
 
 292 {interface1.class, 1, 1, new Object1(), new Object[]{},
 293 new Object[]{}, interface1, null},
 
 I believe you should move the methodsNum constant and the declMethods
 num constant to an annotation on the interface/class in question. For
 example:
 
 @MyTestData(numMethods=1, declMethods=1)
 41 interface interface1 {
 42 @DefaultMethod(isDefault = true)
 43 @StaticMethod(isStatic = false)
 44 @ReturnValue(value = interface1.defaultMethod)
 45 default String defaultMethod(){ return interface1.defaultMethod; };
 46 }
 
 That way it is much easier to inspect that the constants are right.
 
 The same can probably be done with the return values encoded.
 
 Instead of all these new Objects[] {} can't you create a field,
 
 Object [] EMPTY_PARAMETERS = new Object() {}
 
 and reuse it?
 
 That way it will be much easier to verify that the encoded test data is
 correct.
 
 I'll comment on the other tests shortly.
 
 cheers
 /Joel
 
 On 2013-06-13, Amy Lu wrote:
 This has been pending on review for long ... Please help to review.
 I also need a sponsor for this.
 
 Thank you very much.
 
 /Amy
 
 On 5/23/13 10:48 PM, Amy Lu wrote:
 Please help to review:
 
 More tests for 7184826: (reflect) Add support for Project Lambda
 concepts in core reflection
 
 This includes:
 1. improvement for existing tests with more situations
 2. Test for invoking interface default/static method by j.l.r.Method
 3. Test for invoking interface default/static method by MethodHandle
 
 https://dl.dropboxusercontent.com/u/5812451/yl153753/7184826/webrev.00/index.html
 
 
 Thanks,
 Amy
 
 

Re: Use of super in type parameters

[Dan Smith]

On Apr 17, 2013, at 6:11 PM, Zhong Yu zhong.j...@gmail.com wrote:

 On Wed, Apr 17, 2013 at 4:53 PM, Martin Buchholz marti...@google.com wrote:
 With the coming of lambda, it is more likely that people will be creating
 APIs with not quite correct generic types, as we are doing in
 
 I believe that we can tweak generic signatures in APIs without
 breaking calling codes.

It's one thing to make a change that won't break callers.  It's another to make 
a change that won't break implementers.

S super T foo(ListS, ListS)
 but we declare it today as, regrettably,
foo(List? super T, List? super T)

Unfortunately (without some sort of language change...), neither one of these 
signatures can be used to override the other.  So if somebody extends my class 
and overrides my signature, when I change it to the other one later, their code 
will break.

—Dan

Enumeration adapters in SE 8

[Dan Smith]

I was pointed to some comments on core-libs about adapting Enumerations to for 
loops in SE 8.  (Sorry for the new thread -- I wasn't subscribed.)  It turns 
out lambdas + extension methods will make this very easy.

In the API:

interface EnumerationE extends IteratorE {
  boolean hasMoreElements();
  E nextElement();
  boolean hasNext() default #hasMoreElements;
  E next() default #nextElement;
  void remove() default { throw new UnsupportedOperationException(); }
}

Note that Iterable is a function type -- a thunk producing an Iterator -- and 
so we can express Iterables with lambdas and method references.  It's becoming 
clear that a for loop should provide a target type for these things, so that 
will probably be part of the SE 8 feature set.

With a method reference:

HashtableString,Object h = …;
for (String s : h#keys) { … }

With a lambda:

for (String s : #{ - path().to().enumeration() }) { ... }

My preference is to also support Iterators directly in for loops; this may or 
may not make it in:

for (String s : path().to().enumeration()) { ... }

—Dan