-
Notifications
You must be signed in to change notification settings - Fork 5.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
8224243: Add implSpec's to AccessibleObject and seal Executable #4133
Conversation
/csr |
👋 Welcome back darcy! A progress list of the required criteria for merging this PR into |
@jddarcy this pull request will not be integrated until the CSR request JDK-8267506 for issue JDK-8224243 has been approved. |
Webrevs
|
I think this will require reaching out to Google Guava, I think its their Invokable API that extends AccessibleObject outside of the JDK. We ran this when doing the module system where we didn't initially take into account sub-classes that were outside of java.base. |
Mailing list message from Éamonn McManus on core-libs-dev: Thanks for the tip, Alan. Guava's public Invokable I looked in Google's code base and I found one other project that extends I do wonder what the purpose of the change would be. The constructor On Thu, 20 May 2021 at 10:36, Alan Bateman <alanb at openjdk.java.net> wrote: |
1 similar comment
Mailing list message from Éamonn McManus on core-libs-dev: Thanks for the tip, Alan. Guava's public Invokable I looked in Google's code base and I found one other project that extends I do wonder what the purpose of the change would be. The constructor On Thu, 20 May 2021 at 10:36, Alan Bateman <alanb at openjdk.java.net> wrote: |
Mailing list message from Joe Darcy on core-libs-dev: Hi ?amonn, On 5/20/2021 2:59 PM, ?amonn McManus wrote:
The intent of the constructor's text to me implies the class is not The original impetus for 8224243 was the implementation of certain If all the subclasses were in the JDK (which we know is not that case), HTH, -Joe |
1 similar comment
Mailing list message from Joe Darcy on core-libs-dev: Hi ?amonn, On 5/20/2021 2:59 PM, ?amonn McManus wrote:
The intent of the constructor's text to me implies the class is not The original impetus for 8224243 was the implementation of certain If all the subclasses were in the JDK (which we know is not that case), HTH, -Joe |
Mailing list message from Éamonn McManus on core-libs-dev: Hi Joe, I see that I blogged Still, if the main motivation of sealing it is to avoid adding @implSpec to Thanks, On Thu, 20 May 2021 at 15:47, Joe Darcy <joe.darcy at oracle.com> wrote:
|
1 similar comment
Mailing list message from Éamonn McManus on core-libs-dev: Hi Joe, I see that I blogged Still, if the main motivation of sealing it is to avoid adding @implSpec to Thanks, On Thu, 20 May 2021 at 15:47, Joe Darcy <joe.darcy at oracle.com> wrote:
|
Pushed an updated version of the fix that deprecates the AccessibleObject constructor, adds the @implSpec tags, and makes Executable a sealed class. |
* @throws NullPointerException {@inheritDoc} | ||
* @since 1.5 | ||
*/ | ||
@Override | ||
public <T extends Annotation> T getAnnotation(Class<T> annotationClass) { | ||
throw new AssertionError("All subclasses should override this method"); | ||
throw new IllegalStateException("All subclasses should override this method"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure that ISE is the most appropriate exception here because there isn't an alternative state that would accept the input. UOE might be better.
The new proposal to just seal Executable looks reasonable.
Mailing list message from Kasper Nielsen on core-libs-dev: On Fri, 21 May 2021 at 03:44, Joe Darcy <darcy at openjdk.java.net> wrote:
The implementation of setAccessible(boolean) is identical on both /Kasper |
Hi all, I am the author of picocli. I will look into moving away from extending So, thank you for reconsidering sealing (And many thanks to @kaspernielsen for making me aware of this issue!) |
Missed the part about only Executable being made sealed. But if Executable is sealed, setAccessible(boolean) could still be moved from Method/Constructor down to Executable. |
…ct`. There was no strong reason for this inheritance. Additionally, it is an accident that `AccessibleObject` is subclassable at all outside its class; its constructor is `protected` but should really have been package-private. That's an API mistake that was made in Java 1.2 and is hard to correct now, but the protected constructor is [being deprecated](openjdk/jdk#4133). (My related rant from 15 years ago is [here](https://www.artima.com/weblogs/viewpost.jsp?thread=164042).) `Invokable` also no longer implements `GenericDeclaration`. `Invokable` does continue to provide instance methods with the same signatures as the ones inherited from `AccessibleObject` and `GenericDeclaration`, as they stood in Java 8. This change is technically a breaking API change. In principle users could have assigned an instance of `Invokable` to a variable of type `AccessibleObject`. No code does that in Google's giant code base so it seems unlikely in practice. (In fact we only have a couple of dozen uses of `Invokable` overall.) Also, this API is `@Beta` so a hypothetical-but-very-unlikely breakage is acceptable. RELNOTES=`Invokable` no longer inherits from `AccessibleObject` or `GenericDeclaration`, though it continues to define instance methods with the same signatures as the formerly-inherited ones. This is technically a breaking API change to this `@Beta` API. We think it very unlikely that anyone is affected in practice. PiperOrigin-RevId: 375191535
…ct`. There was no strong reason for this inheritance. Additionally, it is an accident that `AccessibleObject` is subclassable at all outside its class; its constructor is `protected` but should really have been package-private. That's an API mistake that was made in Java 1.2 and is hard to correct now, but the protected constructor is [being deprecated](openjdk/jdk#4133). (My related rant from 15 years ago is [here](https://www.artima.com/weblogs/viewpost.jsp?thread=164042).) `Invokable` also no longer implements `GenericDeclaration`. `Invokable` does continue to provide instance methods with the same signatures as the ones inherited from `AccessibleObject` and `GenericDeclaration`, as they stood in Java 8. This change is technically a breaking API change. In principle users could have assigned an instance of `Invokable` to a variable of type `AccessibleObject`. No code does that in Google's giant code base so it seems unlikely in practice. (In fact we only have a couple of dozen uses of `Invokable` overall.) Also, this API is `@Beta` so a hypothetical-but-very-unlikely breakage is acceptable. RELNOTES=`Invokable` no longer inherits from `AccessibleObject` or `GenericDeclaration`, though it continues to define instance methods with the same signatures as the formerly-inherited ones. This is technically a breaking API change to this `@Beta` API. We think it very unlikely that anyone is affected in practice. PiperOrigin-RevId: 375191535
…ct`. There was no strong reason for this inheritance. Additionally, it is an accident that `AccessibleObject` is subclassable at all outside its class; its constructor is `protected` but should really have been package-private. That's an API mistake that was made in Java 1.2 and is hard to correct now, but the protected constructor is [being deprecated](openjdk/jdk#4133). (My related rant from 15 years ago is [here](https://www.artima.com/weblogs/viewpost.jsp?thread=164042).) `Invokable` also no longer implements `GenericDeclaration`. `Invokable` does continue to provide instance methods with the same signatures as the ones inherited from `AccessibleObject` and `GenericDeclaration`, as they stood in Java 8. This change is technically a breaking API change. In principle users could have assigned an instance of `Invokable` to a variable of type `AccessibleObject`. No code does that in Google's giant code base so it seems unlikely in practice. (In fact we only have a couple of dozen uses of `Invokable` overall.) Also, this API is `@Beta` so a hypothetical-but-very-unlikely breakage is acceptable. RELNOTES=`Invokable` no longer inherits from `AccessibleObject` or `GenericDeclaration`, though it continues to define instance methods with the same signatures as the formerly-inherited ones. This is technically a breaking API change to this `@Beta` API. We think it very unlikely that anyone is affected in practice. PiperOrigin-RevId: 375191535
Mailing list message from Joe Darcy on core-libs-dev: Hi Alan, On 5/22/2021 9:41 AM, Alan Bateman wrote:
Yeah; I was looking over which exception type to use. On the whole, UOE Thanks, -Joe |
Mailing list message from Joe Darcy on core-libs-dev: On 5/24/2021 2:05 AM, Kasper Nielsen wrote:
Perhaps; the caller sensitive nature of the methods may preclude or Thanks, -Joe |
…ct`. There was no strong reason for this inheritance. Additionally, it is an accident that `AccessibleObject` is subclassable at all outside its class; its constructor is `protected` but should really have been package-private. That's an API mistake that was made in Java 1.2 and is hard to correct now, but the protected constructor is [being deprecated](openjdk/jdk#4133). (My related rant from 15 years ago is [here](https://www.artima.com/weblogs/viewpost.jsp?thread=164042).) `Invokable` also no longer implements `GenericDeclaration`. `Invokable` does continue to provide instance methods with the same signatures as the ones inherited from `AccessibleObject` and `GenericDeclaration`, as they stood in Java 8. This change is technically a breaking API change. In principle users could have assigned an instance of `Invokable` to a variable of type `AccessibleObject`. No code does that in Google's giant code base so it seems unlikely in practice. (In fact we only have a couple of dozen uses of `Invokable` overall.) Also, this API is `@Beta` so a hypothetical-but-very-unlikely breakage is acceptable. RELNOTES=`Invokable` no longer inherits from `AccessibleObject` or `GenericDeclaration`, though it continues to define instance methods with the same signatures as the formerly-inherited ones. This is technically a breaking API change to this `@Beta` API. We think it very unlikely that anyone is affected in practice. PiperOrigin-RevId: 375566405
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The updated proposal looks good although the JBS issue should probably be renamed as the proposal is no longer to seal AccessibleObject.
@jddarcy This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be:
You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been no new commits pushed to the ➡️ To integrate this PR with the above commit message to the |
/integrate |
Conceptually, AccessbileObject is a sealed class with a protected constructor stating
With the language now supporting sealed classes, the AccessbileObject should be marked as sealed.
Executable and Field are the subclasses of AccessbileObject in the JDK; as Executable has subclasses, it is marked as non-sealed.
Please also review the corresponding CSR:
https://bugs.openjdk.java.net/browse/JDK-8224243
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/4133/head:pull/4133
$ git checkout pull/4133
Update a local copy of the PR:
$ git checkout pull/4133
$ git pull https://git.openjdk.java.net/jdk pull/4133/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 4133
View PR using the GUI difftool:
$ git pr show -t 4133
Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/4133.diff