Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add whitespace check capability and ParenPadQuickFix #298

Open
wants to merge 20 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,9 @@ public void addError(AuditEvent error) {
error.getLocalizedMessage().getColumnCharIndex() + 1,
error.getMessage(),
severity.toString().toLowerCase(),
error.getSourceName().substring(error.getSourceName().lastIndexOf('.') + 1)));
error.getSourceName().substring(error.getSourceName().lastIndexOf('.') + 1),
error.getLocalizedMessage().getKey())
);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
/*
* Copyright (C) jdneo

* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
* the Free Software Foundation, either version 3 of the License, or
* any later version.

* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.

* You should have received a copy of the GNU General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/

package com.shengchen.checkstyle.quickfix;

import org.eclipse.jface.text.Document;
import org.eclipse.jface.text.IRegion;
import org.eclipse.text.edits.TextEdit;

import java.util.function.Predicate;

public abstract class BaseEditQuickFix implements IQuickFix {

public abstract TextEdit createTextEdit(IRegion lineInfo, int markerStartOffset, String violationKey, Document doc);

protected int measureToken(String string, Predicate<Character> tokenPredicate) {
return measureToken(string, 0, tokenPredicate);
}

protected int measureToken(String string, int from, Predicate<Character> tokenPredicate) {
final int n = string.length();
for (int i = from; i < n; i++) {
if (!tokenPredicate.test(string.charAt(i))) {
return i - from;
}
}
return n - from;
}

protected int measureTokenBackwards(String string, Predicate<Character> tokenPredicate) {
return measureTokenBackwards(string, -1, tokenPredicate);
}

protected int measureTokenBackwards(String string, int from, Predicate<Character> tokenPredicate) {
final int n = string.length();
if (from == -1) {
from = n - 1;
}
for (int i = from; i >= 0; i--) {
if (!tokenPredicate.test(string.charAt(i))) {
return from - i;
}
}
return from + 1;
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
import java.util.List;
import java.util.stream.Collectors;

public abstract class BaseQuickFix {
public abstract class BaseQuickFix implements IQuickFix {
public abstract ASTVisitor getCorrectingASTVisitor(IRegion lineInfo, int markerStartOffset);

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,16 @@ public enum FixableCheck {
// Modifier
MODIFIER_ORDER_CHECK("ModifierOrderCheck"), REDUNDANT_MODIFIER_CHECK("RedundantModifierCheck"),

// Whitespace
PAREN_PAD_CHECK("ParenPadCheck"),
WHITESPACE_AFTER_CHECK("WhitespaceAfterCheck"),
WHITESPACE_AROUND_CHECK("WhitespaceAroundCheck"),
NO_WHITESPACE_AFTER_CHECK("NoWhitespaceAfterCheck"),
NO_WHITESPACE_BEFORE_CHECK("NoWhitespaceBeforeCheck"),
NEWLINE_AT_END_OF_FILE_CHECK("NewlineAtEndOfFileCheck"),
GENERIC_WHITESPACE_CHECK("GenericWhitespaceCheck"),
METHOD_PARAM_PAD_CHECK("MethodParamPadCheck"),

// Misc
FINAL_PARAMETERS_CHECK("FinalParametersCheck"), UNCOMMENTED_MAIN_CHECK("UncommentedMainCheck"),
UPPER_ELL_CHECK("UpperEllCheck"), ARRAY_TYPE_STYLE_CHECK("ArrayTypeStyleCheck");
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
/*
* Copyright (C) jdneo

* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
* the Free Software Foundation, either version 3 of the License, or
* any later version.

* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.

* You should have received a copy of the GNU General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/

package com.shengchen.checkstyle.quickfix;

public interface IQuickFix {

}
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,14 @@
import com.shengchen.checkstyle.quickfix.modifier.ModifierOrderQuickFix;
import com.shengchen.checkstyle.quickfix.modifier.RedundantModifierQuickFix;
import com.shengchen.checkstyle.quickfix.utils.EditUtils;
import com.shengchen.checkstyle.quickfix.whitepace.GenericWhitespaceQuickFix;
import com.shengchen.checkstyle.quickfix.whitepace.MethodParamPadQuickFix;
import com.shengchen.checkstyle.quickfix.whitepace.NewlineAtEndOfFileQuickFix;
import com.shengchen.checkstyle.quickfix.whitepace.NoWhitespaceAfterQuickFix;
import com.shengchen.checkstyle.quickfix.whitepace.NoWhitespaceBeforeQuickFix;
import com.shengchen.checkstyle.quickfix.whitepace.ParenPadQuickFix;
import com.shengchen.checkstyle.quickfix.whitepace.WhitespaceAfterQuickFix;
import com.shengchen.checkstyle.quickfix.whitepace.WhitespaceAroundQuickFix;
import com.shengchen.checkstyle.runner.api.IQuickFixService;

import org.eclipse.jdt.core.ICompilationUnit;
Expand All @@ -49,15 +57,21 @@
import org.eclipse.jface.text.Document;
import org.eclipse.jface.text.IRegion;
import org.eclipse.lsp4j.WorkspaceEdit;
import org.eclipse.text.edits.MalformedTreeException;
import org.eclipse.text.edits.MultiTextEdit;
import org.eclipse.text.edits.TextEdit;

import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;

public class QuickFixService implements IQuickFixService {

private final Map<String, BaseQuickFix> quickFixMap;
private final Map<String, IQuickFix> quickFixMap;

public QuickFixService() {
quickFixMap = new HashMap<>();
Expand All @@ -81,16 +95,25 @@ public QuickFixService() {
quickFixMap.put(FixableCheck.STRING_LITERAL_EQUALITY.toString(), new StringLiteralEqualityQuickFix());
quickFixMap.put(FixableCheck.MULTIPLE_VARIABLE_DECLARATIONS_CHECK.toString(),
new MultipleVariableDeclarationsQuickFix());
quickFixMap.put(FixableCheck.PAREN_PAD_CHECK.toString(), new ParenPadQuickFix());
quickFixMap.put(FixableCheck.WHITESPACE_AFTER_CHECK.toString(), new WhitespaceAfterQuickFix());
quickFixMap.put(FixableCheck.WHITESPACE_AROUND_CHECK.toString(), new WhitespaceAroundQuickFix());
quickFixMap.put(FixableCheck.NO_WHITESPACE_AFTER_CHECK.toString(), new NoWhitespaceAfterQuickFix());
quickFixMap.put(FixableCheck.NO_WHITESPACE_BEFORE_CHECK.toString(), new NoWhitespaceBeforeQuickFix());
quickFixMap.put(FixableCheck.NEWLINE_AT_END_OF_FILE_CHECK.toString(), new NewlineAtEndOfFileQuickFix());
quickFixMap.put(FixableCheck.GENERIC_WHITESPACE_CHECK.toString(), new GenericWhitespaceQuickFix());
quickFixMap.put(FixableCheck.METHOD_PARAM_PAD_CHECK.toString(), new MethodParamPadQuickFix());
}

public BaseQuickFix getQuickFix(String sourceName) {
public IQuickFix getQuickFix(String sourceName) {
return quickFixMap.get(sourceName);
}

public WorkspaceEdit quickFix(
String fileToCheckUri,
List<Double> offsets,
List<String> sourceNames
List<String> sourceNames,
List<String> violationKeys
) throws JavaModelException, IllegalArgumentException, BadLocationException {
final ICompilationUnit unit = JDTUtils.resolveCompilationUnit(fileToCheckUri);
final Document document = new Document(unit.getSource());
Expand All @@ -100,16 +123,83 @@ public WorkspaceEdit quickFix(
final CompilationUnit astRoot = (CompilationUnit) astParser.createAST(null);
astRoot.recordModifications();

final List<TextEdit> textEdits = new ArrayList<>();

for (int i = 0; i < offsets.size(); i++) {
final IQuickFix quickFix = getQuickFix(sourceNames.get(i));
if (quickFix == null) {
continue;
}

final int offset = offsets.get(i).intValue();
final BaseQuickFix quickFix = getQuickFix(sourceNames.get(i));
if (quickFix != null) {
final IRegion lineInfo = document.getLineInformationOfOffset(offset);
astRoot.accept(quickFix.getCorrectingASTVisitor(lineInfo, offset));
final IRegion lineInfo = document.getLineInformationOfOffset(offset);
final String violationKey = violationKeys.get(i);
if (quickFix instanceof BaseQuickFix) {
astRoot.accept(((BaseQuickFix) quickFix).getCorrectingASTVisitor(lineInfo, offset));
} else if (quickFix instanceof BaseEditQuickFix) {
final TextEdit edit = ((BaseEditQuickFix) quickFix).createTextEdit(
lineInfo, offset, violationKey, document);
if (edit != null) {
addAllEdits(edit, textEdits);
}
}
}

/* Add text edits before AST edits, as whitespace changes need to be applied first */
final MultiTextEdit result = new MultiTextEdit();
for (final TextEdit textEdit : textEdits) {
try {
result.addChild(textEdit.copy());
} catch (MalformedTreeException e) {
/* Ignore text edits that can't be added; it is due to conflicts */
}
}

final TextEdit edit = astRoot.rewrite(document, null);
return EditUtils.convertToWorkspaceEdit(unit, edit);
final MultiTextEdit astEdits = (MultiTextEdit) astRoot.rewrite(document, null);
while (astEdits.getChildrenSize() > 0) {
final TextEdit astEdit = astEdits.removeChild(0);
try {
result.addChild(astEdit);
} catch (MalformedTreeException e) {
/* Ignore text edits that can't be added; it is due to conflicts */
}
}

return EditUtils.convertToWorkspaceEdit(unit, result);
}

private void addAllEdits(final TextEdit source, final List<TextEdit> dest) {
for (final TextEdit anEdit : allEdits(source)) {
if (canAddEdit(anEdit, dest)) {
dest.add(anEdit);
}
}
}

/**
* Check whether we can add the given new edit to our list of existing edits. This prevents
* edits that might conflict or double-up.
*/
private boolean canAddEdit(TextEdit edit, Collection<TextEdit> existingEdits) {
for (final TextEdit existingEdit : existingEdits) {
if (existingEdit instanceof MultiTextEdit) {
if (!canAddEdit(edit, Arrays.asList(((MultiTextEdit) existingEdit).getChildren()))) {
return false;
}
} else {
if (existingEdit.covers(edit) || existingEdit.getOffset() == edit.getOffset()) {
return false;
}
}
}
return true;
}

private Iterable<TextEdit> allEdits(TextEdit edit) {
if (edit instanceof MultiTextEdit) {
return Arrays.asList(((MultiTextEdit) edit).getChildren());
} else {
return Collections.singleton(edit);
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
/*
* Copyright (C) jdneo

* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
* the Free Software Foundation, either version 3 of the License, or
* any later version.

* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.

* You should have received a copy of the GNU General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/

package com.shengchen.checkstyle.quickfix.whitepace;

import com.shengchen.checkstyle.quickfix.BaseEditQuickFix;

import org.eclipse.jface.text.BadLocationException;
import org.eclipse.jface.text.Document;
import org.eclipse.jface.text.IRegion;
import org.eclipse.text.edits.DeleteEdit;
import org.eclipse.text.edits.InsertEdit;
import org.eclipse.text.edits.TextEdit;

/**
* Quick fix for
* https://checkstyle.org/apidocs/com/puppycrawl/tools/checkstyle/checks/whitespace/GenericWhitespaceCheck.html
*/
public class GenericWhitespaceQuickFix extends BaseEditQuickFix {

@Override
public TextEdit createTextEdit(IRegion lineInfo, int markerStartOffset, String violationKey, Document doc) {
try {
final int fromStartOfLine = markerStartOffset - lineInfo.getOffset();
final String string = doc.get(lineInfo.getOffset(), lineInfo.getLength());

if ("ws.illegalFollow".equals(violationKey)) {
return new InsertEdit(markerStartOffset + 1, " ");
} else if ("ws.notPreceded".equals(violationKey)) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be better if we can directly reference the static const defined in the Checkstyle lib, seems it's not easily achievable given the current OSGi framework. 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jdneo yes, I agree. I don't have experience with OSGi or how all of those dependencies are setup so I took the path of least resistance.

return new InsertEdit(markerStartOffset, " ");
} else if ("ws.followed".equals(violationKey)) {
final int tokenLength = measureToken(string, fromStartOfLine + 1, Character::isWhitespace);
if (tokenLength > 0) {
return new DeleteEdit(markerStartOffset + 1, tokenLength);
}
} else if ("ws.preceded".equals(violationKey)) {
final int tokenLength = measureTokenBackwards(string, fromStartOfLine - 1, Character::isWhitespace);
if (tokenLength > 0) {
return new DeleteEdit(markerStartOffset - tokenLength, tokenLength);
}
}

return null;
} catch (BadLocationException e) {
return null;
}
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
/*
* Copyright (C) jdneo

* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
* the Free Software Foundation, either version 3 of the License, or
* any later version.

* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.

* You should have received a copy of the GNU General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/

package com.shengchen.checkstyle.quickfix.whitepace;

import com.shengchen.checkstyle.quickfix.BaseEditQuickFix;

import org.eclipse.jface.text.BadLocationException;
import org.eclipse.jface.text.Document;
import org.eclipse.jface.text.IRegion;
import org.eclipse.text.edits.DeleteEdit;
import org.eclipse.text.edits.InsertEdit;
import org.eclipse.text.edits.TextEdit;

/**
* Quick fix for
* https://checkstyle.org/apidocs/com/puppycrawl/tools/checkstyle/checks/whitespace/MethodParamPadCheck.html
*/
public class MethodParamPadQuickFix extends BaseEditQuickFix {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems not handle well with:

<module name="MethodParamPad">
  <property name="tokens" value="METHOD_DEF"/>
  <property name="option" value="space"/>
  <property name="allowLineBreaks" value="true"/>
</module>

image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jdneo when I test this, Checkstyle reports that the "(" is not preceded by whitespace, and the quick fix adds a space before the "(". That seems like it's doing the right thing to me?


@Override
public TextEdit createTextEdit(IRegion lineInfo, int markerStartOffset, String violationKey, Document doc) {
try {
final int fromStartOfLine = markerStartOffset - lineInfo.getOffset();
final String string = doc.get(lineInfo.getOffset(), lineInfo.getLength());

if ("ws.notPreceded".equals(violationKey)) {
return new InsertEdit(markerStartOffset, " ");
} else if ("ws.preceded".equals(violationKey)) {
final int tokenLength = measureTokenBackwards(string, fromStartOfLine - 1, Character::isWhitespace);
if (tokenLength > 0) {
return new DeleteEdit(markerStartOffset - tokenLength, tokenLength);
}
}

return null;
} catch (BadLocationException e) {
return null;
}
}

}
Loading