codeconsole opened a new pull request, #3213: URL: https://github.com/apache/groovy/pull/3213
JIRA: [GROOVY-12692](https://issues.apache.org/jira/browse/GROOVY-12692) ### The bug With `groovy.typecheckers.FormatStringChecker` enabled, a format call whose arguments are not all constants is checked as if `%%` and `%n` each took an argument. A valid call is rejected: ```groovy import groovy.transform.TypeChecked @TypeChecked(extensions = 'groovy.typecheckers.FormatStringChecker') String show(String s) { String.format('100%% %s', s) } ``` ```text [Static type checking] - MissingFormatArgument: Format specifier '%s' ``` `java.util.Formatter` gives `%%` and `%n` no argument, so the call returns `100% done` for `s = 'done'`. With a constant argument the checker accepts the same format string. On the same path: | call | `java.util.Formatter` | checker before | | --- | --- | --- | | `String.format('%s%n', s)` | `done` and a line separator | MissingFormatArgument `'%n'` | | `String.format('%s %%', s)` | `done %` | MissingFormatArgument `'%%'` | | `String.format('%s %% %<s', s)` | `done % done` | MissingFormatArgument `'%%'` | | `String.format('100%% %d', s)` | IllegalFormatConversion `d != java.lang.String` | MissingFormatArgument `'%d'` | | `String.format('%% %<s', s)` | MissingFormatArgument `'%<s'` | no error | ### Cause On the non-constant path, `checkFormatStringTypes` handled every specifier that the format pattern matches as one that takes an argument. It checked the index against the number of arguments, read that argument's type, and advanced both the next index and the previous argument that `%<` refers to. The `switch` after that skipped `%` and `n`, but only after the bookkeeping. The constant path runs `java.util.Formatter` itself and was not affected. ### Fix `%%` and `%n` are now handled before the argument bookkeeping: they get the duplicate-flag check they already had, and nothing else. The duplicate-flag check moves into a `checkDupFlags` helper, next to `checkBadFlags` and `checkBadWidth`, so both places use it. The `case '%'` / `case 'n'` branch in the `switch` can no longer be reached and is removed. `%%` and `%n` are the only conversions that take no argument: on JDK 21, `String.format` with no argument succeeds for `%` and `n` and fails for every other letter. `%t%` still goes through the date/time branch and is reported as `UnknownFormatConversion`, as before. Not changed: this path still checks only duplicate flags on `%%` and `%n`. Their other flags, width and precision are not checked, as before. On master, a malformed one such as `%+%`, `%5n` or `%<%` was usually rejected, but only through this bug: the error was a `MissingFormatArgument`, often for another specifier. With this change it is accepted on this path, and `Formatter` rejects it at run time. Checking those the way `Formatter` does would be a separate improvement. ### Tests Two new tests in `FormatStringCheckerTest`: - `testPercentAndLineSeparatorTakeNoArgumentForInferredArgs`: `%%` and `%n` before, after and between non-constant arguments, with `%<` after them and with a width on `%%`, compile and give the `Formatter` result. - `testFormatErrorsAfterPercentAndLineSeparatorForInferredArgs`: errors next to `%%` and `%n` are reported as the right error: `IllegalFormatConversion` for `'100%% %d'`, `MissingFormatArgument` naming the `%s` for `'%s %% %s'` and `'%s%n%s'` with one argument, `MissingFormatArgument` for `'%% %<s'`, and the existing `DuplicateFormatFlags` for `'%--3% %s'`. Both tests failed on master. In the first, all eight calls were rejected with `MissingFormatArgument`. In the second, the first four cases gave the wrong result (`MissingFormatArgument` for `'%d'`, `'%%'` and `'%n'`, and no error for `'%% %<s'`), and the duplicate-flag case already passed. Both pass with the change. ```text ./gradlew :groovy-typecheckers:test --tests groovy.typecheckers.FormatStringCheckerTest master: 2 of 20 failed; with the change: 20 passed ./gradlew :groovy-typecheckers:test --rerun 388 tests, 0 failures ./gradlew :groovy-typecheckers:lintGate :groovy-typecheckers:rat BUILD SUCCESSFUL ``` ### Affected versions Reproduced with JDK 21 on the Groovy 6.0.0, 6.0.0-beta-1, 5.0.8 and 5.0.0 distributions and on master (7.0.0-SNAPSHOT at 5e9212ce1e). ### Backport Suggested target: `GROOVY_6_0_X`. `FormatStringChecker.groovy` and `FormatStringCheckerTest.groovy` are the same there as on master. The change also merges without conflicts with #3207 (GROOVY-12688) and #3208 (GROOVY-12689), which change other lines of the same file, alone or together. -- 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]
