Community
Participate
Working Groups
When researching the meaning of "The field X.f is never used locally" in the context of bug 185682 I came about the following example: public class Outer { private class InnerPriv { public boolean flag = true; } public class InnerPub extends InnerPriv { } } The compiler warns: "The field Outer.InnerPriv.flag is never read locally" However, the following client will prove the compiler "wrong": public class Challenge { boolean test(Outer o) { return o.new InnerPub().flag; } } One might argue that the field is indeed not read *locally*, but the warning is a false positive: It would be wrong to remove this 'unused' field. I understand the logic behind the analysis as: members of a private class which do not implement/override a member of a non-private superclass can only be accessed from the declaring scope of the private class. However, by subclassing the private class, those members can leak to clients outside the current compilation unit. I'd propose the following strategy: when connecting type hierarchies tag all private types that are supertype of a non-private type. Tagged private types should not be included in the 'unused' analysis.
Bug 201912 which introduced this specific analysis did not discuss the kind of leak I constructed in my example. This makes me think this is indeed an oversight.
(In reply to comment #1) > Bug 201912 which introduced this specific analysis did not discuss > the kind of leak I constructed in my example. > This makes me think this is indeed an oversight. Looks like it. We should indeed not consider these fields as being "private" as they are exposed from the subclass.
Created attachment 181578 [details] proposed fix & test This is the simple fix I was thinking of. The key concept is a new flag TagBits.IsPrivateWithNonPrivateSub, which is set on all private classes that have a non-private subclass, signaling that the scope of the class is not closed as for other private classes (restricted to the enclosing compilation unit). However, when running tests against my patch I see a conflict with ProblemTypeAndMethodTest.test099() on behalf of bug 296660. I suggest that my patch should actually replace the solution in bug 296660, the reasons I will give in that bug. One might want to discuss whether the new bit should actually be an ExtraCompilerModifier instead of a TagBit, both bitsets being limited resources.
Created attachment 181581 [details] patch v2 By a closer look at bug 296660 I learned that both bugs overlap, but neither subsumes the other. Thus, here is a combined patch that provides both solutions under a common strategy. From this bug we take the capability to handle fields (which bug 296660 ignores), from the other bug we take the capability to handle method overriding (which my previous patch ignored). The resulting strategy is as follows: - private classes with a non-private subclass are tagged with TagBits.IsAccessibleViaNonPrivateSub, but they are still reported as isOrEnclosedByPrivateType(). - non-private fields of thusly tagged classes are also tagged as TagBits.IsAccessibleViaNonPrivateSub (recursively up the superclass chain). Once tagged these fields answer false to isOrEnclosedByPrivateType(). - methods inherited by an accessible class (non-isOrEnclosedByPrivateType) are also tagged iff they are not overridden in the accessible class. Once tagged these methods answer false to isOrEnclosedByPrivateType(). BTW, when creating the patch I was again hit by bug 214085, caused by non ascii § in a file without explicit encoding (MethodVerifier15).
Srikanth: since this patch modifies your patch from bug 296660 would you like to comment? I think the change is sufficiently limited in scope so we can safely release this for M3, OK?
(In reply to comment #5) > Srikanth: since this patch modifies your patch from bug 296660 > would you like to comment? Comments on the patch: - (matter of/subject to taste): Is IsAccessibleViaNonPrivateSub better named IsAccessibleViaSubclass or IsExposedViaSubclass ? - ClassScope.java: Moving the first below to just below SourceTypeBinding sourceType = this.referenceContext.binding; and using sourceType in the new code rather than this.referenceContext.binding leads to cleaner code ? - IMO, the changes to the various implementations of isOrEnclosedByPrivateType are not in good style. It is true that these methods are used today only to warn about unused private members, but the current changes break the behavior/contract implied/expected given the name of the method. - The cast in org.eclipse.jdt.internal.compiler.lookup.SourceTypeBinding.tagIndirectlyAccessibleFields() viz. ((SourceTypeBinding)this.superclass) is dangerous. It assumes that a source type can have only another source type as its super class. - I am not sure we even need a separate tag bit to accomplish this. I'll try and come up with a patch shortly that will eliminate this tag bit. - I think the fix to bug 296660 as well as the current one ignore types. Is there something that needs to be done for types ? I didn't test to verify that there is indeed a problem with types, but would like to verify that methods, fields and types all get uniform treatment. > I think the change is sufficiently limited in scope so we can safely > release this for M3, OK? I would suggest we not rush this, let it cook for a while and we can take care of it just after M3.
Created attachment 181611 [details] Proposed alternate patch This is a much simpler patch. This has not been tested fully. It passes all tests in ProgrammingProblemsTest and ProblemTypeAndMethodTest
(In reply to comment #7) > Created an attachment (id=181611) [details] > Proposed alternate patch > > This is a much simpler patch. This has not been tested > fully. It passes all tests in ProgrammingProblemsTest > and ProblemTypeAndMethodTest I agree, this is simpler and looks essentially good to me. Does it handle this case (I can't test right now)? class Outer { private class Inner1 { int foo; } private class Inner2 extends Inner1 { } class Inner3 extends Inner2 { } // foo is exposed here } However, in this patch the flag AccLocallyUsed is no longer used in accordance with its name. We certainly wouldn't want to call it AccLocallyUsedOrAccesibleViaNonPrivateSubclass or AccWeCannotSafelySayItIsUnused, but we should at least document the twofold use of this flag. I assume member types of private classes should be treated like fields. BTW, regarding the cast I was assuming that a private class can only be used as the superclass when both types are within the same CU (=> STB). Obviously, I should have documented that assumption. I won't be able to work on it before today's call, so, yes, lets put this into M4.
(In reply to comment #8) > Does it handle this case (I can't test right now)? > class Outer { > private class Inner1 { > int foo; > } > private class Inner2 extends Inner1 { } > class Inner3 extends Inner2 { } // foo is exposed here > } No, I inadvertently deleted the recursive call in your patch. That will have to be reinstated. > However, in this patch the flag AccLocallyUsed is no longer used > in accordance with its name. We certainly wouldn't want to call it > AccLocallyUsedOrAccesibleViaNonPrivateSubclass or > AccWeCannotSafelySayItIsUnused, but we should at least document the > twofold use of this flag. Yes, but there are many other places already where this flag is used to mean AccWeCannotSafelySayItIsUnused. Agree it would be a good idea to document this. > I assume member types of private classes should be treated like fields. Very likely yes. > BTW, regarding the cast I was assuming that a private class can only be > used as the superclass when both types are within the same CU (=> STB). > Obviously, I should have documented that assumption. I missed that point, yes, a comment would help. > I won't be able to work on it before today's call, so, yes, lets > put this into M4. Sounds good.
(In reply to comment #9) > No, I inadvertently deleted the recursive call in your patch. > That will have to be reinstated. Basically I started with a clean work space and started copy pasting some elements from your fix, So my patch is also missing the copyrights etc you had added. Sorry about that.
(In reply to comment #10) > Basically I started with a clean work space Which is probably why you come up with a cleaner patch :) No problem, I'll collect all findings into a new patch.
Created attachment 193589 [details] improved patch with tests This patch should combine all findings so far. I started with the alternate patch from comment 7 and made these changes: * The transitive case has been re-fixed (recursive call in STB.tagIndirectlyAccessibleFields()). * In the same method I added an instanceof check before the cast (just to play real safe) and documented why it should always yield true. * Added code comments to AccLocallyUsed. Summarizing the state of affairs for non-field members: * For methods the issue is already handled as per bug 296660. * Member types were not correctly handled, I added treatment similar to fields in STB.tagIndirectlyAccessibleFields() (now called tagIndirectlyAccessibleMembers()). New test is ProgrammingProblemTest.test0052b(). Tests are currently running.
What do you think, Srikanth?
Patch looks good.
Released for 3.7M7.
Verified for 3.7M7 using build I20110425-1800.
