Add nullability contract to PasswordEncoder#encode

[scordio]

Originally reported at uber/NullAway#1273 (comment):

It's PasswordEncoder.encode from Spring Security. The JSpecify annotations were recently added throughout Spring Security, but are currently only released as milestones of v7. If you have a suggestion for how they can improve their usage of these annotations, now would be an ideal time to let them know.

This PR should solve the use case reported in the NullAway issue.

I don't know whether there should be a test for this change or how to compose it... in case there should be one, please let me know and I'll think about something 🙂

[rwinch]

Thank you for the PR @scordio! I've set this to be merged as soon as the build passes.

I do think we should add tests, but we can likely do this in another commit. I can see how to test it with a non-null value passed in, but a null value would probably need to borrow infrastructure from NullAway's testing? @sdeleuze how do you recommend testing @Contract?

[scordio]

Thanks for fixing Checkstyle!

[scordio]

I can see how to test it with a non-null value passed in, but a null value would probably need to borrow infrastructure from NullAway's testing?

Hi @msridhar, do you happen to have a suggestion on how to test this change in Spring Security?

[sdeleuze]

@sdeleuze how do you recommend testing @Contract?

One way would be to have spring-gradle-plugins/nullability-plugin#10 implemented.

The other one, potentially complementary one, would be to enable NullAway checks on tests and write a test that is supposed to fail NullAway checks but I have mixed feelings about NullAway on tests since sometimes you want on purpose to write invalid tests for a null-safety perspective and that can be a huge task, so I would just recommend enabling https://github.com/uber/NullAway/wiki/Configuration#contract-checking when the nullability plugin issue mentioned above will be fixed.

[rwinch]

The other one, potentially complementary one, would be to enable NullAway checks on tests and write a test that is supposed to fail NullAway checks

Can you expand on this? I get how the success case could work. However, if it fails the checks, I don't know how a failed compilation would be success without some infrastructure around this?

[scordio]

Just FYI, here is a test I contributed to the NullAway codebase using the NullAwayTestBase infrastructure.

That test was focused on suppressing NullAway, but I suppose the concept would be similar for @Contract testing.

[msridhar]

LGTM! Technically, I think this contract should also be copied down to any implementations of encode in classes implementing PasswordEncoder as well. That way, if you enable NullAway's contract checking, it will check that the implementation adheres to the contract. Plus, right now if you write BCryptPasswordEncoder b = ...; x = b.encode(...);, NullAway won't see the contract on the interface method and won't apply it.

[scordio]

Ah, good point! Addressed in #18490.

[sdeleuze]

Can you expand on this? I get how the success case could work. However, if it fails the checks, I don't know how a failed compilation would be success without some infrastructure around this?

Either what @scordio shared or just you rely on your broken test NullAway checks as a special kind of test conceptually speaking. But I don't necessarily recommend that.