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

@CheckReturnValue decoration for ApplicationContextAssert::getBean(s) methods #32683

Open
scordio opened this issue Oct 12, 2022 · 7 comments
Open
Labels
status: blocked An issue that's blocked on an external project change type: enhancement A general enhancement
Milestone

Comments

@scordio
Copy link
Contributor

scordio commented Oct 12, 2022

I suggest to decorate ApplicationContextAssert::getBean(s) methods with a @CheckReturnValue annotation so that incomplete statements like the following are spotted by static analysis tools like SpotBugs and IntelliJ IDEA:

assertThat(context).getBean(MyService.class);

To achieve this, no additional dependency is needed and Spring Boot Test can declare its own @CheckReturnValue annotation, similarly to what AssertJ and Mockito do.

I can raise a PR with the changes if the idea is accepted.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Oct 12, 2022
@scordio
Copy link
Contributor Author

scordio commented Oct 16, 2022

To achieve this, no additional dependency is needed and Spring Boot Test can declare its own @CheckReturnValue annotation, similarly to what AssertJ and Mockito do.

On second thought, Spring Boot Test might use org.assertj.core.util.CheckReturnValue directly since ApplicationContextAssert is an AssertJ-based implementation anyway.

@wilkinsona
Copy link
Member

Thanks for the suggestion, @scordio. Using @CheckReturnValue sounds like a good option in this particular case. However, I wonder if we have other APIs where a similar annotation would be beneficial. A bit like nullability annotations for Kotlin, handling this inconsistently may create more problems than it solves. Flagging for team attention to see what everyone else thinks.

@wilkinsona wilkinsona added the for: team-attention An issue we'd like other members of the team to review label Nov 18, 2022
@mhalbritter
Copy link
Contributor

I think this is a good idea if we do this consistently.

@philwebb
Copy link
Member

philwebb commented Dec 2, 2022

I like the idea as well and error prone has been on the list of things to investigate for a while.

@sdeleuze
Copy link
Contributor

sdeleuze commented Dec 5, 2022

This could be connected to spring-projects/spring-framework#28797 (tentatively planned for 6.1) if we extends its scope via jspecify/jspecify#200.

@sbrannen
Copy link
Member

sbrannen commented Dec 5, 2022

Regarding Eclipse IDE support, see https://bugs.eclipse.org/bugs/show_bug.cgi?id=572496.

@philwebb philwebb changed the title @CheckReturnValue decoration for ApplicationContextAssert::getBean(s) methods @CheckReturnValue decoration for ApplicationContextAssert::getBean(s) methods Dec 14, 2022
@philwebb philwebb added type: enhancement A general enhancement status: blocked An issue that's blocked on an external project change and removed for: team-attention An issue we'd like other members of the team to review status: waiting-for-triage An issue we've not yet triaged labels Dec 14, 2022
@philwebb philwebb added this to the 3.x milestone Dec 14, 2022
@philwebb
Copy link
Member

Marking this as blocked as we want to see what Framework do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: blocked An issue that's blocked on an external project change type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

7 participants