Skip to content
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

JDK-8275308: Add valueOf(Runtime.Version) factory to SourceVersion #5973

Closed
wants to merge 12 commits into from

Conversation

jddarcy
Copy link
Member

@jddarcy jddarcy commented Oct 15, 2021

I wanted to get input on the design of API of the method before writing the tests.

The new factory method maps from a Runtime.Version object, a object which formally modes the version of the JDK, to a corresponding SourceVersion object.

Given the current histories of versioning of the JDK and SourceVersion this amount to mapping "JDK N" to RELEASE_N, for example, JDK 17 to RELEASE_17, etc. As mentioned in the API note, this could potentially change in the future if the release model changes. Runtime.Version has added in JDK 9, but earlier versions can be modeled. Note that no attempt is made to map "1.2" to RELEASE_2 and that since Runtime.Version grammar does not allow a leading 0 term, RELEASE_0 will not be returned by the new method.

Another design point: an out-of-range feature version is treated as an error so a Runtime.Version with a feature of 19 mapped in JDK 18 will fail with an IllegalArgumentException rather than saturating at RELEASE_18. If saturating would be more helpful for the envisioned use cases, I'm open to changing the spec accordingly.

CSR to follow once the API is nailed down.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8275308: Add valueOf(Runtime.Version) factory to SourceVersion

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/5973/head:pull/5973
$ git checkout pull/5973

Update a local copy of the PR:
$ git checkout pull/5973
$ git pull https://git.openjdk.java.net/jdk pull/5973/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5973

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/5973.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Oct 15, 2021

👋 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 openjdk bot added the rfr label Oct 15, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Oct 15, 2021

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

@openjdk openjdk bot added the compiler label Oct 15, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Oct 15, 2021

* {@code 17}, to the corresponding source version, {@code
* RELEASE_17}, is<br>:
*
* {@code SourceVersion.valueOf(Runtime.Version.parse(Integer.toString(17)))
Copy link
Member

@pavelrappo pavelrappo Oct 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* {@code SourceVersion.valueOf(Runtime.Version.parse(Integer.toString(17)))
* {@code SourceVersion.valueOf(Runtime.Version.parse(Integer.toString(17)))}

Copy link
Member

@pavelrappo pavelrappo Oct 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand that this PR is an invitation to a discussion, not the final solution. Still, it's easier to reason about API whose doc comments render properly.

Copy link
Member Author

@jddarcy jddarcy Oct 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doh! Sorry, fixed.

* @apiNote
* An expression to convert from an integer value, for example
* {@code 17}, to the corresponding source version, {@code
* RELEASE_17}, is<br>:
*
* {@code SourceVersion.valueOf(Runtime.Version.parse(Integer.toString(17)))}
Copy link
Contributor

@jonathan-gibbons jonathan-gibbons Oct 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, The use of integer value in the example is something of a distraction, and suggests a hole in the Runtime.Version API. IMO, a more likely use case is for code to have a string representing a version, perhaps found as an argument on the command line

@mbien
Copy link
Contributor

@mbien mbien commented Oct 16, 2021

although the new factory method is quite useful to convert from one version representation to another. Unfortunately, I don't think it would solve one particular use case (although a very niche one). NetBeans for example relies on javac for all its editor features and can optionally use a newer javac than the JDK has, on which it is running on. This allows to have java 17 editor features while starting NB on JDK 8 (for whatever reason).

That is why you can find code like in apache/netbeans@56cf1c5 all over the place since it has to check a feature version against an enum which might not be there. And that is why my first thought went to "wouldn't it be nice if we could just compare ordinals or ask SourceVersion for the feature version int". Calling version.feature() >= 11 would not require having access to the RELEASE_11 enum you would need to compare the version with.

Calling version.compareTo(SourceVersion.valueOf(Runtime.Version.parse("11")))) >= 0 wouldn't help a lot since it basically has to throw exceptions since it can't just make up enums.

But this is so niche that it is probably not worth considering.

NB could just put

   public static boolean supports(SourceVersion model, int featureVersionToCheck) {
       return model.ordinal() >= featureVersionToCheck;
   }

somewhere which will work until the unlikely event of the enum having non-feature versions before it needs to be changed. (or just keep doing the static initializer workaround)

@jonathan-gibbons
Copy link
Contributor

@jonathan-gibbons jonathan-gibbons commented Oct 16, 2021

Would it help to have a method that provides the inverse mapping

Runtime.Version SourceVersion.toRuntimeVersion()

@mbien
Copy link
Contributor

@mbien mbien commented Oct 16, 2021

@jonathan-gibbons I think that would help indeed. The conversion method would always be able to return a runtime version without throwing exceptions, which would make it a convenient way to access the feature version int. It seems to be also forward compatible to all (prob. unlikely) scenarios of SourceVersion adding non-feature-version enums.

@hns
Copy link
Member

@hns hns commented Oct 18, 2021

FWIW there is one place in javadoc that would also benefit from a SourceVersion to Runtime.Version conversion method. (That code would be less ugly if it used ordinal(), the idea was that it would fail more predictably this way if the version naming pattern was ever abandoned.)

private int getSourceVersionNumber() {
SourceVersion sourceVersion = configuration.docEnv.getSourceVersion();
// TODO it would be nice if this was provided by SourceVersion
String versionNumber = sourceVersion.name().substring(8);
assert SourceVersion.valueOf("RELEASE_" + versionNumber) == sourceVersion;
return Integer.parseInt(versionNumber);
}

@jddarcy
Copy link
Member Author

@jddarcy jddarcy commented Oct 20, 2021

In the latest push, stubbed in support for the reverse mapping from SourceVersion to a runtime version. For now, limiting the mapping to RELEASE_9 and higher since Runtime.Version was added in 9.

if (feature > Runtime.version().feature()) {
throw new IllegalArgumentException("No matching SourceVersion for " + rv);
} else {
Copy link
Contributor

@jonathan-gibbons jonathan-gibbons Oct 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using Runtime.version() as a stand-in for the max SourceVersion seems non-obvious. Would it be better to use SourceVersion.latest().runtimeVersion() instead?

What about when running this API on JDK N-1?

Copy link
Contributor

@mbien mbien Oct 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if could be potentially dropped since valueOf("RELEASE_" + feature) is throwing IAE already.

Copy link
Member Author

@jddarcy jddarcy Oct 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated as suggested.

Copy link
Member Author

@jddarcy jddarcy Oct 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using Runtime.version() as a stand-in for the max SourceVersion seems non-obvious. Would it be better to use SourceVersion.latest().runtimeVersion() instead?

What about when running this API on JDK N-1?

Updated the supported range to go up to the feature corresponding to SourceVersion.latest().

Copy link
Contributor

@jonathan-gibbons jonathan-gibbons Oct 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this affects the doc comment and CSR.

Copy link
Member Author

@jddarcy jddarcy Oct 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right; I think the the current spec

 * ... If the runtime version's {@linkplain
 * Runtime.Version#feature() feature} is greater than the feature
 * of the {@linkplain #runtimeVersion() runtime version} of the
 * {@linkplain #latest() latest source version}, an {@code
 * IllegalArgumentException} is thrown.

does reflect that.

@hns
Copy link
Member

@hns hns commented Oct 22, 2021

In the latest push, stubbed in support for the reverse mapping from SourceVersion to a runtime version. For now, limiting the mapping to RELEASE_9 and higher since Runtime.Version was added in 9.

Thanks for adding the reverse mapping! Is there a problem with Source.Version representing a version earlier than 9, other than that it was added in that release? Supporting earlier versions would be very useful for javadoc, where we support versions starting from 7.

@openjdk openjdk bot added rfr and removed rfr labels Oct 25, 2021
@jddarcy
Copy link
Member Author

@jddarcy jddarcy commented Oct 25, 2021

In the latest push, stubbed in support for the reverse mapping from SourceVersion to a runtime version. For now, limiting the mapping to RELEASE_9 and higher since Runtime.Version was added in 9.

Thanks for adding the reverse mapping! Is there a problem with Source.Version representing a version earlier than 9, other than that it was added in that release? Supporting earlier versions would be very useful for javadoc, where we support versions starting from 7.

Added support back to 6, the release the javax.lang.model API was added. Strictly speaking release 6, 7, and 8 aren't modeled by Runtime.Version, but just using the feature value can be reasonable.

I don't think it is worthwhile to formally map, for instance, RELEASE_5 to something like RuntimeVersion.parse("5.0") or RuntimeVersion.parse("5.0.0") to try to model the versioning scheme used then.

@jddarcy
Copy link
Member Author

@jddarcy jddarcy commented Oct 25, 2021

In the latest push, stubbed in support for the reverse mapping from SourceVersion to a runtime version. For now, limiting the mapping to RELEASE_9 and higher since Runtime.Version was added in 9.

Thanks for adding the reverse mapping! Is there a problem with Source.Version representing a version earlier than 9, other than that it was added in that release? Supporting earlier versions would be very useful for javadoc, where we support versions starting from 7.

Changed to RELEASE 6 and later in the latest push; the javax.lang.model API was added in JDK 6.

@mbien
Copy link
Contributor

@mbien mbien commented Oct 25, 2021

would the doc need @since 18 tags?

@jddarcy
Copy link
Member Author

@jddarcy jddarcy commented Oct 25, 2021

/csr needed

@jddarcy
Copy link
Member Author

@jddarcy jddarcy commented Oct 25, 2021

CSR now available for review: https://bugs.openjdk.java.net/browse/JDK-8275888

@openjdk openjdk bot added the csr label Oct 25, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Oct 25, 2021

@jddarcy this pull request will not be integrated until the CSR request JDK-8275888 for issue JDK-8275308 has been approved.

@jddarcy
Copy link
Member Author

@jddarcy jddarcy commented Oct 25, 2021

would the doc need @since 18 tags?

Sure; added.

@jddarcy
Copy link
Member Author

@jddarcy jddarcy commented Oct 27, 2021

Any more comments on this PR?

* {@return the latest source version that is usable under the
* runtime version argument} If the runtime version's {@linkplain
* Runtime.Version#feature() feature} is greater than the feature
* of the {@linkplain #runtimeVersion() runtime version} of the
* {@linkplain #latest() latest source version}, an {@code
* IllegalArgumentException} is thrown.
Copy link
Contributor

@jonathan-gibbons jonathan-gibbons Oct 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@openjdk openjdk bot removed the csr label Oct 28, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Oct 28, 2021

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

8275308: Add valueOf(Runtime.Version) factory to SourceVersion

Reviewed-by: jjg

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 master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential 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.

@openjdk openjdk bot added the ready label Oct 28, 2021
@jddarcy
Copy link
Member Author

@jddarcy jddarcy commented Oct 28, 2021

/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Oct 28, 2021

Going to push as commit 48f3fca.

@openjdk openjdk bot closed this Oct 28, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Oct 28, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Oct 28, 2021

@jddarcy Pushed as commit 48f3fca.

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

@jddarcy jddarcy deleted the JDK-8275308 branch Dec 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler integrated
5 participants