Community
Participate
Working Groups
Build ID: Version: 3.5.0, Build id: I20081030-1917 Steps To Reproduce: 1. Cut & Paste the following into package explorer: ------------------ 8< --------------- /** * A CU level javadoc comment. Is ignored by the javadoc tool i.e * any tags here neither get sanity checked nor get extracted and * become a part of the generated documentation. * * As such it is confusing and misleading if we offer completion * suggestions here. As of Version: 3.5.0, Build id: I20081030-1917 * we offer several completion suggestions as the user types "@", some * of them such "@param" and "@deprecated" don't make any sense here. * * We should either offer none at all, or at worst offer only what * is legal in package-info.java * * */ package xy; public class C { } ------------------ 8< --------------- 2. Goto the top level comment and on a fresh line type @ and you will see a completion proposals window open up with several choices. None of these choices really make sense as there are no defined tags for a javadoc comment at this level. Either we should offer no suggestions at all, or in the worst case only offer those tags that are valid at a package level (the package-info.java being a compilation unit so to speak) More information: See bug 252555 for some related discussion. Relevant code sits in : org.eclipse.jdt.core/codeassist/org/eclipse/jdt/internal/codeassist/complete/CompletionOnJavadocTag.filterPossibleTags
Looks like this one is related https://bugs.eclipse.org/bugs/show_bug.cgi?id=139621 which talks about javadoc comment without any type should still have completions offered. Couple of test cases added for this bug fix are failing with the fix. JavadocBugsCompletionModelTest.testBug139621h() JavadocBugsCompletionModelTest.testBug139621i() These test cases essentially look for completions in javadoc comments that are typed in without a class. I would assume that without a class declared, this javadoc comment would fall under the Compilation Unit scope. If that's the case, then we may not want to offer any completions here.
(In reply to comment #1) > Looks like this one is related > https://bugs.eclipse.org/bugs/show_bug.cgi?id=139621 > which talks about javadoc comment without any type should still have > completions offered. [...] > These test cases essentially look for completions in javadoc comments that are > typed in without a class. I would assume that without a class declared, this > javadoc comment would fall under the Compilation Unit scope. If that's the > case, then we may not want to offer any completions here. Wouldn't resurface bug #139621 ? I think it should be possible to fix the current bug without breaking the fix for bug #139621 by checking if we are dealing with the fake type.
Created attachment 134737 [details] Proposed patch The fix is to look for the FAKE_TYPE_NAME inside the CompletionOnJavadocTag#filterPossibleTags and allow the CLASS_TAGS and else if we know that the javadoc is unambiguously in compilation unit scope, return COMPILATION_UNIT_TAGS, which is nothing but empty.
Created attachment 134738 [details] Alternate Patch This is the alternate solution I am suggesting. The fix is done in the Parser#consumePackageComment to consider the javadoc only for package-info.java. With this approach the javadoc comments in Compilation Unit scope won't be processed and added to the AST, which I believe is okay. However, there are a couple of optional tests (inside testBug93880_15a, testBug93880_14a) which are failing in the ASTConverterJavadocTest. Since I am not sure whether we need those tests (in other words whether we need the compilation unit level javadoc comments for non package-info) I am putting forward this patch also.
(In reply to comment #3) > Created an attachment (id=134737) [details] > Proposed patch > The fix is to look for the FAKE_TYPE_NAME inside the > CompletionOnJavadocTag#filterPossibleTags and allow the CLASS_TAGS and else if > we know that the javadoc is unambiguously in compilation unit scope, return > COMPILATION_UNIT_TAGS, which is nothing but empty. In the code block: CompilationUnitDeclaration compilationUnit = scope.referenceCompilationUnit(); if (compilationUnit != null && (compilationUnit.isPackageInfo() || (compilationUnit.types.length > 0 && compilationUnit.types[0].name == CompletionParser.FAKE_TYPE_NAME))) { specifiedTags = CLASS_TAGS; } else { specifiedTags = COMPILATION_UNIT_TAGS; } - Is the check for isPackageInfo() necessary ? It would appear that would never be true since for package-info.java, we enter this method with scope being MethodScope and not CU scope ? - It would appear that the null check against compilationUnit is redundant. In most other uses of referenceCompilationUnit(), there doesn't seem to be a null check. - Likewise maybe even the check for compilationUnit.types.length > 0 is redundant since CompletionParser#consumeCompilationUnit ensures it would be otherwise ?
(In reply to comment #4) > Created an attachment (id=134738) [details] > Alternate Patch > This is the alternate solution I am suggesting. The fix is done in the > Parser#consumePackageComment to consider the javadoc only for > package-info.java. With this approach the javadoc comments in Compilation Unit > scope won't be processed and added to the AST, which I believe is okay. Not so sure about that, for one thing as you point out it will break the fix for bug# 93880. IMO, the previous patch uses a cleaner, side effect free approach.
I agree, the first two checks were never really required. As for the types.lenth check, I am just wondering if there is a possibility that we can get in to filterPossibleTags with CU scope without going inside consumeCompilationUnit method. If, for any exceptional reasons, it does happen, then this code will fail. About the latter patch, there is a note that the additional tests (that are failing with the patch) may not be required.
Fred, Can you please review this patch and let me know your comments/suggestions? Jay
(In reply to comment #8) > Fred, > Can you please review this patch and let me know your comments/suggestions? > > Jay > I guess you mean the 'Proposed patch'? If so, could you deprecate the other one? Thx
Fred, Sorry, I meant both the patches. The question about the alternate patch really is whether we can omit processing the javadoc at compilation unit level for non package-info.
(In reply to comment #10) > Fred, > > Sorry, I meant both the patches. The question about the alternate patch really > is whether we can omit processing the javadoc at compilation unit level for non > package-info. > I agree with Srikanth (comment 6). The javadoc is necessary while building DOM/AST nodes, hence its parse cannot be skipped...
The proposed patch looks good to me. I agree with Srikanth that the isPackageInfo() condition is not necessary in this case. Otherwise, I would prefer to keep the other conditions as even if they are automatically ensured by the current parsing, they would prevent future failures in case this path was changed for any reason and this dependency were not captured by our regression tests... For the orphan comment another test with a package could be also interesting to have: package javadoc.bugs; /** * @ */
Created attachment 135328 [details] Latest patch Resubmitting the patch with changed suggested by Srikanth and Fred. Fred, the scenario you described has already been covered by the test case JavadocBugsCompletionModelTest#testBug139621h
Comment on attachment 135328 [details] Latest patch Patch looks good
Released for 3.5RC1 in HEAD stream.
David, as you correctly warned me, this bug is missing another commiter review for RC1... Could you please review it? Thanks
Patch looks good
Verified for 3.5RC1 using I20090513-2000
