Skip to content

Commit

Permalink
Improve Javadoc generated by "Make Static" refactoring #1043 (#1084)
Browse files Browse the repository at this point in the history
This change improves the Javadoc generation behavior of the "Make
Static" refactoring and aligns it with other refactorings such as
"Change Method Signature". It ensures that generated parameter tags are
inserted at proper positions within an existing Javadoc comment. The tag
for the parameter itself is added as the first parameter tag, following
any author or version tags. The tags for potential type parameters are
added as the last parameter tags, preceding any other tags than author
or version.

The reused functionality from
ChangeMethodSignatureRefactoringContribution is factored out to the
JavadocUtil class.

Fixes #1043
  • Loading branch information
HeikoKlare committed Jan 18, 2024
1 parent 5e0f474 commit 36a9f78
Show file tree
Hide file tree
Showing 8 changed files with 182 additions and 47 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@
import org.eclipse.jdt.internal.corext.refactoring.RefactoringCoreMessages;
import org.eclipse.jdt.internal.corext.refactoring.base.ReferencesInBinaryContext;
import org.eclipse.jdt.internal.corext.refactoring.code.TargetProvider;
import org.eclipse.jdt.internal.corext.refactoring.util.JavadocUtil;
import org.eclipse.jdt.internal.corext.refactoring.util.TextEditBasedChangeManager;
import org.eclipse.jdt.internal.corext.util.Messages;

Expand Down Expand Up @@ -261,17 +262,22 @@ private void addInstanceAsParameterIfUsed() throws JavaModelException {
ListRewrite lrw= fTargetMethodDeclarationASTRewrite.getListRewrite(fTargetMethodDeclaration, MethodDeclaration.PARAMETERS_PROPERTY);
lrw.insertFirst(generateNewParameter(), null);
//Changes to fTargetMethodDeclaration's signature need to be adjusted in JavaDocs too
updateJavaDocs();
addParameterToJavaDoc();
}

private void updateJavaDocs() {
private void addParameterToJavaDoc() {
Javadoc javadoc= fTargetMethodDeclaration.getJavadoc();
if (javadoc != null) {
TagElement newParameterTag= fTargetMethodDeclarationAST.newTagElement();
newParameterTag.setTagName(TagElement.TAG_PARAM);
newParameterTag.fragments().add(fTargetMethodDeclarationAST.newSimpleName(fParameterName));
ListRewrite tagsRewrite= fTargetMethodDeclarationASTRewrite.getListRewrite(javadoc, Javadoc.TAGS_PROPERTY);
tagsRewrite.insertFirst(newParameterTag, null);
TagElement previousTag= JavadocUtil.findTagElementToInsertAfter(javadoc.tags(), TagElement.TAG_PARAM);
if (previousTag == null) {
tagsRewrite.insertFirst(newParameterTag, null);
} else {
tagsRewrite.insertAfter(newParameterTag, previousTag, null);
}
}
}

Expand Down Expand Up @@ -311,7 +317,6 @@ private void updateTargetMethodTypeParamList() throws JavaModelException {
IType parentType= fTargetMethod.getDeclaringType();
ITypeParameter[] classTypeParameters= parentType.getTypeParameters();
ListRewrite typeParamsRewrite= fTargetMethodDeclarationASTRewrite.getListRewrite(fTargetMethodDeclaration, MethodDeclaration.TYPE_PARAMETERS_PROPERTY);
Javadoc javadoc= fTargetMethodDeclaration.getJavadoc();
List<String> methodParameterTypes= getMethodParameterTypes();
List<String> methodTypeParametersNames= getTypeParameterNames();

Expand All @@ -330,7 +335,7 @@ private void updateTargetMethodTypeParamList() throws JavaModelException {
//only insert if typeParam not already existing
if (!methodTypeParametersNames.contains(typeParameter.getName().getIdentifier())) {
typeParamsRewrite.insertLast(typeParameter, null);
addNewTypeParamsToJavaDoc(javadoc, typeParameter);
addTypeParamToJavaDoc(typeParameter);
}
}
}
Expand Down Expand Up @@ -381,16 +386,21 @@ private List<String> getTypeParameterNames() {
return methodTypeParametersNames;
}

private void addNewTypeParamsToJavaDoc(Javadoc javadoc, TypeParameter typeParameter) {
private void addTypeParamToJavaDoc(TypeParameter typeParameter) {
Javadoc javadoc= fTargetMethodDeclaration.getJavadoc();
if (javadoc != null) {
//add new type params to javaDoc
TextElement textElement= fTargetMethodDeclarationAST.newTextElement();
textElement.setText("<" + typeParameter.getName().getIdentifier() + ">"); //$NON-NLS-1$ //$NON-NLS-2$
TagElement newParameterTag= fTargetMethodDeclarationAST.newTagElement();
newParameterTag.setTagName(TagElement.TAG_PARAM);
newParameterTag.fragments().add(textElement);
TagElement subsequentTag= JavadocUtil.findTagElementToInsertBefore(javadoc.tags(), TagElement.TAG_PARAM);
ListRewrite tagsRewrite= fTargetMethodDeclarationASTRewrite.getListRewrite(javadoc, Javadoc.TAGS_PROPERTY);
tagsRewrite.insertLast(newParameterTag, null);
if (subsequentTag == null) {
tagsRewrite.insertLast(newParameterTag, null);
} else {
tagsRewrite.insertBefore(newParameterTag, subsequentTag, null);
}
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
package org.eclipse.jdt.internal.corext.refactoring.structure;

import java.util.ArrayList;
import java.util.Arrays;
import java.util.HashMap;
import java.util.HashSet;
import java.util.Iterator;
Expand Down Expand Up @@ -2338,7 +2337,7 @@ private void changeJavadocTags() throws JavaModelException {
}
} else if (isTopOfRipple && Signature.SIG_VOID.equals(fMethod.getReturnType())){
TagElement returnNode= createReturnTag();
TagElement previousTag= findTagElementToInsertAfter(tags, TagElement.TAG_RETURN);
TagElement previousTag= JavadocUtil.findTagElementToInsertAfter(tags, TagElement.TAG_RETURN);
insertTag(returnNode, previousTag, tagsRewrite);
tags= tagsRewrite.getRewrittenList();
}
Expand Down Expand Up @@ -2378,7 +2377,7 @@ private void changeJavadocTags() throws JavaModelException {

if (! isOrderSameAsInitial()) {
// reshuffle (sort in declaration sequence) & add (only add to top of ripple):
TagElement previousTag= findTagElementToInsertAfter(tags, TagElement.TAG_PARAM);
TagElement previousTag= JavadocUtil.findTagElementToInsertAfter(tags, TagElement.TAG_PARAM);
boolean first= true; // workaround for bug 92111: preserve first tag if possible
// reshuffle:
for (ParameterInfo info : fParameterInfos) {
Expand Down Expand Up @@ -2455,7 +2454,7 @@ private void changeJavadocTags() throws JavaModelException {
}
// reshuffle:
tags= tagsRewrite.getRewrittenList();
TagElement previousTag= findTagElementToInsertAfter(tags, TagElement.TAG_THROWS);
TagElement previousTag= JavadocUtil.findTagElementToInsertAfter(tags, TagElement.TAG_THROWS);
for (ExceptionInfo info : fExceptionInfos) {
if (info.isAdded()) {
if (!isTopOfRipple)
Expand Down Expand Up @@ -2532,40 +2531,6 @@ private void insertTag(TagElement tag, TagElement previousTag, ListRewrite tagsR
tagsRewrite.insertAfter(tag, previousTag, fDescription);
}

/**
* @param tags existing tags
* @param tagName name of tag to add
* @return the <code>TagElement</code> just before a new <code>TagElement</code> with name
* <code>tagName</code>, or <code>null</code>.
*/
private TagElement findTagElementToInsertAfter(List<TagElement> tags, String tagName) {
List<String> tagOrder= Arrays.asList(
TagElement.TAG_AUTHOR,
TagElement.TAG_VERSION,
TagElement.TAG_PARAM,
TagElement.TAG_RETURN,
TagElement.TAG_THROWS,
TagElement.TAG_EXCEPTION,
TagElement.TAG_SEE,
TagElement.TAG_SINCE,
TagElement.TAG_SERIAL,
TagElement.TAG_SERIALFIELD,
TagElement.TAG_SERIALDATA,
TagElement.TAG_DEPRECATED,
TagElement.TAG_VALUE
);
int goalOrdinal= tagOrder.indexOf(tagName);
if (goalOrdinal == -1) // unknown tag -> to end
return (tags.isEmpty()) ? null : (TagElement) tags.get(tags.size());
for (int i= 0; i < tags.size(); i++) {
int tagOrdinal= tagOrder.indexOf(tags.get(i).getTagName());
if (tagOrdinal >= goalOrdinal)
return (i == 0) ? null : (TagElement) tags.get(i - 1);
}
return (tags.isEmpty()) ? null : (TagElement) tags.get(tags.size() - 1);
}


@Override
protected SingleVariableDeclaration createNewParamgument(ParameterInfo info, List<ParameterInfo> parameterInfos, List<SingleVariableDeclaration> nodes) {
return createNewSingleVariableDeclaration(info);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@

package org.eclipse.jdt.internal.corext.refactoring.util;

import java.util.Arrays;
import java.util.HashSet;
import java.util.Iterator;
import java.util.List;
Expand All @@ -38,11 +39,26 @@

public class JavadocUtil {

private static List<String> tagOrder= Arrays.asList(
TagElement.TAG_AUTHOR,
TagElement.TAG_VERSION,
TagElement.TAG_PARAM,
TagElement.TAG_RETURN,
TagElement.TAG_THROWS,
TagElement.TAG_EXCEPTION,
TagElement.TAG_SEE,
TagElement.TAG_SINCE,
TagElement.TAG_SERIAL,
TagElement.TAG_SERIALFIELD,
TagElement.TAG_SERIALDATA,
TagElement.TAG_DEPRECATED,
TagElement.TAG_VALUE
);

private JavadocUtil() {
// static-only
}

//TODO: is a copy of ChangeSignatureRefactoring.DeclarationUpdate#createParamTag(..)
public static TagElement createParamTag(String parameterName, AST ast, IJavaProject javaProject) {
TagElement paramNode= ast.newTagElement();
paramNode.setTagName(TagElement.TAG_PARAM);
Expand Down Expand Up @@ -96,4 +112,49 @@ public static void addParamJavadoc(String parameterName, MethodDeclaration metho
TagElement parameterTag= createParamTag(parameterName, astRewrite.getAST(), javaProject);
JavadocTagsSubProcessorCore.insertTag(tagsRewrite, parameterTag, leadingNames, groupDescription);
}

/**
* In a given list of tags, finds the one after which a first tag of type {@code tagName} should
* be inserted.
*
* @param tags existing tags
* @param tagName name of tag to add
* @return the <code>TagElement</code> just before the positions where a new
* <code>TagElement</code> with name <code>tagName</code> should be inserted, or
* <code>null</code> if it should be inserted as first.
*/
public static TagElement findTagElementToInsertAfter(List<TagElement> tags, String tagName) {
int goalOrdinal= tagOrder.indexOf(tagName);
if (goalOrdinal == -1) // unknown tag -> to end
return (tags.isEmpty()) ? null : (TagElement) tags.get(tags.size());
for (int i= 0; i < tags.size(); i++) {
int tagOrdinal= tagOrder.indexOf(tags.get(i).getTagName());
if (tagOrdinal >= goalOrdinal)
return (i == 0) ? null : (TagElement) tags.get(i - 1);
}
return (tags.isEmpty()) ? null : (TagElement) tags.get(tags.size() - 1);
}

/**
* In a given list of tags, finds the one before which a last tag of type {@code tagName} should
* be inserted.
*
* @param tags existing tags
* @param tagName name of tag to add
* @return the <code>TagElement</code> just after the positions where a new
* <code>TagElement</code> with name <code>tagName</code> should be inserted, or
* <code>null</code> if it should be inserted as last.
*/
public static TagElement findTagElementToInsertBefore(List<TagElement> tags, String tagName) {
int goalOrdinal= tagOrder.indexOf(tagName);
if (goalOrdinal == -1) // unknown tag -> to end
return (tags.isEmpty()) ? null : (TagElement) tags.get(tags.size());
for (int i= tags.size() - 1; i >= 0; i--) {
int tagOrdinal= tagOrder.indexOf(tags.get(i).getTagName());
if (tagOrdinal <= goalOrdinal)
return (i == tags.size() - 1) ? null : (TagElement) tags.get(i + 1);
}
return null;
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
class Foo<A> {

int j;

/**
* Some documentation ...
*
* @author someone
* @see Object#equals(Object)
* @throws IllegalArgumentException
*/
private void bar() throws IllegalArgumentException {
this.j= 0;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
class Foo<A> {

int j;

/**
* Some documentation ...
*
* @author someone
* @param foo
* @param <A>
* @see Object#equals(Object)
* @throws IllegalArgumentException
*/
private static <A> void bar(Foo<A> foo) throws IllegalArgumentException {
foo.j= 0;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
class Foo<A, B extends Runnable> {

int j;

/**
* Some documentation ...
*
* @author someone
* @param value1
* @return empty string
* @param value2
* @param value3
* @see Object#equals(Object)
* @param <T>
* @param <Z>
* @throws IllegalArgumentException
*/
private <T, Z> String bar(T value1, T value2, Z value3) throws IllegalArgumentException {
this.j= 0;
return "";
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
class Foo<A, B extends Runnable> {

int j;

/**
* Some documentation ...
*
* @author someone
* @param foo
* @param value1
* @return empty string
* @param value2
* @param value3
* @see Object#equals(Object)
* @param <T>
* @param <Z>
* @param <A>
* @param <B>
* @throws IllegalArgumentException
*/
private static <T, Z, A, B extends Runnable> String bar(Foo<A, B> foo, T value1, T value2, Z value3) throws IllegalArgumentException {
foo.j= 0;
return "";
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -576,6 +576,26 @@ public void testJavaDocWithGenerics() throws Exception {
assertHasNoCommonErrors(status);
}

/**
* See https://github.com/eclipse-jdt/eclipse.jdt.ui/issues/1043
*/
@Test
public void testJavaDocInsertBetweenExistingTags() throws Exception {
//If javadoc already contains tags, insert the new parameter information at reasonable positions
RefactoringStatus status= performRefactoringAndMatchFiles(new String[] { "p.Foo" }, 12, 18, 12, 21);
assertHasNoCommonErrors(status);
}

/**
* See https://github.com/eclipse-jdt/eclipse.jdt.ui/issues/1043
*/
@Test
public void testJavaDocShuffledTagsWithGenerics() throws Exception {
//If javadoc already has several tags in usual order, insert the new parameter information at reasonable positions
RefactoringStatus status= performRefactoringAndMatchFiles(new String[] { "p.Foo" }, 18, 27, 18, 30);
assertHasNoCommonErrors(status);
}

/**
* See https://github.com/eclipse-jdt/eclipse.jdt.ui/issues/1045
*/
Expand Down

0 comments on commit 36a9f78

Please sign in to comment.