Fixes #759 - Plugin generates corrupted shaded sources beyond recognition #759 - #760
Fixes #759 - Plugin generates corrupted shaded sources beyond recognition #759#760ursaj wants to merge 2 commits into
Conversation
kwin
left a comment
kwin
left a comment
There was a problem hiding this comment.
Thanks for the PR and sorry for the late review.
| * Match dot, slash or space at end of string | ||
| */ | ||
| private static final Pattern RX_ENDS_WITH_DOT_SLASH_SPACE = Pattern.compile("[./ ]$"); | ||
| private static final Pattern RX_ENDS_WITH_DOT_SLASH_SPACE = Pattern.compile("(\\.|/|[^<?:+\\-*/^|&] )$"); |
There was a problem hiding this comment.
Why are three conditions necessary when the first and the second can never be true without the last one? Isn't the last one sufficient then? This also requires some renaming of this constant.
There was a problem hiding this comment.
Also a comment would be much appreciated why those characters are not allowed...
There was a problem hiding this comment.
original regex was:
- ether dot
- or slash
- or space at the end of string
new regex matches
- either dot
- or slash
- or space at the end of string. where string does NOT end with operator (generic type, binary, trinary)
regarding comment or renaming - I don't mind. your suggestions?
There was a problem hiding this comment.
added comment. @kwin please check
without this change new tests would fail on the following declarations:
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Adds more robust Java-token handling to shade sources relocation to prevent corrupted output (issue #759), expanding coverage to additional keywords and operators.
Changes:
- Extend
SimpleRelocatorregex matching to recognize more Java keywords and better handle operator-adjacent boundaries. - Add new relocation test cases covering generics,
throw/new,instanceof, ternary, boolean ops, arithmetic, bitwise ops, andswitch/case.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/test/java/org/apache/maven/plugins/shade/relocation/SimpleRelocatorTest.java | Expands test input/expected strings to cover additional Java syntax and operators during relocation. |
| src/main/java/org/apache/maven/plugins/shade/relocation/SimpleRelocator.java | Updates regexes to treat more Java keywords and operator contexts as relocation boundaries. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| + " boolean flag2 = org.apache.maven.Utils.yes() ? org.apache.maven.Utils.one() : org.apache.maven.Utils.zero();\n" | ||
| + " boolean flag3 = org.apache.maven.Utils.yes() || org.apache.maven.Utils.no();\n" | ||
| + " boolean flag4 = org.apache.maven.Utils.yes() && org.apache.maven.Utils.no();\n" | ||
| + " boolean flag5 = org.apache.maven.Utils.yes() ^^ org.apache.maven.Utils.no();\n" |
| + " boolean flag2 = com.acme.maven.Utils.yes() ? com.acme.maven.Utils.one() : com.acme.maven.Utils.zero();\n" | ||
| + " boolean flag3 = com.acme.maven.Utils.yes() || com.acme.maven.Utils.no();\n" | ||
| + " boolean flag4 = com.acme.maven.Utils.yes() && com.acme.maven.Utils.no();\n" | ||
| + " boolean flag5 = com.acme.maven.Utils.yes() ^^ com.acme.maven.Utils.no();\n" |
| + " int value1 = org.apache.maven.Utils.x() + org.apache.maven.Utils.y()\n" | ||
| + " int value2 = org.apache.maven.Utils.x() - org.apache.maven.Utils.y()\n" | ||
| + " int value3 = org.apache.maven.Utils.x() * org.apache.maven.Utils.y()\n" | ||
| + " int value4 = org.apache.maven.Utils.x() / org.apache.maven.Utils.y()\n" | ||
| + " int value5 = org.apache.maven.Utils.one() | org.apache.maven.Utils.two()\n" | ||
| + " int value6 = org.apache.maven.Utils.one() & org.apache.maven.Utils.two()\n" | ||
| + " int value7 = org.apache.maven.Utils.one() ^ org.apache.maven.Utils.two()\n" |
| + " case org.apache.maven.Utils.STATIC_VALUE:\n" | ||
| + " org.apache.maven.Utils.info(\"known value\"):\n" | ||
| + " break;\n" | ||
| + " default:\n" | ||
| + " org.apache.maven.Utils.warn(\"unknown value\"):\n" | ||
| + " break;\n" |
| * </ul> | ||
| */ | ||
| private static final Pattern RX_ENDS_WITH_DOT_SLASH_SPACE = Pattern.compile("[./ ]$"); | ||
| private static final Pattern RX_ENDS_WITH_DOT_SLASH_SPACE = Pattern.compile("(\\.|/|[^<?:+\\-*/^|&] )$"); |
This PR adds support for binary operations and missing Java keywords into
shade sourcesfunctionality of the maven-shade-plugin as reported in #759.Following this checklist to help us incorporate your
contribution quickly and easily:
Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
Run
mvn clean verifyto make sure basic checks pass. A more thorough check willbe performed on your pull request automatically.
You have run the integration tests successfully (
mvn -Prun-its clean verify).I hereby declare this contribution to be licenced under the Apache License Version 2.0, January 2004
Plugin generates corrupted shaded sources beyond recognition #759
Make sure there is a JIRA issue filed
for the change (usually before you start working on it). Trivial changes like typos do not
require a JIRA issue. Your pull request should address just this issue, without
pulling in other changes.
Each commit in the pull request should have a meaningful subject line and body.
Format the pull request title like
[MSHADE-XXX] - Fixes bug in ApproximateQuantiles,where you replace
MSHADE-XXXwith the appropriate JIRA issue. Best practiceis to use the JIRA issue title in the pull request title and in the first line of the
commit message.
Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
Run
mvn clean verifyto make sure basic checks pass. A more thorough check willbe performed on your pull request automatically.
You have run the integration tests successfully (
mvn -Prun-its clean verify).If your pull request is about ~20 lines of code you don't need to sign an
Individual Contributor License Agreement if you are unsure
please ask on the developers list.
To make clear that you license your contribution under
the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.
I hereby declare this contribution to be licenced under the Apache License Version 2.0, January 2004
In any other case, please file an Apache Individual Contributor License Agreement.