Community
Participate
Working Groups
It would be nice if it was possible to navigate through the inherited method through the {@inheritDoc} tag when present in a javadoc comment. For example in following test case public class Y extends X { X.java: public class X { /** * Main description of the method foo... * * @param obj The given object * @return an empty string * @see Object the root class * @see String the string class */ public String foo(Object obj) { return ""; } } Y.java: public class Y extends X { /** * Specific description of the overriding method... *<br> * {@inheritDoc} * @return <code>null</code> */ public String foo(Object obj) { return null; } } I expect to jump to X.foo javadoc comment when hitting F3 on the {@inheritDoc] inline tag...
Not sure if there was any changes need in JDT/UI for this or only return the correct selected element would be enough?
Should also check hyperlinking (Ctrl+click) in the Java editor when codeSelect works.
*** Bug 118648 has been marked as a duplicate of this bug. ***
I'd like to see this fixed for 3.7.
I'll follow up.
(In reply to comment #5) > I'll follow up. This is a bit more complicated than I thought. The SelectionJavadocParser currently relies on ASTNode for a type,method, field, etc when it needs to be parse the reference after a tag such as @see, or @param. Code select currently does not have any infrastructure to work with inline tags without any accompanying ASTNode. Even the method for which the javadoc contains the @inheritDoc and the type defining the method are not available to the SelectionJavadocParser when parseTag(int) is called.
Created attachment 189151 [details] proposed fix v1.0 This patch makes the SelectionJavadocParser record that selection has taken place on an @inheritDoc inline tag, so that the SelectionEngine can process the node that we need to navigate to. Using this, F3 works fine. The SelectionRequestor can only accept an IJavaElement. Since javadoc is not really an IJavaElement, our best best is to add the corresponding method declaration to the SelectionRequestor's list. So pressing F3 on inheritDoc navigates to the overriden method. I feel this is ok because the inherited doc is anyways just going to be right on top of the overriden method declaration. The problem, though, is when one presses CTRL + Click on the @inheritDoc tag, the UI processes it in the same fashion as it would if one does a CTRL+click on a method. Markus, Can anything be done to alter this behaviour for @inheritDoc tags?
(In reply to comment #7) > Markus, Can anything be done to alter this behaviour for @inheritDoc > tags? We can take a look into Ctrl+Click once it's working for F3 (the detector needs to be adjusted).
(In reply to comment #7) > So pressing F3 on inheritDoc > navigates to the overriden method. I feel this is ok because the inherited doc > is anyways just going to be right on top of the overriden method declaration. Yeah, I guess that's the best we can do. I didn't try the patch, but from looking at the code, it seems you just find any overridden method in the hierarchy. Unfortunately, the real {@inheritDoc} algorithm is more complex, since it can also inherit the doc from further up the hierarchy. This is comparable to "super.method()", which can also jump multiple super classes until it finds the right one. Note that not all {@inheritDoc} tags in the same Javadoc refer to the same super spec. See the implementation we use in the Javadoc hovers in org.eclipse.jdt.internal.ui.text.javadoc.JavadocContentAccess2.InheritDocVisitor and the links to the spec there.
(In reply to comment #9) > See the implementation we use in the Javadoc hovers in > org.eclipse.jdt.internal.ui.text.javadoc.JavadocContentAccess2.InheritDocVisitor > and the links to the spec there. Ok so it seems like I just implemented the InheritDocVisitor.visit() method (looking at its implementation in org.eclipse.jdt.internal.ui.text.javadoc.JavadocContentAccess2.findAttachedDocInHierarchy(IMethod), it used MethodOverrideTester to find the overriden method, which seems to do what my findOverriddenMethodInHierarchy(..) does.).I still need to deal with the remaining pieces. And, as i understand, that would be for cases where the javadoc is not on the immediate super method , but still some levels above, right?
JavadocContentAccess2#findAttachedDocInHierarchy(..) is just our fallback when we don't have attached source and still want to find at least some Javadoc for binary elements. That's not what we should do here. InheritDocVisitor has indeed been written to help with cases where inherited doc snippets (from main doc, @param, @return, or @throws/@exception) need to be found anywhere in the super hierarchy.
The problem is how to figure out whether a particular method, which has been determined to be overriden following the InheritDocVisitor algorithm, actually contains a javadoc or not. Javadoc can be obtained via java elements, but in this case i only have the compiler bindings, from which i can't obtain a java element. I was also thinking, wouldn't it be easier to implement in the UI itself, if jdt/core can add the current method into the requestor and set a flag signalling that the selection took place on the inheritDoc tag?
> I was also thinking, wouldn't it be easier to implement in the UI itself, if > jdt/core can add the current method into the requestor and set a flag > signalling that the selection took place on the inheritDoc tag? There's no requestor at the API level. We only have ICodeAssist#codeSelect(..) that returns an IJavaElement[]. I agree that the resolution of inheritDoc tags is not trivial, but that would rather speak for offering an API for this in jdt.core than implementing everything in the UI.
And in the UI, we have exactly the same problem that we don't have ASTs of the super types at hand, and that we have to create these on-the-fly if necessary to find the inherited doc.
(In reply to comment #13) > I agree that the resolution of inheritDoc tags is not trivial, but that would > rather speak for offering an API for this in jdt.core than implementing > everything in the UI. Actually, the whole issue is just extracting the javadoc out of a method to make sure the method code select is returning is the one where javadoc is being inherited from. And i'm having a tough time trying to figure out how to achieve that in the SelectionEngine. My first reaction was that since the UI hovers already fetch the correct javadoc when I move the cursor over a method with inheritDoc tag, and also since JavadocContentAccess and JavadocContentAccess2 has all functionality in place to do so, it should be a trivial task for the UI. By API, do you mean we should move this down to jdt.core? Even the perProjectInfo doesn't seem to have the javadoc in this case, so I dont understand how to fetch it.
Core does not need to resolve the target of {@inheritDoc} for other reasons and UI already has most of the necessary infrastructure to resolve the tag in Javadoc hovers, so I think it's too much work adding this to jdt.core now. Moving this to UI for a fix similar to bug 269112.
(In reply to comment #16) > Core does not need to resolve the target of {@inheritDoc} for other reasons and > UI already has most of the necessary infrastructure to resolve the tag in > Javadoc hovers, so I think it's too much work adding this to jdt.core now. > > Moving this to UI for a fix similar to bug 269112. I have already made the necessary changes in core to fix this. I was just in the process of adding regression tests. Are you sure you want to move this to UI?
Created attachment 189479 [details] proposed fix v2.0 + tests This patch follows the correct algo for finding which method to navigate to, using the InheritDocVisitor of JavadocContentAccess. The visit() method had to be adjusted to make sure that the current method being visited actually has a javadoc. The fix works for all cases - when all classes are in the source, when some classes in the hierarchy are binaries in external JARs with either source or javadoc attached, and even when all the classes are binaries in an external JAR with source attached.
(In reply to comment #18) > Created attachment 189479 [details] [diff] > proposed fix v2.0 + tests OK, that looks smaller than I though it would become, and I also found no issue when testing it. To avoid the compiler warning, you may want to make the InheritDocVisitor package-visible. OK to go if Olivier agrees.
(In reply to comment #19) > (In reply to comment #18) > > Created attachment 189479 [details] [diff] [details] [diff] > > proposed fix v2.0 + tests > > OK, that looks smaller than I though it would become, and I also found no issue > when testing it. To avoid the compiler warning, you may want to make the > InheritDocVisitor package-visible. Ok, i'll clean up the patch.
+1
(In reply to comment #21) > +1 Olivier, the patch contains a new method SelectionRequestor#findMethod(IType type, char[] selector, String[] paramTypeNames). The same was introduced in InternalCompletionProposal by Jay, to make sure we obtain the correct handle to the method and not just a dummy, so that the java model cache can be looked up to get the info about a method. Since it's already being used at 2 places now, and may again be used somewhere in the future, I suggest it to be made an API in IType, alongside getMethod(..) and findMethod(..) functions. What do you think?
(In reply to comment #22) The findMethod(..) implementations look wrong regarding their handling of constructors. A method can actually have a constructor name, but you can't distinguish these cases. You should add a boolean isConstructor parameter. Then the method would be similar to our JavaModelUtil#findMethod(String, String[], boolean, IType). The name 'paramTypeNames' is a bit odd, since this variable holds signatures, not names. Before you can make this API, you need to rethink the general utility of the method. In an API, you can't just stop and return null if there are multiple matching methods. Note that IType#findMethods(IMethod) specs that it only compares simple names of the argument types (the implementation actually compares simple names of the erasure). This leaves it up to the client to do more thorough checks on the returned methods. I don't see enough use cases where the proposed method would actually be correct, so I wouldn't add this as API.
I concur with Markus that we should only add an API if absolutely necessary. So if we can solve this case without adding new API, this would be better. My +1 was for resolving it at the core level.
(In reply to comment #23) > (In reply to comment #22) > You should add a boolean isConstructor parameter. Good point! > Before you can make this API, you need to rethink the general utility of the > method. In an API, you can't just stop and return null if there are multiple > matching methods. Note that IType#findMethods(IMethod) specs that it only > compares simple names of the argument types (the implementation actually > compares simple names of the erasure). This leaves it up to the client to do > more thorough checks on the returned methods. > > I don't see enough use cases where the proposed method would actually be > correct, so I wouldn't add this as API. Yeah i agree. What i was actually looking forward was to avoid duplicating this method. So maybe we can add this to org.eclipse.jdt.internal.compiler.util.Util class.
Created attachment 189586 [details] proposed fix v2.1 + tests This is the cleaned up patch. Here's the explanation: This patch fixes the problem in 2 parts: 1) Make sure SelectionParser parses the inheritDoc tag properly and sets a flag to denote that selection took place on inheritDoc tag. This needs to be done because the tag itself is not a valid AST node, and hence the SelectionNode can't really be created corresponding to this tag. So, the patch tries to build the SelectionNode corresponding to the method on which the inheritDoc tag is found and the boolean flag differentiates this case from the case when selection takes place on the method declaration itself. Changes in SelectionJavadocParser and JavadocParser take care of this part. org.eclipse.jdt.internal.codeassist.select.SelectionJavadoc.internalResolve(Scope) recognizes the flag and creates the SelectionNodeFound. 2) Make sure the SelectionEngine can find the desired method to navigate to, and adds it to the SelectionRequestor. This is done using the InheritDocVisitor class, taken from jdt.ui, which follows the spec on finding the correct method. Since in SelectionEngine, we only have the methodBinding, and an IMethod element is needed to find the javadoc (which we need to find to make sure the method we found actually has some javadoc that is being inherited). The implementation of InheritDocVisitor#visit() takes care of this. All tests pass.
Olivier, can you please review? Thanks!
Looks good.
Released in HEAD for 3.7M6
Raised bug 338373 to take care of comment 8 above.
Verified for 3.7M6.
