daniellansun commented on code in PR #2834:
URL: https://github.com/apache/groovy/pull/2834#discussion_r3873675506
##########
src/main/java/org/codehaus/groovy/control/ClassNodeResolver.java:
##########
@@ -304,28 +324,91 @@ private static LookupResult findByClassLoading(final
String name, final Compilat
return tryAsScript(name, compilationUnit, null);
} catch (CompilationFailedException cfe) {
throw new GroovyBugError("The lookup for " + name + " caused a
failed compilation. There should not have been any compilation from this
call.", cfe);
+ } catch (NoClassDefFoundError ncdfe) {
+ return recoverFromNoClassDefFoundError(name, compilationUnit,
loader, ncdfe);
}
- //TODO: The case of a NoClassDefFoundError needs a bit more research;
- // a simple recompilation is not possible it seems. The current class
- // we are searching for is there, so we should mark that somehow.
- // Basically the missing class needs to be completely compiled before
- // we can again search for the current name.
- /*catch (NoClassDefFoundError ncdfe) {
- cachedClasses.put(name,SCRIPT);
- return false;
- }*/
if (cls == null) return null;
- //NOTE: we might return false here even if we found a class,
- // because we want to give a possible script a chance to
- // recompile. This can only be done if the loader was not
- // the instance defining the class.
+ // NOTE: even if we found a class we still give a possible script a
chance
+ // to recompile, but only when this loader was not the instance that
defined it.
ClassNode cn = ClassHelper.make(cls);
if (cls.getClassLoader() != loader) {
return tryAsScript(name, compilationUnit, cn);
}
return new LookupResult(null,cn);
}
+ /**
+ * Recovers from {@link NoClassDefFoundError} thrown while defining or
linking
+ * {@code name}. The class being looked up is present — a referenced type
is not —
+ * so treating the failure as a miss would poison {@link #resolveName} with
+ * {@link #NO_CLASS} and hide bytecode or a groovy source that can still
be used.
+ * <p>
+ * Recovery order:
+ * <ol>
+ * <li>ASM decompilation of matching bytecode, which does not link the
class,
+ * so a missing superclass or interface does not prevent producing a
+ * {@link ClassNode}.</li>
+ * <li>If a {@code .class} resource exists for this path but declares a
+ * different binary name (the JVM {@code defineClass} name check,
also
+ * seen on case-insensitive filesystems), treat the lookup as a miss
and
+ * fall through to script lookup. Detection uses the bytecode name,
not
+ * {@link NoClassDefFoundError} text.</li>
+ * <li>A groovy source of the same name, added to the compilation
queue.</li>
+ * </ol>
+ * If none of those succeed the error is rethrown with {@code name} in the
+ * message so callers see both the class under lookup and the missing type.
+ */
+ private LookupResult recoverFromNoClassDefFoundError(final String name,
final CompilationUnit compilationUnit,
+ final GroovyClassLoader loader, final NoClassDefFoundError ncdfe) {
+ LookupResult decompiled = findDecompiled(name, compilationUnit,
loader, false);
Review Comment:
Thank you — that reading of the recovery path was right, and the patch has
been restructured around it.
### 1. The extra `findDecompiled` in recovery
When `asmResolving` is on we have already parsed the resource. Calling
`findDecompiled` again on the same name was redundant; a hit would have
returned before `loadClass`.
The guard you wondered about is in the caller (`tryAsLoaderClassOrScript`),
not in `findDecompiled`. So when `asmResolving` is off, that first parse never
ran — and that is the GROOVY-12303 case (unlinkable bytecode, class-loader
lookup only). There we still decompile **once**, as a last resort, because
`loadClass` cannot produce a `ClassNode` for a type that failed to link. That
hatch is no longer an unmarked second copy of the happy-path decompile.
`recoverFromNoClassDefFoundError` is gone. The first pass stores a
`ClassFile` (`MATCH` / `MISMATCH` / `ABSENT`, or `null` if ASM was not
attempted). The `NoClassDefFoundError` arm uses that result; it parses only
when the value is `null`.
### 2. `classFileDeclaresDifferentName` belongs on the first parse
Agreed. The bytecode-name check is now part of that single ASM read
(`readClassFile`). A mismatch is no longer collapsed to “not found” and
rediscovered with a second `DecompiledClassNode`.
The check still requires bytecode, so it does not run as a free-standing
decompile when ASM was already skipped — except in the last-resort hatch above,
which is the first parse of that lookup. We do not inspect
`NoClassDefFoundError` text (HotSpot’s `"wrong name"` phrase is not a portable
API).
A mismatch means this resource is not `name`. `loadClass` still runs
afterwards: `getResource` and `defineClass` / `findLoadedClass` are independent
(in-memory class of the right name, case-insensitive `getResource`). If
`loadClass` then throws `NoClassDefFoundError` and we already know the resource
was a mismatch, that is treated as “the requested name never existed”
(`tryAsScript` with no `oldClass`), not as a linkage failure of `name`.
### 3. `tryAsScript(name, cu, null)` is not legal for `NoClassDefFoundError`
Agreed. That was the CNFE contract (`oldClass == null` ⇒ any groovy source
wins, no `isSourceNewer`, no origin check). `NoClassDefFoundError` means some
loader found a class (or bytes it tried to derive). Replacing it with a script
is only legal when that class is from a parent — the same rule as the success
path (`cls.getClassLoader() != loader`) and as the decompile path
(`isFromAnotherClassLoader` then `tryAsScript(name, cu, decompiled)` so
`isSourceNewer` applies).
That is now the only class→script path. Same-loader unlinkable bytecode
keeps the `DecompiledClassNode`. If there is no matching class-file resource,
we cannot get a timestamp or a defining loader, so we rethrow the wrapped
`NoClassDefFoundError` and do not cache `NO_CLASS` — including when a groovy
source of the same name happens to exist.
Thank you again for catching this.
--
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]
