Fix the iOS String.format crash and the toString it relied on (issue #5482) - #5510
Open
shai-almog wants to merge 3 commits into
Open
Fix the iOS String.format crash and the toString it relied on (issue #5482)#5510shai-almog wants to merge 3 commits into
shai-almog wants to merge 3 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>
Contributor
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".
Contributor
✅ ByteCodeTranslator Quality ReportTest & Coverage
Benchmark Results
Static Analysis
Generated automatically by the PR CI workflow. |
Contributor
✅ Continuous Quality ReportTest & Coverage
Static Analysis
Generated automatically by the PR CI workflow. |
Contributor
Cloudflare Preview
|
Collaborator
Author
|
Compared 146 screenshots: 146 matched. Benchmark ResultsDetailed Performance Metrics
|
Collaborator
Author
|
Compared 146 screenshots: 146 matched. Benchmark ResultsDetailed Performance Metrics
|
Collaborator
Author
|
Compared 147 screenshots: 147 matched. |
Collaborator
Author
|
Compared 147 screenshots: 147 matched. |
Collaborator
Author
|
Compared 181 screenshots: 181 matched. |
Collaborator
Author
|
Compared 146 screenshots: 146 matched. Benchmark ResultsDetailed Performance Metrics
|
Collaborator
Author
|
Compared 144 screenshots: 144 matched. |
Collaborator
Author
|
Compared 217 screenshots: 217 matched. |
Collaborator
Author
|
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>
Collaborator
Author
|
Compared 149 screenshots: 149 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
Contributor
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.
Collaborator
Author
|
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>
Contributor
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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