Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Looks into indirect types during compilation #543

Closed
CsCherrYY opened this issue Nov 17, 2022 · 14 comments · Fixed by #2543
Closed

Looks into indirect types during compilation #543

CsCherrYY opened this issue Nov 17, 2022 · 14 comments · Fixed by #2543
Assignees

Comments

@CsCherrYY
Copy link
Contributor

Hello, I know it's an error message with a long history with eclipse:

The type xxx cannot be resolved. It is indirectly referenced from required .class files

There are still a lot of related eclipse issues open: https://bugs.eclipse.org/bugs/show_bug.cgi?id=328057, https://bugs.eclipse.org/bugs/show_bug.cgi?id=508306, https://bugs.eclipse.org/bugs/show_bug.cgi?id=533490, https://bugs.eclipse.org/bugs/show_bug.cgi?id=461074, https://bugs.eclipse.org/bugs/show_bug.cgi?id=545108 and so on.

after investigating, I found that in most cases, if xxx is a type belongs to JDK (e.g., java.lang.Object), the cause is likely the JDK configuration issue. If xxx is a type from external dependency, it looks like ecj has troubing when looking into indirectly types. Recently #327 improves the error massage so that in come cases we can get the exact error type to do troubleshooting. But this error is still exists and keep troubling users.

I can imagine serveral scenarios we will look into/need those indirect types:

Serveral thoughts and questions (please correct me if I'm missing any background :)):

  • I think there is room to improve this behavior, since the above scenarios don't cover all the issues we encounter. Expect the overriding scenario, can we avoid looking for the indirect types?
  • In the above scenarios, is looking into indirect types needed absolutely? e.g., can we get an incomplete member list, and so on.
@CsCherrYY
Copy link
Contributor Author

An example about looking into indirectly types without extending and overriding: https://github.com/Atmosphere/atmosphere, jdt reports

The type org.eclipse.jetty.util.DecoratedObjectFactory cannot be resolved. It is indirectly referenced from required type org.eclipse.jetty.websocket.server.WebSocketServerFactory

in Jetty93AsyncSupportWithWebSocket.java, which imports type WebSocketServerFactory. Type WebSocketServerFactory just imports type org.eclipse.jetty.util.DecoratedObjectFactory, without extending.

@stephan-herrmann
Copy link
Contributor

Improvement in this area largely depends on reproducing examples, as there can be millions of reasons for this infamous error.

When we have an example, I see three possible outcomes

  1. if the type is actually present on the Build Path then the error message might be caused by a bug in lookup. Once we can reproduce such bug, we may look into it with priority.
  2. if the type is absent we may find a reason (which may be unexpected, non-obvious) why the compiler needs to look at that type - no code changes will happen here
  3. only for the remainder (probably the minority of cases) we can try to ignore missing types in more locations (there is already significant machinery to this purpose in place).

I do hope that the added information in the error message will help users to understand the chain of dependencies and correct their build paths for all cases (2).

@CsCherrYY
Copy link
Contributor Author

@stephan-herrmann thanks a lot for the reply.

I do hope that the added information in the error message will help users to understand the chain of dependencies and correct their build paths for all cases (2).

Yeah, in my mentioned case, the added information can show where the indirect type is referenced, that's pretty good :).

For that sample I mentioned in #543 (comment), since we can execute mvn compile successfully via javac, I would imagine that it would be a point we can improve in ecj. I'd like to find if my sample (which is actually absent) is in case 3 (can be ignored as a corner case of the current machinery) or case 2 (the indirect type should be looked), where should I start?

@stephan-herrmann
Copy link
Contributor

... where should I start?

@CsCherrYY to better help you debug the issue it would be good to know how exactly you are able to reproduce the problem: ideally and in the end we'd need a JUnit test. As long as isolating the problem is tricky, you may of course debug the real-life version of the problem. As I don't know how ecj is invoked, the most general hint would be to use remote debugging.

The most obvious breakpoint would then be on ProblemReporter.isClassPathCorrect(..) where the problem is reported. From there you'd work your way backwards, to the cause. First question: what kind of lookup was active when the problem occurred? Was UnresolvedReferenceBinding.resolve(..) active on the stack? For a basic understanding of this class see also https://wiki.eclipse.org/JDT_Core_Programmer_Guide/ECJ/Bindings#Main_Kinds_of_Bindings - it plays a central role in delaying type lookup as much as possible.

If theory tells, that the type should be found, then it's likely that LookupEnvironment.getType(char[][], ModuleBinding) had a problem. While stepping through this method - and deeper into its innards, do you see a reason why that type was not found?

OTOH, if you're more interested in finding why the compiler needed to resolve that type, tell us the call stack in which the problem was detected, and which program elements where involved.

@CsCherrYY
Copy link
Contributor Author

Well, I think I have found one of the causes from the example projects, which is about anonymous class.

Given sample project: https://github.com/Atmosphere/atmosphere, we can get the mentioned error about type org.eclipse.jetty.util.DecoratedObjectFactory(DecoratedObjectFactory in the following), referenced by direct type org.eclipse.jetty.websocket.server.WebSocketServerFactory(WebSocketServerFactory in the following, from external libraries), which can be found in some classes in package org.atmosphere.container.

First of all, the indirect type org.eclipse.jetty.util.DecoratedObjectFactory is indeedly not present at the project classpath.

However, there are serveral snippets creating an anonymous class, whose supertype is WebSocketServerFactory: https://github.com/Atmosphere/atmosphere/blob/4228070f3f7304c88549469db0f16ddead98be02/modules/cpr/src/main/java/org/atmosphere/container/Jetty93AsyncSupportWithWebSocket.java#L41-L48, when resolving the anonymous class, the inherit methods are going to be looked (although they are not invoked), so the error appears.

We can found the call stack shows the problem occurs when looking up the inherit methods from anonymous class:
image
besides, as mentioned, UnresolvedReferenceBinding.resolve() is active on the stack.

One interesting thing is that, the direct type WebSocketServerFactory is also referenced but not used to construct an anonymous class in another place, where ecj works well, no error is reported: https://github.com/Atmosphere/atmosphere/blob/4228070f3f7304c88549469db0f16ddead98be02/modules/cpr/src/main/java/org/atmosphere/container/AbstractJetty9AsyncSupportWithWebSocket.java#L146

My point is that now we will look into inherit methods in anonymous class, even though they will not be invoked. If we're invoking a method whose return type is an indirectly type not present in the classpath, we should report error indeed, but if those methods are not invoked, things can be improved here?

@stephan-herrmann
Copy link
Contributor

@srikanth-sankaran could you join me in a thought experiment regarding this ancient, recurring problem?

Background:

The oldest idea I remember was to ensure that the transitive closure of all referenced classes is indeed visible to the compiler. But some transitive dependencies may be private to another dependency / library (read: not re-exported). Hence it should be visible only for that library, not for the client code, for which the team around Phillipe coined the term "forbhidden" 😄 - Still nobody actually put this idea into code, perhaps because we have no general solution to actually retrieve all transitive dependencies (since JDT is agnostic towards all build systems).

Next we have tried to avoid resolving a few types here and there, but nothing systematic came from that.

Every other bug report in this area (correctly) states that for this particular program that particular lookup was not necessary, but it seems each program would need its own strategy of what to resolve and more importantly: when. But we learned the hard way that toying with the (order of) our systematic phases of compilation is very risky.

New? Idea

Could we, instead of such hard core changes, "simply" defer the isClasspathCorrect() error until the thing that needs the missing type is needed itself? We already have MissingTypeBinding to represent the result of failed lookup. Now if any method has a missing type in its signature, then I suggest to

  • record the method selector in the declaring class as tainted but continue without reporting an error
  • only when methods of that selector come into focus report the problem:
    • if a sub type has a method with the same selector that needs to be checked for any conflicts (MethodVerifier)
    • if an invocation with that selector happens on a type having the problematic type in its hierarchy

Please see that currently the drastic abort is done because at the point where it happens we are analysing binary classes and hence cannot blame any particular source code. If not knowing whom to blame is the problem, then perhaps waiting until we have a location to blame would be the natural solution, no?

Since overloading implies we don't know which exact method in a given type hierarchy is needed when, I feel that recording the selector is the right level of caution. You can't call x.foo() if somewhere in the hierarchy there is a method foo() with missing types. Similar for possible overrides of methods with missing types, again per selector. (if we are going crazy, we'll record selector and arity for even more precision, but beware of variable arity messing up the logic).

@srikanth-sankaran
Copy link
Contributor

@srikanth-sankaran could you join me in a thought experiment regarding this ancient, recurring problem?

I will. Would love to see this vexing problem get some solution. Give me a couple of days please.

eclipse-egit-bot pushed a commit to eclipse-egit/egit that referenced this issue May 24, 2024
Include slf4j.api and org.apache.sshd.osgi on the build classpath to
work around a JDT bug[1] in Eclipse newer than 2023-12.

[1] eclipse-jdt/eclipse.jdt.core#543

Bug: egit-35
Change-Id: Icac5492f2285d6563782ee4f3fe0bf6e08c1357e
Signed-off-by: Thomas Wolf <twolf@apache.org>
@srikanth-sankaran
Copy link
Contributor

Hi @stephan-herrmann I think your outline looks promising enough - I don't have any holes to poke, why don't we prototype this and see - perhaps the new behavior could come under a preference and we can selectively ask people who complain about this problem to try the option and report feedback.

Folks impacted by #1842 could be the initial testers -

@chrisrueger
Copy link

chrisrueger commented May 31, 2024

Folks impacted by #1842 could be the initial testers -

Glad to help testing with my reported problem.

@stephan-herrmann stephan-herrmann self-assigned this Jun 6, 2024
stephan-herrmann added a commit to stephan-herrmann/eclipse.jdt.core that referenced this issue Jun 9, 2024
fixes eclipse-jdt#543

+ tolerate missing types during BTB.resolveTypesFor(MethodBinding)
+ new parameterCompatibilityLevel NEED_MISSING_TYPE
+ when NEED_MISSING_TYPE is encountered during resolution
  answer ProblemMethodBinding(ProblemReason.MissingTypeInSignature)
+ for varargs invocation try to discern if a missing type is relevant

Tests:
+ adjust expected secondary errors (neither is better than the other)

TODO:
+ check all call paths into parameterCompatibilityLevel()
+ fields
@stephan-herrmann
Copy link
Contributor

We can found the call stack shows the problem occurs when looking up the inherit methods from anonymous class:

Good news: BinaryTypeBinding.resolveTypesFor(MethodBinding) features prominently in this stack and is indeed the location where work in #2543 plugs into.

stephan-herrmann added a commit to stephan-herrmann/eclipse.jdt.core that referenced this issue Jun 22, 2024
fixes eclipse-jdt#543

+ tolerate missing types during BTB.resolveTypesFor(MethodBinding)
+ new parameterCompatibilityLevel NEED_MISSING_TYPE
+ when NEED_MISSING_TYPE is encountered during resolution
  answer ProblemMethodBinding(ProblemReason.MissingTypeInSignature)
+ for varargs invocation try to discern if a missing type is relevant

Tests:
+ adjust expected secondary errors (neither is better than the other)

TODO:
+ check all call paths into parameterCompatibilityLevel()
+ fields
@srikanth-sankaran
Copy link
Contributor

srikanth-sankaran commented Jul 10, 2024

Well, I think I have found one of the causes from the example projects, which is about anonymous class.

Given sample project: https://github.com/Atmosphere/atmosphere, we can get the mentioned error about type

Has the PR been validated against this sample project ? Either by you @stephan-herrmann or by pulling in @CsCherrYY for help ?? I am not sure this would help @chrisrueger's report - As @stephan-herrmann mentioned elsewhere Improvement in this area largely depends on reproducing examples, as there can be millions of reasons for this infamous error.

@chrisrueger
Copy link

@srikanth-sankaran just want to mention here too, that i got my problem solved. See #1842 (comment)

stephan-herrmann added a commit that referenced this issue Jul 11, 2024
+ tolerate missing types during BTB.resolveTypesFor(MethodBinding)
+ new parameterCompatibilityLevel NEED_MISSING_TYPE
+ when NEED_MISSING_TYPE is encountered during resolution
  answer ProblemMethodBinding(ProblemReason.MissingTypeInSignature)
+ for varargs invocation try to discern if a missing type is relevant
+ detect when method return type can be ignored
+ detect missing types during type inference
  - don't let constraint with missing type fail type inference
  - prefer that candidate during overload resolution
+ let more locations propagate HasMissingType
+ improve collectMissingTypes() & protect against infinite recursion

Tests:
+ adjust expected secondary errors (neither is better than the other)
+ Java50Tests pointed to incomplete impl. at compliance below 1.8
  => restrict new policy to 1.8+
+ DependencyTests.testMissingClassFile()
  + bump test to 1.8 and let it include positive & negative cases
+ MultiProjectTests.test461074_error() split to 1.5 and 1.8 variants:
  + no change at 1.5
  + no longer an error at 1.8+
+ improved errors in GenericsRegressionTest_1_8.testBug525580*() et al
+ fix JCL_LIB -> JCL18_LIB in ImportRewrite18Test
+ JavaSearchBugsTest2:
   - fix tests by adding required import
   - create one test variant with missing import and POTENTIAL_MATCH

fixes #543
robstryker pushed a commit to robstryker/eclipse.jdt.core that referenced this issue Jul 18, 2024
+ tolerate missing types during BTB.resolveTypesFor(MethodBinding)
+ new parameterCompatibilityLevel NEED_MISSING_TYPE
+ when NEED_MISSING_TYPE is encountered during resolution
  answer ProblemMethodBinding(ProblemReason.MissingTypeInSignature)
+ for varargs invocation try to discern if a missing type is relevant
+ detect when method return type can be ignored
+ detect missing types during type inference
  - don't let constraint with missing type fail type inference
  - prefer that candidate during overload resolution
+ let more locations propagate HasMissingType
+ improve collectMissingTypes() & protect against infinite recursion

Tests:
+ adjust expected secondary errors (neither is better than the other)
+ Java50Tests pointed to incomplete impl. at compliance below 1.8
  => restrict new policy to 1.8+
+ DependencyTests.testMissingClassFile()
  + bump test to 1.8 and let it include positive & negative cases
+ MultiProjectTests.test461074_error() split to 1.5 and 1.8 variants:
  + no change at 1.5
  + no longer an error at 1.8+
+ improved errors in GenericsRegressionTest_1_8.testBug525580*() et al
+ fix JCL_LIB -> JCL18_LIB in ImportRewrite18Test
+ JavaSearchBugsTest2:
   - fix tests by adding required import
   - create one test variant with missing import and POTENTIAL_MATCH

fixes eclipse-jdt#543
@iloveeclipse
Copy link
Member

Today I saw this error in my default SDK workspace, using latest nightly 4.33 M2 build.
Error was about missing java.util.stream.IntStream and first appeared during refactoring in jdt.ui.tests project, that references Java 17. I've rebuild the project and then error appeared on other, fully unrelated SDK projects, 4 or 5 of them. I couldn't fix it by clean builds and so restarted IDE. After that problem disappeared.
I never observed such behavior before, I assume it could be effect from recent changes here.
I can't reproduce that. No errors in the log.

@stephan-herrmann
Copy link
Contributor

Today I saw this error in my default SDK workspace, using latest nightly 4.33 M2 build. Error was about missing java.util.stream.IntStream

Ups, I didn't steal java.util.stream.IntStream, honestly!

What exactly did the error say?

and first appeared during refactoring in jdt.ui.tests project, that references Java 17. I've rebuild the project and then error appeared on other, fully unrelated SDK projects, 4 or 5 of them. I couldn't fix it by clean builds and so restarted IDE. After that problem disappeared. I never observed such behavior before, I assume it could be effect from recent changes here. I can't reproduce that. No errors in the log.

This issue avoids reporting missing types, when those are not really needed during compilation.

There is only one part of the fix, where detection of missing types has been made more complete, notably when a missing type is referenced from the bound of a type variable. This surfaced, e.g., in eclipse/xtext#3102, but there it affects a test that intentionally works without java libraries.

Nothing of this causes a type to be missing, all this is only about how we react after a type is found to be missing.

No, what you describe sounds like some other flakiness in reading system libraries.

gayanper pushed a commit to gayanper/eclipse.jdt.core that referenced this issue Sep 7, 2024
+ tolerate missing types during BTB.resolveTypesFor(MethodBinding)
+ new parameterCompatibilityLevel NEED_MISSING_TYPE
+ when NEED_MISSING_TYPE is encountered during resolution
  answer ProblemMethodBinding(ProblemReason.MissingTypeInSignature)
+ for varargs invocation try to discern if a missing type is relevant
+ detect when method return type can be ignored
+ detect missing types during type inference
  - don't let constraint with missing type fail type inference
  - prefer that candidate during overload resolution
+ let more locations propagate HasMissingType
+ improve collectMissingTypes() & protect against infinite recursion

Tests:
+ adjust expected secondary errors (neither is better than the other)
+ Java50Tests pointed to incomplete impl. at compliance below 1.8
  => restrict new policy to 1.8+
+ DependencyTests.testMissingClassFile()
  + bump test to 1.8 and let it include positive & negative cases
+ MultiProjectTests.test461074_error() split to 1.5 and 1.8 variants:
  + no change at 1.5
  + no longer an error at 1.8+
+ improved errors in GenericsRegressionTest_1_8.testBug525580*() et al
+ fix JCL_LIB -> JCL18_LIB in ImportRewrite18Test
+ JavaSearchBugsTest2:
   - fix tests by adding required import
   - create one test variant with missing import and POTENTIAL_MATCH

fixes eclipse-jdt#543
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants