Adding feature for removing unnecessary lambda expression block . #167
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -513,6 +513,92 @@ public void testIgnoresPrefixMatchFlag() throws IOException { | |
| .doTest(); | ||
| } | ||
|
|
||
| // Tests for cleanup of unnecessary blocks in the body of a lambda expression. | ||
| @Test | ||
| public void testRemoveUnnecessaryBlockLambda() 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( | ||
| "TestRemoveBlocks.java", | ||
| "package com.uber.piranha;", | ||
| "import java.util.function.Consumer;", | ||
| "import java.util.function.Supplier;", | ||
| "class TestRemoveBlocks {", | ||
| " private XPTest experimentation;", | ||
| " public void testMethod(){", | ||
| " Consumer<String> someConsumer = (s) -> {", | ||
| " if(experimentation.isToggleDisabled(\"STALE_FLAG\")){", | ||
| " System.out.println(s);", | ||
| " } ", | ||
| " else {", | ||
|
There was a problem hiding this comment. Probably doesn't matter for a test, but this is a weird way to format the code vs |
||
| " System.out.println(s + \"123\");", | ||
| " }", | ||
| " };", | ||
| " Supplier<String> someSupplier = () -> {", | ||
| " if(experimentation.isToggleDisabled(\"STALE_FLAG\")){", | ||
| " return \"ABC\";", | ||
| " } ", | ||
| " else {", | ||
| " return \"123\";", | ||
| " }", | ||
| " };", | ||
| " Supplier<String> someSupplier1 = () -> {", | ||
| " if(experimentation.isToggleDisabled(\"STALE_FLAG\")){", | ||
| " System.out.println(\"ABC\");", | ||
| " return \"ABC\";", | ||
| " } ", | ||
| " else {", | ||
| " System.out.println(\"123\");", | ||
| " return \"123\";", | ||
| " }", | ||
| " };", | ||
| " Consumer<String> someConsumer1 = (s) -> {", | ||
| " if(experimentation.isToggleDisabled(\"STALE_FLAG\")){", | ||
| " System.out.println(s);", | ||
| " } ", | ||
| " else {", | ||
| " System.out.println(s + \"123\");", | ||
| " }", | ||
| " System.out.println(\"No match\");", | ||
| " };", | ||
| " }", | ||
| "}") | ||
| .addOutputLines( | ||
| "TestRemoveBlocks.java", | ||
| "package com.uber.piranha;", | ||
| "import java.util.function.Consumer;", | ||
| "import java.util.function.Supplier;", | ||
| "class TestRemoveBlocks {", | ||
| " private XPTest experimentation;", | ||
| " public void testMethod(){", | ||
| " Consumer<String> someConsumer = (s) -> {", | ||
| " System.out.println(s);", | ||
| " };", | ||
|
There was a problem hiding this comment. I don't understand. Aren't these blocks exactly what you are trying to remove? Shouldn't the rewrite here be: ? |
||
| " Supplier<String> someSupplier = () -> {", | ||
| " return \"ABC\";", | ||
| " };", | ||
| " Supplier<String> someSupplier1 = () -> {", | ||
| " System.out.println(\"ABC\");", | ||
| " return \"ABC\";", | ||
| " };", | ||
| " Consumer<String> someConsumer1 = (s) -> {", | ||
| " System.out.println(s);", | ||
| " System.out.println(\"No match\");", | ||
| " };", | ||
| " }", | ||
| "}") | ||
| .doTest(); | ||
| } | ||
|
|
||
| @Test | ||
| public void testRemoveSpecificAPIpatternsMockito() throws IOException { | ||
|
|
||
|
|
@@ -954,137 +1040,6 @@ public void testRemoveSpecificAPIPatternsInstanceMethod() throws IOException { | |
| .doTest(); | ||
| } | ||
|
|
||
|
There was a problem hiding this comment. In my previous PR, I had split this test case into smaller test case. But I had not deleted this test case. |
||
| @Test | ||
| public void testMethodChainTreated() { | ||
| String staleFlag = "stale_flag"; | ||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is matching on every lambda expression of the original AST, in parallel with the traversal you are using to rewrite XP flag using code. This will result in two issues:
a) This will not actually simplify expressions after Piranha removes the code related to stale flags, since that happens later in the traversal (deeper inside the AST) than this matcher. And also since Piranha doesn't rewrite the AST in place (for good reasons, given the EP model) but rather generates a fix suggestion to be applied after compilation and analysis by the checker.
b) This will switch all single-statement lambdas present in the original code to use the expression body style vs the block body style. This is not something we want Piranha to be doing. In general, this is not the tool to do code changes that are entirely unrelated to removing stale flags.
I am not sure that the refactoring you want to implement can be done easily. You would either need to match the "lambda containing an if" pattern here and duplicate/refactor much of the
matchIflogic, or have secondary traversal over the already rewritten code, but this is hard to do inside the Piranha checker itself and would require some integration work to run two rewriters (it would also need some logic to detect that the code being "simplified" was already touched by Piranha and is not an unrelated lambda expression).This is an instance of the "deep clean" issue with Piranha Java.