Fix custom class loader registry lookups - #195
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #195 +/- ##
=======================================
Coverage 90.77% 90.77%
=======================================
Files 269 269
Lines 33390 33392 +2
=======================================
+ Hits 30310 30312 +2
Misses 3080 3080 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![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: 759b48e336
ℹ️ 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.
Pull request overview
This PR fixes class-name format mismatches between JVM internal names (pkg/Class) and Java binary names (pkg.Class) when interacting with java.lang.ClassLoader, improving correctness for custom loaders and array/class registry lookups.
Changes:
- Convert internal JVM names to binary names before calling Java
ClassLoader.loadClass. - Convert binary names back to internal names for bootstrap loading, array definition, and
findLoadedClassregistry lookups. - Extend the
ClassMetadatafixture to cover repeated packaged-class and array resolution through a custom loader.
Reviewed changes
Copilot reviewed 6 out of 14 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test_data/src/loader/Payload.java | Adds a packaged fixture class used by the custom loader test. |
| test_data/src/loader/Base.java | Adds a packaged superclass fixture for packaged resolution testing. |
| test_data/src/ClassMetadata.java | Extends the custom loader fixture to load packaged classes and arrays repeatedly. |
| test_data/ClassMetadata.txt | Updates expected output for the extended fixture assertions. |
| jvm/src/runtime/java_lang_class_loader.rs | Ensures internal names are converted to binary names before invoking ClassLoader.loadClass. |
| java_runtime/src/classes/java/lang/class_loader.rs | Adjusts loadClass/findLoadedClass to use internal names for JVM lookups while accepting Java binary names. |
Suppressed comments (1)
java_runtime/src/classes/java/lang/class_loader.rs:214
- When delegating to
parent.loadClass(...)and callingthis.findClass(...), this forwards the originalnameargument. If internal runtime callers ever pass internal names containing '/', this will send internal names into JavaClassLoaderAPIs (which expect binary names with '.'), reproducing the name-mismatch problem for custom loaders. Normalizing to a binary-namejava_namebefore these calls avoids that.
let class: ClassInstanceRef<Class> = if !parent.is_null() {
jvm.invoke_virtual(&parent, "loadClass", "(Ljava/lang/String;)Ljava/lang/Class;", (name.clone(),))
.await?
} else {
jvm.load_bootstrap_class(&internal_name).await?.into()
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
ClassLoader.loadClassRoot cause
The JVM registry stores names such as
loader/Payload, while JavaClassLoaderAPIs receiveloader.Payload.findLoadedClassqueried the registry with the binary name, so packaged classes were not found and the loader attempted to define them again. The inverse mismatch also sent internal names to custom Java class loaders.Known issue
The JVM class registry is currently keyed only by internal class name. Defining the same binary name in multiple custom class loaders is not isolated yet and requires a loader-aware registry redesign.
Validation
cargo fmt --all -- --checkcargo test --workspacecargo clippy --workspace --all-targets -- -D warnings