SteveDunn / Vogen

A semi-opinionated library which is a source generator and a code analyser. It Source generates Value Objects
Apache License 2.0
853 stars 45 forks source link

Vogen 4 System.Text.Json converters won't compile targeting < .NET 8 #619

Closed danielcweber closed 3 months ago

danielcweber commented 3 months ago

Describe the bug

This repo won't compile for .NET 7, 6 or 5.

The issue is that this overload of JsonSerializer.Serialize is not present on .NET 5,6 and 7 despite the documentation advertising otherwise. Besides, JsonSerializerOptions.GetTypeInfo is advertised to be available from .NET 6 on, but doesn't seem to be available until .NET 7.

Moreover, the overload that is matched from .NET 8 is not generic and suboptimal. An optimal generic overload could be matched but the result from GetTypeInfo would have to be cast to a higher type ("Returned metadata can be downcast to JsonTypeInfo<T> and used with the relevant JsonSerializer overloads.")

I'm wondering whether just calling this overload would already do in all cases. It's present on all platforms.

I'll happily open a PR with the mentioned fix (or any other suggested fix).

Steps to reproduce

See https://github.com/danielcweber/VogenRepro.

Expected behaviour

Code targeting .NET 7 or lower successfully compiles.

SteveDunn commented 3 months ago

Thanks for the bug report @danielcweber . Apologies for this regression (I'm assuming it is a regression). I'd be very happy to accept a PR! One thing you might find when running .\runsnapshots.ps1 is that hundreds/thousands/tens of thousands(!) of snapshot tests will fail due to differences in the expected and actual content of what is generated I generally do the fix, starting with a new test in GeneralTests, ensure that compiles, then run .\build.ps1, then ensure it works as expected in the Consumers.sln project, and then finish off by running the snapshot tests as described here.

I'll happily assist with the housekeeping once the fix is done.

Thanks again!