-
Notifications
You must be signed in to change notification settings - Fork 92
Add Objects.requireNonNullElse/ElseGet support to AnnotateNullableParameters
#743
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Objects.requireNonNullElse/ElseGet support to AnnotateNullableParameters
#743
Conversation
The recipe now recognizes Objects.requireNonNull calls as null-checking operations and will annotate parameters passed to requireNonNull with @nullable. This handles both standalone calls and assignment statements using requireNonNull. - Added Objects.requireNonNull to the list of null-checking method matchers - Added visitMethodInvocation to handle standalone requireNonNull calls - Added tests for single and multiple parameter cases with requireNonNull
|
Not sure I agree with Claude here; when using |
Objects.requireNonNull throws an exception if the argument is null, enforcing non-null parameters. In contrast, requireNonNullElse and requireNonNullElseGet provide default values for null arguments, indicating they accept nullable parameters. - Replaced Objects.requireNonNull with requireNonNullElse and requireNonNullElseGet - These methods imply the parameter can be null since they provide fallback values - Updated all tests to use the new methods that actually indicate nullable parameters
Objects.requireNonNullElse/ElseGet support to AnnotateNullableParameters
|
@stefanodallapalma this seems right up your alley ; any suggestions or concerns with this addition? |
Added Optional.ofNullable to the list of null-checking methods in the AnnotateNullableParameters recipe. When a method parameter is passed to Optional.ofNullable(), it will now be annotated with @nullable to indicate that the parameter can accept null values. This improves the recipe's ability to identify nullable parameters, as Optional.ofNullable is specifically designed to handle potentially null values. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
@timtebeek I had a similar concern here.
|
|
Thanks yes |
|
Oh yeah, thanks. The current diff looks good to me. I only left a minor comment |
| } | ||
| public Optional<String> conditionalWrap(@Nullable String data) { | ||
| if (data != null && data.length() > 5) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find this test case a little redundant due to data != null. I'd rather test something like the following:
Optional.ofNullable(data).ifPresent(it -> ...);
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes makes sense, thanks! Will merge with that change.
… 2.18.0 to 2.19.0 [skip ci] Bumps [org.openrewrite.recipe:rewrite-static-analysis](https://github.com/openrewrite/rewrite-static-analysis) from 2.18.0 to 2.19.0. Release notes *Sourced from [org.openrewrite.recipe:rewrite-static-analysis's releases](https://github.com/openrewrite/rewrite-static-analysis/releases).* > 2.19.0 > ------ > > What's Changed > -------------- > > * Fix FinalClass recipe to correctly handle nested static subclasses by [`@timtebeek`](https://github.com/timtebeek) in [openrewrite/rewrite-static-analysis#742](https://redirect.github.com/openrewrite/rewrite-static-analysis/pull/742) > * Add `Objects.requireNonNullElse/ElseGet` support to `AnnotateNullableParameters` by [`@timtebeek`](https://github.com/timtebeek) in [openrewrite/rewrite-static-analysis#743](https://redirect.github.com/openrewrite/rewrite-static-analysis/pull/743) > * Fix ReplaceLambdaWithMethodReference handling of nested class imports by [`@timtebeek`](https://github.com/timtebeek) in [openrewrite/rewrite-static-analysis#745](https://redirect.github.com/openrewrite/rewrite-static-analysis/pull/745) > * OpenRewrite recipe best practices by [`@timtebeek`](https://github.com/timtebeek) in [openrewrite/rewrite-static-analysis#746](https://redirect.github.com/openrewrite/rewrite-static-analysis/pull/746) > * Update recipe documentation examples by [`@timtebeek`](https://github.com/timtebeek) in [openrewrite/rewrite-static-analysis#747](https://redirect.github.com/openrewrite/rewrite-static-analysis/pull/747) > * Support for-each loop in NeedBraces by [`@timtebeek`](https://github.com/timtebeek) in [openrewrite/rewrite-static-analysis#749](https://redirect.github.com/openrewrite/rewrite-static-analysis/pull/749) > * Update documentation examples by [`@timtebeek`](https://github.com/timtebeek) in [openrewrite/rewrite-static-analysis#750](https://redirect.github.com/openrewrite/rewrite-static-analysis/pull/750) > * Update documentation examples by [`@timtebeek`](https://github.com/timtebeek) in [openrewrite/rewrite-static-analysis#751](https://redirect.github.com/openrewrite/rewrite-static-analysis/pull/751) > * fixed test that got changed behavior in rewrite-core by [`@Jenson3210`](https://github.com/Jenson3210) in [openrewrite/rewrite-static-analysis#753](https://redirect.github.com/openrewrite/rewrite-static-analysis/pull/753) > * No longer remove String.valueof when it's called on a method invocation by [`@Laurens-W`](https://github.com/Laurens-W) in [openrewrite/rewrite-static-analysis#752](https://redirect.github.com/openrewrite/rewrite-static-analysis/pull/752) > > New Contributors > ---------------- > > * [`@Jenson3210`](https://github.com/Jenson3210) made their first contribution in [openrewrite/rewrite-static-analysis#753](https://redirect.github.com/openrewrite/rewrite-static-analysis/pull/753) > > **Full Changelog**: <openrewrite/rewrite-static-analysis@v2.18.0...v2.19.0> Commits * [`3c1a313`](openrewrite/rewrite-static-analysis@3c1a313) No longer remove String.valueof when it's called on a method invocation ([#752](https://redirect.github.com/openrewrite/rewrite-static-analysis/issues/752)) * [`1d8d504`](openrewrite/rewrite-static-analysis@1d8d504) fixed test that got changed behavior in rewrite-core ([#753](https://redirect.github.com/openrewrite/rewrite-static-analysis/issues/753)) * [`504a582`](openrewrite/rewrite-static-analysis@504a582) Update documentation examples ([#751](https://redirect.github.com/openrewrite/rewrite-static-analysis/issues/751)) * [`cd26b74`](openrewrite/rewrite-static-analysis@cd26b74) Update documentation examples ([#750](https://redirect.github.com/openrewrite/rewrite-static-analysis/issues/750)) * [`b80e4b6`](openrewrite/rewrite-static-analysis@b80e4b6) Support for-each loop in NeedBraces ([#749](https://redirect.github.com/openrewrite/rewrite-static-analysis/issues/749)) * [`b93d869`](openrewrite/rewrite-static-analysis@b93d869) Update recipe documentation examples ([#747](https://redirect.github.com/openrewrite/rewrite-static-analysis/issues/747)) * [`7165caf`](openrewrite/rewrite-static-analysis@7165caf) OpenRewrite recipe best practices ([#746](https://redirect.github.com/openrewrite/rewrite-static-analysis/issues/746)) * [`eb91699`](openrewrite/rewrite-static-analysis@eb91699) Fix ReplaceLambdaWithMethodReference handling of nested class imports ([#745](https://redirect.github.com/openrewrite/rewrite-static-analysis/issues/745)) * [`a5d78d7`](openrewrite/rewrite-static-analysis@a5d78d7) Polish MoveFieldAnnotationToType * [`0bd9340`](openrewrite/rewrite-static-analysis@0bd9340) Add `Objects.requireNonNullElse/ElseGet` support to `AnnotateNullableParamete... * Additional commits viewable in [compare view](openrewrite/rewrite-static-analysis@v2.18.0...v2.19.0) [](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- Dependabot commands and options You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - `@dependabot show ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)
Summary
AnnotateNullableParametersrecipe to recognizeObjects.requireNonNullElse()andObjects.requireNonNullElseGet()calls as indicators of nullable parametersWhy this change?
Objects.requireNonNull()throws an exception if the argument is null, enforcing non-null parametersObjects.requireNonNullElse()andObjects.requireNonNullElseGet()provide default values for null arguments, indicating the parameters can be null@NullableChanges
Objects.requireNonNullElse(..)andObjects.requireNonNullElseGet(..)to the list of null-checking method matchersvisitMethodInvocationto handle standalone calls (not just those in if conditions)Test plan
Objects.requireNonNullElsewith default valuesObjects.requireNonNullElseGetwith supplier functions🤖 Generated with Claude Code