test: Constrain optional<T> operator<< by streamability #35625

Should be a relatively uncontroversial change from #35587. Without this constraint, an operator<<(std::ostream&, const std::optional<T>&) overload is selectable for any T, even when T itself has no operator<<

<!--e57a25ab6845829454e8d69fc972939a-->

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

<!--006a51241073e994b41acfe9ec718e94-->

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35625.

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #31260 (scripted-diff: Type-safe settings retrieval by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

<!--5faf32d7da4f0f540f40219e4f7537a3-->

Seems fine, because this is the right thing to do if this were a public library function. However, here it isn't, so the only change is the compiler error message?

In fact, I'd argue that the current compiler error message is preferable, because it directly declares the reason of the exact compile failure with a single error, as opposed to a list of non-matching functions.

My recommendation would be to leave this as-is

How does this make the changes in #35587 easier to achieve?

The stringify method uses osstream if available, which is the case for optional<T>, but fails to compile because T itself is not osstream: https://github.com/bitcoin/bitcoin/pull/35587/changes/6b701fb9f1009f29f3bae80f52395354966e004a#diff-4f90cd1cc1f04c6d9d08e11c8a63e4fb3835642beb3a02a8453b91346e28bf00R117 line 117

I don't think the silent compile-time fallback to return "<unknown type>"; makes sense.

Boost doesn't have that either, and for a good reason, I'd say:

Imagine a unit tests that passes when written and when merged. However, it intermittently fails 1 out of 10'000 times. The failure would then just print:

<unknown type> != <unknown type>

Which is close in usefulness to having no failure output at all.

Again, it would be better to leave this as-is and keep the behavior as-is, so that a proper and well-understood compile-time failure is raised before a test that compares new types even has a chance to run, and before any reviewers spend time on reviewing such a test.

Sure, will fix in #35587