Community
Participate
Working Groups
Build Identifier: I20110310-1119 Stack trace to be attached. Reproducible: Always Steps to Reproduce: I can reproduce the exception, but I don't know which steps are really related to the problem: 1. I have a class X which implements an interface Foo<Bar<T>>. I removed the parameter type T from Bar: X thus contains errors. 2. I start removing the type parameter "<T>" in occurrences of Bar<T> in class X: some are simple types like "Bar<T>" and some appear in array types like "Bar<T>[]". 3. When modifying an occurrence of the latter form (the selection thus being 'Bar<T>[]'), the error dialog pops up: 'An internal error occurred during: "Requesting Java AST from selection"'. 4. If I close the editor and reopen X, the exception does not occur anymore. I need to restore the previous version of the file from CVS, and then perform step 2 again.
Created attachment 193126 [details] stack trace
Hi Phillipe, Can you please elaborate on what exactly did u do that caused the exception? Was it just during manual deletion? It'll be really good if you can attach a testcase here which clearly throws the CCE. I can't reproduce this right now.
Created attachment 193148 [details] foo.zip > Can you please elaborate on what exactly did u do that caused the exception? > Was it just during manual deletion? I managed to trigger the CCE with the attached project. In fact the (manual) deletion was not mandatory. 1. import the zipped Java project 2. Open class X in the Java editor 3. click inside the return type "T1<T>[]" of method f() in order to trigger a selection event: the problem dialog should appear.
This is caused by the fix for bug 337964
(In reply to comment #4) > This is caused by the fix for bug 337964 In fact, was bug 337964 even a bug? Backing out the changes done for the fix seems to fix this issue.
Created attachment 193157 [details] proposed fix Backing out fix for bug 337964
I'll take a look.
Simply reverting the fix for 337964 is not good enough as it is causing a NPE. I'll provide a new patch shortly.
Created attachment 193158 [details] Proposed fix + regression tests
Ayushman, please review. The fix for bug 337964 was wrong. Only the usage of binding in the context of the parameterized qualified type reference had to be fixed. When the reference is a parameterized type reference (qualified or not), it can contain a dimension. So the check for array type is required.
Yup looks good.
Thanks for the test case. Released for 3.7M7. Regression tests added in: org.eclipse.jdt.core.tests.dom.ASTConverter15Test#test0351 org.eclipse.jdt.core.tests.dom.ASTConverter15Test#test0352
I tend to disagree with your solution: IMO the bug is in the compiler not the DefaultBindingResolver. At the compiler level we find a ParameterizedQualifiedTypeReference for "test0352.I1<T>[]" which has as its resolvedType a SourceTypeBinding "test0352.I1". At the point where QualifedTypeReference.internalResolveType() detects the error (I1 is not generic), we skip creating the array binding. Throughout this method, whenever we are past an assignment to resolvedType it is ensured that array bindings are created. However, also findNextTypeBinding() may cause an assignment to resolvedType in which case the scalar type may remain in place although the reference has dimensions. To me this looks inconsistent and should not be worked around in the binding resolver. I couldn't easily create another test case where inconsistent resolvedType would cause harm, because return null from internalResolveType normally aborts treatment of that element. So, maybe the field resolvedType isn't that important for code with errors? But if other clients besides the binding resolver also depend on this field I would prefer fixing in the compiler. Also note, that this case is different from the section starting at line 1570: here the second check isArrayType() is needed because newAstToOldAst is not bijective, it may indeed map, e.g., a SimpleName to an ArrayAllocationExpression. (See bug 339226).
(In reply to comment #13) > To me this looks inconsistent and should not be worked around in the binding > resolver. I couldn't easily create another test case where inconsistent > resolvedType would cause harm, because return null from internalResolveType > normally aborts treatment of that element. So, maybe the field resolvedType > isn't that important for code with errors? But if other clients besides > the binding resolver also depend on this field I would prefer fixing in the > compiler. I understand your point. We are at a point in the game where we need to be careful about making deep changes. I also considered changing the value of resolveType to be an array type if the type reference has a dimension. I wanted to fix this problem as this was introduced with a recent change (so it looks like a regression) by minimizing the impact on other usages of the resolvedType. What you suggest is perfectly fine for 3.8, but meanwhile I think this approach is safer. Please open a new bug report to fix this inconsistency for 3.8 or provide a patch that fixes this issue the way you suggested. We are at a point where we need to make trade-offs. Let me know what you think.
Created attachment 193271 [details] A new proposal Stephan, I think this is more in line with what you wanted. Please check it. The part that I could not quite see if the case where the isGenericError (ParameterizedSingleTypeReference) would return a binding without assigning it to resolvedType first. This is at least a first attempt to find a better fix for this issue.
Created attachment 193272 [details] Same patch Same patch with some unrelated changes removed.
(In reply to comment #15) > Created attachment 193271 [details] > A new proposal > > Stephan, I think this is more in line with what you wanted. Please check it. > The part that I could not quite see if the case where the isGenericError > (ParameterizedSingleTypeReference) would return a binding without assigning it > to resolvedType first. > This is at least a first attempt to find a better fix for this issue. At a first glance this looks good to me. I will give it a thorough check before our next call. Anyway, I fully understand your concerns. If we're not at ease with the new patch I'm happy with postponing for 3.8 and sticking to the previous patch.
Created attachment 193439 [details] my proposal Starting from your last patch this is what I did: Simplify ParameterizedQualifiedTypeReference by extracting a new method; now array bindings are always created at the end. For ParameterizedSingleTypeReference I made explicit the three possible outcomes (combinations of return value and what's stored in resolvedType). In this patch I assume that hasGenericError should always correspond to !resolvedType.isValidBinding() I added one more test to witness how we can still get into line 1570 of DefaultBindingResolver (the part mentioned in bug 339226) - just to document that the remaining checks are all needed. If the change in ParameterizedSingleTypeReference is good we might actually use that strategy also in ParameterizedQualifiedTypeReference. I'm currently running tests, and since the hasGenericError flag was introduced in the context of apt (bug 196200) I'm including compiler.apt.tests, too.
(In reply to comment #18) > I'm currently running tests, and since the hasGenericError flag was introduced > in the context of apt (bug 196200) I'm including compiler.apt.tests, too. Meanwhile I got my test configuration fixed and I can confirm that all jdt.core tests plus tests org.eclipse.jdt.compiler.apt.tests.AllTests pass with my patch from comment 18.
Patch looks good. Please release.
Released for 3.7M7.
Verified for 3.7M7 using build I20110425-1800.
Stephan - I know it's 4 years later, but on line 203 of compiler/org/eclipse/jdt/internal/compiler/ast/ParameterizedQualifiedTypeReference.java (in HEAD = d6d10602861f99a2e87d2ce2ba051c7b3da388a8) we have return type == null ? type : this.resolvedType; which seems odd; you'd expect `type != null : type ...`. Do you know if this might be a typo? I came across this while working on https://git.eclipse.org/r/#/c/58779/ ; I return there a non-null binding, which trips this return statement. It's vague, but with this line, a bunch of tests fail pretty catastrophically; if I change this line, I get only a handful of failures related to stuff like Map.Entry<K, V>[][] being recognized as Map.Entry, and not as an array.
(In reply to Carmi Grushko from comment #23) > Stephan - I know it's 4 years later, but on line 203 of > compiler/org/eclipse/jdt/internal/compiler/ast/ParameterizedQualifiedTypeReference.java > > > (in HEAD = d6d10602861f99a2e87d2ce2ba051c7b3da388a8) > > we have > > return type == null ? type : this.resolvedType; > > which seems odd; you'd expect `type != null : type ...`. I know, this looks odd, and I'm sorry I wrote such obfuscated code. > Do you know if this might be a typo? > > I came across this while working on https://git.eclipse.org/r/#/c/58779/ ; I > return there a non-null binding, which trips this return statement. > It's vague, but with this line, a bunch of tests fail pretty > catastrophically; if I change this line, I get only a handful of failures > related to stuff like > > Map.Entry<K, V>[][] > > being recognized as Map.Entry, and not as an array. There are two aspects to consider: - returning 'type' would bypass the side effects createArrayType() and resolveAnnotations which affect this.resolvedType. - returning 'null' is relevant as to tell the caller that an error has been detected. Generally, the protocol of handling compile errors is: (1) put a ProblemBinding or MissingTypeBinding into this.resolvedType (2) return null Step (1) is done to inform later phases of compilation about details of the error. Back to the line in question, the clearer variant would be: return type == null ? null : this.resolvedType;
