WrappedConfig Strings don't seem to read from files

[sisby-folk]

(from kaleido#2)

under a quick test, all the basic object types seem to work except String.

[OroArmor]

I believe this is a problem with the old WrappedConfig system, as JVM details can change whether or not to modify a final field. If you switch to the new ReflectiveConfig system, that should work.

[sisby-folk]

We're die-hard wrappedconfig users and strongly feel it's one of the main advantage qconfig has over other libraries - if strings don't work in wrapped, we'd love a workaround for just that type specifically. Direct field access is what we prefer, but if strings specifically won't work, I'll accept something that requires .get().

[ix0rai]

@sisby-folk it seems like the only reason other types currently work is because the JVM isn't perfect and doesn't do constant inlining consistently, as discussed in discord. if we were to implement a workaround for strings, it would be perfectly possible that a Java update would break other types. as such, I don't really see how we can support wrapped configs long-term.

[sisby-folk]

Having read the thread, doesn't removing the requirement for the field to be final solve this problem?

It's correct that setting primitives in code wouldn't work - but they never have, and that just needs to be documented if it can't practically be enforced by the compiler.

Instead of primitives must be final, can't we just do primitives can't be final? lists and maps and sections can stay final because they work fine, but it seems like a reasonable limitation of the representation in code to me.

[ix0rai]

removing the final requirement does make everything work -- but the design becomes inconsistent, stuff will silently fail, and I'm not sure docs are enough mitigation for the newly weird behaviour

to me, dropping the final requirement on just a few select types is even worse, since it introduces yet another inconsistency

[lukebemish]

Requiring the field to be non-final fixes the problem, yes, but at that point you've removed the main advantage of wrapped-config - the idea of an immutable state read in and then not modifiable

If you were to go with this sort of WrappedConfig system, you'd want to force nothing to be final - while it might work "right now" for lists/maps, there's no reason a sufficiently smart JVM couldn't find places to safely inline those or do other weirdnesses that this wouldn't work with

[sisby-folk]

As someone who uses wrappedconfig in like eight mods, it doesn't remove the main advantage of wrappedconfig for us - we like direct field access. a public static field of a standard java type (list, map, boolean, etc) is an intuitive thing for a config value to be, and I like how it looks when read out.

nothing being allowed to be final seems fine but can we make sure that's a warning or a compile time error and not a runtime crash? We need to preserve stability on our end for jij.

[ix0rai]

making that change requires breaking the wrapped config API, which is a no-go on our end due to QLoader.

what I could see as a future for wrapped is you pulling it into kaleido, and supporting it from there!

[lukebemish]

Unfortunately that's not something that can easily be done as a compile-time warning or anything either, at least not without a custom annotation processor or something - and for a given config to be guaranteed to work no matter what JVM it's running on, it really does have to be something that is required

[sisby-folk]

I don't see the issue, can't the requirement for fields to be final just be dropped and a warning logged at runtime when a field is final? That's not a breaking API change.

wrappedconfigs should at least be fixed before they're deleted. And yeah we may well adopt them when they are properly removed.

[ix0rai]

yeah, we can do that I'll look into that change sometime this week

[sisby-folk]

If the warning needs to be dev only to avoid players complaining that's fine, I just wanna have this last use case working before any big migration or rework (one of the kaleido things we wanted to ensure is not breaking api without changing classpath!)

[ix0rai]

the warning would be global (probably just via sysout lol) because I am too lazy to special-case stuff in QLoader :p

[sisby-folk]

This is looking really good! one issue - this warning shouldn't be applying to maps or lists should it? probably not sections either?

[lukebemish]

To what degree the JVM JIT trims out references to final fields is an implementation detail, so I'd argue it definitely should apply to all final fields being replaced this way -- but on the flip side string/other normally constant fields are the biggest worries by far

[ix0rai]

it's applying to everything for that reason, and simply for consistency

[sisby-folk]

sure thing, fair enough - I'll just go with non-final for everything then!