Overhaul of how type inference handles capture bounds#4735
Conversation
Phase 1: Stop to clear all capture bounds at end of incorporate + remove only those bounds that have produced a new TypeBound + incl. new failing test Result of TestAll at compliance 25: 2 failures Fixes eclipse-jdt#4635
e5c52fc to
270c2b1
Compare
Phase 2: Rewrite dependency-computation
+ new method IC18.collectDependencies() creates a map iv -> iv*
- use that map also in pickFromCycle
- re-implement dependsOn based on that map,
this replaces BoundSet.dependsOnResolutionOf() et al
- obsoletes also ThreeSets.inverseBounds
+ shrink set of vars to resolve during IC18.resolve()
+ maintain separate sets captures / allCaptures
- IC18.resumeSuspendedInference resets captures but not allCaptures
+ avoid CaptureBinding18.upperBounds == null
+ fully enable the new test
Result of TestAll at compliance 25: 39 failures
Fixes eclipse-jdt#4635
Compensate regressions with NON-JLS improvement eclipse-jdt#1: + consider existing same bounds if substitution makes them proper Result of TestAll at compliance 25: 11 failures Fixes eclipse-jdt#4635
Compensate regressions with NON-JLS improvement eclipse-jdt#2: + consider *all* expression constraints as soft Result of TestAll at compliance 25: 10 failures Fixes eclipse-jdt#4635
Compensate regressions with NON-JLS improvement eclipse-jdt#3: + try to resolve ignoring super-dependencies - fall back to considering those, too, if resolve would then fail + adjust 2 expected results in NullTypeAnnotationTest Result of TestAll at compliance 25: 1 failure Fixes eclipse-jdt#4635
Adopt test & fix from PR 4565 Also-by: coehlrich <coehlrich@users.noreply.github.com>
270c2b1 to
fdb0a61
Compare
|
@srikanth-sankaran @jarthana @coehlrich this PR marks a significant change to type inference, so I'd like to invite you to comment on this multi stage saga: two steps forward ...I identified two aspects where ECJ can and should be brought closer to JLS: computing dependencies between inference variablesInitial investigation of #4635 indicated that dependency computation for 18.4. needed a fix. I realized that helper method As the improved implementation still didn't help for #4635 I realized that it was working on wrong input: at this stage of type inference capture boundsFor twelve years I was hesitant about the following line at the end in
(see 18.1.3. for the definition of capture bounds). Up to this point my changes should have brought us closer to JLS, but it brought us to a great distance to what we should be doing according to javac :( Then I braced myself for addressing all those "regressions" for whatever it costs. ... and many steps backwardsAll subsequent changes apply extra-constitutional tweaks in the hope that my tweaks might be morally similar to the many such tweaks which javac is reportedly applying.
In the end even risks?In a way I was surprised myself that this series of tweaks gradually fixed all the intermediate regressions. In light of size and significance of the change I was going to ask someone for close code review, but I'm afraid this would keep the reviewer busy for long time. More time than we have available. Actually I'm not sure if a clear yes or no can possibly be given to this PR. The status quo, OTOH, has known defects. And a third solution is not in sight. My feeling is: as long as ECJ has those obvious bugs wrt JLS, we cannot expect help from Oracle with any difficulties we are facing in bug reports of this area. Perhaps we should do both: first do exactly as JLS mandates, and then on-top apply any opportunistic tweaks that help accepting more programs which also javac accepts (based just on common sense reasoning plus experiments). To mitigate the inherent risk in this, I'd like to ask if people could help STRESS TESTING this change. I made a start by compiling all of the Eclipse SDK with no new errors reported. ✔️ I will add more details about individual tweaks and what I learned along the road, and be it only as a way of self-review 😄 |
relevance of ivar dependenciesWhy is it crucial that resolution (18.4) works with exact dependencies between ivars? Each ivar is put into a group with all other ivars on which it (transitively) depends. ivars in the same group are then resolved in one batch. If the first strategy in 18.4. fails for any ivar in that set, all ivars will be resolved using the "second attempt" where fresh type variables "Yi" are assigned (our code uses the symbol Z from previous versions of JLS). If, e.g., an ivar If, however, any ivar in the same group forces resolution into the "second attempt", then the upper bound will be preserved as an upper bound, resulting in a type variable Y which is not compatible with String. In this case other ivars depending on So, resolving ivars in smaller groups may give better results, provided that the sharp result for one ivar will not cause subsequent resolution to fail. While dependencies are well-defined in 18.4. we have these two weak spots:
|
bi-directional dependenciesIn one test I observed If we interpret If OTOH, we consider So considering I noticed that this tweak allows inference to give better results in particular in situations of inferring a standalone expression, as seen at import java.util.*;
import java.util.stream.*;
import static java.util.Arrays.asList;
public class C {
static final List<Integer> DIGITS = Collections.unmodifiableList(asList(0,1,2,3,4,5,6,7,8,9));
Collection<String> flatMapSolutions(final boolean b) {
Collection<String> solutions =
DIGITS.stream().flatMap( s -> {
return b ? Stream.empty() : Stream.of("");
}) .collect(Collectors.toList());
return solutions;
}
}Here |
same-boundsThe "second attempt" in 18.4 seems to ignore type bounds of a shape like Edit: As per a quick experiment: Replacing the previous fix regarding same-bounds with a solution where |
soft constraintsIt was one of the first admissions by Oracle, that javac is wrong in assuming subtyping between a raw type and a parameterized type. If they don't care to fix this in 12 years, then why should we care about the extent of that bug? |
capture boundsThe interaction between inference and inner inference isn't sufficiently specified in JLS. In this area we are already dependent on advice from javac developers. So I see leeway in interpreting whether or not capture bounds from inner inference should be considered in outer inference. Hence keeping separate sets I'm a bit uneasy about the follow-up questions of which code location should use which of these sets, but here I fully rely on "if it helps, its good". As we are inside a blind spot of JLS, we cannot expect any help from JLS. |
|
What are the chances that I can get some comments any time soon? If brain capacity for this saga is low atm, perhaps I may ask @jarthana for some stress testing? I'd like to merge this for M2, to give it more exposure sooner rather than later. We had some recent regressions in the wider area of type inference, and I'm hesitant to address any of them without first bringing this PR to conclusion. |
@coehlrich - you might be interested?
If nobody answers, please. |
Done. Of course I'm still interested in comments. |
Looks good. |
|
For posterity: also this part seems to become unnecessary by wip in #5004:
|
…k to false as recommended
The PR brings some parts of our implementation closer to JLS, while adding extra-constitutional tweaks to compensate for regressions caused by the former set of changes.
Closer to JLS:
InferenceContext18.getSmallestVariableSet()is quite beside the point. This is a full re-implementation - currently ignoring performance aspects until we have measurements of how bad things are.NON-JLS
Actually I added a new task tag "NON-JLS" to help find all relevant code locations that are based on our own invention rather than JLS.
capturesandallCaptures, so that different code locations can leverage or ignore capture bounds created during nested inference.InferenceContext18.ARGUMENT_CONSTRAINTS_ARE_SOFTencoded the advice that only specific expression constraints should be "soft" (which is a NON-JLS concept in its own). I identified a test case which requires that all expression constraints are "soft".α :> βdo not considerαto depend onβ(only the reverse dependency fromβtoαwill be considered in that mode). If, however, this tweak would make resolve() fail, then fall back to the original version wereα :> βis seen as a bidirectional dependency.Finally I had to include the fix from #4565 to fix the remaining regressions from the initial changes.
Fixes #4635