Community
Participate
Working Groups
Type inference relies on a new method Expression.unresolve() which is probably unsafe, and not tested for completeness of various AST kinds. In bug 424930 I'm even adding one more call to this method (in AllocationExpression, in analogy to MessageSend). On the long run we should either find a way to avoid using this method, or find a way of convincing ourselves that this approach is / can be made safe.
From bug 427483 comment 4 (by Srikanth): We should also take out all the unresolve() calls as soon as reasonably possible. Recently, I implemented support for method/constructor references targetting varargs method by transforming the method reference into a synthesized implicit lambda. I needed bits and pieces of virgin AST for this and this was achieved by: final Parser parser = new Parser(this.enclosingScope.problemReporter(), false); final char[] source = this.compilationResult.getCompilationUnit().getContents(); ReferenceExpression copy = (ReferenceExpression) parser.parseExpression(source, this.sourceStart, this.sourceEnd - this.sourceStart + 1, this.enclosingScope.referenceCompilationUnit(), false /* record line separators */); This should work for any expression actually.
Stephan, is this actually a NOP ? If I comment out the calls to unresolve() from MessageSend (both the calls) I don't see any failures in RunAllJava8Tests. The comment in Expression.java says: // call this before resolving an expression for the second time. // FIXME: we should find a better strategy, see LambdaExpressionsTest.testLambdaInference1() f. for tests that currently need this. void unresolve() { this.resolvedType = null; } These tests are not failing. It is also my understanding that MessageSend's, AllocationExpression's and ExplicitConstructorCalls are not resolve more than once and are patched otherwise directly when feasible - is this true ?
Well, ideally, yes, we should resolve each invocation only once. The calls from AE to unresolve() have been added as recently as bug 424930. Interestingly even that call doesn't seem to be needed anymore. In ECC we never saw the need (may be due to fewer tests). It might be, that each situation requiring the unresolve() call actually has some other root cause. I'll use the AE case to search for the fix that made the unresolve() unnecessary. My current thinking would be to replace the unresolve() calls with some kind of harness that reports this as an implementation problem (PR.needImplementation() or such). WDYT?
(In reply to Stephan Herrmann from comment #3) > The calls from AE to unresolve() have been added as recently as bug 424930. > Interestingly even that call doesn't seem to be needed anymore. In ECC we > never saw the need (may be due to fewer tests). > > It might be, that each situation requiring the unresolve() call actually has > some other root cause. I'll use the AE case to search for the fix that made > the unresolve() unnecessary. An interesting find: the call from AE to unresolve() was made unnecessary by a "Work around [...] for Bug 427483" [1]. The workaround is this little paragraph inside LambdaExpression.resolveType(): if (this.resolvedType != null) return this.resolvedType; This sounds like an effective prevention of double resolve :)) The only risk remaining: what if this.resolveType was set prematurely? The current strategy is to use only getResolvedCopyForInferenceTargeting() and isCompatibleWith(), until we are sure what the target type will be. This *should* be safe in terms of saying that once we have a resolvedType this is it. [1] http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?h=BETA_JAVA8&id=d71b3ce043b24a156efd3f7e8d9fe75d5e1e2f32 I should probably do a similar exercise for the MessageSend case.
(In reply to Stephan Herrmann from comment #4) > I should probably do a similar exercise for the MessageSend case. That call is unnecessary since the fix for bug 424167. In this case it's not directly obvious which part of the patch is relevant for the matter at hand. We may even say that any matters of type inference pre-dating bug 424167 should be considered pre-historic, not relevant for modern historiography. I'm currently running all tests at the state of bug 424167 minus the calls from MessageSend to unresolve(). If that succeeds I'll replace the calls with diagnostic harness as mentioned above.
(In reply to Stephan Herrmann from comment #5) > I'm currently running all tests at the state of bug 424167 minus the calls > from MessageSend to unresolve(). If that succeeds I'll replace the calls > with diagnostic harness as mentioned above. Two more regressions in that experiment (NLET.testBug423129b, LET.testLambdaInference1), which both triggered by new diagnostic code. These two were fixed via bug 424710, more specifically by this change inside ASTNode.resolvePolyExpressionArguments(): from: updatedArgumentType = argument.checkAgainstFinalTargetType(parameterType); to: if (infCtx != null && infCtx.stepCompleted == InferenceContext18.BINDINGS_UPDATED) updatedArgumentType = argument.resolvedType; // in this case argument was already resolved via InferenceContext18.acceptPendingPolyArguments() else updatedArgumentType = argument.checkAgainstFinalTargetType(parameterType); This gives a perfectly reasonable explanation, why we're now more successful in avoiding double resolution. Looks like we're on a good track here. Rerunning tests at state after the fix for bug 424710 (Jan 6) which my pending changes.
I viewed it from the point of view of answering the question: Where exactly is the second or third or any subsequent call to resolveType originating from ? I don't see a control flow that would reach it.
(In reply to Srikanth Sankaran from comment #7) > I viewed it from the point of view of answering the question: Where exactly > is the second or third or any subsequent call to resolveType originating > from ? Well, *in the past* the machinery around IC18.rebindInnerPolies() and ASTNode.resolvePolyExpressionArguments() was driven by apparently incomplete logic and "succeeded" to call checkAgainstFinalTargetType() on an already resolved node. > I don't see a control flow that would reach it. Great, so we seem to agree on removing unresolve(). I have code in my workspace which - for the unlikely case we still run into this situation [1] - will raise an error "Error detected during type inference: Argument was unexpectedly found resolved", OK? Problem id = GenericInferenceError. This only reminds me, that we might want to avoid releasing this id as public API?? [1] As you see I'm still not 100% sure those control flows will *never* occur, after the same problem was triggered from several quite different situations in the past. Can you accept my cowardice? :)
(In reply to Stephan Herrmann from comment #8) > [1] As you see I'm still not 100% sure those control flows will *never* > occur, after the same problem was triggered from several quite different > situations in the past. Can you accept my cowardice? :) Hi Stephan, in that case, I vote for simply retargetting this to 4.4 and deal with at leisure.
Created attachment 239979 [details] proposed change For illustration: this is the change I was going to include in my next big test run. With this diagnostic code in place we'd get the information we need in case the situation still occurs in some corner case. Let me propose: wait if bug 428254 hits this sore spot and decide after that one is well understood, OK?
After the connection to bug 428254 was a red herring, I went ahead and released the patch that swaps the unresolve() calls with: problemReporter().genericInferenceError("Argument was unexpectedly found resolved"); I'm comfortable with removing unresolve() I just don't (yet) want to let it go unnoticed should the double resolving still occur. Released via http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?h=BETA_JAVA8&id=6af4748240d783fb0fd75666c88ce3ea32b7386e I'll leave this bug open at reduced priority as a reminder to also remove the error call (for 4.4 sounds fine).
(In reply to Stephan Herrmann from comment #11) > I'll leave this bug open at reduced priority as a reminder to also remove > the error call (for 4.4 sounds fine). I've created bug 428489 as the new home for this cleanup. Resolving.
Verified by code inspection at the time of Eclipse + Java 8 RC1.
