8327640: Allow NumberFormat strict parsing #18339

justin-curtis-lu · 2024-03-18T06:24:54Z

Please review this PR and associated CSR which introduces strict parsing for NumberFormat.

The concrete subclasses that will utilize this leniency value are DecimalFormat and CompactNumberFormat. Strict leniency allows for parsing to be used as an input validation technique for localized formatted numbers. The default leniency mode will remain lenient, and can only be set to strict through an intentional setLenient(boolean) method call. Leniency operates differently based off the values returned by isGroupingUsed(), getGroupingSize(), and isParseIntegerOnly().

Below is an example of the change, the CSR can be viewed for further detail.

DecimalFormat fmt = (DecimalFormat) NumberFormat.getNumberInstance(Locale.US);
fmt.parse("1,,,0,,,00,.23Zabced45");  // returns 1000.23
fmt.setLenient(false);
fmt.parse("1,,,0,,,00,.23Zabced45"); // Now throws a ParseException
fmt.setGroupingSize(2);
fmt.parse("12,34,567"); // throws ParseException
fmt.setParseIntegerOnly(true)
fmt.parse("12,34.56"); // throws ParseException
fmt.parse("12,34"); // successfully returns the Number 1234

The associated tests are all localized, and the data is converted properly during runtime.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue
Change requires CSR request JDK-8327703 to be approved

Issues

JDK-8327640: Allow NumberFormat strict parsing (Enhancement - P4)
JDK-8327703: Allow NumberFormat strict parsing (CSR)

Reviewers

Naoto Sato (@naotoj - Reviewer) ⚠️ Review applies to eec992e3

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/18339/head:pull/18339
$ git checkout pull/18339

Update a local copy of the PR:
$ git checkout pull/18339
$ git pull https://git.openjdk.org/jdk.git pull/18339/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 18339

View PR using the GUI difftool:
$ git pr show -t 18339

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/18339.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2024-03-18T06:25:43Z

👋 Welcome back jlu! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2024-03-18T06:26:35Z

@justin-curtis-lu This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8327640: Allow NumberFormat strict parsing

Reviewed-by: naoto

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 14 new commits pushed to the master branch:

397d948: 8329605: hs errfile generic events - move memory protections and nmethod flushes to separate sections
f706949: 8329781: Serial: Remove serialFullGC.inline.hpp
0f78d01: 8329658: Serial: Refactor ContiguousSpace::_next_compaction_space
b8f675f: 8329771: G1: Refactor G1BlockOffsetTable::verify
3e9c381: 8329488: Move OopStorage code from safepoint cleanup and remove safepoint cleanup code
77a217d: 8330095: RISC-V: Remove obsolete vandn_vi instruction
717a07b: 8322140: javax/swing/JTable/JTableScrollPrintTest.java does not print the rows and columns of the table in Nimbus and Aqua LookAndFeel
aebfd53: 8329660: G1: Improve TestGCLogMessages to be more precise
006a516: 8329962: Remove CardTable::invalidate
c7fcd62: 8330006: Serial: Extract out ContiguousSpace::block_start_const
... and 4 more: https://git.openjdk.org/jdk/compare/ece7d4349a13f75c654e2ca0f4d1b66d3af5cf10...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2024-03-18T06:26:59Z

@justin-curtis-lu The following labels will be automatically applied to this pull request:

core-libs
i18n

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

mlbridge · 2024-03-18T06:29:39Z

Webrevs

Replace setLenient() with setStrict() to avoid messy inversion of boolean. Add a leniency section to NumberFormat. Overhaul of NumberFormat class specification to be much more organized/readable.

turbanoff · 2024-03-19T08:55:58Z

src/java.base/share/classes/java/text/DecimalFormat.java

-                gotPositive = text.regionMatches(position,positiveSuffix,0,
-                        positiveSuffix.length());
+                boolean containsPosSuffix =
+                        text.regionMatches(position, positiveSuffix,0, positiveSuffix.length());


Suggested change

text.regionMatches(position, positiveSuffix,0, positiveSuffix.length());

text.regionMatches(position, positiveSuffix, 0, positiveSuffix.length());

turbanoff · 2024-03-19T08:56:12Z

src/java.base/share/classes/java/text/DecimalFormat.java

-                gotNegative = text.regionMatches(position,negativeSuffix,0,
-                        negativeSuffix.length());
+                boolean containsNegSuffix =
+                        text.regionMatches(position, negativeSuffix,0, negativeSuffix.length());


Suggested change

text.regionMatches(position, negativeSuffix,0, negativeSuffix.length());

text.regionMatches(position, negativeSuffix, 0, negativeSuffix.length());

turbanoff · 2024-03-19T08:56:29Z

src/java.base/share/classes/java/text/DecimalFormat.java

+                boolean containsNegSuffix =
+                        text.regionMatches(position, negativeSuffix,0, negativeSuffix.length());
+                boolean endsWithNegSuffix =
+                        containsNegSuffix && text.length() == position  + negativeSuffix.length();


Suggested change

containsNegSuffix && text.length() == position + negativeSuffix.length();

containsNegSuffix && text.length() == position + negativeSuffix.length();

turbanoff · 2024-03-19T08:56:46Z

src/java.base/share/classes/java/text/DecimalFormat.java

@@ -2438,7 +2479,7 @@ int subparseNumber(String text, int position,
                       boolean isExponent, boolean[] status) {
        // process digits or Inf, find decimal position
        status[STATUS_INFINITE] = false;
-        if (!isExponent && text.regionMatches(position,symbols.getInfinity(),0,
+        if (!isExponent && text.regionMatches(position, symbols.getInfinity(),0,


Suggested change

if (!isExponent && text.regionMatches(position, symbols.getInfinity(),0,

if (!isExponent && text.regionMatches(position, symbols.getInfinity(), 0,

Thanks, this and the other suggestions you provided should be fixed.

- specify and throw UOE as default - override and implement in dFmt and cmpctNFmt parseStrict should be protected, and utilized by subclasses. The public methods should just serve as API.

naotoj

Looks good overall. Left some minor comments.
As to the tests, good to see those corner cases, but they should have unit tests for the new methods, i.e, isStrict() and setStrict(). Also I think equality/serialization for those methods should be examined.

naotoj · 2024-04-11T01:29:11Z

src/java.base/share/classes/java/text/DecimalFormat.java

+import sun.util.locale.provider.LocaleProviderAdapter;
+import sun.util.locale.provider.ResourceBundleBasedAdapter;
+


Internal packages typically follow public packages.

Fixed. (IntelliJ is adamant on having it that way, and even after I previously reverted it, it seems like it slipped through again. Oddly, it never used to do this, need to look at the settings)

naotoj · 2024-04-11T01:32:25Z

src/java.base/share/classes/java/text/DecimalFormat.java

@@ -2575,7 +2648,24 @@ int subparseNumber(String text, int position,
            }
        }
        return position;
+    }
+
+    // Checks to make sure grouping size is not violated. Used when strict.


If it is only supposed to be used in strict, might be helpful to put an assertion here.

naotoj · 2024-04-11T01:34:18Z

src/java.base/share/classes/java/text/DecimalFormat.java


+    // Calculates the index that violated the grouping size
+    // Calculation is determined whether it was an under or over violation


Not quite sure what under/over violation means here. Need more comments?

naotoj · 2024-04-11T01:36:34Z

src/java.base/share/classes/java/text/NumberFormat.java

+import sun.util.locale.provider.LocaleProviderAdapter;
+import sun.util.locale.provider.LocaleServiceProviderPool;
+


Same as above

naotoj · 2024-04-11T01:38:25Z

src/java.base/share/classes/java/text/NumberFormat.java

+     * @since 23
+     */
+    public boolean isStrict() {
+        throw new UnsupportedOperationException();


Some message might be helpful.

naotoj · 2024-04-11T01:38:37Z

src/java.base/share/classes/java/text/NumberFormat.java

+     * @since 23
+     */
+    public void setStrict(boolean strict) {
+        throw new UnsupportedOperationException();


justin-curtis-lu · 2024-04-11T20:51:48Z

Thank you for the review @naotoj. All of your comments should be reflected in the two previous commits.

As for the test suggestions, I added to existing tests if possible, otherwise I created new test files to address your suggestions. The new DecimalFormat equality and serialization test can definitely have other qualities tested (beyond parseStrict), but perhaps that's best for a separate issue.

naotoj

LGTM. Thanks for the changes.

justin-curtis-lu · 2024-04-16T16:17:38Z

/integrate

openjdk · 2024-04-16T16:18:12Z

Going to push as commit 941bee1.
Since your change was applied there have been 46 commits pushed to the master branch:

2ede143: 8330279: Typo in java.text.Bidi class description
90df3b7: 8329190: (ch) DatagramChannel.receive should throw ClosedChannelException when called on closed channel
f11a496: 8180450: secondary_super_cache does not scale well
bfff02e: 8330165: C2: make superword consistently use PhaseIdealLoop::register_new_node()
e073d5b: 8329665: fatal error: memory leak: allocating without ResourceMark
6e77d91: 8330261: Clean up linking steps
61fa4d4: 8330351: Remove the SHARED_LIBRARY and STATIC_LIBRARY macros
56ff87a: 8330359: G1: Remove unused forward declaration in g1BlockOffsetTable.hpp
8a5b86c: 8196106: Support nested infinite or recursive flat mapped streams
58911cc: 8188784: javax/management/notification/BroadcasterSupportDeadlockTest.java - TEST FAILED: deadlock
... and 36 more: https://git.openjdk.org/jdk/compare/ece7d4349a13f75c654e2ca0f4d1b66d3af5cf10...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-04-16T16:18:18Z

@justin-curtis-lu Pushed as commit 941bee1.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

init

8843c56

openjdk bot added csr Pull request needs approved CSR before integration rfr Pull request is ready for review labels Mar 18, 2024

openjdk bot added core-libs core-libs-dev@openjdk.org i18n i18n-dev@openjdk.org labels Mar 18, 2024

Replace protected field with a public getter -> isStrict().

c3a3250

Replace setLenient() with setStrict() to avoid messy inversion of boolean. Add a leniency section to NumberFormat. Overhaul of NumberFormat class specification to be much more organized/readable.

turbanoff reviewed Mar 19, 2024

View reviewed changes

justin-curtis-lu added 11 commits March 20, 2024 10:56

remove unsupported methods header, should go in JDK-8327875 instead

6f1b53a

implement white space suggestions

2552e57

Re-work specification wording from Format down to subclasses

c09a34d

setStrict and isStrict should be optional in NumberFormat

ab66026

- specify and throw UOE as default - override and implement in dFmt and cmpctNFmt parseStrict should be protected, and utilized by subclasses. The public methods should just serve as API.

drop Format implNote as well

0c811c1

replace protected field for private fields in subclasses for consistency

4edb480

update example for lenient parsing

100f293

use directed 'inheritDoc' tag

f607c06

revert cleanup changes; (to go into a separate change)

25187a1

Merge branch 'master' into JDK-8327640-NumberFormat-strictParsing

fee51bf

improve wording consistency and see tags

3f2b097

openjdk bot removed the csr Pull request needs approved CSR before integration label Apr 4, 2024

Merge branch 'master' into JDK-8327640-NumberFormat-strictParsing

aa1c284

openjdk bot changed the title ~~JDK-8327640: Allow NumberFormat strict parsing~~ 8327640: Allow NumberFormat strict parsing Apr 10, 2024

naotoj reviewed Apr 11, 2024

View reviewed changes

reflect impl suggestions

7ffcc91

Reflect test suggestions

eec992e

naotoj approved these changes Apr 11, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Apr 11, 2024

justin-curtis-lu added 2 commits April 11, 2024 16:13

Merge branch 'master' into JDK-8327640-NumberFormat-strictParsing

0e77f67

correct assert statement

6c9c469

openjdk bot added the integrated Pull request has been integrated label Apr 16, 2024

openjdk bot closed this Apr 16, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Apr 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8327640: Allow NumberFormat strict parsing #18339

8327640: Allow NumberFormat strict parsing #18339

justin-curtis-lu commented Mar 18, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Mar 18, 2024

openjdk bot commented Mar 18, 2024 •

edited

Loading

openjdk bot commented Mar 18, 2024

mlbridge bot commented Mar 18, 2024 •

edited

Loading

turbanoff Mar 19, 2024

turbanoff Mar 19, 2024

turbanoff Mar 19, 2024

turbanoff Mar 19, 2024

justin-curtis-lu Mar 21, 2024

naotoj left a comment

naotoj Apr 11, 2024

justin-curtis-lu Apr 11, 2024

naotoj Apr 11, 2024

naotoj Apr 11, 2024

naotoj Apr 11, 2024

naotoj Apr 11, 2024

naotoj Apr 11, 2024

justin-curtis-lu commented Apr 11, 2024

naotoj left a comment

justin-curtis-lu commented Apr 16, 2024

openjdk bot commented Apr 16, 2024

openjdk bot commented Apr 16, 2024

	text.regionMatches(position, positiveSuffix,0, positiveSuffix.length());
	text.regionMatches(position, positiveSuffix, 0, positiveSuffix.length());

	text.regionMatches(position, negativeSuffix,0, negativeSuffix.length());
	text.regionMatches(position, negativeSuffix, 0, negativeSuffix.length());

	containsNegSuffix && text.length() == position + negativeSuffix.length();
	containsNegSuffix && text.length() == position + negativeSuffix.length();

	if (!isExponent && text.regionMatches(position, symbols.getInfinity(),0,
	if (!isExponent && text.regionMatches(position, symbols.getInfinity(), 0,

		import sun.util.locale.provider.LocaleProviderAdapter;
		import sun.util.locale.provider.ResourceBundleBasedAdapter;


		// Calculates the index that violated the grouping size
		// Calculation is determined whether it was an under or over violation

		import sun.util.locale.provider.LocaleProviderAdapter;
		import sun.util.locale.provider.LocaleServiceProviderPool;

8327640: Allow NumberFormat strict parsing #18339

8327640: Allow NumberFormat strict parsing #18339

Conversation

justin-curtis-lu commented Mar 18, 2024 • edited by openjdk bot Loading

Progress

Issues

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Mar 18, 2024

openjdk bot commented Mar 18, 2024 • edited Loading

openjdk bot commented Mar 18, 2024

mlbridge bot commented Mar 18, 2024 • edited Loading

Webrevs

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

naotoj 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

justin-curtis-lu commented Apr 11, 2024

naotoj left a comment

Choose a reason for hiding this comment

justin-curtis-lu commented Apr 16, 2024

openjdk bot commented Apr 16, 2024

openjdk bot commented Apr 16, 2024

justin-curtis-lu commented Mar 18, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Mar 18, 2024 •

edited

Loading

mlbridge bot commented Mar 18, 2024 •

edited

Loading