Introduce NonStaticImport check
#450
Conversation
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
cernat-catalin
changed the title
cernat-catalin
force-pushed
the
cernat-catalin/static-import
branch
from
679f597
to
56ad071
Compare
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
|
Hey @cernat-catalin, thanks for opening the PR 🚀 ! We have a GitHub Action that runs Pitest on the changes of the PR. This helps identifying edge cases that are not tested. Can you look at the comments the bot placed on the changes in the Files changed tab? If you need help interpreting the results, let me know! |
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
|
@rickie Pitest is lit! It's the first time I encounter it but it was really helpful. There still are three mutations on some conditionals. I could remove them (and suppress a warning generated by nullaway) but I have a feeling that they are useful however I can't find any examples 😞 . |
|
I might wait for @rickie to do a first review round (I'm quite swamped atm), but let me say: great PR description! ❤️ |
There was a problem hiding this comment.
Added two commits.
Till now I have mostly placed comments and improvements on the style. I needed to do a round of that first before I can dive in a little more. I took some time to get a full picture of the check.
So will have to do another round some other time. Already left some comments. Let me know if you have any questions :).
error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/BadStaticImport.java
Outdated
Show resolved
Hide resolved
error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/BadStaticImport.java
Outdated
Show resolved
Hide resolved
error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/BadStaticImportTest.java
Outdated
Show resolved
Hide resolved
error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/BadStaticImportTest.java
Outdated
Show resolved
Hide resolved
error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/BadStaticImportTest.java
Outdated
Show resolved
Hide resolved
error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/BadStaticImport.java
Outdated
Show resolved
Hide resolved
error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/BadStaticImport.java
Outdated
Show resolved
Hide resolved
| private Description getDescription(IdentifierTree tree, VisitorState state) { | ||
| Symbol symbol = ASTHelpers.getSymbol(tree); | ||
| SuggestedFix.Builder builder = | ||
| SuggestedFix.builder().removeStaticImport(getImportToRemove(symbol)); |
There was a problem hiding this comment.
Hmm, I'm curious if we can always do this 🤔. Removing an import can be dangerous. This is usually a task for the RemoveUnusedImport check in a subsequent run.
However, I'm thinking that we can actually get away with this one. What if there are two identifiers statically imported that we update, is there a possibility that one of the two cannot be imported 🤔. If there is a possibility that can happen, we cannot remove the static import.
There was a problem hiding this comment.
Good question. I'm thinking the following:
Say we have an identifier X that with two occurrences (or more) in the same file. IIUC, the situation we want to avoid is the isMatch method returning true in one place and false in another.
However, for that to happen, one of the following must return different results for the occurrences:
symbol.owner != TYPCan't have different ownersisDefinedInThisFile(...)They can't be defined in different filesisIdentifierStaticallyImported(...)Since it is the same identifier we will have the same resultisExemptedandisCandidateWill return the same results
I guess it boils down to the fact that we never use the surrounding context of the identifier to make a decision. WDYT?
error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/BadStaticImport.java
Outdated
Show resolved
Hide resolved
| ".", symbol.getEnclosingElement().getQualifiedName(), symbol.getSimpleName()); | ||
| } | ||
|
|
||
| private static boolean isMatch(IdentifierTree tree, VisitorState state) { |
There was a problem hiding this comment.
Hmm isMatch should have a more descriptive name. Will check this out later. Or feel free to propose something.
There was a problem hiding this comment.
Hard one as it is a composition of multiple conditions. Maybe something like isValidCandidate. Still a bit vague and clashes somewhat with isCandidate.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
| private static String getImportToRemove(Symbol symbol) { | ||
| return String.join( | ||
| ".", | ||
| requireNonNull(symbol.getEnclosingElement()).getQualifiedName(), |
There was a problem hiding this comment.
| requireNonNull(symbol.getEnclosingElement()).getQualifiedName(), | |
| symbol.owner.getQualifiedName(), |
Now that I think of it, we can probably just use owner directly. That also makes me wonder if we should call it "enclosingSymbol" further down, that's probably not the correct term 🤔.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
2 similar comments
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
|
Thanks @rickie for the review! Will try to go over it later today (and see if I can spot any other things as well). |
cernat-catalin
force-pushed
the
cernat-catalin/static-import
branch
from
8f79c19
to
4158c4f
Compare
|
Rebased, added some more identifiers and 🔫 all of the surviving mutants (with a somewhat weird example). |
|
Looks good. All 50 mutations in this change were killed.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
|
@rickie @Stephan202 Is this something that we still want? |
Hi @cernat-catalin, yes definitely. Indeed, there is not a lot of activity from our side on this PR lately. There are a few (relatively) big PRs open in EPS that require quite some time + deep focus to review. The fact we didn't review recently does not mean the PR is not good. We are aware of the PR andit will be reviewed, given our schedules it's hard to give an estimation. All I can say is that, this still a really nice PR and we definitely didn't forget about it. |
|
Thanks for the answer @rickie ! No rush from my side and I understand there is a lot of work to be done and not that many people that can do it. Glad to see that it is still on your radar! |
rickie
force-pushed
the
cernat-catalin/static-import
branch
from
4a9350b
to
bf21b9f
Compare
|
Rebased and added a commit with the revert, will merge once 🟢 ! |
rickie
force-pushed
the
cernat-catalin/static-import
branch
from
bf21b9f
to
6fea4d8
Compare
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
1 similar comment
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
|
Please retry analysis of this Pull-Request directly on SonarCloud. |
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
Stephan202
changed the title
NonStaticImport check
Introduce a rule that flags bad static imports (complements the existing
StaticImportrule).BadStaticImportmight not be the best name but it was in line withcom.google.errorprone.bugpatterns.BadImport.Approach
Initially I tried matching static imports and then finding the relevant identifiers but that seemed too complicated 🧶 . After many rewrites matching method invocations and identifiers seemed to work (and was simpler than expected) though I'm not sure if this covers all cases or if there are false positives. I'm also not sure what (if any) is the performance penalty for matching on identifiers.
Static imports and bad static imports
Since these two rules need to coexist they need to be mutually exclusive (otherwise bad things could happen like the order in which are applied would matter). As far as I can tell, there are multiple ways to do this depending on how much flexibility we want to allow when defining exceptions (i.e. exempted members).
I've tried to allow the rules to be as flexible as possible so the following should be possible to specify:
The mutual exclusion is guaranteed by the tests. Consider the following notation:
T.*Types that should be statically imported (defined inStaticImport)T.IDType identifier pairs that should be statically imported (defined inStaticImport)T'.*Types that should not be statically imported (defined inBadStaticImport)T'.ID'Type identifier that should not be statically imported (defined inBadStaticImport)*.ID'Identifiers that should not be statically imported (defined inBadStaticImport)In
StaticImportwe allow all type identifier pairs from this set:(T.* \ T'.ID' \ *.ID') U T.IDIn
BadStaticImportwe allow all type identifier pairs from this set:(T'.* \ T.ID) U T'.ID' U *.ID'In the tests we also enforce the following which should guarantee the mutual exclusion of the above defined sets (
^is intersection):T.* ^ T'.* = emptyT.ID ^ T'.ID' = emptyT.ID ^ *.ID' = emptyThere are still some comments to resolve plus candidates and tests to be added (will get to it in some subsequent commits).