KubeJS-Mods / KubeJS

https://kubejs.com
GNU Lesser General Public License v3.0
311 stars 91 forks source link

Cannot instantiate a Java Class or call vararg methods #583

Open C0D3-M4513R opened 1 year ago

C0D3-M4513R commented 1 year ago

Minecraft Version

1.18.2

KubeJS Version

1802.5.5-build.567

Rhino Version

1802.2.1-build.255

Architectury Version

4.10.88

Forge/Fabric Version

Forge 40.2.0

Describe your issue

I would expect

new java('net.minecraft.network.chat.ClickEvent')(java('net.minecraft.network.chat.ClickEvent$Action').OPEN_URL, url)

to return a valid ClickEvent back. Instead I get hit with Error occurred while handling event 'command.registry': TypeError: dev.latvian.mods.rhino.NativeJavaObject@1fbc3b1f is not a function, it is object.

As for just calling Java Reflevction methods like so

java('net.minecraft.network.chat.ClickEvent').getContructor(java('net.minecraft.network.chat.ClickEvent$Action'), java('java.lang.String')).newInstance(java('net.minecraft.network.chat.ClickEvent$Action').OPEN_URL, "url")

that gives a Wrapped java.lang.IllegalArgumentException: argument type mismatch error.

Crash report/logs

No response

C0D3-M4513R commented 1 year ago

Also please don't close this with saying, that I should just use Component.clickEventOf. This issue is a general one. It applies to every java class.

ChiefArug commented 1 year ago

I think for the first one the () for the constructor are trying to execute before the new keyword. Save the result of java() to a variable before trying to do anything with it. Standarrd KJS practice is to name the variable $ClassName.

For the second, its not working because you are passing in a ClickEvent$Action and a NativeString. Not a ClickEvent$Action and a String. Normally Rhino would typewrap the NativeString to a String, however it does not here because newInstance takes a vararg of Objects, so it does not see a need to.

ChiefArug commented 1 year ago

Also, please use the #support channel for stuff like this, like you were told in the discord. This almost certainly isn't a bug.

C0D3-M4513R commented 1 year ago

https://ptb.discord.com/channels/303440391124942858/303440391124942858/1074467065949339769 I was told in discord explicitly to make a support ticket...

ChiefArug commented 1 year ago

Support ticket means a post in the #support channel, not an issue on Github. Issues on Github are for bugs and feature requests, of which this is neither.

C0D3-M4513R commented 1 year ago

Also how do I get a Java String in KubeJS. Because I see no way to do so, which again makes construction of many objects impossible.

So how is this not a bug?

C0D3-M4513R commented 1 year ago

Also according to your logic:

global.ClickEvent = java('net.minecraft.network.chat.ClickEvent');
const ClickEvent = global.ClickEvent;
console.log(new ClickEvent(global.ClickEventAction.OPEN_URL, url));

should work, but it fails with the same error message.

ChiefArug commented 1 year ago

So how is this not a bug?

Because Rhino will and does convert between NativeString and String whenever it sees a need to. The only time it doesn't is when the code could take a NativeString already, ie it takes an Object. (note that type erasure does apply here as well). If the method says it takes an Object there is no way to know if it should actually be taking something else. Rhino can only see it takes an Object. You can force Rhino to coerce something to a String by using the String constructor that takes a String. Rhino will see that it takes a String and convert your NativeString to an actual String before passing it

const $String = java('java.lang.String')
let foo_js = "foo"
let foo_java = new $String(foo_js)

\ I am not sure why that is still failing with that updated example. Someone will probably have to do some testing. If it does turn out to be a bug, it will need fixing in Rhino, which is no longer getting updated because Lat is busy working on its successor, Ichor. So there is not much you can do either way unfortunately. Use ComponentWrapper#clickEventOf for this usecase. If there is another use case that you need reflection for ask in the Discord #support channel and we can help you figure out another way around it.

C0D3-M4513R commented 1 year ago

So this just went from being a nuisance, to being a major BLOCKING issue for us. We cannot use the forge permission system currently, due to this constraint. (adding $ infront of PermissionNode changed nothing btw.)

const PermissionTypes = java('net.minecraftforge.server.permission.nodes.PermissionTypes');
const PermissionNode = java('net.minecraftforge.server.permission.nodes.PermissionNode');
const $String = java('java.lang.String');
const modId = 'id';
const deletePermission = 'delete';
new PermissionNode(//startup_scripts:warp_perms.js#19
        new $String(modId),
        new $String(deletePermission),
        PermissionTypes.BOOLEAN,
        ()=>false,
        []
    );

Startup log:

[19:02:19] [INFO ] Hello, World! (You will only see this line once in console, during startup)
[19:02:19] [INFO ] Loaded script startup_scripts:script.js in 0.064 s
[19:02:19] [INFO ] Loaded script startup_scripts:utils.js in 0.015 s
[19:02:19] [INFO ] Loaded script startup_scripts:permissions.js in 0.012 s
[19:02:19] [ERR  ] Error loading KubeJS script: TypeError: dev.latvian.mods.rhino.NativeJavaObject@3fdd9595 is not a function, it is object. (startup_scripts:warp_perms.js#19)
[19:02:19] [INFO ] Loaded 3/4 KubeJS startup scripts in 0.436 s
MichaelHillcox commented 1 year ago

What am I doing wrong here?

https://c.mikey.pro/scr/yEWWawlK.png

const clickEvent = java('net.minecraft.network.chat.ClickEvent');
const action = java("net.minecraft.network.chat.ClickEvent$Action")
const e = new clickEvent(action.RUN_COMMAND, "/list");

console.log(e.getAction())
console.log(e.getValue())

const PermissionTypes = java('net.minecraftforge.server.permission.nodes.PermissionTypes');
const PermissionNode = java('net.minecraftforge.server.permission.nodes.PermissionNode');

const modId = 'id';
const deletePermission = 'delete';
const node = new PermissionNode(
        modId,
        deletePermission,
        PermissionTypes.BOOLEAN,
        ()=>false,
        []
);

console.log(node.getNodeName())
console.log(node.getReadableName())
console.log(node.getDefaultResolver())

I can run both of your example scripts absolutely fine in 1.18.2? cu

C0D3-M4513R commented 1 year ago

@MichaelHillcox what versions of KubeJS, Rhino and Architectury are you using? Depending on that, the issue may already be fixed, which would be good to hear.

EDIT: This code also works for me. Let me find a minimal example where it breaks. Because quite frankly I don't know where the major difference lies.

MichaelHillcox commented 1 year ago

Tested in dev using the latest dev branch for 1.18.2. I will note, class loading in dev can be a bit funky, so I had modified some code, but the production env didn't have any modifications. I dumped my code into the server scripts folder, and it just worked as shown above.

This is what I used to test in a production env

If you can find a way to break it, please let me know and I'll look into it more. I'm trying to take up some more slack with KubeJS dev where I can.

C0D3-M4513R commented 1 year ago

try:

const clickEvent = java('net.minecraft.network.chat.ClickEvent');
const action = java("net.minecraft.network.chat.ClickEvent$Action")
const e = new clickEvent(action.RUN_COMMAND, "/list");

console.log(e.getAction())
console.log(e.getValue())

global.PermissionTypes = java('net.minecraftforge.server.permission.nodes.PermissionTypes');
global.PermissionNode = java('net.minecraftforge.server.permission.nodes.PermissionNode');
const PermissionTypes = global.PermissionTypes;
const PermissionNode = global.PermissionNode;

const modId = 'id';
const deletePermission = 'delete';
const node = new PermissionNode(
        modId,
        deletePermission,
        PermissionTypes.BOOLEAN,
        ()=>false,
        []
);

console.log(node.getNodeName())
console.log(node.getReadableName())
console.log(node.getDefaultResolver())
squoshi commented 1 year ago

Ideally, as Chief said, you can use the support forums on the Discord. The GitHub is to report issues with the mod itself and give feature requests, and again, as already stated, this is not a bug or a feature request.

C0D3-M4513R commented 1 year ago

This is a issue with the mod itself. PermissionNode fails to properly construct. How is this not a mod issue, but my error? I fail to see the reason here. Assigning stuff to a variable once shouldn't change anything, but clearly does in this case?

Also Chief's response was based on that the js string is represented by Rhino's NativeString, but I've shown that even with proper types and no java.lang.Object parameters this still happens, so from my Point of View his critique doesn't matter anymore.

MichaelHillcox commented 1 year ago

I'm testing your code now, but if this doesn't work, my assumption is it's an issue with how global is handled more than it is an issue with the construction of Java classes.

C0D3-M4513R commented 1 year ago

Yeah, that is what I was thinking too. I tried not using global (but instead an intermediate const variable), but that actually worked.

MichaelHillcox commented 1 year ago

Right, I can confirm, there is definitely something wrong with assigning a Java class directly to the global object. I'd assume it's an issue with the Java sided data type that backs this data store, but without looking I can't confirm.

Here is a solid test case

global.sharedClasses = {
  PermissionTypes: java(
    "net.minecraftforge.server.permission.nodes.PermissionTypes"
  ),
  PermissionNode: java(
    "net.minecraftforge.server.permission.nodes.PermissionNode"
  ),
};

global.PermissionTypes = java("net.minecraftforge.server.permission.nodes.PermissionTypes");
global.PermissionNode = java("net.minecraftforge.server.permission.nodes.PermissionNode");

// Self contained block to stop scope creep
(() => {
  const node = new global.sharedClasses.PermissionNode(
    "id",
    "delete",
    global.sharedClasses.PermissionTypes.BOOLEAN,
    () => false,
    []
  );

  console.log(node.getNodeName());
  console.log(node.getReadableName());
  console.log(node.getDefaultResolver());
})();

// Self contained block to stop scope creep
console.log("Second");
(() => {
  const node = new global.PermissionNode(
    "id",
    "delete",
    global.PermissionTypes.BOOLEAN,
    () => false,
    []
  );

  console.log(node.getNodeName());
  console.log(node.getReadableName());
  console.log(node.getDefaultResolver());
})();

Although this is clearly an issue, I'd recommend against polluting the global space with classes regardless. My recommendation would be to do something like I have done by containing the shared classes inside a nested object.

global.sharedClasses = {
  PermissionTypes: java(
    "net.minecraftforge.server.permission.nodes.PermissionTypes"
  ),
  PermissionNode: java(
    "net.minecraftforge.server.permission.nodes.PermissionNode"
  ),
};
C0D3-M4513R commented 1 year ago

So

global.sharedClasses = {
  PermissionTypes: java(
    "net.minecraftforge.server.permission.nodes.PermissionTypes"
  ),
  PermissionNode: java(
    "net.minecraftforge.server.permission.nodes.PermissionNode"
  ),
};

works, for creating class instances? like new global.sharedClasses.PermissionNode(some_args)?

Edit: It doesn't.

MichaelHillcox commented 1 year ago
  const node = new global.sharedClasses.PermissionNode(
    "id",
    "delete",
    global.sharedClasses.PermissionTypes.BOOLEAN,
    () => false,
    []
  );

  console.log(node.getNodeName());
  console.log(node.getReadableName());
  console.log(node.getDefaultResolver());

This code here works fine for me

C0D3-M4513R commented 1 year ago

huh, so probably just another instance of me not being able to type... I'm just glad that there is a workaround.

MichaelHillcox commented 1 year ago

I'll investigate and talk to Lat, I'm pretty sure it's gunna be an issue with the backing data store, likely quite far down the stack so it'll be a pain to fix