Suggestion
- ClassInfo's
parser field is upper-bounded on T (Parser<? extends T>); it should be invariant (Parser<T>).
- ClassInfo accepts
Class<T> as its constructor parameter. It should instead accept Class<? super T> or Class<?> and do an unchecked cast internally.
Why?
-
Parsers are both producers and consumers of their parameter type; String toString(T, int) is a consumer of T whereas T parse(String, ParseContext) is a producer of T. By typing the field as <? extends T>, we are forced into a contract where our T is not necessarily a valid input to toString (as it may expect some arbitrary subclass of T).
For example, in the current system Parser<Long> is a perfectly valid parser for ClassInfo<Number>. This is fine for Parser#parse, but incorrect for Parser#toString: the `ClassInfo needs its parser to convert any Number to a string, regardless of whether it's a Long or not.
-
Since types with generic parameters are invariant by default, Class<T<Q>> is not a sub-type of Class<T>. This means our current system prevents T from having generic parameters. For example, Skript's minecrafttag class info has the type ClassInfo<Tag>, when it should have ClassInfo<Tag<?>>.
Other
Changing these types would be a breaking API change in some cases; for example, Skript's minecrafttag class info would no longer compile after this change because Java incorrectly infers it to be ClassInfo<Tag>; this can be fixed by explicitly specifying the type parameter as Tag<?>: new ClassInfo<Tag<?>>(Tag.class, "minecrafttag")
I'm particularly interested in addon developers' opinion about this change 😅 (@ShaneBeee @eyesniper2?). These changed types would be objectively correct and prevent errors, but the changes would force you to update your add-ons to match. Because of erasure, these changes should be completely backwards-compatible.
Agreement
Suggestion
parserfield is upper-bounded on T (Parser<? extends T>); it should be invariant (Parser<T>).Class<T>as its constructor parameter. It should instead acceptClass<? super T>orClass<?>and do an unchecked cast internally.Why?
Parsers are both producers and consumers of their parameter type;
String toString(T, int)is a consumer ofTwhereasT parse(String, ParseContext)is a producer ofT. By typing the field as<? extends T>, we are forced into a contract where ourTis not necessarily a valid input totoString(as it may expect some arbitrary subclass ofT).For example, in the current system
Parser<Long>is a perfectly valid parser forClassInfo<Number>. This is fine forParser#parse, but incorrect forParser#toString: the `ClassInfo needs its parser to convert any Number to a string, regardless of whether it's a Long or not.Since types with generic parameters are invariant by default,
Class<T<Q>>is not a sub-type ofClass<T>. This means our current system preventsTfrom having generic parameters. For example, Skript'sminecrafttagclass info has the typeClassInfo<Tag>, when it should haveClassInfo<Tag<?>>.Other
Changing these types would be a breaking API change in some cases; for example, Skript's
minecrafttagclass info would no longer compile after this change because Java incorrectly infers it to beClassInfo<Tag>; this can be fixed by explicitly specifying the type parameter asTag<?>:new ClassInfo<Tag<?>>(Tag.class, "minecrafttag")I'm particularly interested in addon developers' opinion about this change 😅 (@ShaneBeee @eyesniper2?). These changed types would be objectively correct and prevent errors, but the changes would force you to update your add-ons to match. Because of erasure, these changes should be completely backwards-compatible.
Agreement