Adding feature for removing unnecessary lambda expression block . #167
Conversation
ketkarameya
changed the title
| @@ -906,137 +992,6 @@ public void testRemoveSpecificAPIpatternsJUnit() throws IOException { | |||
| .doTest(); | |||
| } | |||
|
|
|||
| @Test | |||
| public void testRemoveSpecificAPIpatterns() throws IOException { | |||
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.
There was a problem hiding this comment.
Unfortunately, this is not easy to do, and this PR is incorrect at the moment (see the refactoring you are testing for in the test case, it's not actually doing anything new). Comments below as to why this approach won't directly work.
p.s. separately, this one is your call now rather than mine, but prefixing Piranha Java PRs with [PiranhaJava] will make them much easier to track in the changelog, particularly when - unlike now - other versions of the tool (for other languages) are seeing more active development.
| " 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 } else {. Similar in the cases below.
| " 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:
Consumer<String> someConsumer = s -> System.out.println(s);
Supplier<String> someSupplier = () -> \"ABC\";
[...]
?
| // This scenario is possible in case of single return statements and expression statements | ||
| // in the lambda body after Piranha removes the code related to stale flags. | ||
| @Override | ||
| public Description matchLambdaExpression(LambdaExpressionTree tree, VisitorState visitorState) { |
There was a problem hiding this comment.
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 matchIf logic, 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.
| @Override | ||
| public Description matchLambdaExpression(LambdaExpressionTree tree, VisitorState visitorState) { | ||
| Tree lambdaBody = tree.getBody(); | ||
| if (lambdaBody instanceof BlockTree) { |
There was a problem hiding this comment.
There are more fundamental problems (see above), but you want LambdaExpressionTree.getBodyKind()
| } | ||
| if (expr != null) { | ||
| return buildDescription(tree) | ||
| .addFix(SuggestedFix.replace(bt, visitorState.getSourceForNode(expr))) |
There was a problem hiding this comment.
Might be safer to build a new lambda expression with expr as the body and replace tree (but again, this is moot, since this won't do the rewrite you want).
This PR adds a deep clean feature that removes unnecessary block statements in lambda expression bodies after the removal of code related to XP flags. Issue #166.