Handle generics in PreferJavaUtilObjectsRequireNonNull#538
Handle generics in PreferJavaUtilObjectsRequireNonNull#538timtebeek merged 5 commits intoopenrewrite:mainfrom
Conversation
Before this patch, the PreferJavaUtilObjectsRequireNonNull recipe would only migrate usage of exactly `checkNotNull(Object)` but not any other (sub)types. We now match invocations of checkNotNull with exactly one or two arguments (both in refaster). Note that there is also a three argument version that doesn't have a straight-forward replacement, I skipped that for now and added a testcase.
|
Thanks for the clear improvement @mbruggmann ! Wasn't aware we only did those very selective replacements before. Great feedback! We indeed do not yet respect any static import hints on our Refaster reimplementation; that was a conscious choice at the time allowing us to move faster We'd of course hope to circle back in rewrite-templating to pick up and respect those hints, but haven't gotten around to that just yet. Until then we can expect the format here, and perhaps follow up with other recipes like UseStaticImport. |
|
Nice, thanks for fixing this up and getting it merged @timtebeek 🙇🏻 |
2 participants
What's changed?
Before this patch, the
PreferJavaUtilObjectsRequireNonNullrecipe would only migrate usage of exactlycheckNotNull(Object)but not any other (sub)types such ascheckNotNull(String),checkNotNull(CustomType)etc.We now match invocations of checkNotNull with exactly one or two arguments which makes the recipe more broadly applicable.
Note that there is also a three argument version that doesn't have a straight-forward replacement, I skipped that for now and added a testcase.
What's your motivation?
Migrate more usage of Preconditions.checkNotNull throughout our codebase.
Anything in particular you'd like reviewers to focus on?
The proposed implementation using refaster doesn't deal well with preserving the static import if one was used before. I'm not sure how to best handle that.
Anyone you would like to review specifically?
@timtebeek since you added refaster support.
Have you considered any alternatives or workarounds?
I have also tried modifying the method pattern to be
checkNotNull(*)in no-guava.yml but that did not work as expected. I couldn't find other examples of using a wildcard match on an argument, maybe it isn't supported?Checklist