Community
Participate
Working Groups
Build Identifier: When the commons-lang-2.3.jar is added to a project or the rg.apache.commons.lang_2.3.0.v200803061910.jar from Orbit is used then some classes can no longer be refactored. Reproducible: Always Steps to Reproduce: 1. Add the plug-in @ http://download.eclipse.org/tools/orbit/downloads/drops/R20100114021427/bundles/org.apache.commons.lang_2.3.0.v200803061910.jar to your target platform. 2. Import the attached zip file. 3. Attempt to rename the Foo.getName() method.
Created attachment 172186 [details] Example project demonstrating the problem
When the refactoring fails you will find the following in the log: eclipse.buildId=I20100520-1744 java.version=1.6.0_20 java.vendor=Sun Microsystems Inc. BootLoader constants: OS=win32, ARCH=x86, WS=win32, NL=en_US Command-line arguments: -os win32 -ws win32 -arch x86 -data d:\dev\workspace-5.0.x This is a continuation of log file D:\dev\workspace-5.0.x\.metadata\.bak_0.log Created Time: 2010-06-15 12:45:18.124 Error Thu Jun 17 20:30:16 CDT 2010 Internal Error java.lang.reflect.InvocationTargetException at org.eclipse.ltk.internal.ui.refactoring.WorkbenchRunnableAdapter.run(WorkbenchRunnableAdapter.java:91) at org.eclipse.jface.operation.ModalContext$ModalContextThread.run(ModalContext.java:121) Caused by: Java Model Exception: Java Model Status [org.apache.commons.lang.enum [in lib/commons-lang-2.3.jar [in com.itko.lisa.platform]] does not exist] at org.eclipse.jdt.internal.core.JavaElement.newJavaModelException(JavaElement.java:502) at org.eclipse.jdt.internal.core.Openable.generateInfos(Openable.java:246) at org.eclipse.jdt.internal.core.Openable.openAncestors(Openable.java:504) at org.eclipse.jdt.internal.core.Openable.generateInfos(Openable.java:240) at org.eclipse.jdt.internal.core.SourceRefElement.generateInfos(SourceRefElement.java:107) at org.eclipse.jdt.internal.core.JavaElement.openWhenClosed(JavaElement.java:515) at org.eclipse.jdt.internal.core.BinaryType.getElementInfo(BinaryType.java:286) at org.eclipse.jdt.internal.core.JavaElement.getElementInfo(JavaElement.java:238) at org.eclipse.jdt.internal.core.BinaryType.isInterface(BinaryType.java:723) at org.eclipse.jdt.internal.corext.refactoring.rename.RippleMethodFinder2.findAllRippleMethods(RippleMethodFinder2.java:242) at org.eclipse.jdt.internal.corext.refactoring.rename.RippleMethodFinder2.getAllRippleMethods(RippleMethodFinder2.java:168) at org.eclipse.jdt.internal.corext.refactoring.rename.RippleMethodFinder2.getRelatedMethods(RippleMethodFinder2.java:161) at org.eclipse.jdt.internal.corext.refactoring.structure.ChangeSignatureProcessor.checkFinalConditions(ChangeSignatureProcessor.java:817) at org.eclipse.ltk.core.refactoring.participants.ProcessorBasedRefactoring.checkFinalConditions(ProcessorBasedRefactoring.java:224) at org.eclipse.ltk.core.refactoring.CheckConditionsOperation.run(CheckConditionsOperation.java:85) at org.eclipse.ltk.core.refactoring.CreateChangeOperation.run(CreateChangeOperation.java:121) at org.eclipse.core.internal.resources.Workspace.run(Workspace.java:1975) at org.eclipse.ltk.internal.ui.refactoring.WorkbenchRunnableAdapter.run(WorkbenchRunnableAdapter.java:87) ... 1 more Root exception: Java Model Exception: Java Model Status [org.apache.commons.lang.enum [in lib/commons-lang-2.3.jar [in com.itko.lisa.platform]] does not exist] at org.eclipse.jdt.internal.core.JavaElement.newJavaModelException(JavaElement.java:502) at org.eclipse.jdt.internal.core.Openable.generateInfos(Openable.java:246) at org.eclipse.jdt.internal.core.Openable.openAncestors(Openable.java:504) at org.eclipse.jdt.internal.core.Openable.generateInfos(Openable.java:240) at org.eclipse.jdt.internal.core.SourceRefElement.generateInfos(SourceRefElement.java:107) at org.eclipse.jdt.internal.core.JavaElement.openWhenClosed(JavaElement.java:515) at org.eclipse.jdt.internal.core.BinaryType.getElementInfo(BinaryType.java:286) at org.eclipse.jdt.internal.core.JavaElement.getElementInfo(JavaElement.java:238) at org.eclipse.jdt.internal.core.BinaryType.isInterface(BinaryType.java:723) at org.eclipse.jdt.internal.corext.refactoring.rename.RippleMethodFinder2.findAllRippleMethods(RippleMethodFinder2.java:242) at org.eclipse.jdt.internal.corext.refactoring.rename.RippleMethodFinder2.getAllRippleMethods(RippleMethodFinder2.java:168) at org.eclipse.jdt.internal.corext.refactoring.rename.RippleMethodFinder2.getRelatedMethods(RippleMethodFinder2.java:161) at org.eclipse.jdt.internal.corext.refactoring.structure.ChangeSignatureProcessor.checkFinalConditions(ChangeSignatureProcessor.java:817) at org.eclipse.ltk.core.refactoring.participants.ProcessorBasedRefactoring.checkFinalConditions(ProcessorBasedRefactoring.java:224) at org.eclipse.ltk.core.refactoring.CheckConditionsOperation.run(CheckConditionsOperation.java:85) at org.eclipse.ltk.core.refactoring.CreateChangeOperation.run(CreateChangeOperation.java:121) at org.eclipse.core.internal.resources.Workspace.run(Workspace.java:1975) at org.eclipse.ltk.internal.ui.refactoring.WorkbenchRunnableAdapter.run(WorkbenchRunnableAdapter.java:87) at org.eclipse.jface.operation.ModalContext$ModalContextThread.run(ModalContext.java:121)
I think there may be an issue with the fact that the commons-lang jar has a java package org.apache.commons.lang.enum, that ends in enum. If you bring up the open type dialog and start typing EnumUtils, you will find 2 packages with the class. One with the name org.apache.commons.lang.enum and the other is org.apache.commons.lang.enums. However, if you select to open the org.apache.commons.lang.enum.EnumUtils class then eclipse displays a dialog that says "Type 'org.apache.commons.lang.enum.EnumUtils' could not be found in <path to the jar>' Make sure all the workspace resources are refreshed.". Another potential clue is that if you look at the Plug-in Dependencies for the project, and expand the commons-lang entry then you will see that it doesn't even list the org.apache.commons.lang.enum package at all.
Hmm, I forgot to put the Eclipse version as this fails with Eclipse 3.6RC4. However, I also checked versions 3.5.2, 3.4.2 and 3.3.2 and only version 3.3.2 successfully refactors the class. So there is a regression between 3.3.2 and 3.4.2. What makes this a major pain, is that the failure of the refactoring is simply based on the presence of the jar file not as if any code from the jar is involved in the refactoring.
Reproduced in N20100617-2000. The problem is indeed that the JAR contains a package org.apache.commons.lang.enum. (The package is deprecated, and there's already a replacement called org.apache.commons.lang.enums.) When you change the JRE System Library of the org.foo.bar project to J2SE-1.3, then everything works fine. In this case, JDT/Core could detect that org.apache.commons.lang_2.3.0.v200803061910.jar contains old classfiles, and choose the compiler compliance for the JAR according to the class file version. If that's too complicated, you could also fix the search engine, such that it doesn't report the bad matches any more. See also bug 90120.
This is actually a variant of bug 293861. There, the package name was always invalid, but here, it's only invalid for certain compiler compliance levels.
Satyam, since you worked on 293861, can you please follow up and see what can be the better approach for fixing this? Thanks!
Created attachment 175371 [details] Proposed patch Patch to filter type names of assert and enum for the appropriate project compliance. This is done only for the Search api's which return the types as their outputs. I have tried to have a minimal performance overhead. Hence, the code is duplicated in two places.
Created attachment 175372 [details] Jar file for the added test This jar need to be copied into org.eclipse.jdt.core.tests.model/workspace/JavaSearchBugs/lib for the test to run.
Jay, Can you review this patch, TIA.
Finally, I am able to reproduce the bug. This is what I overlooked: The bug doesn't appear once you refresh your Java project. Looking at the video, I observe that the nested folders are created, jar is copied (note that all this inside the project) and immediately the user library is created using the newly added Jar. When the jar is added to the library, there is no error in the dialog. However, when we close the dialog and reopen again, we see the error. When the project is refreshed, the error goes away. Investigating further.
(In reply to comment #11) > Finally, I am able to reproduce the bug. This is what I overlooked: The bug > doesn't appear once you refresh your Java project. Looking at the video, I > observe that the nested folders are created, jar is copied (note that all this > inside the project) and immediately the user library is created using the newly > added Jar. When the jar is added to the library, there is no error in the > dialog. However, when we close the dialog and reopen again, we see the error. > > When the project is refreshed, the error goes away. Investigating > further. Can we just ignore this comment? This doesn't belong here. Sorry.
(In reply to comment #8) > Created an attachment (id=175371) [details] > Proposed patch > > Patch to filter type names of assert and enum for the appropriate project > compliance. This is done only for the Search api's which return the types as > their outputs. > I have tried to have a minimal performance overhead. Hence, the code is > duplicated in two places. Patch looks fine, Satyam. One minor suggestion is to use some constants instead of literals for "enum" and "assert".
(In reply to comment #13) > Patch looks fine, Satyam. One minor suggestion is to use some constants instead > of literals for "enum" and "assert". Jay, Thanks for the review. I couldn't put a nice place to extract these into constants. Hence, leaving it.
Released the patch on HEAD
Verified for 3.7M1 using build I20100802-1800.
Verified.
Frederic, what is your take on back porting this to 3.6.1 ?
(In reply to comment #18) > Frederic, what is your take on back porting this to 3.6.1 ? I think this is OK to backport this fix to 3.6.1. (In reply to comment #8) > Created an attachment (id=175371) [details] > Proposed patch > > Patch to filter type names of assert and enum for the appropriate project > compliance. This is done only for the Search api's which return the types as > their outputs. > I have tried to have a minimal performance overhead. Hence, the code is > duplicated in two places. I think it's possible to still improve the performance by doing the filtering only for class files. In fact as this is a JDK compatibility issue, that cannot happen for source files as they would have got a compiler error on the keyword name, hence I think we can avoid to call the filterMatch method systematically. In TypeNameMatchRequestorWrapper.acceptType, we already know whether the match is from a class file or not. In MatchLocator, I guess that the call of filterMatch can be only done in reportBinaryMemberDeclaration, just before reporting the match...
*** Bug 268293 has been marked as a duplicate of this bug. ***
I took a look at the patch since it's in a critical area. Besides the comments from Frédéric, I'd like to not have the code duplication (==> smaller patch for 3.6.1): add one helper method that can be called to filter. We also need this method in the completion engine because currently code assist still returns the types inside *.enum.* or *.assert.* packages. It would be good if the 3.6.1 fix would fix the entire problem once and for all (depending on the size of the fix in the completion engine).
(In reply to comment #21) > I took a look at the patch since it's in a critical area. Besides the comments > from Frédéric, I'd like to not have the code duplication (==> smaller patch for > 3.6.1): add one helper method that can be called to filter. We also need this > method in the completion engine because currently code assist still returns the > types inside *.enum.* or *.assert.* packages. It would be good if the 3.6.1 fix > would fix the entire problem once and for all (depending on the size of the fix > in the completion engine). Dani, I haven't done for the completion engine because of the impact of the performance that it could make. As it is rare and the impact could be very less, I will not fix for 3.6.1, but will try to get one for 3.7. I didn't use an helper method, because I was trying to optimize for performance and I was trying to get the project compliance as late as possible. However, I think I can change the strategy and change it to an helper. Let me try out.
>I didn't use an helper method, because I was trying to optimize for performance Did you actually measure any performance problem? If, not I wouldn't start to mess up the code until then. Same for completion engine: do you have numbers of the performance impact?
(In reply to comment #23) > >I didn't use an helper method, because I was trying to optimize for performance > Did you actually measure any performance problem? If, not I wouldn't start to > mess up the code until then. Same for completion engine: do you have numbers of > the performance impact? I don't have the numbers but my initial trials weren't good for the completion engine. I have tried doing the check at searchAllTypeNames() which returns just names and the results weren't good. Instead of doing at that api level, I probably can try doing something at the Completion engine. I didn't measure with the helper method. I am doing it now.
Frédéric, for the completion engine, couldn't we add a filter just right before we'd report the match? Of course again, only for binary stuff.
(In reply to comment #25) > Frédéric, for the completion engine, couldn't we add a filter just right before > we'd report the match? Of course again, only for binary stuff. It does look to be possible and am doing it now.
(In reply to comment #25) > Frédéric, for the completion engine, couldn't we add a filter just right before > we'd report the match? Of course again, only for binary stuff. As the completion engine relies on the result of a search all type names request but using a different requestor (an IRestrictedAccessTypeRequestor instead of TypeNameMatchRequestorWrapper ). So, as Satyam said, the filter needs to be implemented in the completion engine in order to be placed just before definitely accepting the type (typically this filter should be called near the end of the CompletionEngine.acceptType(...) method). Additional code should be similar methods than those added to TypeNameMatchRequestorWrapper and logically that should have the same performance impact than for the open type operation...
Satyam, Unfortunately my first verification of performance was wrong. Looking closer at performance results, I finally did notice a 10% regression on the FullSourceWorkspaceSearchTests#testSearchAllTypeNameMatches() perf test :-( Which means that the Open Type operation will also get a performance regression. Not sure whether this performance regression is a no go for the patch or not...!?
(In reply to comment #28) > Satyam, > > Unfortunately my first verification of performance was wrong. Looking closer at > performance results, I finally did notice a 10% regression on the > FullSourceWorkspaceSearchTests#testSearchAllTypeNameMatches() perf test :-( > > Which means that the Open Type operation will also get a performance > regression. Not sure whether this performance regression is a no go for the > patch or not...!? I believe doing only for binary types should get the test fine, but yes there will be some performance degradation for the binary types, which we are probably not testing. I think we can beat this further more by trying to do it only once for a package fragment.
> So, as Satyam said, the filter >needs to be implemented in the completion engine I didn't say something else, actually my questions started with "Frédéric, for the completion engine," ;-) >I think we can beat this further more by trying to do it >only once for a package fragment. Not sure about this. Wouldn't this mean to cache the info? If so, accessing the cache might be slower than do the filtering. Let's do a step back: in the past 10 years the only bug reports about 'enum' or 'assert' causing trouble was with the package in the org.apache.commons.lang JAR. So, instead of doing the full fix with performance impact for everyone, could we use that information to do a less invasive fix just for that JAR, e.g. caching a global Boolean which tells whether such a JAR is known to the Java model and only then to the additional filtering?
(In reply to comment #30) > Let's do a step back: in the past 10 years the only bug reports about 'enum' or > 'assert' causing trouble was with the package in the org.apache.commons.lang > JAR. So, instead of doing the full fix with performance impact for everyone, > could we use that information to do a less invasive fix just for that JAR, e.g. > caching a global Boolean which tells whether such a JAR is known to the Java > model and only then to the additional filtering? Dani, Thanks for the interesting data point. I am sure, there will be very minimal performance impact if we check only for that jar.
Satyam, Please undo the change in HEAD to bring back the Open Type performance in 3.7 to its original value. TIA
(In reply to comment #31) > (In reply to comment #30) > > Let's do a step back: in the past 10 years the only bug reports about 'enum' or > > 'assert' causing trouble was with the package in the org.apache.commons.lang > > JAR. So, instead of doing the full fix with performance impact for everyone, > > could we use that information to do a less invasive fix just for that JAR, e.g. > > caching a global Boolean which tells whether such a JAR is known to the Java > > model and only then to the additional filtering? > Dani, Thanks for the interesting data point. I am sure, there will be very > minimal performance impact if we check only for that jar. Yes, I think this is a real good idea. Maybe this boolean can be set while initializing the index locations in the IndexSelector...
>initializing the index locations in the IndexSelector... Is that triggered when adding/removing stuff from the build path of a project? Otherwise we might just update the flag when modifying a classpath.
(In reply to comment #34) > >initializing the index locations in the IndexSelector... > Is that triggered when adding/removing stuff from the build path of a project? > Otherwise we might just update the flag when modifying a classpath. The IndexSelector.initializeIndexLocations() method is called for each search request to see which indexes should be used to perform the search. For doing that, it uses a JavaSearchScope which is aware of any project classpath change. So, IMO, it's the best place to set this boolean...
>So, IMO, it's the best place to set this boolean... I would disagree for two reasons: 1. why test it every time when we know the workspace does not contain that JAR? 2. the test might be expensive Each time a project gets this JAR on its build path we could increase a counter and decrease it when one goes away.
(In reply to comment #36) Comment 35 might do the trick if the flag computation out of the JavaSearchScope is fast.
(In reply to comment #37) > (In reply to comment #36) > Comment 35 might do the trick if the flag computation out of the > JavaSearchScope is fast. That's what I'm expecting. This additional test should not have any performance impact. It's done only once at the beginning of the search request and should be definitely negligible regarding of the rest of the search operation (looking at indexes and matches locating...)
(In reply to comment #38) > (In reply to comment #37) > > (In reply to comment #36) > > Comment 35 might do the trick if the flag computation out of the > > JavaSearchScope is fast. > > That's what I'm expecting. This additional test should not have any performance > impact. It's done only once at the beginning of the search request and should > be definitely negligible regarding of the rest of the search operation (looking > at indexes and matches locating...) Sounds good then. Is safer than adding code that tries to keep a ref count.
(In reply to comment #32) > Satyam, > > Please undo the change in HEAD to bring back the Open Type performance in 3.7 > to its original value. > > TIA I have reverted back the changes.
Created attachment 177329 [details] Proposed patch on HEAD Made the patch to check only for org.apache.commons.lang.enum in org.apache.commons.lang_2*.jar. Have set a boolean by checking for this jar in a minimalistic way. This patch also takes care for content assist. Frederic, Please review.
Created attachment 177332 [details] Jar file for the added test This jar file needs to be copied into \org.eclipse.jdt.core.tests.model\workspace\JavaSearchBugs\lib\b317264 and \org.eclipse.jdt.core.tests.modelworkspace\Completion\b317264 for the tests to run
(In reply to comment #41) > Created an attachment (id=177329) [details] > Proposed patch on HEAD > > Made the patch to check only for org.apache.commons.lang.enum in > org.apache.commons.lang_2*.jar. Have set a boolean by checking for this jar in > a minimalistic way. This patch also takes care for content assist. > Frederic, Please review. Patch looks good to me except the static variable DOT_ENUM in CompletionEngine. It's defined as: private final static char[] DOT_ENUM = "enum".toCharArray(); //$NON-NLS-1$ and used with following comment: ...DOT_ENUM)) { //note: it should be .enum and not just enum IMO, you missed the dot in the string. Am I right? Note also that Keywords.java already defines the constant ENUM = "enum".toCharArray(); Two other minor remarks: 1) the mayHaveEnum should be capitalized as this is a static field and maybe its name could be improved a little bit (e.g. SHOULD_FILTER_ENUM). 2) In MatchLocator.report(SearchMatch), please use the static boolean in the if condition. This will avoid to call the method filterMatch(SearchMatch) most of the time
Created attachment 177381 [details] Revised patch for HEAD Frederic, Thanks for catching the missing dot :). Have fixed those and incorporated the other feedback.
(In reply to comment #44) > Created an attachment (id=177381) [details] > Revised patch for HEAD > > Frederic, Thanks for catching the missing dot :). Have fixed those and > incorporated the other feedback. Patch looks good, +1 for 3.6.1.
Released on HEAD and 3.6.1
Nice work!
Verified for 3.6.1 RC2 using build M20100825-0800.
Verified for 3.7M2 using build I20100909-1700.
