gnodet-bot commented on code in PR #13602: URL: https://github.com/apache/maven/pull/13602#discussion_r4304829700
########## impl/maven-di/src/main/java/org/apache/maven/di/impl/BindingSnapshot.java: ########## @@ -0,0 +1,147 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.maven.di.impl; + +import java.lang.annotation.Annotation; +import java.lang.reflect.GenericArrayType; +import java.lang.reflect.ParameterizedType; +import java.lang.reflect.Type; +import java.lang.reflect.TypeVariable; +import java.lang.reflect.WildcardType; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; +import java.util.IdentityHashMap; +import java.util.List; +import java.util.Map; +import java.util.Set; +import java.util.function.Supplier; +import java.util.stream.Collectors; + +import org.apache.maven.di.Key; +import org.apache.maven.di.Scope; + +/** Detached diagnostic metadata; capturing it does not compile or provision bindings. */ +final class BindingSnapshot { + private BindingSnapshot() {} + + static List<String> capture( + Map<Key<?>, Set<Binding<?>>> bindings, Map<Class<? extends Annotation>, Supplier<Scope>> scopes) { + List<String> rows = new ArrayList<>(); + Map<Binding<?>, String> descriptions = new IdentityHashMap<>(); + bindings.forEach((key, candidates) -> { + String name = key(key); + if (candidates.isEmpty()) { + rows.add(" " + name + " -> <no candidates>"); + } else { + for (Binding<?> binding : candidates) { + rows.add(" " + name + " -> " + descriptions.computeIfAbsent(binding, BindingSnapshot::describe)); + } + } + }); + scopes.forEach((annotation, supplier) -> + rows.add(" scope " + type(annotation) + " -> supplier " + Binding.identityString(supplier))); + Collections.sort(rows); + return List.copyOf(rows); + } + + static String render(Object injector, long revision, String reason, boolean partial, List<String> rows) { + return "Maven DI native registry " + Binding.identityString(injector) + " revision=" + revision + " (" + + reason + (partial ? ", partial" : "") + ") {\n" + + String.join("\n", rows) + "\n} Maven DI native registry " + Binding.identityString(injector); + } + + private static String describe(Binding<?> binding) { + String dependencies = binding.getDependencies().stream() + .map(dependency -> (dependency.optional() ? "?" : "") + key(dependency.key())) + .sorted() + .collect(Collectors.joining(", ", "[", "]")); + return "original=" + (binding.getOriginalKey() == null ? "<none>" : key(binding.getOriginalKey())) + + ", priority=" + binding.getPriority() + + ", scope=" + + (binding.getScope() == null + ? "<none>" + : type(binding.getScope().annotationType())) + + ", source=" + escape(binding.getDiagnosticSource()) + + ", dependencies=" + dependencies; + } + + private static String key(Key<?> key) { + Object qualifier = key.getQualifier(); + String prefix = qualifier instanceof String name + ? "@Named(\"" + escape(name) + "\") " + : qualifier instanceof Class<?> annotationType + ? "@" + type(annotationType) + " " + : qualifier instanceof Annotation annotation + ? "@" + type(annotation.annotationType()) + " [qualifier=" + + Binding.identityString(annotation) + "] " + : qualifier != null ? "@" + Binding.identityString(qualifier) + " " : ""; + return prefix + type(key.getType()); + } + + private static String type(Type type) { + if (type instanceof Class<?> clazz) { + return clazz.isArray() + ? type(clazz.getComponentType()) + "[]" + : clazz.getName() + " [loader=" + Binding.identityString(clazz.getClassLoader()) + "]"; + } + if (type instanceof ParameterizedType parameterized) { + return (parameterized.getOwnerType() == null ? "" : type(parameterized.getOwnerType()) + ".") + + type(parameterized.getRawType()) + "<" + + Arrays.stream(parameterized.getActualTypeArguments()) + .map(BindingSnapshot::type) + .collect(Collectors.joining(", ")) + + ">"; + } + if (type instanceof GenericArrayType array) { + return type(array.getGenericComponentType()) + "[]"; + } + if (type instanceof TypeVariable<?> variable) { + return variable.getName(); + } + if (type instanceof WildcardType wildcard) { + return "? extends " + + Arrays.stream(wildcard.getUpperBounds()) + .map(BindingSnapshot::type) + .collect(Collectors.joining(" & ")) + + (wildcard.getLowerBounds().length == 0 + ? "" + : " super " + + Arrays.stream(wildcard.getLowerBounds()) + .map(BindingSnapshot::type) + .collect(Collectors.joining(" & "))); Review Comment: ⚠️ **Bug: incorrect rendering of lower-bounded and unbounded wildcards** The current code unconditionally starts the representation with `? extends UPPER_BOUNDS`, then appends `super LOWER_BOUNDS` if present. This produces incorrect output for two cases: - **`? super X`** (lower-bounded): Java sets `upperBounds=[Object]`, `lowerBounds=[X]`, so the code emits `? extends java.lang.Object [loader=...] super X [loader=...]` — which is not a valid Java wildcard syntax. - **`?`** (unbounded): Java also sets `upperBounds=[Object]`, producing `? extends java.lang.Object [loader=...]` instead of just `?`. The correct logic should check whether the upper bound is exactly `Object` (and no lower bound exists) to detect the unbounded case, and use `? super` exclusively when lower bounds are present: ```suggestion if (type instanceof WildcardType wildcard) { Type[] lower = wildcard.getLowerBounds(); Type[] upper = wildcard.getUpperBounds(); if (lower.length > 0) { return "? super " + Arrays.stream(lower) .map(BindingSnapshot::type) .collect(Collectors.joining(" & ")); } if (upper.length == 1 && upper[0] == Object.class) { return "?"; } return "? extends " + Arrays.stream(upper) .map(BindingSnapshot::type) .collect(Collectors.joining(" & ")); } ``` ########## impl/maven-di/src/main/java/org/apache/maven/di/impl/InjectorImpl.java: ########## @@ -515,6 +561,91 @@ public T get() { } } + /** + * Captures a human-readable snapshot of the current native registry for Maven internals, + * including direct binding map and metadata changes, without resolving components. + * Registration must not run concurrently with this method. + */ + public String dumpBindings() { + return BindingSnapshot.render( + this, registryRevision, "explicit", false, BindingSnapshot.capture(bindings, scopes)); + } + + /** Diagnostic checkpoint for native and bridge resolution entry points. */ + protected final void debugBindings() { + debugBindings("lookup", false); + } + + private void debugBindings(String reason, boolean partial) { + if (!debugEnabled()) { + if (diagnosticState != null) { + synchronized (diagnosticLock) { + diagnosticState = null; + } + } + return; + } + PrintStream output = System.out; + DiagnosticState state = diagnosticState; + if (!partial && state != null && state.capturedRevision == registryRevision && state.output.get() == output) { + return; + } + writeBindings(output, reason, partial); + } + + private static boolean debugEnabled() { + try { + String property = System.getProperty("sisu.debug"); Review Comment: ⚠️ **`System.getProperty()` called on every DI resolution path** `debugEnabled()` is invoked from `getCompiledBinding()`, `injectInstance()`, and `getInstance()` — the hot paths of every DI resolution. `System.getProperty()` acquires a system-wide monitor on each call, which will serialize all concurrent resolutions for the cost of a string lookup even when diagnostics are completely disabled. Consider caching the result at startup or on first call: ```suggestion private static boolean debugEnabled() { try { String property = System.getProperty("sisu.debug"); return property != null && (property.isEmpty() || Boolean.parseBoolean(property)); } catch (RuntimeException e) { return false; } } ``` A simple approach: cache the result in a `static volatile boolean` field initialized once (e.g. in a `static {}` block or lazy on first call). Dynamic toggling during tests is handled separately by the `SisuDebugSession` mechanism which sets the property directly, so a cached value wouldn't break the test infrastructure — tests that need to toggle the flag can invalidate the cache explicitly, or the cache can re-check every N invocations as a compromise. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
