Generator::currentElementAsString breaks compilation of a lot of our tests

[stefanhaller]

In 48f3226, a new method currentElementAsString was added to generators. As far as I can tell, it isn't used for anything.

The problem is that this breaks compilation of a lot of our tests, with no obvious workaround. The reason is that we define CATCH_CONFIG_FALLBACK_STRINGIFIER to a non-existing symbol in order to catch mistakes where failing CHECKs wouldn't stringify correctly. This is very useful, and actually I think it should be the default.

But this now causes compilation failures for GENERATE statements where the type that's generated isn't stringifiable.

To illustrate, here's a common pattern that we tend to use a lot in our tests:

using T = std::tuple<InputType1, InputType2, SomeHelper, ExpectedOutput1, ExpectedOutput2>;
const auto [input1, input2, helper, expectedOutput1, expectedOutput2] = GENERATE(values<T>({
  { ... },
}));

CAPTURE(input1, input2);

// ...

In this example, InputType1 and InputType2 are stringifiable, and have to be because of the CAPTURE. SomeHelper might, for example, be a std::function. ExpectedOutput1 and ExpectedOutput2 only need to be stringifiable if they appear in a CHECK statement, which they often do, but don't necessarily have to.

I'd appreciate advice how to work around this. I suppose I could subclass FixedValuesGenerator with an overwritten stringifyImpl method (which would only assert(false)?), but this seems like a lot of work.

For now we revert 48f3226 locally, but this is not a satisfactory long-term solution either, of course.

[horenmar]

That's an interesting use case for CATCH_CONFIG_FALLBACK_STRINGIFIER, but one that I am not convinced should be supported.

The reason for currentElementAsString() (and also currentElementIndex() added at about the same time) is to eventually allow the reporters to also include current generator state (either as element index, or as element string) in their output. This requires the ability to stringify the generator's elements, and Detail::stringify and the related machinery is how Catch2 does that.

[stefanhaller]

That's an interesting use case for CATCH_CONFIG_FALLBACK_STRINGIFIER, but one that I am not convinced should be supported.

It has saved us so many times that I wouldn't want to live without it.

The problem with stringification is that it can break so easily. Maybe through a refactoring, a header that defines operator<<(ostream) for a given type is no longer being included. Nobody notices this, until a test fails (maybe months later) and shows only question marks in its output. In our case, compilation fails immediately, which is extremely helpful.

Not to mention the problem of ODR violations, where forgetting an include in one compilation unit can break stringification in all other compilation units too. This can be extremely difficult to diagnose, and again this is nicely solved in our setup.

The reason for currentElementAsString() (and also currentElementIndex() added at about the same time) is to eventually allow the reporters to also include current generator state (either as element index, or as element string) in their output. This requires the ability to stringify the generator's elements, and Detail::stringify and the related machinery is how Catch2 does that.

Yes, I understand all that, and it would indeed be nice if reporting for generater-based tests would have more information about which row is failing. However, I'm not convinced that stringifying the entire generator element is the right solution. As shown in my example above, generator elements can often consist of a mixture of inputs and expected outputs (and sometimes even other unrelated stuff), and it's really only the inputs that are required to identify the generator row. The current crutch of using CAPTURE to approximate the feature is a lot less nice, but at least it gives us the flexibility to decide what should take part in identifying the element.

[stefanhaller]

I'd be interested in suggestions how to work around the issue (apart from "don't abuse CATCH_CONFIG_FALLBACK_STRINGIFIER like this" -- we definitely want to keep the behavior of getting compilation failures when types can't be stringified).

Subclassing FixedValuesGenerator and overriding stringifyImpl, as I originally thought, doesn't help of course, because even though at runtime our overridden method would be chosen, the compiler still has to compile the base class version in IGenerator, and this still fails.

I experimented with using a custom tuple type that has a custom StringMaker which only prints the elements that can be stringified, and prints questions marks for the rest. This sort of works, but there's no real way to determine at compile time whether a type can be stringified, so I can only approximate this using IsStreamInsertable, which is not really good enough (e.g. I get question marks for tuple elements that are vectors, even though they would be stringifiable). I might decide to live with this for now, since currentElementAsString isn't used for anything anyway, but the whole situation is highly unsatisfactory.

---

I'd also be interested in thoughts about my previous comment, regarding the approach of stringifying the entire generator element to identify it. I find this problematic for the given reasons, and I think it would be worth rethinking the approach. For example, maybe there could be a way to attach description strings to generator elements (somehow), and reporters would use those if available, or fallback to the index if not.

[kdu-ableton]

That's an interesting use case for CATCH_CONFIG_FALLBACK_STRINGIFIER, but one that I am not convinced should be supported.

Maybe an alternative could be to offer a more straightforward solution to allow compilation to fail if a type cannot be stringified. And if that solution supports somehow checking the context (e.g. a non-stringifiable type used in a CHECK macro should break compilation vs. a non-stringifiable type used in a GENERATOR expression should compile and print ???), then @stefanhaller's problem would be solved.

[horenmar]

I gave this some thought for #2509. I still believe that the ability to print out stringified generator states is too valuable to give up, so, here are two options I see for this (apart from ignoring it, because this is not the intended use of fallback stringifier 😃).

1. Make the IGenerator<T>::stringifyImpl use sfinae to decide whether it should call into ::Catch::Detail::stringify, or return an alternative info string. This is relatively clean, the downside is extra compilation cost, and that the fallback stringifier needs to be sfinae friendly, which I'd expect is a breaking change for most users.

2. Add compile-time configuration option, that changes whether IGenerator<T>::stringifyImpl calls to stringify, or returns alternative info string (empty string or something that is obviously-enough a placeholder). This adds another compile-time configuration, which is annoying due to their higher maintenance cost, but it is not a breaking change for anyone.

[stefanhaller]

Thanks for looking into this! We solved our problem a while ago by actually implementing all the missing stringifiers for all the types we use in GENERATE statements, so it's no longer a pressing issue for us. I'd be ok with simply closing this issue for now.

Depending on how #2509 turns out, the biggest issue might not be getting everything to compile at all, but output that is too noisy. In the example from the issue description above, only input1 and input2 need to be part of the output (it's the only things we CAPTURE); but the other three tuple elements will also be printed, that's just unnecessary noise. Of course we can fix this by replacing the tuple with a struct with an appropriate stringifier, but the tuple pattern is so damn convenient. 😄

[horenmar]

the biggest issue might not be getting everything to compile at all, but output that is too noisy.

Agreed, I intend the default output to be just the "indices" of active generators, but let the users ask for full stringification. Luckily, there is now a way to pass options to individual reporters, which this option can use.