[
https://issues.apache.org/jira/browse/GROOVY-12303?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18108436#comment-18108436
]
ASF GitHub Bot commented on GROOVY-12303:
-----------------------------------------
blackdrag commented on code in PR #2834:
URL: https://github.com/apache/groovy/pull/2834#discussion_r3865601614
##########
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:
I do not understand this step. The name we use here is the one we got the
class loading error for before, but to get here, we already tried decompilation
I thought - Unless decompilation is not enabled, in which case I think calling
this method should not happen (or I overlooked the guard in the method). So I
assume decompiled is always giving null here. The other decompilation in
classFileDeclaresDifferentName as imho one that should be tried initially...
also it ignores the flag to not to decompile - without decompilation the method
cannot do the check it tries to do right now.
And then the change basically boils down to tryAsScript as it is done for a
ClassNotFoundException. Which is not always legal. Here exactly we have to
consider if the class is in the same class loader or not. We can define the
ClassNode as script only if the failing class is from a parent. tryAsScript is
not doing that check... the current check is in tryAsScript -> oldClass!=null
-> isSourceNewer, which does not work if we do not have a class. Which means we
cannot reliably know much about the class unless we look at the class file
resource. If there is no such resource - which does not have to be the case -
then this fails too.
> ClassNodeResolver: NoClassDefFoundError during class-loader lookup aborts
> resolution
> ------------------------------------------------------------------------------------
>
> Key: GROOVY-12303
> URL: https://issues.apache.org/jira/browse/GROOVY-12303
> Project: Groovy
> Issue Type: Bug
> Reporter: Daniel Sun
> Priority: Major
>
> When class-loader lookup is used ({{{}asmResolving{}}} off, or the type
> exists only in memory), {{ClassNodeResolver}} calls
> {{{}GroovyClassLoader.loadClass{}}}. If the requested class *exists* but
> cannot be linked (missing superclass or interface), the JVM throws
> {{{}NoClassDefFoundError{}}}.
> That error used to escape resolution. Compilation aborted with an {{Error}}
> that named the {*}missing dependency{*}, not the type being resolved.
> {{resolveName}} could also cache the name as a miss ({{{}NO_CLASS{}}}), so a
> later successful compile of the dependency would not be retried.
> A TODO in {{findByClassLoading}} has noted this since the 2012 split out of
> {{{}ResolveVisitor{}}}.
> h3. Expected
> * If bytecode for the requested name is still on the class path, decompile
> it (ASM does not link) and continue.
> * Else if a groovy source of the same name is available, add it to the
> compilation queue.
> * Else if a {{.class}} resource exists for that path but declares a
> different binary name (JVM {{defineClass}} name check; also case-insensitive
> filesystems), treat the lookup as a miss. Detect this from the bytecode name,
> not from {{NoClassDefFoundError}} text (HotSpot's {{wrong name}} phrase is
> English-only; OpenJ9 does not use it).
> * Otherwise rethrow {{NoClassDefFoundError}} with the looked-up name in the
> message, and do not cache {{{}NO_CLASS{}}}.
> h3. Actual
> {{NoClassDefFoundError}} propagated out of
> {{{}ClassNodeResolver.findByClassLoading{}}}.
> h3. Reproducer
> Put {{HasDep.class}} (extends a type that is not loadable) on the class path,
> disable ASM resolving, and compile:
> {code:groovy}
> HasDep x = null
> {code}
> This fails with {{NoClassDefFoundError}} for the missing super-type instead
> of resolving {{{}HasDep{}}}.
>
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
