8000 Type Consolidator & Generic support for Map/List/Method/Constructor by ZZZank · Pull Request #1853 · mozilla/rhino · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Type Consolidator & Generic support for Map/List/Method/Constructor #1853

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Draft
wants to merge 45 commits into
base: master
Choose a base branch
from
Draft

Type Consolidator & Generic support for Map/List/Method/Constructor #1853

wants to merge 45 commits into from

Conversation

ZZZank
Copy link
Contributor
@ZZZank ZZZank commented Mar 18, 2025

Java Generic support

3 PRs in total:

PR#2: Type Consolidator

This PR includes changes from #1849

Before #1849 is reviewed, this PR will keep being a draft.

A description for this PR is not yet finished, you can see descriptions in #1849 for how type consolidation works, and newly added tests for what this PR brings

ZZZank added 30 commits February 28, 2025 12:06
@ZZZank
TypeInfo
65a666a
@ZZZank
make params in ParameterizedTypeInfo immutable
932aa47
@ZZZank
allow more flexible TypeInfo formatting
a566551
@ZZZank
make NoTypeInfo singleton
af7ca07
@ZZZank
better primitive class matching
2a5734d
@ZZZank
replace Context.jsToJava internal to use TypeInfo
4b639c0
@ZZZank
add TypeInfo.getTypeTag()
e354b0e
@ZZZank
rename TypeInfo.componentType() to getComponentType() for consistency
e32a3a8
@ZZZank
make MemberBox#memberObject an Executable so that get type info lazil…
46034c4 
…y is possible
@ZZZank
make createInterfaceAdapter(...) accepts a class instead of TypeInfo
e088ace
@ZZZank
use TypeInfo for atg type whenever possible
17ef489
@ZZZank
add "main bound" for VariableType to make sure asClass() can return…
6cd3271 
… the same class as the one provided by Java
@ZZZank
simplify component class collecting
3c039cb
@ZZZank
fix type tag for ParameterizedTypeInfo
b912a5b 
it's not likely to happen, but always be cautious
@ZZZank
generic info for method return type
a84a63b
@ZZZank
add TypeInfo.isAssignableFrom and TypeInfo.isInstance, apply spot…
ca626e7 
…less
@ZZZank
remove MemberBox#argTypes
76fbc61
@ZZZank
fix compiling and Android compatibility
fa7fb52
@ZZZank
fix function preference and array creation
8204334
@ZZZank
fix live connect signature, add test
265c770
@ZZZank
spotless, fix come compilation warnings
099b3dc
@ZZZank
FN_INTERFACE should be private static final
d18ca22
@ZZZank
add test
df4d6d2
@ZZZank
spotless
7385976
@ZZZank
java doc++, rename isObject() to isObjectExact()
a5280c1
@ZZZank
better TypeInfo.isNumber(), add test for it
a26a4fd
@ZZZank
add more predefined types in TypeInfoExt and make use of them
d3b13ff
@ZZZank
avoid reference comparison on types with generic
3439726
@ZZZank
fix again
77afae0
@ZZZank
3rd fix, javadoc++
055e601
ZZZank added 13 commits March 17, 2025 09:42
@ZZZank ZZZank mentioned this pull request Mar 18, 2025
Closed
@ZZZank
Copy link
Contributor Author
ZZZank commented Mar 18, 2025 •
edited
Loading

I think I found an inconsistency between test and documentation. The method WrapFactory#wrap(...) requires a param named staticType, that was described as:

     * @param staticType type hint. If security restrictions prevent to wrap object based on its
     *     class, staticType will be used instead.

So, in org.mozilla.javascript.tests.SecurityControllerTest, when there is a security restriction present, accessing members from implementation should cause an error, but the test itself claimed:

        // f.create produces "SomeClass extends ArrayList<String> implements
        // SomeInterface"
        // we may access array methods, like 'size' defined by ArrayList,
        // but not methods like 'bar' defined by SomeClass, because it is in a restricted package

The return type of f.create() is SomeInterface, so when wrapping object, the staticType is SomeInterface. In this case, since SomeInterface did not define the .size() method, the method should not be considered as accessible. That is, line92:

rhino/tests/src/test/java/org/mozilla/javascript/tests/SecurityControllerTest.java

Lines 90 to 93 in f8ce76a

"f = new com.example.securitytest.SomeFactory();\n"
+ "var i = f.create();\n"
+ "i.size();\n"
+ "i.bar();";

should cause an error, when access is restricted.

It was not throwing exception before because NativeJavaList was always using the list class itself as staticType. But now it breaks because I actually passed a staticType from WrapFactory to NativeJavaList

This was referenced Apr 17, 2025
Open
Open
@rPraml
Copy link
Contributor
rPraml commented May 5, 2025

@gbrail I would like to 'ping' you about feedback of this PR and how to proceed:
My summary:

  • I do understand (at least I hope), what this PR does
  • I did a review and this makes technically sense
  • I am not happy about the package structure (First: it introduces cycles, Second: naming)

For the cycles, I made an experimental branch, where I tried to break it: https://github.com/FOCONIS/rhino/tree/type-consolidator-2
I can make a PR for easier review, but credits should go to @ZZZank ;)

@rPraml rPraml mentioned this pull request May 13, 2025
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ZZZank @rPraml
0