Skip to content

Commit

Permalink
close KengoTODA#36: Merge remote-tracking branch 'origin/issue-36'
Browse files Browse the repository at this point in the history
  • Loading branch information
@KengoTODA
KengoTODA committed Sep 18, 2015
2 parents b82b52d + 71ac69b commit c9099aa
Show file tree
Hide file tree
Showing 4 changed files with 79 additions and 26 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -99,4 +99,5 @@

## 1.2.2

- SLF4J_SIGN_ONLY_FORMAT should work even if format is given as method parameter (issue #36)
- Fix `Can't get stack offset 0 from []` bug (issue #37)
98 changes: 73 additions & 25 deletions bug-pattern/src/main/java/jp/skypencil/findbugs/slf4j/WrongPlaceholderDetector.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import java.util.regex.Matcher;
import java.util.regex.Pattern;

import javax.annotation.Nonnull;
import javax.annotation.Nullable;

import jp.skypencil.findbugs.slf4j.parameter.AbstractDetectorForParameterArray;
Expand Down Expand Up @@ -33,15 +34,20 @@ public class WrongPlaceholderDetector extends AbstractDetectorForParameterArray
/**
* Called method, index of argument for log message, and expected placeholders
*/
private Table<Method, Integer, List<PotentialBug>> potentialBugs;
private Table<Method, Integer, List<PotentialPlaceHolderMismatch>> potentialPlaceHolderMismatch;
/**
* Called method, index of argument for log message, and expected placeholders
*/
private Table<Method, Integer, List<PotentialSignOnlyFormat>> potentialSignOnlyFormat;

public WrongPlaceholderDetector(BugReporter bugReporter) {
super(bugReporter);
}

@Override
public void visitClassContext(ClassContext classContext) {
potentialBugs = HashBasedTable.create();
potentialPlaceHolderMismatch = HashBasedTable.create();
potentialSignOnlyFormat = HashBasedTable.create();
super.visitClassContext(classContext);
validatePrivateMethodCall();
}
Expand All @@ -52,15 +58,14 @@ public void visitClassContext(ClassContext classContext) {
* @see https://github.com/KengoTODA/findbugs-slf4j/issues/35
*/
private void validatePrivateMethodCall() {
for (Cell<Method, Integer, List<PotentialBug>> cell : potentialBugs.cellSet()) {
for (Cell<Method, Integer, List<PotentialPlaceHolderMismatch>> cell : potentialPlaceHolderMismatch.cellSet()) {
Method method = cell.getRowKey();
Integer argumentIndex = cell.getColumnKey();
List<Object> constants = getConstantsToCall(method, argumentIndex);
if (constants == null) {
continue;
}

for (PotentialBug bug : cell.getValue()) {
for (PotentialPlaceHolderMismatch bug : cell.getValue()) {
for (Object constant : constants) {
int placeholders = countPlaceholder(constant.toString());
if (placeholders != bug.getParameterCount()) {
Expand All @@ -69,6 +74,22 @@ private void validatePrivateMethodCall() {
}
}
}
for (Cell<Method, Integer, List<PotentialSignOnlyFormat>> cell : potentialSignOnlyFormat.cellSet()) {
Method method = cell.getRowKey();
Integer argumentIndex = cell.getColumnKey();
List<Object> constants = getConstantsToCall(method, argumentIndex);
if (constants == null) {
continue;
}
for (PotentialSignOnlyFormat bug : cell.getValue()) {
for (Object constant : constants) {
String format = constant.toString();
if (!verifyFormat(format)) {
getBugReporter().reportBug(bug.getRawBug(format));
}
}
}
}
}

@Override
Expand All @@ -89,25 +110,35 @@ protected void onLog(@Nullable String format, @Nullable ArrayData arrayData) {

if (format == null) {
int argumentIndexOfLogFormat = getArgumentIndexOfLogFormat();
List<PotentialBug> list = potentialBugs.get(this.getMethod(), argumentIndexOfLogFormat);
if (list == null) {
list = Lists.newArrayList();
potentialBugs.put(this.getMethod(), argumentIndexOfLogFormat, list);
}
list.add(new PotentialBug(createBugInstance(-1, parameterCount), parameterCount));
get(potentialPlaceHolderMismatch, getMethod(), argumentIndexOfLogFormat).add(new PotentialPlaceHolderMismatch(createPlaceHolderMismatchBugInstance(-1, parameterCount), parameterCount));
get(potentialSignOnlyFormat, getMethod(), argumentIndexOfLogFormat).add(new PotentialSignOnlyFormat(createSignOnlyFormatBugInstance(format)));
return;
}
int placeholderCount = countPlaceholder(format);
verifyFormat(format);
if (!verifyFormat(format)) {
getBugReporter().reportBug(createSignOnlyFormatBugInstance(format));
}

if (placeholderCount != parameterCount) {
BugInstance bug = createBugInstance(placeholderCount,
BugInstance bug = createPlaceHolderMismatchBugInstance(placeholderCount,
parameterCount);
getBugReporter().reportBug(bug);
}
}

private BugInstance createBugInstance(int placeholderCount,
private BugInstance createSignOnlyFormatBugInstance(@Nullable String formatString) {
BugInstance bug = new BugInstance(this,
"SLF4J_SIGN_ONLY_FORMAT", NORMAL_PRIORITY)
.addSourceLine(this)
.addClassAndMethod(this)
.addCalledMethod(this);
if (formatString != null) {
bug.addString(formatString);
}
return bug;
}

private BugInstance createPlaceHolderMismatchBugInstance(int placeholderCount,
int parameterCount) {
BugInstance bug = new BugInstance(this,
"SLF4J_PLACE_HOLDER_MISMATCH", HIGH_PRIORITY)
Expand All @@ -120,21 +151,18 @@ private BugInstance createBugInstance(int placeholderCount,
return bug;
}

private void verifyFormat(String formatString) {
/**
* @return true if given formatString is valid
*/
private boolean verifyFormat(@Nonnull String formatString) {
CodepointIterator iterator = new CodepointIterator(formatString);
while (iterator.hasNext()) {
if (Character.isLetter(iterator.next().intValue())) {
// found non-sign character.
return;
return true;
}
}

BugInstance bug = new BugInstance(this,
"SLF4J_SIGN_ONLY_FORMAT", NORMAL_PRIORITY)
.addString(formatString)
.addSourceLine(this).addClassAndMethod(this)
.addCalledMethod(this);
getBugReporter().reportBug(bug);
return false;
}

int countParameter(OpcodeStack stack, String methodSignature, ThrowableHandler throwableHandler) {
Expand Down Expand Up @@ -200,10 +228,30 @@ public void store(Item storedItem, ArrayData data, int index) {
};
}

private static final class PotentialBug {
private <R,C,E> List<E> get(Table<R,C,List<E>> table, R row, C column) {
List<E> list = table.get(row, column);
if (list == null) {
list = Lists.newArrayList();
table.put(row, column, list);
}
return list;
}

private static final class PotentialSignOnlyFormat {
private final BugInstance bug;
private PotentialSignOnlyFormat(BugInstance bug) {
this.bug = bug;
}

private BugInstance getRawBug(String format) {
return bug.addString(format);
}
}

private static final class PotentialPlaceHolderMismatch {
private final BugInstance bug;
private final int parameterCount;
private PotentialBug(BugInstance bug, int parameterCount) {
private PotentialPlaceHolderMismatch(BugInstance bug, int parameterCount) {
this.bug = bug;
this.parameterCount = parameterCount;
}
Expand Down
4 changes: 4 additions & 0 deletions test-case/src/main/java/pkg/UsingSignOnly.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,5 +7,9 @@ public class UsingSignOnly {
private final Logger logger = LoggerFactory.getLogger(getClass());
void method() {
logger.info("{}, {}", "Hello", "world");
log("{}", "Hello");
}
private void log(String format, Object data) {
logger.debug(format, data);
}
}
2 changes: 1 addition & 1 deletion test-case/src/test/java/jp/skypencil/findbugs/slf4j/UsingSignOnlyTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
public class UsingSignOnlyTest {
@Test
public void test() {
Map<String, Integer> expected = Collections.singletonMap("SLF4J_SIGN_ONLY_FORMAT", 1);
Map<String, Integer> expected = Collections.singletonMap("SLF4J_SIGN_ONLY_FORMAT", 2);
new XmlParser().expect(pkg.UsingSignOnly.class, expected);
}
}

0 comments on commit c9099aa

Please sign in to comment.