Introduce `FluxImplicitBlock` check #472

benhalasi · 2023-01-20T13:12:38Z

fixes #468

Flags implicitly blocking operators on Reactor's Flux as fragile code.

Flux#toStream

Suggested alternatives for flux.toStream():

SuppressWarnings
if guava is available, replace with: flux.collect(toImmutableList()).block().stream();
replace with: flux.collect(toUnmodifiableList()).block().stream();

Flux#toIterable

Suggested alternatives for flux.toIterable():

SuppressWarnings
if guava is available, replace with: flux.collect(toImmutableList()).block();
replace with: flux.collect(toUnmodifiableList()).block();

github-actions · 2023-01-20T13:14:25Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

github-actions · 2023-01-20T13:28:48Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

github-actions · 2023-01-20T14:03:55Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

Stephan202 · 2023-01-20T14:28:48Z

Thanks for filing this PR @benhalasi! The linked issue does suggest rewriting these methods, but also references FluxFlatMapUsage. I wonder whether we should go that same route here, and introduce a BugChecker rather than Refaster rules. My thinking is that, while the current approach is pragmatic and does cause toStream() and toIterable() to be flagged, the alternatives are considerably less efficient, and may even cause an OutOfMemoryError in case of very large or unbounded reactive streams.

The BugChecker could suggest several fixes, accompanied with a warning an OutOfMemoryError warning:

Adding @SuppressWarnings (like in IdentityConversion).
Adding .collect(toImmutableList()), if Guava is available.
Adding .collect(toList()).

(Given our coding style, Picnic users would likely select option (1) or (2). If I'd find this in code I was personally responsible for, I'd likely add @SuppressWarnings and file a cleanup ticket 🤔.)

CC @Ernir for input as the original author of the issue.

benhalasi · 2023-01-20T15:35:05Z

I see how I didn't get it right for the first time, will try to work it out and come back with something until Monday.

rickie · 2023-01-20T16:51:22Z

No problem @benhalasi, it appears to be a bit more challenging than anticipated. In retrospect we maybe should've worked out this issue a bit further beforehand.

If you have any questions, let us know 😄. We're happy to help.

Stephan202 · 2023-01-20T18:20:37Z

I see how I didn't get it right for the first time, will try to work it out and come back with something until Monday.

No rush! 💪

github-actions · 2023-01-23T09:09:57Z

Surviving mutants in this change: 1
Killed mutants in this change: 19

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.ImplicitBlockingFluxOperation`	1	19

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

github-actions · 2023-01-23T09:42:36Z

Surviving mutants in this change: 1
Killed mutants in this change: 19

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.ImplicitBlockingFluxOperation`	1	19

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

rickie

Hey @benhalasi thanks for changing the implementation to this check! Will do a more thorough review probably tomorrow. Already left some comments to explain why the CI fails currently 😄.

...-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ImplicitBlockingFluxOperation.java

...trib/src/test/java/tech/picnic/errorprone/bugpatterns/ImplicitBlockingFluxOperationTest.java

...-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ImplicitBlockingFluxOperation.java

benhalasi · 2023-01-23T11:03:44Z

Thank you for the fast feedback.

Also here's the comment I forgot to attach:

I tried to stick to the coding style I saw in other files as much as possible, but I'm sure there's a lot of room to improve. I'm the least confident in the comments/javadocs and the tests.

I'm looking forward to any suggestion or change request, and also to answers to some questions:

Code duplication between checkers

I copied matchers from e.g. FluxFlatMapUsage, is there a pattern to have these potentially shared matchers collected or the checkers considered to be fully independent from each-other and we don't care about these kinds of duplication?

How to replace a method call with a method call chain.

In the getCollectAndBlockFix method I want to replace the matched method call with something like flux.collect(..).block()....

For this, originally, I wanted to use SuggestedFix.replace(). This lost the flux symbol that we had the method calls on.

Then I tried to use one of the SuggestedFixes.renameMethod*() methods, but as I saw they expect a method name instead of a method call chain, they put a () after the replacement - it would be possible to use these methods, but it seems hacky.

As I saw the best solution is to have a 'replacer' that is similar to the existing rename methods, but instead of a method name it expects (potentially chained) method calls.

I didn't find such method and I wasn't sure enough about this to implement it without any feedback, so I went with a temporary - hacky - solution where I use the normal replace and prefix it with the part we would have lost.

github-actions · 2023-01-23T11:07:26Z

Surviving mutants in this change: 1
Killed mutants in this change: 19

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.ImplicitBlockingFluxOperation`	1	19

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

github-actions · 2023-01-23T11:15:16Z

Surviving mutants in this change: 1
Killed mutants in this change: 19

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.ImplicitBlockingFluxOperation`	1	19

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

github-actions · 2023-01-23T11:48:08Z

Surviving mutants in this change: 1
Killed mutants in this change: 19

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.ImplicitBlockingFluxOperation`	1	19

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

github-actions · 2023-01-23T12:01:29Z

Surviving mutants in this change: 1
Killed mutants in this change: 19

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.ImplicitBlockingFluxOperation`	1	19

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

benhalasi · 2023-01-23T12:10:07Z

I committed the changes @rickie suggested.

I see some checks are still failing, I'm not able to reproduce them locally nor I want to spam further this PR so I leave it here until further feedback. The gust of the PR is already done I think.

rickie

I copied matchers from e.g. FluxFlatMapUsage, is there a pattern to have these potentially shared matchers collected or the checkers considered to be fully independent from each-other and we don't care about these kinds of duplication?

Good question. If some Matchers are used multiple times, we can consider adding them to MoreMatchers.java. Maybe we should have a Reactor specific matchers file in error-prone-contribs tech.picnic.errorprone.bugpatterns.util package 🤔.

All to say, yes we care about this kind of duplication and we want to avoid it 😄.

How to replace a method call with a method call chain.
In the getCollectAndBlockFix method I want to replace the matched method call with something like flux.collect(..).block()....

I think the CollectorMutability check has some nice similarities that might be interesting. Based on that check I think you are on the the right path, because here you can see an example that does the same: creating a string and replacing that completely.

Didn't dive too deep but it seems like the approach is right.

Oh and btw, the Pitest plugin is complaining because a test is missing. We need almost the same test as the identificationWithoutGuavaOnClasspath that is in CollectorMutabilityTest.java. There needs to be a test that verifies that it doesn't perform the Guava fix if the classpath is empty, that is the reason there is a withClasspath() method in the test I mention here. If you don't get around to adding that test, I'll do it when I do a proper review :).

Ahh there is now a failing test because of this line:

String flux = source.substring(0, source.indexOf("."));

I think this is because I changed flux(). with Flux.just(1) in the tests and is currently a limitation of the implementation. This is indeed where you asked specifically for help. I think there might be even a better option that we can use. See an example here in the StringCaseLocaleUsage check. It is also possible to specify specific positions where the fix should be applied. It might be smart to go this route.

A lot of things to consider. Feel free to play with the options 😄. Thanks for the good questions and I hope my answers make some things clear. I'll soon dive into this again myself. However, for today I need to focus on some other stuff. If you have questions, let us know!

I added a commit and applied some of the suggestions :).

...-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ImplicitBlockingFluxOperation.java

rickie · 2023-01-23T12:24:34Z

...-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ImplicitBlockingFluxOperation.java

+public class ImplicitBlockingFluxOperation extends BugChecker
+    implements MethodInvocationTreeMatcher {
+  private static final long serialVersionUID = 1L;
+  private static final Matcher<ExpressionTree> FLUX_TO_ITERABLE =


Will push a change for this one. Now the check I have at the bottom of the class is not so nice, will come back to that. Maybe indeed we need this setup but utilizing the #namedAnyOf makes a bit more sense to me. Maybe there is a easier way of checking whether we are making a SuggestedFix for the toIterable variant or not.

...-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ImplicitBlockingFluxOperation.java

rickie · 2023-01-23T12:25:51Z

...-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ImplicitBlockingFluxOperation.java

+   * merges `flux.collect(...).block()...` fix into given fix with specified collector and postfix
+   * to match the original expression tree.
+   *
+   * @param collector expression


Note to self: this Javadoc is not really corresponding with the actual parameters 😉. Also, we add a . after every sentence.

...-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ImplicitBlockingFluxOperation.java

...trib/src/test/java/tech/picnic/errorprone/bugpatterns/ImplicitBlockingFluxOperationTest.java

rickie · 2023-01-23T12:28:35Z

...trib/src/test/java/tech/picnic/errorprone/bugpatterns/ImplicitBlockingFluxOperationTest.java

+            "",
+            "class A {",
+            "  void m() {",
+            "    // BUG: Diagnostic contains:",


We can simplify this testing setup like this:

Suggested change

" // BUG: Diagnostic contains:",

" // BUG: Diagnostic contains:",

"class A {",

" void m() {",

" Flux.just(1).toIterable();",

" Flux.just(2).toStream();",

" }")

Additionally it would be nice to add some other test cases where we use toIterable and toStream on other objects then Flux to show it doesn't flag those.

benhalasi · 2023-01-25T08:20:28Z

The answers and suggestions were very helpful, ASTHelpers and ErrorProneTokens are awesome.

I did a fairly large rewrite and I got some additional questions too :D, mostly about assumptions I made, whether they are valid. I marked these with XXX comments too.

`replaceMethodInvocationWithCollect` method

Main concerns:

assumes that the replacement expression will evaluate to a Mono<Collection<?>> - which is valid for the current cases, but could be misleading later, when extending this rule.
would return emptyFix if we a match an expression that evaluates to a type we don't handle.
- if this is the way to go, how to test this behaviour?

`replaceMethodInvocation` method

It replaces method(args) in a matched Clazz.optionally().chained().method(args) expression.

First of all I'm not even sure if this is the correct name for it or not.

in CollectorMutability there's method named the same that would just replace the whole match if I'm not mistaken.
the SuggestedFix.replace(tree.getMethodSelect(), replacement) would replace Clazz.optionally().chained().method without the brackets or args.

I'm guessing the reason for this is that someone could just replace the method and then handle its args separately?

Has some assumptions that I'm not sure if it can have. If not, whether it should throw an error / warning or maybe should just return an Optional. Currently it will fail when:

ErrorProneTokens.getTokens(...) returns empty list
ASTHelpers.getStartPosition(tree) returns Position.NOPOS, which should only happen if tree is null (I think)
token list doesn't contain the matched method.
token list contains elements after the matched method invocation and its parenthesizes.

other 'helper' methods

I would guess the functionality of isTokenTheInvokedMethod and isClassValidSubstituteFor methods are already implemented and I just didn't find them, so these are naive implementations, may need some extra work if we go with them.

tests

I added test for identification without Guava on the classpath.

Additionally it would be nice to add some other test cases where we use toIterable and toStream on other objects then Flux to show it doesn't flag those.

Didn't find any other class with toStream or toIterable methods, should I just create an empty class for this, if so, where should I implement it?

move flux matchers to `MoreMatchers`

I didn't work on this yet, it seems like a separate issue from this one. Am I wrong and should include it here?

github-actions · 2023-01-25T08:20:51Z

Surviving mutants in this change: 5
Killed mutants in this change: 39

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.ImplicitBlockingFluxOperation`	5	39

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

github-actions · 2023-01-25T09:15:04Z

Surviving mutants in this change: 5
Killed mutants in this change: 39

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.ImplicitBlockingFluxOperation`	5	39

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

rickie

Thanks for applying the feedback @benhalasi . I see some really nice progress and a nice thought process 🚀!

I did a review but didn't yet get to the end. We are on the right path here!

I addressed most of your questions in my comments. However as my review didn't get till the end I wasn't able to apply to all questions yet.

Added a few commits 😄.

Let me answer most of your other questions:

assumes that the replacement expression will evaluate to a Mono<Collection<?>> - which is valid for the current cases, but could be misleading later, when extending this rule.

Good consideration; however, I think this is fine. In the case of toStream() the resulting type will be exactly the same right, as we end with .stream().

if this is the way to go, how to test this behaviour?

No we generally use Optionals for this if this is really necessary. However, in this exact case it probably wasn't necessary because we use Matchers to match quite specific expressions.

It replaces method(args) in a matched Clazz.optionally().chained().method(args) expression.

Yes this sounds good, it should only change the part you mention. I didn't thoroughly check this part of the logic yet so exact review TBD 😉.

ASTHelpers.getStartPosition(tree) returns Position.NOPOS

We should probably handle that case 😄.

I would guess the functionality of isTokenTheInvokedMethod and isClassValidSubstituteFor methods are already implemented and I just didn't find them, so these are naive implementations, may need some extra work if we go with them.

You are correct, worked on this and will push a suggestion!

Didn't find any other class with toStream or toIterable methods, should I just create an empty class for this, if so, where should I implement it?

Ah indeed, it was harder than expected. At the very least we should indeed have a simple class with a method .toStream() and .toIterable()` to show it does nothing with those methods.

I didn't work on this yet, it seems like a separate issue from this one. Am I wrong and should include it here?

Hmm yeah that's true. If we move it we would also need to add tests to the respective class. We can defer it.

rickie · 2023-01-25T09:13:58Z

...-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ImplicitBlockingFluxOperation.java

+import static com.google.errorprone.BugPattern.SeverityLevel.SUGGESTION;
+import static com.google.errorprone.BugPattern.StandardTags.STYLE;
+import static com.google.errorprone.matchers.method.MethodMatchers.instanceMethod;
+import static com.google.errorprone.util.ASTHelpers.*;


When running mvn clean install, we can see that Error Prone flags star imports. See relevant documentation here.

...-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ImplicitBlockingFluxOperation.java

rickie · 2023-01-25T09:22:39Z

...trib/src/test/java/tech/picnic/errorprone/bugpatterns/ImplicitBlockingFluxOperationTest.java

+            "    Flux.just(2).toStream();",
+            "  }",
+            "}")
+        .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH);


Lets write them like this:

Suggested change

.doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH);

.doTest(TestMode.TEXT_MATCH);

Made an extra comment about this on an open ticket (I thought it was already there 😬), see here.

rickie · 2023-01-25T09:24:46Z

...trib/src/test/java/tech/picnic/errorprone/bugpatterns/ImplicitBlockingFluxOperationTest.java

+
+  @Test
+  void identificationWithoutGuavaOnClasspath() {
+    CompilationTestHelper.newInstance(ImplicitBlockingFluxOperation.class, getClass())


Hmm, now that I'm seeing this I found out that it is actually a bit harder than expected.

Now we see that code is actually flagged by this check which could mean that the Guava fix is being applied. The CompilationTestHelper test doesn't actually show us which fix is applied... We need to verify that in a specific case a specific SuggestedFix is actually not applied.

This probably needs to be tested similar to how we test our util classes. See e.g. MoreJUnitMatchersTest.java. This is a little more involved. Let me think if there is a more easy way to test this 🤔.

...trib/src/test/java/tech/picnic/errorprone/bugpatterns/ImplicitBlockingFluxOperationTest.java

...-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ImplicitBlockingFluxOperation.java

rickie · 2023-01-25T12:24:01Z

...-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ImplicitBlockingFluxOperation.java

+        && Objects.equals(token.name().toString(), getSymbol(tree).getQualifiedName().toString());
+  }
+
+  // XXX: Replace with prewritten solution. (?)


So I addressed this XXX together with the next one. Indeed there is a solution provided out-of-the-box that we can use instead of these two methods. Error Prone doesn't have that good of a documentation that easily helps us identify such improvements. We can rely on the Types#isSubType here, which allows us to do pretty cool stuff.

Will push a suggestion please take a look at it 😄. Let me know if you have any questions. I combined it with a usage of our own beautiful mini "DSL", see MoreTypes.java. That allows us to greatly simplify the logic used here.

This might not be easy to get at first, so if you have any questions about it, let us know :).

rickie · 2023-01-25T12:26:07Z

...-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ImplicitBlockingFluxOperation.java

+        ErrorProneTokens.getTokens(SourceCode.treeToString(tree, state), state.context);
+
+    int treeStartPosition = getStartPosition(tree);
+    int methodInvocationStartPosition =


Nice improvements done here!

I think there might be a solution where we don't need to use the ErrorProneTokens. Usually we only go that route as "last resort" or if comments are involved. Now that I think of that, that could also be the case here. Maybe we should add test cases for that. See the StringCaseLocaleUsage test cases for some examples. However, I'm not sure yet if that is the correct way to go about this either. I need to take some more time to dive into this before being able to give a good advice. For now I need to focus on some other things so will stop here :).

github-actions · 2023-01-25T12:39:50Z

Surviving mutants in this change: 1
Killed mutants in this change: 26

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.ImplicitBlockingFluxOperation`	1	26

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

github-actions · 2023-01-25T13:44:19Z

Surviving mutants in this change: 1
Killed mutants in this change: 26

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.ImplicitBlockingFluxOperation`	1	26

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

rickie · 2023-01-25T14:26:36Z

Marked as ready for review because that's what we are doing 😉.

Co-authored-by: Rick Ossendrijver <[email protected]>

…gh `BugCheckerRefactoringTestHelper` Co-authored-by: Rick Ossendrijver <[email protected]>

matcher: - improve temp solution to handle cases when the matched methodSelect has multiple '.' chars in it. Like `Flux.just(...).toStream()` tests: - Add test `identificationWithoutGuavaOnClasspath` - Apply code-style suggestions/conventions - Can we get the type of the matched expression

- implement test for identification without guava on classpath

…hods with it

…der is enforced by other tests.

github-actions · 2023-02-20T09:01:30Z

Surviving mutants in this change: 1
Killed mutants in this change: 17

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.FluxImplicitBlock`	1	17

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

Stephan202 mentioned this pull request Jan 20, 2023

Disallow implicit blocking operators of Project Reactor's Flux #468

Closed

4 tasks

benhalasi marked this pull request as draft January 20, 2023 14:47

benhalasi changed the title ~~Introduce Flux.to{Stream,Iterable} Refaster rules~~ Introduce ImplicitBlockingFluxOperation check Jan 23, 2023

rickie requested changes Jan 23, 2023

View reviewed changes

rickie force-pushed the benhalasi/introduce-avoid-accidental-blocking-rules branch from e51b172 to b46a6e9 Compare January 25, 2023 09:12

rickie requested changes Jan 25, 2023

View reviewed changes

rickie marked this pull request as ready for review January 25, 2023 14:26

benhalasi force-pushed the benhalasi/introduce-avoid-accidental-blocking-rules branch from b0eefdb to c8f1c5b Compare January 29, 2023 13:53

benhalasi and others added 26 commits February 20, 2023 09:58

checkstyle: new-line at the start of a block

030f091

Co-authored-by: Rick Ossendrijver <[email protected]>

convention: refering BugCheckerRefactoringTestHelper.TestMode throu…

14165d6

…gh `BugCheckerRefactoringTestHelper` Co-authored-by: Rick Ossendrijver <[email protected]>

Suggestions

bfcc395

Resolve MethodMatchers.MethodClassMatcher deprecation

b160ab3

Suggestions 2

408a968

Refactor implementation for readability and resiliency

0343892

- implement test for identification without guava on classpath

Suggestions

c244042

Drop semi-duplicate method and inline a method

7fc2527

Use Types#isSubType in combination with MoreTypes DSL

ac931e4

Discussed this with @Stephan202 :)

2f1e6da

resolve replaceMethodInvocation method's assumptions with Optional

e97dde7

implement helper class and verify that matcher only picks up Flux met…

54aa877

…hods with it

javadoc for default constructor

8ff5b12

kill NULL_RETURNS mutant

cf15383

making tests fail when guava is introduced despite that's not allowed

22fb2dd

formatting

0273b3f

Suggestions

0ae778b

Add missing imports

3e07247

Suggestions and rename

b242c78

Suggestions

c90c588

Naming tweaks

4a3eb93

Suggestions

c8afb35

More accurate phrasing

db3be54

Allow suggestion order changes in identification test, since the or…

4de090e

…der is enforced by other tests.

s/ImplicitBlockingFlux/FluxImplicitBlock/

56067cb

rickie force-pushed the benhalasi/introduce-avoid-accidental-blocking-rules branch from a60c4d1 to 56067cb Compare February 20, 2023 08:58

rickie changed the title ~~Introduce ImplicitBlockingFlux check~~ Introduce FluxImplicitBlock check Feb 20, 2023

rickie merged commit 82d4677 into PicnicSupermarket:master Feb 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce `FluxImplicitBlock` check #472

Introduce `FluxImplicitBlock` check #472

benhalasi commented Jan 20, 2023 •

edited

Loading

github-actions bot commented Jan 20, 2023

github-actions bot commented Jan 20, 2023

github-actions bot commented Jan 20, 2023

Stephan202 commented Jan 20, 2023

benhalasi commented Jan 20, 2023

rickie commented Jan 20, 2023

Stephan202 commented Jan 20, 2023

github-actions bot commented Jan 23, 2023

github-actions bot commented Jan 23, 2023

rickie left a comment

benhalasi commented Jan 23, 2023

github-actions bot commented Jan 23, 2023

github-actions bot commented Jan 23, 2023

github-actions bot commented Jan 23, 2023

github-actions bot commented Jan 23, 2023

benhalasi commented Jan 23, 2023

rickie left a comment

rickie Jan 23, 2023

rickie Jan 23, 2023

rickie Jan 23, 2023

benhalasi commented Jan 25, 2023

github-actions bot commented Jan 25, 2023

github-actions bot commented Jan 25, 2023

rickie left a comment

rickie Jan 25, 2023

rickie Jan 25, 2023

rickie Jan 25, 2023

rickie Jan 25, 2023

rickie Jan 25, 2023

github-actions bot commented Jan 25, 2023

github-actions bot commented Jan 25, 2023

rickie commented Jan 25, 2023

github-actions bot commented Feb 20, 2023

	.doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH);
	.doTest(TestMode.TEXT_MATCH);

Introduce FluxImplicitBlock check #472

Introduce FluxImplicitBlock check #472

Conversation

benhalasi commented Jan 20, 2023 • edited Loading

github-actions bot commented Jan 20, 2023

github-actions bot commented Jan 20, 2023

github-actions bot commented Jan 20, 2023

Stephan202 commented Jan 20, 2023

benhalasi commented Jan 20, 2023

rickie commented Jan 20, 2023

Stephan202 commented Jan 20, 2023

github-actions bot commented Jan 23, 2023

github-actions bot commented Jan 23, 2023

rickie left a comment

Choose a reason for hiding this comment

benhalasi commented Jan 23, 2023

Code duplication between checkers

How to replace a method call with a method call chain.

github-actions bot commented Jan 23, 2023

github-actions bot commented Jan 23, 2023

github-actions bot commented Jan 23, 2023

github-actions bot commented Jan 23, 2023

benhalasi commented Jan 23, 2023

rickie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benhalasi commented Jan 25, 2023

replaceMethodInvocationWithCollect method

replaceMethodInvocation method

other 'helper' methods

tests

move flux matchers to MoreMatchers

github-actions bot commented Jan 25, 2023

github-actions bot commented Jan 25, 2023

rickie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 25, 2023

github-actions bot commented Jan 25, 2023

rickie commented Jan 25, 2023

github-actions bot commented Feb 20, 2023

Introduce `FluxImplicitBlock` check #472

Introduce `FluxImplicitBlock` check #472

benhalasi commented Jan 20, 2023 •

edited

Loading

`replaceMethodInvocationWithCollect` method

`replaceMethodInvocation` method

move flux matchers to `MoreMatchers`