RFR: JDK-8312418: Add Elements.getEnumConstantBody [v2]

[Pavel Rappo]

On Wed, 23 Aug 2023 09:14:40 GMT, Joe Darcy <darcy at openjdk.org> wrote:

>> Trivial implementation to get feedback on the proposed API.
>
> Joe Darcy has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Update printing processor.

Thanks for doing this; it looks like it also brings jdk.javadoc closer to fixing [JDK-8144631: No documentation is created for methods of enum constants' class bodies](https://bugs.openjdk.org/browse/JDK-8144631)

src/java.compiler/share/classes/javax/lang/model/util/Elements.java line 769:

> 767:      * The default implementation of this method returns {@code null}
> 768:      * if the argument is an {@code enum} constant and throws an
> 769:      * {@code IllegalArgumentException}  if it is not.

Nit:
Suggestion:

     * {@code IllegalArgumentException} if it is not.

src/jdk.compiler/share/classes/com/sun/tools/javac/model/JavacElements.java line 733:

> 731:     @Override @DefinedBy(Api.LANGUAGE_MODEL)
> 732:     public TypeElement getEnumConstantBody(VariableElement enumConstant) {
> 733:         if (enumConstant.getKind() == ElementKind.ENUM_CONSTANT) {

Inverting `if`-`else` (exception first) might be cleaner and can also save indentation if `else` is not used. But it's a matter of style.

test/langtools/tools/javac/processing/model/util/elements/TestGetEnumConstantBody.java line 73:

> 71:                     case ENUM_CONSTANT -> testEnumConstant(field, typeElt);
> 72:                     default            -> throw new RuntimeException("Unexpected field kind seen");
> 73:                     }

That's unusual indentation.

test/langtools/tools/javac/processing/model/util/elements/TestGetEnumConstantBody.java line 139:

> 137:         try {
> 138:             var typeElement = supplier.get();
> 139:             messager.printError(message, typeElement);

Testing for an expected exception is surprisingly subtle. Testing for an expected _unchecked_ exception is even more so.

If a test framework is not used, we should be very careful. Consider moving printError outside try-catch to exclude even the slightest possibility of false negative.

test/langtools/tools/javac/processing/model/util/elements/TestGetEnumConstantBody.java line 213:

> 211:     // Nested classes hosting a variety of different kinds of fields.
> 212: 
> 213:     private static enum Body {

I think it makes sense to expand testing, to make sure there are no surprises with constants that override or implement methods, as those are the important cases as per JLS 8.9.1, which you quote.

-------------

PR Review: https://git.openjdk.org/jdk/pull/14939#pullrequestreview-1591067681
PR Review Comment: https://git.openjdk.org/jdk/pull/14939#discussion_r1302685141
PR Review Comment: https://git.openjdk.org/jdk/pull/14939#discussion_r1302689705
PR Review Comment: https://git.openjdk.org/jdk/pull/14939#discussion_r1302709266
PR Review Comment: https://git.openjdk.org/jdk/pull/14939#discussion_r1302717119
PR Review Comment: https://git.openjdk.org/jdk/pull/14939#discussion_r1302796206