search

SkriptLang / skript-reflect

Powerful reflection utilities for Skript.
MIT License
65 stars 19 forks source link

Skript cannot save classes imported by skript-reflect. #56

Open ashandelle opened 2 years ago

ashandelle commented 2 years ago

Describe the bug Skript cannot serialize imported classes even if they implement serializable. (Only tested on BigDecimal and MathContext)

To reproduce

  1. Import the class
import:
    java.math.BigDecimal
  1. Create variable
set {test} to new BigDecimal(100)
  1. Attempt to reload or stop (save variables)

After doing this errors appear and skript will be unable to save any variables.

Expected behavior Expected to be able to save java's BigDecimal class to variables.csv but was instead unable to save anything.

Screenshots image image image image

Server information

ashandelle commented 2 years ago

Actually this is an even bigger bug than I thought, when trying to just create a variable even if its marked as unsaved (by changing the config and enabling the - prefix) it will still throw the error. image image image

TPGamesNL commented 2 years ago

By default, no java objects (anything returned from a method, constructor or field call) can be saved. This is because all java objects are wrapped in an ObjectWrapper, which is a registered skript-reflect type (named javaobject). Any object (talking about regular Skript objects here, not Java) can only be saved by Skript if the type has a registered serializer. So, because we cannot guarantee that every java object can be serialized, skript-reflect doesn't have a serializer registered for javaobject.

However, to allow using java objects in regular Skript syntax, skript-reflect will automatically convert a javaobject to a Skript object (from another registered type) if needed. For example, if you use send "abc" to event.getPlayer(), skript-reflect converts the javaobject (from the method call event.getPlayer()) to a Skript player object. This allows it to be used in the send effect here.

Skript also has a number type registered, which is represented by the class java.lang.Number. While Skript does have a serializer registered for this type, allowing you to store numbers in your CSV file, this serializer isn't actually used by Skript. This is because Skript's serialization system, Yggdrasil, serializes the regular numbers natively: int, float, long, double etc, and all of their corresponding wrapper types (java.lang.Integer, java.lang.Double, all extending java.lang.Number) don't require a registered serialized, Yggdrasil handles these by itself.

This is where the problem arises: Skript says it can serialize all java.lang.Number, by having a registered serializer for the number type. However, the actual serialization (implemented natively by Yggdrasil) can only handle the common number implementations, i.e. the wrapper classes like Integer and Double. The uncommon implementations, such as BigDecimal, don't have any functioning serializer, despite what Skript indicates. This is what causes your errors: the javaobject returnen from the constructor, a BigDecimal, is converted to a more direct Skript object, namely a number, and Skript tries to serialize it as such, but it can't because of the reason above.

This issue should be fixed in Skript, because Skript having a number type with a serializer registered says it'll be able to serialize all numbers, not just some.

If you test this on MathContext, you wouldn't get the same errors, but instead you'd just see that the variable is <none> after restarting your server. This is because java objects don't have serializer and can therefore not be saved, not depending on if they implement Serializable or not. Usually Skript would give a warning for this, just like if you try to save a living entity in a global variable, but this is currently not possible due to some internal stuff of both skript-reflect and Skript.

I might be able to do something about this (meaning I might be able to print that warning when reloading your script), but only in some cases, so I don't think I'll spend time on this soon (for myself if I change my mind in the future: return type of ExprJavaCall can be derived, if the java call is known: target object return type + method name + known signature gives a java member match, at runtime must be instance of that member return type, or for constructor even easier).

While you can mark variables in Skript as unsaved using the - tag, this is just a side effect of being able to split your variable storage over multiple databases, and is therefore not well supported. Skript doesn't know that your variable won't be saved in a database until very late in its process, so it has already serialized the variable at that point (or, in your case, tried serializing the variable).

https://github.com/SkriptLang/Skript/issues/1049 talks about this stuff, but it also mentions that the current solution (editing database's variable pattern) isn't great.

ashandelle commented 2 years ago

Thanks for responding, I've decided to just try and make functions to convert them to and from strings and just store those instead. Hopefully that works.