Dev-next-gen commented on code in PR #13139:
URL: https://github.com/apache/maven/pull/13139#discussion_r4015243378
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/model/profile/ConditionParser.java:
##########
@@ -379,15 +419,24 @@ private Object parseVariableOrUnknownFunction() {
/**
* Parses a list of arguments for a function call.
+ * For the {@code if} function, only the branch selected by the condition
is evaluated,
+ * the other one is skipped and passed as {@code null}.
*
+ * @param functionName the name of the called function
* @return a list of parsed arguments
* @throws RuntimeException if there's a mismatch in parentheses
*/
- private List<Object> parseArgumentList() {
+ private List<Object> parseArgumentList(String functionName) {
List<Object> args = new ArrayList<>();
current++; // Skip the opening parenthesis
while (current < tokens.size() && !tokens.get(current).equals(")")) {
- args.add(parseLogicalOr());
+ int index = args.size();
+ if ("if".equals(functionName) && (index == 1 || index == 2) &&
toBoolean(args.get(0)) != (index == 1)) {
Review Comment:
You're right that this line is the only place where the parser, which
otherwise only sees the injected function map, depends on a function name. I
took the minimal path in 92a381c: the literal is now `private static final
String IF_FUNCTION = "if"`, documented as the function whose unselected branch
is skipped, so the coupling is visible from the top of the class. Spotless
wrapped the condition over three lines, and `ConditionParserTest` plus
`ConditionProfileActivatorTest` still pass (76 tests, 3 skipped as before).
I stayed away from a `LazyExpressionFunction` sub-interface in this PR
because it changes the public `ExpressionFunction` contract, and that feels
like a call for the maintainers rather than for a bug fix. If you'd rather have
it here, I'm glad to rework the change along those lines.
##########
impl/maven-impl/src/test/java/org/apache/maven/impl/model/profile/ConditionParserTest.java:
##########
@@ -182,6 +182,40 @@ void testIfFunction() {
assertEquals("short", parser.parse("if(length('hi') > 3, 'long',
'short')"));
}
+ /**
+ * Only the selected branch of {@code if(..)} is evaluated. The other one
may not be valid for
+ * the current input, as in the example from the {@code condition}
documentation: with a
+ * {@code java.version} that has no {@code -}, the unselected {@code
substring(..)} would get an
+ * end index of -1.
+ */
+ @Test
+ void testIfFunctionOnlyEvaluatesSelectedBranch() {
+ assertEquals(
+ "1.8.0_292",
+ parser.parse("if(contains(${java.version}, '-'), "
+ + "substring(${java.version}, 0,
indexOf(${java.version}, '-')), ${java.version})"));
+ assertEquals(
+ "21",
+ parser.parse("if(contains('21-ea', '-'), substring('21-ea', 0,
indexOf('21-ea', '-')), '21-ea')"));
+ assertThrows(RuntimeException.class, () -> parser.parse("if(true,
'a')"));
+ assertThrows(RuntimeException.class, () -> parser.parse("if(false,
substring('a', 0, 5), 'b'"));
Review Comment:
Good catch, that path was only covered by reasoning. I added
`assertThrows(RuntimeException.class, () -> parser.parse("if(false, 'a')"))`
right after the `if(true, 'a')` case in 92a381c. It takes the skip branch you
describe, `if_` receives `[false, null]` and rejects the two-argument call, and
the test passes along with the rest of `ConditionParserTest` (38 tests).
--
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]
