Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve error message in painless when parameter must be a constant but isn't #82638

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -34,5 +34,31 @@ public static char StringTochar(final String value) {
return value.charAt(0);
}

public static String toOrdinal(int number) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just the number is fine, no need to spend lines on adding the ordinal suffix.

StringBuilder stringBuilder = new StringBuilder();
stringBuilder.append(number);
int hundredModule = number % 100;
int decimalModule = number % 10;
if (11 <= hundredModule && hundredModule <= 13) {
stringBuilder.append("th");
} else {
switch (decimalModule) {
case 1:
stringBuilder.append("st");
break;
case 2:
stringBuilder.append("nd");
break;
case 3:
stringBuilder.append("rd");
break;
default:
stringBuilder.append("th");
break;
}
}
return stringBuilder.toString();
}

private Utility() {}
}
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

import org.elasticsearch.painless.AnalyzerCaster;
import org.elasticsearch.painless.Operation;
import org.elasticsearch.painless.Utility;
import org.elasticsearch.painless.ir.BinaryImplNode;
import org.elasticsearch.painless.ir.BinaryMathNode;
import org.elasticsearch.painless.ir.BooleanNode;
Expand Down Expand Up @@ -66,9 +67,11 @@
import org.elasticsearch.painless.symbol.IRDecorations.IRDInstanceBinding;
import org.elasticsearch.painless.symbol.IRDecorations.IRDMethod;
import org.elasticsearch.painless.symbol.IRDecorations.IRDOperation;
import org.elasticsearch.painless.symbol.IRDecorations.IRDName;

import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
import java.util.Locale;
import java.util.function.Consumer;

/**
Expand Down Expand Up @@ -1107,13 +1110,19 @@ private void replaceCallWithConstant(
ExpressionNode argNode = irInvokeCallMemberNode.getArgumentNodes().get(i);
IRDConstant constantDecoration = argNode.getDecoration(IRDConstant.class);
if (constantDecoration == null) {
// TODO find a better string to output
throw irInvokeCallMemberNode.getLocation()
.createError(
new IllegalArgumentException(
"all arguments to [" + javaMethod.getName() + "] must be constant but the [" + (i + 1) + "] argument isn't"
)
);
// offering the symbol name in error message (CastNode was evolved from ESymbol)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this handle cases when there are multiple arguments that should be constants?

Copy link
Contributor

@stu-elastic stu-elastic Jan 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CastNode was evolved from ESymbol

What does this mean?

String argumentName = argNode instanceof CastNode
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this handle the case of 2 + x as an argument?

Copy link
Contributor Author

@Kxrr Kxrr Jan 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can't handle the 2 + x case yet, so if we need to handle this more complex case, I'm going to write a special method to build the original expression from Node.

Copy link
Contributor Author

@Kxrr Kxrr Jan 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that the argNode has a location field with some information about the original expression, but it's missing the length. It would be much simpler to use this location field (no need to traverse the argNode's data structure)

? ((CastNode) argNode).getChildNode().getDecoration(IRDName.class).getValue()
Copy link
Contributor

@stu-elastic stu-elastic Jan 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we go with this code (which is not clear at the moment), getDecorationValue should be used to avoid the potential null dereference.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I will use getDecorationValue.

: "";
throw irInvokeCallMemberNode
.getLocation()
.createError(new IllegalArgumentException(String.format(
Locale.ROOT,
"All arguments of the [%s] method must be constants, but the [%s] argument [%s] is not",
javaMethod.getName(),
Utility.toOrdinal(i+1),
argumentName
)));
}
args[i] = constantDecoration.getValue();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -240,7 +240,7 @@ public void testClassMethodCompileTimeOnlyVariableParams() {
IllegalArgumentException.class,
() -> scriptEngine.compile(null, "def a = 2; classMul(2, a)", BindingsTestScript.CONTEXT, Collections.emptyMap())
);
assertThat(e.getMessage(), equalTo("all arguments to [classMul] must be constant but the [2] argument isn't"));
assertThat(e.getMessage(), equalTo("All arguments of the [classMul] method must be constants, but the [2nd] argument [a] is not"));
}

public void testClassMethodCompileTimeOnlyThrows() {
Expand Down Expand Up @@ -269,7 +269,7 @@ public void testInstanceMethodCompileTimeOnlyVariableParams() {
IllegalArgumentException.class,
() -> scriptEngine.compile(null, "def a = 2; instanceMul(a, 2)", BindingsTestScript.CONTEXT, Collections.emptyMap())
);
assertThat(e.getMessage(), equalTo("all arguments to [instanceMul] must be constant but the [1] argument isn't"));
assertThat(e.getMessage(), equalTo("All arguments of the [instanceMul] method must be constants, but the [1st] argument [a] is not"));
}

public void testCompileTimeOnlyParameterFoldedToConstant() {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/

package org.elasticsearch.painless;

import org.elasticsearch.test.ESTestCase;

import java.util.Map;

import static java.util.Map.entry;

public class UtilityTests extends ESTestCase {

public static final Map<Integer, String> ORDINALS = Map.ofEntries(
entry(0, "0th"),
entry(1, "1st"),
entry(2, "2nd"),
entry(3, "3rd"),
entry(4, "4th"),
entry(5, "5th"),
entry(10, "10th"),
entry(11, "11th"),
entry(12, "12th"),
entry(13, "13th"),
entry(14, "14th"),
entry(20, "20th"),
entry(21, "21st"),
entry(22, "22nd"),
entry(23, "23rd"),
entry(24, "24th"),
entry(100, "100th"),
entry(101, "101st"),
entry(102, "102nd"),
entry(103, "103rd"),
entry(104, "104th"),
entry(111, "111th"),
entry(112, "112th"),
entry(113, "113th"),
entry(114, "114th"),
entry(1000, "1000th")
);

public void testToOrdinal() {
for (Map.Entry<Integer, String> item : ORDINALS.entrySet()) {
assertEquals(Utility.toOrdinal(item.getKey()), item.getValue());
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ fetch:
---
mutable pattern:
- do:
catch: /all arguments to \[grok\] must be constant but the \[1\] argument isn't/
catch: /All arguments of the \[grok\] method must be constants, but the \[1st\] argument \[\] is not/
search:
index: http_logs
body:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ fetch:
---
mutable pattern:
- do:
catch: /all arguments to \[grok\] must be constant but the \[1\] argument isn't/
catch: /All arguments of the \[grok\] method must be constants, but the \[1st\] argument \[\] is not/
search:
index: http_logs
body:
Expand Down