Community
Participate
Working Groups
Created attachment 250278 [details] proposed fixes In experiments regarding bug 458201 I ran into an NPE. It starts when I say classFile.getWorkingCopy((WorkingCopyOwner)null, null); on a classFile representing a class of an external jar, for which a source attachment exists. In BecomeWorkingCopyOperation.executeOperation() we NPE upon a non-existent resource. I tried two ways for avoiding the NPE: 1. If resource == null completely avoid adding a delta. This approach caused another NPE down the line: Util.getUnresolvedJavaElement(TypeBinding ...) found a SourceTypeBinding for which none of the following attempts succeed: - check if file is *.class -> create a ClassFile - else getCompilationUnit() In this situation fileName referred to a *.jar, => we return null. NPE occurred when the result of a recursive invocation was not checked for null (the type variable branch). Should jars be handled here? Or is just the fileName wrong? Should it point to the class file insided the jar? 2. As an alternative I tried branching into "// report an ADDED delta". This one additionally(?) caused CCE in ClassFile.existsUsingJarTypeCache() where we expect a ClassFile but now have a ClassFileWorkingCopy. Here an instanceof check seems a natural solution. I'm attaching a patch which should make all three locations safer - for good measure. It demonstrates (2) even though the other option might be preferrable(?)
The issues should be reproducible when applying the patch attachment 250165 [details] (from the JDT/UI bug 458201). Then in a runtime workbench, open any class from the JRE, and try Ctrl-1 somewhere in the class file editor. For the 2nd NPE (in Util) one needs to select a type variable before hitting Ctrl-1. @Jay, can you advise regarding the choice (1) or (2) above? I.e., should we generate a delta (added) for a ClassFileWorkingCopy, or just skip that step? Investigating, whether Util.getUnresolvedJavaElement() should handle jar files sounds like a bonus, but since null seems to be a legal return value, a simple null return if null was found sounds fair enough, IMHO.
Created attachment 250325 [details] proposed fixes v2 This version balances the fix in BecomeWorkingCopyOperation with a corresponding fix in DiscardWorkingCopyOperation, which is necessary to prevent leaking a ClassFileWorkingCopy. I also found that the deltas are part of the API contract, so the current solution should be the right thing to do (rather than completely avoiding this part for class files). Whether or not these fixes will be needed depends on whether bug 458201 is accepted or not.
(In reply to Stephan Herrmann from comment #1) > @Jay, can you advise regarding the choice (1) or (2) above? I.e., should we > generate a delta (added) for a ClassFileWorkingCopy, or just skip that step? Stephan, I haven't applied the patch and look at the code yet or the problems you ran into, but I think it's okay not to generate delta for class file working copy? At the moment I can't imagine why we would need that.
I caught one more situation where a ClassFileWorkingCopy causes grief downstream: ASTParser. Luckily, simply reverting to the underlying ClassFile resolves the issue in a two-liner. Symptom was: CU generated from the CFWC had a wrong file name (the jar name), which caused resolving to fail (IProblem.PublicClassMustMatchFileName). Special handling for exact this issue is already present for ClassFile. That's what the fix piggy-backs on. Feature branch, commit 50247d3918ef4bb3d4a8928e6849a54c53425221
New Gerrit change created: https://git.eclipse.org/r/41754
Gerrit change https://git.eclipse.org/r/41754 was merged to [master]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=09617092d586fc57d56d7255b395176bf3d6b1e7
I've released (for 4.5M6) the safety net for those locations where ClassFileWorkingCopy has been observed to trigger exceptions.
Verified for 4.5 M6 by code inspection.
