Generate uses type and uses method #24
Conversation
src/main/java/org/openrewrite/java/template/RefasterTemplateProcessor.java
Show resolved
Hide resolved
src/main/java/org/openrewrite/java/template/RefasterTemplateProcessor.java
Outdated
Show resolved
Hide resolved
src/main/java/org/openrewrite/java/template/RefasterTemplateProcessor.java
Outdated
Show resolved
Hide resolved
| @@ -100,7 +106,47 @@ public J visitMethodInvocation(J.MethodInvocation elem, ExecutionContext ctx) { | |||
| } | |||
|
|
|||
| }; | |||
| return Preconditions.check( | |||
| new UsesType<>("java.util.Objects", false), | |||
There was a problem hiding this comment.
Out of curiosity: Why is there no UsesMethod for Objects#equals() here? I would have expected the precondition to check for either Objects#equals() or Integer#compare().
There was a problem hiding this comment.
Currently those would only be generated if we use a static import of those methods; we can expand that of course, but figured this first implementation already limits the execution of the recipes to recipes using the type, which at scale already trims it down quite a bit.
There was a problem hiding this comment.
Thanks for the explanation. Yes, also covering the others would indeed make sense.
There was a problem hiding this comment.
Logged as an improvement:
| }; | ||
| return Preconditions.check( | ||
| Preconditions.or( | ||
| Preconditions.and(new UsesType<>("java.util.HashMap", false), new UsesType<>("java.util.Map", false)), |
There was a problem hiding this comment.
Since Map is not necessarily directly referenced by the input source, I think we should either leave it out or use the overload with the includeImplicit parameter. Otherwise the precondition may be too strict. For example:
Collection m = new HashMap();There was a problem hiding this comment.
That being said, I am not 100% sure if Refaster would match this. But I think it would.
There was a problem hiding this comment.
Hmm; we don't differentiate between in and output types when we detect imports right now. Would passing true to usesType e7d0883 be enough to reduce your concern here?
| }; | ||
| return Preconditions.check( | ||
| Preconditions.or( | ||
| Preconditions.and(new UsesType<>("java.util.HashMap", false), new UsesType<>("java.util.Map", false)), |
There was a problem hiding this comment.
That being said, I am not 100% sure if Refaster would match this. But I think it would.
| }; | ||
| return Preconditions.check( | ||
| Preconditions.or( | ||
| Preconditions.and(new UsesType<>("java.util.HashMap", true), new UsesType<>("java.util.Map", true)), |
There was a problem hiding this comment.
For HashMap we could actually pass the second argument as true, because it must be directly referenced. Map is different, because you got that from the template's return type and not the template body. But we can still fix this later.
There was a problem hiding this comment.
Thanks! Captures as a note here: #26 (comment)
What's changed?
Generate Preconditions based on imports and static imports used in before templates.
What's your motivation?
Speed up recipe execution.
Fixes #23
Anything in particular you'd like reviewers to focus on?
Would we want to detect additional methods now already?
Figured this was good enough to start to limit recipe execution.
Have you considered any alternatives or workarounds?
We could add a new detector specific for methods used in the before templates, but that's perhaps more work we can do later.
Any additional context
UsesTypeandUsesMethodto limit recipe execution and improve performance #23doAfterVisit(UnnecessaryParentheses)to generated recipes #22