Dev-next-gen opened a new pull request, #13139:
URL: https://github.com/apache/maven/pull/13139
Following this checklist to help us incorporate your
contribution quickly and easily:
- [x] Your pull request should address just one issue, without pulling in
other changes.
- [x] Write a pull request description that is detailed enough to understand
what the pull request does, how, and why.
- [x] Each commit in the pull request should have a meaningful subject line
and body.
Note that commits might be squashed by a maintainer on merge.
- [x] Write unit tests that match behavioral changes, where the tests fail
if the changes to the runtime are not applied.
This may not always be possible but is a best-practice.
- [x] Run `mvn verify` to make sure basic checks pass.
A more thorough check will be performed on your pull request automatically.
- [ ] You have run the [Core IT][core-its] successfully.
If your pull request is about ~20 lines of code you don't need to sign an
[Individual Contributor License
Agreement](https://www.apache.org/licenses/icla.pdf) 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](http://www.apache.org/licenses/LICENSE-2.0)
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](http://www.apache.org/licenses/LICENSE-2.0)
- [ ] In any other case, please file an [Apache Individual Contributor
License Agreement](https://www.apache.org/licenses/icla.pdf).
[core-its]: https://maven.apache.org/core-its/core-it-suite/
---
I tried the "Conditional logic" example from the `<condition>` documentation
in `maven.mdo`:
```
if(contains(${java.version}, '-'), substring(${java.version}, 0,
indexOf(${java.version}, '-')), ${java.version})
```
With a `java.version` that has no `-` (the test context uses `1.8.0_292`,
and `21.0.12` behaves the same), the condition fails with
`StringIndexOutOfBoundsException: Range [0, -1) out of bounds for length 9`,
and the profile is reported as an error instead of being evaluated.
`ConditionParser` computes every operand while it parses, so both branches of
`if(..)` are always evaluated, even the one that is discarded. `&&` and `||`
have the same problem: `length(${p}) >= 3 && substring(${p}, 0, 3) == 'abc'`
throws when `p` is shorter than three characters, because the guard can't stop
the right-hand side from running.
This change skips the branch that isn't selected in `if(..)`, and skips the
right operand of `&&` or `||` once the left operand has already decided the
result. Skipping walks the tokens up to the next terminator outside parentheses
(`,` or `)`, plus `&&`/`||` depending on the operator). It still rejects a
missing operand or unbalanced parentheses, so the existing
`testParenthesesMismatch` cases keep failing the way they did before. For
operands that do get evaluated, nothing changes: `if` still goes through the
registered function, which checks the argument count, and a non-boolean on
either side of `&&` or `||` still fails as before.
I added `testIfFunctionOnlyEvaluatesSelectedBranch`, which uses the
documented example verbatim, and `testLogicalOperatorsShortCircuit`. Without
the change to `ConditionParser`, both fail with the exceptions above. With it,
`ConditionParserTest` and `ConditionProfileActivatorTest` pass (76 tests), and
`mvn verify` on `impl/maven-impl` passes: 690 tests, checkstyle and spotless
included. I ran that with Maven 3.8.4 and `-Denforcer.skip -Drat.skip`, which
were the only toolchain constraints on that machine. I did not run the core ITs.
Found by a defect-hunting pipeline I build and run
([Dev-next-gen](https://github.com/Dev-next-gen)), using Claude Code with
Anthropic's Claude Opus 5.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
