feat: add nullness annotations to public API part 2

[greyhairredbear]

As discussed, this is a continuation of #1009. Opening again as draft.

Open items

• missing annotations in score.stream.common.ConstraintCollectors
• score.stream.penta
• score.stream.quad
• check impl related to score package
• check if all impl related to solver package has been annotated
• config
• timefold-solver-benchmark
• timefold-solver-test

[greyhairredbear]

@triceo I have a general question here on ConstraintCollectors: There are many functions without any information on nullability in the Javadoc, but I guess for most of the parameters (mappers and valueSetFunctions and similar likes), @NonNull should be appropriate, correct? Are there any instances in this file where a parameter or a return value might be @Nullable? I coudn't find any quickly searching/browsing through this file, but I want to make sure I'm correct before over-promising.

[triceo]

None of these methods will ever return null - that is for sure.
When it comes to arguments, I don't expect them to be nullable. If any of them are, I'd expect the Javadoc to say so.

[greyhairredbear]

Small sidenote since I stumbled upon it: toConnectedRanges/tonConnectedTemporalRanges has some overloads that IntelliJ reports as unused, so I guess there are no tests for those either. Don't know how much value there would be of testing those specific cases, just wanted to flag this for the small chance it has gone unnoticed.

[greyhairredbear]

@triceo As my next step is the config package: Lots of the members of a config are initialized with null and there is little information in the Javadoc. I guess annotating getters and parameters for setters with...-methods as @Nullable, do you agree?

[triceo]

Getters and setters in the config typically use the convention "null means user did not specify, pick default." Those should probably all be annotated as nullable.

The withers, on the other hand, should be annotated as non-null. If you use a wither, you are deliberately specifying what you want; not specifying means not using the wither at all.

[greyhairredbear]

Hi @triceo, just wanted to keep you updated on my progress here, since I didn't manage to complete the work here by tonight.

Currently, I'm still working on the config package, but I have some free resources tomorrow and Tuesday night, so I suppose I should be through with the changes by then. There are still some todos in the PR, so I'll add comments on those so we can track them easier.

I hope this delay in schedule doesn't lead to too many inconveniences for you, I hope the time buffer until the release date on Nov 10th (iirc) is sufficient.

[triceo]

Hey @greyhairredbear - thanks for letting me know! As long as we can still merge it this week, we should be good.

[greyhairredbear]

@triceo I guess I'm through with the last commit, I'll comment and tag you on the open todos/questions I have tomorrow (probably before I start my workday). After completing that I suppose this should be good to merge

[triceo]

I have hopefully answered all the comments.
Unfortunately, in at least one of the configs, the nullability doesn't follow the agreed pattern, and therefore I must request changes.

[greyhairredbear]

@triceo I fixed the wrongly annotated getters/setters in PlannerBenchmarkConfig and reviewed the other Config files, didn't find any other mistakes like this. I've also commented on the last remaining todos for this PR.

[triceo]

Answering more of the comments.

You found some very bizarre pieces of code that we have in there. Note to self: some of this could use refactoring.

[triceo]

The Sonar warnings, some of them important, are not your concern - don't worry about them. Something for me to look into after.

[triceo]

Thank you, @greyhairredbear, for this work!
The solver and its users will be all the better for it.

I'll merge when the CI machinery approves. (Sonar won't, but I'll take care of that later.)

[sonarqubecloud]

Quality Gate failed

Failed conditions
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint