Community
Participate
Working Groups
Source code based, v_770 One of the overrides of ReferenceBinding#superInterfaces() manages to return null in some circumstances. Discovered this while working on bug 77918 (that should be revisited if we decide to fix this one). Best way to see the problem is to apply the patch below, then run TypeHierarchyTests: testCycle2 then raises an error. ### Eclipse Workspace Patch 1.0 #P org.eclipse.jdt.core Index: compiler/org/eclipse/jdt/internal/compiler/lookup/ClassScope.java =================================================================== RCS file: /cvsroot/eclipse/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/lookup/ClassScope.java,v retrieving revision 1.149 diff -u -r1.149 ClassScope.java --- compiler/org/eclipse/jdt/internal/compiler/lookup/ClassScope.java 15 May 2007 14:39:22 -0000 1.149 +++ compiler/org/eclipse/jdt/internal/compiler/lookup/ClassScope.java 14 Jun 2007 09:25:08 -0000 @@ -956,8 +956,18 @@ sourceType.tagBits |= TagBits.EndHierarchyCheck; noProblems &= connectTypeVariables(referenceContext.typeParameters, false); sourceType.tagBits |= TagBits.TypeVariablesAreConnected; - if (noProblems && sourceType.isHierarchyInconsistent()) - problemReporter().hierarchyHasProblems(sourceType); + if (noProblems) { + if (sourceType.isHierarchyInconsistent()) { + problemReporter().hierarchyHasProblems(sourceType); + } else { + for (ReferenceBinding current = this.referenceContext.binding.superclass(); + current != null; current = current.superclass()) { + if (current.superInterfaces() == null) { + throw new RuntimeException("should not get here"); + } + } + } + } } connectMemberTypes(); LookupEnvironment env = environment(); Note: this behavior seems to be specific to direct accesses to the model; attempts to reproduce within a standard build context failed. While we could merely accept that superInterfaces() sometimes return null, considerable efforts are made throughout our code to avoid this and to return NO_INTERFACES at worst, hence I'd rather get this isolated case eliminated.
This has consequences for bug 77918. If this bug gets resolved after 77918, please revisit the code accordingly.
The test in the patch is flawed. Here are the 2 types: class CycleParent extends CycleBase<CycleChild> {} class CycleChild extends CycleParent implements Comparable<CycleChild> {} Since CycleParent is computing its superclass at the time that CycleChild is asked to find its superclass, why would CycleParent knows its superinterfaces? It doesn't indicate that CycleParent will answer null to superinterfaces() once its finished connecting its type hierarchy & in fact it doesn't - it answers the empty array. ALL type hierarchies need to be finished & connected before you can expect to get non-null results.
So the situation would be: - finished connecting and everything connected fine => superInterfaces() is guarantied to return non-null, potentially NO_INTERFACES; - finished connecting and something wrong => superInterfaces() is guarantied to return non-null, potentially NO_INTERFACES, the latter potentially meaning something got wrong, or else (really) no interfaces; - not finished connecting => whatever, including null. Note that we are talking about superInterfaces(), not superInterfaces. Right?
There are no guarantees & we're definitely checking superinterfaces() against null in some places & under specific circumstances. Have you found another case where we need to check for null?
See senders of superInterfaces() in ClassScope
I would not consider the fact that we protect ourselves as a definite proof that we need to. The fact is, applying the patch I suggest in my initial post to our code yields a single error when running all our tests. (Should have told that earlier.) Hence it seemed to me that the null case was really rare. Moreover, I have seen specific places in the code where we returned NO_INTERFACES rather than null when encountering problems. I thought I should raise the issue. My question finally boils down to: do we want a 'quadristate' (non-empty array, NO_INTERFACES meaning no interfaces, NO_INTERFACES meaning problem, null meaning uninitialized - or problem?) or not, and what is the meaning of each state in each circumstance (especially for the NO_INTERFACES meaning problem: suggests that we simply swallow issues here, except if we're 100% sure we raised errors)? If we are perfectly happy with what we have, then it's OK with me. If we feel there is room for improvement, then it might be the best time to tackle them. The current suggested fix for bug 77918 introduces a test against null that could be removed if we guarantied that ReferenceBinding#superInterfaces() never returned null. This is the one which probed the test case of my initial post. If you want to, I'll equip each calling point with a null test and make a stat of how often this happens.
