Bump PMD to version 7.0.0 #17717

crammond · 2024-04-11T05:16:50Z

I've been playing around with upgrading RuneLite to Java 21. Part of that work would require upgrading PMD to version 7.0.0, which was a big enough change on its own that felt like it needed its own PR.

Adam- · 2024-04-11T13:08:09Z

I've looked into this before, but decided to not update because they made the immutable field check no longer work with Lombok (I guess due to the new Java record types?). But since we use Lombok so heavily it is just a loss of functionality. I'd probably rather just remove the immutable field check than go around and annotate everything as PMD ignore though.

I am not familiar with this pmd-compat6 dep, do you know what that is for?

crammond · 2024-04-11T13:32:08Z

I've looked into this before, but decided to not update because they made the immutable field check no longer work with Lombok (I guess due to the new Java record types?). But since we use Lombok so heavily it is just a loss of functionality. I'd probably rather just remove the immutable field check than go around and annotate everything as PMD ignore though.

PMD's states their reasoning here. It's an unfortunate change considering use cases like Lombok. I'd be happy to modify this PR to remove that rule if there is still interest in the upgrade, though.

I am not familiar with this pmd-compat6 dep, do you know what that is for?

This was added specifically to support maven-pmd-plugin until they update to version 7.0.0 themselves (relevant release notes). There's already a PR to do that here, apache/maven-pmd-plugin#144, so the need for pmd-compat6 will go away quickly.

Adam- · 2024-04-11T13:52:23Z

I guess I don't mind the update / I am sort of indifferent to it. I see you mentioned moving to Java 21, but to do that we first have to update the launcher to 21 and then push launcher updates out. Updating the launcher version is a pretty major thing, is somewhat irreversible, and tends to uncover obscure Java bugs and also break our code in places we are depend on 11 specific functionality. It is not something I want to sink a bunch of time in unless I have a compelling reason to update.

Adam- · 2024-04-11T14:45:36Z

pmd/pmd#4474 looks like they made it ignore fields annotated with @Getter or @Setter in 7.0.0. Do you know why you are still having to annotate those fields as ignored?

crammond · 2024-04-12T02:02:27Z

I see you mentioned moving to Java 21, but to do that we first have to update the launcher to 21 and then push launcher updates out. Updating the launcher version is a pretty major thing, is somewhat irreversible, and tends to uncover obscure Java bugs and also break our code in places we are depend on 11 specific functionality. It is not something I want to sink a bunch of time in unless I have a compelling reason to update.

Thanks for your thoughts on this. I mentioned it in this PR to gauge interest, and it is understandable that you would not want to push for the upgrade without a very good reason, which I admittedly do not have. I also see in the launcher that you are currently depending on JDKs with 32bit Windows support for which there does not seem to be a temurin offering at this time.

they made it ignore fields annotated with @Getter or @Setter in 7.0.0. Do you know why you are still having to annotate those fields as ignored?

Ah, great catch. I was following their migration guide which suggested going to 6.55.0 first before 7.0.0, and I think as a result 6.55.0 reported these erroneously and I fixed them before bumping to 7.0.0. Let me redo the version bump in one go and see what changes that ends up requiring.

crammond · 2024-04-12T02:56:38Z

I believe that is 27 less use cases of @SuppressWarnings("PMD.ImmutableField") than I had before, great catch!

Adam-

There are also several // NOPMD in the code, some of them can be removed because they have been fixed upstream I think. Please check those.

Adam- · 2024-04-12T15:07:24Z

runelite-client/src/main/java/net/runelite/client/game/chatbox/ChatboxTextMenuInput.java

@@ -56,7 +56,7 @@ private static final class Entry
 	private String title;

 	@Getter
-	private List<Entry> options = new ArrayList<>();
+	private final List<Entry> options = new ArrayList<>();


are all of these types of changes (setting final on getter/setter fields) then also unnecessary? Ideally I'd like the minimum amount of change that keeps pmd happy.

I believe PMD is intentionally still flagging @Getter only fields. I did a re-run from master with 7.0.0 and these are the changes it asked for.

Is the ImmutableFieldRule flagging it? Since it really looks like it shouldn't pmd/pmd@66ddd56#diff-f0d84f4b43ab1c1e2fbc8b8ee09c4c0c3ee2c5a7b981d99fae749652821ec07aR64

It is, @Getter was specifically removed a few days later per pmd/pmd@d9a98b9

Adam- · 2024-04-12T15:07:38Z

runelite-client/pom.xml

+					</dependency>
+					<dependency>
+						<groupId>net.sourceforge.pmd</groupId>
+						<artifactId>pmd-javascript</artifactId>


I meant to comment or add more info in the commit message, but maven-pmd-plugin depends on these, and I think with the API changes from 6 to 7 pmd-compat6 needs all of these updated and available to work properly. Their docs for upgrading PMD at runtime also show upgrading these packages too.

Adam- · 2024-04-12T15:07:41Z

runelite-client/pom.xml

+					</dependency>
+					<dependency>
+						<groupId>net.sourceforge.pmd</groupId>
+						<artifactId>pmd-jsp</artifactId>


crammond · 2024-04-18T01:44:03Z

There are also several // NOPMD in the code, some of them can be removed because they have been fixed upstream I think. Please check those.

I was able to remove a couple of // NOPMDs in the code without changes. I also checked old @SuppressWarnings("PMD.<Rule>")s but they were still needed without other changes.

crammond · 2024-04-18T01:53:09Z

runelite-client/src/main/java/net/runelite/client/plugins/cluescrolls/clues/CrypticClue.java

@@ -1879,6 +1879,7 @@ public int[] getConfigKeys()
 		return new int[]{text.hashCode()};
 	}

+	@SuppressWarnings("PMD.UnusedPrivateMethod")


Possibly pmd/pmd#4861

This function is used, not sure why it caught

crammond force-pushed the pmd-7 branch from c6d03d6 to c4565e2 Compare April 12, 2024 02:52

Adam- reviewed Apr 12, 2024

View reviewed changes

crammond force-pushed the pmd-7 branch 2 times, most recently from 6cd9ed8 to 530b436 Compare April 18, 2024 01:23

crammond commented Apr 18, 2024

View reviewed changes

crammond added 12 commits April 29, 2024 21:59

Bump PMD to 7.0.0, Maven PMD Plugin to 3.22.0

48ac603

Fix PMD failures for ImmutableField

c71f0a2

Fix PMD failures for UseDiamondOperator

58746c5

Fix PMD failures for UnnecessaryFullyQualifiedName

3315986

Fix PMD failures for UnnecessaryCast

812ec2f

Fix PMD failures for UnusedLocalVariable

78c78a4

Fix PMD failures for UnnecessaryBoxing

c552a61

Fix PMD failures for FinalFieldCouldBeStatic

dc76d98

Fix PMD failure for UnusedPrivateMethod

1fd10e5

This function is used, not sure why it caught

Fix PMD failure for InefficientStringBuffering

a476756

Fix PMD failure for PreserveStackTrace

ae77a38

Remove NOPMDs that can be removed without changes

0483f81

crammond force-pushed the pmd-7 branch from 530b436 to 0483f81 Compare April 30, 2024 02:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bump PMD to version 7.0.0 #17717

Bump PMD to version 7.0.0 #17717

crammond commented Apr 11, 2024

Adam- commented Apr 11, 2024 •

edited

crammond commented Apr 11, 2024 •

edited

Adam- commented Apr 11, 2024

Adam- commented Apr 11, 2024

crammond commented Apr 12, 2024

crammond commented Apr 12, 2024

Adam- left a comment

Adam- Apr 12, 2024

crammond Apr 12, 2024

Adam- Apr 12, 2024

crammond Apr 16, 2024

Adam- Apr 12, 2024

crammond Apr 12, 2024

Adam- Apr 12, 2024

crammond commented Apr 18, 2024

crammond Apr 18, 2024

Bump PMD to version 7.0.0 #17717

Are you sure you want to change the base?

Bump PMD to version 7.0.0 #17717

Conversation

crammond commented Apr 11, 2024

Adam- commented Apr 11, 2024 • edited

crammond commented Apr 11, 2024 • edited

Adam- commented Apr 11, 2024

Adam- commented Apr 11, 2024

crammond commented Apr 12, 2024

crammond commented Apr 12, 2024

Adam- 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crammond commented Apr 18, 2024

Choose a reason for hiding this comment

Adam- commented Apr 11, 2024 •

edited

crammond commented Apr 11, 2024 •

edited