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.

Sign up for GitHub

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

Enable Error Prone check: AnnotateFormatMethod #14933

Closed
wants to merge 9 commits into from
Closed

Enable Error Prone check: AnnotateFormatMethod #14933

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
c47502a
Fix log messages which are not format strings
ksobolew Mar 17, 2022
0a47b89
Call semanticException method as a format method in SetColumnTypeTask
ksobolew Dec 23, 2022
ea4c436
Annotate semanticException factories as format methods
ksobolew Nov 8, 2022
6479924
Make some conditional uses of NodeRepresentation#appendDetails explicit
ksobolew Nov 10, 2022
2764f59
Annotate NodeRepresentation#appendDetails as a format method
ksobolew Nov 8, 2022
d2036b3
Annotate Failures#checkCondition as a format method
ksobolew Nov 8, 2022
3a09752
Annotate format methods in trino-parquet
ksobolew Nov 8, 2022
d1f0b9b
Annotate all remaining cases of format methods
ksobolew Nov 8, 2022
5b04fba
Enable Error Prone check: AnnotateFormatMethod
ksobolew Nov 7, 2022
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
2 changes: 1 addition & 1 deletion core/trino-main/src/main/java/io/trino/execution/SetSessionTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ public ListenableFuture<Void> execute(
propertyMetadata.decode(objectValue);
}
catch (RuntimeException e) {
throw semanticException(INVALID_SESSION_PROPERTY, statement, e.getMessage());
throw semanticException(INVALID_SESSION_PROPERTY, statement, "Invalid session property value '%s': %s", propertyName.toString(), e.getMessage());
Copy link
Member

Choose a reason for hiding this comment

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

SET SESSION is invoked for a particular session property.
we don't need to provide the property name in the exception message, do we?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The other places that throw in this method do provide the property name, so I wanted to be consistent.

Copy link
Member

Choose a reason for hiding this comment

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

awesome. can this be a separate change, or even separate PR?
this change is orthognal to error-prone

}

stateMachine.addSetSessionProperties(propertyName.toString(), value);
Expand Down
9 changes: 5 additions & 4 deletions core/trino-main/src/main/java/io/trino/sql/analyzer/AggregationAnalyzer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
package io.trino.sql.analyzer;

import com.google.common.collect.ImmutableList;
import com.google.errorprone.annotations.FormatMethod;
import io.trino.Session;
import io.trino.metadata.Metadata;
import io.trino.spi.StandardErrorCode;
Expand Down Expand Up @@ -438,8 +439,7 @@ protected Boolean visitFunctionCall(FunctionCall node, Void context)
if (node.getFilter().isPresent()) {
throw semanticException(FUNCTION_NOT_AGGREGATE,
node,
"Filter is only valid for aggregation functions",
node);
"Filter is only valid for aggregation functions");
}
if (node.getOrderBy().isPresent()) {
throw semanticException(FUNCTION_NOT_AGGREGATE, node, "ORDER BY is only valid for aggregation functions");
Expand Down Expand Up @@ -787,12 +787,13 @@ private boolean hasOrderByReferencesToOutputColumns(Node node)
return hasReferencesToScope(node, analysis, orderByScope.get());
}

private void verifyNoOrderByReferencesToOutputColumns(Node node, StandardErrorCode errorCode, String errorString)
@FormatMethod
private void verifyNoOrderByReferencesToOutputColumns(Node node, StandardErrorCode errorCode, String errorString, Object... errorStringArguments)
{
getReferencesToScope(node, analysis, orderByScope.get())
.findFirst()
.ifPresent(expression -> {
throw semanticException(errorCode, expression, errorString);
throw semanticException(errorCode, expression, errorString, errorStringArguments);
});
}
}
41 changes: 23 additions & 18 deletions core/trino-main/src/main/java/io/trino/sql/analyzer/ExpressionAnalyzer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@
import com.google.common.collect.LinkedHashMultimap;
import com.google.common.collect.Multimap;
import com.google.common.collect.Streams;
import com.google.errorprone.annotations.FormatMethod;
import com.google.errorprone.annotations.FormatString;
import io.trino.Session;
import io.trino.execution.warnings.WarningCollector;
import io.trino.metadata.OperatorNotFoundException;
Expand Down Expand Up @@ -758,7 +760,7 @@ protected Type visitDereferenceExpression(DereferenceExpression node, StackableA
for (RowType.Field rowField : rowType.getFields()) {
if (fieldName.equalsIgnoreCase(rowField.getName().orElse(null))) {
if (foundFieldName) {
throw semanticException(AMBIGUOUS_NAME, field, "Ambiguous row field reference: " + fieldName);
throw semanticException(AMBIGUOUS_NAME, field, "Ambiguous row field reference: %s", fieldName);
}
foundFieldName = true;
rowFieldType = rowField.getType();
Expand Down Expand Up @@ -1243,7 +1245,7 @@ protected Type visitFunctionCall(FunctionCall node, StackableAstVisitorContext<C
Expression expression = arguments.get(0);
Type expressionType = process(expression, context);
if (!(expressionType instanceof VarcharType)) {
throw semanticException(TYPE_MISMATCH, node, format("Expected expression of varchar, but '%s' has %s type", expression, expressionType.getDisplayName()));
throw semanticException(TYPE_MISMATCH, node, "Expected expression of varchar, but '%s' has %s type", expression, expressionType.getDisplayName());
}
}

Expand Down Expand Up @@ -1273,7 +1275,7 @@ protected Type visitFunctionCall(FunctionCall node, StackableAstVisitorContext<C
// After optimization, array constructor is rewritten to a function call.
// For historic reasons array constructor is allowed to have 254 arguments
if (node.getArguments().size() > 254) {
throw semanticException(TOO_MANY_ARGUMENTS, node, "Too many arguments for array constructor", function.getSignature().getName());
throw semanticException(TOO_MANY_ARGUMENTS, node, "Too many arguments for array constructor: %s", function.getSignature().getName());
}
}
else if (node.getArguments().size() > 127) {
Expand Down Expand Up @@ -1491,7 +1493,7 @@ else if (frame.getType() == GROUPS) {
}
}
else {
throw semanticException(NOT_SUPPORTED, frame, "Unsupported frame type: " + frame.getType());
throw semanticException(NOT_SUPPORTED, frame, "Unsupported frame type: %s", frame.getType());
}
}
}
Expand Down Expand Up @@ -2421,7 +2423,7 @@ protected Type visitLambdaExpression(LambdaExpression node, StackableAstVisitorC

if (types.size() != lambdaArguments.size()) {
throw semanticException(INVALID_PARAMETER_USAGE, node,
format("Expected a lambda that takes %s argument(s) but got %s", types.size(), lambdaArguments.size()));
"Expected a lambda that takes %s argument(s) but got %s", types.size(), lambdaArguments.size());
}

ImmutableList.Builder<Field> fields = ImmutableList.builder();
Expand Down Expand Up @@ -2557,7 +2559,7 @@ public Type visitJsonValue(JsonValue node, StackableAstVisitorContext<Context> c
!isDateTimeType(returnedType) ||
returnedType.equals(INTERVAL_DAY_TIME) ||
returnedType.equals(INTERVAL_YEAR_MONTH)) {
throw semanticException(TYPE_MISMATCH, node, "Invalid return type of function JSON_VALUE: " + node.getReturnedType().get());
throw semanticException(TYPE_MISMATCH, node, "Invalid return type of function JSON_VALUE: %s", node.getReturnedType().get());
}

JsonPathAnalysis pathAnalysis = jsonPathAnalyses.get(NodeRef.of(node));
Expand Down Expand Up @@ -2803,7 +2805,7 @@ private ResolvedFunction getInputFunction(Type type, JsonFormat format, Node nod
if (isStringType(type)) {
yield QualifiedName.of(VARBINARY_TO_JSON);
}
throw semanticException(TYPE_MISMATCH, node, format("Cannot read input of type %s as JSON using formatting %s", type, format));
throw semanticException(TYPE_MISMATCH, node, "Cannot read input of type %s as JSON using formatting %s", type, format);
}
case UTF8 -> QualifiedName.of(VARBINARY_UTF8_TO_JSON);
case UTF16 -> QualifiedName.of(VARBINARY_UTF16_TO_JSON);
Expand All @@ -2828,23 +2830,23 @@ private ResolvedFunction getOutputFunction(Type type, JsonFormat format, Node no
if (isStringType(type)) {
yield QualifiedName.of(JSON_TO_VARBINARY);
}
throw semanticException(TYPE_MISMATCH, node, format("Cannot output JSON value as %s using formatting %s", type, format));
throw semanticException(TYPE_MISMATCH, node, "Cannot output JSON value as %s using formatting %s", type, format);
}
case UTF8 -> {
if (!VARBINARY.equals(type)) {
throw semanticException(TYPE_MISMATCH, node, format("Cannot output JSON value as %s using formatting %s", type, format));
throw semanticException(TYPE_MISMATCH, node, "Cannot output JSON value as %s using formatting %s", type, format);
}
yield QualifiedName.of(JSON_TO_VARBINARY_UTF8);
}
case UTF16 -> {
if (!VARBINARY.equals(type)) {
throw semanticException(TYPE_MISMATCH, node, format("Cannot output JSON value as %s using formatting %s", type, format));
throw semanticException(TYPE_MISMATCH, node, "Cannot output JSON value as %s using formatting %s", type, format);
}
yield QualifiedName.of(JSON_TO_VARBINARY_UTF16);
}
case UTF32 -> {
if (!VARBINARY.equals(type)) {
throw semanticException(TYPE_MISMATCH, node, format("Cannot output JSON value as %s using formatting %s", type, format));
throw semanticException(TYPE_MISMATCH, node, "Cannot output JSON value as %s using formatting %s", type, format);
}
yield QualifiedName.of(JSON_TO_VARBINARY_UTF32);
}
Expand Down Expand Up @@ -3137,7 +3139,8 @@ private void coerceType(StackableAstVisitorContext<Context> context, Expression
coerceType(expression, actualType, expectedType, message);
}

private Type coerceToSingleType(StackableAstVisitorContext<Context> context, Node node, String message, Expression first, Expression second)
@FormatMethod
ksobolew marked this conversation as resolved.
Show resolved Hide resolved
private Type coerceToSingleType(StackableAstVisitorContext<Context> context, Node node, @FormatString String message, Expression first, Expression second)
{
Type firstType = UNKNOWN;
if (first != null) {
Expand All @@ -3163,7 +3166,9 @@ private Type coerceToSingleType(StackableAstVisitorContext<Context> context, Nod
return superType;
}

throw semanticException(TYPE_MISMATCH, node, message, firstType, secondType);
@SuppressWarnings("FormatStringAnnotation") // Error Prone wants the types of format arguments to be the same as where @FormatString is declared, but we need them to be different
TrinoException exception = semanticException(TYPE_MISMATCH, node, message, firstType, secondType);
Copy link
Member

Choose a reason for hiding this comment

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

new TrinoException?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know, I think it's valid, just hits some limitations of Java type system (or Error Prone's interpretation of it)

ksobolew marked this conversation as resolved.
Show resolved Hide resolved
throw exception;
}

private Type coerceToSingleType(StackableAstVisitorContext<Context> context, String description, List<Expression> expressions)
Expand All @@ -3184,12 +3189,12 @@ private Type coerceToSingleType(StackableAstVisitorContext<Context> context, Str
for (Type type : types) {
Optional<Type> newSuperType = typeCoercion.getCommonSuperType(superType, type);
if (newSuperType.isEmpty()) {
throw semanticException(TYPE_MISMATCH, Iterables.get(typeExpressions.get(type), 0).getNode(), format(
throw semanticException(TYPE_MISMATCH, Iterables.get(typeExpressions.get(type), 0).getNode(),
"%s must be the same type or coercible to a common type. Cannot find common type between %s and %s, all types (without duplicates): %s",
description,
superType,
type,
typeExpressions.keySet()));
typeExpressions.keySet());
}
superType = newSuperType.get();
}
Expand All @@ -3200,12 +3205,12 @@ private Type coerceToSingleType(StackableAstVisitorContext<Context> context, Str

if (!type.equals(superType)) {
if (!typeCoercion.canCoerce(type, superType)) {
throw semanticException(TYPE_MISMATCH, Iterables.get(coercionCandidates, 0).getNode(), format(
throw semanticException(TYPE_MISMATCH, Iterables.get(coercionCandidates, 0).getNode(),
"%s must be the same type or coercible to a common type. Cannot find common type between %s and %s, all types (without duplicates): %s",
description,
superType,
type,
typeExpressions.keySet()));
typeExpressions.keySet());
}
addOrReplaceExpressionsCoercion(coercionCandidates, type, superType);
}
Expand Down Expand Up @@ -3559,7 +3564,7 @@ public static ExpressionAnalyzer createWithoutSubqueries(
session,
TypeProvider.empty(),
parameters,
node -> semanticException(errorCode, node, message),
node -> semanticException(errorCode, node, "%s", message),
warningCollector,
isDescribe);
}
Expand Down
5 changes: 2 additions & 3 deletions core/trino-main/src/main/java/io/trino/sql/analyzer/JsonPathAnalyzer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,6 @@
import static io.trino.sql.analyzer.TypeSignatureProvider.fromTypes;
import static io.trino.sql.jsonpath.tree.ArithmeticUnary.Sign.PLUS;
import static io.trino.type.Json2016Type.JSON_2016;
import static java.lang.String.format;
import static java.util.Objects.requireNonNull;

public class JsonPathAnalyzer
Expand Down Expand Up @@ -361,9 +360,9 @@ protected Type visitNamedVariable(NamedVariable node, Void context)
.filter(name -> name.equalsIgnoreCase(node.getName()))
.findFirst();
if (similarName.isPresent()) {
throw semanticException(INVALID_PATH, pathNode, format("no value passed for parameter %s. Try quoting \"%s\" in the PASSING clause to match case", node.getName(), node.getName()));
throw semanticException(INVALID_PATH, pathNode, "no value passed for parameter %s. Try quoting \"%s\" in the PASSING clause to match case", node.getName(), node.getName());
}
throw semanticException(INVALID_PATH, pathNode, "no value passed for parameter " + node.getName());
throw semanticException(INVALID_PATH, pathNode, "no value passed for parameter %s", node.getName());
}

if (parameterType.equals(JSON_2016)) {
Expand Down
3 changes: 3 additions & 0 deletions core/trino-main/src/main/java/io/trino/sql/analyzer/SemanticExceptions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
*/
package io.trino.sql.analyzer;

import com.google.errorprone.annotations.FormatMethod;
import io.trino.spi.ErrorCodeSupplier;
import io.trino.spi.TrinoException;
import io.trino.sql.tree.Expression;
Expand All @@ -38,11 +39,13 @@ public static TrinoException ambiguousAttributeException(Expression node, Qualif
throw semanticException(AMBIGUOUS_NAME, node, "Column '%s' is ambiguous", name);
}

@FormatMethod
public static TrinoException semanticException(ErrorCodeSupplier code, Node node, String format, Object... args)
{
return semanticException(code, node, null, format, args);
}

@FormatMethod
public static TrinoException semanticException(ErrorCodeSupplier code, Node node, Throwable cause, String format, Object... args)
{
throw new TrinoException(code, extractLocation(node), format(format, args), cause);
Expand Down