When returntype is Object and class extends another type one gets error "The type X cannot be resolved. It is indirectly referenced from required type"

[laeubi]

Problem

I today found a case where I think ECJ can avoid emitting The type X cannot be resolved. It is indirectly referenced from required type

Take this example project:

https://github.com/eclipse-equinox/equinox/tree/master/bundles/org.eclipse.equinox.jsp.jasper.registry

now remove from the manifest these imports:
https://github.com/eclipse-equinox/equinox/blob/2fd1eb7b348f3a3925b60f0c33dafeb6feae4b9a/bundles/org.eclipse.equinox.jsp.jasper.registry/META-INF/MANIFEST.MF#L13-L14

And you will get this error then

Description	Resource	Path	Location	Type
The type javax.servlet.http.HttpServlet cannot be resolved. It is indirectly referenced from required type org.eclipse.equinox.jsp.jasper.JspServlet	JSPFactory.java	/org.eclipse.equinox.jsp.jasper.registry/src/org/eclipse/equinox/jsp/jasper/registry	line 1	Java Problem

One would argue that this is unavoidable because JspServlet extends HttpServlet so the compiler must know the type., but interestingly if you change this line

https://github.com/eclipse-equinox/equinox/blob/2fd1eb7b348f3a3925b60f0c33dafeb6feae4b9a/bundles/org.eclipse.equinox.jsp.jasper.registry/src/org/eclipse/equinox/jsp/jasper/registry/JSPFactory.java#L58

from public Object create() to public JspServlet create() it works! This shows another issue here, the error is actually shown at the top of the file but the error obviously is related to the return type of the method, so I would have expected it to be shown at line 58!

Current behavior

As far as I understand the reason for this is that ECJ wants to check that the return type is compatible, so it needs to check JspServlet, all its super classes and implemented interfaces and then discovers that it can not see the extended type HttpServlet.

Expected behavior

As every class must extend Object there is no need to further check if return types are compatible. It would even valuable to first check if the Type itself already implements any compatible class or interface already, but I do not know if that maybe already happens.

Impact

Currently users are forced to import unused packages. Also when writing code one sometimes hit this issue and it is very annoying to then need to find out what to do to make the compiler happy, and even PDEs Quick-fix currently can not help here as there is no trace anywhere that javax.servlet.http is needed at all.

So summary:

• Error is shown at wrong line
• the type HttpServlet is actually not needed here as regardless of what this type looks like it must extend Object and is compatible.

[stephan-herrmann]

Currently users are forced to import unused packages. Also when writing code one sometimes hit this issue and it is very annoying to then need to find out what to do to make the compiler happy, and even PDEs Quick-fix currently can not help here as there is no trace anywhere that javax.servlet.http is needed at all.

It is not at all clear what should be considered as "unused" or "needed".

Certainly, at runtime you cannot instantiate a class if any of its super classes cannot be loaded, right?

Generally, build systems manage dependencies such that compilation works in a context that faithfully mirrors the situation at runtime. I would always expect that classes needed at runtime must also be present in the transitive closure of compile-time dependencies. Otherwise there is no guarantee that the compiler can check validity of the program.

In JDT we have made some investment in reducing the compiler's traversal of required types to avoid the error at hand, wherever reasonably possible. Obviously, "reasonably" is not a clear-cut specification in this context.

This is said to adjust expectations:

• Users should always first reconsider the build configuration (or their build tool), whether compile-time and runtime contexts really match.
• Secondly, ecj will try to avoid lookup of types that provably do not impact type checking of the current source file. There is no clear specification of what can be done here, as inevitably this effort relates to the actual implementation of ecj.

[stephan-herrmann]

That said (prev. comment), I can give two hints that may help mitigate the specific issue here:

• In class ProblemTypeAndMethodTest you will find plenty of examples how presence/absence of the error at hand can be checked. We need some more tests in this category to work on any improvement.
• Considering that analysis of "unchecked conversion" has a finger in the pie, I can see that org.eclipse.jdt.internal.compiler.lookup.TypeBinding.needsUncheckedConversion(TypeBinding) could easily detect more cases of false even before calling currentType.findSuperTypeOriginatingFrom(targetType). This would significantly reduce unnecessary type lookup.

[laeubi]

@stephan-herrmann thanks for looking into this, please be aware that I likely have not enough understanding on how JDT works here internally nor about specific specification details.

So in fact this might be a better task for a skilled JDT developer, but I also understand that JDT developers are quite busy and tried to get at least the ball rolling as this is a really annoying (and recurring) problem in PDE due to how OSGi classloading works.

So here some more clarifications before I try to figure out a better approach and hopefully you can better understand the issue I try to solve:

It is not at all clear what should be considered as "unused" or "needed".

In OSGi each bundle has an own classloader, these classloaders still need to work together because of one can define things that are exported (usually a java package) and things that are imported (e.g. a package or another bundle what is slightly different but unimportant for the matter here).

These imports produce so called wires between bundle and PDE tries to reflect these wires through the classpath using access restrictions at compile time.

So an unused import (or unneeded) is one that is actually not required at runtime, that means it can be removed without creating any ClassNotFoundException or ClassCastException at runtime.

Certainly, at runtime you cannot instantiate a class if any of its super classes cannot be loaded, right?

The case here now is that we have (at least) three bundles

• A (the API) exporting the package javax.servlet.http with interface I
• P (the provider) that implements a type T implementing I from A and exports a package org.eclipse.equinox.jsp.jasper
• C (the consumer) that uses a type from P

Now P of course needs to import from A the API and java can happily load I and C needs to import P to load T through the classloader of P (that delegates to A for the supertype I.

Now the question is do need C import A and there we came to this interesting situation where the runtime and the compile time path diverges:

1. If C would have any parameter type or local variable or field of I then it must import A (to ensure classspace consistency) and you will find e.g. references to the type in compiled class files.
2. If C only references T and never calls any method on it that overrides or contains parameter from I then is must not import A as it only ever will load anything through P and you will not find any reference to the type in any compiled classfile.

So for the referenced example what happens is:

• C only ever calls the constructor of T
• it returns that instance from a method with a return type of Object
• the classloader of C would never need to load I as only P is used for loading and therefore does not need to import A

Now if we remove A from the classpath of C the following happens:

• The compiler is perfectly happy with construct the Object T
• If I want to return that object from a method that itself returns Object type it complains because it can not check the unchecked conversion because now it wants to check all types in the class hierarchy but can't find the type extended by T
• If I change the return type to T the compiler is happy.

So this does not work:

Object testMe() {
   return new T();
}

this does work:

T testMe() {
   return new T();
}

so my naive interpretation of the situation would be that every type is actually convertible into Object and the check for unchecked conversion is not needed if we know the resulting type is Object and not the full type hierarchy must be known.

[stephan-herrmann]

@stephan-herrmann thanks for looking into this, please be aware that I likely have not enough understanding on how JDT works here internally nor about specific specification details.

So in fact this might be a better task for a skilled JDT developer, but I also understand that JDT developers are quite busy and tried to get at least the ball rolling as this is a really annoying (and recurring) problem in PDE due to how OSGi classloading works.

So here some more clarifications before I try to figure out a better approach and hopefully you can better understand the issue I try to solve:

It is not at all clear what should be considered as "unused" or "needed".

In OSGi each bundle has an own classloader, these classloaders still need to work together because of one can define things that are exported (usually a java package) and things that are imported (e.g. a package or another bundle what is slightly different but unimportant for the matter here).

These imports produce so called wires between bundle and PDE tries to reflect these wires through the classpath using access restrictions at compile time.

So an unused import (or unneeded) is one that is actually not required at runtime, that means it can be removed without creating any ClassNotFoundException or ClassCastException at runtime.

Sure, so if the package import is not needed OSGi-wise, then we could ask if PDE provides to the compiler a configuration that is a good match for the runtime situation. If a class is visible via an OSGi wire, then hiding it from the compiler is not a perfect match.

I recall discussions about whether PDE should populate the build path (a) with only direct dependencies, (b) with direct + re-exported, or (c) with all transitive dependencies. Option (c) would certainly expose classes that are not visible by OSGi rules, but here the mechanism of access rules already provides the necessary distinction between visibility and accessibility (i.e., not every class that can be found via the name environment can be legally accessed from the current project). I believe PDE already uses access rules, e.g., to represent x-internal and x-friends directives, but I don't know the full details of this mapping.

[stephan-herrmann]

Certainly, at runtime you cannot instantiate a class if any of its super classes cannot be loaded, right?

The case here now is that we have (at least) three bundles

• A (the API) exporting the package javax.servlet.http with interface I

• P (the provider) that implements a type T implementing I from A and exports a package org.eclipse.equinox.jsp.jasper

• C (the consumer) that uses a type from P

Now P of course needs to import from A the API and java can happily load I and C needs to import P to load T through the classloader of P (that delegates to A for the supertype I.

Now the question is do need C import A and there we came to this interesting situation where the runtime and the compile time path diverges:

1. If C would have any parameter type or local variable or field of I then it must import A (to ensure classspace consistency) and you will find e.g. references to the type in compiled class files.

2. If C only references T and never calls any method on it that overrides or contains parameter from I then is must not import A as it only ever will load anything through P and you will not find any reference to the type in any compiled classfile.

First, the program needs no explicit type reference (from local, field or method parameter) to invoke new T().foo().

Secondly, (welcome to the wonderful world of Java), if you invoke t.foo(x), where x has some type X, then even if you find foo(Y) in T where X is compatible with Y, you cannot be sure that this is indeed the method being called. You need to consider all inherited methods searching for the best matching overload. If you select the wrong overload, you might assume a wrong return type of that method, i.e., if the compiler would think Z z = t.foo(x) is valid code, an inherited method Other foo(X) would cause a type error. This error must be reported by the compiler so it needs to consider inherited methods as soon as any method of T is invoked (unless the shape of the method rules out any overloading, like ()).

(To be exact: if the compiler does not see the best matching overload it will create a call to the one method it can see, thus bypassing the type error, but producing byte code that violates JLS. The type error may surface in the future and out of the blue, should at some later point the supertype become visible -- use your imagination to think of other cruel effects of this problem).

This significantly reduces the set of programs where a class T can be referenced without inspecting its supertype hierarchy.

So far I didn't consider the specifics of your example. Just illustrating that the need to inspect super types is more wide spread than one might think looking from a distance. And this need is not subject to personal preferences, it is required for correctness.

[stephan-herrmann]

Now, let's talk specific:

So for the referenced example what happens is:

• C only ever calls the constructor of T

OK, calling a constructor doesn't require inspecting the supertype hierarchy (thankfully, even overloaded constructors only need to be searched in the exact class).

• it returns that instance from a method with a return type of Object

Let's try to abstract away the return. What we need to check: is the expression new T() compatible with type Object, and: does this compatibility involve unchecked conversion.

BINGO: both checks can be performed without inspecting supertypes (because there's one supertype that is known a priori: Object).

So it is legitimate to fine tune the implementation such that it recognizes that no super type inspection is needed to assert compatibility and presence/absence of unchecked conversion. Ergo, this particular case can be improved indeed.

And when comparing the implementation in question against JLS, we should be able to avoid supertype lookup in more cases than just the one being discussed here.