Fix the iOS String.format crash and the toString it relied on (issue #5482) - #5510
Fix the iOS String.format crash and the toString it relied on (issue #5482)#5510shai-almog wants to merge 12 commits into
Conversation
…5482) String.format was a native method whose Objective-C branch formatted a string, threw it away, and returned fromNSString([NSString init]) -- sending init to the NSString class object, which aborts the process with "+[NSString<0x...> init]: cannot init a class object". Every String.format call on iOS killed the app. The C fallback that ran everywhere else did not crash but ignored width and precision, so "%.3f" printed every digit of the double. The Apple branch sits behind #if defined(__APPLE__) && defined(__OBJC__) and parparvm-tests runs on ubuntu-latest, so CI only ever compiled the #else branch and nothing flagged either problem. Formatting is string manipulation, so drop the native entirely and implement it once in java.lang.StringFormatter. One implementation now serves iOS, the JavaScript target and the C fallback, which also makes the Linux CI test meaningful for iOS. Supported conversions are s S b B h H c C d o x X e E f g G n %, with the - + ' ' 0 , ( # flags, width, precision and the %n$ / %< argument selectors. %a (hexadecimal float) and %t (date and time) are not implemented and raise UnknownFormatConversionException rather than producing something wrong. A malformed format string now raises the java.util exception the JVM raises (the ten missing classes are added here) instead of taking the process down, which is what the reporter asked for. Two further defects surfaced once the output could be compared with a JVM: - Double.toString and Float.toString were badly non-conforming. They asked snprintf for a fixed "%f" (six decimals) in the plain range and "%1.20E" (twenty-one significant digits) in the scientific range, so 1.0/3.0 rendered as "0.333333" instead of "0.3333333333333333" and 1e30 rendered as "1.00000000000000001988E30" instead of "1.0E30". That affected every concatenation of a double, not just formatting. Replaced with a search for the shortest rendering that round trips, which is what the specification asks for. - Math.abs(-0.0) returned -0.0, because "a < 0" is false for negative zero. Now fabs/fabsf. Coverage: StringFormatIntegrationTest and GetClassIntegrationTest run the same program on a real JVM and under ParparVM and diff it case by case, so the expectations are the JDK's rather than hand written. On macOS the harness compiles nativeMethods.m as Objective-C, so these exercise the branch that was crashing. Off-line, the implementation was diffed against the JDK over 384k value cases, 300k randomly assembled format strings and 399k random double and float bit patterns with no divergence. GetClassIntegrationTest also covers the reporter's other suspicion, that getClass() was returning null. It does not: getClassImpl cannot return null for a non-null receiver, and Class identity, hashing, string conversion and use as a HashMap key all match the JVM under allocation churn. The test pins that. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Not ready to approve
The new formatter has a confirmed edge-case bug for explicit argument index 0$ handling and there is a public JavaAPI compatibility issue in IllegalFormatException constructor visibility.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR fixes the iOS hard-crash in String.format() (issue #5482) by removing the native implementation and replacing it with a shared Java formatter (java.lang.StringFormatter) that is exercised by new differential integration tests. It also corrects ParparVM’s Double.toString/Float.toString conformance and fixes Math.abs(-0.0) to match JVM behavior.
Changes:
- Replace native
String.format()with a Java implementation (StringFormatter) used across targets (iOS/JS/C fallback). - Fix ParparVM floating-to-string rendering to produce the shortest round-tripping decimal and correct
Math.abs()for negative zero. - Add differential integration tests (JVM vs ParparVM) for
String.format()output andgetClass()/Classbehavior under allocation churn.
File summaries
| File | Description |
|---|---|
| vm/tests/src/test/resources/com/codename1/tools/translator/StringFormatApp.java | Test program emitting per-case String.format() results for line-by-line diffing. |
| vm/tests/src/test/resources/com/codename1/tools/translator/GetClassApp.java | Test program pinning getClass() and Class identity/hash/toString under churn. |
| vm/tests/src/test/java/com/codename1/tools/translator/StringFormatIntegrationTest.java | Differential JVM vs ParparVM integration test for formatting output and unsupported conversions. |
| vm/tests/src/test/java/com/codename1/tools/translator/GetClassIntegrationTest.java | Differential JVM vs ParparVM integration test for getClass() and Class map-key behavior. |
| vm/JavaAPI/src/java/util/UnknownFormatConversionException.java | Adds missing java.util exception used by formatter error paths. |
| vm/JavaAPI/src/java/util/MissingFormatWidthException.java | Adds missing java.util exception used by formatter width validation. |
| vm/JavaAPI/src/java/util/MissingFormatArgumentException.java | Adds missing java.util exception used for missing args/indexes. |
| vm/JavaAPI/src/java/util/IllegalFormatWidthException.java | Adds missing java.util exception used for illegal width handling. |
| vm/JavaAPI/src/java/util/IllegalFormatPrecisionException.java | Adds missing java.util exception used for illegal precision handling. |
| vm/JavaAPI/src/java/util/IllegalFormatFlagsException.java | Adds missing java.util exception used for illegal flag combinations. |
| vm/JavaAPI/src/java/util/IllegalFormatException.java | Adds missing java.util base exception type for formatter-related unchecked errors. |
| vm/JavaAPI/src/java/util/IllegalFormatConversionException.java | Adds missing java.util exception used for wrong argument type per conversion. |
| vm/JavaAPI/src/java/util/IllegalFormatCodePointException.java | Adds missing java.util exception for invalid code points in %c/%C. |
| vm/JavaAPI/src/java/util/FormatFlagsConversionMismatchException.java | Adds missing java.util exception for flag/conversion mismatches. |
| vm/JavaAPI/src/java/util/DuplicateFormatFlagsException.java | Adds missing java.util exception for duplicated flags. |
| vm/JavaAPI/src/java/lang/StringFormatter.java | New shared Java implementation of String.format() logic (parsing, conversions, rounding). |
| vm/JavaAPI/src/java/lang/String.java | Switches String.format() from native to StringFormatter.format(). |
| vm/ByteCodeTranslator/src/nativeMethods.m | Fixes Double.toString/Float.toString conformance and Math.abs() negative-zero behavior; removes native String.format. |
| vm/ByteCodeTranslator/src/javascript/parparvm_runtime.js | Removes JavaScript native String.format() binding (now handled in Java). |
| vm/ByteCodeTranslator/src/com/codename1/tools/translator/JavascriptNativeRegistry.java | Removes String.format from JS native registry list. |
Review details
- Files reviewed: 20/20 changed files
- Comments generated: 2
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d24938b462
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
✅ ByteCodeTranslator Quality ReportTest & Coverage
Benchmark Results
Static Analysis
Generated automatically by the PR CI workflow. |
✅ Continuous Quality ReportTest & Coverage
Static Analysis
Generated automatically by the PR CI workflow. |
Cloudflare Preview
|
|
Compared 146 screenshots: 146 matched. Benchmark ResultsDetailed Performance Metrics
|
|
Compared 146 screenshots: 146 matched. Benchmark ResultsDetailed Performance Metrics
|
|
Compared 147 screenshots: 147 matched. |
|
Compared 147 screenshots: 147 matched. |
|
Compared 181 screenshots: 181 matched. |
|
Compared 146 screenshots: 146 matched. Benchmark ResultsDetailed Performance Metrics
|
|
Compared 144 screenshots: 144 matched. |
|
Compared 217 screenshots: 217 matched. |
|
Compared 143 screenshots: 143 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
…headers Three review findings: - "%0$s" recorded argIndex == 0, which the parser then treated as "no explicit index given" and satisfied from the next sequential argument. Argument indexes are 1-based, so this now raises IllegalFormatArgumentIndexException the way JDK 16 and later do. JDK 11 still accepts index zero, so the case is asserted against ParparVM alone rather than through the shared diff. - A '.' with no digits after it defaulted the precision to zero, so "%.s" quietly produced an empty string. Every supported JDK rejects it with UnknownFormatConversionException naming '.' as the conversion; so do we now. - The four new test files were missing the Codename One GPLv2 + Classpath Exception header, which failed check-copyright-headers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
iOS Metal screenshot updatesCompared 149 screenshots: 148 matched, 1 updated.
Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
There was a problem hiding this comment.
🟡 Not ready to approve
There are correctness/test-stability issues in the new formatter parsing and integration test setup that should be addressed to avoid divergent behavior and flaky results.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (2)
vm/JavaAPI/src/java/lang/StringFormatter.java:154
- The parser currently allows combining an explicit argument index (e.g. "%2$") with the previous-argument flag ('<'). That combination is not meaningful and the current logic will silently prioritize
previousand ignore the explicit index (or throwMissingFormatArgumentExceptionif it’s the first specifier), diverging fromjava.util.Formatter’s behavior for invalid format strings. Consider rejecting '<' when an explicit argument index was already parsed for this specifier.
} else if (f == '<') {
previous = true;
pos++;
continue;
vm/tests/src/test/resources/com/codename1/tools/translator/StringFormatApp.java:244
- This integration test derives its expected output from a JVM run of
String.format(), but several cases depend on the JVM default locale (e.g.%,d,%,.2f, and%Suppercasing). Without pinning the default locale, the diff can become environment-dependent and fail on machines with non-"en_US" defaults even if ParparVM is correct. Consider setting the default locale explicitly at the start ofmain().
public static void main(String[] args) {
strings();
integers();
floats();
failures();
- Files reviewed: 21/21 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
|
Compared 148 screenshots: 148 matched. Benchmark Results
Detailed Performance Metrics
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2e37e4b8ef
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Two more review findings, both confirmed against the JDK first: - format(fmt, (Object[]) null) is not an empty argument list. The JVM skips the bounds checks and hands every specifier a null, so "%s %s" renders "null null" where this threw MissingFormatArgumentException. "%<" is the one exception: it reuses the previous argument, so it still requires that one existed, which is what the parser fuzz caught after the first attempt made null unconditional. - '<' was consumed outside the duplicate-flag check, so "%s %<<s" quietly rendered "a a" instead of raising DuplicateFormatFlagsException. It is now a flag bit like every other, which also makes it participate in the %% and %n flag validation without the special case that was there before. Both behaviours are identical on JDK 11, 17 and 25, so the new cases go through the shared JVM-vs-ParparVM diff rather than being asserted one-sided. The parser sweep now also generates null argument arrays and doubled flags; divergence is still zero apart from the documented %a and %t gap. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Not ready to approve
The new differential test currently depends on the host default Locale and the public String.format() javadoc doesn’t document the locale-independent behavior, both of which can cause avoidable instability/confusion.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (2)
vm/tests/src/test/java/com/codename1/tools/translator/StringFormatIntegrationTest.java:202
- The JVM side of this differential test depends on the host default Locale for grouping/decimal separators. Since the ParparVM implementation is documented as locale-independent (always ',' grouping and '.' decimal), this test can become flaky or fail on machines with a non-English default locale. Consider forcing a known locale for the JVM run via system properties so the expected output matches the intended locale-independent behavior consistently.
ProcessBuilder pb = new ProcessBuilder(
javaExe,
"-cp",
classesDir + System.getProperty("path.separator") + javaApiDir,
"StringFormatApp"
vm/JavaAPI/src/java/lang/String.java:1085
- The public String.format() javadoc here doesn’t mention that this implementation is locale-independent (fixed '.' decimal separator and ',' grouping, and %n emits '\n'), which is a behavioral difference from the JDK that callers may rely on. Since this is a public API entry point, consider documenting the locale behavior (and that unsupported conversions like %a/%t throw UnknownFormatConversionException) here rather than only in the internal StringFormatter class.
/**
* Returns a formatted string using the specified format string and arguments.
* Supports the {@code s b h c d o x e f g n %} conversions (and their uppercase
* variants) with the {@code - + ' ' 0 , ( #} flags, width, precision, and the
* {@code %n$} / {@code %<} argument selectors.
- Files reviewed: 21/21 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
The vm/tests differential covers ParparVM's C and Objective-C target, but that is not the path a shipping app takes: the iOS build translates the core out of the bundled iOSPort.jar rather than out of the reactor, JavaSE and Android run their own java.util.Formatter, and the JavaScript port runs the translated Java through its own runtime. Those are four different code paths reaching the same API, and nothing was asserting that they agree. StringFormatTest runs the same 124 expectations on every port the suite covers: iOS GL and Metal, tvOS, watchOS, Android, JavaScript, mac native, Linux, Windows and the JavaSE simulator. It takes no screenshot; it is a pure assertion test in the shape of the existing FloatingToStringTest. Every expected value was produced by a real Java SE java.util.Formatter rather than written by hand. The generator is checked in as scripts/hellocodenameone/tools/generate-string-format-cases.java so the table can be regenerated and audited, and it emits a byte-identical table on JDK 8, 11, 17, 21 and 25 -- so this pins behaviour that does not drift with the JDK the suite happens to build against. Coverage includes the conversions, flags, widths and precisions an app actually uses, the HALF_UP rounding cases where Java disagrees with C printf, and the four malformed-format cases found in review on this branch (empty precision, repeated flags, grouping on hex, zero padding on a string). Three things are deliberately excluded because they are genuinely not consistent across these runtimes, rather than papered over: - %a and %t, which ParparVM does not implement. - %0$s, accepted before JDK 16 and rejected from 16 on. - a pinned value for %n, since the JDK emits the platform line separator and that is "\r\n" on a Windows JVM. The test asserts it is a line separator instead. JavaSE and Android format through the default locale, so the test probes the platform's decimal and grouping separators rather than assuming them, and fails with a specific message if the platform also localises the digits. Registered in Cn1ssDeviceRunner and the java-standard-apis feature group. The stored per-port reports get a not-run entry, which the next master publish replaces with the real result; the pinned test count moves 170 -> 171. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2087fbc906
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…p exhaustively
The reported case is real and JDK 11, 17 and 25 agree on it: for a character
conversion the JVM reports an unsupported flag ahead of the missing width, which
is the opposite order from every other conversion.
"%-0c" -> FormatFlagsConversionMismatchException (not MissingFormatWidth)
"%-0s" -> MissingFormatWidthException
"%-0d" -> MissingFormatWidthException
"%-#b" -> FormatFlagsConversionMismatchException
Six variants were wrong; the character branch now checks flags first.
Finding it this way was the fourth validation-ordering defect review has caught
that the curated tables could not, so this adds the net that finds them
automatically instead. StringFormatConformanceTest copies the formatter and its
exceptions into a neutral package -- nothing can load a java.lang class
otherwise -- compiles them, and drives about a million single-specifier format
strings through both it and the JDK, comparing the exact rendering or the exact
exception. Single-specifier is deliberate: the JVM validates a whole format
string before formatting any of it, so a format with several broken specifiers
can legitimately report a different one first, and restricting to one specifier
removes that variable.
It immediately found a fifth defect nobody had reported: the JVM rejects a wrong
argument type for %x and %o ahead of the sign-flag check it defers to print time,
so "%+x" with a String raised FormatFlagsConversionMismatchException here where
the JVM raises IllegalFormatConversionException. 1470 cases. Fixed by accepting
the argument's type before applying the deferred flags.
The sweep is now zero mismatches on JDK 8, 11, 17, 21 and 25. Java 8 ignores a
width on the literal "%" conversion where Java 9 honours it, so the test detects
that at runtime rather than pinning a version, and asserts a modern JDK skips
nothing.
Confirmed the test fails when the code is wrong by restoring the character bug:
it reports "%-0c" by name.
Regression cases for both orderings added to the device and differential tables.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Human review recommended
It changes core runtime behavior (formatting + floating-point string conversion) across multiple ParparVM backends and should receive final human review despite strong test coverage.
Review details
Suppressed comments (2)
scripts/hellocodenameone/tools/GenerateStringFormatCases.java:325
- The generator still prints the old (pre-rename) file path in its header comment, which is inconsistent with the current filename and with StringFormatTest’s javadoc reference. This makes it harder to follow the “regenerate and paste” workflow.
System.out.println("// Generated by scripts/hellocodenameone/tools/generate-string-format-cases.java");
scripts/hellocodenameone/common/src/main/java/com/codenameone/examples/hellocodenameone/tests/StringFormatTest.java:69
- This embedded generated header comment still references the old generator path (generate-string-format-cases.java). Since the generator file is now GenerateStringFormatCases.java, update this to match so readers can easily find the tool that produced this table.
// Generated by scripts/hellocodenameone/tools/generate-string-format-cases.java
// on Azul Systems, Inc. JDK 25 -- do not edit by hand.
- Files reviewed: 38/38 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
The stored reports carried it as not-run, which was wrong on its own terms: the point of adding a test is that it runs, and marking a brand new test as not-run invents a state it was never in. It also mis-reported the java-standard-apis feature on the website as incomplete. This PR's CI ran it on all eleven ports and it passed on every one, so that is what the reports say now. Verified per port from the port-status artifacts: android, ios-gl, ios-metal, javascript, linux-arm64, linux-x64, mac-native, tvos, watchos, windows-arm64, windows-x64. The only delta against master in each report is now +1 pass and the new entry; every other counter, including not-run, is back to its original value. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b2958f7a4b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
🟡 Human review recommended
It makes broad, correctness-sensitive changes to core runtime formatting and floating-point string conversion across multiple backends, which warrants final human review.
Review details
- Files reviewed: 38/38 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Review is right and this was my error. Each stored report is a snapshot of one
specific run, pinned by its commit and run_url fields, not a running description
of the port. Every one of them records a July 15 or 16 commit:
android b51436a 2026-07-16
ios-gl 205dcc6 2026-07-16
tvos 205dcc6 2026-07-16
watchos dec3d17 2026-07-15
windows-x64 d17e18e 2026-07-16
...
None of those commits contains or registers StringFormatTest, so none of those
runs could have executed it. Writing "pass" into them attributed a result to a
run that provably never produced one, which is fabricating test results no matter
that the test does pass elsewhere.
Back to not-run, which is what those runs actually did with a test that did not
exist yet. The real results land when master publishes reports from a run whose
commit contains the test; port-status-publish.yml does that on master push, and
the entry is already wired up to receive them.
The evidence that the test passes on all eleven ports belongs in the PR
discussion, not hand-written into a machine-generated snapshot.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Not ready to approve
A few verified issues remain (test determinism around locale, stale generator path references, and an API-visible arbitrary numeric cap in the format parser) that should be addressed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (4)
scripts/hellocodenameone/tools/GenerateStringFormatCases.java:325
- The generator prints a stale source path in its output header ("generate-string-format-cases.java"), but the file in this PR is named "GenerateStringFormatCases.java". This makes the generated block misleading and also propagates into StringFormatTest’s embedded header comment.
System.out.println("// Generated by scripts/hellocodenameone/tools/generate-string-format-cases.java");
scripts/hellocodenameone/common/src/main/java/com/codenameone/examples/hellocodenameone/tests/StringFormatTest.java:69
- This embedded header comment references the old generator path ("generate-string-format-cases.java"), but the generator in this PR is "GenerateStringFormatCases.java". Keeping these in sync helps future updates/regeneration.
// Generated by scripts/hellocodenameone/tools/generate-string-format-cases.java
// on Azul Systems, Inc. JDK 25 -- do not edit by hand.
vm/tests/src/test/resources/com/codename1/tools/translator/StringFormatApp.java:281
- StringFormatIntegrationTest diffs ParparVM output against a JVM run, but StringFormatApp doesn’t force a stable default Locale. On machines with a non-English default locale, Java’s String.format may use different decimal/grouping separators or locale-sensitive uppercasing, causing spurious diffs unrelated to ParparVM behavior.
vm/JavaAPI/src/java/lang/StringFormatter.java:783 - parseNumber() rejects any width/precision/index above 1,000,000 with a generic IllegalArgumentException. This introduces a non-JDK limit and exception behavior for valid format strings (large widths are legal in java.util.Formatter, even if they may OOM at runtime). If you want to guard the parse, it’s safer to only reject integer overflow (matching Integer.parseInt-style behavior) rather than an arbitrary cap.
int result = 0;
for (int i = start; i < end; i++) {
result = result * 10 + (value.charAt(i) - '0');
if (result > 1000000) {
// Guard against a width so large it would only be a denial of service.
throw new IllegalArgumentException(value.substring(start, end));
}
- Files reviewed: 38/38 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
…enerator path
Four points from the latest Copilot review, all of which hold.
parseNumber rejected any width, precision or argument index above 1,000,000 with
a bare IllegalArgumentException. That was my invention, and it rejected legal
format strings: "%1000001d" really does produce a million characters on the JVM.
It now mirrors what the JVM does instead, which the JVM was asked directly:
"%1000001d" -> a 1000001 character string
"%12.1000001f" -> a 1000003 character string
"%2147483648d" -> IllegalFormatWidthException
"%.99999999999f" -> IllegalFormatPrecisionException
"%2147483648$d" -> IllegalFormatArgumentIndexException
"%1000001$d" -> MissingFormatArgumentException
A digit run that does not fit in an int saturates, and each field turns that into
the exception it calls for. All ten probes match. The conformance sweep now
covers oversized fields explicitly, since its widths only went up to 12 and it
could never have found this.
StringFormatIntegrationTest diffed ParparVM against a JVM run without pinning a
locale. This formatter is locale independent while the JVM side follows the
default, so a build machine in a comma-decimal locale would have produced diffs
that had nothing to do with ParparVM. The JVM side now runs with an explicit
en_US.
The generator printed its own former file name into the header of everything it
generated, which had propagated into StringFormatTest. Fixed and regenerated.
Full ParparVM suite 412 green, device test clean, contract and headers green.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Addressed the four suppressed comments from the latest Copilot review in a1208ab. All four were valid. 1. A digit run that does not fit in an int now saturates and each field turns that into the exception it calls for. All ten probes match the JVM. Worth noting why the million-case sweep missed this: its widths only go up to 12, so it could never reach the cap. The sweep now covers oversized fields explicitly. 2. 3 and 4. Stale generator path. The generator printed its own former file name into the header of everything it generated, which had propagated into Verification after the change: full ParparVM suite 412 green, the ~1M-case conformance sweep still zero on JDK 8/11/17/21/25, device test clean on the JVM, port-status contract and copyright headers green. On the one CI failure showing on the previous commit -- |
There was a problem hiding this comment.
🟡 Not ready to approve
The new formatter currently rejects the standard %F conversion and the conformance sweep omits %F, leaving a spec-compatibility gap untested.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (2)
vm/JavaAPI/src/java/lang/StringFormatter.java:250
- StringFormatter currently rejects the standard Java Formatter conversion '%F' (uppercase floating-point). In java.util.Formatter, both %f and %F are valid; %F mainly affects the casing of NaN/Infinity. Because validateSpecifier only whitelists uppercase conversions in "SBHCXEG", any %F format string will incorrectly throw UnknownFormatConversionException.
private static char validateSpecifier(char conversion, boolean upper, int flags,
int width, int precision) {
if (upper && "SBHCXEG".indexOf(conversion) < 0) {
// 'D' and 'O' have no uppercase form in java.util.Formatter.
throw new UnknownFormatConversionException(String.valueOf(conversion));
}
vm/tests/src/test/java/com/codename1/tools/translator/StringFormatConformanceTest.java:79
- The exhaustive conformance sweep doesn't include the standard '%F' conversion, so it won't catch regressions/bugs in uppercase floating-point formatting (or the current rejection of %F). Since this test is meant to cover all supported conversions against the JDK, add "F" to the CONVERSIONS list.
- Files reviewed: 38/38 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a1208ab9a6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // rather than silently wrong. See the class javadoc. | ||
| throw new UnknownFormatConversionException(String.valueOf(conversion)); | ||
| } | ||
| checkFlags(conversion, lower, flags, width, precision); |
There was a problem hiding this comment.
When a supported uppercase conversion is rejected, this passes the original uppercase character into the exception path: for example, %G with an Integer produces an IllegalFormatConversionException whose getConversion() is G, and %+S produces a FormatFlagsConversionMismatchException containing S. The JDK normalizes these values to g and s, respectively, so callers inspecting the newly added getters or messages observe incompatible results; retain the uppercase flag for rendering but use lower when constructing conversion-related exceptions.
Useful? React with 👍 / 👎.
…eep first
The oversized-field assertions I added in the previous commit failed vm-tests,
and that failure was mine. A width too large for an int is silently dropped up
to at least Java 11 and rejected from Java 17 on:
"%2147483648d" -> "1" on JDK 8 and 11
"%2147483648d" -> IllegalFormatWidthException on JDK 17, 21 and 25
The sweep compares against whichever JDK runs it, CI runs it on 8, and this
formatter follows the modern behaviour, so the comparison could not hold there.
Those specs are now gated on a runtime probe, the way the "%5%" width difference
already was. The large-but-representable cases -- "%1000001d" and friends, which
are the ones the review comment was actually about -- are still asserted
unconditionally, because every JDK agrees on them.
The probe needed a guard of its own: on a modern JDK it throws rather than
returning a value, which the cross-JDK check caught.
Also moved the oversized block after the main sweep assertion. It ran before it,
so a failure there masked the result of the million-case sweep entirely -- the CI
run that failed never reported whether the sweep itself was clean.
Verified per JDK rather than assumed: sweep and oversized checks are both clean
on 8, 11, 17, 21 and 25, and the test passes under Maven on JDK 8, which is the
configuration that failed. Full ParparVM suite 412 green.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Human review recommended
It replaces core formatting behavior across multiple runtimes and adjusts native numeric rendering, which is high-impact and warrants final human validation despite the strong test additions.
Review details
- Files reviewed: 38/38 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 15faf83a9f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (args != null && nextArg >= args.length) { | ||
| throw new MissingFormatArgumentException(format.substring(specStart, pos)); |
There was a problem hiding this comment.
The earlier single-specifier validation fix does not cover malformed specifiers later in the format: with no arguments, String.format("%s %q") on the JDK throws UnknownFormatConversionException because the whole format is parsed before argument selection, while this bounds check immediately throws MissingFormatArgumentException for the first %s. The same masking occurs for later duplicate flags, missing widths, and invalid precisions, so parse and validate all specifiers before beginning argument lookup and conversion.
Useful? React with 👍 / 👎.
| private static void failMissingWidth(char conversion, int flags, int width, int needsWidth) { | ||
| if (width < 0 && (flags & needsWidth) != 0) { | ||
| throw new MissingFormatWidthException("%" + flagString(flags) + conversion); | ||
| } |
There was a problem hiding this comment.
When a width-dependent flag is combined with an explicit argument index, this reconstruction discards the index from the new exception's observable format specifier. For example, %1$-s produces MissingFormatWidthException.getFormatSpecifier() equal to %-s, whereas the JDK reports %-1$s; %2$0d similarly becomes %0d instead of %02$d. Pass the parsed argument index into this path and construct the canonical indexed specifier so callers inspecting the getter or message receive compatible metadata.
Useful? React with 👍 / 👎.
…ng the test My hand edits to these files are gone. They are machine generated snapshots of a single run, pinned by commit and run_url, and I had no business writing statuses into them by hand -- first not-run, which inflated every port's count from 5 to 6, then pass, which attributed a result to July runs that could not have produced it. These are the reports this PR's own CI generated, at commit 12ecf37, which contains and registers StringFormatTest. Nothing here is written by hand. java-standard-apis now executes everywhere: FloatingToString, Java17, NanoTimeApi, StreamApi, StringApi, StringFormat and TimeApi all report a real result on all eleven ports rather than not-run. not-run drops from 5 to 0 on the device ports, and to 3 on linux and windows where three tests genuinely do not run. The reports also carry seven failures on linux and windows -- AudioMixerApi, Crypto, FileSystemStorageOpenInputStreamMissing, SurfacesPublish, TimeApi and friends. Those are not from this branch. Master's own run today at 9783b6c records the identical seven on the same ports, and the stored snapshots simply predate them by three weeks. They arrive here because the reports are being refreshed, not because anything in this PR caused them. Contract validate and the contract unit tests are green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Human review recommended
It changes core formatting and floating-point/string rendering behavior across multiple runtimes (JavaAPI, ParparVM native, and JS), which is high-impact and warrants final human review despite the added conformance tests.
Review details
- Files reviewed: 38/38 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1d675aa4f1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if ((flags & FLAG_HASH) != 0) { | ||
| throw new FormatFlagsConversionMismatchException("#", conversion); | ||
| } | ||
| return text(arg == null ? "null" : arg.toString(), upper, flags, width, precision); |
There was a problem hiding this comment.
When an argument's toString() implementation returns null, the unadorned %s conversion passes that null into text(), which immediately dereferences it and throws NullPointerException. The JDK instead renders String.format("%s", object) as "null" in this scenario, so existing objects with an unusual but permitted toString() result regress from printable output to a runtime failure; normalize the result to the literal "null" before calling text().
Useful? React with 👍 / 👎.
Fixes #5482.
The crash
String.formatwas a native method. Its Objective-C branch formatted a string, threw it away, and returnedfromNSString([NSString init])-- sendinginitto theNSStringclass object:That aborts the process with
+[NSString<0x...> init]: cannot init a class object, which is exactly the termination the reporter pasted. EveryString.formatcall on iOS killed the app.The C fallback used everywhere else did not crash, but ignored width and precision entirely, so
"%.3f"printed every digit of the double and"%.2e"printed%.followed by the whole number and a literal2e.Neither was caught because the Apple branch is behind
#if defined(__APPLE__) && defined(__OBJC__)andparparvm-testsruns onubuntu-latest, so CI only ever compiled the#elsebranch.The fix
Formatting is string manipulation, so the native is gone and the work happens once in
java.lang.StringFormatter. A single implementation now serves iOS, the JavaScript target and the C fallback -- which also means the Linux CI test is finally meaningful for iOS.Supported:
s S b B h H c C d o x X e E f g G n %, the- + ' ' 0 , ( #flags, width, precision, and the%n$/%<argument selectors. Rendering is locale independent.%a(hexadecimal float) and%t(date and time) are not implemented; they raiseUnknownFormatConversionExceptionrather than producing something wrong. Both are documented in the class javadoc and pinned by the test.A malformed format string now raises the
java.utilexception the JVM raises -- the ten missing exception classes are added here -- instead of taking the process down. That was the reporter's closing request: "it should have been a trapped error, presented as some kind of a runtime fault, rather than a hard crash."Two further defects the new test exposed
Once the output could be diffed against a JVM, two unrelated ParparVM bugs failed the test:
Double.toString/Float.toStringwere badly non-conforming. They asked snprintf for a fixed"%f"(six decimals) in the plain range and"%1.20E"(twenty-one significant digits) in the scientific range.1.0/3.0rendered as"0.333333"instead of"0.3333333333333333";1e30rendered as"1.00000000000000001988E30"instead of"1.0E30". This hit every concatenation of a double on iOS, not just formatting. Replaced with a binary search for the shortest rendering that round trips, which is what the specification asks for.Math.abs(-0.0)returned-0.0, becausea < 0is false for negative zero. Nowfabs/fabsf.The reporter's other suspicion
He also suspected
getClass()was returning null. It is not:getClassImplcannot return null for a non-null receiver -- it hands back&ClazzClazzwhen the class reference is absent.GetClassIntegrationTestreproduces his exact shape (interface-typed reference,getClass()used as aHashMapkey, the"class is " + clconcatenation that printednull) and re-checks every invariant across 200k allocations of churn. Class identity, hashing, string conversion and map lookup all match the JVM. That symptom was downstream of the process already being wrecked.Coverage
StringFormatIntegrationTestandGetClassIntegrationTestrun the same program on a real JVM and under ParparVM and diff it case by case, so the expectations are the JDK's rather than hand written. On macOS the harness compilesnativeMethods.mas Objective-C, so these exercise the branch that was crashing.Off-line, the implementation was diffed against the JDK over:
%a/%tgap)toStringJDK 11 and 17 differ from the last row on ~5.6% of random doubles because they predate JDK-4511638; the values used in the committed test are byte-identical across JDK 11, 17 and 25.
Full ParparVM suite: 411 tests, 0 failures.
🤖 Generated with Claude Code