Hey folks, I wanted to send this over the mailing list as I am not sure it's a "bug" per-se and there might be some discussion on it too. Hopefully this is helpful all the same.
I have been using Groovy 6's recent betas and believe I have found a snag with the new experimental invoke dynamic cold reflective feature/functionality. There's two "threads" for discussion here that are of note. I have split them out explicitly for the sake of convenience and clarity. — First: `groovy.indy.cold.reflection` Default Value Mismatch? It looks like the default value of `groovy.indy.cold.reflection` is set to true/on by default in the code at https://github.com/apache/groovy/blame/master/src/main/java/org/codehaus/groovy/vmplugin/v8/IndyInterface.java#L64. However, according to the Git commit message/PR description that merged in the feature and that line (https://github.com/apache/groovy/pull/2673), it is supposed to _off_ by default. There's probably good arguments for both. My best guess is that the PR description/Git commit message is erroneous here and it really _is_ supposed to be enabled and on by default. However, I could very well likely be wrong and mistaken, I am just speculating. The slight mismatch not withstanding, I was a bit surprised to find this out anyway since the facility is listed as an experimental feature in the ticket GROOVY-12137 and throughout the code’s documentation (granted, it's listed as "incubating" in the release notes as part of AOT support). I would think that experimental features are generally are opt-in (i.e. I interpret experimental as not stable/reliable for production usage just yet). Not wrong and I am sure that the rationale behind it makes sense (i.e. if we don't turn it on then nobody would know to use it), just surprising that's all. BTW, regardless of how it's resolved, the feature flag is quite well hidden anyways. By sheer dumb luck, I only knew about this facility even being feature flagged since it was part of some other performance work I was doing on the compiler and just so happened to be of relevance that I had it in the back of my mind. It's not listed as having a toggle'able flag anywhere in the release notes on/at https://groovy-lang.org/releasenotes/groovy-6.0.html either. I would then guess that the feature flag is more of a convenience escape-hatch for rare edge cases as part getting the feature ready to be finalized/made production-ready, rather than an explicitly, fully supported switch intended for end-user configuration at their discretion? — Second: Cold Reflection & Callstack Snag. This is the actual root of the "problem" and where it gets interesting from a design perspective. Well, I am not sure it's _exactly_ a "problem" per-se. It _might_ be? So in truth, I only actually noticed something was wrong due to seeing my logs for my application all of a sudden start reporting JDK internal methods as the logger's callsite when using Groovy 6 (e.g. all of my logs were printing `jdk.internal.reflect.DirectMethodHandleAccessor.invoke` as the method where `log.info`/etc. was being invoked from instead of their real method). This was extremely suspicious and didn't make sense as to why logging all of a sudden just kind of broke. After some investigating, I found out why I was seeing `jdk.internal.reflect.DirectMethodHandleAccessor.invoke` as the callsite: the changes in IndyDispatch to support Cold Reflective Invocation. Thankfully Logback had a facility already baked in, `%caller{N}`, for dumping out the callstack's stack trace for a number of frames to make finding this out much easier. Without just papering over the issue by registering Groovy's internal compiler packages as framework internals in Logback, I thought about this a bit more: I wondered if this change might subtly affect other things that don't expect the callstack to change/have intermediaries when invoked dynamically? I haven't done a thorough investigation or spent time digging into this in any great depth, however, I would imagine that there are some deeper, subtler implications elsewhere that this might cause. I just happened to find this out via logging, which is not necessarily mission critical (though one could make the argument that having data silently go missing from logs is far from acceptable). I would be curious to see if anyone else runs into subtleties with having reflection based invocations enabled by default. I am not saying it's a bad feature or should be removed/changed or anything. Just food-for-thought. — Anyhow, I just thought I should surface this to the team and in case anyone else runs into this issue, they can understand "what" and “why” a little easier. Maybe this should get some eyeballs on it before Groovy 6 ships so that if it needs some tweaking or extra documentation, it can be ready in time? Thanks, Matt
