<i18n dev> RFR: 8253459: Formatter treats index, width and precision > Integer.MAX_VALUE incorrectly [v5]

[Marcono1234]

On Thu, 15 Oct 2020 19:20:28 GMT, Ian Graves <igraves at openjdk.org> wrote:

>> The `java.util.Formatter` format specifies support for field widths, argument indexes, or precision lengths of a field that relate to the variadic arguments supplied to the formatter. These numbers are specified by integers, sometimes negative. For argument index, it's specified in the documentation that the highest allowed argument is limited by the largest possible index of an array (ie the largest possible variadic index), but for the other two it's not defined. Moreover, what happens when a number field in a string is too large or too small to be represented by a 32-bit integer type is not defined.
>> 
>> This fix adds documentation to specify what error behavior occurs during these cases. Additionally it adds an additional exception type to throw when an invalid argument index is observed.
>> 
>> A CSR will be required for this PR.
>
> Ian Graves has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Additional specificity around width

Maybe it would be cleaner for `IllegalFormatArgumentIndexException`, `IllegalFormatWidthException` and `IllegalFormatPrecisionException` to provide no-arg constructors (which use `Integer.MIN_VALUE` as value) for the cases where the value cannot be represented.

Are you also planning to add some tests? That would probably be useful.

And would it be necessary to mention the exception for `index=0` in the release notes? I am personally a little bit afraid that there is code out there which uses it (since indices starting with 0 is not that uncommon). For example when someone used only one format specifier but wanted to be explicit, then a value of 0 would not have caused any issues for them in the past.

For what it's worth, personally I prefer having a separate exception type (`IllegalFormatArgumentIndexException`) to make it consistent with the other exceptions. The large number of exception types might not be ideal, especially since the user likely rarely needs to differentiate between them, but this pattern was chosen back then and it makes sense to stick to it now.

Might also be good to make the documentation for `getIndex()`, `getPrecision()` and `getWidth()` more consistent:
- `getIndex()`:
  - Starts with "Gets the ..."
  - "Returns ... if not ..."
- `getPrecision()`:
  - Starts with "Returns the ..."
  - "If the ... isn't ... will return"
- `getWidth()`:
  - Starts with "Returns the ..."
  - "If the ... is not ... then returns"

----

Note that I am neither an OpenJDK contributor nor have I signed the Terms of Use. Please let me know if should stop bothering you on this pull request.

src/java.base/share/classes/java/util/Formatter.java line 708:

> 706:  * <p> Values of <i>width</i> must be in the range one to
> 707:  * {@link Integer#MAX_VALUE}, inclusive, otherwise
> 708:  * {@link IllegalFormatWidthException} will be thrown

Missing period.
Suggestion:

 * {@link IllegalFormatWidthException} will be thrown.

src/java.base/share/classes/java/util/Formatter.java line 711:

> 709:  * Note that widths can appear to have a negative value, but the negative sign
> 710:  * is a <i>flag</i>. For example in the format string {@code "%-20s"} the
> 711:  * <i>width</i> is <i>20</i> and the <i>flag</i> is "-".</p>

Should the `"-"` here be formatted either as italic (to match the `<i>20</i>`; though not sure if italic is really needed there in the first place) or as code?

src/java.base/share/classes/java/util/Formatter.java line 2802:

> 2800:                     // skip the trailing '$'
> 2801:                     index = Integer.parseInt(s, start, end - 1, 10);
> 2802:                     if(index <= 0) {

Suggestion:

                    if (index <= 0) {

src/java.base/share/classes/java/util/IllegalFormatArgumentIndexException.java line 32:

> 30:  * range of supported argument index values. If an index value isn't
> 31:  * representable by an {@code int} type, then the value
> 32:  * {@code Integer.MIN_VALUE} will be used in the exception.

Maybe use a `{@link ...}` to make it easier for the reader of the documentation?

src/java.base/share/classes/java/util/IllegalFormatArgumentIndexException.java line 53:

> 51:     /**
> 52:      * Gets the value of the illegal index.
> 53:      * Returns {@code Integer.MIN_VALUE} if the illegal index is not

Maybe use a `{@link ...}` to make it easier for the reader of the documentation?

src/java.base/share/classes/java/util/IllegalFormatPrecisionException.java line 32:

> 30:  * {@code -1}, the conversion does not support a precision, or the value is
> 31:  * otherwise unsupported. If the precision is not representable by an
> 32:  * {@code int} type, then the value {@code Integer.MIN_VALUE} will be used

Maybe use a `{@link ...}` to make it easier for the reader of the documentation?

src/java.base/share/classes/java/util/IllegalFormatPrecisionException.java line 56:

> 54:     /**
> 55:      * Returns the precision. If the precision isn't representable by an
> 56:      * integer type, then will return {@code Integer.MIN_VALUE}.

Maybe use a `{@link ...}` to make it easier for the reader of the documentation?

src/java.base/share/classes/java/util/IllegalFormatPrecisionException.java line 56:

> 54:     /**
> 55:      * Returns the precision. If the precision isn't representable by an
> 56:      * integer type, then will return {@code Integer.MIN_VALUE}.

Suggestion:

     * {@code int} type, then will return {@code Integer.MIN_VALUE}.
To be consistent with other documentation and to be more explicit (`long` could also be understood as integer type).

src/java.base/share/classes/java/util/IllegalFormatWidthException.java line 54:

> 52: 
> 53:     /**
> 54:      * Returns the width. If the width is not representable by an integer type,

Suggestion:

     * Returns the width. If the width is not representable by an {@code int} type,
To be consistent with other documentation and to be more explicit (`long` could also be understood as integer type).

src/java.base/share/classes/java/util/IllegalFormatWidthException.java line 32:

> 30:  * than {@code -1} or is otherwise unsupported. If a given format width is not
> 31:  * representable by an {@code int} type, then the value
> 32:  * {@code Integer.MIN_VALUE} will be used in the exception.

Maybe use a `{@link ...}` to make it easier for the reader of the documentation?

src/java.base/share/classes/java/util/IllegalFormatWidthException.java line 55:

> 53:     /**
> 54:      * Returns the width. If the width is not representable by an integer type,
> 55:      * then returns {@code Integer.MIN_VALUE}.

Maybe use a `{@link ...}` to make it easier for the reader of the documentation?

src/java.base/share/classes/java/util/IllegalFormatPrecisionException.java line 30:

> 28: /**
> 29:  * Unchecked exception thrown when the precision is a negative value other than
> 30:  * {@code -1}, the conversion does not support a precision, or the value is

-1 being used by `java.util.Formatter.FormatSpecifier.precision(String, int, int)` as default value is in my opinion an implementation detail and should not be part of this documentation.

src/java.base/share/classes/java/util/IllegalFormatWidthException.java line 30:

> 28: /**
> 29:  * Unchecked exception thrown when the format width is a negative value other
> 30:  * than {@code -1} or is otherwise unsupported. If a given format width is not

-1 being used by `java.util.Formatter.FormatSpecifier.width(String, int, int)` as default value is in my opinion an implementation detail and should not be part of this documentation.

src/java.base/share/classes/java/util/IllegalFormatArgumentIndexException.java line 52:

> 50: 
> 51:     /**
> 52:      * Gets the value of the illegal index.

Maybe drop "illegal" here, for `@return` and for the constructor, or (preferred) add it to the existing `IllegalFormatPrecisionException` and `IllegalFormatWidthException` getter and constructor documentation for consistency.

src/java.base/share/classes/java/util/Formatter.java line 2802:

> 2800:                     // skip the trailing '$'
> 2801:                     index = Integer.parseInt(s, start, end - 1, 10);
> 2802:                     if(index <= 0) {

Note that the `java.util.Formatter.formatSpecifier` pattern does not permit a `-` in the first place, so maybe throwing an `AssertionError` would be more appropriate?

Same applies to `width` and `precision`.

src/java.base/share/classes/java/util/Formatter.java line 713:

> 711:  * <i>width</i> is <i>20</i> and the <i>flag</i> is "-".</p>
> 712:  *
> 713:  * <p> Values of <i>index</i> must be in the range one to

Suggestion:

 * <p> Values of <i>index</i> must be in the range 1 to
Maybe write a number here to make it faster to read.

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

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