Have DirectReturn check consider finally blocks
#568
Conversation
| * <p>Inlining an expression generally does not change its return type, but in rare cases the | ||
| * operation may have a functional impact. The sole case considered here is the inlining of a | ||
| * Mockito mock or spy construction without an explicit type. In such a case the type created | ||
| * depends on context, such as the method's return type. | ||
| */ | ||
| private static boolean canInlineToReturnStatement( |
There was a problem hiding this comment.
Should now find a better name for this method. Suggestions welcome.
There was a problem hiding this comment.
We could add Tree or something, but that doesn't add a lot.
In EP they use the term "inlineable". We could do something with that. I cannot think of many suggestions as "inline" is the best verb here IMO.
There was a problem hiding this comment.
Hmm, my concern here is more that canInline is actually too strong, since now this method only tests are required, but not sufficient set of conditions to be met. I guess we can just merge as-is.
|
Looks good. All 18 mutations in this change were killed.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
Stephan202
force-pushed
the
sschroevers/direct-return-finally-blocks
branch
from
d17ea37 to
0231333
Compare
|
Looks good. All 20 mutations in this change were killed.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
1 similar comment
|
Looks good. All 20 mutations in this change were killed.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
|
Kudos, SonarCloud Quality Gate passed!
|
|
Not necessarily recommended 😅 |
rickie
force-pushed
the
sschroevers/direct-return-finally-blocks
branch
from
0231333 to
d513989
Compare
|
Looks good. All 20 mutations in this change were killed.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
| * executed <em>after</em> control flow returns from the {@link VisitorState#getPath() current | ||
| * location}. | ||
| */ | ||
| private static boolean isSymbolReferencedInAssociatedFinallyBlock( |
There was a problem hiding this comment.
For this method name we are not using identifierSymbol while below we are 🤔. Should we update this method name to also mentions identifier as that is what it does.
| * <p>Inlining an expression generally does not change its return type, but in rare cases the | ||
| * operation may have a functional impact. The sole case considered here is the inlining of a | ||
| * Mockito mock or spy construction without an explicit type. In such a case the type created | ||
| * depends on context, such as the method's return type. | ||
| */ | ||
| private static boolean canInlineToReturnStatement( |
There was a problem hiding this comment.
We could add Tree or something, but that doesn't add a lot.
In EP they use the term "inlineable". We could do something with that. I cannot think of many suggestions as "inline" is the best verb here IMO.
| * <p>Inlining an expression generally does not change its return type, but in rare cases the | ||
| * operation may have a functional impact. The sole case considered here is the inlining of a | ||
| * Mockito mock or spy construction without an explicit type. In such a case the type created | ||
| * depends on context, such as the method's return type. | ||
| */ | ||
| private static boolean canInlineToReturnStatement( |
There was a problem hiding this comment.
Hmm, my concern here is more that canInline is actually too strong, since now this method only tests are required, but not sufficient set of conditions to be met. I guess we can just merge as-is.
|
Looks good. All 20 mutations in this change were killed.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
|
Changed this to a bug as it was reported as such :). |
Resolves an issue reported by @francisco-f-silva.
Suggested commit message: