Generics Inference codemod #154
Conversation
|
|
danielostrow changed the title 
| | ||
| This change makes the code easier to review and removes IDE warnings that could harm readability. | ||
| ```diff | ||
| - List<String> myList = new ArrayList<String>(); |
There was a problem hiding this comment.
Why the gap in between these two?
| @@ -0,0 +1,10 @@ | |||
| Type inference is a feature introduced in Java 7 simplifying the building of generic classes. | |||
| | |||
| When using type parameters, the compiler infers the type arguments based on context, rather than explicitly specifying them. | |||
There was a problem hiding this comment.
It seems like the first 2 sentences should be in the same paragraph?
| @@ -0,0 +1,5 @@ | |||
| { | |||
| "summary" : "Type inference is a feature introduced in Java 7 simplifying the building of generic classes.", | |||
There was a problem hiding this comment.
Not a good summary, see previous PR
| @@ -0,0 +1,5 @@ | |||
| { | |||
| "summary" : "Type inference is a feature introduced in Java 7 simplifying the building of generic classes.", | |||
| "change" : "This change makes the code easier to review and removes IDE warnings that could harm readability.", | |||
There was a problem hiding this comment.
The change should be descriptive about the change. This text describes the value of the change
| rules: | ||
| - id: replace-generics-in-var | ||
| patterns: | ||
| - pattern-either: |
There was a problem hiding this comment.
Why is it $VALUE and not $X? Just trying to understand if there's something particular about the first generic? Why not just start at $A, $B, etc?
I don't see many 3+ generics... in fact, I might want to stay away from that code. It seems arbitrary to stop at 7, and rare to find anything more than 2. My vote is to only support up to 2-length generics.
There was a problem hiding this comment.
I'll make the changes.
Having $X in type covers cases when someone declares Map or List. The initializer will be a different value for the type and I needed a separate variable $X to read it. There was no organization in variable choices, I just chose different variables. I will correct that.
I will limit the rule to 2-length generics, however I do not see the negatives in including more than 2 besides a goofy looking rule.
| // Remove type arguments from the initializer, need to keep diamond operator | ||
| String cleanedInitializerCode = initializerCode.replaceAll("<.*?>", "<>"); | ||
| // Fixes bad output instances with nested types | ||
| String checkInitializerCode = cleanedInitializerCode.replaceAll("<>>", "<>"); |
There was a problem hiding this comment.
This is a code smell. Why does this happen? I think your regex is not great. I'm also wondering if this could be tricked/abused.
What about code like this?
List<String> foo = new ArrayList< /* i have weird chars here <for some reason> */>(); I'm guessing there's a cleaner way to do this in the JavaParser API without any regex weirdness. Keep looking.
| String checkInitializerCode = cleanedInitializerCode.replaceAll("<>>", "<>"); | ||
| | ||
| // Create a new StringLiteralExpr with the cleaned code | ||
| Expression cleanedInitializer = new StringLiteralExpr(checkInitializerCode); |
There was a problem hiding this comment.
This whole thing strategy leaves me terrified. There's a right way to do this and I really doubt this is it. Keep grinding. Use a debugger and inspect the AST objects in memory, and find the right one in there.
| @Codemod( | ||
| id = "pixee:java/replace-generics-in-var", | ||
| reviewGuidance = ReviewGuidance.MERGE_WITHOUT_REVIEW) | ||
| public final class ReplaceGenericsInVarCodemod |
There was a problem hiding this comment.
Is it ReplaceGenerics or is it... EmptyGenerics (the verb empty...). Can we take a look at this name again?
| VariableDeclarationExpr variableDeclarationExpr, | ||
| com.contrastsecurity.sarif.Result result) { | ||
| | ||
| for (VariableDeclarator variableDeclarator : variableDeclarationExpr.getVariables()) { |
There was a problem hiding this comment.
I'm not sure this is right, but it doesn't feel right. You can define multiple vars in a single declaration statement:
List<String> a = new ArrayList<String>(), b = new ArrayList<>(); It seems like the Semgrep is probably identifying one, but we're acting on all of them. It may be correct, because the left hand of the expression may make it redundant on all the right hand initializers, but I'm too tired to think through that right now. Anyhow, it definitely warrants a comment to discuss this.
| | ||
| variableDeclarator.setInitializer(formatted); | ||
| }); | ||
| } |
There was a problem hiding this comment.
I don't like the way this code is written because the place where we return true actually has no idea if any changes were made. It's tempting to put a lot of code into those ifPresent() and ifEmpty() chains, but it's tough get any state to escape from them.
| I'm closing this PR. Feel free to re-open when you're closer to a mergable state. Until then, it clutters about my view of the work that is "close to the main branch". |
3 participants
This codemod is for replacing generics in var creation.