Skip to content

Commit

Permalink
Setting endPos correctly when deleting the statement
Browse files Browse the repository at this point in the history 
Further refactored the code

Delete the accidentally added DS store files

switching back to macos 11

tryinh macos-latest again

reverting the other changes too

switching back to macos 11

fixing the workflow

fixing the build warning

trying to resolve build error

Made the matcher nicer and added more corner cases

Addressed the issues mentioned in the PR and offline

Added support for then* pattern

Added delete statement logic for specific patterns menntioned in issue uber#104
  • Loading branch information
Ameya Ketkar committed Dec 2, 2021
1 parent 4d2330b commit 079f33d
Show file tree
Hide file tree
Showing 4 changed files with 131 additions and 20 deletions.
4 changes: 2 additions & 2 deletions .github/workflows/swift.yml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,14 @@ jobs:
runs-on: macos-11
strategy:
matrix:
xcode: ['13.0']
xcode: ['13.1']
name: "PiranhaSwift using ${{ matrix.xcode }} on ${{ matrix.os }}"
steps:
- name: Check out Piranha sources
uses: actions/checkout@v2
- name: Set Xcode ${{ matrix.xcode }}
run: |
sudo xcode-select -s /Applications/Xcode_13.0_beta.app
sudo xcode-select -s /Applications/Xcode_13.1.app
xcodebuild -version
swift --version
swift package --version
Expand Down
1 change: 1 addition & 0 deletions java/piranha/build.gradle
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ dependencies {

testCompile deps.test.junit4
testCompile deps.test.daggerAnnotations
testImplementation group: 'org.mockito', name: 'mockito-core', version: '2.1.0'
testCompile(deps.build.errorProneTestHelpers) {
exclude group: "junit", module: "junit"
}
Expand Down
63 changes: 45 additions & 18 deletions java/piranha/src/main/java/com/uber/piranha/XPFlagCleaner.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,13 @@
package com.uber.piranha;

import static com.google.errorprone.BugPattern.SeverityLevel.SUGGESTION;
import static com.google.errorprone.matchers.ChildMultiMatcher.MatchType.ALL;
import static com.google.errorprone.matchers.Matchers.allOf;
import static com.google.errorprone.matchers.Matchers.instanceMethod;
import static com.google.errorprone.matchers.Matchers.methodInvocation;
import static com.google.errorprone.matchers.Matchers.receiverOfInvocation;
import static com.google.errorprone.matchers.Matchers.staticMethod;
import static com.google.errorprone.matchers.Matchers.toType;

import com.facebook.infer.annotation.Initializer;
import com.google.auto.service.AutoService;
Expand All @@ -27,6 +34,7 @@
import com.google.errorprone.bugpatterns.BugChecker;
import com.google.errorprone.fixes.SuggestedFix;
import com.google.errorprone.matchers.Description;
import com.google.errorprone.matchers.Matcher;
import com.google.errorprone.util.ASTHelpers;
import com.google.errorprone.util.FindIdentifiers;
import com.sun.source.tree.AnnotationTree;
Expand Down Expand Up @@ -71,6 +79,7 @@
import java.util.Objects;
import java.util.Optional;
import java.util.Set;
import java.util.regex.Pattern;
import java.util.stream.Collectors;
import java.util.stream.Stream;
import javax.annotation.Nullable;
Expand Down Expand Up @@ -683,29 +692,47 @@ private boolean evalTreatmentGroupCheck(MethodInvocationTree methodInvocationTre
*/
private Description updateCode(
Value v, ExpressionTree tree, ExpressionTree expr, VisitorState state) {
boolean update = false;
String replacementString = "";

if (v.equals(Value.TRUE)) {
update = true;
replacementString = TRUE;
} else if (v.equals(Value.FALSE)) {
update = true;
replacementString = FALSE;
}

if (update) {
Description.Builder builder = buildDescription(tree);
SuggestedFix.Builder fixBuilder = SuggestedFix.builder();
fixBuilder.replace(expr, replacementString);
if (v.equals(Value.TRUE) || v.equals(Value.FALSE)) {
SuggestedFix.Builder fixBuilder = handleSpecificAPIPatterns(state);
if (fixBuilder.isEmpty()) {
String replacementString = v.equals(Value.TRUE) ? TRUE : FALSE;
fixBuilder = SuggestedFix.builder().replace(expr, replacementString);
endPos = state.getEndPosition(expr);
}
decrementAllSymbolUsages(expr, state, fixBuilder);
builder.addFix(fixBuilder.build());
endPos = state.getEndPosition(expr);
return builder.build();
return buildDescription(tree).addFix(fixBuilder.build()).build();
}
return Description.NO_MATCH;
}

private static final String MOCKITO_QN = "org.mockito.Mockito";
private static final String MOCKITO_WHEN = "when";
private static final Pattern MOCKITO_THEN = Pattern.compile("then\\W*\\w*");

private final Matcher<Tree> MOCKITO_UNNECESSARY_MOCKING_PATTERN =
toType(
MethodInvocationTree.class,
allOf(
instanceMethod().anyClass().withNameMatching(MOCKITO_THEN),
receiverOfInvocation(
methodInvocation(
staticMethod().onClass(MOCKITO_QN).named(MOCKITO_WHEN),
ALL,
(argument, vs) -> {
Value value = evalExpr(argument, vs);
return value == Value.TRUE || value == Value.FALSE;
}))));

private SuggestedFix.Builder handleSpecificAPIPatterns(VisitorState state) {
ExpressionStatementTree stmt =
ASTHelpers.findEnclosingNode(state.getPath(), ExpressionStatementTree.class);
if (stmt != null && MOCKITO_UNNECESSARY_MOCKING_PATTERN.matches(stmt.getExpression(), state)) {
endPos = state.getEndPosition(stmt);
return SuggestedFix.builder().delete(stmt);
}
return SuggestedFix.builder();
}

private boolean isTreatmentGroupEnum(Symbol.ClassSymbol enumSym) {
// Filter out some generic names, like CONTROL to make sure we don't match the wrong
// TreatmentGroup
Expand Down
83 changes: 83 additions & 0 deletions java/piranha/src/test/java/com/uber/piranha/CorePiranhaTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -499,4 +499,87 @@ public void testIgnoresPrefixMatchFlag() throws IOException {
"class TestClassPartialMatch { }")
.doTest();
}

@Test
public void testRemoveSpecificAPIpatterns() throws IOException {

ErrorProneFlags.Builder b = ErrorProneFlags.builder();
b.putFlag("Piranha:FlagName", "STALE_FLAG");
b.putFlag("Piranha:IsTreated", "false");
b.putFlag("Piranha:Config", "config/properties.json");

BugCheckerRefactoringTestHelper bcr =
BugCheckerRefactoringTestHelper.newInstance(new XPFlagCleaner(b.build()), getClass());

bcr = bcr.setArgs("-d", temporaryFolder.getRoot().getAbsolutePath());
bcr = PiranhaTestingHelpers.addHelperClasses(bcr);
bcr.addInputLines(
"XPFlagCleanerSinglePositiveCase.java",
"package com.uber.piranha;",
"import static org.mockito.Mockito.when;",
"import org.mockito.invocation.InvocationOnMock;",
"import org.mockito.stubbing.Answer;",
"import org.mockito.stubbing.OngoingStubbing;",
"class MockitoTest {",
" private XPTest experimentation;",
" public void evaluate() {",
" Answer ans = new Answer<Integer>() {\n"
+ " public Integer answer(InvocationOnMock invocation) throws Throwable {\n"
+ " return (Integer) invocation.getArguments()[0];\n"
+ " }};",
" when(experimentation.isToggleDisabled(\"STALE_FLAG\")).thenReturn(false);",
" when(experimentation.isToggleEnabled(\"STALE_FLAG\")).thenThrow(new RuntimeException());",
" when(experimentation.isToggleDisabled(\"STALE_FLAG\")).thenCallRealMethod();",
" when(experimentation.isToggleEnabled(\"STALE_FLAG\")).then(ans);",
" boolean b1 = someWrapper(when(experimentation.isToggleDisabled(\"STALE_FLAG\")).thenCallRealMethod());",
" boolean b2 = someWrapper(when(experimentation.isToggleDisabled(\"OTHER_FLAG\")).thenCallRealMethod());",
" someWhenWrapper(when(experimentation.isToggleDisabled(\"STALE_FLAG\"))).thenCallRealMethod();",
" someWhenWrapper(when(experimentation.isToggleDisabled(\"OTHER_FLAG\"))).thenCallRealMethod();",
" when(experimentation.isToggleEnabled(\"OTHER_FLAG\")).thenReturn(false);",
" when(experimentation.isToggleEnabled(\"OTHER_FLAG\")).thenThrow(new RuntimeException());",
" when(experimentation.isToggleDisabled(\"OTHER_FLAG\")).thenCallRealMethod();",
" when(experimentation.isToggleEnabled(\"OTHER_FLAG\")).then(ans);",
" when(foobar()).thenReturn(false);",
" when(foobar()).thenAnswer(ans);",
" }",
" public boolean foobar() { return true;}",
" public boolean someWrapper(Object obj) { return true;}",
" public OngoingStubbing<Boolean> someWhenWrapper(OngoingStubbing<Boolean> x) { return x;}",
"}")
.addOutputLines(
"XPFlagCleanerSinglePositiveCase.java",
"package com.uber.piranha;",
"import static org.mockito.Mockito.when;",
"import org.mockito.invocation.InvocationOnMock;",
"import org.mockito.stubbing.Answer;",
"import org.mockito.stubbing.OngoingStubbing;",
"class MockitoTest {",
" private XPTest experimentation;",
" public void evaluate() {",
" Answer ans = new Answer<Integer>() {\n"
+ " public Integer answer(InvocationOnMock invocation) throws Throwable {\n"
+ " return (Integer) invocation.getArguments()[0];\n"
+ " }};",
"",
"",
"",
"",
" boolean b1 = someWrapper(when(true).thenCallRealMethod());",
" boolean b2 = someWrapper(when(experimentation.isToggleDisabled(\"OTHER_FLAG\")).thenCallRealMethod());",
" someWhenWrapper(when(true)).thenCallRealMethod();",
" someWhenWrapper(when(experimentation.isToggleDisabled(\"OTHER_FLAG\"))).thenCallRealMethod();",
" when(experimentation.isToggleEnabled(\"OTHER_FLAG\")).thenReturn(false);",
" when(experimentation.isToggleEnabled(\"OTHER_FLAG\")).thenThrow(new RuntimeException());",
" when(experimentation.isToggleDisabled(\"OTHER_FLAG\")).thenCallRealMethod();",
" when(experimentation.isToggleEnabled(\"OTHER_FLAG\")).then(ans);",
" when(foobar()).thenReturn(false);",
" when(foobar()).thenAnswer(ans);",
" }",
" public boolean foobar() { return true;}",
" public boolean someWrapper(Object obj) { return true;}",
" public OngoingStubbing<Boolean> someWhenWrapper(OngoingStubbing<Boolean> x) { return x;}",
"}")
.doTest();
// OngoingStubbing<Boolean> w = when(true).the;
}
}

0 comments on commit 079f33d

Please sign in to comment.