Community
Participate
Working Groups
When investigating differences between javac and ecj, we can identify situation(s) where ecj finds an acceptable solution, which javac does not find, while in fact JLS does not provide an explicit path towards that solution. This bug shall collect deviations that match the above description. Users will typically *not* complain about these, but still being more in line with JLS is desirable.
The example from https://stackoverflow.com/questions/21905169/java8-ambiguity-with-lambdas-and-overloaded-methods demonstrates a situation where users where surprised that javac8 reports ambiguity. Ecj does *not* report this ambiguity, which is nice from a users' p.o.v. but may be wrong from a JLS p.o.v. Reporting ambiguity is avoided by our initial stanza inside Scope.mostSpecificMethodBinding, where we recheck parameterCompatibilityLevel in knowledge of substitutions performed during applicability inference. This stanza is not mandated by JLS, but OTOH, it is required in order to establish javac compatibility for many other situations. Prominent example: bug 429490 required this stanza both for the original example as well as for bug 429490 comment 33. Other changes that lead to the current situation: - bug 427628 - bug 428366 comment 2 Ergo: we should weed out the various reasons why we need that stanza of checks, like: - filter out overridden methods which should never have entered the inference in the first place - avoid ambiguity with some ad-hoc reasoning because javac doesn't report any
(In reply to Stephan Herrmann from comment #1) > The example from > https://stackoverflow.com/questions/21905169/java8-ambiguity-with-lambdas- > and-overloaded-methods demonstrates a situation where users where surprised > that javac8 reports ambiguity. Ecj does *not* report this ambiguity, which > is nice from a users' p.o.v. but may be wrong from a JLS p.o.v. > > Reporting ambiguity is avoided by our initial stanza inside > Scope.mostSpecificMethodBinding, where we recheck > parameterCompatibilityLevel in knowledge of substitutions performed during > applicability inference. This is creating problems for us - https://bugs.eclipse.org/bugs/show_bug.cgi?id=443596 is an example. The materialized PGMB as well as call site arguments have captures and what is a perfectly applicable method gets rejected :( In that bug I also see odd captures like (IIRC) ? extends ? extends ? Type1 & Type2 - does that make sense. Problem is compounded by ICTB not implementing uncapture (yet). ICTB is also not hooked up everywhere in isCompatibleWith and isEquivalent to etc. What would it take to move to reusing the existing Binding.INTERSECTION_TYPE wrapped in a WildcardBinding instead of ICTB Stephan ?
(In reply to Srikanth Sankaran from comment #2) > Problem is compounded by ICTB not implementing uncapture (yet). Does not implement swapResolved either. Just things to watch for.
(In reply to Srikanth Sankaran from comment #2) > What would it take to move to reusing the existing Binding.INTERSECTION_TYPE > wrapped in a WildcardBinding instead of ICTB Stephan ? I see your point. Hm, originally I didn't have my stakes in ICTB, but meanwhile inference is using ICTB in some places as the canonical representation of "intersection types". So, removing ICTB is slightly delicate, but I see no hard reason against. OTOH, when wrapping such thing in a WB, what assumptions are clients making when they see a WB? We won't have meaningful values for rank and typeVariable, but could be harmless? If we set boundKind to EXTENDS, even WB.signature() could work for an intersection type. Maybe, it's even more educating to look at what clients expect when they see Binding.INTERSECTION_TYPE? Anything wildcardish in that area? If so, that could be a killer? If feasible, I'd welcome, if we manage to reduce the number of Binding classes.
(In reply to Srikanth Sankaran from comment #2) > (In reply to Stephan Herrmann from comment #1) > > The example from > > https://stackoverflow.com/questions/21905169/java8-ambiguity-with-lambdas- > > and-overloaded-methods demonstrates a situation where users where surprised > > that javac8 reports ambiguity. Ecj does *not* report this ambiguity, which > > is nice from a users' p.o.v. but may be wrong from a JLS p.o.v. > > > > Reporting ambiguity is avoided by our initial stanza inside > > Scope.mostSpecificMethodBinding, where we recheck > > parameterCompatibilityLevel in knowledge of substitutions performed during > > applicability inference. > > This is creating problems for us - > https://bugs.eclipse.org/bugs/show_bug.cgi?id=443596 is an example. The > materialized PGMB as well as call site arguments > have captures and what is a perfectly applicable method gets rejected :( but where did the PGMB get its captures from? Shouldn't those be identical to the call site arguments in this case? Could be a bug in the area of capture bounds during inference...
(In reply to Stephan Herrmann from comment #4) > OTOH, when wrapping such thing in a WB, what assumptions are clients making > when they see a WB? We won't have meaningful values for rank and > typeVariable, but could be harmless? If we set boundKind to EXTENDS, even > WB.signature() could work for an intersection type. > > Maybe, it's even more educating to look at what clients expect when they see > Binding.INTERSECTION_TYPE? Anything wildcardish in that area? If so, that > could be a killer? Given a WildcardBinding with binding kind set to INTERSECTION_TYPE is what Java 7 engine uses shouldn't we expect all these concerns to be addressed already ? Are there any special requirements for Java 8 case that you can recall off the top of your head that ICTB has been extended to support ?
(In reply to Stephan Herrmann from comment #5) > but where did the PGMB get its captures from? Shouldn't those be identical > to the call site arguments in this case? Could be a bug in the area of > capture bounds during inference... I'll sit down with Shankha next week and understand this in more detail and share my findings.
I am curious about why inferInvocation type is split from PGMB.computeCompatibleMethod - i.e why doesn't PGMB.computeCompatibleMethod return a fully inferred method considering target type et all, as the Java 7 engine does ?
(In reply to Srikanth Sankaran from comment #8) > I am curious about why inferInvocation type is split from > PGMB.computeCompatibleMethod - i.e why doesn't PGMB.computeCompatibleMethod > return a fully inferred method considering target type et all, as the Java 7 > engine does ? Are you questioning the split into 18.5.1 and 18.5.2? Have you seen the argument "inferenceLevel" in PGMB.computeCompatibleMethod?
(In reply to Stephan Herrmann from comment #9) > Have you seen the argument "inferenceLevel" in PGMB.computeCompatibleMethod? Yes. I see that MessageSend never returns a poly type binding. I suspect, this will integrate a lot cleanly if we don't have these levels and PGMB.computeCompatibleMethod - where expression context defines a target type and a target type is available or where expression context does not define a target type - returns a fully instantiated method on success (invocation type inferred) - returns null on failure. - where expression context defines a target type and a target type is not available (invocation context) - performs applicability inference and if it fails return null. - if applicability inference succeeds returns a method it returns today BUT with its return type set to PolyTypeBinding. This would fit well with the general overload resolution scheme which is driven off of two calls: isCompatibleWith and sIsMoreSpecific - which MessageSend would have to implement - there was a patch I posted earlier that had infrastructure support for these. If this is in place, then F in Scope.java does not have to deal with inference at all. All the various rebind methods would go away too and all the inner poly expression resolution can happen in one place in AST.resolvePoly* I'll see if I can get Sasi to prototype this solution and we can see what it looks like and see if it is worth integrating. My gut feel is it will significantly simplify the interaction between F & G and get rid of a few bugs there. Sasi, this would be a very nice project to work on after lambda shape analysis. Let us discuss next week.
Just to document how we got here: I was integrating inference with the general mechanisms of method lookup under the premise to essentially keep the 1.7- code for method lookup as is. If you develop a second universe of the Scope.findMethod family of methods which is structurally closer to the 1.8 spec, this will certainly give a cleaner solution for 1.8 inference, too (for the price of code duplication). I'm not sure if this will completely remove the need for rebinding etc. but we'll find out when we get there, I suppose.
(In reply to Stephan Herrmann from comment #11) > Just to document how we got here: I was integrating inference with the > general mechanisms of method lookup under the premise to essentially keep > the 1.7- code for method lookup as is. > > If you develop a second universe of the Scope.findMethod family of methods > which is structurally closer to the 1.8 spec, this will certainly give a > cleaner solution for 1.8 inference, too (for the price of code duplication). > > I'm not sure if this will completely remove the need for rebinding etc. but > we'll find out when we get there, I suppose. I have made lots of progress on this - so far it looks very promising. I have about 40+ failures to clean up in RunOnlyJava8Tests before declaring victory on the first milestone. I am noticing some "interesting" results produced by inference: Given: List<Person> roster = new ArrayList<>(); Map<String, Person> map = roster .stream() .collect( Collectors.toMap( Person::getLast, Function.identity() )); The PGMB for collect has this signature: Collector<? super Person, Object ,Map<java.lang.String,Person>> as can be seen in the hover. This is wrong. It should be Collector<? super Person, ? ,Map<java.lang.String,Person>> If you extract the toMap call and assign it to a variable of the same type we incorrectly infer: Collector<? super Person, Object ,Map<java.lang.String,Person>> some = Collectors.toMap( Person::getLast, Function.identity() ); This does not compile (correct) While this compiles (correct again) Collector<? super Person, ?,Map<java.lang.String,Person>> some = Collectors.toMap( Person::getLast, Function.identity() ); Due to our bypassing re-evaluation of inner expressions and directly updating the bindings, we are able to get away with this - This shows up as a problem in the new implementation. I'll see what needs to be done.
(In reply to Srikanth Sankaran from comment #10) > Sasi, this would be a very nice project to work on after lambda shape > analysis. > Let us discuss next week. I started looking into what it would take and the scope of it turns out to be very large (my evolving patch modifies ~30 files and the patch is ~4 KLOC) not the best choice for beginner project, so let me handle this and I'll continue to look for interesting work for you and Shankha in other areas.
Created attachment 247456 [details] Evolving solution - with warts, hacks and all. This patch completely rewires the integration between part G & part F. This looks very very promising IMO. I'll write detailed design notes once I confirm it will fly. Still fails 18 tests in RunOnlyJava8Tests - this is under investigation. Stephan, CEF:290 reads: TypeBinding rPrime = rAppl.capture(inferenceContext.scope, 14); // FIXME capture position?? Consider your wrist slapped :) Actually it made me LOL, after a long investigation led me to there :)
One thing that was not clear to me was whether in the current scheme of things, inner poly expressions are fully re-resolved or not. It appears that in some paths we simply update the bindings and not reevaluate the expression against the eventual target type ? If so that would be problematic since there is prologue - find method - epilogue protocol. After the call to findMethodBinding() there is a big body of code in MessageSend for example. I say this because, some of the bindings computed in for example https://bugs.eclipse.org/bugs/show_bug.cgi?id=437444 are wrong and reevaluation would have reported the error - we silently compile the program. This is proving to be a very challenging yet fun work - hopefully it will fly and we will have a very strong and clean integration that should serve us in good stead.
(In reply to Srikanth Sankaran from comment #15) > One thing that was not clear to me was whether in the current scheme of > things, inner poly expressions are fully re-resolved or not. It appears > that in some paths we simply update the bindings and not reevaluate the > expression against the eventual target type ? If so that would be problematic > since there is prologue - find method - epilogue protocol. After the call > to findMethodBinding() there is a big body of code in MessageSend for > example. You're right, this isn't fully kosher. As for checking argument expressions against final target types I'm silently assuming that inference won't produce a solution that would fail such a check. For other checks I have no "excuse". > I say this because, some of the bindings computed in for example > https://bugs.eclipse.org/bugs/show_bug.cgi?id=437444 are wrong and > reevaluation > would have reported the error - we silently compile the program. Which particular check would fail in that example? Historically, when first working on MessageSend I was still hoping that processing order could essentially be maintained as in 1.7- times. When specific problems re diamond allocations occurred, I bit the bullet and split that resolve method into three parts. After doing so for AllocationExpression, it sounds like a good idea to revisit MessageSend and try if the approach from AllocationExpression could be transferred also to MessageSend: separate those parts concerning tentative resolve from those that are done after overload resolution & inference have picked the final method & types.
(In reply to Stephan Herrmann from comment #16) > > I say this because, some of the bindings computed in for example > > https://bugs.eclipse.org/bugs/show_bug.cgi?id=437444 are wrong and > > reevaluation > > would have reported the error - we silently compile the program. > > Which particular check would fail in that example? I was working with so many snippets I don't know where I found the problem, I didn't recheck whether the code in https://bugs.eclipse.org/bugs/show_bug.cgi?id=437444 exhibits the problem, perhaps it does, but ... Any nested inference where the nested call returns a wildcard parameterized type would/should fail re-evaluation of the nested poly method call since re-evaluation of the MessageSend would "recapture" the returned expression and we have no mechanisms in place to recognize they are identical captures of the same wildcard. In the evolving patch, I first attempted to solve the problem by associating the capturing ASTNode with the capture, but this falls flat because we copy lambdas and the original capture during type inference is from an alien parallel universe :) Finally, I have had to change TypeSystem to hand out unique captures on a CUD basis: i.e new CaptureBinding(...) is not safe anymore, LE.createCapturedWildcard should be used instead - this will "intern" captures in such a way that recaptures from the safe site would return the prior capture. Luckily, we didn't need to create parallel Scope's and LEs, so this should work for us. > > Historically, when first working on MessageSend I was still hoping that > processing order could essentially be maintained as in 1.7- times. When > specific problems re diamond allocations occurred, I bit the bullet and > split that resolve method into three parts. > After doing so for AllocationExpression, it sounds like a good idea to > revisit MessageSend and try if the approach from AllocationExpression could > be transferred also to MessageSend: separate those parts concerning > tentative resolve from those that are done after overload resolution & > inference have picked the final method & types.
(In reply to Srikanth Sankaran from comment #17) > Any nested inference where the nested call returns a wildcard parameterized > type would/should fail re-evaluation of the nested poly method call since Correction: The above *and* inference solution on the aggregate basis found some instantiation that involved a captured wildcard that loops through outer and inner.
(In reply to Srikanth Sankaran from comment #18) > (In reply to Srikanth Sankaran from comment #17) > > > Any nested inference where the nested call returns a wildcard parameterized > > type would/should fail re-evaluation of the nested poly method call since > > Correction: > > The above *and* inference solution on the aggregate basis found some > instantiation > that involved a captured wildcard that loops through outer and inner. If it's indeed a type compatibility check post findMethod-with-inference, then performing those checks might be wrong, actually. If inference finds a solution it is correct by definition. We should just trust in its result, no? I thought you might have observed issues regarding visility, staticness, missing enclosing instance or such. Some of those might not be covered by inference, indeed.
(In reply to Stephan Herrmann from comment #19) > If it's indeed a type compatibility check post findMethod-with-inference, > then performing those checks might be wrong, actually. If inference finds a > solution it is correct by definition. We should just trust in its result, > no? I am saying it is just an implementation detail today that will result in inner's reevaluation to fail. If the inner is reevaluated and it applies wildcard capture on its result value - the capture should not act as a fresh capture, that does not make sense but our implementation of PTB.capture() is not set up to handle the case.
Created attachment 247501 [details] More closer to home - Revised patch This patch fails 15 tests in RunAllJava8Tests. At least half of them require simply remastering the expected result - since we now always reevaluate the inner expressions we get additional errors. So very close to being green. I have made sure various experimental changes I had in the earlier version are removed. I plan to release a first batch of infrastructure changes that have no real bearing on the rest of the work - just so we have sharper focus and less distraction. This change set will be carefully selected so the final question of moving forward with solution or not does not become fait_accompli of sorts. Every change pertinent to the applicability :) of the core solution will go through the review process. I'll write up detailed design document and a structured review plan once all tests are green - i.e in some cases it does make sense to view the delta in the comparator, but just read the new code etc.
The scope has broadened. Adjusted the title accordingly.
Created attachment 247615 [details] Revised patch - Just two failures in RunAllJava8Tests Once those are addressed, I'll start the clean up/polish + document the code changes.
Today is the day I am high on questions and low on answers. Down to one failure, but the last one has me in a bind: GRT.testBug431408: Looking at this passage: Otherwise, if R θ is a parameterized type, G<A1, ..., An>, and one of A1, ..., An is a wildcard, then, for fresh inference variables β1, ..., βn, the constraint formula ‹G<β1, ..., βn> → T› is reduced and incorporated, along with the bound G<β1, ...,βn> = capture(G<A1, ..., An>), with B2. I am having trouble mapping this to our implementation: As already discussed and acknowledged 1. there is no capture in our code. 2. I also don't see the bound G<β1, ...,βn> = capture(G<A1, ..., An>) being reduced and incorporated, but could that have been by design ? We instead keep track of this capture bound details in the bound set and handle it during incorporation. Is this indirect strategy because 18.1.3 says: A bound of the form G<α1, ..., αn> = capture(G<A1, ..., An>) indicates that α1, ..., αn are placeholders for the results of capture conversion. This is necessary because capture conversion can only be performed on a proper type, and the inference variables in A1, ..., An may not yet be resolved. 3. R θ is a .... passage calls for fresh variables explicitly, we are reusing existing variables - Is this right ? 4. Should we creating new inference variables for arguments that are plain types ? if R is SomeType<T, String> does it make sense to create an inference variable for String ? 5. Should we be creating fresh variables against the generic type's type variables as opposed to the parameterized type's type arguments ? 6. Does the while loop in incorporate that handles capture bounds effectively amount to the capture bound being incorporated ? 7. There are some passages that talk about all inference variables being instantiated before capture bounds being processed. Are we handling this correctly ?
(In reply to Srikanth Sankaran from comment #24) > Today is the day I am high on questions and low on answers. > > Down to one failure, but the last one has me in a bind: GRT.testBug431408: > > Looking at this passage: > > Otherwise, if R θ is a parameterized type, G<A1, ..., An>, and one of A1, > ..., An is a wildcard, then, for fresh inference variables β1, ..., βn, the > constraint > formula ‹G<β1, ..., βn> → T› is reduced and incorporated, along with the > bound > G<β1, ...,βn> = capture(G<A1, ..., An>), with B2. > > I am having trouble mapping this to our implementation: As already discussed > and acknowledged > > 1. there is no capture in our code. In bug 445274 comment 2 I acknowledged this as a bug. Today, however, I believe this was premature: the word "capture" in these constraints is just a symbol that must not be evaluated, because when processing the capture bound we only operate on the original G, A1, ... An (see, e.g., 18.3.2). No part of the spec seems to use the (evaluated) *type* "capture(G<A1,...An>)". I see it similar to the relation "→throws", all forms listed in 18.1.2 and 18.1.3 are forms of contraints/bounds to be processed by pattern matching according to the rules of chap.18, *not* by regular evaluation. > 2. I also don't see the bound G<β1, ...,βn> = capture(G<A1, ..., An>) being > reduced and incorporated, but could that have been by design ? We instead > keep > track of this capture bound details in the bound set and handle it during > incorporation. > > Is this indirect strategy because 18.1.3 says: > > A bound of the form G<α1, ..., αn> = capture(G<A1, ..., An>) indicates that > α1, ..., αn are placeholders for the results of capture conversion. This is > necessary because capture conversion can only be performed on a proper type, > and the inference variables in A1, ..., An may not yet be resolved. Right, "capture" bounds are handled specifically, these only appear in - 18.1.3 (definition) - 18.3.2 (incorporation) - 18.4 (considered for inference variable dependencies and during actual resolution) - 18.5.2 (produces these bounds) In particular, reduction is applied to *constraints* - capture bounds, however, are already considered as bounds, hence no reduction is necessary in fact. See that also "throws" bounds are handled in a separate structure. > 3. R θ is a .... passage calls for fresh variables explicitly, we are > reusing existing variables - Is this right ? We're looking at this line of code, right? InferenceVariable[] betas = inferenceContext.addTypeVariableSubstitutions(arguments); Inside that method, we essentially create fresh type variables, but indeed we exempt existing inference variables. I'd have to dig deeper to retrieve the case where this exception was needed. Might be worth trying if this exception is still needed. > 4. Should we creating new inference variables for arguments that are plain > types ? if R is SomeType<T, String> does it make sense to create an inference > variable for String ? Here I see some room for interpretation, indeed. spec speaks of the full set of "fresh inference variables β1, ..., βn", without explicit exceptions. I'm already excluding inference variables. When substituting String by βi, aren't we immediately deriving a bound βi = String ? In that case avoiding this βi in the first place might be an optimization, but I wouldn't expect this to change observable behavior. Have you observed differently? Maybe the same reasoning actually holds for existing inference variables, implying it doesn't really matter whether or not we create an inference variable for an inference variable. > 5. Should we be creating fresh variables against the generic type's type > variables as opposed to the parameterized type's type arguments ? I don't think so. Spec explicitly says "R θ is a parameterized type, G<A1, ..., An>". What we need are new bounds that "capture" information about these A1, ... An, which are type arguments. > 6. Does the while loop in incorporate that handles capture bounds effectively > amount to the capture bound being incorporated ? Yes, that loop implements 18.3.2. > 7. There are some passages that talk about all inference variables being > instantiated before capture bounds being processed. Are we handling this > correctly ? Where do you see this? In fact treating capture() purely as a symbol implies that we evaluate all constraints and bounds before actually performing any capture. HTH
(In reply to Stephan Herrmann from comment #25) > In bug 445274 comment 2 I acknowledged this as a bug. Today, however, I > believe this was premature: the word "capture" in these constraints is just > a symbol that must not be evaluated, because when processing the capture > bound we only operate on the original G, A1, ... An (see, e.g., 18.3.2). No > part of the spec seems to use the (evaluated) *type* "capture(G<A1,...An>)". You are right that 18.3.2 operates on original G, However, I don't think capture is just a symbol there - 18.5.2 "Otherwise, if R θ is a parameterized type, G<A1, ..., An>," portion needs a capture for correctness. So I think the right thing to do is to apply the capture, but for 18.3.2 operate on the uncaptured type. > but I wouldn't expect > this to change observable behavior. Have you observed differently? I didn't try - but I expect no change to observable behavior either. > Maybe the same reasoning actually holds for existing inference variables, > implying it doesn't really matter whether or not we create an inference > variable for an inference variable. Would think so. > > 6. Does the while loop in incorporate that handles capture bounds effectively > > amount to the capture bound being incorporated ? > > Yes, that loop implements 18.3.2. OK, more questions. (8) The only place where I see us adding an entry to BoundSet.captures is in inferPolyInvocation* - Are there supposed to be other places that the implementation is missing (I have missed noticing ?). The choice of Set and the iteration makes me expect other. 18.3 preamble talks about "New capture variables are not generated when reducing subtyping constraints". Is this saying no new capture bounds are created ? It says variables though. (9) What necessitates this stashing of the capture bound in BoundSet.captures and processing it during next round of incorporation rather than doing it right away in inferPoly* ? (10) After the round of incorporation, we immediately call captures.clear() Is that correct ? There are quite a few clauses that read like if there exist a capture bound ... for example 18.4 requires it. Should we not be clearing this. but instead be setting a bit to say we have processed the capture bound ?
(In reply to Stephan Herrmann from comment #25) > > 4. Should we creating new inference variables for arguments that are plain > > types ? if R is SomeType<T, String> does it make sense to create an inference > > variable for String ? > > Here I see some room for interpretation, indeed. spec speaks of the full set > of "fresh inference variables β1, ..., βn", without explicit exceptions. I'm > already excluding inference variables. When substituting String by βi, > aren't we immediately deriving a bound βi = String ? Eureka ! > aren't we immediately deriving a bound βi = String ? That holds the key to the puzzle. The capture bound handling code looks like this: while (captIter.hasNext()) { // ... for (int i = 0, length = parameters.length; i < length; i++) { // A set of bounds on α1, ..., αn, constructed from the declared bounds of P1, //..., Pn as described in 18.1.3, is immediately implied. TypeVariableBinding pi = parameters[i]; InferenceVariable alpha = (InferenceVariable) gAlpha.arguments[i]; addBounds(pi.getTypeBounds(alpha, theta), context.environment); TypeBinding ai = gA.arguments[i]; TypeBinding cai = cgA.arguments[i]; if (ai instanceof WildcardBinding) { } else { addBound(new TypeBound(alpha, ai, ReductionResult.SAME), context.environment); } } // -- while this code above would provide for "immediately deriving a bound βi = String" it would not help us derive the bound "βi = capture#1-of ?" in this scenario: On HEAD/master, suppose in CEF.inferPoly* - method's *declared* returnType is Collector<? super T, A, R> - rTheta is Collector<T#0,?,List<T#0>> - gbeta would then be Collector<T#0,?#1,List<T#0>#2> Then in incorporate: - gAlpha is same as gbeta of inferPoly* so == Collector<T#0,?#1,List<T#0>#2> - ga same as parameterizedType of inferPoly* which same as rTheta - So ga == Collector<T#0,?,List<T#0>> Given this and the code above what would allow us to discover "?#1 = capture# of ?" and thereby discover A#1 = capture# of ? Nothing. Ergo. BoundSet.capture should be passed in the parameterized type after wildcard capture and it should arrange to discover "?#1 = capture# of ?" and propagate that. I think we get by on master because, we are not reevaluating inner message sends. So most of the mystery is solved. (In reply to Srikanth Sankaran from comment #26) > (8) The only place where I see us adding an entry to BoundSet.captures is > in inferPolyInvocation* - Are there supposed to be other places that the > implementation is missing (I have missed noticing ?). The choice of Set and > the iteration makes me expect other. OK, I got the answer to this open. Capture bounds get bubbled up from inner poly invocations. So we need the set and the iterations. Makes sense. > (9) What necessitates this stashing of the capture bound in BoundSet.captures > and processing it during next round of incorporation rather than doing it > right away in inferPoly* ? I think I have the answer to this one too. So the one question that still puzzles me is : Are we prematurely clearing BoundSet.captures ? Resolve() checks to see if there are capture bounds - When we enter resolve captures is cleared by prior incorporation. Is it that resolution itself can cause some capture bounds to be created ?
In http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=2156fedddc9b679afe1f45ef1e9d318ac4df4d9a, I release a set of 23 files with changes that enable, but are only peripheral to the core solution. Since of these changes produce a large delta (refactoring of locals into fields for example with the this. qualifier all over the place) they will be a distraction given that reviewer's time is at a premium. Here are the description of the changes: - Enable the test suite harness to support capture free comparison of expected and actual errors. One test in particular (GRT1_8.testBug433158) produces non-deterministic capture ids. - New suite RunOnly335CompilerTests.java to run only a subset of generics and polyexpression tests. - Fix to broken existing test (ASTConverter17.java) caused by bad code in Lambda shape analysis. - Preparing AE, QAE and MessageSend for reentrancy by extracting some locals into fields. (Why doesn't Java support C's function static variables again ? - their semantics cannot be carried over as is since we need to discriminate between instance and class variables, but ...) - Support for "interned captures" at the type system and attendant changes to a couple of files to produce unique capture positions - some expression's source end overlaps the contained expression's source end. BTW ATM this is disabled for null analysis - a couple of tests fail. Seeing that there is some capture binding tweaking code that is null analysis specific, I thought it is best left for you Stephan to determine what is best. - Make CaptureBinding implement substituteInferenceVariable rather than inherit super's method which is wrong for captures. - Fix an NPE potential site (CUScope.java) - A few useful APIs - (Poly)TypeBinding, ProblemMethodBinding etc. Later in the day, I'll post the final patch for review along with a review plan.
Created attachment 247626 [details] Patch containing only test changes. I have tried hard to make sure that diagnostics produced match what we do on master. A few tests need remastering and are in this patch: Description of changes: - A few tests produce 1 less or 1 more message due to inner poly expression's reevaluation as opposed to binding update. These message look reasonable. - GRT1_8.testBug435767 becomes a conform test from being a negative test. JDK 8u20 compiles this code too. - One test NullTAT used to emit duplicate message - went way. I have verified the messages for all changed tests against javac as well as ECJ head. Looks reasonable.
Created attachment 247627 [details] Changed files that need only cursory glance through. These are peripheral changes, but could not be checked in without changes that require deep review going in. A cursory look through is fine for these files.
Created attachment 247628 [details] Changes requiring close inspection. Description: [While I have tried to minimize white space changes, some files still have quite a few noise changes, so set your comparator to ignore white spaces, Thanks] ASTNode.java: - One version of ResolvePolyExpressions went away and the other one got simplified. I suggest that the comparator be used only for structure compare and the (only) method be directly studied - it is only 30 LOC. Invocation.java: - Loses the APIs: - usesInference - updateBindings - innersNeedUpdate - InnerUpdateDone - innerInferenceHelper Again, a structure compare and a glance through would be best. ExplicitConstructorCall.java: - Adjust to changes in Invocation. - ECC not being an expression, some of the Invocation APIs don't make sense. Changed to either just return or throw exception. Scope.java: - We don't use inference levels anymore. In particular F does not drive G. G drives itself. So there are no calls to inferInvocationType from Scope. Nor do we pass around inference levels. - We now skip compatibility checks after PGMB.cCM ONLY if invocation type was inferred. When only applicability is inferred, we fall through and check compatibility. This is so we can weed out incompatible methods that would have passed applicability in G due to arguments being not pertinent to applicability. (I need to think a bit more to see if this calls for additional changes for direct type variables in parameters targetting functional expressions) - Parameter compatibility checks got simplified quite a bit. compatibilityLevel18FromInner and compatibilityLevel18 are not needed anymore. All poly expressions respond to isCompatibleWith and isBoxingCompatibleWith - getExactMethod and getExactConstructor now return a PGMB. Finally ! :) - Ignore inference variables boolean is not used anymore. I don't remember why/how it is not required anymore, but believe me it is not :) - The preamble stanza in mSMB went away. The raison d'etre of this bug - Removing 38 LOC required ~4.5 KLOC and two weeks of time :) MessageSend: - Is designed to be called once or reentered if a poly expression. - Announces itself as a poly type if G finds an applicable method with no target type in invocation context. - Some code for improved error reporting by stealing the closes match. - Implements isCompatibleWith and isBoxingCompatibleWith which is what helps us eliminate all the inner compatibility checks from Scope. (this can trigger needless re-inference, but there is an open bug to address that) QAE: - Loses inner inference helper. AE: - ResolutionState abstraction goes away. The three parts are merged into one part. - Implements isCompatibleWith and isBoxingCompatibleWith - Respond to changes in Invocation. PGMB: - All 1.8 code got moved into cCM18 - This method now returns a PolyParameterizedGenericMethod if applicability succeeded and we have no target type. - This is a "marker" class. It serves to let MessageSend know that it is a poly expression. This is also wrapped as the closest match in case post applicability inference compatibility checks fail so that we have better error reports. - I also expect that this will address ambiguities at mSMB, i.e if we see a PPGMB, simply return that as the MSM and let MessageSend deal with it - not yet done. IC18: - All nested inference handling and inner poly expression rebinding code is eliminated - Now this is the responsibility of ASTNode.resolvePoly* BoundSet: - BoundSet.captures now get passed captured GA. It establishes the trivial relationships between the fresh inference variables and capture(GA) and arranges to propagate these to dependancies. Perhaps this code could be a bit cleaner ? CTF: - Now in formulas we can encounter a PolyTB, if we do, we need to resolve them and discover the real type against the target type. CEF: - In reduce against a proper type, we had several clauses to handle different kinds of expressions. Now we take advantage of polymorphism and ask the expression if it is compatible against the proper type. This addresses some loose ends also that are acknowledged in the code. - Fix to an AIOOB I observed in reference expression reduction. - Capture position 14 fixed :) - Missing capture for inexact method reference fixed. - Pass captured parametrized type for capture bound incorporation - Other capture position problems fixed. Shortly, I'll also post a cumulative patch.
Created attachment 247629 [details] Cumulative patch Recommended structured review plan: 1. (Optional) Glance through the infrastructure changes already released: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=2156fedddc9b679afe1f45ef1e9d318ac4df4d9a 2. (Optional) Glance through the ~12 tests that underwent diagnostic change. https://bugs.eclipse.org/bugs/attachment.cgi?id=247626 3. Cursory review of https://bugs.eclipse.org/bugs/attachment.cgi?id=247627 4. Close scrutiny of the patch https://bugs.eclipse.org/bugs/attachment.cgi?id=247628 documented at https://bugs.eclipse.org/bugs/show_bug.cgi?id=437444#c31. A good order would be ASTNode, Invocation, MessageSend, Scope, PGMB, QAE, AE, ECC, IC18, CEF, CTF, BoundSet, Thanks a million in advance, I know this is a big ask of your time and energy - I appreciate your comments/feedback. IMHO this reintegration is what your effort at G deserves given how well/clean it is written ! I'll get some additional testing done through the UI team as well as other IBM teams.
Noopur, could you please get all UI tests run with the cumulative patch and report back results ? TIA.
OK, so what of the design itself ? Preamble: The basic "problem" is that on master MessageSend does not play the role of a poly expression to perfection i.e it never announces itself as a poly expression and does not implement certain parts of the poly expression protocol (which we never unfortunately documented in an IPolyExpression interface) As a result it does not take advantage of the solution devised for https://bugs.eclipse.org/bugs/show_bug.cgi?id=382701#c54 which is to introduce a level of indirection by implementing isCompatibleWith and isBoxingCompatibleWith For https://bugs.eclipse.org/bugs/show_bug.cgi?id=400874 in https://bugs.eclipse.org/bugs/attachment.cgi?id=237885, I did post some early prototype code for this, but never found the time to follow through :( So the present patch follows through on that. Key ideas: - MessageSend announces itself as a poly expression if G finds an applicable method and has no target type to proceed to invocation type inference. - This is accomplished by PGMB.cCM18 returning an object of a "marker" class PolyParametrizedGenericMethodBinding - MessageSend.binding when called by G against a specific target type computes the binding freshly (this method should be renamed resolveMethodExpecting and LE.getResolved* and RE.findCompile* should also be renamed so they all speak in the same tongue) - By also implementing isCompatibleWith and isBoxingCompatible with this removes any G specific code in F to deal with inner compatibility. (Having that is intrusive there, the reason for this bug.) - All poly expression resolution code is centralized into ASTNode.resolvePoly* - there is no need for IC18 to keep track of inner polies and schedule rebinding etc. In a way the inners are already tracked by the outer in its this.arguments and this.argumentTypes. So this redundancy is removed. - Re-resolving the nested poly expressions after the outer call is fully resolved also addresses problems about various checks being skipped and only bindings being updated. - Diamond inference also is simplified a good bit. Now AE refuses to resolve itself until a target type is known if the context provides a target type. Follow up ideas: (against different tickets) - When target type is absent throwing AbsentTargetTypeException may lead to an even cleaner design. Some open bugs that deal with ambiguity post applicability inference where there should be no ambiguity should go away with that - Write up IPolyExpression. Unify all disparate methods we have under one scheme (resolveTypeExpecting, resolveMethodExpecting, isCompatibleWith, isBoxingCompatibleWith ...) - See if ReferenceExpression should really implement Invocation. We "invoke" a method in RE, not to execute it, but to pass it around. So that should qualify. Too many places there is casts and instanceof checks due to InvocationSite and Invocation split. - All poly expressions should implement a cache against targets to avoid recomputation. - Check that all poly expressions' resolve method looks like: if (this.constant != Constant.NotAConstant) { } else { }
Created attachment 247630 [details] Cumulative patch - Passes all JDT/Core tests (In reply to Srikanth Sankaran from comment #31) > ExplicitConstructorCall.java: > - ECC not being an expression, some of the Invocation APIs don't > make sense. Changed to either just return or throw exception. This was not in the tested version. I added code to throw an ISE when I was scanning through the changes to document them here, bad idea :) Fixed and reposted the cumulative patch that passes all JDT Core tests. (In reply to Srikanth Sankaran from comment #32) > I appreciate your comments/feedback. IMHO this reintegration is what your > effort at G deserves given how well/clean it is written ! That sentence looks subject to interpretation. Just so it does not seem unseemly, by "how well/clean it is written !" I was referring to your work in type inference Stephan, not to my work here :)
For some broken cases, I get an NPE during build. I'll look into this - it does not have to hold the review. // -- import java.util.ArrayList; import java.util.List; import java.util.stream.Collectors; public class X { public static void main(String[] args) { List<Person> roster = new ArrayList<>(); Map<String, Person> map = roster .stream() .collect( Collectors.toMap( Person::getLast, Function.identity() )); } } class Person { }
(In reply to Srikanth Sankaran from comment #36) > For some broken cases, I get an NPE during build. I'll look into this - it > does > not have to hold the review. Trivial problem, I have a fix in my workspace. Basically, when arguments have errors, we do some trickery to find the closest match which happens to be a PGMB. We return to the caller and then outer inference results in binding() and isCompatible et al being called with argumenTypes array containing nulls. Fix is simply to promote the fact we had argument errors to a field and consult in binding and isCom* and do the right thing.
There is a "mild" problem: 15.12.2 says Although the method invocation may be a poly expression, only its argument expressions - not the invocation's target type - influence the selection of applicable methods. // -- This would require a bit of redoing/rethinking. So let us hold on the review. I'll let you know when it is ready.
(In reply to Srikanth Sankaran from comment #38) > There is a "mild" problem: > > 15.12.2 says > > Although the method invocation may be a poly expression, only its argument > expressions - not the invocation's target type - influence the selection of > applicable methods. > > // -- > > This would require a bit of redoing/rethinking. So let us hold on the review. > I'll let you know when it is ready. Damage assessment: Nothing major. Right now I am proceeding to invocation type inference if target type is available, I need to split this as is done on master. This would also simplify the implementation further by a good bit. Invocation.binding() can simply revert to what is doing on master - rather than looking up the method against the target type. I can also eliminate the repeat eventual resolution by memoizing the results from nested inference. It is going to take a couple of days of work - I'll get on it after a day's break. (In reply to Srikanth Sankaran from comment #34) > - MessageSend.binding when called by G against a specific target type > computes the binding freshly This would be a waste of time. The behavior of binding should revert to what it is on head. > - Re-resolving the nested poly expressions after the outer call is fully > resolved also addresses problems about various checks being skipped and only > bindings being updated. This can simply re-use the memoized result. > Follow up ideas: (against different tickets) > > - When target type is absent throwing AbsentTargetTypeException may lead > to an even cleaner design. Some open bugs that deal with ambiguity post > applicability inference where there should be no ambiguity should go away > with that Not sure if this idea will lead to a a conforming implementation.
(In reply to Srikanth Sankaran from comment #33) > Noopur, could you please get all UI tests run with the cumulative patch and > report back results ? TIA. All tests pass with the cumulative patch.
The test cases mentioned in Bug 437973 do not get resolved through this patch.
(In reply to Srikanth Sankaran from comment #38) > There is a "mild" problem: > > 15.12.2 says > > Although the method invocation may be a poly expression, only its argument > expressions - not the invocation's target type - influence the selection of > applicable methods. > > // -- > > This would require a bit of redoing/rethinking. So let us hold on the review. > I'll let you know when it is ready. OK, I have analyzed this closely. No change is required. There are two ways invocation's target type can illegally influence/affect the selection of MSMB. - If we somehow take that into consideration to pick the method - we never did and we don't now with this patch - If invocation type inference "improves" the method and the improved method beats others to quality as the MSMB and it would not otherwise have but for the improvement. The latter point was what worried me - but it proved to be unnecessary. During MSMB computation, we operate on the generic method and not on the instantiations. So any improvements seen during invocation type inference cannot affect overload resolution. While investigating this point though I noticed that my redone Invocation.binding was horrible/dumb incurring expensive computation which would be thrown right away. I have fixed this. Patch will follow shortly.
Created attachment 247687 [details] Proposed patch - Ready for review, passes all tests
(In reply to Srikanth Sankaran from comment #43) > Created attachment 247687 [details] > Proposed patch - Ready for review, passes all tests In http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=817f5b60f1b5283073ee91db13e8dcaec6a51e5d, I released some more infrastructure changes that are peripheral in nature to minimize clutter in the change set input to review. The proposed patch is essentially the same as the "Cumulative patch" posted earlier, with: - Reverting the behavior of Invocation.binding to simply return the binding and not do wasteful computation. - Improved handling of compatibility checks for methods that pass applicability - to weed out incompatible method early. - Some clean up of C set construction based on https://bugs.openjdk.java.net/browse/JDK-8052325 - Some performance improvements to isCompatibleWith - there is no need to tunnel through overload resolution all over again. Known issues I plan to address in a separate ticket along with issues raised during code review: 1. AllocationExpression can use common code blocks replicated in 2-3 places, These could be factored into a method. 2. Further improvements to isCompatibeWith by caching results. 3. When inner is reevaluated, it computes its binding all over. We need to arrange for results forwarded/cached etc. It has been gruelling work to get to this stage, these issues that are not blockers can be addressed separately in a few days time. Ball is back in your court Stephan, at your convenient pace. TIA. I am happy help any way I can, answer questions etc. Let me know.
Created attachment 247700 [details] Incremental fix to bug 428811 This incremental patch fixes bug 428811. Two changes were required: - Earlier "Proposed patch" over eagerly pruned some code necessary to weed out incompatible method from MSMB race. - We attempt to diamond infer when we should not. e.g: ArrayList<T>::new should not trigger diamond inference while ArrayList::new should.
Question: This chunk of code from master in C set construction: if (expri instanceof Invocation && expri.isPolyExpression()) { Invocation invocation = (Invocation) expri; MethodBinding innerMethod = invocation.binding(null, false, null); if (innerMethod instanceof ParameterizedGenericMethodBinding) { InferenceContext18 innerCtx = invocation.getInferenceContext((ParameterizedMethodBinding) innerMethod); if (innerCtx != null) { // otherwise innerMethod does not participate in inference return addConstraintsToC(invocation.arguments(), c, innerMethod.genericMethod(), innerCtx.inferenceKind); } } Is it doing the right thing for diamond inference resolving to a non-generic method ? // -- If ei is a poly class instance creation expression (§15.9) or a poly method invocation expression (§15.12), C contains all the constraint formulas that would appear in the set C generated by §18.5.2 when inferring the poly expression's invocation type. // -- A class instance creation expression is a poly expression (§15.2) if it uses the diamond form for type arguments to the class, and it appears in an assignment context or an invocation context (§5.2, §5.3). Otherwise, it is a standalone expression. So it looks like we should not check for PGMB instance ?
(In reply to Srikanth Sankaran from comment #46) > So it looks like we should not check for PGMB instance ? I think you're right. Originally, Invocation.getInferenceContext() required a PGMB argument, that's why we have this instanceof - using then the innerCtx as an indication that the inner actually uses inference. In Bug 427483 I generalized getInferenceContext() to accept a PMB, and in that bug's commit you see one instanceof check adjusted. It probably makes sense that other similar locations should have been adjusted accordingly, which conceptually acknowledges the spec passages you cite: not PGMB vs. PMB tells us about poly or not poly, but getInferenceContext(PMB) will. OTOH, just changing this one instanceof check may not be effective, unless we also re-visit the use of PGMB in other methods of AllocationExpression: registerInferenceContext(), updateBindings(), usesInference(). Would this change how you handle things in this bug?
(In reply to Srikanth Sankaran from comment #32) > Recommended structured review plan: > > 1. (Optional) Glance through the infrastructure changes already released: > http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/ > ?id=2156fedddc9b679afe1f45ef1e9d318ac4df4d9a Looking at this commit... AllocationExpression: I was curious about moving argumentTypes from ResolutionState to AE itself. When accounting for multiple invocations of AE.resolveType() I figured we should push all intermediate resolution results into the ResolutionState, to allow for different results during different attempts. At a closer look I don't see how different attempts could produce different argumentTypes, though => Looks OK. CaptureBinding: Own version of substituteInferenceVariable() no longer handles superclass & superInterfaces (nor lowerBound, which we didn't handle before, either). Question: can we assert that the fresh CaptureBinding created here gets initializeBounds called when needed? Could we end up working with an insufficiently initialized CaptureBinding downstream? => @Srikanth, please comment. ProblemReasons: InferredAppliableMethodInapplicable => typo :) ParameterizedTypeBinding: capture() has this comment: // A couple of NTAT tests fail with interned captures. The new messages look better and correct. Stephan to validate. => I filed bug 446434 to follow up. No need to let it block this bug. Everything else looks fine with no further questions asked.
Let us start numbering issues so it is easy for follow up and cross referencing: Follow up tasks so far: 1. AllocationExpression uses common code blocks replicated in 2-3 places, These could be factored into a method. 2. Further improvements to isCompatibeWith by caching results. 3. When inner is reevaluated, it computes its binding all over. We need to arrange for results forwarded/cached etc. 4. CaptureBinding.substituteInferenceVariable should check for need to initialize superclass and super interfaces in the substitute. 5. Typo in ProblemReasons:InferredAppliableMethodInapplicable (Clean up of instanceof PGMB check in C set construction included in a patch for the spec bug fix already)
Another question: Should the calls to getResolvedCopyForInferenceTargeting in CExceptionF.reduce and CExpressionF.inputVariables() be preceded by computation of ground target type ? I am not sure there is a correctness issue here - If nothing else they would defeat the caching being put in at least for implicit lambdas with wild card parametrized funcional interface targets ?
Created attachment 247764 [details] Incremental fix to bug 444891 (specification amendment) I am cross posting the patch here because it needs to be applied on top of the others already posted here. I have verified that this patch run all tests clean + fixes bug 433158 bug 435187 bug 433852 bug 442916 bug 442769 bug 432682 bug 442446 bug 435767 which were tagged as duplicates. The one outlier is https://bugs.eclipse.org/bugs/show_bug.cgi?id=439594. This was tagged as a duplicate but problem persists - There should be something more there that should be investigated. I'll reopen that bug shortly.
(In reply to Srikanth Sankaran from comment #51) > Created attachment 247764 [details] > Incremental fix to bug 444891 (specification amendment) > > I am cross posting the patch here because it needs to be applied on top > of the others already posted here. > > I have verified that this patch run all tests clean + fixes > bug 433158 bug 435187 bug 433852 bug 442916 bug 442769 bug 432682 bug 442446 > bug 435767 which were tagged as duplicates. That's very good news, as it shows that both improvements harmonize. Hooray!
(In reply to Srikanth Sankaran from comment #49) > (Clean up of instanceof PGMB check in C set construction included in a > patch for the spec bug fix already) In that universe: do methods AE.{registerInferenceContext,updateBindings,usesInference} still exist? If so, would they still need the same cleanup?
(In reply to Srikanth Sankaran from comment #50) > Another question: > > Should the calls to getResolvedCopyForInferenceTargeting in > CExceptionF.reduce > and CExpressionF.inputVariables() be preceded by computation of ground target > type ? > > I am not sure there is a correctness issue here - If nothing else they would > defeat the caching being put in at least for implicit lambdas with wild card > parametrized funcional interface targets ? Why precede? Inside resolveType we call findGroundTargetType(). Isn't that sufficient? Do you want to use the ground target type as a key for caching? Is the plain target type inappropriate for that task? How?
With the set of patches here, These go away: https://bugs.eclipse.org/bugs/show_bug.cgi?id=445725, https://bugs.eclipse.org/bugs/show_bug.cgi?id=429430 https://bugs.eclipse.org/bugs/show_bug.cgi?id=434394 https://bugs.eclipse.org/bugs/show_bug.cgi?id=445274 https://bugs.eclipse.org/bugs/show_bug.cgi?id=429555 I'll include tests for these in a later patch. Of these, https://bugs.eclipse.org/bugs/show_bug.cgi?id=429430 is interesting. We have a call at the tail end of MessageSend that reads: recordExceptionsForEnclosingLambda(scope, this.binding.thrownExceptions); Inner's full reevaluation enables the propagation of the thrown exceptions now.
(In reply to Stephan Herrmann from comment #54) > Why precede? Inside resolveType we call findGroundTargetType(). Isn't that > sufficient? No, the caching code is going to look like, - check the HashMap<TargetType,LE> and if an entry is found recycle it. - otherwise copy + resolve + add to map and return. > Do you want to use the ground target type as a key for caching? Is the plain > target type inappropriate for that task? How? Thing is from some parts of the engine we call getResolved* with the ground target type and from others (the ones I cited earlier) with wildcards. So these will produce two copies. Inside LE.resolve, we compute the ground target type, but that is after a "cache miss" (In reply to Stephan Herrmann from comment #53) > (In reply to Srikanth Sankaran from comment #49) > > (Clean up of instanceof PGMB check in C set construction included in a > > patch for the spec bug fix already) > > In that universe: do methods > AE.{registerInferenceContext,updateBindings,usesInference} still exist? If > so, would they still need the same cleanup? The last two go away, I think we do the right thing for the first one.
(In reply to Srikanth Sankaran from comment #32) > 2. (Optional) Glance through the ~12 tests that underwent diagnostic change. > https://bugs.eclipse.org/bugs/attachment.cgi?id=247626 At a cursory glance no obvious problems. I'll give it slightly deeper scrutiny after reviewing the actual code changes, at what point I can make more sense of debugging a few selected samples. Some of the additional errors look like candidates for better efforts in detecting the root cause in the presence of inference failure (part 1 of the three-years-AI-research-project :) ). Needn't hold up the current bug.
(In reply to Srikanth Sankaran from comment #56) > Thing is from some parts of the engine we call getResolved* with the > ground target type and from others (the ones I cited earlier) with wildcards. > So these will produce two copies. > > Inside LE.resolve, we compute the ground target type, but that is after a > "cache miss" From all callers of getResolved* this one seems to be the outlier: In CExprF.reduce() inside "if (this.left instanceof LambdaExpression) {" That's the only one using the ground target type, and perhaps unnecessarily so. Do you want to try replacing the 't' argument by 'this.right'? From a spec p.o.v. using 't' may not be necessary, inside resolveType we use the ground target type anyway and if it helps agreeing on one hash key, couldn't this be our friend?
(In reply to Srikanth Sankaran from comment #32) > 3. Cursory review of https://bugs.eclipse.org/bugs/attachment.cgi?id=247627 part of the change in RE has crept into commit 817f5b60f1b5283073ee91db13e8dcaec6a51e5d, needs to be manually merged, when applying the attached patch. One more issue for the list: 6. FE has bogus white space changes (wow I'm finding "problems" :) ) I didn't immediately understand the changes in ProblemReporter.invalidMethod, will return to this later.
(In reply to Stephan Herrmann from comment #58) > In CExprF.reduce() inside "if (this.left instanceof LambdaExpression) {" > That's the only one using the ground target type, and perhaps unnecessarily > so. > Do you want to try replacing the 't' argument by 'this.right'? > > From a spec p.o.v. using 't' may not be necessary, inside resolveType we use > the ground target type anyway It could be too late for inference. I think CEF.fGTT adds additional type variables for explicitly typed lambdas for functional interface parameterization inference - no ?
7. Tighten capture semantics. Right now we use ASTNode, sourceEnd pair. This may not be enough as shown by the changes made for Assignment and CastExpression - for these expressions, the contained expression's source end overlaps the containing expression's source end. A better solution would be to use ASTNode, sourceEnd, sourceStart. 8. Present capture interning code likely to introduce performance problems. For a given wildcard all captures go in the same same linear list across all modules compiled under the same LookupEnv*. Déjà vu all over again. 9. On master we bypass post inference compatibility checks in Scope.cCM. On the proposed solution, we by pass post inference compatibility checks in Scope.cCM if IC18.stepCompleted >= TYPE_INFERRED. This needs to be adjusted so that these checks are not redone for arguments pertinent to applicability as this can wastefully trigger re-inference through isCompatibleWith calls. 10. On master as well on this patch we are skipping 15.12.3 Compile-Time Step 3 for Java 8 ??
Created attachment 247783 [details] Incremental fix to bug 444891 (specification amendment) (In reply to Srikanth Sankaran from comment #51) > Created attachment 247764 [details] > Incremental fix to bug 444891 (specification amendment) > The one outlier is https://bugs.eclipse.org/bugs/show_bug.cgi?id=439594. > This was tagged as a duplicate but problem persists - There should be > something > more there that should be investigated. I'll reopen that bug shortly. Actually, it is very much a duplicate, but was failing due to a small error in the previous patch. (Instead of passing along the boolean `interleaved' I was passing false incorrectly from addConstraintsToC into addConstraintsToC_OneExpr. A one token correction fixes the problem. I should also mention apart from the fix for the spec amendment, this patch also sneaks in: - Lambda copy caching. - A bug fix to lambda shape analysis, - A performance fix to isBoxingCompatibile, It does not make sense for LE, RE and AE and for MS only on occasion (when the target type is a primitive or boxed primitive) We should not trigger expensive reinference in these cases. - Gets rid of a a couple of bogus uncapture() calls. - Cleanups to a couple of places that materialize lambda return expressions in a convoluted way.
(In reply to Stephan Herrmann from comment #59) > 6. FE has bogus white space changes (wow I'm finding "problems" :) ) IIRC, this indentation problem is fixed in one of the later incremental patches. That said, some files have undergone significant change: most notably AE, Scope and IC18 and some amount of noise diff sneaked in - sorry about that,
Created attachment 247816 [details] Incremental fix to bug 432626 During 18.5.2 bullet 3.3.1, we hit a snag. We see two types HashMap<K#8,V#9> and HashMap<K#8,ArrayList<T>> and conclude they are two different parameterizations of the same generic class or interface. But current bound set contains the instantiation V#9 = ArrayList<T> and they are the one and the same. Fix to to compare after application of instatiations. This patch also contains a couple of clean ups in CTF and RTB.
(In reply to Srikanth Sankaran from comment #64) I made a note in bug 428061 comment 8.
Stephan, when only applicability is inferred, would be wrong to leave unresoved type variables instatiated by themselves as opposed to jlO ? This would simplify post inference compatibility checking. What i would like to implement is: - if invocation type inferred, trust inference completely and skip any compatibility checks - If only applicability was inferred, skip compatibility checks for arguments pertinent to applicability, but still check those parameters whose arguments were not pertinent to applicability. The latter step is weed out obviously incompatible cases such as a lambda with a bad target type. By admitting patently incompatible methods we make MSMB deal with them and sometime run into ambiguity or other problems. This scheme would work well, if the PGMB computed after just applicability does not have jlO substitutions, but instead has the unresolved type variables intact. Since we anyway toss out any resolutions and start with the shallow method for further inference, there should be no issues ?
(In reply to Srikanth Sankaran from comment #60) > (In reply to Stephan Herrmann from comment #58) > > > In CExprF.reduce() inside "if (this.left instanceof LambdaExpression) {" > > That's the only one using the ground target type, and perhaps unnecessarily > > so. > > Do you want to try replacing the 't' argument by 'this.right'? > > > > From a spec p.o.v. using 't' may not be necessary, inside resolveType we use > > the ground target type anyway > > It could be too late for inference. I think CEF.fGTT adds additional type > variables for explicitly typed lambdas for functional interface > parameterization inference - no ? To avoid potential misunderstanding: I didn't mean to delete the call to fGTT, only to not use its result 't' in the call to getResolvedCopy*. But I'm not on firm grounds yet ...
(In reply to Srikanth Sankaran from comment #66) > This scheme would work well, if the PGMB computed after just applicability > does not have jlO substitutions, but instead has the unresolved type > variables intact. I'm confused: I understood that after only applicability inference we don't produce any PGMB but a PolyParameterizedGenericMethod? What am I missing? BTW, I plan to get started on the core review on Tuesday. I found your recommendation in comment 32 very helpful. Is this still applicable 30+ comments later?
(In reply to Stephan Herrmann from comment #68) > I'm confused: I understood that after only applicability inference we don't > produce any PGMB but a PolyParameterizedGenericMethod? Yes, but a PPGMB is a PGMB. PPGMB is just a marker class that wraps the PGMB produced by applicability inference. It has no other state or behavior. We may as well set a bit in the PGMB to say only applicability was inferred or alternately consult IC18.stepCompleted to concluded that the MessageSend should announce itself as a poly expression, but using the new "marker" class makes the design intent clear. > BTW, I plan to get started on the core review on Tuesday. Sounds great. > I found your recommendation in comment 32 very helpful. Is this still > applicable 30+ comments later? For the most part should be. I'll take a look and refresh later today if needed.
(In reply to Srikanth Sankaran from comment #66) > Stephan, when only applicability is inferred, would be wrong to leave > unresoved type variables instatiated by themselves as opposed to jlO ? > > This would simplify post inference compatibility checking. I am about to give up on this problem, I am not sure it is solvable without a whole lot of AI. I'll shortly post a patch that treads the middle path by: - Not doing any post inference compatibility checks if full inference was completed. - If only applicability was inferred, skip compatibility checks for all arguments pertinent to applicability. i.e trust inference engine fully for the above scenarios. - When only applicability was inferred, if argument is an functional type, assert that parameter is also and otherwise weed out the method. - If the PPGMB's parameter does not mention jlO, then do full checks for functional expression compatibility. I don't think more can be done easily. The PPGMB is a chimera that doesn't lend itself well to compatibility checks due to partial resolution and object substitutions for the rest.
I'll repost a cumulative patch shortly. To facilitate the review here is a (re) summary of key changes: 1. All poly expression resolution against eventual target type is centralized in one place. ASTNode.resolvePolyExpressionArguments. 2. Poly expressions are reevaluated, we don't just update the bindings. On the +ve side, this enables all the checks that take place post method resolution to take place. On the -ve side, ATM, this means some wasteful reinference, but we have a task in follow up task list to have the results forwarded to avoid reinference. I'll address this as a top priority task post review and release of the work here. 3. Poly allocation expression resolution got overhauled completely. The three part resolution is back as one part in AE.resolveType. 4. Perhaps the biggest change of all is that MessageSend announces itself as poly expression by returning a PolyTypeBinding when required. 5. All poly expressions implement isCompatibleWith and isBoxingCompatibleWith thereby eliminating the need for all the inner invocation compatibility checking code in Scope. The existing Java 7 overload resolution code "just works" given these APIs. 6. In master we skip post inference compatibility checks whether or not full inference is run. We make an improved effort to weed out incompatible methods now with the proposed solution.
Created attachment 247819 [details] Cumulative proposed patch This patch merged all the incremental fixes and also includes a candidate fix for bug 432605 based on the interpretation that a captured wildcard has to be treated as a wildcard for type argument containment assertion purposes. If we hear otherwise from the spec lead, we can back out this change.
Here is the revised structured review plan: (I am halting all work on this so the goal post don't keep shifting) 1. Since your last glance through, the changed test suites: GenericTypeTest.java GenericsRegressionTest_1_8.java LambdaExpressionsTest.java NegativeLambdaExpressionsTest.java NullTypeAnnotationTest.java have had several tests added or enabled. Only a couple of tests got _modified_. These are safe changes and so no need to review these again. For all new and modified tests, behavior has been compared with 8u20. 2. These files have minimal peripheral changes. Some of it you have skimmed through already, some new. It wouldn't hurt to take glance through afresh to regain context. I estimate that 15 minutes should be plenty. Expression.java CodeSnippetScope.java RawTypeBinding.java SyntheticFactoryMethodBinding.java TypeBinding.java ProblemReporter.java FunctionalExpression.java ReferenceExpression.java PolyParameterizedGenericMethodBinding.java PolyTypeBinding.java ProblemReasons.java ConditionalExpression.java LambdaExpression.java QualifiedAllocationExpression.java 3. That leaves us with 11 files requiring close inspection. These can be broken down into an easy set and a more involved set. 3.A Easy set: summary of changes: --------------------------------- (a) ASTNode.java - One version of ResolvePolyExpressions went away and the other one got simplified. I suggest that the comparator be used only for structure compare and the (only) method be directly studied - it is only 30 LOC. (b) Invocation.java: - Loses the APIs: - usesInference - updateBindings - innersNeedUpdate - InnerUpdateDone - innerInferenceHelper and a couple of other APIs lose a parameter or two. Again, a structure compare and a glance through would be best. (c) ExplicitConstructorCall.java: - Adjusts to changes in Invocation. - ECC not being an expression, some of the Invocation APIs don't make sense. Changed to either just return. (d) MessageSend.java (ignore whitespace in comparator) - Announces itself as a poly expression when required. - Implements isCompatibleWith and isBoxingCompatibleWith - Adjusts to Invocation interface changes. - Loses inner inference helper. 3.B More involved set with description of changes: -------------------------------------------------- (a) BoundSet.java: - Capture bound handling changes. - Correction to 18.5.2 bullet 3.3.1 handling to consult instantiations. (b) ConstraintTypeFormula.java: - Experimental fix for bug 432605 that treats as a captured wildcard as a wildcard for type argument containment assertions. - Handling POLY_TYPEs inside the engine in type equations. - isInsignificantParameterized eliminated and replaced with a call to TypeBinding.isParameterizedWithOwnVariables. (c) ConstraintExpressionFormula.java: - Reduction against a proper type is handled polymorphically via isCompatibleWith and isBoxingCompatibleWith. - Pass proper scope to Invocation.binding() - For inner invocations interleaved by a lambda, reduce is a nop as the C set construction code has effectively lifted everything that needs to be lifted that would amount to B3. We must lift them at C set construction time because type variables would need substitution with inference variables before they get on the C set. - Lambda return expressions directly materialized with resultExpressions - Fixes to reference expression reduction to avoid AIOOB, implement missing/inappropriate captures, to not treat ArrayList<T>::new as diamond inference, proper handling of capture bounds in inferPolyInvocationType etc. (d) ParameterizedGenericMethodBinding.java: - All 1.8+ code moved into cCM18 - We don't use inference level anymore. If target type is available or if context does not provide a target type, we proceed to infer invocation type. (e) Scope.java - Inference level not passed around anymore. - Various places that would call inferInvocation type don't do that anymore since PGMB.cCM18 already proceeds to do that. - Make best effort basis compatibility checks post applicability inference. - parameterCompatibilityLevel18, compatibilityLevel18FromInner and some helpers used by them are eliminated. - getExactMethod and getExactConstructor return a PGMB when required. - mSMB leading stanza got simplified/eliminated. - parameterCompatibilityLevel(MethodBinding, TypeBinding[], boolean) makes an effort to weed out methods that are blatant mismatches. - Other assorted obvious changes. (f) AllocationExpression.java (This file is easier directly reviewed) - We don't use ResolutionState abstraction anymore. - Loses inner inference helper. - Three part resolution merged into one. - Implements isCompatibleWith and isBoxingCompatibleWith - Adjusts to changes in Invocation. (g) InferenceContext18.java: - Loses a bunch of abstraction, state and behavior dealing with inner poly expression rebinding. - Corrections to javadoc and comments to bring it up to date - Support for nested inference with an interleaving lambda. This calls for us to lift the type variables and initial bounds from inner to current context. This needs to happen here and now during C set construction because the formulas that get on the C set should see the inference variable and not the type variables. So input variables and output variables could be computed correctly and formulas could be sequenced/scheduled properly for reduction to make progress. That should be it. Thanks Stephan. As promised, I'll freeze on the changes until the review is over and not go on sneaking in more and more. We have rounded up all the nasties anyways.
Upto date list of follow up tasks: Follow up tasks so far: 1. AllocationExpression uses common code blocks replicated in 2-3 places, These could be factored into a method. 2. Further improvements to isCompatibeWith by caching results. 3. When inner is reevaluated, it computes its binding all over. We need to arrange for results forwarded/cached etc. 4. CaptureBinding.substituteInferenceVariable should check for need to initialize superclass and super interfaces in the substitute. 5. Tighten capture semantics. Right now we use ASTNode, sourceEnd pair. This may not be enough as shown by the changes made for Assignment and CastExpression - for these expressions, the contained expression's source end overlaps the containing expression's source end. A better solution would be to use ASTNode, sourceEnd, sourceStart. 6. Present capture interning code likely to introduce performance problems. For a given wildcard all captures go in the same same linear list across all modules compiled under the same LookupEnv*. Déjà vu all over again. 7. See if we can improve further on post applicability compatibility checks. 8. On master as well on this patch we are skipping 15.12.3 Compile-Time Step 3 for Java 8 ??
I propose that we address only absolute ship stoppers before release and move all others to post release in a follow up ticket. Thanks.
Created attachment 247850 [details] Cumulative proposed patch (In reply to Srikanth Sankaran from comment #72) > Created attachment 247819 [details] > Cumulative proposed patch > > This patch merged all the incremental fixes and also includes a candidate > fix for bug 432605 based on the interpretation that a captured wildcard > has to be treated as a wildcard for type argument containment assertion > purposes. If we hear otherwise from the spec lead, we can back out this > change. I heard back from the spec lead that a captured wildcard is to be treated as a type and not as a wildcard. Happily we don't need to deviate in our treatment to fix https://bugs.eclipse.org/bugs/show_bug.cgi?id=432605. The whole confusion came about because of my disabling of interned captures while null analysis is turned on (bug 446434). The patch has been updated to backout this hacky/unwarranted change.
(In reply to Srikanth Sankaran from comment #76) > Created attachment 247850 [details] > Cumulative proposed patch All JDT UI tests are green with this patch.
(In reply to Noopur Gupta from comment #77) > All JDT UI tests are green with this patch. Thanks Noopur. Jay has offered to verify that we build JRE8 alrigtht with this patch and Shankha will work with other IBM departments to get some stress testing done. Thanks Jay and Shankha.
Defining the goal post to be attachment 247850 [details] from comment 76 (and assuming bug 446434 has been taken care of - test result pending) I'm starting the main review now.
RawTypeBinding does participate in capture() (inherited from PTB), but in uncapture() we explicitly opt out of the inherited behavior. Do you have a quick explanation, why? I wouldn't be worried, were it not for RawTypeBinding.initializeArguments() ...
(In reply to Srikanth Sankaran from comment #71) > 2. Poly expressions are reevaluated, we don't just update the bindings. > On the +ve side, this enables all the checks that take place post method > resolution to take place. On the -ve side, ATM, this means some wasteful > reinference, but we have a task in follow up task list to have the results > forwarded to avoid reinference. I'll address this as a top priority task > post review and release of the work here. Looking at the removal of SyntheticFactoryMethodBinding.applyTypeArgumentsOnConstructor(): This method was introduced to obtain a useful map key for AE.inferenceContexts After the removal we are in danger not to find some registered inference contexts (original key: factory method, lookup key: constructor). Could this difference be masked by the fact that now from ASTNode.resolvePolyExpressionArguments() we directly go into: ParameterizedGenericMethodBinding.computeCompatibleMethod18(MethodBinding, TypeBinding[], Scope, InvocationSite) line: 166 ParameterizedGenericMethodBinding.computeCompatibleMethod(MethodBinding, TypeBinding[], Scope, InvocationSite) line: 81 MethodScope(Scope).computeCompatibleMethod(MethodBinding, TypeBinding[], InvocationSite, boolean) line: 733 MethodScope(Scope).computeCompatibleMethod(MethodBinding, TypeBinding[], InvocationSite) line: 690 MethodScope(Scope).getStaticFactory(ParameterizedTypeBinding, ReferenceBinding, TypeBinding[], InvocationSite) line: 4947 AllocationExpression.inferElidedTypes(ParameterizedTypeBinding, ReferenceBinding, TypeBinding[], Scope) line: 562 AllocationExpression.resolveType(BlockScope) line: 470 ASTNode.resolvePolyExpressionArguments(Invocation, MethodBinding, TypeBinding[], BlockScope) line: 674 ... without any attempt to re-use the existing inference contexts? Did the removed method create any trouble? Otherwise we might choose to keep it (incl. its use) to avoid bad surprises when later we try to restore some of the caching-related code?
On error reporting: I see in the patch: - some additional secondary errors in test cases - removal of an attempt in ProblemReporter to delegate reporting to an outer inference context to avoid secondary errors. Is this tendency to report more errors intended in terms of user experience or a result from technical considerations?
Nitpick re ConditionalExpression: isPertinentToApplicability() has comment: "// not perfect." From my p.o.v. this implementation *is* perfect, if you see reason against please specify :) isFunctionalType() uses || where I'd expect &&.
Nitpick re ASTNode: Change removes the attempt to fetch variableArity from an existing inference context. If such inference context exists, infCtx.isVarArgs() is authoritative. The remaining check is best effort to recover this information. I have no counter example, so the common sense checks *may* be sufficient?
More re ASTNode: Removed javadoc snippet seems to be still relevant: * If this resolving produces better types for any arguments, update the 'argumentTypes' array in-place as an * intended side effect that will feed better type information in checkInvocationArguments() and others. Regarding this check: if (!lambda.isCompatibleWith(parameterType, scope) || lambda.hasErrors()) I found no test requiring the first part. Is it needed? I was curious how we could get into that situation.
ExplicitConstructorCall: Why doesn't this remember any inference contexts any more? I'd assume that test coverage for this part is low, so not seeing a test that needs it would be a weak reason for removing functionality, IMHO.
I was thinking about the type of PolyTypeBinding.expression: I thought the design might be clearer if we invent an interface (PolyExpression or such) and move/copy all methods required by PolyTypeBinding into that interface. Unfortunately, methods like Expr.isCompatibleWith are used in a few contexts where arbitrary expressions are possible, not just potential PolyExpressions. While the group of nodes that have meaningful implementations is small, requiring additional (expr instanceof PolyExpression) checks might be overkill. Just thinking aloud.
(In reply to Stephan Herrmann from comment #80) > RawTypeBinding does participate in capture() (inherited from PTB), but in > uncapture() we explicitly opt out of the inherited behavior. Do you have a > quick explanation, why? > > I wouldn't be worried, were it not for RawTypeBinding.initializeArguments() > ... I don't think it matters in terms of observable behavior, but for clarity's sakes, RTB should override capture to return itself unmodified. I don't understand what RTB.initializeArguments purports to do, but in any event it cannot introduce a wildcard into the picture so leaving capture to be inherited from PTB should have no effect, but I'll override it in RTB. (In reply to Stephan Herrmann from comment #81) > Did the removed method create any trouble? Otherwise we might choose to keep > it (incl. its use) to avoid bad surprises when later we try to restore some > of the caching-related code? Sounds good, I'll add it to the follow up tasks. (In reply to Stephan Herrmann from comment #82) > Is this tendency to report more errors intended in terms of user experience > or a result from technical considerations? The honest answer is it is neither. I made a good bit of effort to minimize diagnostic differences and we are where we are with this patch after such effort. I think we should take a fresh look at this whole issue separately perhaps under https://bugs.eclipse.org/bugs/show_bug.cgi?id=406614 https://bugs.eclipse.org/bugs/show_bug.cgi?id=404675 https://bugs.eclipse.org/bugs/show_bug.cgi?id=428489 https://bugs.eclipse.org/bugs/show_bug.cgi?id=400831 merged into one project. (In reply to Stephan Herrmann from comment #83) > Nitpick re ConditionalExpression: > > isPertinentToApplicability() has comment: "// not perfect." > From my p.o.v. this implementation *is* perfect, if you see reason against > please specify :) I think this one is fine and the comment can be deleted. > isFunctionalType() uses || where I'd expect &&. Here it is intentional even if it seems a bit odd for what could be seen to be an API. isFunctionalType() is used eliminate incompatible methods in scope.cCM. If one arm of the ternary is a functional type, that is enough to assert that the parameter should be a functional interface. Post applicability inference, I am checking only functional arguments for compatibility. Checking more/all arguments is not incorrect, but a waste of time. I see two possibilities: Rename the method to be mayBeFunctionalType or kindaSortaFunctionalType :) or document that CE behaves this way, so if in future we make use of this API for additional purposes, there is documentation. (In reply to Stephan Herrmann from comment #84) > Nitpick re ASTNode: > > Change removes the attempt to fetch variableArity from an existing inference > context. I think it should be restored along with doing what the TODO suggests. (Long ago there was a mechanism to let the expressions know they are arguments at the ellipsis position.) Alternately we should be able to trivially determine if variable arity was a concern in overloading/type inference by simply checking the argument to see if it is an array or element. (In reply to Stephan Herrmann from comment #85) > More re ASTNode: > > Removed javadoc snippet seems to be still relevant: > * If this resolving produces better types for any arguments, update the > 'argumentTypes' array in-place as an > * intended side effect that will feed better type information in > checkInvocationArguments() and others. Correct, this should be reinstated. > Regarding this check: > if (!lambda.isCompatibleWith(parameterType, scope) || lambda.hasErrors()) > I found no test requiring the first part. Is it needed? I was curious how we > could get into that situation. Actually, I put it in there because some test produced more/less messages than required, don't recall which - it may not have been from the suite, but from some code I played with in the IDE. The thing is LE.resolveType can finish successfully when it should not - for example, value/void compatibility is not checked during resolve and so hasErrors() will return false if that is the only sort of error. Once Sasi's work on lambda shape analysis is integrated, this should be a non-issue. The new analyzeShape() phase can be initiated right within LE.resolveType (In reply to Stephan Herrmann from comment #86) > ExplicitConstructorCall: > > Why doesn't this remember any inference contexts any more? > > I'd assume that test coverage for this part is low, so not seeing a test > that needs it would be a weak reason for removing functionality, IMHO. ECC is not an expression but a statement ? It can never be an inner/nested poly expression. So I felt there is no use in preserving the context. However I already see one use for the inference context. Per earlier /**/ in ASTNode.resolvePoly* if we want to check if the method should be seen to be varargs, it could be useful. However, I think we can simply check the argument and parameter to see if argument is an array or element type. expression. Based on how we resolve the ASTNode.resolvePoly* issue, we can decided on restoring this. (In reply to Stephan Herrmann from comment #87) > I was thinking about the type of PolyTypeBinding.expression: I thought the > design might be clearer if we invent an interface (PolyExpression or such) > and move/copy all methods required by PolyTypeBinding into that interface. > > Unfortunately, methods like Expr.isCompatibleWith are used in a few contexts > where arbitrary expressions are possible, not just potential > PolyExpressions. While the group of nodes that have meaningful > implementations is small, requiring additional (expr instanceof > PolyExpression) checks might be overkill. > > Just thinking aloud. I think it is useful to provide a IPolyExpression interface that is implemented by the 5 ASTNodes - with "default methods" in Expression that are overridden in specific types. If nothing else, it makes the design clear and will also help unify the rest of the work. E.g: Invocation.binding(), LE.getResolvedCopy* and RE.findCompile* should all ideally have the same name.
(In reply to Srikanth Sankaran from comment #88) > Invocation.binding(), LE.getResolvedCopy* and RE.findCompile* should > all ideally have the same name. BTW, we have punted so far on the issue of what should be the return type of LE.resolveType and RE.resolveType - It should ideally be a new abstraction FunctionTypeBinding which is a subtype of SourceTypeBinding. There is a skeleton in LambdaExpression.getTypeBinding().LambdaTypeBinding.
See also https://bugs.eclipse.org/bugs/show_bug.cgi?id=429430#c35. But this is a separate project - not an urgent one.
A change in BoundSet reminded me of this "ancient" question: (In reply to Srikanth Sankaran from comment #26) > (10) After the round of incorporation, we immediately call captures.clear() > Is that correct ? There are quite a few clauses that read like if there exist > a capture bound ... for example 18.4 requires it. Should we not be clearing > this. but instead be setting a bit to say we have processed the capture > bound ? I guess this is still valid, right? Indeed incorporation makes no mentioning of deleting capture bounds, only resolution does.
(In reply to Stephan Herrmann from comment #91) > A change in BoundSet reminded me of this "ancient" question: > > (In reply to Srikanth Sankaran from comment #26) > > (10) After the round of incorporation, we immediately call captures.clear() > > Is that correct ? There are quite a few clauses that read like if there exist > > a capture bound ... for example 18.4 requires it. Should we not be clearing > > this. but instead be setting a bit to say we have processed the capture > > bound ? > > I guess this is still valid, right? > > Indeed incorporation makes no mentioning of deleting capture bounds, only > resolution does. Yes, at the moment, the patch retains the behavior on master. We don't have a test case that establishes that clearing them after incorporation is a problem.
Diving into the changes in BoundSet, apparently we need to resume the discussion in comment 24 - comment 27. (In reply to Srikanth Sankaran from comment #26) > (In reply to Stephan Herrmann from comment #25) > > > In bug 445274 comment 2 I acknowledged this as a bug. Today, however, I > > believe this was premature: the word "capture" in these constraints is just > > a symbol that must not be evaluated, because when processing the capture > > bound we only operate on the original G, A1, ... An (see, e.g., 18.3.2). No > > part of the spec seems to use the (evaluated) *type* "capture(G<A1,...An>)". > > You are right that 18.3.2 operates on original G, However, I don't think > capture is just a symbol there - 18.5.2 "Otherwise, if R θ is a parameterized > type, G<A1, ..., An>," portion needs a capture for correctness. So I think > the right thing to do is to apply the capture, but for 18.3.2 operate on > the uncaptured type. I'm not convinced by your reasoning. If I say "x is bounded by the capture of y" there's nothing requiring me to actually compute the "capture of y". Performing capture and uncapture is a noop, unless someone really refers to "capture of y" as a type. I can't find this in the spec. Interestingly, reverting this change causes regressions, because of the way you continue using cgA (inside BoundSet.incorporate), so let's include that in the discussion: (In reply to Srikanth Sankaran from comment #27) > ... > Given this and the code above what would allow us to discover > > "?#1 = capture# of ?" and thereby discover A#1 = capture# of ? > > Nothing. > > Ergo. > > BoundSet.capture should be passed in the parameterized type after wildcard > capture and it should arrange to discover "?#1 = capture# of ?" and propagate > that. Your inventing! :) Do you have any backing by the spec, or can you get it from Oracle?
(In reply to Stephan Herrmann from comment #93) > I'm not convinced by your reasoning. If I say "x is bounded by the capture > of y" there's nothing requiring me to actually compute the "capture of y". > Performing capture and uncapture is a noop, unless someone really refers to > "capture of y" as a type. I can't find this in the spec. The specification calls for "Otherwise, if R θ is a parameterized type, G<A1, ..., An>, and one of A1, ..., An is a wildcard, then, for fresh inference variables β1, ..., βn, the constraint formula ‹G<β1, ..., βn> → T› is reduced and incorporated, along with the bound G<β1, ..., βn> = capture(G<A1, ..., An>), with B2. I think this is as explicit as it needs to get ? That we add the capture bound to a set and incorporate later is an implementation detail. What would reducing and incorporating the bound G<β1, ..., βn> = capture(G<A1, ..., An>) right then and there do/look like ? > Interestingly, reverting this change causes regressions, because of the way > you continue using cgA No, I don't think it is because of the way ... It is because that is what is required to be done :) See there needs to NECESSARILY be a capture of the return type as it is an expression and so and must go through capture conversion. Capture there is not a symbol - what else would/could it symbolize ? This is the only mention of capture there. > Do you have any backing by the spec, or can you get it from Oracle? This looks too cut and dry IMHO to escalate ? Just as a parallel see that in many other places the spec calls for captures - in reduce reference expression compatibility for one example.
(In reply to Srikanth Sankaran from comment #94) > (In reply to Stephan Herrmann from comment #93) > > > I'm not convinced by your reasoning. If I say "x is bounded by the capture > > of y" there's nothing requiring me to actually compute the "capture of y". > > Performing capture and uncapture is a noop, unless someone really refers to > > "capture of y" as a type. I can't find this in the spec. > > The specification calls for > > "Otherwise, if R θ is a parameterized type, G<A1, ..., An>, and one of A1, > ..., An is a wildcard, then, for fresh inference variables β1, ..., βn, the > constraint formula ‹G<β1, ..., βn> → T› is reduced and incorporated, along > with the bound G<β1, ..., βn> = capture(G<A1, ..., An>), with B2. > > I think this is as explicit as it needs to get ? This still doesn't convince me, but debugging this disagreement may indeed be beside the point. The main point I'm objecting is the additional formula you seem to be inventing inside incorporate() (and which uses the capture): if (!reduceOneConstraint(context, ConstraintTypeFormula.create(bound.right, cai, ReductionResult.SAME))) return false; Do you have a reference for this?
Oops, this typo might be significant: (In reply to Stephan Herrmann from comment #93) > Your inventing! :) Meant to say: "You're inventing", implying that inventing new rules is s.t. we are not allowed to do.
Inside ConstraintTypeFormula.reduceSubType() there's a new case Binding.POLY_TYPE: I wanted to watch this code block live and in color, but RunAllJava8Tests doesn't seem to trigger this. Do you recall if this was required by any tests or added just to keep us honest about all possible cases?
(In reply to Stephan Herrmann from comment #95) > This still doesn't convince me, but debugging this disagreement may indeed > be beside the point. > > The main point I'm objecting is the additional formula you seem to be > inventing inside incorporate() (and which uses the capture): > > if (!reduceOneConstraint(context, > ConstraintTypeFormula.create(bound.right, cai, ReductionResult.SAME))) > return false; > > Do you have a reference for this? Let us go back to the your comment#25: (In reply to Stephan Herrmann from comment #25) >Here I see some room for interpretation, indeed. spec speaks of the full > set of "fresh inference variables β1, ..., βn", without explicit >exceptions. I'm >already excluding inference variables. When substituting >String by βi, aren't >we immediately deriving a bound βi = String ? Let us ignore What ever is the means by which we would derive the bound βi = String. It is a requirement for inference to succeed that βi must be found to be String. In a similar fashion, in these programs that fail without the change under discussion, we must discover that "?#1 = capture# of ?" and therefore discover A#1 = capture# of ? Now we can certainly argue/debate whether the code change in place is the best way to get it done. I am open to suggestions on improvements or a cleaner fix - Just that not applying capture and dropping a vital clue to solving the inference puzzle will necessarily mean we will fail.
(In reply to Stephan Herrmann from comment #97) > Inside ConstraintTypeFormula.reduceSubType() there's a new > case Binding.POLY_TYPE: > > I wanted to watch this code block live and in color, but RunAllJava8Tests > doesn't seem to trigger this. Do you recall if this was required by any > tests or added just to keep us honest about all possible cases? Strange, I did add it after some test failed ! I didn't find it surprising that this block would be required there, because now MessageSend returns a PolyTB.
(In reply to Stephan Herrmann from comment #93) > > BoundSet.capture should be passed in the parameterized type after wildcard > > capture and it should arrange to discover "?#1 = capture# of ?" and propagate > > that. > > Your inventing! :) Actually adding a bound just for "?#1 = capture# of ?" should suffice as incorporation should take care of the rest - that surely should not be objectionable ?
Here's how the contention could potentially be resolved: --------------------------- Change 18.3.2 Add before the bullets: Let G<T1,...,Tn> be a capture of G<A1,...,An>. Change bullet 1 from * If Ai is not a wildcard, then the bound αi = Ai is implied. to * If Ai is not a wildcard, then the bound αi = Ai is implied. Otherwise, the bound αi = Ti is implied. Change bullet 2.1 from * αi = R implies the bound false to * αi = R, if R is not a wildcard, implies the bound false --------------------------- This would match to your implementation (taking comment 100 into consideration) and passes most [1] tests. We should suggest this to Dan. I expect that he will agree that s.t. is missing, but he might come up with a different fix for the problem. [1] GenericsRegressionTest.testBug431408() indicates that the additional bound αi = Ti must only be added inside this branch: if (ai instanceof WildcardBinding) { ... if (three != null) { ... if (three.sameBounds != null) { // HERE I don't have an explanation for this, but I have to stop for today.
See https://bugs.openjdk.java.net/browse/JDK-8055963 and https://bugs.openjdk.java.net/browse/JDK-8056092.
(In reply to Stephan Herrmann from comment #101) > We should suggest this to Dan. > > I expect that he will agree that s.t. is missing, but he might come up with > a different fix for the problem. I am prototyping a solution that would follow the spec closely and eliminate deviations however minor. i.e - Create fresh inference variables, not recycle. - Not apply capture at inferPolyInvocationType - I think JLS is clear that capture should not be attempted on non-proper types. - Eliminate the changes I made to incorporate dealing with captures and the propagation by reducing new constraints. - Not clear captures after incorporation. - In Resolve, CaptureBinding18 should be created with new but with a factory method that would ensure internment. Let us see if toeing the line in toto helps.
(In reply to Srikanth Sankaran from comment #103) > - In Resolve, CaptureBinding18 should be created with new but with Should not be created with new ...
(In reply to Srikanth Sankaran from comment #103) > Let us see if toeing the line in toto helps. Looks promising on first glance, IC18.freshCapture has some problems. CaptureBinding.sourceType is set to this.scope.enclosingSourceType(). Can't be good :)
(In reply to Srikanth Sankaran from comment #105) > (In reply to Srikanth Sankaran from comment #103) > > > Let us see if toeing the line in toto helps. > > Looks promising on first glance, IC18.freshCapture has some problems. > CaptureBinding.sourceType is set to this.scope.enclosingSourceType(). > > Can't be good :) Hmm, that in itself is OK, but I am checking this part of the code anyways.
Created attachment 247885 [details] Patch to remove deviations. This patch removes a few minor deviations in existing code on master as well backs out my ad-hoc experimentation. - Fresh variables are created for capture bound - We used to not call incorporate from reduceAndIncorporate. Fixed. - Don't apply captures on non-proper types. - Not clear captures after incorporation. Why would this code fail LET.testCapture ? Perhaps we will get answer if we study that. We fail reducing this constrain where the RHS is a Capturebinding18. I don't yet know what to make of this: Type Constraint: ⟨java.util.Map<K#3,U#4> = <Z#7-Map<K#3,U#4>#7>⟩
(In reply to Srikanth Sankaran from comment #107) > We fail reducing this constrain where the RHS is a Capturebinding18. I don't > yet know what to make of this: > > Type Constraint: > ⟨java.util.Map<K#3,U#4> = <Z#7-Map<K#3,U#4>#7>⟩ Analysis: // -- 18.4: If the bound set contains a bound of the form G<..., αi, ...> = capture(G<...>) for some i (1 ≤ i ≤ n), or; If the bound set produced in the step above contains the bound false; then let Y1, ..., Yn be fresh type variables whose bounds are as follows: // -- On master due to our premature clearing of captures from the boundset, we never reach the "let Y1 .. Yn" portion with capture bounds intact. We reach this portion of code only for the "Or; if the bound set ... contains the bound false" portion. There are only 4 tests in RunAllJava8Tests all from GRT1_8 viz testBug426671_ok testBug426671_medium testBug426671_full testBug426540 that ever reach here first having created fresh inference variables in CEF.inferPolyInvocationType. But in all these cases, we reach 18.4 from IC18.inferInvocationType with a request to resolve a subset of inference variables none of which are the fresh inference variables created in CEF.inferPolyInvocation* As a result, we have never encountered certain situations with additional fresh inference variables and dependencies between them in this portion of IC18.resolve() Now that I have disabled premature clearing of captures, incorporation is spewing out formulas with CaptureBinding18 in them that CTF is not prepared to handle. From my reading of 18.4, ATM, it looks like there is no gap in spec and we should be able to solve the problem without escalation - Let us see if this bears out to be true.
(In reply to Srikanth Sankaran from comment #108) > Now that I have disabled premature clearing of captures, incorporation > is spewing out formulas with CaptureBinding18 in them that CTF is not > prepared to handle. > > From my reading of 18.4, ATM, it looks like there is no gap in spec and > we should be able to solve the problem without escalation - Let us see > if this bears out to be true. I tied this - unfortunately, this requires a fair amount of invention/interpretation along the way. For example, how are we supposed to reduce this: ⟨java.util.Map<K#3,U#4> = <Z#8-Map<K#3,U#4>#7 extends java.lang.Object>⟩ ? RHS is CB18, LHS is PTB. We don't enter: if ((this.left.isClass() || this.left.isInterface()) && (this.right.isClass() || this.right.isInterface()) && TypeBinding.equalsEquals(this.left.erasure(), this.right.erasure())) what is supposed to the erasure of the "fresh type variable" <Z#8-Map<K#3,U#4>#7 extends java.lang.Object> ? If I invent formulas like: if (this.right.isSyntheticTypeVariable()) { CaptureBinding18 cb18 = (CaptureBinding18) this.right; return ConstraintTypeFormula.create(cb18.getInferenceVariable(), this.left, SAME, this.isSoft); } if (this.left.isSyntheticTypeVariable()) { CaptureBinding18 cb18 = (CaptureBinding18) this.left; return ConstraintTypeFormula.create(cb18.getInferenceVariable(), this.right, SAME, this.isSoft); } and also a bunch of changes to CB18 to prefer consulting lower bound before upper bound(s) I am able to get it to infer the collect call in import java.util.List; import java.util.Map; import java.util.stream.Collectors; class Person { String getLast() { return null; }; } public class X { void test1(List<Person> roster) { Map<String, Person> map = roster .stream() .collect( Collectors.toMap( p -> p.getLast(), //[1] p -> p //[2] )); } } to be Map<java.lang.String,Person> collect(Collector<? super Person,java.lang.Object,Map<java.lang.String,Person>>) but that is missing the wildcard capture. I don't think the spec leads to a solution that would contain the capture. As long as we don't associate "?#1 = capture# of ?" we are not going to solve it and I don't see that in JLS.
(In reply to Srikanth Sankaran from comment #109) > I don't think the spec leads to a solution that would contain the > capture. As long as we don't associate > > "?#1 = capture# of ?" > > we are not going to solve it and I don't see that in JLS. Stephan, while we wait for clarifications from Oracle, based on your time availability, we can still proceed with the review of the rest of the patch. (Unfortunately, though this should be a side issue for the core solution it is not - the new approach depends on being able to reevaluate and obtain the same capture that inference saw and we are having trouble locating in the JLS where inference should apply the capture - nevertheless, it is one isolated issue and rest of the changes can continue to be reviewed.)
Created attachment 247955 [details] Fixes for two problems found by other IBM projects. (In reply to Srikanth Sankaran from comment #78) > Shankha will work with other IBM departments to get some stress testing > done. Thanks Jay and Shankha. Testing went fine, two issues got reported. This patch has corrections for those two problems. - AllocationExpression.isBoxingCompatible was missing a call to compute boxing type. - After ground target type is determined, we may still be left with wildcards in the ground target type that could make the target type and the lambda incompatible - on master this case was handled in Expression.resolveTypeExpecting. Now we don't reach there, so a check is needed in LE itself.
(In reply to Srikanth Sankaran from comment #78) > Jay has offered to verify that we build JRE8 alrigtht with this patch Jay, to test JRE and Eclipse SDK builds, you will have to apply the patch "Cumulative ..." and "Fixes for two ..." and NOT the "patch to remove ..." That was an experimental patch I wanted to share with Stephan.
(In reply to Srikanth Sankaran from comment #112) > Jay, to test JRE and Eclipse SDK builds, you will have to apply the patch > "Cumulative ..." and "Fixes for two ..." and NOT the "patch to remove ..." > That was an experimental patch I wanted to share with Stephan. I tried to build JRE from source after long time today and found 6 compilation errors, of which 5 are present on HEAD. Here's the testcase for the additional error: public class OpenMBeanInfoSupport extends MBeanInfo { public OpenMBeanInfoSupport(MBeanNotificationInfo[] notifications) { super((notifications == null) ? null : notifications.clone()); } } class MBeanInfo { public MBeanInfo(MBeanNotificationInfo[] notifications) throws IllegalArgumentException { } } interface MBeanNotificationInfo {} Here's the testcase for the errors that are produced in HEAD (this has nothing to do with the patch, though, so I will raise a new bug for this): public class RMIConnector { private Integer addListenerWithSubject(MarshalledObject<String> filter) { final MarshalledObject<String>[] filters = Util.cast(new MarshalledObject<?>[] { filter }); return null; } } class Util { @SuppressWarnings("unchecked") public static <T> T cast(Object x) { return (T) x; } } class MarshalledObject<T> {}
(In reply to Jayaprakash Arthanareeswaran from comment #113) > (In reply to Srikanth Sankaran from comment #112) > > Jay, to test JRE and Eclipse SDK builds, you will have to apply the patch > > "Cumulative ..." and "Fixes for two ..." and NOT the "patch to remove ..." > > That was an experimental patch I wanted to share with Stephan. > > I tried to build JRE from source after long time today and found 6 > compilation errors, of which 5 are present on HEAD. Here's the testcase for > the additional error: Thanks Jay, I see three problems: - NPE only with the patch while compiling one of the snippets - traced it to an pre-existing/dormant bug in Expression.isBoxingCompatibleWith. It is missing a null check compared to its counterpart Expression.isCompatibleWith. We simply didn't exercise this path earlier. - Failure in type inference on master and with the patch. I traced it to a bug in BoundSet.ThreeSets.upperBounds(boolean, InferenceVariable). If the only upper bound is an ArrayBinding, we incorrectly return NO_TYPES. These two are fixed. - Failure to compile the super call with poly conditional expression. I traced it to existing behavior of LE.computeArrayClone - it returns the same binding for all array clones with return type set to Object ! We need one for each array type. I am working on this.
Created attachment 247972 [details] Fix for JRE8 build problems I think this should fix the problems seen on master as well as with the patch set under consideration here. Jay, appreciate your giving it a quick spin, tests are still running, but I don't expect any issues.
(In reply to Srikanth Sankaran from comment #115) > Jay, appreciate your giving it a quick spin, tests are still running, but > I don't expect any issues. RunAllJava8Tests finished alright. Jay also confirmed that JRE8 full build is fine.
Completed some more testing - building the eclipse SDK with the patches. Everything looks good. I was able to compile the eclipse projects successfully as is and with the artificially boosted compliance of 1.8 with the patch provided here: https://bugs.eclipse.org/bugs/show_bug.cgi?id=428203#c6
Summarizing: - JDT/Core, JDT/UI, JDT/APT tests look good, - JRE8 builds fine. - Boostrap tests with artificial boost to project settings to 1.8 went OK. - Other IBM teams engaged in testing and the reported issues resolved.
I'm back to asking trivial questions :) PGMB.cCM18() - Comment has been added: "// CHECK" Who's supposed to check what? - Given that methodSubstitute is a local variable, what's this snippet saying (2 occurrences): methodSubstitute = null; return problemMethod; ?
(In reply to Srikanth Sankaran from comment #114) > - Failure in type inference on master and with the patch. I traced it to > a bug in BoundSet.ThreeSets.upperBounds(boolean, InferenceVariable). If the > only upper bound is an ArrayBinding, we incorrectly return NO_TYPES. Thanks for catching this. Follow-up task for me: Seeing an ArrayBinding in local variable 'simpleUpper' raises an alert: Check if "instanceof ReferenceBinding" was meant to be !right.isPrimitiveType() I'm pretty sure I was thinking that ArrayBinding is a ReferenceBinding. (Then: change type of 'rights' to TypeBinding[])
I'm having a bit trouble digesting the changes in Scope.parameterCompatibilityLevel(). - what exactly is checkOnlyFunctionalTypes controling? - it seems to cause both more and less checking (see the "return NOT_COMPATIBLE" and "continue" arms inside) - from three added blocks governed by "if (checkOnlyFunctionalTypes)" (1) has different logic than (2) & (3), which all could be converted to (almost?) the same shape. Is there a significant difference? - how is mentioning of jlO connected to the issue of functional types? - why is some of this controlled from outside via Scope.shouldConsultShadowOriginal()? Note the typo, should be "shallow".
Status reached in reviewing: - The majority of classes with changes have been reviewed. - Changes in BoundSet await clarification from Dan - Reviewing still to be completed for: - ConstraintExpressionFormula - InferenceContext18 - Scope - Tests Refreshing our memories of issues raised in previous comments: Comment 88 has several (yet unnumbered) follow-up tasks. From comment 83, part two (re isFunctionalType()): The answer in comment 88 doesn't fully convince me, aren't you saying b ? new Runnable() : new Object() makes the expression a functional thing? Probably I didn't fully grok the semantics of "kindaSortaFunctionalType" :) Comment 97 calls for constructing a new test case. It could be quite hard, though, to direct the control flow into this dark corner ...
(In reply to Stephan Herrmann from comment #119) > I'm back to asking trivial questions :) > > PGMB.cCM18() > - Comment has been added: "// CHECK" > Who's supposed to check what? > > - Given that methodSubstitute is a local variable, what's this snippet saying > (2 occurrences): > methodSubstitute = null; > return problemMethod; > ? Sorry, these should be cleaned up. (In reply to Stephan Herrmann from comment #122) > Refreshing our memories of issues raised in previous comments: > Comment 88 has several (yet unnumbered) follow-up tasks. Don't worry about this, I'll make one full pass over all the scattered comments and enumerate them at the right time. > From comment 83, part two (re isFunctionalType()): > The answer in comment 88 doesn't fully convince me, aren't you saying > b ? new Runnable() : new Object() > makes the expression a functional thing? Probably I didn't fully grok the > semantics of "kindaSortaFunctionalType" :) No, I am saying b ? () -> { /* */ } : new Object() is an argument expression that is a functional type despite only one arm of it being a functional type. Basically an argument expression being tagged as a functional type allows to us assert the corresponding parameter MUST be a functional interface type. In this case, even though "new Object()" does not require its matching parameter to be a functional interface, the lambda does and so it is correct to assert that the whole conditional expression can be a compatible argument only if the parameter is a functional interface. (In reply to Stephan Herrmann from comment #121) > I'm having a bit trouble digesting the changes in > Scope.parameterCompatibilityLevel(). After my morning table tennis game :)
(In reply to Srikanth Sankaran from comment #123) > No, I am saying > > b ? () -> { /* */ } : new Object() > > is an argument expression that is a functional type despite only one > arm of it being a functional type. i.e on the argument expression side, we are checking if it is a functional type (i.e whether a lambda or a method reference) not whether it is a functional interface, while on the parameter side we are checking if it is a functional interface type not whether it is a functional type (which does not make sense as a lambda or a method reference cannot be a parameter, only an argument)
Created attachment 247988 [details] Patch with much clearer version of Scope.parameterCompatibilityLevel (In reply to Srikanth Sankaran from comment #123) > (In reply to Stephan Herrmann from comment #121) > > I'm having a bit trouble digesting the changes in > > Scope.parameterCompatibilityLevel(). > > After my morning table tennis game :) OK, 4 games (longer version of the game) to 3, great start to the day :) This attachment eliminates the (misspelled) hack around Scope.consultShadowOriginal() and also has Scope.parameterCompatibilityLevel rewritten to make for much easier understanding. (It also corrects some self defeating even if not incorrect code that was not achieving the intended purpose) The small test case GRT1_8.testBug424205b highlights many of the problems in compatibility checking post just applicability inference. Here is a further reduced version of it: interface I { void bar(String t); } public class X<T> { X(String x) {} X(T x) {} public void one(X<I> c){} public void two() { one(new X<>((String s) -> { })); } } During the initial resolve of the poly allocation expression, new X<>((String s) -> { }), in the static factory based type inference we readily disqualify X(String x) as not being applicable and X(T x) as being applicable with the PPGMB computed being X<java.lang.Object> <factory>(java.lang.Object) (We don't have a target type - so don't/can't proceed to invocation type inference.) Now if this PPGMB were to be subjected to argument/parameter compatibility checks (if invocation type is inferred, we just accept results from part G) here are some pertinent observations that can be made about how/what those compatibility checks should do or look like. 1. If the argument expression is NOT a functional type (FunctionalExpression), it is anyway pertinent to applicability, inference would have ensured that it is compatible and so it is wasteful to recheck compatibility post inference. It would not be incorrect to recheck, but just wasteful. So the checkOnlyFunctionalTypes is an optimization. (The optimization was not properly implemented in the earlier version perhaps leading to the confusion in understanding) 2. If the argument expression is a functional type then we cannot necessarily immediately assert that the corresponding parameter of the PPGMB must be a functional interface type. Some caution is required to first make sure that the argument expression is pertinent to applicability given the shallow original's parameter. For example: (in the snippet above) During diamond inference, the (non-generic) candidate constructor X<T>.X(T x) from the example above gets a parameterized static factory <T'> X<T'> <factory>(T') and the inferred PPGMB in the absence of a target type is X<java.lang.Object> <factory>(java.lang.Object). Now if we ask the argument expression (String s) -> { } if it is compatible with the parameter 'Object' of the PPGMB, it would answer false since Object is not a functional interface. But we should not be asking that question at all, since in the generic factory method against which inference would check for compatibility in the presence of a target type the corresponding parameter is T' and T' as the target type for the lambda expression makes it not pertinent to applicability given it is a type variable of the generic method in consideration (even though the lambda is an explicit lambda expression) This explains the need for and all the uses of code of the form: if (!shallowParameters[i].isPertinentToApplicability(arg, shallowOriginal)) continue; and its minor variants. This recently introduced API reaches FunctionalExpression.isPertinent* through a circuitous route when it should and when it need not, answers appropriately directly. 3. We may have to exercise caution when applicability inference would/could have instantiated a PPGMB with partial substitution of inferred solutions and j.l.Object for others for the same parameter's type - I don't have a test case for this, but this is just defensive coding. If we see the parameter mentioning j.l.O at all, we say it is not safe to check compatibility. In future we may relax this - this needs much more thinking to convince ourselves that we will not, looking at partially substituted parameter dismiss the method as being incompatible. See that the whole "post applicability inference compatibility check" effort is a best case effort to weed out some methods that on master we would allow into Scope.mSMB and may be to result in ambiguity: See https://bugs.eclipse.org/bugs/show_bug.cgi?id=428811#c3 for example of a case that would be readily be weeded out due to such checks. (Since Characteristics[] is not a functional interface while the argument expression ImmutableList::copyOf is a functional expression) (even though in that bug these checks are not needed with the proposed patch since we now proceed to invocation type inference if target type is available and target type *is* available in that case) A slightly modified case from there where the Collector.of call is not in return expression, but is an argument to an invocation would benefit from these checks post applicability inference. See that I also have a follow up task in https://bugs.eclipse.org/bugs/show_bug.cgi?id=437444#c74 7. See if we can improve further on post applicability compatibility checks. So the goal for the present bug is to improve on master behavior without introducing any bugs --> defensive/conservative checks. If unsure we can safely check compatibility, back off. Hence the check for jlO being mentioned. (this would cover the type variable case also, but ...) Finally, the Scope.shouldConsultShadowOriginal() piece is withdrawn. It was originally introduced to tackle a certain difficulty in tunneling through overload resolution all over again after inferring elided to types to find the constructor binding. This is unncessary. By reinstating SyntheticFactoryMethodBinding.applyTypeArgumentsOnConstructor(TypeBinding[]) we get directly get the constructor without looking up the method again.
(In reply to Srikanth Sankaran from comment #125) > the constructor binding. This is unncessary. By reinstating > SyntheticFactoryMethodBinding.applyTypeArgumentsOnConstructor(TypeBinding[]) > we get directly get the constructor without looking up the method again. See the new method AE.inferConstructorOfElidedParameterizedType. It should be noted that on both master and with the proposed patch, after the <> is inferred, we needlessly tunnel through overload resolution again via the call to findConstructorBinding. We should just retrieve the constructor from the static factory by using applyTypeArgumentsOnConstructor. That is a follow up task.
(In reply to Srikanth Sankaran from comment #125) > So the checkOnlyFunctionalTypes is an optimization. (The optimization > was not properly implemented in the earlier version perhaps leading to the > confusion in understanding) [...] > See that the whole "post applicability inference compatibility check" effort > is a best case effort to weed out some methods that on master we would allow > into Scope.mSMB and may be to result in ambiguity: I just realized that the concerned Scope.parameterCompatibilityLevel could be reached from Scope.mSMB, in which case this optimization would be incorrect. When called from Scope.mSMB, we not only want to know the method is compatible given the arguments, but also the compatibility "level", while post applicability inference, we don't care about the level only that it is compatible. Three ways of resolving: - Restore master behavior and say for 1.8 PGMBs and PPGMBs don't check compatibility post inference. Get rid of the changes to pCL. Deal with any problems with ambiguity as they arise. - make checkOnlyFunctionalTypes a parameter to pCL. - Post applicability inference at 1.8 don't attempt the optimization and check all arguments against parameters. I am open to suggestions. In particular, I am fine punting on this whole issue and deferring to a later date despite the effort invested.
(In reply to Srikanth Sankaran from comment #127) > I am open to suggestions. In particular, I am fine punting on this whole > issue and deferring to a later date despite the effort invested. After thinking long and hard, I have decided to defer this issue - the present project is complicated enough that we should not attempt to pile on more and more. So, I'll restore the behavior from master with respect to post-applicability- only-inference compatibility checks into the proposed solution. I have raised https://bugs.eclipse.org/bugs/show_bug.cgi?id=447767 which fails on master and will also fail once I withdraw the code changes in Scope.pCL made on behalf of this bug. This bug nicely illustrates some of the complexities I was trying to handle in pCL. We will deal with https://bugs.eclipse.org/bugs/show_bug.cgi?id=447767 separately post resolution of the current bug. This should also take care of the follow up task (7) listed earlier. Patch withdrawing the elaborate checks in pCL is under test, will post shortly.
Created attachment 247989 [details] Patch to eliminate post inference compatibility checks. This patch - Eliminates the hack around the mis-spelt Scope.shouldConsultShadowOriginal(). That hack was to cope with some complications in tunneling through overload resolution post elided type inference to compute the constructor binding. Now we avoid that unnecessary tunneling by using the reinstated method SFMB.applyTypeArgumentsOnConstructor See AE.inferConstructorOfElidedParameterizedType. - Restores the behavior from master that we will not do compatibility checks on inferred PGMBs/PPGMBs. This means we will fail on https://bugs.eclipse.org/bugs/show_bug.cgi?id=447767 with this patch as with master and that failure will have to be handled separately. I also verified that the need for tolerateInferenceVariable usage goes away legitimately with the proposed solution. Ball is back at your court Stephan.
(In reply to Srikanth Sankaran from comment #102) > See https://bugs.openjdk.java.net/browse/JDK-8055963 and > https://bugs.openjdk.java.net/browse/JDK-8056092. Follow up task for this: Remove BoundSet.applyInstantiation and just check for supers[0-1] being a proper type.
(In reply to Srikanth Sankaran from comment #124) > (In reply to Srikanth Sankaran from comment #123) > > > No, I am saying > > > > b ? () -> { /* */ } : new Object() > > > > is an argument expression that is a functional type despite only one > > arm of it being a functional type. > > i.e on the argument expression side, we are checking if it is a functional > type (i.e whether a lambda or a method reference) not whether it is a > functional interface, while on the parameter side we are checking if it is > a functional interface type not whether it is a functional type (which does > not make sense as a lambda or a method reference cannot be a parameter, only > an argument) OK, I see: if we have at least one LE or RE, the target type *must* be a functional type. Thanks. Would "needsFunctionalTargetType" capture the intended semantics, then?
(In reply to Srikanth Sankaran from comment #128) > After thinking long and hard, I have decided to defer this issue - the > present project is complicated enough that we should not attempt to pile > on more and more. Thanks (In reply to Srikanth Sankaran from comment #129) > Created attachment 247989 [details] > Patch to eliminate post inference compatibility checks. I've pulled this into my workspace and continue reviewing from here.
(In reply to Stephan Herrmann from comment #131) > Would "needsFunctionalTargetType" capture the intended semantics, then? Sounds better than kindaSortaFunctionalType actually :) Thanks, will do.
Revisiting AE after comment 129 I'm curious about the role of this line: ParameterizedTypeBinding parameterizedType = scope.environment().createParameterizedType(genericType, genericType.typeVariables(), genericType.enclosingType()); (several occurrences) Parameterizing a generic type with its own type variables doesn't really add interesting information. While I don't see a bug here, I wonder if this could be simplified. The PTB is needed only for passing into inferElidedTypes() and finally getFactoryMethod(). Inside getFactoryMethod() most work is done using its genericType(), only one call to mMSB() uses the actual PTB. Even there, the generic type *might* suffice.
(In reply to Stephan Herrmann from comment #134) > Parameterizing a generic type with its own type variables doesn't really add > interesting information. While I don't see a bug here, I wonder if this > could be simplified. Probably, that code is from Java 7 days. I think there was an outline of what needed to be done shared in the EG list IIRC and it was supposed to be a close implementation of that.
I was wondering why everything about this thingy in Scope.mSMB was removed: innerInferenceHelper.getArgumentTypesForCandidate(..) this lead me to see that indeed the root cause leading to that tricky solution is fixed: instead of using argumentTypes from the provisionalResult of applicability inference, we now simply get a PolyTypeBinding in argumentTypes. This PolyTypeBinding serves well for all compatibility checks (In master MessageSend doesn't answer PolyTypeBinding). Cool! (reference this obsoletes all that was said and done in bug 426290).
(In reply to Srikanth Sankaran from comment #88) > (In reply to Stephan Herrmann from comment #82) > > > Is this tendency to report more errors intended in terms of user experience > > or a result from technical considerations? > > The honest answer is it is neither. I made a good bit of effort to minimize > diagnostic differences and we are where we are with this patch after > such effort. I think we should take a fresh look at this whole issue > separately perhaps under > > https://bugs.eclipse.org/bugs/show_bug.cgi?id=406614 > https://bugs.eclipse.org/bugs/show_bug.cgi?id=404675 > https://bugs.eclipse.org/bugs/show_bug.cgi?id=428489 > https://bugs.eclipse.org/bugs/show_bug.cgi?id=400831 > > merged into one project. Note for that future project: in master, IC18 has unfinished work on leveraging a connection from inner to outer inference for better error reporting. It may be interesting to restore IC18.outerContext once we more fully address this.
(In reply to Stephan Herrmann from comment #136) > I was wondering why everything about this thingy in Scope.mSMB was removed: > > innerInferenceHelper.getArgumentTypesForCandidate(..) > > this lead me to see that indeed the root cause leading to that tricky > solution is fixed: instead of using argumentTypes from the provisionalResult > of applicability inference, we now simply get a PolyTypeBinding in > argumentTypes. This PolyTypeBinding serves well for all compatibility checks > (In master MessageSend doesn't answer PolyTypeBinding). For the record, this is also the reason why the need for `tolerateInferenceVariable' goes away.
(In reply to Stephan Herrmann from comment #137) > Note for that future project: in master, IC18 has unfinished work on > leveraging a connection from inner to outer inference for better error > reporting. It may be interesting to restore IC18.outerContext once we more > fully address this. Right, will do.
Created attachment 248033 [details] Merged patch brought up to date with master. (In reply to Stephan Herrmann from comment #132) > I've pulled this into my workspace and continue reviewing from here. Since you have applied the patches already, I took the liberty of merging the 4 outstanding patches into one, bringing it upto date with master so it can be applied on top of master in one stroke and reposting here. There are no new changes in this patch.
Created attachment 248034 [details] Correct merged patch Sorry, uploaded the wrong patch by mistake.
Inching along: In IC18.addConstraintsToC_OneExpr() the early return according to https://bugs.openjdk.java.net/browse/JDK-8052325 should only happen within } else if (expri instanceof Invocation && expri.isPolyExpression()) { not sure if it makes a difference, tests seem to be fine with this move.
More re IC18.addConstraintsToC_OneExpr() Inside the "Invocation" branch I begin to believe that more preparation might be necessary before recursion. OK (I'm only unsure, why this preparation would happen only *sometimes*). However, the block inside "if (expri instanceof LambdaExpression)" looks unmotivated to me from a spec p.o.v. Is this another pending spec amendment? After "If ei is a LambdaExpression, C contains ‹LambdaExpression →throws Fi θ›" I only see a full stop. :) Running GRT_18 with and without that code block I see obvious regressions, but when reverting GRT_18 to master, regressions are less significant. The difference is due to - changed test expectations - added tests Question: is that added code block intrinsic part of the new F-G-integration, or a change on top, perhaps in trying to better align with javac?
CExprF.reduceReferenceExpressionCompatibility: you moved the lookup of reference.findCompileTimeMethodTargeting into the branch for inexact RE. This leaves the branch for exact RE without check for applicable method. Apparently, you compensated this by } else if (n != k) { return FALSE; - are there other criteria for potentially applicable that we are now missing here? - if it stays, could you please add a comment, saying that this new branch corresponds to the check for applicable method? On its own it seems to be in conflict with the spec saying: "In all other cases, n = k". HEADS-UP: I feel I'm running out of original new critique. Phew. Great piece of work!! Once we receive answer from Dan - at least concerning the capture bounds issue - we should be ready to come to some conclusion ... and that conclusion will be positive :)
(In reply to Stephan Herrmann from comment #144) > CExprF.reduceReferenceExpressionCompatibility: > > Apparently, you compensated this by > > } else if (n != k) { > return FALSE; I meant to document which test revealed this connection: It's LET.test430766
PS: I'm deferring a final walk through test changes until after we decided what goes in, how capture bounds will be handled, ..., i.e. freeze the behaviour of the compiler.
(In reply to Stephan Herrmann from comment #142) > Inching along: > > In IC18.addConstraintsToC_OneExpr() the early return according to > https://bugs.openjdk.java.net/browse/JDK-8052325 should only happen within > > } else if (expri instanceof Invocation && expri.isPolyExpression()) { > > not sure if it makes a difference, tests seem to be fine with this move. I think it is correct where it is, but there is value in moving it to where you suggest so that someone in future reading code in parallel with the specification would find it more aligned. Will do. (In reply to Stephan Herrmann from comment #143) > More re IC18.addConstraintsToC_OneExpr() > > Inside the "Invocation" branch I begin to believe that more preparation > might be necessary before recursion. OK (I'm only unsure, why this > preparation would happen only *sometimes*). OK, here is the explanation: We need to do this preparation only for invocations interleaved by a lambda. For invocations that are direct arguments to the outer generic method, because they are necessarily pertinent to the applicability of the outer method, the equivalent "preparation" would have happened in CEF.reduce() during applicability inference. See that reduce of a poly invocation expression calls for the bound set b3 to be transferred to the outer context - we don't literally do this on master (or in this patch) but we lift the inference variables, initial constraints from parameters and the initial bound set from type parameter bounds into the outer context => This is b2 from the inner call. And then by calling CEF.inferPoly*, we add additional constraints for 18.5.2. All this "effectively amounts to" b3 from the inner call ==> we are good on master (and since this is not changed in the proposed patch, we are good with the patch too on this part) However when a poly invocation is interleaved by a lambda that is not pertinent to applicability (which means I should check if we are doing extra/incorrect work when the interleaving lambda *is* pertinent to applicability of the outer call), this preparation would not have happened during the applicability inference of the outer call. i.e outer inference context will not yet have the inference variables, initial bound set from type variables ... it its context. Hence we need to do this preparation to lift them to the outer context during C set construction. Corollarywise, because for lambda interleaved inner poly invocations, we lift the inference variables, initials bounds from type parameters and initial constraints from parameters during C set construction, their reduction at CEF.reduce is a NOP. > However, the block inside "if (expri instanceof LambdaExpression)" looks > unmotivated to me from a spec p.o.v. Is this another pending spec amendment? > After > "If ei is a LambdaExpression, C contains ‹LambdaExpression →throws Fi θ›" > I only see a full stop. > :) I think you are missing https://bugs.openjdk.java.net/browse/JDK-8038747 referenced by https://bugs.eclipse.org/bugs/show_bug.cgi?id=444891. > Question: is that added code block intrinsic part of the new > F-G-integration, or a change on top, perhaps in trying to better align with > javac? Per above, it is to cover the spec amendment for https://bugs.openjdk.java.net/browse/JDK-8038747. Without this change, none of the dozen or so duplicates of https://bugs.openjdk.java.net/browse/JDK-8038747 would work.
(In reply to Srikanth Sankaran from comment #147) > However when a poly invocation is interleaved by a lambda that is not > pertinent to applicability (which means I should check if we are doing > extra/incorrect work when the interleaving lambda *is* pertinent to > applicability of the outer call), this preparation would not have > happened during the applicability inference of the outer call. i.e outer > inference context will not yet have the inference variables, initial > bound set from type variables ... it its context. Hence we need to do > this preparation to lift them to the outer context during C set > construction. Explicit documentation for a future reader: ... Hence we need to do this preparation ... ... because if we don't do this preparation during C set construction, the recursive call to addConstraintsToC from addConstraintsToC_OneExpr for a poly invocation interleaved by an impertinent(!) lambda, will not be able substitute inner's type variables with equivalent inference variables and leave them as naked type variables themselves, which will means the C set constraints will not see the inference variables from the inner call, which will means reduction sequencing using "bottom set - input variables - output variables" algorithm will see the naked type variables and not the inference variables which will mean reduction sequencing will break down, which will mean things won't work.
(In reply to Srikanth Sankaran from comment #147) > > > Question: is that added code block intrinsic part of the new > > F-G-integration, or a change on top, perhaps in trying to better align with > > javac? > > Per above, it is to cover the spec amendment for > https://bugs.openjdk.java.net/browse/JDK-8038747. Without this change, > none of the dozen or so duplicates of > https://bugs.openjdk.java.net/browse/JDK-8038747 would work. I meant none of the dozen or so duplicates of https://bugs.eclipse.org/bugs/show_bug.cgi?id=432682 would work.
(In reply to Stephan Herrmann from comment #144) > CExprF.reduceReferenceExpressionCompatibility: > > you moved the lookup of reference.findCompileTimeMethodTargeting into the > branch for inexact RE. This leaves the branch for exact RE without check for > applicable method. Apparently, you compensated this by > > } else if (n != k) { > return FALSE; > > - are there other criteria for potentially applicable that we are now > missing here? > > - if it stays, could you please add a comment, saying that this new branch > corresponds to the check for applicable method? On its own it seems to be in > conflict with the spec saying: "In all other cases, n = k". Analysis: I misspoke in https://bugs.eclipse.org/bugs/show_bug.cgi?id=437444#c138 regarding the reason why the need for tolerateInferenceVariables goes away. Here is the proper analysis: During reference expression reduction, we can take one of 4 possible paths: (a) proper target type, exact method reference (b) proper target type, inexact method reference. (c) improper target type, exact method reference, (d) improper target type, inexact method reference. For (a) and (b), we will never see inference variables during method lookup - we also don't get into CEF.reduceREC. For (d), we assert that function type's parameters must be proper types. so again we will not need the tolerateInferenceVariable provision. So only for (c) we will hit that situation, but for (c) we should really NOT be tunneling through overload resolution since we know we have an exact method reference lying around. The other interesting point here is that the spec talks of a potentially applicable method - we don't have the infrastructure to deliver answers for that. We compute compatible method which does more work than necessary. So what I can do as a follow up here is to restore the code relating to this change from master so the code closely follows/aligns with the spec and push the change into RE.findCompile* i.e this method would where an exact method reference exists return that if n == k or n == k + 1 otherwise null. Recording another follow up task so it does not fall through the cracks: ASTNode.resolvePolyExpressionArguments requires a null check at if (argumentTypes[i].isPolyType()) { as I noticed an NPE when debugging a broken test case.
(In reply to Stephan Herrmann from comment #144) > HEADS-UP: I feel I'm running out of original new critique. Phew. > > Great piece of work!! Thanks, my goal is to come up with an integration worthy of the inference engine which is very well written, hopefully this achieves that. OK, now the mutual admiration society membership subscription is complete. Perfect :) > Once we receive answer from Dan - at least concerning the capture bounds > issue - we should be ready to come to some conclusion ... and that > conclusion will be positive :) There are several follow up tasks scattered throughout the comments. AFAICT, the one about the capture bound incorporation is the only one that needs to be addressed to satisfaction before release, others can be relegated to a different ticket. (In reply to Stephan Herrmann from comment #146) > PS: I'm deferring a final walk through test changes until after we decided > what goes in, how capture bounds will be handled, ..., i.e. freeze the > behaviour of the compiler. Sounds good for now. I have sent a reminder to Dan since it is a week now with no response. With M3 approaching, I hope we hear soon. I would really like this to go in for M3 - on the due diligence part, we look good. BTW, there are very small patches on top of the present patch posted for review at https://bugs.eclipse.org/bugs/show_bug.cgi?id=430686 https://bugs.eclipse.org/bugs/show_bug.cgi?id=440019 and https://bugs.eclipse.org/bugs/show_bug.cgi?id=448028 I estimate it should take no more than an hour - it would be good to release them also for M3. TIA.
(In reply to Srikanth Sankaran from comment #151) > BTW, there are very small patches on top of the present patch posted for > review at > > https://bugs.eclipse.org/bugs/show_bug.cgi?id=430686 > https://bugs.eclipse.org/bugs/show_bug.cgi?id=440019 and > https://bugs.eclipse.org/bugs/show_bug.cgi?id=448028 and one more: https://bugs.eclipse.org/bugs/show_bug.cgi?id=444334 For the record, we have word from the spec lead that there are unresolved issues with capture bound incorporation details: "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." We need to decide what to do for M3. I am very comfortable with the change we have - it is small & localized. If deemed necessary, I am willing to have this controlled by an environment variable/system property, but that is an overkill IMO. Duplicates included, we are looking at 30+ issues being resolved.
(In reply to Srikanth Sankaran from comment #152) > I am very comfortable with the change we have - it is small & localized. I meant to add - with a further change to infer bounds of the form "?#1 = capture# of ?" only when the wildcard in GA is of proper type. (the proposed patch does more than that)
(In reply to Srikanth Sankaran from comment #152) > For the record, we have word from the spec lead that there are unresolved > issues with capture bound incorporation details: > > "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." > > We need to decide what to do for M3. > > I am very comfortable with the change we have - it is small & localized. > If deemed necessary, I am willing to have this controlled by an environment > variable/system property, but that is an overkill IMO. Right, I'm not interested in a system property, either. While we don't have the final solution, I'm mainly interested in estimating the effort to change this, once the final solution is given at the spec level. Let's just document the exact extent of the extra-constitutional part, so that it will be easy, to pull the plug on all of that, without forgetting parts. This can happen by a compile time constant or any documentation device. The other question is, will the interim solution make it harder to adopt the final solution? This could potentially happen, if the interim solution makes some wrong assumptions and if other changes piled on top of this bend over backwards in order to harmonize with this interim change. To give a number into the calculation: when draftily reverting the changes re capture bounds, RunAllJava8Tests gives 17 regressions. What's the cause behind those? Maybe I pulled the wrong plug? Was it the previous implementation (master) that bent over backwards to harmonize with the lack of this change? Since the test changes contained in this bug are part of that equation, I will take a look at those as my next step.
(In reply to Stephan Herrmann from comment #154) > Let's just document the exact extent of the extra-constitutional part, so > that it will be easy, to pull the plug on all of that, without forgetting > parts. This can happen by a compile time constant or any documentation > device. Sounds good. > The other question is, will the interim solution make it harder to adopt the > final solution? This could potentially happen, if the interim solution makes > some wrong assumptions and if other changes piled on top of this bend over > backwards in order to harmonize with this interim change. It is hard to make predictions especially about future, but I think we are on safe grounds here. Basically the change is to infer a bound equating the fresh type variable that stands for a wild card with a capture of the wildcard when the wildcard is a proper type. I don't expect other changes building on top of this. > To give a number into the calculation: when draftily reverting the changes > re capture bounds, RunAllJava8Tests gives 17 regressions. What's the cause > behind those? Basically, you will find that most if not all of the failing tests will have Collectors.toMap call in them. This has a signature Collector<T, ?, M> toMap(...) introducing the wildcard into the picture. On master as https://bugs.eclipse.org/bugs/show_bug.cgi?id=445274 points out, we silently instantiate the enclosing collect call to be Collector<? super Person, Object ,Map<java.lang.String,Person>>. Now if after outer inference, we push the target type onto inner and ask it to evaluate it would fail because outer inference is missing a captured wildcard and jlO is seen there instead. The situation on master is problematic as some refactoring such as extract local variable would yield a code fragment that would not compile. I think bottom line is that JLS has some open issues as acknowledged by Dan. Our implementation also has issues - in terms of premature clearing of capture bounds as well as how CaptureBinding18 behaves. I think it is supposed to act as a type that potentially has both lower and upper bounds, but it for the most part it looks only at upper bound. Another interesting experiment would be what would happen with master if we don't clear the capture bounds - I recall seeing a handful of failures with this experiment when I tried earlier. I think the spec intends that they not be cleared and sooner or later we will run into a test case that will trigger this scenario. So this area is going to require revisit anyways, not just on account of this extra constitutional change.
Tomorrow, (a) I'll prepare a patch isolating the changes for capture bound incorporation under a compile time constant as well as minimizing the changes if possible. I didn't so far get to investigate your observations at the bottom of https://bugs.eclipse.org/bugs/show_bug.cgi?id=437444#c101. (b) Raise follow up bugs for all issues pointed out by both of us so we have a clear picture of what the finishing touch required is going to be.
Created attachment 248173 [details] Addendum for capture bound change under compile time constant This patch isolates the changes for capture bound incorporation under a compile time constant IC18.SHOULD_WORKAROUND_BUG_JDK_8054721 There are exactly two chunks of change under this guard and I am convinced they are absolutely harmless. It is likely that when we adjust fully to proper handling of capture bound incorporation (i.e don't clear capture bounds prematurely and deal with them in 18.4 taking https://bugs.openjdk.java.net/browse/JDK-8054721 into account and https://bugs.eclipse.org/bugs/show_bug.cgi?id=447576 into account) we may not need real captures anymore. Thing is either we need proper simulated captures or real captures, it cannot be neither and this patch takes the real capture route.
OK, I have gone through every single comment with a fine toothed comb and raised follow up tickets under the umbrella bug: https://bugs.eclipse.org/bugs/show_bug.cgi?id=448791. We have 13 tasks of none of which is a blocker for M3. I would rate: https://bugs.eclipse.org/bugs/show_bug.cgi?id=447767 https://bugs.eclipse.org/bugs/show_bug.cgi?id=448792 https://bugs.eclipse.org/bugs/show_bug.cgi?id=448793 https://bugs.eclipse.org/bugs/show_bug.cgi?id=448794 and https://bugs.eclipse.org/bugs/show_bug.cgi?id=448802 as candidates for M4. For various other small corrections/suggestions that don't have to be deferred to their own tickets, I have a prepared a patch that is being tested against all JDT/Core tests. Will upload once done. Looks all set.
Created attachment 248174 [details] Addendum to incorporate review comments
The patch "addendum for capture bound ..." and "Addendum to incorporate ..." need to be applied on top of the patch for https://bugs.eclipse.org/bugs/show_bug.cgi?id=444334 which itself specifies its application order in https://bugs.eclipse.org/bugs/show_bug.cgi?id=444334#c12. (I should really start working on branches) All tests are green - Once I hear a final go, I'll promote the changes. If there are no objections, I would like to promote all of the follow up fixes listed in https://bugs.eclipse.org/bugs/show_bug.cgi?id=437444#c152. There are 4 of them - these are routine bug fixes and can be reviewed one by one, at your leisure. Or perhaps you could give it a high level once over before promotion and closer review can come in the next week or two. Thanks Stephan.
(In reply to Srikanth Sankaran from comment #158) > OK, I have gone through every single comment with a fine toothed comb > and raised follow up tickets under the umbrella bug: > https://bugs.eclipse.org/bugs/show_bug.cgi?id=448791. > > We have 13 tasks of none of which is a blocker for M3. Looks clean. I agree, no blocker for M3.
A summary from another p.o.v: pending spec changes adopted by this patch: see comment 44: https://bugs.openjdk.java.net/browse/JDK-8052325 see comment 102: https://bugs.openjdk.java.net/browse/JDK-8055963 (javac bug) https://bugs.openjdk.java.net/browse/JDK-8056092 (spec bug) see comment 147 & bug 444891 https://bugs.openjdk.java.net/browse/JDK-8038747 see comment 152: https://bugs.openjdk.java.net/browse/JDK-8054721 None of these spec bugs are resolved, our implementation is built on good faith that directions outlined in the bugs will actually appear in a future version of JLS and represent the original intention for Java 8. Am I describing the situation correctly?
(In reply to Stephan Herrmann from comment #162) > Am I describing the situation correctly? Yes, that is correct. We need to see if there is a way to add ourselves to the notification list to listen in further amendments and/or resolution. I am not sure how to do this yet. I'll forward to the team as well as also document here the link provided by Daniel Smith on specification bugs open, in progress and recently resolved: // --- > [Do you have a canned query that would show me all the open and recently > resolved defects against the JLS ?] https://bugs.openjdk.java.net/issues/?jql=project%20%3D%20JDK%20AND%20component%20%3D%20specification%20AND%20status%20in%20(Open%2C%20%22In%20Progress%22)%20ORDER%20BY%20fixVersion%20ASC Roughly, "Open" and "In Progress" bugs are acknowledged problems, with varying degrees of resolution (some are problem statements, some have suggested replacement text). "Resolved" and "Closed" bugs have been addressed and the fixes published. (The line between the two states in each case is fuzzy, as far as I know.) Fix Version will tell you whether something is intended for a maintenance review of SE 8 or deferred until (at least) SE 9. These are subject to change. // ---
(In reply to Stephan Herrmann from comment #161) > Looks clean. I agree, no blocker for M3. I'll wait for an explicit go and not treat this as one. I think per https://bugs.eclipse.org/bugs/show_bug.cgi?id=437444#c146 you wanted to look through the test changes after the code changes are frozen.
Closing one more loose end: My concern raised in comment 143 is indeed answered by the reference to https://bugs.openjdk.java.net/browse/JDK-8038747. Just two tiny question remain: (1) why this line: if (substF.isFunctionalInterface(skope)) { // could be an inference variable. after a check experiment (only GRT18) this should suffice: if (substF instanceof ReferenceBinding) { Do you have a counter-example? (2) before calling IC18.getInferenceContext() we inconsistently ask instanceof PMB vs. PGMB (getInferenceContext only requires PMB). I seem to remember that some of the pending patches has changes already in this field? If not, might be worth a tiny new child of bug 448791 Certainly no blockers, though. (In reply to Srikanth Sankaran from comment #164) > (In reply to Stephan Herrmann from comment #161) > > > Looks clean. I agree, no blocker for M3. > > I'll wait for an explicit go and not treat this as one. I think per > https://bugs.eclipse.org/bugs/show_bug.cgi?id=437444#c146 > you wanted to look through the test changes after the code changes are > frozen. Right, comment 161 concerned the follow-up tasks. Final look through test changes starting right now ...
(In reply to Stephan Herrmann from comment #84) > Change removes the attempt to fetch variableArity from an existing inference > context. > > If such inference context exists, infCtx.isVarArgs() is authoritative. > > The remaining check is best effort to recover this information. > > I have no counter example, so the common sense checks *may* be sufficient? Yes, this is sufficient.
(In reply to Stephan Herrmann from comment #165) > Just two tiny question remain: > > (1) why this line: > if (substF.isFunctionalInterface(skope)) { // could be an inference > variable. > after a check experiment (only GRT18) this should suffice: > if (substF instanceof ReferenceBinding) { OK, the thing is we can generate meaningful C set elements only when substF is a functional interface. Given that we can quit early checking for functional interface target or quit a bit later after seeing that we don't have a sam which is really the same check. (isFI populates the cache so getSAM() returns instantaneously.) One thing I could have done and didn't choose to was to return false if target is not a functional interface. Originally it was there, but then I realized that we could see an inference variable and could return false incorrectly and turned it to what it is now. We could still return false if it is not functional and not an inference variable, but I didn't see this part's role in triggering an early failure as an important one. > Do you have a counter-example? > > (2) before calling IC18.getInferenceContext() we inconsistently ask > instanceof PMB vs. PGMB (getInferenceContext only requires PMB). > I seem to remember that some of the pending patches has changes already in > this field? If not, might be worth a tiny new child of bug 448791 > > Certainly no blockers, though. > > > (In reply to Srikanth Sankaran from comment #164) > > (In reply to Stephan Herrmann from comment #161) > > > > > Looks clean. I agree, no blocker for M3. > > > > I'll wait for an explicit go and not treat this as one. I think per > > https://bugs.eclipse.org/bugs/show_bug.cgi?id=437444#c146 > > you wanted to look through the test changes after the code changes are > > frozen. > > Right, comment 161 concerned the follow-up tasks. Final look through test > changes starting right now ...
The OCD part of the team has found no reason for holding back this patch any longer: +1 for releasing attachment 248034 [details] and attachment 248173 [details]
(In reply to Stephan Herrmann from comment #165) > (2) before calling IC18.getInferenceContext() we inconsistently ask > instanceof PMB vs. PGMB (getInferenceContext only requires PMB). > I seem to remember that some of the pending patches has changes already in > this field? If not, might be worth a tiny new child of bug 448791 This one needs a longer explanation and also a peek into the pending patches. With all 7 patches applied I see all calls to getIC preceded consistently by an instanceof check for PGMB and not PMB. In the final implementation if the <> constructor is NOT generic it is not useful to return the static factory's context. It is actually harmful. All patches considered, the inner inference context is used for these purposes: 1. To decide whether reduction to b3 should just be a NOP, effectively equivalent things having already been done during C set construction. (this has some issues - there is a FIXME in CEF.reduce that maps to https://bugs.eclipse.org/bugs/show_bug.cgi?id=448802) 2. To transfer b2 to the outer context to subsequently augment it to b3. We should transfer b2 and not "what would effectively reduce to b2" as documented elsewhere as the atomicity problem" 3. To determine what inference kind was used earlier. 4. To see if unchecked conversion was involved earlier Non-generic constructor case: ----------------------------- Now for a non-generic constructor returning the inference context of the factory is harmful because the factory is really a synthesized method with extra type variables to stand in for the class's type variables. It does not map properly to the generic constructor itself. There are "out of scope" variables, bounds etc, so things quickly go south. The else part of if (previousMethod instanceof ParameterizedGenericMethodBinding) { // ... } else { } effectively does what we do on master. Is this sufficient ? Clearly (3) is addressed and (2) is addressed as well as on master but for (1) and (4) it needs some thinking through. Now considering the case where the constructor does turns out to be generic: This generic constructor is not inferred, but is directly manufactured by AE.inferConstructorOfElidedParameterizedType by using SFMB.applyTypeArgumentsOnConstructor As a result, we don't have an inference context for that. I am not certain it would be right/wrong to use the factory's context for the generic constructor for reasons cited just now. That means that for a generic constructor we should be doing: if (innerCtx == null) { // no inference -> assume it wasn't ... TypeBinding exprType = this.left.resolvedType; if (exprType == null || !exprType.isValidBinding()) return FALSE; return ConstraintTypeFormula.create(exprType, this.right, COMPATIBLE, this.isSoft); } Is this sufficient ? I don't know. I say "we should be doing" because I don't see a single test triggering the conditional breakpoint previousMethod instanceof ParameterizedGenericMethodBinding && previousMethod.isConstructor() in RunAllJava8Tests - i.e we don't have a test for a <> allocation that would bind to a generic method *and* is itself a nested invocation under a generic method. I'll add a follow up task to investigate this further under https://bugs.eclipse.org/bugs/show_bug.cgi?id=448799. This particular task would need to be done for M3 while the other activities of https://bugs.eclipse.org/bugs/show_bug.cgi?id=448799 can come in for M4.
(In reply to Srikanth Sankaran from comment #169) > I'll add a follow up task to investigate this further under > https://bugs.eclipse.org/bugs/show_bug.cgi?id=448799. > This particular task would need to be done for M3 while the other > activities of https://bugs.eclipse.org/bugs/show_bug.cgi?id=448799 can > come in for M4. https://bugs.eclipse.org/bugs/show_bug.cgi?id=448799#c1. It also outlines a possible resolution - I'll get this followed up for M3. Given the complexity, the size and scope this project has come to assume some amount of going back and forth, sorry about that. Looking at https://bugs.eclipse.org/bugs/show_bug.cgi?id=437444#c129, AE.inferConstructorOfElidedParameterizedType was introduced at the same time Scope.consultShadowOriginal() hack and post applicability checks were withdrawn. (1) Post applicability compatibility checks cause problems when tunneling through overload resolution for a constructor after inferring its elided types in the absence of a target type. (2) Scope.consultShadowOriginal() was a mechanism to circumvent those problems. (3) AE.inferConstructorOfElidedParameterizedType avoids tunneling through overload resolution. Given (1) is withdrawn, (2) is unnecessary and is withdrawn. Given (1) is withdrawn, tunneling through overload resolution is safe and so (3) which is just an optimization can be foregone. If we forego (3), we will have an inference context for a generic constructor as we would expect. Ergo, this should be a simple fix.
(In reply to Stephan Herrmann from comment #168) > The OCD part of the team has found no reason for holding back this patch any > longer: > > +1 for releasing attachment 248034 [details] and attachment 248173 [details] Here: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=f357f309273e0bfe8345ff708d18fa83c6a34931 and here: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=192820efad9191d4773fbd88aa6d8a5ea915ea14. Thanks. OCD ? which one of http://acronyms.thefreedictionary.com/OCD did you mean ? :)
Comment on attachment 248174 [details] Addendum to incorporate review comments I'll repost this one with two further changes called out: (1) Ensure generic constructor gets an inference context. (2) CEF.reduce: merge if (innerCtx == null) and if (innerCtx.stepCompleted >= InferenceContext18.TYPE_INFERRED) I will do this well in time for your planned review slot tomorrow.
(In reply to Srikanth Sankaran from comment #171) > OCD ? which one of http://acronyms.thefreedictionary.com/OCD did you mean ? > :) The first, of course. To prove: if I were not OCD I'd leave this question unanswered ...
SFMB.applyTypeArgumentsOnConstructor always returns a PMB, it only receives the class type arguments and not the method type arguments. I think it should receive both and if method type arguments is not empty instantiate and return a PGMB - No ?
Created attachment 248182 [details] Addendum to incorporate review comments New additions: - Makes suitable changes to SFMB.applyTypeArgumentsOnConstructor to make it returns a PGMB when it should. - Establish an inference context for a parameterized generic constructor. Passes RunOnlyJava8Tests. Running all tests now. (this patch should be applied after the 4 bug fixes in queue)
(In reply to Srikanth Sankaran from comment #175) > Passes RunOnlyJava8Tests. Running all tests now. All clear.
Noopur, Shankha, Jay, Sorry for the rework, but the changes for M3 have been phenomenal in scope and volume. Can we get another round of elaborate testing organized for M3 (i.e right away) for JDT/UI, JRE8, SDK bootstrap and working with other IBM team to get some testing done. If you are unable to spare cycles, please let me know, I'll make alternate plans: I would say 95% of the code is already on master, you will need to apply these patches on top of master in this order: (1) The patch at https://bugs.eclipse.org/bugs/show_bug.cgi?id=444334 (2) The patch here: (Addendum ...) (3) The patch https://bugs.eclipse.org/bugs/show_bug.cgi?id=443596 Thanks in advance.
Created attachment 248194 [details] CombinedPatch(Comment 177) (In reply to Srikanth Sankaran from comment #177) > > (1) The patch at https://bugs.eclipse.org/bugs/show_bug.cgi?id=444334 > (2) The patch here: (Addendum ...) > (3) The patch https://bugs.eclipse.org/bugs/show_bug.cgi?id=443596 > All three patches combined.
I am able to compile JRE 8 and Eclipse SDK with the patches without any issue. Looks good to me.
Created attachment 248206 [details] Revised patch incorporating review comments Same patch but refreshed since antecedents got some methods relating to unchecked conversion reporting restored.
In CEF.inferPoly*() I think there should be a call to initializeBounds() on the capture - Not sure - the test cases that necessitate this whole work around mostly use unbounded wildcard (Collectors.groupingBy() and Collectors.toMap()) for those cases it should not matter, but ...
(In reply to Srikanth Sankaran from comment #177) UI test results are good.
Review comments incorporated and released here: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=b0ee678b65f48d1cfab35972dd53d6bd85389446
(In reply to Srikanth Sankaran from comment #181) > In CEF.inferPoly*() I think there should be a call to initializeBounds() > on the capture - Not sure - the test cases that necessitate this whole > work around mostly use unbounded wildcard (Collectors.groupingBy() and > Collectors.toMap()) for those cases it should not matter, but ... Addressed here: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=9c251eade23f2143003635a87c2f3f96ae52d898
Verified for 4.5 M3 using I20141029-2000 build We also need to decide on whether or not we want this for 4.4.2. This is fairly big change for a maintenance release, but here are reasons why we might want this: 1. Obviously this fixes lot of issues in inference and overload resolution areas. 2. Since this alters an important area of the compiler in a big way, maintaining two different versions will be difficult. Srikanth/Stephan, would like to add anything?
(In reply to Jayaprakash Arthanareeswaran from comment #185) > Srikanth/Stephan, would like to add anything? I personally see no choice here - This fixes four cluster of bugs around (a) nested inference in the presence of an interleaving lambda expression (b) handling wildcard capture bounds and (c) raw types and (d) assorted others that go away with the new implementation. Unfortunately, apart from the one deviation in our implementation (https://bugs.eclipse.org/bugs/show_bug.cgi?id=448793) and some small bugs here and there, we fail to compile these cases by *being compliant* to the spec as it existed on Java 8 GA. Stephan's comment at https://bugs.eclipse.org/bugs/show_bug.cgi?id=437444#c162 summarizes this situation nicely. There is too much inter-dependency in the solution to attempt to isolate just the fixes and attempt to back port only them. So I think rather than ask should we back port, a better question would be how can we minimize risks - this we can do by beefing up due diligence activities: apart from unit tests, work with other IBM teams to stress test verify JRE8 builds and Eclipse SDK bootstrap builds are fine etc. And perhaps most importantly, involve the community (after M3 fixes are a few select M4 candidates are back ported) with an honest feedback that 4.4.2 is heading to be a release that carries much more changes for a maintenance release than usual and this is because (a) Java 8 is a huge change to Java (b) because there were crucial spec changes post GA which were implemented by javac *ahead of GA* and so we require active engagement from them to test it out. I recommend we look at backport of M3 fixes after a full 1 month of field testing of M3. That is my input for consideration to the JDT leads
(In reply to Srikanth Sankaran from comment #186) > I recommend we look at backport of M3 fixes after a full 1 month of field > testing of M3. Ref: Bug 449389 - We will be using M3 compiler soon for building eclipse SDK.
After discussions with Srikanth and Stephan, it's been decided that this is too big a change for back porting.
*** Bug 455928 has been marked as a duplicate of this bug. ***
*** Bug 458806 has been marked as a duplicate of this bug. ***
*** Bug 459145 has been marked as a duplicate of this bug. ***
*** Bug 459504 has been marked as a duplicate of this bug. ***
*** Bug 461539 has been marked as a duplicate of this bug. ***
*** Bug 461639 has been marked as a duplicate of this bug. ***
*** Bug 461571 has been marked as a duplicate of this bug. ***
*** Bug 462006 has been marked as a duplicate of this bug. ***
*** Bug 462769 has been marked as a duplicate of this bug. ***
*** Bug 465192 has been marked as a duplicate of this bug. ***
*** Bug 465859 has been marked as a duplicate of this bug. ***
*** Bug 467627 has been marked as a duplicate of this bug. ***
*** Bug 468831 has been marked as a duplicate of this bug. ***
*** Bug 470270 has been marked as a duplicate of this bug. ***
*** Bug 470847 has been marked as a duplicate of this bug. ***
*** Bug 472361 has been marked as a duplicate of this bug. ***
*** Bug 469217 has been marked as a duplicate of this bug. ***
*** Bug 470817 has been marked as a duplicate of this bug. ***
*** Bug 443467 has been marked as a duplicate of this bug. ***
