Avoid SuppressWarnings of "this-escape" in Java Setting hierarchy
With #16496 and commit 5ca068693a6cbec28bf794cb1cf6f560c5053351 in #16490 were introduced some @SupressWarning annotations to avoid errors with JDK 21 like:
/buildkite/builds/bk-agent-prod-k8s-1728654414425952824/elastic/logstash-pull-request-pipeline/logstash-core/src/main/java/org/logstash/settings/BaseSetting.java:105: warning: [this-escape] possible 'this' escape before subclass is fully initialized
--
| validate(defaultValue);
| ^pileJava
| error: warnings found and -Werror specified
| 1 error
| 1 warning
|
| > Task :logstash-core:compileJava FAILED
|
| FAILURE: Build failed with an exception.
As per comment https://github.com/elastic/logstash/pull/16490#pullrequestreview-2360999039 this issue is to ask for removal of such annotations and leverage static method instead of calling instance methods on non-completely initialised instances.
Shaped the rough idea in #16543 but still coerce method is harder to avoid https://github.com/elastic/logstash/blob/6064587bc48d2e542cf7be7022f4e7cbcd661afa/logstash-core/src/main/java/org/logstash/settings/Coercible.java#L31
Hey @andsel @robbavey, dusted off this JDK 21 nit while auditing build warnings—smart call to ditch those @SuppressWarnings for cleaner hierarchy init. Skimmed BaseSetting.java and the Coercible interface; the this-escape pops because we're invoking instance validate/coerce pre-super() full init, but yeah, coercing defaults in a static wrapper feels like the play. Quick root & hurdles:
- Core issue: Subclass constructors call super(defaultValue) → triggers validate(defaultValue) on half-baked this. Static refactor sidesteps it without losing type safety.
- Sticky bit in #16543: Coercible.coerce(defaultValue)—it's instance-bound, but we could hoist to a static util (e.g., Coercible.coerceStatic(Class<T> clazz, Object raw)) with reflection or generics magic, or make coerce a static default method in the interface (Java 8+ friendly).
Fix sketch if picking up:
- Refactor BaseSetting ctor to static factory: static <T> Setting<T> create(...) { validateStatic(raw); return new Subclass(...); }.
- For Coercible: Add static overload, migrate callers (e.g., in StringSetting), add specs for init flows.
- Bonus: Bump min JDK in CI to 21, gate warnings-as-errors.
#16543's a great skeleton—any thoughts on the coerce refactor? Happy to revive it with a branch/tests. Can you assign this to me?
Hi @0xShubhamSolanki thanks a lot to chime in!
If you would you can provide a new PR with the solution you are proposing, so that we can think about it, summarizing in the PR which are the changes you are proposing.
My idea was to completely migrate the settings hierarchy from Ruby to Java and then fix this issue, to have full context, but I don't expect any significant changes in the super - sub constructors call chains to pop in. So if you mind to shape a solution for this I would me more than happy to review it!