[
https://issues.apache.org/jira/browse/GROOVY-12696?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18138432#comment-18138432
]
ASF GitHub Bot commented on GROOVY-12696:
-----------------------------------------
codeconsole opened a new pull request, #3217:
URL: https://github.com/apache/groovy/pull/3217
On the path of `FormatStringChecker` that checks calls whose arguments are
not all constants, `checkFormatStringTypes` used the explicit argument index of
a format specifier (`%1$s`, `%2$d`) directly as a zero-based position in the
argument list. `java.util.Formatter` numbers explicit indices from 1, so the
checker inspected the argument after the one `Formatter` uses.
`String.format('%1$s', s)` with a `String s` parameter was rejected with
`MissingFormatArgument`, `String.format('%2$s %s %<s %1$s', s, i)` likewise,
and `String.format('%1$d', s, i)` with `Integer i` was accepted and failed at
run time with `IllegalFormatConversionException: d != java.lang.String`,
because the second argument was checked instead of the first. Calls whose
arguments are all constants were not affected, since that path runs `Formatter`
itself.
The change subtracts one from an explicit index before it is used (`int
indexToUse = argIndex ? argIndex - 1 : next`), so explicit, ordinary and
relative (`%<`) specifiers resolve to the same arguments as `Formatter` does.
The ordinary counter and the index remembered for `%<` were already zero-based
and are unchanged, as is the constant-argument path. No public API changes.
Tested with two new tests in `FormatStringCheckerTest`:
`testExplicitArgumentIndexForInferredArgs` asserts that valid calls with
explicit indices, including ones mixed with ordinary and `%<` specifiers and an
`sprintf` call, compile and produce the same output as `Formatter`;
`testExplicitArgumentIndexErrorsForInferredArgs` asserts that
`IllegalFormatConversion` is reported for the argument the index actually names
(including through `%<`) and that an index beyond the argument count is still
reported as `MissingFormatArgument`. Both tests fail on master without the fix
and pass with it. `./gradlew :groovy-typecheckers:test` (394 tests) and
`./gradlew :groovy-typecheckers:lintGate :groovy-typecheckers:rat` pass on JDK
21.0.12. The reproducer scripts were also run against a `installGroovy`
distribution built from this branch and now behave like `Formatter` on every
case, while 6.0.0, 6.0.1-SNAPSHOT and master without the fix all reproduce the
defect.
Relation to #3213 (GROOVY-12692): that pull request changes the same method,
teaching it that `%%` and `%n` take no argument. The two changes are
independent and are based on the same master commit (d86ea7eb85). A three-way
merge (`git merge-file`) of both the checker and the test class merges cleanly
in either order; the new tests here are placed at the end of the test class,
after the ones #3213 adds.
Suggested backport: GROOVY_6_0_X, where the same code is present.
> FormatStringChecker treats explicit argument index as zero-based when
> arguments are not constants
> -------------------------------------------------------------------------------------------------
>
> Key: GROOVY-12696
> URL: https://issues.apache.org/jira/browse/GROOVY-12696
> Project: Groovy
> Issue Type: Bug
> Affects Versions: 6.0.0
> Reporter: Scott Murphy Heiberg
> Priority: Major
>
> *Environment:* macOS 26.5, JDK 21.0.12 (BellSoft Liberica), Groovy 6.0.0,
> 6.0.1-SNAPSHOT (GROOVY_6_0_X at 7c0dcf17dd) and master (7.0.0-SNAPSHOT at
> d86ea7eb85)
> {{groovy.typecheckers.FormatStringChecker}} has two paths. When every
> argument of a format call is a constant it runs {{java.util.Formatter}}
> itself and reports whatever {{Formatter}} throws. When at least one argument
> is not a constant it walks the format specifiers and checks each argument's
> static type. On that second path an explicit argument index ({{%1$s}},
> {{%2$d}}, ...) is used directly as a zero-based position in the argument
> list, while {{Formatter}} numbers explicit indices from 1. The checker
> therefore looks at the argument after the one {{Formatter}} will use: it
> rejects valid calls and accepts calls that throw at run time.
> h3. Reproducer
> {code:groovy}
> import groovy.transform.TypeChecked
> @TypeChecked(extensions = 'groovy.typecheckers.FormatStringChecker')
> class Repro {
> static String run(String s, Integer i) {
> String.format('%1$s', s)
> }
> }
> println '[' + Repro.run('x', 1) + ']'
> {code}
> Actual output ({{groovy Repro.groovy}}):
> {noformat}
> org.codehaus.groovy.control.MultipleCompilationErrorsException: startup
> failed:
> Repro.groovy: 6: [Static type checking] - MissingFormatArgument: Format
> specifier '%1$s'
> @ line 6, column 9.
> String.format('%1$s', s)
> ^
> 1 error
> {noformat}
> Expected: the script compiles and prints {{[x]}}. {{String.format('%1$s',
> 'x')}} is valid and returns {{x}}; with the constant argument {{'x'}} in
> place of {{s}} the checker accepts the call.
> The opposite case, with the same class but
> {code:groovy}
> String.format('%1$d', s, i)
> {code}
> compiles without a type error and then fails at run time:
> {noformat}
> Caught: java.util.IllegalFormatConversionException: d != java.lang.String
> {noformat}
> Expected: a compile-time error {{IllegalFormatConversion: d !=
> java.lang.String}}, which is what the checker reports for the same call with
> constant arguments, {{String.format('%1$d', 'x', 1)}}.
> More calls with {{String s}} and {{Integer i}} parameters, run on all three
> versions listed above:
> || call || Formatter at run time || FormatStringChecker ||
> | {{String.format('%1$s', s)}} | {{x}} | MissingFormatArgument: Format
> specifier '%1$s' |
> | {{String.format('%2$s', s, i)}} | {{1}} | MissingFormatArgument: Format
> specifier '%2$s' |
> | {{String.format('%2$d %1$s', s, i)}} | {{1 x}} | MissingFormatArgument:
> Format specifier '%2$d' |
> | {{String.format('%1$s %2$d', s, i)}} | {{x 1}} | MissingFormatArgument:
> Format specifier '%2$d' |
> | {{String.format('%2$s %s %<s %1$s', s, i)}} | {{1 x x x}} |
> MissingFormatArgument: Format specifier '%2$s' |
> | {{String.format('%1$d', i, s)}} | {{1}} | IllegalFormatConversion: d !=
> java.lang.String (checks {{s}} instead of {{i}}) |
> | {{String.format('%1$d', s, i)}} | IllegalFormatConversionException: d !=
> java.lang.String | accepted |
> | {{String.format('%3$s', s, i)}} | MissingFormatArgumentException |
> MissingFormatArgument: Format specifier '%3$s' (correct only because 3 and 2
> both exceed the last index) |
> | {{String.format('%1$s', 'x')}} (constants) | {{x}} | accepted |
> | {{String.format('%1$d', 'x', 1)}} (constants) |
> IllegalFormatConversionException | IllegalFormatConversion: d !=
> java.lang.String |
> h3. Cause
> In {{FormatStringChecker.checkFormatStringTypes}}
> (subprojects/groovy-typecheckers/src/main/groovy/groovy/typecheckers/FormatStringChecker.groovy,
> line 184 on master):
> {code:groovy}
> int indexToUse = argIndex ?: next
> {code}
> {{argIndex}} is the integer parsed from the {{n$}} prefix and is used as is
> to index {{args}}, which is zero-based. The ordinary counter {{next}} and the
> index remembered for {{%<}} are zero-based, so an explicit index is off by
> one relative to them and to {{Formatter}}, whose {{format}} loop uses {{index
> - 1}} for an explicit index. The bounds check {{indexToUse >= args.size()}}
> then rejects {{%n$}} whenever {{n}} equals the argument count, and the type
> check otherwise inspects argument {{n + 1}} instead of {{n}}. The
> constant-argument path ({{checkFormatStringConstantArgs}}) is not affected.
> The fix is to subtract one from an explicit index before using it: {{int
> indexToUse = argIndex ? argIndex - 1 : next}}. With that change every call in
> the table above is checked the same way {{Formatter}} resolves it. A pull
> request with regression tests follows.
> h3. Versions
> Reproduces on Groovy 6.0.0, 6.0.1-SNAPSHOT (GROOVY_6_0_X at 7c0dcf17dd) and
> master (d86ea7eb85). The line has been unchanged since the checker was added
> for GROOVY-11261 (5.0.0-alpha-5), so 5.0.x is presumably affected as well,
> though that was not run.
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
