JDK-8312418: Add Elements.getEnumConstantBody #14939

jddarcy · 2023-07-19T20:49:06Z

Trivial implementation to get feedback on the proposed API.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue
Change requires CSR request JDK-8313276 to be approved

Issues

JDK-8312418: Add Elements.getEnumConstantBody (Enhancement - P4)
JDK-8313276: Add Elements.getEnumConstantBody (CSR)

Reviewers

Vicente Romero (@vicente-romero-oracle - Reviewer) ⚠️ Review applies to 095eedb6

Contributors

Jan Lahoda <jlahoda@openjdk.org>

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/14939/head:pull/14939
$ git checkout pull/14939

Update a local copy of the PR:
$ git checkout pull/14939
$ git pull https://git.openjdk.org/jdk.git pull/14939/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 14939

View PR using the GUI difftool:
$ git pr show -t 14939

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/14939.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2023-07-19T20:50:13Z

👋 Welcome back darcy! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2023-07-19T20:52:03Z

@jddarcy The following label will be automatically applied to this pull request:

compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

jddarcy · 2023-07-19T21:47:48Z

/csr needed

openjdk · 2023-07-19T21:49:44Z

@jddarcy has indicated that a compatibility and specification (CSR) request is needed for this pull request.

@jddarcy please create a CSR request for issue JDK-8312418 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.

jddarcy · 2023-08-23T02:18:41Z

Add code from @lahodaj and make the PR non-draft to continue the discussion.

jddarcy · 2023-08-23T02:19:46Z

/contributor add @lahodaj

openjdk · 2023-08-23T02:19:54Z

@jddarcy
Contributor Jan Lahoda <jlahoda@openjdk.org> successfully added.

mlbridge · 2023-08-23T02:45:51Z

Webrevs

pavelrappo

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

pavelrappo · 2023-08-23T08:40:05Z

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

+     * @implSpec
+     * The default implementation of this method returns {@code null}
+     * if the argument is an {@code enum} constant and throws an
+     * {@code IllegalArgumentException}  if it is not.


Nit:

Suggested change

* {@code IllegalArgumentException} if it is not.

* {@code IllegalArgumentException} if it is not.

pavelrappo · 2023-08-23T08:43:49Z

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

@@ -725,6 +728,37 @@ public boolean isAutomaticModule(ModuleElement module) {
        return (msym.flags() & Flags.AUTOMATIC_MODULE) != 0;
    }

+    @Override @DefinedBy(Api.LANGUAGE_MODEL)
+    public TypeElement getEnumConstantBody(VariableElement enumConstant) {
+        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.

pavelrappo · 2023-08-23T08:59:09Z

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

+                    switch (field.getKind()) {
+                    case FIELD         -> expectException(field);
+                    case ENUM_CONSTANT -> testEnumConstant(field, typeElt);
+                    default            -> throw new RuntimeException("Unexpected field kind seen");
+                    }


That's unusual indentation.

pavelrappo · 2023-08-23T09:05:34Z

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

+    private void expectException0(Supplier<TypeElement> supplier, String message) {
+        try {
+            var typeElement = supplier.get();
+            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.

pavelrappo · 2023-08-23T10:08:42Z

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

+
+    // Nested classes hosting a variety of different kinds of fields.
+
+    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.

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

jddarcy · 2023-08-23T21:02:23Z

A few explanatory notes on the changes in the printing processor. Consider an enum like

public enum BodyEnum {
    GOLGI(true) {
        public boolean isOrganelle() {return true;}
    },

    HEAVENLY(true) {
        public boolean isCelestial() {return true;}
    };

    private BodyEnum(boolean predicate) {
        this.predicate = predicate;
    }

    private boolean predicate;
    public static int field = 42;
    public void method() {return;}
}

which uses enum constants with bodies. If this enum class is printed from a class file, but not source code, the output is like:

public enum BodyEnum permits BodyEnum$1, BodyEnum$2 {

  GOLGI,
  HEAVENLY;

  private boolean predicate;
  public static int field;

  public static BodyEnum[] values();

  public static BodyEnum valueOf(java.lang.String arg0);

  private BodyEnum(boolean arg0);

  public void method();
}

This does reflect how javac currently lowers an enum to a class file, but the permits information is not really helpful and is better to be elided, as the extra check now does.

If one goes out of the way to explicitly request printing of the anonymous class used for an enum contanst, the output is similar to:

$ jdk-21/bin/javac -Xprint 'BodyEnum$1'
new BodyEnum(java.lang.String arg0,
  int arg1,
  boolean arg2) {

  public boolean isOrganelle();
}

While this is reasonable for a general anonymous class, it isn't helpful for an enum body and I think it is best to just have no output in this case.

Generally we haven't tested the exact printing processor output and I think it is best if we continue that policy here.

jddarcy · 2023-08-23T21:25:23Z

To address some concerns raised off-list by @jonathan-gibbons, I modified the semantics of the default method to always throw some exception, either an IllegalArgumentException or an UnsupportedOperationException. This is (slightly) a more precise approximation to the "real" behavior of the method than always throwing UnsupportedOperationException.

vicente-romero-oracle · 2023-08-24T18:03:53Z

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

+            boolean elementSeen = false;
+
+            for (TypeElement typeElt : ElementFilter.typesIn(typeRoot.getEnclosedElements()) ) {
+                System.out.println("Testing type " + typeElt);


should the println invocations be removed?

This may be a matter of style, but logs like this inside tests may help to determine where is the problem when the test fails on a CI server.

pavelrappo · 2023-08-24T22:15:44Z

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

+     */
+    default TypeElement getEnumConstantBody(VariableElement enumConstant) {
+        switch(enumConstant.getKind()) {
+        case ENUM_CONSTANT -> throw new UnsupportedOperationException();


Hm... Most retrofitted methods in this API return null or false, not throw UnsupportedOperationException. The only other exception seems to be Elements.getFileObjectOf. Is this the new approach, or it just does make more sense for these two particular methods?

I think it depends whether a null result is permitted by the specification of the method. For this method, while null would be the correct result most of the time, it is exactly the wrong result when it is most interesting, and so a non-exception result would be misleading.

I see your point and agree with it: the default implementation will return null if called for an enum constant with the class body. That said, isn't, say, Elements.getTypeElement(ModuleElement, CharSequence) in a similar situation? But it returns null.

vicente-romero-oracle

looks good to me

openjdk · 2023-09-06T15:06:18Z

@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:

8312418: Add Elements.getEnumConstantBody

Co-authored-by: Jan Lahoda <jlahoda@openjdk.org>
Reviewed-by: vromero

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 55 new commits pushed to the master branch:

dccf670: 8306632: Add a JDK Property for specifying DTD support
a62c48b: 8315891: java/foreign/TestLinker.java failed with "error occurred while instantiating class TestLinker: null"
9559e03: 8315578: PPC builds are broken after JDK-8304913
e409d07: 8315696: SignedLoggerFinderTest.java test failed
ab6a87e: 8314838: 3 compiler tests ignore vm flags
b3dfc39: 8315930: Revert "8315220: Event NativeLibraryLoad breaks invariant by taking a stacktrace when thread is in state _thread_in_native"
ebc718f: 8315818: vmTestbase/nsk/jvmti/Allocate/alloc001/alloc001.java fails on libgraal
4a6bd81: 8315854: G1: Remove obsolete comment in G1ReclaimEmptyRegionsTask
c664f1c: 8307352: AARCH64: Improve itable_stub
8ddf9ea: 8315686: G1: Disallow evacuation of marking regions in a Prepare Mixed gc
... and 45 more: https://git.openjdk.org/jdk/compare/1f4cdb327f46085d3134d1d1164fccac35904566...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

jddarcy · 2023-09-08T22:05:11Z

/integrate

openjdk · 2023-09-08T22:06:27Z

Going to push as commit 578ded4.
Since your change was applied there have been 55 commits pushed to the master branch:

dccf670: 8306632: Add a JDK Property for specifying DTD support
a62c48b: 8315891: java/foreign/TestLinker.java failed with "error occurred while instantiating class TestLinker: null"
9559e03: 8315578: PPC builds are broken after JDK-8304913
e409d07: 8315696: SignedLoggerFinderTest.java test failed
ab6a87e: 8314838: 3 compiler tests ignore vm flags
b3dfc39: 8315930: Revert "8315220: Event NativeLibraryLoad breaks invariant by taking a stacktrace when thread is in state _thread_in_native"
ebc718f: 8315818: vmTestbase/nsk/jvmti/Allocate/alloc001/alloc001.java fails on libgraal
4a6bd81: 8315854: G1: Remove obsolete comment in G1ReclaimEmptyRegionsTask
c664f1c: 8307352: AARCH64: Improve itable_stub
8ddf9ea: 8315686: G1: Disallow evacuation of marking regions in a Prepare Mixed gc
... and 45 more: https://git.openjdk.org/jdk/compare/1f4cdb327f46085d3134d1d1164fccac35904566...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-09-08T22:06:33Z

@jddarcy Pushed as commit 578ded4.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

JDK-8312418: Add Elements.getEnumClassBody

20f8856

jddarcy marked this pull request as draft July 19, 2023 20:49

openjdk bot added the compiler compiler-dev@openjdk.org label Jul 19, 2023

Respond to pre-review feedback.

5c9c97e

jddarcy changed the title ~~JDK-8312418: Add Elements.getEnumClassBody~~ JDK-8312418: Add Elements.getEnumConstandBody Jul 19, 2023

jddarcy changed the title ~~JDK-8312418: Add Elements.getEnumConstandBody~~ JDK-8312418: Add Elements.getEnumConstantBody Jul 19, 2023

openjdk bot added the csr Pull request needs approved CSR before integration label Jul 19, 2023

jddarcy added 10 commits July 20, 2023 13:57

Merge branch 'master' into JDK-8312418

a1d4400

Merge branch 'master' into JDK-8312418

2f76389

Add test; check-point implementation.

fae4402

appease jcheck

fd74129

Augment test.

bb0ee8a

Correct test predicate.

01ad1e4

Merge branch 'master' into JDK-8312418

5c7a5b2

Merge branch 'master' into JDK-8312418

183e6bc

Merge in patch from Jan.

72ca5eb

Appease jcheck.

3684d30

jddarcy marked this pull request as ready for review August 23, 2023 02:18

openjdk bot added the rfr Pull request is ready for review label Aug 23, 2023

Update printing processor.

c8eedff

pavelrappo reviewed Aug 23, 2023

View reviewed changes

vicente-romero-oracle reviewed Aug 23, 2023

View reviewed changes

src/java.compiler/share/classes/javax/lang/model/util/Elements.java Show resolved Hide resolved

Change policy of default method.

41c2448

Reorder javadoc tags.

8f69e47

vicente-romero-oracle reviewed Aug 24, 2023

View reviewed changes

openjdk bot removed the csr Pull request needs approved CSR before integration label Aug 24, 2023

Refactor as suggested by jjg.

095eedb

pavelrappo reviewed Aug 24, 2023

View reviewed changes

vicente-romero-oracle approved these changes Sep 6, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Sep 6, 2023

jddarcy added 2 commits September 6, 2023 17:44

Merge branch 'master' into JDK-8312418

dc51ed9

Update copyright.

0ac5b0c

openjdk bot added the integrated Pull request has been integrated label Sep 8, 2023

openjdk bot closed this Sep 8, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Sep 8, 2023

jddarcy deleted the JDK-8312418 branch October 26, 2024 19:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JDK-8312418: Add Elements.getEnumConstantBody #14939

JDK-8312418: Add Elements.getEnumConstantBody #14939

jddarcy commented Jul 19, 2023 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jul 19, 2023

openjdk bot commented Jul 19, 2023

jddarcy commented Jul 19, 2023

openjdk bot commented Jul 19, 2023

jddarcy commented Aug 23, 2023

jddarcy commented Aug 23, 2023

openjdk bot commented Aug 23, 2023

mlbridge bot commented Aug 23, 2023 •

edited

Loading

pavelrappo left a comment

pavelrappo Aug 23, 2023

pavelrappo Aug 23, 2023

pavelrappo Aug 23, 2023

pavelrappo Aug 23, 2023

pavelrappo Aug 23, 2023

jddarcy commented Aug 23, 2023

jddarcy commented Aug 23, 2023

vicente-romero-oracle Aug 24, 2023

lahodaj Sep 6, 2023

pavelrappo Aug 24, 2023 •

edited

Loading

jonathan-gibbons Aug 24, 2023

pavelrappo Aug 24, 2023

vicente-romero-oracle left a comment

openjdk bot commented Sep 6, 2023 •

edited

Loading

jddarcy commented Sep 8, 2023

openjdk bot commented Sep 8, 2023

openjdk bot commented Sep 8, 2023

	* {@code IllegalArgumentException} if it is not.
	* {@code IllegalArgumentException} if it is not.


		// Nested classes hosting a variety of different kinds of fields.

		private static enum Body {

JDK-8312418: Add Elements.getEnumConstantBody #14939

JDK-8312418: Add Elements.getEnumConstantBody #14939

Conversation

jddarcy commented Jul 19, 2023 • edited by openjdk bot Loading

Progress

Issues

Reviewers

Contributors

Reviewing

Webrev

bridgekeeper bot commented Jul 19, 2023

openjdk bot commented Jul 19, 2023

jddarcy commented Jul 19, 2023

openjdk bot commented Jul 19, 2023

jddarcy commented Aug 23, 2023

jddarcy commented Aug 23, 2023

openjdk bot commented Aug 23, 2023

mlbridge bot commented Aug 23, 2023 • edited Loading

Webrevs

pavelrappo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jddarcy commented Aug 23, 2023

jddarcy commented Aug 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavelrappo Aug 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vicente-romero-oracle left a comment

Choose a reason for hiding this comment

openjdk bot commented Sep 6, 2023 • edited Loading

jddarcy commented Sep 8, 2023

openjdk bot commented Sep 8, 2023

openjdk bot commented Sep 8, 2023

jddarcy commented Jul 19, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Aug 23, 2023 •

edited

Loading

pavelrappo Aug 24, 2023 •

edited

Loading

openjdk bot commented Sep 6, 2023 •

edited

Loading