Community
Participate
Working Groups
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.4) Gecko/20091016 Firefox/3.5.4 (.NET CLR 3.5.30729) Build Identifier: M20090917-0800 The problem is caused by fixing https://bugs.eclipse.org/bugs/show_bug.cgi?id=108456 . It happens in Eclipse>=3.4 Reproducible: Always Steps to Reproduce: 1. import the attached project (test5036.zip) 2. try to rename the getName() method You will get the attached exception (exception.txt) For more details see https://jira.jboss.org/jira/browse/JBIDE-5036 The problem happens because Eclipse throws an exception when existing an invalid package name in the classpath. The attached patch (org.eclipse.jdt.ui.patch) ignores such package names.
Created attachment 151012 [details] patch
Created attachment 151013 [details] exception
Since the size of the test project is 2.7 MB and can't be attached, it is placed on https://anonsvn.jboss.org/repos/jbosstools/workspace/snjeza/test5036/test5036.zip
*** Bug 252920 has been marked as a duplicate of this bug. ***
*** Bug 293876 has been marked as a duplicate of this bug. ***
*** Bug 293975 has been marked as a duplicate of this bug. ***
Olivier: I'm CCing you as you responded to my previous bug on this issue. This bug here, however, is more accurate and has a patch. Thanks in advance.
Created attachment 151410 [details] bug.jar The problem is that the search engine reports invalid results. The attached bug.jar contains the following class in 1.0/b/B.class (and an analogous but valid a/A.class): package b; public class B { public String getName() { return null; } } Since "1.0" is not a valid Java package name, the Java model should not consider any files inside that folder as Java class or source files.
Comment on attachment 151012 [details] patch Of course we don't swallow exceptions without considering their cause.
Moving to Core to filter out the invalid packages during search.
(In reply to comment #8) > Created an attachment (id=151410) [details] > bug.jar > > The problem is that the search engine reports invalid results. The attached > bug.jar contains the following class in 1.0/b/B.class (and an analogous but > valid a/A.class): It depends. If the class is really b.B, then we should consider 1.0 as a "root" folder inside the package fragment root. So far I could not get the jar I need to check this.
"1.0" is not part of the package name. com.sun.codemodel.CodeWriter contains only com.sun.codemodel as the package name. So we should support the notion of "roots" inside package fragment roots.
Interestingly enough, the same jar contains entries inside the "1.0" folder as well as the root folder. So in this case it might be possible to simple "ignore" entries inside the "1.0" folder.
> So in this case it might be possible to simple "ignore" entries inside the > "1.0" folder. Yes, I don't think we should start guessing nested roots. If you do the same in a source folder (create src/1.0/a/A.java), the compiler also tries to compile the file (and the reconciler produces a slightly different problem than the builder). In source, the user can at least exclude the folder manually. But it would also be better if the folder was excluded automatically.
Satyam, Please investigate how to get rid of these "invalid" entries from the indexes.
Created attachment 152146 [details] Proposed patch The test in this patch requires bug293861.jar attached as another attachment bug.jar in this bug. It should be copied to \org.eclipse.jdt.core.tests.model\workspace\JavaSearchBugs\lib\bug293861.jar before trying out the test. This patch validates that the package name are valid java identifiers before adding that class into the index. It validates only the package names in jars and doesn't do for java files in source folders. There is one limitation with this patch. The function that validates the package names requires the project compliance. However the indexes for jars are reused across various projects in the workspace. The main difference of the identifiers across Java compliance is that Unicode4 characters are allowed for >=1.5. Hence, this patch passes 1.5 as argument to this validation function!
(In reply to comment #15) > Satyam, > > Please investigate how to get rid of these "invalid" entries from the indexes. (In reply to comment #16) > Created an attachment (id=152146) [details] > Proposed patch > I'm a little bit afraid of performance impact (again...) of this patch. I'm also not sure that removing these kind of entries would be the best solution, as we would slow down the indexing for some rare cases... Remember that the indexing is a high focus for performance when starting a session as search request can be done at the early startup by plugins of tools built on eclipe (e.g. RAD). BTW, could you first give numbers on indexing performance tests of FullSourceWorkspaceSearchTests running them several times with and without your patch?
Created attachment 153222 [details] Proposed patch Moved the check of the package name to happen during the search. The test added requires a jar which am attaching in the next attachment.
Created attachment 153223 [details] Jar file for the added test Please copy this jar file into \org.eclipse.jdt.core.tests.model\workspace\JavaSearchBugs\lib for the test to work.
Frédéric, please review. If ok, we might include this one for M4. If unsure or too risky, then it should be moved to M5.
(In reply to comment #20) > Frédéric, please review. If ok, we might include this one for M4. > If unsure or too risky, then it should be moved to M5. I think it should be moved to M5. I'm still not happy with the implementation. The proposed patch add an extra cost for each SearchDocument which may, for certain common search (e.g. bug 102279), be quite huge. IMO, to have the minimum performance impact, the filter should only be applied just before returning the match. SearchEngine found a match, but if the package name is invalid, then it would just not report it... Do not slow down good code (i.e. which do not have any invalid package name) just to avoid problem with bad code...
Created attachment 154621 [details] Proposed patch Doing the check just before reporting. Also doing the check for searchAllTypeNames. Content assist continues to report these invalid proposals. However, I think that should be fine. Please let me know if this is also not expected. Please use the jar attached in the next attachement for the test.
Created attachment 154622 [details] Jar file for the added test Please copy this jar into \org.eclipse.jdt.core.tests.model\workspace\JavaSearchBugs\lib
(In reply to comment #22) > Created an attachment (id=154621) [details] > Proposed patch > > Doing the check just before reporting. Also doing the check for > searchAllTypeNames. Content assist continues to report these invalid proposals. > However, I think that should be fine. Please let me know if this is also not > expected. > > Please use the jar attached in the next attachement for the test. As I think this kind of verification should have the minimum performance impact, I'd rather prefer not to apply the filter to the search all type names operations until someone demonstrate that it may be a real problem to report this kind of types... I'm aware that this may introduce some inconsistencies between search request and search all type names operations but this will happen only for some rare cases with *invalid* package names. Again, I prefer to have this inconsistency than having a less responsive search all type names operation... Otherwise, I'm fine with the fix for the search requests.
(In reply to comment #24) > As I think this kind of verification should have the minimum performance > impact, I'd rather prefer not to apply the filter to the search all type names > operations until someone demonstrate that it may be a real problem to report > this kind of types... I'm aware that this may introduce some inconsistencies > between search request and search all type names operations but this will > happen only for some rare cases with *invalid* package names. Again, I prefer > to have this inconsistency than having a less responsive search all type names > operation... I would like to get profiling numbers for all type name search with and without the filtering for invalid package names. Returning inconsistent results is evil and should be avoided. If I search for all type names, I can potentially try to open such a bogus type and get the same problem again. So once I can see the performance impact of filtering all type name search, we can decide what we want to do.
There is almost 5% increase in time - I should have run it before! Without the change, the testSearchAllTypeNameMatches() took an average of 225ms and with the change, it took an average of 235 ms. I actually missed the check in the other variant of searchAllTypeNames(). That should take more than 5% time. Considering the time taken, I think we should not do the checks in searchAllTypeNames().
Do we have numbers for the performance of the patch from comment 16? I would rather improve that approach, since indexing JARs is usually a one-time operation, but search is used much more often. The proposed patch could also be improved in a few places, e.g. - in AddJarFileToIndex#execute(..), skip the second check if the first loop didn't find any problematic entries, or - in Util#isPackageNameForClassValid(..), inlining the validation would save the creation of intermediate Strings and char[]s, repeated parsing of version strings, and synchronized invocations of JavaConventions#scannedIdentifier(..). Furthermore, I doubt we can get away with not fixing the searchAllTypeNames(..) case. Wouldn't that mean that Open Type still shows the bad types?
(In reply to comment #27) > Furthermore, I doubt we can get away with not fixing the searchAllTypeNames(..) > case. Wouldn't that mean that Open Type still shows the bad types? This is exactly what I am afraid of. Performance issues should not stop us from returning valid matches. The correctness of answers must come before performance consideration. I think we should check if we can improve the indexing part to completely get rid of those entries. For me returning invalid entries for the all type name query is a no-go.
(In reply to comment #27) (In reply to comment #28) I agree that we should first get performance numbers before taking a decision. So Satyam, could you get numbers of indexing tests (see comment 17), thanks I hope that with the enhancement proposed by Markus we can get good numbers, but IMO, if the numbers were bad, then it would be a no-go for me...
Indexing on only the apache project which has only jars in it -- I am seeing just a degradation of 2%. It takes 1.08 seconds without the change and 1.1 with the changes. There is no considerable change in Indexing of the fullsource. (In reply to comment #27) The first for loop is not always executed. It gets executed only when the index of the jar is already available and yes, in this case it can be optimized, which I will do. I will also try to optimize isPackageNameForClassValid() as much as possible.
Created attachment 154957 [details] Proposed patch Incorporated Markus's feedback stated in comment #27. Scanner Constructor is costly and hence moved isValidPackageNameForClass() into AddJarFileToIndex itself so that the same Scanner object can be used atleast for all classes of a jar. (Passing the scanner object doesn't look right or making it a class static could have forced the method to be made synchronized) The first loop gets executed very rarely and hence haven't modified that around. Performance numbers look as earlier. Less than 2% degradation for the apache project which consists of only jars. No noticeable performance degradation for other projects.
Created attachment 155298 [details] Proposed patch Modified the previous patch to have Frederic's cleaner way of validating the package name. There is no noticeable performance degradation with this patch.
Created attachment 155299 [details] Jar file for the added test Added a new test to make sure that package names having new identifiers in 1.5 like enum is returned by search. So, even updated the jar file and this attached jar file should be used for the test.
Created attachment 155300 [details] Proposed patch There was a problem with the previous attachment.. This should be good!
Comment on attachment 155300 [details] Proposed patch Patch looks good to me. However, the change in performance tests will be released separately as it may have consequence on test duration. Hence, it must be released before in the baseline stream...
(In reply to comment #35) > (From update of attachment 155300 [details]) > Patch looks good to me. > > However, the change in performance tests will be released separately as it may > have consequence on test duration. Hence, it must be released before in the > baseline stream... Released for 3.6M5 in HEAD stream (except the change of org.eclipse.jdt.core.tests.performance project...)
Verified for 3.6M5 using I20100125-0800
Any chance this could make it into Eclipse 3.5.2 ? Otherwise we would need to remove this Sun provided .jar from our default included jar on the classpath container for JBoss servers because of this JDT bug. Not to say others who might have jars with "exotic" naming :)
Could we put this in 3.5 maintenance? With the patch that was released in 3.6, there is no performance degradation on indexing.
This is too late for 3.5.2. We are already in RC3 and this is not a stop ship bug. We can however release it into the 3.5 maintenance branch post 3.5.2 and provide a patch if necessary.
Olivier, please add an entry to the readme.
(In reply to comment #40) > This is too late for 3.5.2. We are already in RC3 and this is not a stop ship > bug. > We can however release it into the 3.5 maintenance branch post 3.5.2 and > provide a patch if necessary. That would be great!
To be clear, my understanding was that the 3.5.x maintenance stream ends after 3.5.2. So the patch build would be suitable only for adopter products but not for adopter's plugin-additions to generic released builds. Is this correct? If you made a patch build it would not get a release version and would not be available on the general download site? Thanks in advance for the clarification.
(In reply to comment #43) > To be clear, my understanding was that the 3.5.x maintenance stream ends after > 3.5.2. So the patch build would be suitable only for adopter products but not > for adopter's plugin-additions to generic released builds. > > Is this correct? If you made a patch build it would not get a release version > and would not be available on the general download site? Yes, this won't be available on the general download site. It will be available as a patch on the JDT/Core web page. Now if you believe this is a show stopper, please explain.
Well, 1) it prevents *any* kind of renaming of methods if you have an otherwise completely ok .jar on the classpath. 2) its a regression - something that used to work before (<Eclipse 3.4) 3) it is not just this specific jar that has issue.
(In reply to comment #45) > Well, > > 1) it prevents *any* kind of renaming of methods if you have an otherwise > completely ok .jar on the classpath. > > 2) its a regression - something that used to work before (<Eclipse 3.4) > > 3) it is not just this specific jar that has issue. and 4) which only relate to us as adaptors, we will have to remove the jar from our classpath containers, which might affect users that relied on it being there in previous versions.
Just to be clear: if we weren't in the RC mode then we would have fixed this for 3.5.2 but now it is simply too late given that - the bug is not critical as we lived with it for more than 2 years now - the fix has only recently put into builds and hence not widely tested >we will have to remove the jar from >our classpath containers, Is the JAR used to compile other stuff or just to launch/run things? If so, you might be able to remove it from the build path and put it on the launch configuration classpath.
Created attachment 158473 [details] Proposal for 3.5.2 readme
Created attachment 158478 [details] New proposal for readme Add link to JDT/Core update page.
> >we will have to remove the jar from > >our classpath containers, > Is the JAR used to compile other stuff or just to launch/run things? If so, you > might be able to remove it from the build path and put it on the launch > configuration classpath. For this particular instance of a jar with folders that are normally ignored by anything else we will remove it from the build classpath and cross our fingers noone need it (its choosing between two evils, failure to compile vs. having rename refactoring fail)
Just to be clear, I understand and respect the hesitation to not put it in 3.5.2. In case we decide to distribute a patched version of JDT, is the proposed patch attached here what we should use ?
We will provide a patched (jar) version of jdt.core that will contain the fix on the update page of JDT/Core. See the readme. The only thing is that this patched version of jdt.core won't be signed.
Readme entry released into M20100210-0800.
Ok, any chance to get hold of that patched jar or the exact patch so we can built it our self to start testing on it ?
Satyam, please prepare a patch for 3.5 maintenance and attach it there. Thanks.
Created attachment 159164 [details] Patch on 3.5 maintenance branch This is a patch on top of 3.5 maintenance stream. This includes a test, which needs the same jar that is attached.
Dumb question: is that 1.0.com.etc. content useful in the JAR? I mean, could it be simply removed from the JAR without harm? We recently encountered this bug and our quick solution was to remove the "1" folder in the root of the JAR, thinking that it shouldn't be usable at all by client code. Are we wrong? Could there be any code that actually uses that JAR AND the contents of /1/0/ folder?
*** Bug 304511 has been marked as a duplicate of this bug. ***
>Could there be any code that actually uses that JAR >AND the contents of /1/0/ folder? Yes, they can load that code with a custom class loader.
(In reply to comment #59) > Yes, they can load that code with a custom class loader. Yes, of course. The actual question would be: are those contents in that "strange" folder layout because the provider actually meant to do that? In other words, is there a good reason for which jaxb-xjc authors included those contents there?
>In other words, is there a good reason for which jaxb-xjc authors included those >contents there? I can't speak for this particular case, you'd have to ask them. But I suspect they included to support some older versions (as an example imagine they have an old file or object format that they want to be able to read but didn't want to pollute the new code in order to read the old format).
This issue is still reproducible in the following ways: Test case 1: - import the project https://anonsvn.jboss.org/repos/jbosstools/workspace/snjeza/jbide5036/jbide5036b.zip - select the test.Test.getName method and call Refactor>Rename You will get an exception (see jbide5036-2.log) The project has the commons-lang.jar library that contains the "org.apache.commons.lang.enum" package that is invalid in JDK>=1.5. Attached is a patch against Eclipse 3.6.1 ((jbide5036-2.patch) Test case 2: - import the project https://anonsvn.jboss.org/repos/jbosstools/workspace/snjeza/jbide5036/jbide5036a.zip - select the test.Test.getName method and call Refactor>Rename You will get an exception (jbide5036-1.log) JDT can't resolve methods in the org.jboss.aop.annotation.AnnotationElement$AnnotationElementAction class (http://anonsvn.jboss.org/repos/jbossas/projects/aop/branches/Branch_2_1/aop/src/main/java/org/jboss/aop/annotation/AnnotationElement.java). It silently throws the following exception: "org.eclipse.jdt.internal.compiler.problem.AbortCompilation: Pb(538) Inconsistent classfile encountered: The undefined type parameter T is referenced from within AnnotationElement.AnnotationElementAction.AnnotationElementAction$2.AnnotationElementAction$2$18". The project contains the org.jboss.aop.annotation.AnnotationElement$AnnotationElementAction class in each of the three libraries included in the Java Build Path. Attached is a patch against Eclipse 3.6.1 (jbide5036-1.patch)
Created attachment 180736 [details] log1
Created attachment 180737 [details] log2
Created attachment 180738 [details] patch1
Created attachment 180739 [details] patch2
I was able to reproduce the problem and here are my comments for it. 1. Issue of "org.apache.commons.lang.enum" package was fixed in bug 317264 but I made an assumption that the jar file will be named org.apache.commons.lang_2*.jar and optimized the patch for it. In your case the jar file is named commons-lang.jar exhibiting the behavior. I will file a different bug for this and provide a fix there. 2. The second issue seems to be similar to bug 325418. Applying the patch given there seems to fix the issue. It is not yet in HEAD or 3.6.1 branch, but should get in soon.
Filed 327654 to take care of the apache_commons issue. As the second part is taken care in bug 325418, am resolving this.
> Test case 2: > - import the project > https://anonsvn.jboss.org/repos/jbosstools/workspace/snjeza/jbide5036/jbide5036a.zip > - select the test.Test.getName method and call Refactor>Rename > Note that this will result in an NPE (bug 250958).
