Skip to content
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 missing @Nullable in SettableListenableFuture #29150

Closed
wants to merge 1 commit into from

Conversation

jensdietrich
Copy link
Contributor

Add @Nullable to return type of org.springframework.util.concurrent.SettableListenableFuture::get()

Supporting test traces illustrating the use of null in actual program behaviour:

org.springframework.util.concurrent.SettableListenableFuture::get:119
org.springframework.util.concurrent.SettableListenableFutureTests::nullIsAcceptedAsValueToSet:223

This issue has been found by an automated analysis and confirmed by manual inspection.
A description of the analysis performed can be found here: (https://github.com/jensdietrich/null-annotation-inference).

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Sep 14, 2022
@jensdietrich
Copy link
Contributor Author

I just realised that checkstyle expects the annotation to be on a separate line. I can withdraw this and create another PR to comply with this rule if required.

@sbrannen sbrannen changed the title added missing @Nullable to get() Add missing @Nullable in SettableListenableFuture Sep 14, 2022
@sbrannen
Copy link
Member

I just realised that checkstyle expects the annotation to be on a separate line. I can withdraw this and create another PR to comply with this rule if required.

No need to create a new PR.

Whenever you need to make a change to an existing PR, you can simply push another commit to the branch for the PR.

In this particular case, we can take care of it for you.

By the way, SettableListenableFuture#get(long, TimeUnit) is also missing a @Nullable declaration, but I'll take care of that.

@sbrannen sbrannen self-assigned this Sep 14, 2022
@sbrannen sbrannen added type: task A general task in: core Issues in core modules (aop, beans, core, context, expression) and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Sep 14, 2022
@sbrannen sbrannen added this to the 5.3.23 milestone Sep 14, 2022
sbrannen added a commit that referenced this pull request Sep 14, 2022
@sbrannen sbrannen closed this in 35d379f Sep 14, 2022
@jensdietrich
Copy link
Contributor Author

thanks @sbrannen for accepting the change. I have a significant number of other missing @Nullable annotations inferred by the tool we have developed (part of our research at Victoria University of Wellington, with support from Oracle Labs), and I wonder what the best way is to get this actioned. I could create more PRs, but I wonder what a good level of granularity would be as some of them affect many classes. I picked this one as a pilot. An alternative would be perhaps via an issue, where I could post the reports generated with details about the inferred annotations including supporting provenance, but committers would have to do the actual changes. BTW, the tool can also make changes to the source code. Any advice is highly appreciated.

module fields params returns all
spring-beans 1 (1) 75 (47) 7 (7) 83 (51)
spring-context 1 (1) 50 (34) 16 (15) 67 (47)
spring-core 0 (0) 39 (22) 1 (1) 40 (23)
spring-orm 0 (0) 17 (13) 4 (4) 21 (14)
spring-oxm 0 (0) 3 (2) 2 (1) 5 (3)
spring-web 19 (8) 164 (76) 25 (19) 208 (97)
spring-webmvc 0 (0) 109 (59) 44 (36) 153 (89)

Missing @nullable Annotations Detected by Module and Type of Annotated Element. The numbers in brackets are the numbers of classes affected. See https://github.com/jensdietrich/null-annotation-inference for the methodology used.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core Issues in core modules (aop, beans, core, context, expression) type: task A general task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants