Looks into indirect types during compilation

[stephan-herrmann]

fixes #543

• tolerate missing types during BTB.resolveTypesFor(MethodBinding)
• new parameterCompatibilityLevel NEEDS_MISSING_TYPE
• when NEEDS_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)

Solution outline:

• we no longer require all types in all method signatures of binary types to be present
• if a method is never touched during resolution of any MessageSend, then we tolerate all its references to missing types
• if a method is touched during overload resolution, but it can be ruled out based only on arity, then references to missing types can still be ignored.
• if a varargs type is missing, but the invocation does not pass any varargs value, then this missing type can be ignored
• if a method is selected during resolution but has a missing return type, absence of that return type can be ignored if the invocation is an expression statement (return value is ignored).

In short: we want to ignore all missing types that have no impact on invocation resolution, and we want to ignore missing return types if the return value is ignored.

In case invocation resolution involves type inference, we proceed like this:

• don't let a constraint with missing type fail the inference
• if any constraint with no missing types reduces to FALSE then it's safe to ignore any missing types seen during inference, inference failure is a definite and sufficient answer here.
• if inference succeeds after ignoring a constraint with missing types, then the candidate method is reported with ProblemReasons.MissingTypeInSignature, in order to escalate this as an error

TODO:

• check all call paths into parameterCompatibilityLevel() ✔️
• fields --> outsourced to Tolerate missing types in more positions? #2658

[stephan-herrmann]

from #543:

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.

Actually, we already have much of what is needed to ignore missing types, specifically flag LookupEnvironment.mayTolerateMissingType was added for this exact purpose.

So rather than inventing much new, this change basically admits missing types throughout BinaryTypeBinding.resolveTypesFor(MethodBinding) using the above flag. We already set TagBits.HasMissingType as needed (on types and on the MethodBinding).

Now we need to make sure that any attempt to "use" a binary method with missing types is correctly detected, resulting in a proper error message, whereas missing types in irrelevant positions are simply ignored.

Methods with a missing return type are already handled in MessageSend.resolveType() where we trigger problemReporter().missingTypeInMethod().

For any missing parameter types I directly hooked into where types are used for applicability tests: Scope.parameterCompatibilityLevel(TypeBinding, TypeBinding, LookupEnvironment, boolean, MethodBinding). When compatibility is checked against a missing type we return a new code NEEDS_MISSING_TYPE, such that callers can take appropriate actions.

Vararg invocations are already handled by the above, but inside MessageSend.resolveType() I added another tweak that detects when a missing varargs type is not used by the specific invocation (assuming that any potential ambiguity was already ruled out before we reach here).

TODOS:

• add a test for a missing return type ✔️
• check if all call paths into Scope.parameterCompatibilityLevel() effectively handle NEEDS_MISSING_TYPE ✔️
• consider Scope.parameterCompatibilityLevel(TypeBinding, TypeBinding) as used for lambdas and reference expressions) -- not relevant here
• outsourced to Tolerate missing types in more positions? #2658 :
  • include fields?
  • search for any other reasons for resolving indirectly referenced types

[stephan-herrmann]

check if all call paths into Scope.parameterCompatibilityLevel() effectively handle NEEDS_MISSING_TYPE

parameterCompatibilityLevel(TypeBinding, TypeBinding, LookupEnvironment, boolean, MethodBinding)

• parameterCompatibilityLevel(MethodBinding, TypeBinding[], boolean)
  • 4 calls, if NEEDS_MISSING_TYPE is seen, this value can only be overridden by NOT_COMPATIBLE, otherwise it is returned to the caller
  • parameterCompatibilityLevel(MethodBinding, TypeBinding[])
    • InferenceContext18.getInferenceKind(MethodBinding, TypeBinding[])
      • NEEDS_MISSING_TYPE is translated to CHECK_LOOSE to ensure this method is seen by first phase of inference
  • parameterCompatibilityLevel(MethodBinding, TypeBinding[], InvocationSite)
    • Scope.mostSpecificMethodBinding(MethodBinding[], int, TypeBinding[], InvocationSite, ReferenceBinding)
      • translate NEEDS_MISSING_TYPE to ProblemMethodBinding(Ambiguous) using the method with missing types
  • Scope.computeCompatibleMethod(MethodBinding, TypeBinding[], InvocationSite, boolean)
    • NEEDS_MISSING_TYPE is immediately translated into ProblemMethodBinding(MissingTypeInSignature)
  • ParameterizedGenericMethodBinding.computeCompatibleMethod(MethodBinding, TypeBinding[], Scope, InvocationSite)
    • invocation guarded by compilerOptions.sourceLevel == ClassFileConstants.JDK1_7 => ignore
  • Scope.isAcceptableMethod(MethodBinding, MethodBinding)
    • used in mostSpecificMethodBinding() only for "JLS7 implementation" --> ignore

Ergo, the following strategy seems to be complete:

• compliance 1.7 => don't care
• checking an individual method:
  • ensure any missing type of any relevant method is reported as either NEEDS_MISSING_TYPE or ProblemMethodBinding(MissingTypeInSignature)
• selecting a method among candidates:
  • ensure any relevant method with missing types is selected and reported as ProblemMethodBinding(Ambiguous), tagBits on that selected method allow to report reference to missing types.
    • similar to mostSpecificMethodBinding() above, also Scope.findMethod0() prefers to answer any ProblemMethod(MissingTypeInSignature) to ensure we don't swallow a problem

[stephan-herrmann]

Looking at the possible impact of a missing return type I think we can ignore the missing type in some situations:

public class B {
    public void A m() { return null; }
}

When A.class is missing while compiling the following against B.class:

public class C {
    void test(B b) {
        b.m(); // no need to analyse A, right?
    }
}

The generated bytecode can still mention the full signature including the fully qualified name of type A, because that name is found in B.class alright.

@srikanth-sankaran some questions:

• do you generally see a problem with ignoring the lack of A.class when compiling C.java?
• as for detection of the situation:
  • much can be inferred from seeing MessageSend.expressionContext == VANILLA, but
    • the expression context doesn't help for b.m().methodOfA(): m's context is VANILLA, but the return type is needed nevertheless.
    • one thing that looks like an ASSIGNMENT context is not tagged as such @An(a=b.bar()) => intention or forgotten? see next comment
    • I was tempted to add RECEIVER as a new ExpressionContext but that would probably be quite disruptive / mix our concerns with those defined in JLS
    • next best solution would be a simple flag MessageSend.isReceiver to be set from the containing MessageSend and FieldReference -- ✔️ I chose this solution

WDYT?

[stephan-herrmann]

one thing that looks like an ASSIGNMENT context is not tagged as such @An(a=b.bar()) => intention or forgotten?

Here I was misled by AnnotationTest.test206() that advertises the syntax @Ann(m=bar()), but when this happens with all types present, compilers still reject:

• javac: "expression not allowed as annotation value"
• ecj: "The value for annotation attribute Ann.m must be a constant expression"

Lesson learned: ecj detects the wrong expression kind only after successful resolution, whereas javac seems to detect this based just on syntax.

Setting VANILLA context for MemberValuePair.value (the default) is plain wrong. With the pending change this would hide not only the missing type but also the wrong expression shape, which is only detected if type resolution was successful!

To fix this we can choose between setting expressionContext to ASSIGNMENT or null. Either helps to signal that ignoring the return type is not OK, so we get at least one error, even if the interesting error ("must be a constant expression") is not shown (no change here). To me ASSIGNMENT feels more natural than null.

@srikanth-sankaran TL;DR: of this comment: do you think we should complain about message send (and others?) in this position where a constant is expected ahead of resolution?

[srikanth-sankaran]

Sorry for the delay. I have finally started reviewing this. A rather naive question given the long history of this: what does javac do ? Is that captured somewhere ??

[stephan-herrmann]

Sorry for the delay. I have finally started reviewing this. A rather naive question given the long history of this: what does javac do ? Is that captured somewhere ??

Short answer: I haven't checked, but perhaps we can just run the new tests with run.javac option enabled.

I think the issue is less relevant for javac, since invocations of javac from a build system like maven tend to present all transitive dependencies on the classpath, so the situation of missing types would simply not occur.

OSGi, however, has always distinguished between dependencies that are visible (re-exported) vs. invisible to clients, and that's what inspired the strategy for java projects in Eclipse and for ecj.

With JPMS in the picture, this distinction would become relevant for javac, too.

[stephan-herrmann]

... perhaps we can just run the new tests with run.javac option enabled.

Results from running ProblemTypeAndMethodTest with -Drun.javac=enabled -Djdk.root=/home/java/jdk-22 -Dcompliance=22:

9 failures, 2 of which occur in new tests:

• testMissingClassNeededForOverloadResolution_varargs2
  • javac reports "class file for p1.A not found"
• testMissingClass_returnType_OK
  • javac reports "class file for p1.A not found"

From the other failures only one looks remotely related:

• test030:
  • ecj: "The hierarchy of the type X is inconsistent"
  • javac: silent

Among the negative tests, where both ecj and javac report problems, javac sometimes reports "class file for p1.A not found" and sometimes reports incompatibilities like

A cannot be converted to B
	public B m(Object o) { return super.m(o); }
	                                     ^

where our new message would be more helpful: "The method m(Object) from the type B refers to the missing type A".
The super call points to

	public A m(Object o) { return new A(); }

"cannot be converted" could actually be wrong. Perhaps A is indeed a subtype of the expected B, but we can't tell since A.class is missing.

So overall compilers generally agree in which cases missing class files should be complained about. In the first part of testMissingClassNeededForOverloadResolution (compiling C.java) they agree to tolerate the missing class file. For the other two positive tests (testMissingClassNeededForOverloadResolution_varargs2 and testMissingClass_returnType_OK) the change would make ecj more tolerant than javac.

[srikanth-sankaran]

Looking at the possible impact of a missing return type I think we can ignore the missing type in some situations:

Could you be more specific ? What do you mean by "some situations" - From the example you have cited and the mention of vanilla context I assume you mean calls where the return value is "dropped" - without assignment/return etc ? And where the return value does not serve as the reference for array indexing, method invocation, field access ?

public class B {
    public void A m() { return null; }
}

When A.class is missing while compiling the following against B.class:

public class C {
    void test(B b) {
        b.m(); // no need to analyse A, right?
    }
}

The generated bytecode can still mention the full signature including the fully qualified name of type A, because that name is found in B.class alright.

@srikanth-sankaran some questions:

• do you generally see a problem with ignoring the lack of A.class when compiling C.java?
• as for detection of the situation:

Not at the outset. I would say we construct a special junit for all these cases and compare behavior with javac and use that as guiding principle.

• much can be inferred from seeing MessageSend.expressionContext == VANILLA, but

  • the expression context doesn't help for b.m().methodOfA(): m's context is VANILLA, but the return type is needed nevertheless.
  • one thing that looks like an ASSIGNMENT context is not tagged as such @An(a=b.bar()) => intention or forgotten? see next comment
  • I was tempted to add RECEIVER as a new ExpressionContext but that would probably be quite disruptive / mix our concerns with those defined in JLS

I agree this is disruptive and best avoided.

* next best solution would be a simple flag `MessageSend.isReceiver` to be set from the containing `MessageSend` and `FieldReference` -- ✔️ I chose this solution

How did you arrive at where it needs to be set from ? It appears you have zoomed into MessageSend, FieldReference and ArrayReference but how about org.eclipse.jdt.internal.compiler.ast.QualifiedAllocationExpression.enclosingInstance
or org.eclipse.jdt.internal.compiler.ast.InstanceOfExpression.expression (An alert here that I introduced the extra linguistic org.eclipse.jdt.internal.compiler.ast.ExpressionContext.INSTANCEOF_CONTEXT for some code simplification)

(I narrowed down on these two by a quick scan Expression's hierarchy.)

In general what is the effect of isReceiver not being set appropriately in some scenarios ? Is it limited to that scenario not benefitting from the improvement brought about by this PR or could there be more deleterious effects ??

WDYT?

[srikanth-sankaran]

Is org.eclipse.jdt.internal.compiler.lookup.TypeBinding.collectMissingTypes(List<TypeBinding>) called from all relevant places ?? For instance I don't see any checks for permitted subtypes missing - what does it mean ??

[srikanth-sankaran]

Can we have a brief solution statement ? It would be useful to have a write up that states in these limited scenarios, rather than issue a hard error about class path being incorrect, the compiler would tolerate missing class files.

What would these be ?

I know I can gather that from perusing various comments here in the PR and the github issue and by looking at the code changes, but it would help to have 4-6 line write up of the current stated objective of the fix - so I can see if that maps fully to the code changes bidirectionally

[srikanth-sankaran]

@srikanth-sankaran TL;DR: of this comment: do you think we should complain about message send (and others?) in this position where a constant is expected ahead of resolution?

That would depend on how easy it is to do so. And if easy I would fork that work off to a different ticket.

[srikanth-sankaran]

OK, I have made an initial pass over the changes and the commentary here and in the ticket. I will make one more pass tomorrow hopefully after getting some response to the questions raised in the interim.

[srikanth-sankaran]

Can we have a brief solution statement ? It would be useful to have a write up that states in these limited scenarios, rather than issue a hard error about class path being incorrect, the compiler would tolerate missing class files.

Reason for this request being some of the strategies/outline discussed in #543 (comment) appear outdated or disjoint from the code changes. This is perhaps due to the realization that various pieces of the puzzle already exist and can further be built upon ??

I am looking for something like: "When a class file referenced in a compilation mentions type names that are missing in the class path, tolerate the missing types if they feature in methods which are never seen to be invoked during the compilation of the sources, or if the missing types only feature in return types that are dropped or in variable arity situations unneeded actually to invoke the call." or something like that. That would help the next round of review and discussions. TIA

What would these be ?

I know I can gather that from perusing various comments here in the PR and the github issue and by looking at the code changes, but it would help to have 4-6 line write up of the current stated objective of the fix - so I can see if that maps fully to the code changes bidirectionally

[stephan-herrmann]

Start of piecemeal answers:

• do you generally see a problem with ignoring the lack of A.class when compiling C.java?

Not at the outset. I would say we construct a special junit for all these cases and compare behavior with javac and use that as guiding principle.

I agree to writing more junits, but I don't see, why javac should be the guiding principle in this matter. As outlined above, I believe Eclipse has a more vital interest to work with an incomplete classpath than javac. I'd love to see ecj more resilient in this matter, just we shouldn't produce wrong byte code.

next best solution would be a simple flag MessageSend.isReceiver to be set from the containing MessageSend and FieldReference -- ✔️ I chose this solution

How did you arrive at where it needs to be set from ?
AST classes with a field receiver :)

It appears you have zoomed into MessageSend, FieldReference and ArrayReference but how about org.eclipse.jdt.internal.compiler.ast.QualifiedAllocationExpression.enclosingInstance

good point. Will add it.

or org.eclipse.jdt.internal.compiler.ast.InstanceOfExpression.expression (An alert here that I introduced the extra linguistic org.eclipse.jdt.internal.compiler.ast.ExpressionContext.INSTANCEOF_CONTEXT for some code simplification)

Thanks for the alert, this actually looks risky to me. How did you ensure that checks like if (expr.expressionContext == ExpressionContext.VANILLA) don't take a wrong turn when encountering the unexpected constant?

I briefly considered an alternative design: what we need is similar to the flag valueRequired in analyseCode() methods. Now, how hard would it be to add the same parameter to relevant resolve methods? I vaguely remember the same had already been discussed in some other context. Interestingly, the variant of analyseCode() that accepts this flag is only ever called from these classes:

• FieldReference
• ConditionalExpression
• MessageSend
• ReferenceExpression

Most of these use the flag for optimizations in case of statics or constants (which I may or may not adopt for my case). Obviously I should add ReferenceExpression.lhs to my list.

The specification to be implemented is a bit backwards: find all AST fields meeting both these criteria:

• will be resolved in VANILLA context
• value is needed nevertheless.

Does BinaryExpression induce a specific ExpressionContext on its operands, or are those vanilla contexts? (same for UnaryExpression?)

===> Ergo: this little distinction is getting a bit convoluted. Perhaps the inverse of valueRequired can be detected using ASTNode.InsideExpressionStatement? I'll give that a try.

[stephan-herrmann]

Is org.eclipse.jdt.internal.compiler.lookup.TypeBinding.collectMissingTypes(List<TypeBinding>) called from all relevant places ??

To me this question looks out-of place: the method is now collectMissingTypes(List<TypeBinding>,boolean), where the addition of one more parameter is one of only two changes I made in this area.

The other change is, that the method is called from one more location.

For instance I don't see any checks for permitted subtypes missing - what does it mean ??

The method asks a MethodBinding which of the types it references are missing. I don't see any relation from a method binding to permitted subtypes.

What am I missing?

[srikanth-sankaran]

Solution outline:

• we no longer require all types in all method signatures of binary types to be present
• if a method is never touched during resolution of any MessageSend, then we tolerate all its references to missing types
• if a method is touched during overload resolution, but it can be ruled out based only on arity, then references to missing types can still be ignored.

I suppose it is also true that if a method is touched during overload resolution, but it can be ruled out based only on earlier parameters which don't refer to any missing types, then references to missing types in subsequent parameters can still be ignored. but perhaps that was true earlier itself ?

[srikanth-sankaran]

On test failures:

• ASTModelBridgeTests.testCreateBindings14a

* previously bit HasMissingType was not propagated properly, thus hiding the problem

• fixed both ways:

[...]

• ImportRewrite18Test.testBug474270_since_8

* previously this was undetected due to incomplete tagging with HasMissingType
* still tagging is incomplete, failed to propagate through substitution

When reviewing the underlying code earlier, I got the sense that we may not be fully evolving these as surrounding code evolves - the general issue could be addressed in #2651, but it is cool that these got caught already

[srikanth-sankaran]

Solution outline:

• we no longer require all types in all method signatures of binary types to be present
• if a method is never touched during resolution of any MessageSend, then we tolerate all its references to missing types
• if a method is touched during overload resolution, but it can be ruled out based only on arity, then references to missing types can still be ignored.

I suppose it is also true that if a method is touched during overload resolution, but it can be ruled out based only on earlier parameters which don't refer to any missing types, then references to missing types in subsequent parameters can still be ignored. but perhaps that was true earlier itself ?

Perhaps more generally, if a method is touched during overload resolution, but it can be ruled out based only on some parameter which doesn't refer to any missing types, then references to missing types in parameters can still be ignored. ??

I glanced through the numerous new tests in ProblemTypeAndMethodTest but don't recall readily if we have a test for this scenario to see what is the behavior:

public void m(String s, MissingType m, Date d);
public void m(String s, Object o, Integer i);

and given call site --> m("Hello", new Object(), 42);

So given two overloads, a first argument which is compatible with either method's first parameter and one candidate missing a type in second parameter position and third argument not compatible with the parameter of the method with the missing 2nd parameter, but compatible with the 3rd parameter of the other overload.

[srikanth-sankaran]

Status of review: Overall looks pretty good.

Files yet to be reviewed:

org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/lookup/Scope.java

and a deeper check through:

org.eclipse.jdt.core.tests.compiler/src/org/eclipse/jdt/core/tests/compiler/regression/ProblemTypeAndMethodTest.java

I'll look at these first thing tomorrow morning. OK ?

[stephan-herrmann]

I suppose it is also true that if a method is touched during overload resolution, but it can be ruled out based only on earlier parameters which don't refer to any missing types, then references to missing types in subsequent parameters can still be ignored. but perhaps that was true earlier itself ?

Right, parameterCompatibilityLevel(MethodBinding, TypeBinding[], boolean) essentially works left-to-right (except for the varargs arg, which is processed first), and if a candidate can be ruled out by an earlier parameter (NOT_COMPATIBLE) then subsequent ones are not looked at. This hasn't changed.

[stephan-herrmann]

Re tagging with HasMissingType and collectMissingTypes():

When reviewing the underlying code earlier, I got the sense that we may not be fully evolving these as surrounding code evolves - the general issue could be addressed in #2651, but it is cool that these got caught already

I feel that tagging and collecting are essential dual to each other, but in ProblemReporter.missingTypeInMethod() (and friends) some inconsistencies hit the fan, when a type was tagged, but collectMissingTypes() wouldn't be able to present the witness. That's why I had to get ahead of myself and venture into #2651 :)

[stephan-herrmann]

public void m(String s, MissingType m, Date d);
public void m(String s, Object o, Integer i);

and given call site --> m("Hello", new Object(), 42);

So given two overloads, a first argument which is compatible with either method's first parameter and one candidate missing a type in second parameter position and third argument not compatible with the parameter of the method with the missing 2nd parameter, but compatible with the 3rd parameter of the other overload.

This actually required a little fine tuning, but now we have testMissingClassNeededForOverloadResolution_pickByLateArg :)

[stephan-herrmann]

I'll look at these first thing tomorrow morning. OK ?

First thing in the morning: breakfast, or perhaps exercise.
After that I'll be happy to take your final verdict.

[srikanth-sankaran]

TODO:

• consider Scope.parameterCompatibilityLevel(TypeBinding, TypeBinding) as used for lambdas and reference expressions)

While its namesakes participate in invocation resolution, this method parameterCompatibilityLevel() is used in situations where ignoring missing types doesn't look like an easy / big win:

Did you mean "handling missing types doesn't look like an easy / big win" as opposed to " ignoring missing types doesn't look like an easy / big win" - we currently seem to ignore missing types here and bubble up NOT_COMPATIBLE ?

[srikanth-sankaran]

I'll look at these first thing tomorrow morning. OK ?

First thing in the morning: breakfast, or perhaps exercise. After that I'll be happy to take your final verdict.

My day officially starts only after exercise and breakfast my friend :)

[srikanth-sankaran]

I only glanced through ProblemTypeAndMethodTest.java but closely reviewed all other files. Looks pretty good to me! Comments uploaded for a few loose ends.

[stephan-herrmann]

TODO:

• consider Scope.parameterCompatibilityLevel(TypeBinding, TypeBinding) as used for lambdas and reference expressions)

While its namesakes participate in invocation resolution, this method parameterCompatibilityLevel() is used in situations where ignoring missing types doesn't look like an easy / big win:

Did you mean "handling missing types doesn't look like an easy / big win" as opposed to " ignoring missing types doesn't look like an easy / big win" - we currently seem to ignore missing types here and bubble up NOT_COMPATIBLE ?

Both: we'd want to handle missing types in such a way that we can ignore those that have no impact on resolution :)

[stephan-herrmann]

Interestingly, my last commit made JavaSearchBugsTests2.testBug123836c ff fail, such that an expected EXACT match is downgraded to POTENTIAL.

• Indeed that test tries to reference Serializable without importing - I don't think this is intended, but let's play fair
• of two sought invocations, p2.compute(null) has a receiver with HasMissingType, this MessageSend has no binding!
• as a result of the missing binding, MethodLocator.resolveLevel() answers INACCURATE_MATCH
• the receiver p2 has type Property<capture#1-of ? extends Serializable>
• indeed propagating HasMissingType from a bound to the enclosing capture is part of 1b4526a
• in the same commit I also added propagation from type argument to ParameterizedTypeBinding, but ...
  • actually this step happens in PTB.initialize() already since https://bugs.eclipse.org/196200

If I would eliminate the propagation type argument -> ParameterizedTypeBinding, then we would not get a match with receiver p2 at all.

With this I believe the best way forward is

• update the tests to actually import Serializable
• add just one variant without the import where expectation would be POTENTIAL not EXACT

@srikanth-sankaran it looks like omitting the import in https://bugs.eclipse.org/123836 was not intended, right?

[srikanth-sankaran]

@srikanth-sankaran it looks like omitting the import in https://bugs.eclipse.org/123836 was not intended, right?

I think so too. In all the additional variants of tests I have posted on that ticket Serializable is imported. In Markus's original comment#0 snippet the import is missing and it is captured as is.