optimize: JSMethod #108
Open
optimize: JSMethod #108
+45
−36
Conversation
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
Reviewer's Guide通过以下方式优化带有 JSMethod 注解的 Java 方法在 QuickJS 中的暴露方式：为 JSMethod 注解新增可命名的 value 值；更新方法注册逻辑以尊重该名称并处理没有无参构造函数的类；为带有 JSMethod 的方法添加 ProGuard/R8 keep 规则；现代化基于反射的实例化方式；以及清理 QuickJSObject 中 Map 转换逻辑及相关测试中的类型安全警告。 QuickJSObject.setProperty 中更新后的 JSMethod 注册时序图sequenceDiagram
participant Caller
participant QuickJSObject
participant JavaClass
participant JSObject
Caller->>QuickJSObject: setProperty(name, clazz)
activate QuickJSObject
QuickJSObject->>JavaClass: clazz.getConstructor().newInstance()
alt constructor exists and is accessible
JavaClass-->>QuickJSObject: javaObj instance
else constructor missing or not accessible
JavaClass-->>QuickJSObject: throws exception
QuickJSObject-->>QuickJSObject: javaObj remains null
end
QuickJSObject->>QuickJSObject: methods = clazz.getMethods()
QuickJSObject->>JSObject: createNewJSObject()
JSObject-->>QuickJSObject: jsObj
loop for each method in methods
QuickJSObject->>QuickJSObject: check if javaObj is null and method is non static
alt javaObj is null and method is non static
QuickJSObject-->>QuickJSObject: skip method
else
QuickJSObject->>QuickJSObject: jsMethod = method.getAnnotation(JSMethod)
alt jsMethod is null
QuickJSObject-->>QuickJSObject: skip method
else
QuickJSObject->>QuickJSObject: jsMethodName = jsMethod.value()
alt jsMethodName is blank
QuickJSObject-->>QuickJSObject: jsMethodName = method.getName()
end
QuickJSObject->>JSObject: jsObj.setProperty(jsMethodName, lambda)
end
end
end
QuickJSObject->>QuickJSObject: setProperty(name, jsObj)
QuickJSObject-->>Caller: return
deactivate QuickJSObject
JSMethod 注解和 QuickJSObject 方法暴露的更新后类图classDiagram
class JSMethod {
<<annotation>>
+String value()
}
class QuickJSObject {
-QuickJSContext context
+void setProperty(String name, Class clazz)
+void convertToMap(Object target, Object map, HashMap circulars, MapFilter filter, Object extra, MapCreator mapCreator)
}
class JSObject {
+long getPointer()
+void setProperty(String name, Object function)
}
class TestJava {
+boolean test(String message)
+boolean test(String message, int age)
+boolean test(JSArray array)
+boolean test(JSObject object)
+boolean test(JSObject object, JSArray message, int age, String name)
}
JSMethod <|.. TestJava : annotates
QuickJSObject --> JSObject : creates_and_populates
QuickJSObject ..> JSMethod : reads_annotation
QuickJSObject ..> TestJava : reflects_on_methods
QuickJSObject ..> MapFilter : uses
QuickJSObject ..> MapCreator : uses
class MapFilter {
}
class MapCreator {
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
Original review guide in EnglishReviewer's GuideOptimizes how JSMethod-annotated Java methods are exposed to QuickJS by introducing a named JSMethod annotation value, updating method registration to respect that name and handle classes without no-arg constructors, adding ProGuard/R8 keep rules for JSMethod methods, modernizing reflection-based instantiation, and cleaning up type-safety warnings in QuickJSObject map conversion logic and related tests. Sequence diagram for updated JSMethod registration in QuickJSObject.setPropertysequenceDiagram
participant Caller
participant QuickJSObject
participant JavaClass
participant JSObject
Caller->>QuickJSObject: setProperty(name, clazz)
activate QuickJSObject
QuickJSObject->>JavaClass: clazz.getConstructor().newInstance()
alt constructor exists and is accessible
JavaClass-->>QuickJSObject: javaObj instance
else constructor missing or not accessible
JavaClass-->>QuickJSObject: throws exception
QuickJSObject-->>QuickJSObject: javaObj remains null
end
QuickJSObject->>QuickJSObject: methods = clazz.getMethods()
QuickJSObject->>JSObject: createNewJSObject()
JSObject-->>QuickJSObject: jsObj
loop for each method in methods
QuickJSObject->>QuickJSObject: check if javaObj is null and method is non static
alt javaObj is null and method is non static
QuickJSObject-->>QuickJSObject: skip method
else
QuickJSObject->>QuickJSObject: jsMethod = method.getAnnotation(JSMethod)
alt jsMethod is null
QuickJSObject-->>QuickJSObject: skip method
else
QuickJSObject->>QuickJSObject: jsMethodName = jsMethod.value()
alt jsMethodName is blank
QuickJSObject-->>QuickJSObject: jsMethodName = method.getName()
end
QuickJSObject->>JSObject: jsObj.setProperty(jsMethodName, lambda)
end
end
end
QuickJSObject->>QuickJSObject: setProperty(name, jsObj)
QuickJSObject-->>Caller: return
deactivate QuickJSObject
Updated class diagram for JSMethod annotation and QuickJSObject method exposureclassDiagram
class JSMethod {
<<annotation>>
+String value()
}
class QuickJSObject {
-QuickJSContext context
+void setProperty(String name, Class clazz)
+void convertToMap(Object target, Object map, HashMap circulars, MapFilter filter, Object extra, MapCreator mapCreator)
}
class JSObject {
+long getPointer()
+void setProperty(String name, Object function)
}
class TestJava {
+boolean test(String message)
+boolean test(String message, int age)
+boolean test(JSArray array)
+boolean test(JSObject object)
+boolean test(JSObject object, JSArray message, int age, String name)
}
JSMethod <|.. TestJava : annotates
QuickJSObject --> JSObject : creates_and_populates
QuickJSObject ..> JSMethod : reads_annotation
QuickJSObject ..> TestJava : reflects_on_methods
QuickJSObject ..> MapFilter : uses
QuickJSObject ..> MapCreator : uses
class MapFilter {
}
class MapCreator {
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - 我已经审核了你的改动，发现有一些需要处理的问题。
- 在 QuickJSObject.setProperty(String, Class<?> ) 中，对 jsMethod.value() 使用 String.isBlank() 可能会在较老的 Java/Android 目标上引发兼容性问题；建议改成使用兼容性更好的检查方式，例如 value == null || value.trim().isEmpty()。
- 新增的 consumer-rules.pro 中的 ProGuard 规则（-keepclassmembers,allowobfuscation class * { @com.whl.quickjs.wrapper.JSMethod ;）似乎缺少成员块的右大括号和分号；请再次核对并修正语法，以确保 keep 规则能够真正生效。
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In QuickJSObject.setProperty(String, Class<?>), using String.isBlank() on jsMethod.value() may cause compatibility issues on older Java/Android targets; consider replacing it with a more broadly supported check like value == null || value.trim().isEmpty().
- The new ProGuard rule in consumer-rules.pro (-keepclassmembers,allowobfuscation class * { @com.whl.quickjs.wrapper.JSMethod <methods>;) appears to be missing a closing brace and semicolon for the members block; please double-check and correct the syntax so the keep rule is actually applied.
## Individual Comments
### Comment 1
<location> `wrapper-java/src/main/java/com/whl/quickjs/wrapper/QuickJSObject.java:106-115` </location>
<code_context>
public void setProperty(String name, Class<?> clazz) {
Object javaObj = null;
try {
- javaObj = clazz.newInstance();
- } catch (IllegalAccessException e) {
+ javaObj = clazz.getConstructor().newInstance();
+ } catch (IllegalAccessException
+ | InstantiationException
+ | InvocationTargetException
+ | NoSuchMethodException e) {
e.printStackTrace();
- } catch (InstantiationException e) {
- e.printStackTrace();
- }
-
- if (javaObj == null) {
- throw new NullPointerException("The JavaObj cannot be null. An error occurred in newInstance!");
}
JSObject jsObj = context.createNewJSObject();
Method[] methods = clazz.getMethods();
+ Object finalJavaObj = javaObj;
</code_context>
<issue_to_address>
**issue (bug_risk):** Constructor failure leads to silent dropping of instance methods with only a stack trace as signal.
With the new use of `clazz.getConstructor().newInstance()`, a missing/inaccessible no-arg constructor (or a constructor that throws) leaves `javaObj` as `null`. The loop then only exposes static methods, and the failure is effectively hidden behind `e.printStackTrace()`. This makes misconfigurations (e.g., adding only a required-args constructor) hard to detect and can silently break JS bindings. Please surface this failure more explicitly: e.g., throw a clear runtime exception, log via your standard logger and fail fast, or support a caller-provided factory when direct instantiation isn’t possible.
</issue_to_address>
### Comment 2
<location> `wrapper-android/consumer-rules.pro:5-6` </location>
<code_context>
--keep class com.whl.quickjs.**{*;}
\ No newline at end of file
+-keep class com.whl.quickjs.**{*;}
+-keepclassmembers,allowobfuscation class * {
+ @com.whl.quickjs.wrapper.JSMethod <methods>;
+}
\ No newline at end of file
</code_context>
<issue_to_address>
**issue (bug_risk):** Allowing obfuscation of @JSMethod methods can break JS APIs that rely on default method names.
Because the binding uses the annotation value or falls back to method.getName() when the value is blank, obfuscating these methods means any JS API that relies on the default (blank) annotation will now be exposed under an obfuscated name in release builds. This is only safe if all JS-exposed methods use an explicit @JSMethod("...") value. If that isn’t guaranteed, consider removing allowobfuscation for these methods or splitting the rule so that only methods with blank annotation values remain unobfuscated.
</issue_to_address>帮我变得更有用！请对每条评论点 👍 或 👎，我会根据你的反馈改进后续的 Review。
Original comment in English
Hey there - I've reviewed your changes and found some issues that need to be addressed.
- In QuickJSObject.setProperty(String, Class<?>), using String.isBlank() on jsMethod.value() may cause compatibility issues on older Java/Android targets; consider replacing it with a more broadly supported check like value == null || value.trim().isEmpty().
- The new ProGuard rule in consumer-rules.pro (-keepclassmembers,allowobfuscation class * { @com.whl.quickjs.wrapper.JSMethod ;) appears to be missing a closing brace and semicolon for the members block; please double-check and correct the syntax so the keep rule is actually applied.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In QuickJSObject.setProperty(String, Class<?>), using String.isBlank() on jsMethod.value() may cause compatibility issues on older Java/Android targets; consider replacing it with a more broadly supported check like value == null || value.trim().isEmpty().
- The new ProGuard rule in consumer-rules.pro (-keepclassmembers,allowobfuscation class * { @com.whl.quickjs.wrapper.JSMethod <methods>;) appears to be missing a closing brace and semicolon for the members block; please double-check and correct the syntax so the keep rule is actually applied.
## Individual Comments
### Comment 1
<location> `wrapper-java/src/main/java/com/whl/quickjs/wrapper/QuickJSObject.java:106-115` </location>
<code_context>
public void setProperty(String name, Class<?> clazz) {
Object javaObj = null;
try {
- javaObj = clazz.newInstance();
- } catch (IllegalAccessException e) {
+ javaObj = clazz.getConstructor().newInstance();
+ } catch (IllegalAccessException
+ | InstantiationException
+ | InvocationTargetException
+ | NoSuchMethodException e) {
e.printStackTrace();
- } catch (InstantiationException e) {
- e.printStackTrace();
- }
-
- if (javaObj == null) {
- throw new NullPointerException("The JavaObj cannot be null. An error occurred in newInstance!");
}
JSObject jsObj = context.createNewJSObject();
Method[] methods = clazz.getMethods();
+ Object finalJavaObj = javaObj;
</code_context>
<issue_to_address>
**issue (bug_risk):** Constructor failure leads to silent dropping of instance methods with only a stack trace as signal.
With the new use of `clazz.getConstructor().newInstance()`, a missing/inaccessible no-arg constructor (or a constructor that throws) leaves `javaObj` as `null`. The loop then only exposes static methods, and the failure is effectively hidden behind `e.printStackTrace()`. This makes misconfigurations (e.g., adding only a required-args constructor) hard to detect and can silently break JS bindings. Please surface this failure more explicitly: e.g., throw a clear runtime exception, log via your standard logger and fail fast, or support a caller-provided factory when direct instantiation isn’t possible.
</issue_to_address>
### Comment 2
<location> `wrapper-android/consumer-rules.pro:5-6` </location>
<code_context>
--keep class com.whl.quickjs.**{*;}
\ No newline at end of file
+-keep class com.whl.quickjs.**{*;}
+-keepclassmembers,allowobfuscation class * {
+ @com.whl.quickjs.wrapper.JSMethod <methods>;
+}
\ No newline at end of file
</code_context>
<issue_to_address>
**issue (bug_risk):** Allowing obfuscation of @JSMethod methods can break JS APIs that rely on default method names.
Because the binding uses the annotation value or falls back to method.getName() when the value is blank, obfuscating these methods means any JS API that relies on the default (blank) annotation will now be exposed under an obfuscated name in release builds. This is only safe if all JS-exposed methods use an explicit @JSMethod("...") value. If that isn’t guaranteed, consider removing allowobfuscation for these methods or splitting the rule so that only methods with blank annotation values remain unobfuscated.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
106
to
115
| try { | ||
| javaObj = clazz.newInstance(); | ||
| } catch (IllegalAccessException e) { | ||
| javaObj = clazz.getConstructor().newInstance(); | ||
| } catch (IllegalAccessException | ||
| | InstantiationException | ||
| | InvocationTargetException | ||
| | NoSuchMethodException e) { | ||
| e.printStackTrace(); | ||
| } catch (InstantiationException e) { | ||
| e.printStackTrace(); | ||
| } | ||
|
|
||
| if (javaObj == null) { | ||
| throw new NullPointerException("The JavaObj cannot be null. An error occurred in newInstance!"); | ||
| } | ||
|
|
||
| JSObject jsObj = context.createNewJSObject(); |
There was a problem hiding this comment.
issue (bug_risk): 构造函数失败时，只打印堆栈而没有进一步处理，会导致实例方法被静默忽略。
在改为使用 clazz.getConstructor().newInstance() 之后，如果没有无参构造函数、无参构造不可访问，或者构造函数本身抛异常，javaObj 就会保持为 null。后续循环只会暴露静态方法，这个失败则被隐藏在 e.printStackTrace() 中。这样会让配置错误（例如只添加了带必填参数的构造函数）很难被发现，并且会在不明显的情况下破坏 JS 绑定。请更明确地暴露这类失败：例如抛出清晰的运行时异常、通过你们的标准日志组件记录并尽早失败，或者在无法直接实例化时支持调用方提供工厂方法。
Original comment in English
issue (bug_risk): Constructor failure leads to silent dropping of instance methods with only a stack trace as signal.
With the new use of clazz.getConstructor().newInstance(), a missing/inaccessible no-arg constructor (or a constructor that throws) leaves javaObj as null. The loop then only exposes static methods, and the failure is effectively hidden behind e.printStackTrace(). This makes misconfigurations (e.g., adding only a required-args constructor) hard to detect and can silently break JS bindings. Please surface this failure more explicitly: e.g., throw a clear runtime exception, log via your standard logger and fail fast, or support a caller-provided factory when direct instantiation isn’t possible.
Comment on lines
+5
to
+6
| -keepclassmembers,allowobfuscation class * { | ||
| @com.whl.quickjs.wrapper.JSMethod <methods>; |
There was a problem hiding this comment.
issue (bug_risk): 允许对带有 @JSmethod 的方法进行混淆，可能会破坏依赖默认方法名的 JS API。
当前绑定逻辑会使用注解中的 value，如果 value 为空则回退到 method.getName()。如果这些方法被混淆，那么任何依赖默认（空）注解值的 JS API 在发布构建中就会暴露为混淆后的方法名。仅当所有暴露给 JS 的方法都显式指定了 @JSmethod("...") 的值时，这才是安全的。如果无法保证这一点，建议对这些方法去掉 allowobfuscation，或者拆分规则，只对注解值为空的 @JSmethod 方法保持不混淆。
Original comment in English
issue (bug_risk): Allowing obfuscation of @JSmethod methods can break JS APIs that rely on default method names.
Because the binding uses the annotation value or falls back to method.getName() when the value is blank, obfuscating these methods means any JS API that relies on the default (blank) annotation will now be exposed under an obfuscated name in release builds. This is only safe if all JS-exposed methods use an explicit @JSmethod("...") value. If that isn’t guaranteed, consider removing allowobfuscation for these methods or splitting the rule so that only methods with blank annotation values remain unobfuscated.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
优化 JSMethod注解使用
消除 QuickJSObject警告
Summary by Sourcery
优化基于 JSMethod 的 Java-to-JS 方法绑定，并清理 QuickJSObject 的警告。
新功能：
缺陷修复：
改进：
构建：
Original summary in English
Summary by Sourcery
Optimize JSMethod-based Java-to-JS method binding and clean up QuickJSObject warnings.
New Features:
Bug Fixes:
Enhancements:
Build: