timefold-solver feat: add nullness annotations on public API (WIP)

Opening this draft PR to see CI results for javadoc & collect early feedback

Open items

[x] domain
[x] impl related to domain package
[x] function
[x] impl related to function package
[x] score
[x] score.analysis
[x] score.buildin
[x] score.calculator
[x] score.constraint
[x] score.director
[x] score.stream
[x] score.stream.bi
[x] score.stream.common
[x] score.stream.penta
[ ] score.stream.quad
[ ] score.stream.tri
[ ] score.stream.uni
[ ] impl related to score package
[x] solver
[ ] (re)check if all impl related to solver package has been annotated
[ ] config
[ ] timefold-solver-benchmark
[ ] timefold-solver-test
[ ] check format :)
[ ] collect open todos and resolve together with maintainer

Skipped (for now):

annotation attributes (e.g. PiggybackShadowVariable.shadowVariableName) - not sure if this would be valuable. IDE complains "Attribute value must be constant" when trying to assign null here. If there is value in this because the solver internally uses these attributes, I'd be ok with adding these too.
deprecated classes and methods

Jul 30 '24 16:07 greyhairredbear

Thanks for the PR, @greyhairredbear! I'll review once the PR is undrafted.

Jul 30 '24 17:07 triceo

FYI @greyhairredbear:

The code coverage metric from Sonar is likely not relevant in this case. You're mostly be touching the API code here, so don't let it discourage you.
Please also think of the config package. Even though it's technically not api, people are likely to use it and we treat it as public API.
There are APIs in the timefold-solver-benchmark and timefold-solver-test module too.

Jul 31 '24 13:07 triceo

Quality Gate failed

Failed conditions
66.7% Coverage on New Code (required ≥ 70%)

See analysis details on SonarCloud

Aug 06 '24 16:08 sonarqubecloud[bot]

@triceo I have a (probably trivial) question because I ran into some formatting issues despite using the eclipse formatting plugin for IntelliJ: Is there a faster way of formatting the whole project with spotless than using mvn install (which is doing a bunch of other stuff)? I've used spotless with gradle where it was just ./gradlew spotlessApply. I'm not that familiar with maven and tried the likes of mvn spotless:apply in the root folder, but this gave me just an error.

UPDATE: Also tried mvn spotless:apply -pl build/build-parent/,core -amd, however, source files with the wrong formatting weren't changed, even though the cmd output suggested spotless had ben run 🤔

Aug 06 '24 21:08 greyhairredbear

@greyhairredbear I never tried to execute Spotless on its own, so I don't know what's going on there. The following is probably the quickest you can do with our build:

mvn install -Dquickly -Dspotless.skip=false

Wrt. IntelliJ - I use that plugin too, and it works for me. What I did forget once in the past though was to configure import ordering, which is a second step after configuring the formatting itself.

Aug 07 '24 05:08 triceo

Late note on the IntelliJ with the eclipse plugin: The issue probably was that some part of IntelliJ still worked on the imports. By default, some packages (e.g. java.util) are configured to be imported with star imports. This broke them for me.

Aug 13 '24 21:08 greyhairredbear

Hi @triceo, I've come across some interfaces like EasyScoreCalculator that only have implementations in the test code. IntelliJ complains about missing nullability annotations, so but it offers to add them as a quickfix, which is what I did and plan on doing for further occurrences.

Now, as expected, this changes a lot of classes in the test code. Any opinions on if the nullability annotations are also OK in the test code? For a reference, you can take a look at 5ab620e. Just want to make sure I don't have to change that back later on, would be a bit tedious 😅

Another note here: When adding the annotation in the test code, the DummySpringEasyScore would contradict the annotations, as it does return null on a @NonNull-annotated method. Haven't looked if something like that also happens elsewhere or looked if some test code relies on this behavior. I guess I could change the return value to some default, but I don't want to break anything, so if you could provide some context here, that'd be pretty nice.

Aug 14 '24 16:08 greyhairredbear

Hi @triceo, I've come across some interfaces like EasyScoreCalculator that only have implementations in the test code. IntelliJ complains about missing nullability annotations, so but it offers to add them as a quickfix, which is what I did and plan on doing for further occurrences.

I think it makes sense. There should be no more warnings after this PR than before it.

Another note here: When adding the annotation in the test code, the DummySpringEasyScore would contradict the annotations, as it does return null on a @NonNull-annotated method. Haven't looked if something like that also happens elsewhere or looked if some test code relies on this behavior. I guess I could change the return value to some default, but I don't want to break anything, so if you could provide some context here, that'd be pretty nice.

This class, like many others, exists just to exist - the solver config that is being tested requires an implementation, and so we provide a dummy implementation. Returning a default value makes sense here; we only return null because it didn't matter what we returned. The class would never be instantiated. This is a difference to Testdata* classes, which are likely to have instances, and very likely will actually be executed.

Aug 14 '24 17:08 triceo

@greyhairredbear I see that the PR is growing nicely!

I just thought I'd give you a heads-up - we will soon start introducing new public APIs from our work on replacing move selectors. I haven't been using nullness annotations there, as I first want to merge this PR. But I do worry about creating conflicts for you, and also expanding your scope by introducing new public APIs.

Do you have any idea when you expect this work to be done? Maybe we can somehow synchronize.

Oct 03 '24 15:10 triceo

Hi @triceo!

I just thought I'd give you a heads-up - we will soon start introducing new public APIs from our work on replacing move selectors. I haven't been using nullness annotations there, as I first want to merge this PR. But I do worry about creating conflicts for you, and also expanding your scope by introducing new public APIs.

Thanks for thinking of that!

Do you have any idea when you expect this work to be done? Maybe we can somehow synchronize. I fear I can give you no reliable estimation on that, since spare time has been more scarce since my wife started working again in the end of August (sorry for the delay). I'm quite certain however, that I won't be finished before the end of next week with my current schedule.

The score.stream and its child packages did contain quite a bit more methods than I expected and annotating all of this is kind of tedious (feeling like an intern with this work 😅). Having added the annotations to the bi package, there are still uni, tri and quad left with a lot of code to annotate. So I guess I'm 1/4 here.

I haven't looked through the code for the config, benchmark and test packages, so I don't know how many changes are required there... Since you know the codebase better, maybe you have a notion of how much work is involved there.

An alternative approach to merging all of this at once (in case the config, benchmark and test are a lot of work) you could also gradually introduce the annotations which could reduce the risk of conflicts. The question is if that is something you'd be willing to do and if this would be worth potential confusion among users.

I also don't know how much extra work the changed move selectors would require, this might also influence any decision on how to proceed.

Oct 03 '24 20:10 greyhairredbear

I fear I can give you no reliable estimation on that, since spare time has been more scarce since my wife started working again in the end of August (sorry for the delay). I'm quite certain however, that I won't be finished before the end of next week with my current schedule.

End of next week, or even the week after, could still be good enough to avoid conflicts.

The score.stream and its child packages did contain quite a bit more methods than I expected and annotating all of this is kind of tedious (feeling like an intern with this work 😅). Having added the annotations to the bi package, there are still uni, tri and quad left with a lot of code to annotate. So I guess I'm 1/4 here.

I feel your pain. Writing CS code is one part fun and three parts boilerplate. Java's type system forces us to repeat ourselves there a lot.

I haven't looked through the code for the config, benchmark and test packages, so I don't know how many changes are required there... Since you know the codebase better, maybe you have a notion of how much work is involved there.

I'd say that once you're done with CS, you're over the hump. Configs are still relatively large, but then test is almost trivial.

An alternative approach to merging all of this at once (in case the config, benchmark and test are a lot of work) you could also gradually introduce the annotations which could reduce the risk of conflicts. The question is if that is something you'd be willing to do and if this would be worth potential confusion among users.

I'd like to avoid a situation of us having this released only partially - as you say, it would confuse users. That said, the next release (1.15.0) is on October 8, and then we have a month until the next one. So if you're willing to commit to delivering the rest by November 12 (1.16.0), we can merge the work-in-progress PR at any time after October 8.

I also don't know how much extra work the changed move selectors would require, this might also influence any decision on how to proceed.

In the first phase, we'll just be introducing new interfaces for the moves, we won't be touching the selectors. I'm hoping that by the time we get to the selectors, which will be weeks later, this PR will have already been merged and from that point onward, we take care of nullity annotations during our standard development process.

To sum up all of the above:

There's probably 1-2 weeks until any serious new APIs start landing on main. From what you wrote, I understand there is still a chance you'll be done by then. Assuming the worst case scenario...
The next solver release is on October 8, after which we have a month when we can merge unfinished work. If you can commit to delivering the entire work by Nov 11 at the latest, I can see us merging this PR as-is before we bring any changes to public API. You'd then bring the remaining changes in another PR, but before Nov 11.

How does that sound?

Oct 04 '24 06:10 triceo

The score.stream and its child packages did contain quite a bit more methods than I expected and annotating all of this is kind of tedious (feeling like an intern with this work 😅). Having added the annotations to the bi package, there are still uni, tri and quad left with a lot of code to annotate. So I guess I'm 1/4 here.

I feel your pain. Writing CS code is one part fun and three parts boilerplate. Java's type system forces us to repeat ourselves there a lot.

Going off topic here, but I'd be interested what feature of a type system could improve that. At first I thought something like default parameters for methods could help, but then I read through one or two methods in the actual code (groupBy and ifNotExistsOther in UniConstraintStream) which gave me the impression that this probably has a lot of drawbacks (E.g. having default parameters on ifNotExistsOther would require the joiner parameters to be nullable or to have a "noop-joiner" or something similar).

To sum up all of the above:

There's probably 1-2 weeks until any serious new APIs start landing on main. From what you wrote, I understand there is still a chance you'll be done by then. Assuming the worst case scenario...

The next solver release is on October 8, after which we have a month when we can merge unfinished work. If you can commit to delivering the entire work by Nov 11 at the latest, I can see us merging this PR as-is before we bring any changes to public API. You'd then bring the remaining changes in another PR, but before Nov 11.

How does that sound?

I think that sounds good. Gives me somewhat of a realistic target and since I did have a nice focused session yesterday, I'm a bit more optimistic than before.

Oct 04 '24 22:10 greyhairredbear

Going off topic here, but I'd be interested what feature of a type system could improve that.

My biggest issue with CS, and the thing that IMO causes the most boilerplate, is the fact that we have to have Uni, Bi, Tri and Quad copies of every stream, because the method arguments are different types (Function for Uni, BiFunction for Bi etc.) In the ideal world, there would be a way to have generics that would take care of this inside a single ConstraintStream interface, which would be parameterized by a simple uni/bi/tri/quad extended interface. (As opposed to duplicating literally every single method in the extended interface, as we do now.)

This duplication is also unfortunate because everything hurts four times. A deprecation is repeated four times for every stream cardinality. New methods have to be copied four times, with their Javadocs manually adjusted for the different arguments. Tests have to be written four times. And so on, and so on. Writing CS is painful. (But we take the pain, so that the users can have a nice fluent API.)

Oct 06 '24 18:10 triceo

Going off topic here, but I'd be interested what feature of a type system could improve that.

My biggest issue with CS, and the thing that IMO causes the most boilerplate, is the fact that we have to have Uni, Bi, Tri and Quad copies of every stream, because the method arguments are different types (Function for Uni, BiFunction for Bi etc.) In the ideal world, there would be a way to have generics that would take care of this inside a single ConstraintStream interface, which would be parameterized by a simple uni/bi/tri/quad extended interface. (As opposed to duplicating literally every single method in the extended interface, as we do now.)

This duplication is also unfortunate because everything hurts four times. A deprecation is repeated four times for every stream cardinality. New methods have to be copied four times, with their Javadocs manually adjusted for the different arguments. Tests have to be written four times. And so on, and so on. Writing CS is painful. (But we take the pain, so that the users can have a nice fluent API.)

Thank you for taking the time to answer that. I really enjoyed thinking about this one for a bit. Guess the issue it comes down to is that Java doesn't support a variable Number of generic parameters. Looking a bit further into it, I learned about C++ having "variadic templates" (which I guess could be the solution for such problems - not saying this should be part of Java though 😅). Seems I went a bit into the rabbit hole on that one, but it was fun and I learned something. And, after all, it makes me appreciate the work you guys have to put into constraint streams even more. Just could have implemented it in a way less boilerplatey but also way less usable way, so thanks for that 😃

Back to the topic at hand: I have a question regarding the expand function. Currently, none of the mapping parameters passed to it are marked with never null in the Javadoc, but it seems to me they should be. Should I add @NonNull annotations there or do you think it is possible or sensible that null is passed to that function?

Oct 09 '24 20:10 greyhairredbear

Back to the topic at hand: I have a question regarding the expand function. Currently, none of the mapping parameters passed to it are marked with never null in the Javadoc, but it seems to me they should be. Should I add @NonNull annotations there or do you think it is possible or sensible that null is passed to that function?

Well spotted! This is a clear omission. map() is a more generic form of expand(), and it does say never null.

Oct 10 '24 05:10 triceo

FYI The new APIs are likely to start landing next week [1]. Based on our earlier conversation, I recommend:

Bringing this PR to a state in which it can be merged. Conflicts resolved, no TODOs etc.
Bringing the remaining changes in a new PR shortly after that, no later than Nov 10.

If you think you won't be able to bring the remaining changes before Nov 10 (in time for the next release), I understand - my intention isn't to rush you. But it means I won't be able to merge your work now and will have to wait until the entire work is complete - that is likely to create new conflicts between now and then.

I'm going to trust you to make the right call here, based on your own schedule.

[1] https://github.com/TimefoldAI/timefold-solver/pull/1107

Oct 18 '24 06:10 triceo

Thanks for the heads-up!

If you think you won't be able to bring the remaining changes before Nov 10 (in time for the next release), I understand - my intention isn't to rush you. But it means I won't be able to merge your work now and will have to wait until the entire work is complete - that is likely to create new conflicts between now and then.

I looked through the files in the benchmark and test projects and also the config package, so I think I can commit to delivering the rest of the work before the next release. I guess setting my personal target to Nov 3rd makes sense.

Bringing this PR to a state in which it can be merged. Conflicts resolved, no TODOs etc.

I guess that's what I'll be doing tomorrow night then. I'll probably post comments on open TODOs, I left them there because I had questions. Four files with conflicts seems managable, I'll rebase and resolve them.

In case anything doesn't go as planned tomorrow, we could still reconsider.

Oct 19 '24 23:10 greyhairredbear

The revapi failures look like false positives, or bugs in revapi. Just add the ignores to the revapi configurations in core/src/build and move on.

Oct 22 '24 13:10 triceo

Wrt. Sonar - I went over the issues and it seems like most of them have already been in the code before, just never notified. I'll take care of it when merged, feel free to ignore.

Oct 22 '24 15:10 triceo

The integration test is broken upstream, unrelated to this PR. (Fix is already in review.)

Oct 22 '24 19:10 triceo

@greyhairredbear My other PR is ready to go and I need to merge it before the end of the week. Is there anything else you intend to do here before marking this as "Ready for review"?

(The actual review is already done.)

Oct 24 '24 05:10 triceo

@greyhairredbear My other PR is ready to go and I need to merge it before the end of the week. Is there anything else you intend to do here before marking this as "Ready for review"?

(The actual review is already done.)

Sorry, I thought this was waiting for #1149 to be completed - but I just saw some tests there are still failing because Java 21 is not supported.

I guess it's ok to merge this, so I'll undraft it. Please don't hesitate to contact me if there are any issues related to this, I'd be glad to help and don't want to block anybody.

I'll open a new PR with the remaining tasks and reference it from here once I start working on the remaining changes.

Oct 24 '24 06:10 greyhairredbear

Thank you, @greyhairredbear! I will merge this later today. As agreed, I'm looking forward to the rest of this great work on/before Nov 3rd!

Oct 24 '24 06:10 triceo