fix: resolves mutation warning - #2545
Conversation
|
Important Review skippedAuto reviews are limited based on label configuration. 🏷️ Required labels (at least one) (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAll three JBang launcher scripts ( ChangesSwitch launcher scripts to -jar invocation
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/dev/jbang/catalog/Alias.java`:
- Around line 63-66: The JavaAgent inner class within the Alias class has final
fields (agentRef and options) that are annotated with `@SerializedName`, causing
Java 26 final-field mutation warnings when Gson reflectively mutates them during
deserialization. Locate the JavaAgent inner class and remove the final keyword
from both the agentRef field and the options field to match the pattern already
applied to the outer Alias class fields.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 87332c72-668a-42c0-b921-ddd0759f9c5a
📒 Files selected for processing (5)
build.gradlesrc/main/java/dev/jbang/catalog/Alias.javasrc/main/java/dev/jbang/catalog/Catalog.javasrc/main/java/dev/jbang/catalog/CatalogRef.javasrc/main/java/dev/jbang/catalog/Template.java
|
-1 on making the code weaker because of openjdk dumb printing warning behaviours. i haven't looked yet but what stops us from running jbang with -jar and it will pick up the flag from there? |
|
Given that ${jarPath} always points to the the jbang.jar file, changing to resolves the mutation warning without having to remove the final methods. I'll rework the PR and change the jbang, jbang.cmd, and jbang.ps1 scripts and change |
@maxandersen , the latest update fully resolves the issue. Thanks for the push back on the original proposal :-) |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
one thing to check is if we ever call jbang with a custom classpath - i.e. during integration runs ...here I guess there will be an issue too but its much less visible. |
If you are referring to the |
|
@maxandersen , this PR is ready to be reviewed (hopefully merged). :-) |
|
Testing with the latest early-access release still produces the following output when using Java 26. |
|
sorry wrong bug :) |
no, I mean when you run with integrations on classpath; like quarkus, i.e. btw. I can't reproduce your output for some reason: which I find weird :/ |
That is super weird if you're running the JVM, but maybe you are using the native version of JBang? If I use Java 26 JVM and JBang.jar (not native), I always see the error: |
This works fine when I ugraded the Quarkus dependency in |
…ev/final-methods-java26
|
not using native but jbang was picking up java 25 over java 26 for some reason. with java 26 forced i see the error too. |
|
i do want this in but will be release after next to avoid chaning too many things at once. |
Closes #2542
@maxandersen , @quintesse - given the discussion in #2430 , the most practical interim solution is to remove the final modifiers from serializable classes. The additional option previously added to the manifest.mf file only works when JBang is started with
java -jar jbang.jar ...., and does not work otherwise.I've commented out (
/* final */) the final modifiers and have not removed them.Summary by CodeRabbit