[
https://issues.apache.org/jira/browse/GROOVY-12378?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18113128#comment-18113128
]
ASF GitHub Bot commented on GROOVY-12378:
-----------------------------------------
Copilot commented on code in PR #2901:
URL: https://github.com/apache/groovy/pull/2901#discussion_r3964650847
##########
src/main/java/org/codehaus/groovy/transform/LogASTTransformation.java:
##########
@@ -171,8 +241,15 @@ private Expression addGuard(final MethodCallExpression
mce) {
variableExpression.setAccessedVariable(logNode);
}
+ boolean simpleArguments = usesSimpleMethodArgumentsOnly(mce);
+ if (staticLocation) {
+ Expression withLocation =
loggingStrategy.wrapLoggingMethodCallWithLocation(
+ receiver, methodName, mce, locationOf(mce,
logNode.getOwner()), !simpleArguments);
+ if (withLocation != null) return withLocation;
+ }
Review Comment:
`locationOf(...)` has a side effect (it appends a new synthetic field) even
if `wrapLoggingMethodCallWithLocation(...)` returns `null` (meaning: leave the
call unchanged). That can create unused synthetic `$log$loc$*` fields for calls
that ultimately aren’t rewritten.
##########
src/main/java/org/codehaus/groovy/transform/LogASTTransformation.java:
##########
@@ -101,9 +110,21 @@ public void visit(final ASTNode[] nodes, final SourceUnit
sourceUnit) {
if (!(targetClass instanceof ClassNode classNode))
throw new GroovyBugError("Class annotation " +
logAnnotation.getClassNode().getName() + " annotated no Class, this must not
happen.");
+ final boolean staticLocation = lookupStaticLocation(logAnnotation);
+ if (staticLocation && !loggingStrategy.supportsStaticLocation()) {
+ addError("staticLocation is not supported by " +
loggingStrategy.getClass().getName()
+ + (loggingStrategy instanceof StaticLocationCapable
+ ? " here: the logging API it needs is not on the
compile classpath"
+ : ""), logAnnotation);
+ return;
+ }
Review Comment:
When `staticLocation` is requested but the strategy reports it can't support
it, the error message is fairly non-actionable (and the “compile classpath”
suffix is bolted on via `StaticLocationCapable`). Consider constructing the
message so it clearly distinguishes “unsupported strategy” vs “missing required
logging API on compile classpath”.
##########
subprojects/groovy-logging-test/src/test/groovy/groovy/util/logging/Log4j2Test.groovy:
##########
@@ -357,4 +358,166 @@ final class Log4j2Test {
assert appenderForCustomCategory.getEvents().size() == 1
assert appender.getEvents().size() == 0
}
+
+ // GROOVY-12378
-------------------------------------------------------------
+
+ /** line number (1-based) of the first line of {@code source} containing
{@code needle} */
+ private static int lineOf(String source, String needle) {
+ int idx = source.readLines().findIndexOf { it.contains(needle) }
+ assert idx >= 0 : "no line contains $needle"
+ idx + 1
+ }
+
+ /** each test compiles its own class: Log4j2 loggers are cached by name,
and a
+ * second appender registered under an existing name is ignored */
+ private static String staticLocationSource(String className) { '''
+ @groovy.util.logging.Log4j2(staticLocation = true)
+ class CLASSNAME {
+ static int evaluations = 0
+ static String expensive() { evaluations++; 'expensive' }
+
+ def instanceMethod() {
+ log.info('plain') // L1 simple
arguments: no guard
+ log.warn("interpolated ${expensive()}") // L2 guarded
+ [1].each {
+ log.error('from closure') // L3 inside a closure
+ }
+ }
+ static void staticMethod() {
+ log.debug('static {}', 42) // L4 parameterised
+ }
+ def withThrowable() {
+ try { throw new IllegalStateException('boom') } catch (e) {
log.error('failed', e) }
+ }
+ def withMarker(org.apache.logging.log4j.Marker m) {
+ log.info(m, 'marked {}', 'x')
+ }
+ def withMarkerAndThrowable(org.apache.logging.log4j.Marker m,
Throwable t) {
+ log.warn(m, 'both {}', 'y', t)
+ }
+ }
+ '''.replace('CLASSNAME', className) }
+
+ @Test
+ void testStaticLocationSuppliesCompileTimeLocations() {
+ String source = staticLocationSource('LocatedA')
+ Class clazz = new GroovyClassLoader().parseClass(source,
'LocatedA.groovy')
+ clazz.log.addAppender(appender)
+ clazz.log.setLevel(Level.ALL)
+ clazz.newInstance().instanceMethod()
+ clazz.staticMethod()
+
+ def events = appender.events
+ assert events*.message == ['plain', 'interpolated expensive', 'from
closure', 'static 42']
+ assert events*.source*.className == ['LocatedA'] * 4
+ assert events*.source*.fileName == ['LocatedA.groovy'] * 4
+ assert events*.source*.methodName == ['instanceMethod',
'instanceMethod', 'instanceMethod', 'staticMethod']
+ assert events*.source*.lineNumber == ['L1', 'L2', 'L3', 'L4'].collect
{ lineOf(source, it) }
+
+ // one synthetic static final field per logging statement
+ def locations = clazz.declaredFields.findAll {
it.name.startsWith('$log$loc$') }
+ assert locations.size() == 7
+ assert locations.every { isStatic(it.modifiers) &&
isFinal(it.modifiers) && it.synthetic && it.type == StackTraceElement }
+ }
+
+ @Test
+ void testStaticLocationKeepsGuardForNonSimpleArguments() {
+ Class clazz = new
GroovyClassLoader().parseClass(staticLocationSource('LocatedB'),
'LocatedB.groovy')
+ clazz.log.addAppender(appender)
+ clazz.log.setLevel(Level.ERROR)
+ clazz.newInstance().instanceMethod()
+
+ assert clazz.evaluations == 0 : 'a disabled level must not evaluate
the interpolated argument'
+ assert appender.events*.message == ['from closure']
+ }
+
+ @Test
+ void testStaticLocationThrowableAndMarker() {
+ Class clazz = new
GroovyClassLoader().parseClass(staticLocationSource('LocatedC'),
'LocatedC.groovy')
+ clazz.log.addAppender(appender)
+ clazz.log.setLevel(Level.ALL)
+ def marker =
org.apache.logging.log4j.MarkerManager.getMarker('GROOVY12378')
+ def instance = clazz.newInstance()
+ instance.withThrowable()
+ instance.withMarker(marker)
+ instance.withMarkerAndThrowable(marker, new
IllegalArgumentException('bad'))
+
+ def events = appender.events
+ assert events.size() == 3
+ assert events[0].message == 'failed'
+ assert events[0].thrown instanceof IllegalStateException
+ assert events[0].source.methodName == 'withThrowable'
+ assert events[1].message == 'marked x'
+ assert events[1].marker == marker
+ assert events[1].thrown == null
+ assert events[1].source.methodName == 'withMarker'
+ assert events[2].message == 'both y'
+ assert events[2].marker == marker
+ assert events[2].thrown instanceof IllegalArgumentException
+ assert events[2].source.methodName == 'withMarkerAndThrowable'
+ }
+
+ @Test
+ void testStaticLocationWithCompileStatic() {
+ Class clazz = new GroovyClassLoader().parseClass('''
+ @groovy.transform.CompileStatic
+ @groovy.util.logging.Log4j2(staticLocation = true)
+ class LocatedStatic {
+ void run(String who) {
+ log.info("hello $who")
+ log.warn('plain')
+ }
+ }
+ ''', 'LocatedStatic.groovy')
+ clazz.log.addAppender(appender)
+ clazz.log.setLevel(Level.ALL)
+ clazz.newInstance().run('world')
+
+ def events = appender.events
+ assert events*.message == ['hello world', 'plain']
+ assert events*.source*.className == ['LocatedStatic', 'LocatedStatic']
+ assert events*.source*.methodName == ['run', 'run']
+ assert events*.source*.lineNumber == [6, 7]
+ }
+
+ @Test
+ void testStaticLocationOffLeavesCallsAlone() {
+ Class clazz = new GroovyClassLoader().parseClass('''
+ @groovy.util.logging.Log4j2
+ class NotLocated {
+ def run() { log.info('plain') }
+ }
+ ''', 'NotLocated.groovy')
+ assert !clazz.declaredFields.any { it.name.startsWith('$log$loc$') }
+ }
+
+ @Test
+ void testStaticLocationRejectedByStrategyWithoutSupport() {
+ // a Log-family annotation whose strategy does not support
compile-time locations
+ def err =
shouldFail(org.codehaus.groovy.control.MultipleCompilationErrorsException) {
+ new GroovyClassLoader().parseClass('''
+ @groovy.util.logging.Log4j2Test.NoLocationLog(staticLocation =
true)
+ class Unsupported {
+ def run() { log.info('plain') }
+ }
+ ''')
+ }
+ assert err.message.contains('staticLocation is not supported by')
+ }
+
+
@java.lang.annotation.Retention(java.lang.annotation.RetentionPolicy.SOURCE)
+ @java.lang.annotation.Target(java.lang.annotation.ElementType.TYPE)
+
@org.codehaus.groovy.transform.GroovyASTTransformationClass('org.codehaus.groovy.transform.LogASTTransformation')
+ static @interface NoLocationLog {
+ String value() default 'log'
+ String category() default
org.codehaus.groovy.transform.LogASTTransformation.DEFAULT_CATEGORY_NAME
+ String visibilityId() default groovy.transform.Undefined.STRING
+ Class<? extends
org.codehaus.groovy.transform.LogASTTransformation.LoggingStrategy>
loggingStrategy() default NoLocationStrategy
+ boolean staticLocation() default false
+ }
+
+ static class NoLocationStrategy extends Log4j2.Log4j2LoggingStrategy {
+ NoLocationStrategy(GroovyClassLoader loader) { super(loader) }
+ @Override boolean supportsStaticLocation() { false }
+ }
Review Comment:
`NoLocationStrategy` extends `Log4j2LoggingStrategy`, which implements
`StaticLocationCapable`. That makes the transformation treat
`supportsStaticLocation()==false` as a “missing API on compile classpath” case,
even though this test is trying to model a strategy that simply doesn’t support
static locations. Using a delegating strategy that does not implement
`StaticLocationCapable` keeps the scenario and error classification consistent.
##########
src/spec/doc/invokedynamic-support.adoc:
##########
@@ -181,7 +181,12 @@ context.frameworkPackages.addAll([
----
Log4j2:: Log4j2 has no equivalent skip list; its location is always the frame
following the logger's
-own class. Use `@CompileStatic` for the classes whose log locations matter.
+own class. Its answer is a location supplied by the caller, which Groovy's
`@Log4j2` transform can
+provide at compile time: `@Log4j2(staticLocation = true)` rewrites each
logging statement to
+`log.atInfo().withLocation(location).log(...)`, where `location` is a
`StackTraceElement` for the
Review Comment:
The example rewrite hard-codes `atInfo()`, but the transform emits
`at<Level>()` corresponding to the original logging method
(info/warn/error/etc). As written, this can mislead readers into thinking
non-info calls are rewritten to `atInfo()`.
> @Log4j2: staticLocation option to emit compile-time caller locations via
> LogBuilder.withLocation
> ------------------------------------------------------------------------------------------------
>
> Key: GROOVY-12378
> URL: https://issues.apache.org/jira/browse/GROOVY-12378
> Project: Groovy
> Issue Type: Improvement
> Reporter: Paul King
> Assignee: Paul King
> Priority: Major
>
> Follow-up to GROOVY-12354. Logging frameworks locate the caller of a logging
> statement by walking the stack, and every frame Groovy's runtime inserts
> between the statement and the logger makes that answer wrong:
> * on a JVM, whenever a call is dispatched through the metaclass rather than
> linked at the call site: the reflective cold tier when enabled, a dynamic
> method name ({{log."$level"(msg)}}), an explicit {{invokeMethod}};
> * in a GraalVM native image, always: GraalVM's method-handle interpreter
> frames are visible to {{StackWalker}} and stack traces in both dispatch modes.
> {{java.util.logging}} and Logback have package skip lists that cover this
> (documented in the invokedynamic guide). Log4j2 has no equivalent hook, and
> its tracker shows it never will: every wrapper-related report (LOG4J2-555,
> LOG4J2-1028, LOG4J2-2975, discussions #2133 and #2243) is answered with "pass
> the boundary FQCN" or "use {{LogBuilder.withLocation}}", and wrappers are
> discouraged outright. The FQCN boundary cannot express Groovy's case, because
> the runtime frames sit *after* the last logger frame. The other answer fits
> exactly: {{LogBuilder.withLocation(StackTraceElement)}} lets the caller
> supply the location, and Log4j2's own LOG4J2-1449 endorsed compile-time
> locations when Scala asked for them (it stalled only on a garbage-free API
> shape, which Groovy does not need).
> The {{@Log4j2}} AST transform knows the class, method, source file and line
> of every logging statement it rewrites, so it can supply the location at
> compile time.
> h3. Proposal
> Add {{boolean staticLocation() default false}} to
> {{groovy.util.logging.Log4j2}}. When set,
> {{Log4j2LoggingStrategy.wrapLoggingMethodCall}} emits the builder form
> instead of the guarded direct call:
> {code:java}
> // today
> if (log.isInfoEnabled()) log.info(msg)
> // with staticLocation = true
> log.atInfo().withLocation($LOC$3).log(msg)
> {code}
> where {{$LOC$n}} is a {{private static final StackTraceElement}} per call
> site, initialised from the annotated class name, enclosing method name,
> source file name and line of the statement. No allocation or stack walk
> happens at run time; the builder's own level check replaces the
> {{isXxxEnabled}} guard.
> Overload mapping onto {{LogBuilder}}: message and parameterised {{log(String,
> Object...)}} as-is; {{Marker}} first argument via {{withMarker}}; trailing
> {{Throwable}} via {{withThrowable}}; {{Supplier}}, {{Message}},
> {{CharSequence}} and {{Object}} arguments onto the corresponding {{log(...)}}
> overloads. Statements the strategy does not rewrite today (see
> {{LogASTTransformation.usesSimpleMethodArgumentsOnly}}) keep their current
> handling.
> Compatibility: {{LogBuilder}} exists since Log4j2 2.13 (2019);
> {{withLocation(StackTraceElement)}} is an interface default that is a no-op
> for foreign implementations, so it degrades to today's behaviour rather than
> failing. If {{staticLocation = true}} and
> {{org.apache.logging.log4j.LogBuilder}} cannot be resolved on the compile
> classpath, the transform must report a compilation error, not silently emit
> the old shape: an opt-in that does nothing would be worse than none.
> h3. Why opt-in
> The generated code is arguably better for every user: supplying the location
> removes Log4j2's runtime stack walk, the expensive part of any
> {{%C}}/{{%M}}/{{%F}}/{{%L}} layout, and fixes the metaclass-routed shapes on
> the JVM that the GROOVY-12354 default flip does not reach. It is nevertheless
> gated because the call shape changes (tests that mock the logger or inspect
> generated bytecode will notice), because of the 2.13 floor, and because a
> behaviour change to a widely used transform belongs in a point release as an
> option first. Flipping the default, and a {{CompilerConfiguration}} switch so
> a native build can enable it project-wide, are follow-ups once it has seen
> use.
> h3. Scope
> Only statements generated by the transform. A hand-declared logger field
> keeps today's behaviour, for which the documented settings remain the answer.
> {{@Log}}, {{@Slf4j}} and {{@Commons}} have no equivalent API (SLF4J's
> {{CallerBoundaryAware}} is a boundary, not a location) and are out of scope.
> h3. Tests and docs
> * Transform tests: generated shape with and without the attribute; each
> overload mapping; one {{StackTraceElement}} per call site with the
> statement's line, not the class or method line; the compile error when
> {{LogBuilder}} is absent.
> * Behavioural test with {{log4j-core}}: a {{%C.%M(%F:%L)}} layout reports the
> annotated class and statement line when the call is made through
> {{log."$level"(msg)}}, which is wrong without the option on every Groovy
> version.
> * Document the attribute on {{@Log4j2}} and add it to the "Caller location"
> section of the invokedynamic guide, including the native-image subsection, as
> the recommended setting for Log4j2 users there.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
