[1.8][inference] refresh inference implementation wrt recent JLS chap 18

[stephan-herrmann]

From https://bugs.eclipse.org/bugs/show_bug.cgi?id=563314

Several unannounced spec changes have gone under the radar over the years.

I'll do a full refresh of our entire implementation of chapter 18 to ensure we implement the spec as closely as possible. I assume that a full refresh might be easier indeed than hunting more bug reports in this area.

[stephan-herrmann]

Collecting some differences as I go, some of which are our own inventions with no counter part in JLS:

• ReductionResult.POTENTIALLY_COMPATIBLE: in 18.5.1 we generate more constraints than specified. This introduces checks into type inference, which should legally happen already during 15.12.2.1. Reduction of such "constraints" always produces TRUE or FALSE immediately, so there is no interaction with type bounds, like during incorporation.
  • introduced in https://bugs.eclipse.org/bugs/show_bug.cgi?id=448954
• ConstraintExpressionFormula.reduce() has extra logic when no valid resolved type exists. If this enhances resilience: good. JLS assumes that a type was resolved, so we are free to handle the negative case as we like.
  • Introduced in https://bugs.eclipse.org/bugs/show_bug.cgi?id=470958
• ConstraintExpressionFormula.reduce() the lambda branch generously answers TRUE when T is an ivar. 18.2.1 requires that we determine if T is a functional interface type, which cannot be answered for an ivar. But then I find this comment in 18.2.1:
  • Note that if the target type is an inference variable, or if the target type's parameter types contain inference variables, we produce false. During invocation type inference (§18.5.2.2), extra substitutions are performed in order to instantiate these inference variables, thus avoiding this scenario. (In other words, reduction will, in practice, never be "invoked" with a target type of one of these forms.)
  • "Fixing" our implementation in this regards causes a regression in GenericsRegressionTest_1_8.testBug545420()
• For reference expressions 18.2.1 requires "Otherwise, a search for a compile-time declaration is performed, as specified in §15.13.1." -- We don't do this, but continue with the potentially applicable method instead. There's already a "Note" comment in our code.
• ConstraintExpressionFormula.needsInference() has a special branch for constructor references of a generic type. We decide that inference is needed which seems to make sense, but this is absent from JLS.
• We fail to implement " If R mentions one of the type parameters of the function type, the constraint reduces to false." - adding this has no effect on existing tests.

[srikanth-sankaran]

Collecting some differences as I go, some of which are our own inventions with no counter part in JLS:

[...]

• We fail to implement " If R mentions one of the type parameters of the function type, the constraint reduces to false." - adding this has no effect on existing tests.

Here is a thought. What would it mean for us to create a new junit suite: JavacEcjDifferingBehaviorTests.java and add tests from submitted snippets from all open tickets to it ? Then when we make a change of this kind, it would be interesting to know if the behavior gets to align on one or more of these tests.

That a code change " has no effect on existing tests." is useful to know. But the way we are set up, we will miss when a code change has an effect - perhaps a welcome one on some test case where we differ in behavior.

It could be a lot of work to come up with such a test suite, but it could be worth it. All committers should know and be mindful of course that JavacEcjDifferingBehaviorTests.java captures "current" behavior - which may not be the right behavior.

[stephan-herrmann]

Here is a thought. What would it mean for us to create a new junit suite: JavacEcjDifferingBehaviorTests.java and add tests from submitted snippets from all open tickets to it ? Then when we make a change of this kind, it would be interesting to know if the behavior gets to align on one or more of these tests.

That a code change " has no effect on existing tests." is useful to know. But the way we are set up, we will miss when a code change has an effect - perhaps a welcome one on some test case where we differ in behavior.

It could be a lot of work to come up with such a test suite, but it could be worth it. All committers should know and be mindful of course that JavacEcjDifferingBehaviorTests.java captures "current" behavior - which may not be the right behavior.

@srikanth-sankaran do you remember our run.javac test mode? I believe it comes very close to what you have in mind, doesn't it? The latest status of that mode can be seen in #386, perhaps the last related bugs in bugzilla also help to get a picture:

• https://bugs.eclipse.org/bugs/show_bug.cgi?id=570021
• https://bugs.eclipse.org/bugs/show_bug.cgi?id=539053

Would it make sense to copy or move all tests with known difference between ecj and javac to a separate test class?

[srikanth-sankaran]

Would it make sense to copy or move all tests with known difference between ecj and javac to a separate test class?

I am talking about a slightly orthogonal matter here. When a defect comes in with a difference in behavior reported, we just capture the test case with the "current" perhaps faulty behavior. A collection of such tests will help us catch inadvertent fixes brought about by a change elsewhere in a completely different context.

For example, those "cannot infer type arguments" bugzilla tickets that we recently discussed. We would have caught that a change made for issue XYZ addresses issue PQR in a welcome manner. Without having to manually test that is.

[srikanth-sankaran]

The starting point was your comment "adding this (some JLS clause) has no effect on existing tests." - existing tests fail to capture all the snippets in hundreds of open tickets. What if your change had an effect on some of those tests for example

[stephan-herrmann]

I am talking about a slightly orthogonal matter here. When a defect comes in with a difference in behavior reported, we just capture the test case with the "current" perhaps faulty behavior. A collection of such tests will help us catch inadvertent fixes brought about by a change elsewhere in a completely different context.

So you are talking about tests that "falsely" succeed where we want to be informed when the outcome changes? In the best case, a failure in that suite could indicate a bug has been fixed without knowing.

That would indeed by feasible, and more likely to help than predicting an exact expected outcome (where tests would still fail on any unrelated difference in compiler messages).

Had we unlimited resources I would still integrate it with run.javac, perhaps with both a current and an expected outcome. This could produce the additional welcome effect that we will be informed, when javac behavior changes in any of these cases. But the current state of affairs in #386 is not very encouraging.

For the time being it makes sense to separate both issues:

• monitor the outcome regarding currently unresolved bugs
• comparing ecj with javac

I guess it's the name JavacEcjDifferingBehaviorTests.java that got me derailed, the test could perhaps host tests for all issues that could not be resolved after due investigation, no matter the reason why the thing was considered a bug.

[srikanth-sankaran]

For the time being it makes sense to separate both issues:

• monitor the outcome regarding currently unresolved bugs
• comparing ecj with javac

I guess it's the name JavacEcjDifferingBehaviorTests.java that got me derailed, the test could perhaps host tests for all issues that could not be resolved after due investigation, no matter the reason why the thing was considered a bug.

I was talking about only the former and your last observation is correct.

Perhaps it should be a project process to, when a new compiler issue gets reported with a reproducible test case, immediately capture the current behavior into a junit test - as long as people understand that it captures likely faulty behavior we should be good.

[stephan-herrmann]

Perhaps it should be a project process to, when a new compiler issue gets reported with a reproducible test case, immediately capture the current behavior into a junit test - as long as people understand that it captures likely faulty behavior we should be good.

@srikanth-sankaran please see https://github.com/eclipse-jdt/eclipse.jdt.core/pull/2305/files#diff-0e52fa5757c780704f8d75a913f25a639b18f55cebadf6957c383bd8fab3a85a -- is that what you have in mind?

[srikanth-sankaran]

Perhaps it should be a project process to, when a new compiler issue gets reported with a reproducible test case, immediately capture the current behavior into a junit test - as long as people understand that it captures likely faulty behavior we should be good.

@srikanth-sankaran please see https://github.com/eclipse-jdt/eclipse.jdt.core/pull/2305/files#diff-0e52fa5757c780704f8d75a913f25a639b18f55cebadf6957c383bd8fab3a85a -- is that what you have in mind?

Perfect, yes, I like the name of the suite (Dubious...). Nit: "outcome of examples were we doubt if the outcome is correct", you meant "where"

Now we just need to systematically start to capture snippets from incoming defect reports. I pledge to do that for the reproducible cases I come across.

@mpalat, @jarthana - likewise I urge you to too.

[stephan-herrmann]

Nit: "outcome of examples were we doubt if the outcome is correct", you meant "where"

As a musician my mechanics of typing are driven by phonetics, while semantic analysis sometimes has difficulties keeping pace :)

[stephan-herrmann]

Observations regarding 18.2.2 ff (ConstraintTypeFormula.reduce()):

• addConstraintsFromTypeParameters(..) contained a hack using TB.isParameterizedWithOwnVariables() to fix bug 430759
  • this is no longer necessary, probably due to bug 488328: the funny parameterized type only occurred as an enclosing type, and since the latter bug shape-problems of enclosing types no longer trigger FALSE.
• findMostSpecificSuperArray() only checks one level of supers, while "most specific" (words from JLS) sounds like more hierarchy-traversal would be needed.
  • I'm only aware of one-and-a-half forms of non-array type with array supertype: ? extends X[] plus capture thereof. So perhaps this one level is enough?
• we have a "speculative addition" for the case ‹S <= T› where T is an ivar, S is a wildcard. Removing would cause
  • alignment with javac in AmbiguousMethodTest.test087() :+1:
  • let us report errors not found by javac in GenericTypeTest.test036/test037 :-1:
• semantically I wonder what the spec expresses when saying ‹Object <: T'›
  • I would always expect Object on the other side of <:, reversing the constraint has no effect in our test suite
• missing in ecj: ‹S = T› where S and T are intersection types !

[stephan-herrmann]

Observations regarding 18.2.5 (ConstraintExceptionFormula.reduce()):

• reduction of ConstraintExceptionFormula has an illegal shortcut for the case n=0
  • implementation closer to the spec was removed via bec58435
  • this was discussed in https://bugs.eclipse.org/429969#c14 ff.

[stephan-herrmann]

Reminder to self: see if (meanwhile?) the spec gives us leeway to add the change from https://git.eclipse.org/r/c/jdt/eclipse.jdt.core/+/99705/