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

[java] New rule ConfusingArgumentToVarargsMethod #4971

Merged
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion Dangerfile
Expand Up @@ -28,7 +28,7 @@ def run_pmdtester
@base_branch = ENV['PMD_CI_BRANCH']
@logger.info "\n\n--------------------------------------"
@logger.info "Run against PR base #{@base_branch}"
@summary = PmdTester::Runner.new(get_args(@base_branch)).run
@summary = PmdTester::Runner.new(get_args(@base_branch, FALSE)).run

unless Dir.exist?('target/reports/diff')
message("No regression tested rules have been changed.", sticky: true)
Expand Down
3 changes: 3 additions & 0 deletions docs/pages/release_notes.md
Expand Up @@ -18,6 +18,9 @@ This is a {{ site.pmd.release_type }} release.

- The new Java rule {%rule java/bestpractices/UnnecessaryVarargsArrayCreation %} reports explicit array creation
when a varargs is expected. This is more heavy to read and could be simplified.
- The new Java rule {%rule java/errorprone/ConfusingArgumentToVarargsMethod %} reports some confusing situations
where a varargs method is called with an inexact argument type. These may end up in a mismatch between the expected
parameter type and the actual value.

### 🌟 Rule Changes

Expand Down
Expand Up @@ -75,7 +75,7 @@ public Object visit(ASTUserClass node, Object data) {
String.valueOf(classLevelThreshold),
};

asCtx(data).addViolation(node, messageParams);
asCtx(data).addViolation(node, (Object[]) messageParams);
}
}
return data;
Expand Down
Expand Up @@ -78,7 +78,7 @@ public Object visit(ASTUserClass node, Object data) {
" total",
classWmc + " (highest " + classHighest + ")", };

asCtx(data).addViolation(node, messageParams);
asCtx(data).addViolation(node, (Object[]) messageParams);
}
}
return data;
Expand Down
Expand Up @@ -76,7 +76,7 @@ static void checkMemberAccess(RuleContext ruleContext, JavaNode refExpr, JAccess
JavaNode node = sym.tryGetNode();
assert node != null : "Node should be in the same compilation unit";
if (reportedNodes.add(node)) {
ruleContext.addViolation(node, new String[] {stripPackageName(refExpr.getEnclosingType().getSymbol())});
ruleContext.addViolation(node, stripPackageName(refExpr.getEnclosingType().getSymbol()));
}
}
}
Expand Down
Expand Up @@ -24,7 +24,7 @@ public MissingOverrideRule() {
@Override
public Object visit(ASTMethodDeclaration node, Object data) {
if (node.isOverridden() && !node.isAnnotationPresent(Override.class)) {
asCtx(data).addViolation(node, new Object[] { PrettyPrintingUtil.displaySignature(node) });
asCtx(data).addViolation(node, PrettyPrintingUtil.displaySignature(node));
}
return data;
}
Expand Down
Expand Up @@ -11,10 +11,8 @@
import net.sourceforge.pmd.lang.java.ast.InvocationNode;
import net.sourceforge.pmd.lang.java.ast.JavaNode;
import net.sourceforge.pmd.lang.java.rule.AbstractJavaRulechainRule;
import net.sourceforge.pmd.lang.java.types.JArrayType;
import net.sourceforge.pmd.lang.java.types.JTypeMirror;
import net.sourceforge.pmd.lang.java.types.OverloadSelectionResult;
import net.sourceforge.pmd.lang.java.types.TypePrettyPrint;

public class UnnecessaryVarargsArrayCreationRule extends AbstractJavaRulechainRule {

Expand All @@ -26,6 +24,9 @@ public UnnecessaryVarargsArrayCreationRule() {

@Override
public Object visit(ASTArrayAllocation array, Object data) {
if (array.getArrayInitializer() == null) {
return null;
}

JavaNode parent = array.getParent();
if (parent instanceof ASTArgumentList && array.getIndexInParent() == parent.getNumChildren() - 1) {
Expand All @@ -39,19 +40,10 @@ public Object visit(ASTArrayAllocation array, Object data) {

List<JTypeMirror> formals = info.getMethodType().getFormalParameters();
JTypeMirror lastFormal = formals.get(formals.size() - 1);
JTypeMirror expectedComponent = ((JArrayType) lastFormal).getComponentType();

if (array.getTypeMirror().isSubtypeOf(expectedComponent)
&& !array.getTypeMirror().equals(lastFormal)) {
// confusing
asCtx(data)
.addViolationWithMessage(
array,
"Unclear if a varargs or non-varargs call is intended. Cast to {0} or {0}[], or pass varargs parameters separately to clarify intent.",
TypePrettyPrint.prettyPrintWithSimpleNames(expectedComponent)
);
} else if (array.getArrayInitializer() != null) {
// just regular unnecessary

if (array.getTypeMirror().equals(lastFormal)) {
// If type not equal, then it would not actually be equivalent to remove the array creation.
// That case may be caught by ConfusingArgumentToVarargsMethod
asCtx(data).addViolation(array);
}
}
Expand Down
Expand Up @@ -52,7 +52,7 @@ private void check(ASTExecutableDeclaration node, Object data) {
for (ASTFormalParameter formal : node.getFormalParameters()) {
ASTVariableId varId = formal.getVarId();
if (JavaAstUtils.isNeverUsed(varId) && !JavaRuleUtil.isExplicitUnusedVarName(varId.getName())) {
asCtx(data).addViolation(varId, new Object[] { node instanceof ASTMethodDeclaration ? "method" : "constructor", varId.getName(), });
asCtx(data).addViolation(varId, node instanceof ASTMethodDeclaration ? "method" : "constructor", varId.getName());
}
}
}
Expand Down
Expand Up @@ -60,11 +60,9 @@ RegexPropertyBuilder defaultProp(String name, String displayName) {
void checkMatches(T node, PropertyDescriptor<Pattern> regex, Object data) {
String name = nameExtractor(node);
if (!getProperty(regex).matcher(name).matches()) {
asCtx(data).addViolation(node, new Object[]{
kindDisplayName(node, regex),
name,
getProperty(regex).toString(),
});
asCtx(data).addViolation(node, kindDisplayName(node, regex),
name,
getProperty(regex).toString());
}
}

Expand Down
Expand Up @@ -90,11 +90,9 @@ public Object visit(ASTEnumConstant node, Object data) {
// This inlines checkMatches because there's no variable declarator id

if (!getProperty(enumConstantRegex).matcher(node.getImage()).matches()) {
asCtx(data).addViolation(node, new Object[]{
"enum constant",
node.getImage(),
getProperty(enumConstantRegex).toString(),
});
asCtx(data).addViolation(node, "enum constant",
node.getImage(),
getProperty(enumConstantRegex).toString());
}

return data;
Expand Down
Expand Up @@ -91,7 +91,7 @@ public Object visit(ASTTryStatement node, Object data) {
// By convention, lower catch blocks are collapsed into the highest one
// The first node of the equivalence class is thus the block that should be transformed
for (int i = 1; i < identicalStmts.size(); i++) {
asCtx(data).addViolation(identicalStmts.get(i), new String[]{identicalBranchName, });
asCtx(data).addViolation(identicalStmts.get(i), identicalBranchName);
}
}
}
Expand Down
Expand Up @@ -116,7 +116,7 @@ private void checkPrefixedTransformMethods(ASTMethodDeclaration node, Object dat
&& prefixes.contains(splitMethodName[0].toLowerCase(Locale.ROOT))) {
// "To" or any other configured prefix found
asCtx(data).addViolationWithMessage(node, "Linguistics Antipattern - The transform method ''{0}'' should not return void linguistically",
new Object[] { nameOfMethod });
nameOfMethod);
}
}

Expand All @@ -125,7 +125,7 @@ private void checkTransformMethods(ASTMethodDeclaration node, Object data, Strin
if (node.isVoid() && containsCamelCaseWord(nameOfMethod, StringUtils.capitalize(infix))) {
// "To" or any other configured infix in the middle somewhere
asCtx(data).addViolationWithMessage(node, "Linguistics Antipattern - The transform method ''{0}'' should not return void linguistically",
new Object[] { nameOfMethod });
nameOfMethod);
// the first violation is sufficient - it is still the same method we are analyzing here
break;
}
Expand All @@ -135,14 +135,14 @@ private void checkTransformMethods(ASTMethodDeclaration node, Object data, Strin
private void checkGetters(ASTMethodDeclaration node, Object data, String nameOfMethod) {
if (startsWithCamelCaseWord(nameOfMethod, "get") && node.isVoid()) {
asCtx(data).addViolationWithMessage(node, "Linguistics Antipattern - The getter ''{0}'' should not return void linguistically",
new Object[] { nameOfMethod });
nameOfMethod);
}
}

private void checkSetters(ASTMethodDeclaration node, Object data, String nameOfMethod) {
if (startsWithCamelCaseWord(nameOfMethod, "set") && !node.isVoid()) {
asCtx(data).addViolationWithMessage(node, "Linguistics Antipattern - The setter ''{0}'' should not return any type except void linguistically",
new Object[] { nameOfMethod });
nameOfMethod);
}
}

Expand All @@ -158,7 +158,7 @@ private void checkBooleanMethods(ASTMethodDeclaration node, Object data, String
for (String prefix : getProperty(BOOLEAN_METHOD_PREFIXES_PROPERTY)) {
if (startsWithCamelCaseWord(nameOfMethod, prefix) && !isBooleanType(t)) {
asCtx(data).addViolationWithMessage(node, "Linguistics Antipattern - The method ''{0}'' indicates linguistically it returns a boolean, but it returns ''{1}''",
new Object[] {nameOfMethod, PrettyPrintingUtil.prettyPrintType(t) });
nameOfMethod, PrettyPrintingUtil.prettyPrintType(t));
}
}
}
Expand All @@ -168,7 +168,7 @@ private void checkField(ASTType typeNode, ASTVariableDeclarator node, Object dat
for (String prefix : getProperty(BOOLEAN_FIELD_PREFIXES_PROPERTY)) {
if (startsWithCamelCaseWord(node.getName(), prefix) && !isBooleanType(typeNode)) {
asCtx(data).addViolationWithMessage(node, "Linguistics Antipattern - The field ''{0}'' indicates linguistically it is a boolean, but it is ''{1}''",
new Object[] { node.getName(), PrettyPrintingUtil.prettyPrintType(typeNode) });
node.getName(), PrettyPrintingUtil.prettyPrintType(typeNode));
}
}
}
Expand All @@ -177,7 +177,7 @@ private void checkVariable(ASTType typeNode, ASTVariableDeclarator node, Object
for (String prefix : getProperty(BOOLEAN_FIELD_PREFIXES_PROPERTY)) {
if (startsWithCamelCaseWord(node.getName(), prefix) && !isBooleanType(typeNode)) {
asCtx(data).addViolationWithMessage(node, "Linguistics Antipattern - The variable ''{0}'' indicates linguistically it is a boolean, but it is ''{1}''",
new Object[] { node.getName(), PrettyPrintingUtil.prettyPrintType(typeNode) });
node.getName(), PrettyPrintingUtil.prettyPrintType(typeNode));
}
}
}
Expand Down
Expand Up @@ -88,7 +88,7 @@ && methodProbablyMeansSame(methodCall)) {
// we don't actually know where the method came from
String simpleName = formatMemberName(next, methodCall.getMethodType().getSymbol());
String unnecessary = produceQualifier(deepest, next, true);
asCtx(data).addViolation(next, new Object[] {unnecessary, simpleName, ""});
asCtx(data).addViolation(next, unnecessary, simpleName, "");
return null;
}
} else if (getProperty(REPORT_FIELDS) && opa instanceof ASTFieldAccess) {
Expand All @@ -98,7 +98,7 @@ && methodProbablyMeansSame(methodCall)) {
String simpleName = formatMemberName(next, fieldAccess.getReferencedSym());
String reasonToString = unnecessaryReasonWrapper(reasonForFieldInScope);
String unnecessary = produceQualifier(deepest, next, true);
asCtx(data).addViolation(next, new Object[] {unnecessary, simpleName, reasonToString});
asCtx(data).addViolation(next, unnecessary, simpleName, reasonToString);
return null;
}
}
Expand All @@ -108,7 +108,7 @@ && methodProbablyMeansSame(methodCall)) {
String simpleName = next.getSimpleName();
String reasonToString = unnecessaryReasonWrapper(bestReason);
String unnecessary = produceQualifier(deepest, next, false);
asCtx(data).addViolation(next, new Object[] {unnecessary, simpleName, reasonToString});
asCtx(data).addViolation(next, unnecessary, simpleName, reasonToString);
}
return null;
}
Expand Down
Expand Up @@ -220,7 +220,7 @@ private void visitImport(ASTImportDeclaration node, Object data, String thisPack
}

private void reportWithMessage(ASTImportDeclaration node, Object data, String message) {
asCtx(data).addViolationWithMessage(node, message, new String[] { PrettyPrintingUtil.prettyImport(node) });
asCtx(data).addViolationWithMessage(node, message, PrettyPrintingUtil.prettyImport(node));
}

@Override
Expand Down
Expand Up @@ -54,12 +54,10 @@ private void reportUnnecessaryModifiers(Object data, JavaNode node,
if (unnecessaryModifiers.isEmpty()) {
return;
}
asCtx(data).addViolation(node, new String[]{
formatUnnecessaryModifiers(unnecessaryModifiers),
PrettyPrintingUtil.getPrintableNodeKind(node),
PrettyPrintingUtil.getNodeName(node),
explanation.isEmpty() ? "" : ": " + explanation,
});
asCtx(data).addViolation(node, formatUnnecessaryModifiers(unnecessaryModifiers),
PrettyPrintingUtil.getPrintableNodeKind(node),
PrettyPrintingUtil.getNodeName(node),
explanation.isEmpty() ? "" : ": " + explanation);
}


Expand Down
Expand Up @@ -85,7 +85,7 @@ public Object visit(ASTConstructorCall ctorCall, Object data) {
JavaNode reportNode = targs == null ? newTypeNode : targs;
String message = targs == null ? RAW_TYPE_MESSAGE : REPLACE_TYPE_ARGS_MESSAGE;
String replaceWith = produceSuggestedExprImage(ctorCall);
asCtx(data).addViolationWithMessage(reportNode, message, new String[] { replaceWith });
asCtx(data).addViolationWithMessage(reportNode, message, replaceWith);
}
return null;
}
Expand Down
Expand Up @@ -56,10 +56,10 @@ private Object visitMethod(ASTExecutableDeclaration node, Object data) {
int cognitive = MetricsUtil.computeMetric(COGNITIVE_COMPLEXITY, node);
final int reportLevel = getReportLevel();
if (cognitive >= reportLevel) {
asCtx(data).addViolation(node, new String[] { node instanceof ASTMethodDeclaration ? "method" : "constructor",
PrettyPrintingUtil.displaySignature(node),
String.valueOf(cognitive),
String.valueOf(reportLevel) });
asCtx(data).addViolation(node, node instanceof ASTMethodDeclaration ? "method" : "constructor",
PrettyPrintingUtil.displaySignature(node),
String.valueOf(cognitive),
String.valueOf(reportLevel));
}

return data;
Expand Down
Expand Up @@ -89,7 +89,7 @@ public Object visitTypeDecl(ASTTypeDeclaration node, Object data) {
" total",
classWmc + " (highest " + classHighest + ")", };

asCtx(data).addViolation(node, messageParams);
asCtx(data).addViolation(node, (Object[]) messageParams);
}
}
return data;
Expand Down Expand Up @@ -120,11 +120,7 @@ private void visitMethodLike(ASTExecutableDeclaration node, Object data) {

String kindname = node instanceof ASTConstructorDeclaration ? "constructor" : "method";


asCtx(data).addViolation(node, new String[] {kindname,
opname,
"",
"" + cyclo, });
asCtx(data).addViolation(node, kindname, opname, "", "" + cyclo);
}
}
}
Expand Down
Expand Up @@ -53,9 +53,9 @@ private void visitTypeDecl(ASTTypeDeclaration node, RuleContext data) {
int noam = MetricsUtil.computeMetric(NUMBER_OF_ACCESSORS, node);
int wmc = MetricsUtil.computeMetric(WEIGHED_METHOD_COUNT, node);

asCtx(data).addViolation(node, new Object[] {node.getSimpleName(),
StringUtil.percentageString(woc, 3),
nopa, noam, wmc, });
asCtx(data).addViolation(node, node.getSimpleName(),
StringUtil.percentageString(woc, 3),
nopa, noam, wmc);
}
}

Expand Down
Expand Up @@ -59,9 +59,9 @@ public Object visit(ASTClassDeclaration node, Object data) {

if (wmc >= WMC_VERY_HIGH && atfd > FEW_ATFD_THRESHOLD && tcc < TCC_THRESHOLD) {

asCtx(data).addViolation(node, new Object[] {wmc,
StringUtil.percentageString(tcc, 3),
atfd, });
asCtx(data).addViolation(node, wmc,
StringUtil.percentageString(tcc, 3),
atfd);
}
return data;
}
Expand Down
Expand Up @@ -132,11 +132,9 @@ public Object visit(ASTFieldAccess node, Object data) {
asCtx(data).addViolationWithMessage(
node,
FIELD_ACCESS_ON_FOREIGN_VALUE,
new Object[] {
node.getName(),
PrettyPrintingUtil.prettyPrint(node.getQualifier()),
foreignDegree(node.getQualifier()),
});
node.getName(),
PrettyPrintingUtil.prettyPrint(node.getQualifier()),
foreignDegree(node.getQualifier()));
}
return null;
}
Expand All @@ -147,11 +145,9 @@ public Object visit(ASTMethodCall node, Object data) {
asCtx(data).addViolationWithMessage(
node,
METHOD_CALL_ON_FOREIGN_VALUE,
new Object[] {
node.getMethodName(),
PrettyPrintingUtil.prettyPrint(node.getQualifier()),
foreignDegree(node.getQualifier()),
});
node.getMethodName(),
PrettyPrintingUtil.prettyPrint(node.getQualifier()),
foreignDegree(node.getQualifier()));
}
return null;
}
Expand Down