Enable Error Prone check: AnnotateFormatMethod #14933

ksobolew · 2022-11-07T14:33:26Z

Description

The idea of it is simple: when it sees a pair of a format string and format arguments passes to a format method, and these two are parameters of a method, it asks the method to be annotated as a format methods as well. This works in tandem with FormatStringAnnotation (see also: #9512) as it allows it to detect more cases of malformed format strings and misused format methods (of which it found plenty in trino-main and trino-spi; most of them are missed opportunities to use the format method, but there are a couple of actual bad format strings).

Requires: #15051, ~~#15052~~

Non-technical explanation

Better compile-time checks and better code quality.

Release notes

(x) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

ksobolew · 2022-11-07T14:34:14Z

(I suppose I could split it into several smaller commits)

core/trino-main/src/main/java/io/trino/execution/CommentTask.java

core/trino-main/src/main/java/io/trino/execution/CreateTableTask.java

testing/trino-testing-services/src/main/java/io/trino/testng/services/Listeners.java

kokosing · 2022-11-07T15:41:42Z

I suppose I could split it into several smaller commits

👍

ksobolew · 2022-11-08T12:55:54Z

There you go: 14 commits :)

core/trino-spi/pom.xml

core/trino-main/src/main/java/io/trino/execution/SetSessionTask.java

kokosing

LGTM for the below commits and the first commit. If you could extract them as separate PR I can merge it.

I left two comments that are required to move forward. I haven't reviewed entire PR.

ksobolew · 2022-11-16T10:13:11Z

LGTM for the below commits and the first commit. If you could extract them as separate PR I can merge it.

Extracted as #15051

ksobolew · 2022-11-17T08:36:19Z

Flaky hit: #14487

kokosing · 2022-11-17T10:06:33Z

Please rebase

core/trino-main/src/main/java/io/trino/execution/SetSessionTask.java

core/trino-main/src/main/java/io/trino/sql/planner/planprinter/PlanPrinter.java

The `in` parameter recently changed type from `SliceInput` to `DataSeekableInputStream`, the latter of which does not implement `toString`. Error Prone complains with `ObjectToString` because of that. Note that this only happens with trinodb#14933 merged, because otherwise the `verify` method is not a `@FormatMethod` and Error Prone can't verify correctness of its invocations. Passing `location` instead of `in` as the format string arguments is correct, because the stream is derived from a file at the location named by `location`.

core/trino-main/src/main/java/io/trino/execution/SetSessionTask.java

core/trino-main/src/main/java/io/trino/sql/analyzer/AggregationAnalyzer.java

kokosing · 2023-02-02T10:05:31Z

core/trino-main/src/main/java/io/trino/sql/analyzer/ExpressionAnalyzer.java

@@ -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);


new TrinoException?

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

core/trino-main/src/main/java/io/trino/sql/planner/planprinter/PlanPrinter.java

The `in` parameter recently changed type from `SliceInput` to `DataSeekableInputStream`, the latter of which does not implement `toString`. Error Prone complains with `ObjectToString` because of that. Note that this only happens with #14933 merged, because otherwise the `verify` method is not a `@FormatMethod` and Error Prone can't verify correctness of its invocations. Passing `location` instead of `in` as the format string arguments is correct, because the stream is derived from a file at the location named by `location`.

See also: 2ec0529 This is currently not enforced in Error Prone because it requires annotating more methods in `airlift:log` with `@FormatMethod`, but that's hard to do, because it will change their semantics.

This annotation allows more cases to be checked by the `FormatStringAnnotation` checker. Fixes up all the trivial cases where they can be used as a format methods but weren't; there's also a less trivial case of a method in `ExpressionAnalyzer` which also needs to be annotated, but with a quirk. And also one helper method which now is also a format method, even though it's not currently used this way

This is not entirely a "fix" because passing extra format arguments does not cause failure at runtime, but it's still sloppy and error-prone (and, fittingly, Error Prone will complain about it loudly).

Fixes up all the remaining trivial cases where is could be used as a format method but wasn't. Note that `PlanPrinter#formatFrame` cannot be (trivially) refactored to call `appendDetails` directly, because it's used in two different contexts.

The biggest one is the constructors of `ParquetCorruptionException`, actually.

...and fix remaining trivial uses of them.

The idea of it is simple: when it sees a pair of a format string and format arguments passes to a format method, and these two are parameters of a method, it asks the method to be annotated as a format method as well. This works in tandem with `FormatStringAnnotation` as it allows it to detect more cases of malformed format strings and misused format methods.

ksobolew · 2023-02-03T10:13:36Z

I also added (at the beginning) a small-ish commit which changes some logger calls to use formatted messages. I though it was relevant (it also makes it easier for me to work on the main branch, because in my setup Error Prone complains about these).

findepi

I stopped reviewing before "Make some conditional uses of NodeRepresentation#appendDetails explicit"

I know i will have to re-review everything. Please split the PR into smaller, so that we don;t have to re-review same code multiple times.

findepi · 2023-02-03T12:32:36Z

plugin/trino-cassandra/src/main/java/io/trino/plugin/cassandra/CassandraSession.java

@@ -571,7 +571,7 @@ private <T> T executeWithSession(SessionCallable<T> sessionCallable)
                    throw e;
                }
                long delay = Math.min(schedule.nextDelay().toMillis(), timeLeft);
-                log.warn(e.getMessage());


does log.warn(String) do any interpolation?

does log.warn(String) do any interpolation?

Not at this moment. I made a change upstream to make it easier to detect places like this, but it's a semantics change.

we should not do a semantics change.
what's the problem with log.warn(String) not doing any interpolation?

what's the problem with log.warn(String) not doing any interpolation?

It's arguably inconsistent with the overload which does take parameters. If this overload didn't exist, or if it would be marked as a format method, it would behave the same way the String.format("abcde") behaves, which does do interpolation (the only interpolation it can do is "%%", though). But this is very arguable and people's expectations differ. I would expect this to be consistent across all invocations, but other people don't like it.

But the primary reason I would like log.warn(String) to be a format method is to allow Error Prone detect cases where the log message is constructed using concatenation. log.warn(e.getMessage()); is a casualty of this, unfortunately :)

core/trino-main/src/main/java/io/trino/execution/SetColumnTypeTask.java

findepi · 2023-02-03T12:35:07Z

core/trino-main/src/main/java/io/trino/execution/SetSessionTask.java

@@ -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());


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

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

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

core/trino-main/src/main/java/io/trino/sql/analyzer/ExpressionAnalyzer.java

ksobolew · 2023-02-03T13:41:19Z

I know i will have to re-review everything. Please split the PR into smaller, so that we don;t have to re-review same code multiple times.

Sure. First two: #15962 and #15963

ksobolew · 2023-02-03T14:24:53Z

Next one: #15964

cla-bot bot added the cla-signed label Nov 7, 2022

ksobolew requested review from findepi, Praveen2112 and kokosing November 7, 2022 14:33

kokosing reviewed Nov 7, 2022

View reviewed changes

github-actions bot added the tests:hive label Nov 7, 2022

ksobolew force-pushed the kudi/error-prone-annotate-format-method branch from 2005910 to a954267 Compare November 8, 2022 12:55

ksobolew force-pushed the kudi/error-prone-annotate-format-method branch 2 times, most recently from e66e37e to a2b64a1 Compare November 10, 2022 09:17

kokosing reviewed Nov 16, 2022

View reviewed changes

core/trino-spi/pom.xml Outdated Show resolved Hide resolved

kokosing reviewed Nov 16, 2022

View reviewed changes

core/trino-main/src/main/java/io/trino/execution/SetSessionTask.java Outdated Show resolved Hide resolved

kokosing reviewed Nov 16, 2022

View reviewed changes

ksobolew force-pushed the kudi/error-prone-annotate-format-method branch from a2b64a1 to b57878d Compare November 16, 2022 14:46

martint force-pushed the master branch from d011a28 to d002d09 Compare November 16, 2022 15:35

ksobolew force-pushed the kudi/error-prone-annotate-format-method branch from b57878d to 6d0e340 Compare November 17, 2022 10:24

kokosing reviewed Nov 17, 2022

View reviewed changes

ksobolew mentioned this pull request Nov 25, 2022

Verify tests do not allocate resources early and do not leave them behind #15165

Merged

findepi mentioned this pull request Nov 28, 2022

Flaky "unable to create native thread: possibly out of memory or process/resource limits reached" in Kudu #14487

Closed

ksobolew mentioned this pull request Feb 2, 2023

Fix verify() call in RcFileReader #15936

Merged

ksobolew force-pushed the kudi/error-prone-annotate-format-method branch from 6d0e340 to d28de07 Compare February 2, 2023 09:10

kokosing reviewed Feb 2, 2023

View reviewed changes

ksobolew added 9 commits February 3, 2023 10:36

Fix log messages which are not format strings

c47502a

See also: 2ec0529 This is currently not enforced in Error Prone because it requires annotating more methods in `airlift:log` with `@FormatMethod`, but that's hard to do, because it will change their semantics.

Call semanticException method as a format method in SetColumnTypeTask

0a47b89

Make some conditional uses of NodeRepresentation#appendDetails explicit

6479924

This is not entirely a "fix" because passing extra format arguments does not cause failure at runtime, but it's still sloppy and error-prone (and, fittingly, Error Prone will complain about it loudly).

Annotate Failures#checkCondition as a format method

d2036b3

Annotate format methods in trino-parquet

3a09752

The biggest one is the constructors of `ParquetCorruptionException`, actually.

Annotate all remaining cases of format methods

d1f0b9b

...and fix remaining trivial uses of them.

ksobolew force-pushed the kudi/error-prone-annotate-format-method branch from d28de07 to 5b04fba Compare February 3, 2023 10:12

findepi reviewed Feb 3, 2023

View reviewed changes

This was referenced Feb 3, 2023

Fix log messages which are not format strings #15962

Merged

Annotate semanticException factories as format methods #15963

Closed

ksobolew mentioned this pull request Feb 3, 2023

Annotate NodeRepresentation#appendDetails as a format method #15964

Merged

ksobolew closed this Feb 3, 2023

ksobolew deleted the kudi/error-prone-annotate-format-method branch February 3, 2023 14:25

ksobolew mentioned this pull request Nov 27, 2023

Move formatting out from ParquetCorruptionException #19907

Closed

hashhar mentioned this pull request Feb 22, 2024

Enable format checks on loggers #4189

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable Error Prone check: AnnotateFormatMethod #14933

Enable Error Prone check: AnnotateFormatMethod #14933

ksobolew commented Nov 7, 2022 •

edited

Loading

ksobolew commented Nov 7, 2022

kokosing commented Nov 7, 2022

ksobolew commented Nov 8, 2022

kokosing left a comment

ksobolew commented Nov 16, 2022

ksobolew commented Nov 17, 2022

kokosing commented Nov 17, 2022

kokosing Feb 2, 2023

ksobolew Feb 3, 2023

ksobolew commented Feb 3, 2023

findepi left a comment

findepi Feb 3, 2023

ksobolew Feb 3, 2023

findepi Feb 3, 2023

ksobolew Feb 6, 2023

findepi Feb 3, 2023

ksobolew Feb 3, 2023

findepi Feb 3, 2023

ksobolew commented Feb 3, 2023

ksobolew commented Feb 3, 2023

Enable Error Prone check: AnnotateFormatMethod #14933

Enable Error Prone check: AnnotateFormatMethod #14933

Conversation

ksobolew commented Nov 7, 2022 • edited Loading

Description

Non-technical explanation

Release notes

ksobolew commented Nov 7, 2022

kokosing commented Nov 7, 2022

ksobolew commented Nov 8, 2022

kokosing left a comment

Choose a reason for hiding this comment

ksobolew commented Nov 16, 2022

ksobolew commented Nov 17, 2022

kokosing commented Nov 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ksobolew commented Feb 3, 2023

findepi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ksobolew commented Feb 3, 2023

ksobolew commented Feb 3, 2023

ksobolew commented Nov 7, 2022 •

edited

Loading