RFR: JDK-8261422: Adjust problematic String.format calls in jdk/internal/util/Preconditions.java outOfBoundsMessage

[Matthias Baesken]

JDK-8261422: Adjust problematic String.format calls in jdk/internal/util/Preconditions.java outOfBoundsMessage

-------------

Commit messages:
 - JDK-8261422

Changes: https://git.openjdk.java.net/jdk/pull/2483/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2483&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8261422
  Stats: 4 lines in 1 file changed: 0 ins; 0 del; 4 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2483.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2483/head:pull/2483

PR: https://git.openjdk.java.net/jdk/pull/2483

[Matthias Baesken]

On Tue, 9 Feb 2021 14:33:22 GMT, Matthias Baesken <[hidden email]> wrote:

> JDK-8261422: Adjust problematic String.format calls in jdk/internal/util/Preconditions.java outOfBoundsMessage

The method outOfBoundsMessage has a few problematic calls to String.format.
Those calls use "%d" however the arguments are of type Number, so we should better use %s .

Example :

https://sonarcloud.io/project/issues?id=shipilev_jdk&languages=java&open=AXcqM87A8sPJZZzON5wY&resolved=false&severities=BLOCKER&types=BUG

return String.format("Index %d out of bounds for length %d",
                     args.get(0), args.get(1));

Sonar error :
An 'int' is expected rather than a .

-------------

PR: https://git.openjdk.java.net/jdk/pull/2483

[Alan Bateman-2]

On Tue, 9 Feb 2021 14:33:22 GMT, Matthias Baesken <[hidden email]> wrote:

> JDK-8261422: Adjust problematic String.format calls in jdk/internal/util/Preconditions.java outOfBoundsMessage

src/java.base/share/classes/jdk/internal/util/Preconditions.java line 212:

> 210:                                      args.get(0), args.get(1), args.get(2));
> 211:             case "checkFromIndexSize":
> 212:                 return String.format("Range [%s, %<s + %s) out of bounds for length %s",

I assume we have a code coverage here and that we need tests to exercise these code paths.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2483

[Matthias Baesken]

On Tue, 9 Feb 2021 15:38:29 GMT, Alan Bateman <[hidden email]> wrote:

>> JDK-8261422: Adjust problematic String.format calls in jdk/internal/util/Preconditions.java outOfBoundsMessage
>
> src/java.base/share/classes/jdk/internal/util/Preconditions.java line 212:
>
>> 210:                                      args.get(0), args.get(1), args.get(2));
>> 211:             case "checkFromIndexSize":
>> 212:                 return String.format("Range [%s, %<s + %s) out of bounds for length %s",
>
> I assume we have a code coverage here and that we need tests to exercise these code paths.

I haven't written the class Preconditions.java, this question should probably go to the authors .
I found the String.format syntax a bit  unusual,  should this be rewritten ?

-------------

PR: https://git.openjdk.java.net/jdk/pull/2483

[stefan-zobel]

On Tue, 9 Feb 2021 15:47:39 GMT, Matthias Baesken <[hidden email]> wrote:

>> src/java.base/share/classes/jdk/internal/util/Preconditions.java line 212:
>>
>>> 210:                                      args.get(0), args.get(1), args.get(2));
>>> 211:             case "checkFromIndexSize":
>>> 212:                 return String.format("Range [%s, %<s + %s) out of bounds for length %s",
>>
>> I assume we have a code coverage here and that we need tests to exercise these code paths.
>
> I haven't written the class Preconditions.java, this question should probably go to the authors .
> I found the String.format syntax a bit  unusual,  should this be rewritten ?

AFAIK, Preconditions was introduced in https://bugs.openjdk.java.net/browse/JDK-8155794

-------------

PR: https://git.openjdk.java.net/jdk/pull/2483

[Christoph Langer]

On Tue, 9 Feb 2021 14:33:22 GMT, Matthias Baesken <[hidden email]> wrote:

> JDK-8261422: Adjust problematic String.format calls in jdk/internal/util/Preconditions.java outOfBoundsMessage

As we're potentially formatting any "Number" type here, theoretically floats could be passed which would result in an Exception. In fact, only decimal types are passed by the callers so this should not happen. Unfortunately there's no super type specifying decimal numbers.

And as we're not doing some fancy formatting but just %d, I think replacing this with %s should be ok.

-------------

Marked as reviewed by clanger (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2483

[Brian Burkhalter-3]

On Tue, 9 Feb 2021 16:04:02 GMT, stefan-zobel <[hidden email]> wrote:

>> I haven't written the class Preconditions.java, this question should probably go to the authors .
>> I found the String.format syntax a bit  unusual,  should this be rewritten ?
>
> AFAIK, Preconditions was introduced in https://bugs.openjdk.java.net/browse/JDK-8155794

It looks like java/util/Objects/CheckIndex should adequately cover this change.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2483

[Matthias Baesken]

On Tue, 9 Feb 2021 14:33:22 GMT, Matthias Baesken <[hidden email]> wrote:

> JDK-8261422: Adjust problematic String.format calls in jdk/internal/util/Preconditions.java outOfBoundsMessage

This pull request has now been integrated.

Changeset: 219b115e
Author:    Matthias Baesken <[hidden email]>
URL:       https://git.openjdk.java.net/jdk/commit/219b115e
Stats:     4 lines in 1 file changed: 0 ins; 0 del; 4 mod

8261422: Adjust problematic String.format calls in jdk/internal/util/Preconditions.java outOfBoundsMessage

Reviewed-by: clanger

-------------

PR: https://git.openjdk.java.net/jdk/pull/2483

[Alan Bateman-2]

On Fri, 12 Feb 2021 11:02:36 GMT, Christoph Langer <[hidden email]> wrote:

>> JDK-8261422: Adjust problematic String.format calls in jdk/internal/util/Preconditions.java outOfBoundsMessage
>
> As we're potentially formatting any "Number" type here, theoretically floats could be passed which would result in an Exception. In fact, only decimal types are passed by the callers so this should not happen. Unfortunately there's no super type specifying decimal numbers.
>
> And as we're not doing some fancy formatting but just %d, I think replacing this with %s should be ok.

I see this change been integrated but there is further work required on the test coverage. Are you planing to do code coverage and create a follow-on issue to add more tests?

-------------

PR: https://git.openjdk.java.net/jdk/pull/2483