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.

Sign up for GitHub

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

Jump to bottom

Cherry-pick upstream fixes for switch expressions with pattern matching #1155

Draft
wants to merge 5 commits into
base: develop
Choose a base branch
from
Draft

Cherry-pick upstream fixes for switch expressions with pattern matching #1155

wants to merge 5 commits into from

Conversation

eholmer
Copy link

@eholmer eholmer commented Oct 21, 2024

Before this PR

After this PR

Cherry-pick the relevant commits to support java 21 style pattern matching in switch expressions.

Also setting java runtime to 21
==COMMIT_MSG==
Cherry-pick upstream changes to support java 21 style pattern matching
==COMMIT_MSG==

Possible downsides?

eholmer1 and others added 4 commits October 19, 2024 17:09
@eholmer1
bump java
a67bad9
@cushon @eholmer1
Remove tests for handling of extra semi-colons
e658a4f 
javac no longer accepts these, see https://bugs.openjdk.org/browse/JDK-8027682

PiperOrigin-RevId: 525775880
@TheCK @eholmer1
Add support for guard clauses in Java 21 switch expressions
065c379 
This PR adds support for `switch` statements where a `case` has a guard clause.

See Issue #983

Fixes #988

COPYBARA_INTEGRATE_REVIEW=google/google-java-format#988 from TheCK:master 4771486db7d8aab84eb4ecf8e68af2612d0c2b5c
PiperOrigin-RevId: 588913297
@cushon @eholmer1
Initial support for pattern matching in switches
2f58013 
Fixes google/google-java-format#937, google/google-java-format#880

PiperOrigin-RevId: 589140113
@changelog-app
Copy link

changelog-app bot commented Oct 21, 2024

Generate changelog in changelog/@unreleased

What do the change types mean?
  • feature: A new feature of the service.
  • improvement: An incremental improvement in the functionality or operation of the service.
  • fix: Remedies the incorrect behaviour of a component of the service in a backwards-compatible way.
  • break: Has the potential to break consumers of this service's API, inclusive of both Palantir services
    and external consumers of the service's API (e.g. customer-written software or integrations).
  • deprecation: Advertises the intention to remove service functionality without any change to the
    operation of the service itself.
  • manualTask: Requires the possibility of manual intervention (running a script, eyeballing configuration,
    performing database surgery, ...) at the time of upgrade for it to succeed.
  • migration: A fully automatic upgrade migration task with no engineer input required.

Note: only one type should be chosen.

How are new versions calculated?
  • ❗The break and manual task changelog types will result in a major release!
  • 🐛 The fix changelog type will result in a minor release in most cases, and a patch release version for patch branches. This behaviour is configurable in autorelease.
  • ✨ All others will result in a minor version release.

Type

  • Feature
  • Improvement
  • Fix
  • Break
  • Deprecation
  • Manual task
  • Migration

Description

Cherry-pick upstream changes to support java 21 style pattern matching

Check the box to generate changelog(s)

  • Generate changelog entry

CRogers
CRogers reviewed Oct 22, 2024
View reviewed changes
build.gradle
@@ -97,3 +95,8 @@ idea.project.ipr.withXml { xml ->
jdks {
daemonTarget = 17
}

javaVersions {
libraryTarget = 21
Copy link
Contributor

Choose a reason for hiding this comment

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

I think one of the tricky parts here is IntelliJ runs with 17 rather than 21. In the IntelliJ plugin, we're probably going to have fork out to a new java process to run the formatting in a 21 JDK (there is already some code that does this iirc but it's slow. This is probably the impetus to get us to always fork but persist the JDK for a bit to make it fast).

Copy link
Contributor

Choose a reason for hiding this comment

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

This is the existing code that uses the configured project JDK for running the formatter: #562

But yeah, it's slow given it spins up a new JVM for each format command.

CRogers
CRogers reviewed Oct 22, 2024
View reviewed changes
build.gradle
@@ -97,3 +95,8 @@ idea.project.ipr.withXml { xml ->
jdks {
daemonTarget = 17
}

javaVersions {
libraryTarget = 21
Copy link
Contributor

@CRogers CRogers Oct 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Will need to test how this works in gradle too. I suspect while we still support Java 17 in our gradle plugins we're going to have to do something a bit cleverer here too. This may also be the impetus to get us off spotless (or just upgrade every project to use 21 Gradle runtime).

CRogers
CRogers reviewed Oct 22, 2024
View reviewed changes
build.gradle
@@ -97,3 +95,8 @@ idea.project.ipr.withXml { xml ->
jdks {
daemonTarget = 17
}

javaVersions {
libraryTarget = 21
Copy link
Contributor

Choose a reason for hiding this comment

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

And there is eclipse... which I have no idea about. Perhaps now is the moment to remove support - we do not support eclipse at all internally at Palantir any more.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we could make it do the same external JDK spinning up code we will end up doing for IntelliJ

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fawind fawind fawind left review comments

@CRogers CRogers CRogers left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@eholmer @CRogers @fawind @eholmer1 @cushon @TheCK