Community
Participate
Working Groups
There's no warning for missing descriptions for @param, @exception, etc. . I would expect to get warnings everywhere or nowhere, but not only for some tags in certain positions. Currently, missing description are only signaled for @return tag (see bug 68017 and bug 73348)
As this new functionality will be time consuming for compiler, we would surely add it an option ("Javadoc tags has to have a description" for example) and disable it by default... Note that nothing in Javadoc spec tells us that these descriptions are mandatory or should be written. Note also that quick-fix does not put any description while inserting javadoc tags. If we consider that descriptions are necessary with this new option, then it should be surely changed...
I agree that quick fix and refactorings should put descriptions (TODOs) when missing descriptions are flagged as errors. Looking at http://java.sun.com/j2se/1.4.2/docs/tooldocs/windows/javadoc.html , I don't see a real difference between missing descriptions after @return and @throws tags: @return description Adds a "Returns" section with the description text. This text should describe the return type and permissible range of values. [..] @throws class-name description [..] Adds a "Throws" subheading to the generated documentation, with the class-name and description text. [..] The additional sentence for @return is IMO just a restriction on the contents of the 'description', but does not make the 'description' more or less mandatory than for the @throws tag.
Defer post 3.1
*** Bug 108798 has been marked as a duplicate of this bug. ***
Dirk, Do you want this bug reopened for 3.2?
My feeling is that this isn't much work to do so I would like to see it reopened for 3.2. It would give us a more consistent story for Javadoc checking. There will be then no difference between a user typing in Javadoc and misses the description and us currently generating TODO tags (in some scenarios).
Reopen for 3.2
Thanks.
Unfortunately, has to defer it again. Set target to 3.3 to work on this earlier for next version...
*** Bug 170759 has been marked as a duplicate of this bug. ***
IMO the whole point of these warnings would be to force people to actually document stuff. And thus to warn about automatically generated javadoc. Having the QuickFix for missing javadoc tags inserting dummy documentation would be directly harmful; it would get us back to where we already are. Unless the QuickFix could actually come up with helpful comments of course (which I kind of doubt...). Please implement these warnings. Please don't make the QuickFix for missing Javadoc tags come up with dummy comments. That should be its own bug if somebody wants that.
(In reply to comment #11) > IMO the whole point of these warnings would be to force people to actually > document stuff. And thus to warn about automatically generated javadoc. > > Having the QuickFix for missing javadoc tags inserting dummy documentation > would be directly harmful; it would get us back to where we already are. > Unless the QuickFix could actually come up with helpful comments of course > (which I kind of doubt...). > The quick-fix is not supposed to add a complete and finalized javadoc comment. It just helps user to save a little time by writing all information that JDT already knows... Descriptions obviously can only be written by user himself and there's nothing else the quick-fix can do here. > Please implement these warnings. > We'll try, but as there are many other things to do, we cannot be sure it will be addressed in 3.3. Javadoc requirements have generally not a high priority, especially when the Javadoc tool does not report any warnings on similar comments. So, only if time permit... > Please don't make the QuickFix for missing Javadoc tags come up with dummy > comments. That should be its own bug if somebody wants that. > I do not agree, as I said above, this quick fix is helpful to save time while writing javadoc comments, especially on methods which have lot of parameters and/or thrown exceptions.
(In reply to comment #12) To clarify what I think would be a good vs bad Quick Fix behavior for a method without Javadoc: public int method(double d) { return 42; } Good (what 3.2.1 does currently), a comment with empty tags that can be warned about: /** * @param d * @return */ Bad, a comment with random text inserted that is hard to warn about and makes generated HTML documentation unreadable: /** * @param d TODO: Document this * @return TODO: Document this */
*** Bug 175992 has been marked as a duplicate of this bug. ***
We'll see if this problem can be addressed for 3.3...
*** Bug 176701 has been marked as a duplicate of this bug. ***
Created attachment 77449 [details] patch + test case Also contains jdt.ui new preference to trigger missing description validation Frederic, Jerome: please review proposed patch Note that empty tag validation should be addressed by bug 201599
(In reply to comment #17) > Created an attachment (id=77449) [details] > patch + test case > > Also contains jdt.ui new preference to trigger missing description validation > Frederic, Jerome: please review proposed patch > Note that empty tag validation should be addressed by bug 201599 > This patch considers return missing description different than other description. Are we sure we want this behavior. Playing with it, it sounds strange to me that when the preference 'Consider tag description' is not set I still have a warning on an empty @return tag. Except this, I'm OK with the patch content
(In reply to comment #17) > Note that empty tag validation should be addressed by bug 201599 > I guess you meant "empty javadoc comments validation"...
(In reply to comment #18) I did not want to change current behavior which consists in validating the @return tag.
(In reply to comment #20) As per the proposed correction, a new preference will be offered to the user, in order to enable / disable the validation of tags description. Currently, we already validate the presence of a return type after the @return tag. If no return type found, corresponding "@return missing return type" warning will be raised. So the question is: what should we do with the @return tag validation? (1) include it as part of the new proposed missing description preference, (2) we keep current behavior and do not include the @return in this new preference. (1) will result in a change of behavior concerning the @return tag validation (2) will result in having situations where we could have a "@return missing return type" warning though having the missing tag description option not activated. My preference goes to (2).
If this is a time to vote for preferred behavior, my vote goes to 1. 2 will be very confusing for all newcomers, and somewhat confusing for everybody. It would give us two preferences: a: "Warn about missing @return tag descriptions" b: "Warn about missing tag descriptions, except for the @return tag which is covered by another option" IMO it's better to fix option (a) which has been broken since forever, than to keep dragging it along in its current state. Eric, cred for coming up with a patch for this!
(In reply to comment #22) Jerome, Frederic: your opinion on this one?
I also vote for 1)
My vote was always for 1).
The problem I see with 1) is that a) either the option is disabled, and we don't flag the missing description on @return, although the Javadoc tool will always flag this missing description b) or the option is enabled, and we flag the missing description on @param, although the Javadoc tool will never flag the missing description So with 1), we have no way to be 100% compatible with the Javadoc tool. Just for the record, I agree that 2) is more consistent and less confusing for the user. So 1) do we want to be 100% compatible with the Javadoc tool ? XOR 2) do we want to be more consistent and less confusing for the user ?
I don't think we need to be compatible with the Javadoc tool, especially since neither the tools functionality nor Javadoc syntax itself is 100% clearly specified. We should try to do the best for the user (as long as we pass the JCK of course ;-).
Used the following test case: public class X { /** * @return * @param aParam * @throws NullPointerException */ public String foo(String aParam) { String default_String = ""; return default_String; } } Using Eclipse SDK Version: 3.3.0 Build id: I20070821-0800 Observed behavior summary 1.4.2 Level Eclipse - compliance 1.4: 1 warning (Javadoc: Missing return type description) Sun JDK 1.4.2: 1 warning - @return tag has no arguments. IBM JDK 1.4.2: 1 warning - @return tag has no arguments. 5.0 Level Eclipse - compliance 5.0: 1 warning (Javadoc: Missing return type description) Sun JDK 1.5: 1 warning - @return tag has no arguments. IBM JDK 1.5: 1 warning - @return tag has no arguments. JRockit 1.5: 1 warning - @return tag has no arguments.
(In reply to comment #27) > I don't think we need to be compatible with the Javadoc tool, especially since > neither the tools functionality nor Javadoc syntax itself is 100% clearly > specified. We should try to do the best for the user (as long as we pass the > JCK of course ;-). > I do not fully agree on that... We need to report at least the same warnings that Javadoc tool reports (ie. we must mimic the tool behavior as specification is not clear enough). We can add other warnings to make our compiler more helpful for users but they need to be conditioned by preferences and disabled by default. So, with this implicit rule, the only solution I can think about is that 'Consider tag description' should have three possible values: NO_TAG, RETURN_TAG, ALL_TAGS. Default would be RETURN_TAG for behavior backward compatibility.
(In reply to comment #29) > So, with this implicit rule, the only solution I can think about is that > 'Consider tag description' should have three possible values: NO_TAG, > RETURN_TAG, ALL_TAGS. > Default would be RETURN_TAG for behavior backward compatibility. > I like this proposal. If a UI can be developed for these 3 states (Dani, Martin ?), I think we should go for it.
I'm not sure if users would really choose the TAG_RETURN option. Why would they keep uncommented tags? Why would you select TAG_NONE? This gives you an error when creating Javadoc. You can always remove tags if you don't want to document them. What about this: Missing @return is always marked as invalid tag: 'Invalid tag: @return tag must always be documented' All other tags are only warned when the user enabled 'Warn about undocumented tags'
Created attachment 77974 [details] [proposed patch + test case] on top v810 - all jdt.core tests OK (also contains patch for jdt.ui new pref) As discussed and agreed on with Martin: - with the new option disabled, the behavior remains unchanged. Already validated tags (@throws, @exception, @see, @return) will remain flagged (if reporting errors in tags). Example: Javadoc: Missing return type description for a malformed @return tag, or Javadoc: Missing reference for a malformed @see tag - with the new option enabled (Report missing description), all the tags that do not already benefit a validation (as listed above) will be flagged with a new warning message (Javadoc: Description expected after this token)
(In reply to comment #32) Olivier, Jerome and Frederic: please review proposed patch - thanks
(In reply to comment #31) > I'm not sure if users would really choose the TAG_RETURN option. Why would they > keep uncommented tags? This would be to match the current behavior of the Javadoc tool. > Why would you select TAG_NONE? This gives you an error > when creating Javadoc. You can always remove tags if you don't want to document > them. As with other options, we want to give the freedom to the user to do whatever s/he feels is best for her/him. > What about this: > > Missing @return is always marked as invalid tag: 'Invalid tag: @return tag must > always be documented' > All other tags are only warned when the user enabled 'Warn about undocumented > tags' > 3 commenters (see comment 22, comment 24 and comment 26) agreed that this option would be confusing for the user. So why are we considering this option ?
I must step back: we should try to be close to the Javadoc tool (as we recently did with other bugs) as Frédéric points out. We could surface the difference between the @return tag description and the others by using a more severe message for the @return tag (like Martin suggested). I'm against making the UI more complex than needed.
Created attachment 78145 [details] [proposed patch + test case] on top v811 - all jdt.core tests OK as per suggestions and last comment #34, new fix proposal: the validation of the missing tag description is now handled with 3 values: - flag @return tags with the specific missing return type warning (default, and current behavior) - no tag: no validation (@return tags are not validated anymore) - all tags: all missing tag descriptions will be flagged Note that the patch for jdt.ui pref page, though part of the patch, is not optimized, due to time constraint. Proposal: a checkbox (label = Report missing description) and combo with 2 values (All tags, Ignore) With the checkbox disabled, the option would be defaulted to validate @return tags only. With the checkbox enabled, user could select between All tags / Ignore values.
(In reply to comment #36) > Proposal: a checkbox (label = Report missing description) and combo with 2 > values (All tags, Ignore) > With the checkbox disabled, the option would be defaulted to validate @return > tags only. So if I uncheck "Report missing description", I'll get warnings about some missing descriptions? Sounds counter-intuitive to me. If we have a three states, we should have a three state combo box (and no checkbox): Report missing descriptions: {None, @return tags only, All}
As already noted in comment 35 I also think the last suggestion is too complicated to use. Also note that there is still clarification needed on 'Report errors in tags' (see bug 202493).
I also think comment 34 is an over-design. At some point the user will have to understand that the Javadoc tool has the strange behavior of only complaining about empty @return, but tolerating other empty tags. If the error/warning messsage on an empty @return tag is mentioning that ('Invalid tag: @return tag must always be documented'), it should become clear. - keep existing option: 'Report errors in tags' - one additional setting: check box 'Report undocumented tags' empty @return will get a warning/error when either the first or the second is checked. Johan, do still think this would be confusing?
(In reply to comment #39) > - keep existing option: 'Report errors in tags' > - one additional setting: check box 'Report undocumented tags' > > empty @return will get a warning/error when either the first or the second is > checked. > > Johan, do still think this would be confusing? Checking "report errors in tags" and just getting warnings for @return tags would confuse me. I still prefer my suggestion from comment 37: " we should have a three state combo box (and no checkbox): Report missing descriptions: {None, @return tags only, All} "
(In reply to comment #40) > Checking "report errors in tags" and just getting warnings for @return tags > would confuse me. > > I still prefer my suggestion from comment 37: > " > we should have a three state combo box (and no checkbox): > Report missing descriptions: {None, @return tags only, All} > " > I totally agree with Johan.
(In reply to comments #40 and #41) last patch (only the jdt.core part) is still valid and needs to be reviewed. In this patch, the new missing tag description has 3 values: - return tag (default): will flag missing return types in @return tags - no tag: validation disabled (and no missing return types in @return tags) - all tags: missing tag description enabled for all tags Implementation details of the new preference will be handled by the UI Team.
Created attachment 78851 [details] [proposed patch + test case] on top v813 - all jdt.core tests OK new (improved?) patch: missing tag description is now a new option, independent the fact that the existing "Report errors in tags" option is enabled or not. This new missing tag description can have 3 values: - return_tag (default) --> @return tag with missing description will be flagged - no_tag --> option disabled (no validation) - all_tags --> all tags with missing description (including @return tags) will be flagged Also note that the message for the missing return type was modified: Missing return type description becomes: Description expected after return tag in order to conform with missing description message: Description expected after this token
(In reply to comment #43) Basing this on your description, I don't feel comfy enough with the source code to read the actual patch. > new (improved?) patch: Sounds like a real improvement to me! > missing tag description is now a new option, independent the fact that the > existing "Report errors in tags" option is enabled or not. I thought the point of this bug was to fix the broken "Report errors in tags" option. Is the broken option still there? If so, keeping it around sounds confusing to me. > Also note that the message for the missing return type was modified: > Missing return type description > becomes: > Description expected after return tag > in order to conform with missing description message: > Description expected after this token Why a separate message for @return tags but not for any other tags? Either having specific messages for all tags or just "Description expected after this token" for all cases would seem more coherent. Eric, if you just remove the old broken option, I would *love* to see this one go in. Great work, really appreciate your effort! Cheers :-) //Johan
(In reply to comment #44) > I thought the point of this bug was to fix the broken "Report errors in tags" > option. Is the broken option still there? If so, keeping it around sounds > confusing to me. With this fix implemented, the "Report errors in tags" option is completely separate from the missing description option. The "Report errors in tags" option will remain present, until possibly deprecated, and replaced with more accurate options. > Why a separate message for @return tags but not for any other tags? Either > having specific messages for all tags or just "Description expected after this > token" for all cases would seem more coherent. We kept the specific message number for the @return tag in order to preserve client relying on it, and made the message text similar to the more generic new one. I agree we could have taken the same "Description expected after this token". But even with 2 separate messages: "Description expected after this token" "Description expected after return tag" I assume one will be able to understand the option that correspond to these warnings/errors.
(In reply to comment #45) > (In reply to comment #44) > > I thought the point of this bug was to fix the broken "Report errors in tags" > > option. Is the broken option still there? If so, keeping it around sounds > > confusing to me. > With this fix implemented, the "Report errors in tags" option is completely > separate from the missing description option. > The "Report errors in tags" option will remain present, until possibly > deprecated, and replaced with more accurate options. For example, with "Report errors in tags" enabled, you will get a "Missing reference" error if you forget to the type/method after @see.
(In reply to comment #46) > For example, with "Report errors in tags" enabled, you will get a "Missing > reference" error if you forget to the type/method after @see. Thanks Jerome, that answers my question. I'm happy with everything. I'll leave the review of the actual code to others, but AFAIC having this patch submitted in its current state would be peachy :-).
>With this fix implemented, the "Report errors in tags" option is completely >separate from the missing description option. Mmh, but according to Frederic's comment in bug 202493 the "Report errors in tags" is meant to be sort of a master flag. I suggest you first clarify that.
(In reply to comment #48) > >With this fix implemented, the "Report errors in tags" option is completely > >separate from the missing description option. > Mmh, but according to Frederic's comment in bug 202493 the "Report errors in > tags" is meant to be sort of a master flag. I suggest you first clarify that. > Please see bug 202493 comment 3 for clarification. (We suggest to rename "Report errors in tags" so that it doesn't look like a master flag for the new "Report missing description" option)
(In reply to comment #43) Patch looks good. Just 3 small suggestions: - the error message should be: Description expected after @return tag" - the Javaddoc for JavaCore#COMPILER_PB_MISSING_JAVADOC_TAG_DESCRIPTION should say: "Possible configurable option ID" - the new constants should be grouped with the other JavaCore#COMPILER_* constants
Created attachment 79053 [details] [proposed patch + test case] on top v814 - all jdt.core tests OK As per Jerome suggestions in comment #43
(In reply to comment #51) Patch looks good to me.
(In reply to comment #51) > Created an attachment (id=79053) [details] > [proposed patch + test case] on top v814 - all jdt.core tests OK > > As per Jerome suggestions in comment #43 > I think that following points need to be changed: 1) I do not like that compiler AST node classes are used in AbstractCommentParser. This parser is intended to implement common code between the javadoc parsers used by compiler (JavadocParser) and DOM/AST Converter (DocCommentParser). If you need to use specific AST nodes, then you should override the method in the corresponding parser. In this case, you only need to override the method in JavadocParser as you problems are not reported in DocCommentParser as sourceParser is null... 2) Javadoc comment of computeSeverity method has been deleted, it should be restored 3) BatchCompilerTests#test13() Changes in this test look suspicious: a) File.separator has been wrongly removed => it will fail on Unix boxes b) I do not understand why the optionKey of the problem has changed?! 4) JavadocBugTests IMO, getCompilerOptions() should not set the value of CompilerOptions.OPTION_ReportMissingJavadocTagDescription to CompilerOptions.RETURN_TAG if reportMissingJavadocDescription is null. It could be interesting to keep the default value set by CompilerOptions and see if it matches the expected behavior. Sorry, I missed points 3)b) and 4) in my first review (comment 18)... :-(
Created attachment 79266 [details] [proposed patch + test case] on top v815 - all jdt.core tests OK Yet another variation of the fix. Missing tag description validation not changed, but warning / error message changed as follows: - with new option enabled, for all tags missing a description (including the @return tag), the message will be: Description expected after @{tag_name} - for the @throws and @param tags, the message will be: Description expected after this reference Also fixed the issues as per Frederic comment #53. May reviewers please review. Thanks.
Created attachment 79362 [details] [proposed patch + test case] on top v815 - all jdt.core tests OK final version
(In reply to comment #55) > Created an attachment (id=79362) [details] > [proposed patch + test case] on top v815 - all jdt.core tests OK > > final version > Patch looks good: +1 Just be careful while releasing as you'll get a conflict with bug 168849 patch (same IProblem ID). You will also need to synchronize with JDT/UI for the changes in the preferences page...
(In reply to comment #55) Patch still looks good to me. Just make sure to synchronize with JDT/UI before releasing it.
Released for 3.4M3 in HEAD stream
Verified for 3.4M3 using I20071029-0010 build.
