Community
Participate
Working Groups
We delete the capture bounds right after incorporation so 18.4 almost never sees them. This is likely incorrect and unintended in the JLS. We need to move to a model that is more closely aligned with JLS and implement the fresh captures scheme in 18.4 robustly - this may hinge on https://bugs.eclipse.org/bugs/show_bug.cgi?id=447576 also.
Other deviation noticed which may be harmess: - We don't always create fresh inference variables in 18.5.2 where JLS requires it (private mail from author recommends creating variables only for wildcard though) - reduceAndIncorporate does only reduce and not incorporate - acknowledged in the code
(In reply to Srikanth Sankaran from comment #0) > We delete the capture bounds right after incorporation so 18.4 almost never > sees them. This is likely incorrect and unintended in the JLS. We need to > move to a model that is more closely aligned with JLS and implement the > fresh captures scheme in 18.4 robustly - this may hinge on > https://bugs.eclipse.org/bugs/show_bug.cgi?id=447576 also. Yes, deleting during incorporate seems wrong. I made a Q&D experiment: remove the bogus clear compensated by a guarding set of already-incorporated keys. I was surprised to see this introduce 22 regressions just in GRT18. Do we (you :) ) have a theory, why being more correct breaks things here?
(In reply to Stephan Herrmann from comment #2) > > Yes, deleting during incorporate seems wrong. > I made a Q&D experiment: remove the bogus clear compensated by a guarding > set of already-incorporated keys. I was surprised to see this introduce 22 > regressions just in GRT18. > Do we (you :) ) have a theory, why being more correct breaks things here? Yes, that is because the code for 18.4 is not fully exercised with capture bounds. See https://bugs.eclipse.org/bugs/show_bug.cgi?id=437444#c109 CTF.reduce() starts encountering formulas of the form: ⟨java.util.Map<K#3,U#4> = <Z#8-Map<K#3,U#4>#7 extends java.lang.Object>⟩ ? that it is not prepared to. Dan's suggestion to not introduce fresh inference variables for non-wildcards should improve matters even if not fully solve the problem.
*** Bug 447576 has been marked as a duplicate of this bug. ***
*** Bug 448800 has been marked as a duplicate of this bug. ***
*** Bug 448914 has been marked as a duplicate of this bug. ***
*** Bug 429264 has been marked as a duplicate of this bug. ***
Documenting email communication from the spec author Daniel Smith@Oracle here: // ----- > I need to spend some more time with this example to be sure (I'll get back to you), but you may be running into JDK-8054721. > > https://bugs.openjdk.java.net/browse/JDK-8054721 > > The portion of 18.4 that talks about "fresh type variables" is intended to simulate capture, replacing the placeholder inference variables with actual type variables. But it has some problems, not distinguishing between declared bounds and inferred bounds. Okay, yes. That's the bug you want to look at. Here's how I'd describe the problem (lowercase for inference vars): You have the bound: Collector<x1,x2,x3> = capture(Collector<t,?,Map<k,u>>) Incorporation (18.3.2) implies: x1 = t x3 = Map<k,u> Resolution (18.4) claims that x1 depends on t (via the capture bound), but also t depends on x1 (via the equals bound). Similarly for x3 and k, u. Now there's a cycle involving a mixture of capture placeholders and normal inference variables. As pointed out in JDK-8054721, this is bad. You can do one of two things: - Follow the suggestion in the bug report: resolve the ivars on the RHS (t, k, and u); perform (real) capture to resolve x1, x2, and x3; then continue with resolution. - Don't bother introducing capture inference variables for non-wildcards. The end result should be equivalent to what ought to happen if you introduced them and gave them equality bounds. Collector<t,x2,Map<k,u>> = capture(Collector<t,?,Map<k,u>>) I'm pretty sure javac does the second one. I'm not sure that is good enough to solve all problems in this space (i.e., there may still be some bugs until it implements the first one, too), but it addresses the common cases. // ---
From the original bug from which our duplicate bug 448914 was spawned: (In reply to Srikanth Sankaran from bug 430686 comment #22) > (In reply to Srikanth Sankaran from bug 430686 comment #21) > > > Problem seems to be that TVB does not chain to its super constructor. > > I'll fix this. > > Or more precisely: The cloned CB18 does not share the id with its prototype > for as yet undiagnosed reason. Perhaps the clone-ctor CB18(CB18) should invoke super(CB) rather than this(...)? The latter call makes us leave the chain of clone constructors. I think I was just mislead by the convenience of delegating to a constructor that lets you define sourceName and originalName.
Created attachment 248316 [details] Patch that does not work (In reply to Stephan Herrmann from comment #9) > From the original bug from which our duplicate bug 448914 was spawned: > > (In reply to Srikanth Sankaran from bug 430686 comment #22) > > (In reply to Srikanth Sankaran from bug 430686 comment #21) > > > > > Problem seems to be that TVB does not chain to its super constructor. > > > I'll fix this. > > > > Or more precisely: The cloned CB18 does not share the id with its prototype > > for as yet undiagnosed reason. > > > Perhaps the clone-ctor CB18(CB18) should invoke super(CB) rather than > this(...)? > The latter call makes us leave the chain of clone constructors. > I think I was just mislead by the convenience of delegating to a constructor > that lets you define sourceName and originalName. I think there is more to this than interning issues. The attached patch which comments out the original fix fails the test even after id assignment is taken care of even retaining the original toString match hack - now with ids computed & transferred properly, we seem to be taking different paths altogether and don't reach the path with the toString() hack. We fail attempting to reduce: ⟨java.lang.Object <: TestClass.Alice<A#0,B#1>⟩ Didn't investigate how we get to that constraint.
See also https://bugs.eclipse.org/bugs/show_bug.cgi?id=449030#c8
Comment on attachment 248316 [details] Patch that does not work Much has changed on HEAD that this patch is not useful. It is worth investigating what other issues apart from clone not sharing proper id (which is already fixed on master) is being masked by the fix in IC18.resolve() to lower to instantiation.
Stephan, can I assign this one to you ? I think you are more familiar with this area. The present state on master is such that: - the two deviations we have in clearing captures prematurely and inferring a bound for capture of proper wildcards seem to, together, cover for all known problems. We don't have a single test case that requires a full/canonical treatment at the moment. - this bug carries target M4 - that would be ideal, since I can participate to the extent required via consultation, brainstorming, code reviews, testing etc - but this target can be adjusted per your convenience. - We have two courses to choose from: The proposed amendment at https://bugs.openjdk.java.net/browse/JDK-8054721 or the suggestion received by private communication documented at comment#8. I had an early version which I tried to attach as a patch, but that is not useful since much has changed on HEAD. I will summarize instead what changes I had in my early work in progress: - We seem to need to integrate inverse bounds right into current bound set and not maintain them separately. - We need to maintain a HashSet<ParameterizedTypeBinding> for incorporatedCaptures so that we can skip them after incorporating them once - since we will not clear capture bounds till 18.4 and this needs to be copied when bound set is cloned. - In 18.4, I think we need to create fresh type variables for all unresolved inference variables and not just the subset constructed by getSmallestVariableSet(). - I had followed Dan's recommendation to not create fresh inference variables for non-wildcard type arguments in GA. So relevant changes in BoundSet.incorporate() were in my patch. - Delete SHOULD_WORKAROUND_BUG_JDK_8054721 blocks. - BoundSet.dependsOnResolutionOf seems to require that the else keyword not be there and the else block be promoted to top level. - Changes from your patch for first bound substitution in CB18 are already on master. - The patch also had changes to reduce and immediately incorporate in InferenceContext18.reduceAndIncorporate
Notes on the two bugs that are "subsumed" as "duplicates": - https://bugs.eclipse.org/bugs/show_bug.cgi?id=429264: CB18 will surface way much more once this is resolved and so bug 429264 should be addressed along with the present one. It was my preference to close it as duplicate, but if your style dictates otherwise, you can reopen it and assign it to Jay. - https://bugs.eclipse.org/bugs/show_bug.cgi?id=447576: As acknowledged in that bug, some of the concerns originally expressed may be just plain wrong. IMO, it is still worth exploring if CB18 should be renamed to SyntheticTypeVariableBinding with CaptureBinding as its subtype rather than as its supertype. Such explorations can happen right here without reopening that bug.
Removing link to https://bugs.eclipse.org/bugs/show_bug.cgi?id=448791 as this project is orthogonal to the alternate integration between F & G.
Not for M4.
When working on this we should also revisit the changes from bug 430686.
Too much on my plate for 4.6. Bulk deferral to 4.7
I was about to file a new bug, but this one seems already on spot, please correct me if this is not relevant. My basic test case is as follows 1: Collector<String,?,Map<Integer,List<String>>> grouping = 2: groupingBy( 3: String::length, 4: () -> new TreeMap<>(reverseOrder()), 5: toList() 6: ); While the code compiles cleanly with javac 1.8.0_74, on Eclipse Mars.2 Release (4.5.2) I get the following errors: The method groupingBy(Function<? super T,? extends K>, Supplier<M>, Collector<? super T,A,D>) in the type Collectors is not applicable for the arguments (String::length, () -> {}, Collector<Object,capture#6-of ?,List<Object>>) on line 1 The type String does not define length(T) that is applicable here on line 2 Cannot infer type arguments for TreeMap<> on line 3 Possible workarounds are: 1. Explicitly specify the type arguments for the TreeMap: Collector<String,?,Map<Integer,List<String>>> grouping1 = groupingBy( String::length, () -> new TreeMap<Integer,List<String>>(reverseOrder()), toList() ); 2. Cast or separately create the Supplier (2n argument) Supplier<Map<Integer,List<String>>> ms = () -> new TreeMap<>(reverseOrder()); Collector<String,?,Map<Integer,List<String>>> grouping2 = groupingBy( String::length, ms, toList() ); 3. Avoid using the diamond (<>) operator (with the ensuing warning about type safety): Collector<String,?,Map<Integer,List<String>>> grouping = groupingBy( String::length, () -> new TreeMap(reverseOrder()), toList() );
(In reply to Marco Torchiano from comment #19) > I was about to file a new bug, but this one seems already on spot, please > correct me if this is not relevant. It may be relevant, let's see. > My basic test case is as follows > > 1: Collector<String,?,Map<Integer,List<String>>> grouping = > 2: groupingBy( > 3: String::length, > 4: () -> new TreeMap<>(reverseOrder()), > 5: toList() > 6: ); > > While the code compiles cleanly with javac 1.8.0_74, on > Eclipse Mars.2 Release (4.5.2) I get the following errors: > > > The method groupingBy(Function<? super T,? extends K>, Supplier<M>, > Collector<? super T,A,D>) in the type Collectors is not applicable for the > arguments (String::length, () -> {}, Collector<Object,capture#6-of > ?,List<Object>>) on line 1 I should warn you, that I'm not immediately convinced that this error is wrong. Compatibility between captures and wildcards is a tricky issue, hardly amenable to common sense reasoning. From your list of solutions / workarounds I'd recommend to use explicit type arguments rather than using a cast. Casts can fail at runtime whereas type arguments simply aid type inference to find the intended solution.
(In reply to Stephan Herrmann from comment #2) > (In reply to Srikanth Sankaran from comment #0) > > We delete the capture bounds right after incorporation so 18.4 almost never > > sees them. This is likely incorrect and unintended in the JLS. We need to > > move to a model that is more closely aligned with JLS and implement the > > fresh captures scheme in 18.4 robustly - this may hinge on > > https://bugs.eclipse.org/bugs/show_bug.cgi?id=447576 also. > > Yes, deleting during incorporate seems wrong. > I made a Q&D experiment: remove the bogus clear compensated by a guarding > set of already-incorporated keys. Alternative implementation: replace the entry#value of a capture bound with a marker instance ALREADY_INCORPORATED. This should be OK, since resolution only needs the key to be present to answer "If the bound set contains a bound of the form G<..., αi, ...> = capture(G<...>) for some i (1 ≤ i ≤ n)". See that the RHS is not even looked at. > I was surprised to see this introduce 22 regressions just in GRT18. Independent of what I replace the captures.clear() with, I get close to 40 regressions in GRT18 as of today. This definitely needs more investigation and coordination with Oracle. Chances for 4.7 are dwindling.
Some comments from today's perspective: (In reply to Srikanth Sankaran from comment #13) > I will summarize instead what changes I had in my early work in progress: > > - We seem to need to integrate inverse bounds right into current bound > set and not maintain them separately. Unfortunately, I don't see what this change would be aiming at. inverseBounds only exist for dependencies between to ivars. I briefly scanned callers of boundsPerVariable.get() but found none that would be interested in a dependency. Should still keep an eye on this suggestion. > - We need to maintain a HashSet<ParameterizedTypeBinding> > for incorporatedCaptures so that we can skip them after incorporating them > once - since we will not clear capture bounds till 18.4 and this needs to > be copied when bound set is cloned. Similar in effect to comment 21 (or vice versa :) ). > - In 18.4, I think we need to create fresh type variables for all > unresolved inference variables and not just the subset constructed by > getSmallestVariableSet(). I disagree. In 18.4 any mention of "fresh type variables" is inside the body of ", let { α1, ..., αn } be a non-empty subset of uninstantiated variables in V such that ...". getSmallestVariableSet() is intended to answer { α1, ..., αn } - so I can see no fault here. > - I had followed Dan's recommendation to not create fresh inference > variables for non-wildcard type arguments in GA. So relevant changes in > BoundSet.incorporate() were in my patch. I'm not sure, why BoundSet.incorporate() was mentioned, I'd expect these changes in ConstraintExpressionFormula.inferPolyInvocationType(), near the invocation of addTypeVariableSubstitutions(). > - Delete SHOULD_WORKAROUND_BUG_JDK_8054721 blocks. OK, depends on the previous item. > - BoundSet.dependsOnResolutionOf seems to require that the else keyword > not be there and the else block be promoted to top level. The logic indeed deserves more scrutiny, but the proposed promotion is unlikely to reflect the spec. My reasoning goes s.t. like this: looking at the definition of dependencies (18.4 initial 4 bullets), we ask whether α depends on β - assuming bullets 2-4 don't answer true - then if α appears on the LHS of a capture bound then β depends on α and *not* vice versa -> false - only if β appears on the LHS of a capture bound then we can conclude α depends on the resolution of β if by swapping α and β one of the four sub bullets matches The latter part seems to be reflected correctly in our code, the former part seems to be lacking a check if α appears on the LHS of a capture bound. A quick experiment in this direction shows no effect in GRT18, which may be a good sign :)
Ran out of time for 4.7. Bulk move to 4.8.
I'm returning here, after bug 528970 reminded me, that part of resolution is hardly ever exercised because of premature deleting of capture bounds. (In reply to Srikanth Sankaran from comment #8) > Documenting email communication from the spec author Daniel Smith@Oracle > here: > > // ----- > > I need to spend some more time with this example to be sure (I'll get back to you), but you may be running into JDK-8054721. > > > > https://bugs.openjdk.java.net/browse/JDK-8054721 > > > > The portion of 18.4 that talks about "fresh type variables" is intended to simulate capture, replacing the placeholder inference variables with actual type variables. But it has some problems, not distinguishing between declared bounds and inferred bounds. > > Okay, yes. That's the bug you want to look at. > > Here's how I'd describe the problem (lowercase for inference vars): > > You have the bound: > > Collector<x1,x2,x3> = capture(Collector<t,?,Map<k,u>>) > > Incorporation (18.3.2) implies: > x1 = t > x3 = Map<k,u> > > Resolution (18.4) claims that x1 depends on t (via the capture bound), but > also t depends on x1 (via the equals bound). Similarly for x3 and k, u. > Now there's a cycle involving a mixture of capture placeholders and normal > inference variables. As pointed out in JDK-8054721, this is bad. > > You can do one of two things: > > - Follow the suggestion in the bug report: resolve the ivars on the RHS (t, > k, and u); perform (real) capture to resolve x1, x2, and x3; then continue > with resolution. > > - Don't bother introducing capture inference variables for non-wildcards. > The end result should be equivalent to what ought to happen if you > introduced them and gave them equality bounds. > Collector<t,x2,Map<k,u>> = capture(Collector<t,?,Map<k,u>>) > > I'm pretty sure javac does the second one. I'm not sure that is good enough > to solve all problems in this space (i.e., there may still be some bugs > until it implements the first one, too), but it addresses the common cases. > // --- I tried the second approach while debugging GRT_18.test499351_extra2(). I found inference failing at this constraint: ⟨X :> Y#1-T#2⟩ which happened while iterating the following set C: ⟨(<no type> t) -> Arrays.asList(t) → Function<? super T#2,? extends U#4>⟩ ⟨(<no type> t) -> Arrays.asList(t) ⊆throws Function<? super T#2,? extends U#4>⟩ ⟨BadInferenceMars451::sum → BinaryOperator<U#4>⟩ ⟨BadInferenceMars451::sum ⊆throws BinaryOperator<U#4>⟩ Dependency analysis put all variables into the same bucket: U#4, R#5, K#3, T#1, T#2, (?)#7, A#6 pickFromCycle selected: ⟨(<no type> t) -> Arrays.asList(t) → Function<? super T#2,? extends U#4>⟩ input variables: T#2 While resolving T#2 a bunch of fresh type variables were introduced (with lower and upper bounds): X <: Y#1-T#2 <: Y#0-T#1 X <: Y#0-T#1 <: Object The next iteration found these Type Bounds: Dependency U#4 :> List<T#8> TypeBound U#4 = List<X> Dependency R#5 = Map<K#3,U#4> Dependency R#5 = Map<Object,U#4> Dependency R#5 = Map<K#3,List<X>> TypeBound R#5 = Map<Object,List<X>> TypeBound R#5 <: Map<Object,List<X>> TypeBound T#8 :> Y#1-T#2 TypeBound T#1 :> X Dependency T#1 <: K#3 TypeBound K#3 :> X TypeBound K#3 = Object TypeBound T#2 :> X Dependency T#2 <: T#1 Dependency T#2 <: K#3 Dependency (?)#7 = A#6 and failed to incorporate those at the constraint mentioned above: ⟨X :> Y#1-T#2⟩ I think a better sequencing might have avoided introducing Y#1 before the final iteration. Note that the connected set of variables includes (?)#7 which is the capture placeholder mentioned by Dan. If I avoid all this by clearing capture bounds, then inference happily instantiates as follows: R#5 : Map<Object,List<X>> A#6 : Object T#2 : X K#3 : Object U#4 : List<X> T#1 : X (?)#7 : Object T#8 : X I should probably use the example from JDK-8054721 to create a minimal trace of the problem.
Too hard to figure out what ecj should be doing using the given advice, with little perceived gain. May have to live with the fact that JLS, javac and ecj define three different languages.
*** Bug 497905 has been marked as a duplicate of this bug. ***
Returning here in preparation for another round of discussion with the spec author. I once more tried the "second thing" from: (In reply to Srikanth Sankaran from comment #8) > You can do one of two things: > > - Follow the suggestion in the bug report: resolve the ivars on the RHS (t, > k, and u); perform (real) capture to resolve x1, x2, and x3; then continue > with resolution. > > - Don't bother introducing capture inference variables for non-wildcards. > The end result should be equivalent to what ought to happen if you > introduced them and gave them equality bounds. > Collector<t,x2,Map<k,u>> = capture(Collector<t,?,Map<k,u>>) > > I'm pretty sure javac does the second one. I'm not sure that is good enough > to solve all problems in this space (i.e., there may still be some bugs > until it implements the first one, too), but it addresses the common cases. For test GRT_18.test499351_extra2() this didn't change the outcome at all. I assume already having the equality bounds mentioned by Dan we have tried all that can be expected from us. Still removing the bogus line from incorporate(): this.captures.clear(); produces lots of regressions, current 46 just in GRT_18.
Some small findings from bug 499955: - in one case I could avoid undesired failure of inference by liberally interpreting this from 18.3.2: " Let R be a type that is not an inference variable (but is not necessarily a proper type)." Maybe CaptureBinding18 should also be exempted from this rule?? - During incorporation of capture bounds one arm invokes reduceOneConstraint() without checking the result - should probably be if (!reduceOneConstraint(..)) return false; - I recall mentioning that javac does not handle any constituents of a capture bound where left is not a wildcard? If we should implement this, one possible location might be in the second attempt of IC18.resolve(): for (int j = 0; j < numVars; j++) { InferenceVariable variable = variables[j]; if (!variable.typeParameter.isWildcard()) continue; //<-added Nothing of this seemed to yield a break through.
