Investigation in Scala2/Scala3 decoder inconsistency

[GerretS]

@alycklama and I noticed in #436 that certain built-in decoders have inconsistent behaviour between Scala 2 and 3.

Since this inconsistency is unrelated to our PR, we decided to make a separate issue to investigate this.

I think it is related to the Shapeless types used in the MapShapedCsvRowDecoder, since Shapeless makes heavy use of Macros which have changed in Scala 3. However, that is where my knowledge of Shapeless ends.

This PR adds a few unit tests that demonstrate the problem. They succeed on Scala 2 and fail on Scala 3.

[alycklama]

I created a very simple case class OneColumn(name: String) to see if I can figure out what's going on. A bit of debugging into the code made me end-up at this point:

IndexedStateT:

  def run(initial: SA)(implicit F: FlatMap[F]): F[(SB, A)] =
    F.flatMap(runF)(f => f(initial))

initial is (name,None), which means that the column wasn't found. So far all is well.

However, when applying f on initial, the result is Right((List(),OneColumn())).

I would have expected a Left here, as the header is None. As I'm not familiar in how to debug this further, I hope this info is going to be useful!

[ybasket]

@GerretS @alycklama Thank you both for the investigation and debugging, your work has indeed proven useful for fixing!

I just pushed some commits that

• fix the problem of not failing for missing columns
• rewrite the corresponding test case to avoid unsafe effect running

It still fails for the test case with default value, but at least with correct error message. The underlying problem is more complex, the Scala 3 compiler for long did not allow to get default parameter values and AFAIK still has terrible UX nowadays as in you need -Yretain-tree, see softwaremill/magnolia#284. shapeless3 doesn't have support for it (yet), so probably the right™️ way would be to contribute it there and then use it here in fs2-data. If you scroll up in CsvRowDecoderTest, you'll see two test cases being commented out that would cover defaults. Do you need defaults in your usage of fs2-data or was it just something you discovered?

As the functionality of the test cases I didn't rewrite is more concisely covered by the existing test cases, I would remove those and then merge this PR – if that's fine for you?

[GerretS]

Thanks for the fixes!

No, I don't think we'll need support for defaults, so we can go forward for now. I only noticed that situation by accident. It was the different handling of missing values that was causing trouble for us.

Feel free to clean up my test cases if they're already covered elsewhere, I didn't put too much thought into them other than to quickly demonstrate the issue.

[satabin]

Thanks for the bug hunt and the fix!

[satabin]

Do not merge right now, doing some work to rebase it.

[satabin]

Ok this can now be merged. Thanks again.