SteveDunn / Vogen

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

Nullable references are not enable in generated files #662

Closed DPDmancul closed 5 days ago

DPDmancul commented 2 weeks ago

Describe the bug

C# does not observe global nullable settings inside generated files, thus generated files without #nullable enable do not use nullable reference types. This, for example, causes the From factory to silently accept a null value without any compile time errors, postponing the error discovery only a runtime!

Steps to reproduce

[Vogen<string>]
public readonly partial struct MyVogenType;

MyVogenType.From(null);

Expected behaviour

This should not compile, but instead it compiles and throws at runtime.

This issue is easily solved by adding #nullable enable at the beginning of generated sources

SteveDunn commented 2 weeks ago

Many thanks for the bug report @DPDmancul . I'll take a look at this soon.

SteveDunn commented 1 week ago

I'll hopefully have something ready tomorrow. I'll need to bump up the major version as it's a breaking change.

SteveDunn commented 6 days ago

@DPDmancul - this is now in version 5.0.0 (beta 1). Would you mind giving it a try to see if it works as expected?

DPDmancul commented 5 days ago

Yes it now gives compilation error as expected, thanks!

PS: I don't know if it is a real use case scenario, but I noticed it is impossible to wrap a nullable reference type:

[Vogen<Guid>]
public readonly partial struct MyGuid;

MyGuid.From(null); // this correctly won't compile

[Vogen<Guid?>]
public readonly partial struct MyNullableGuid;

MyNullableGuid.From(null); // this compiles fine

[Vogen<string>]
public readonly partial struct MyString;

MyString.From(null); // now this correctly won't compile (previously, incorrectly, would have compiled)

[Vogen<string?>] // here we got error because of C# limitations on nullable reference types
public readonly partial struct MyNullableString;

[Vogen(typeof(string?))] // this fails for the same reason
public readonly partial struct MyNullableString;

Before this bugfix one could have used [Vogen<string>] to wrap a nullable string (since in c# string? = string), but now it could no more be don. This saves us from terrible nasty bugs in the case we want a real string, but also prevent us to wrap a nullable string (without using other wrappers, e.g. Option).

I found a workaround for this, but maybe there is a better solution:

#nullable disable
[ValueObject<string>]
public partial class MyNullableString;
#nullable restore

MyNullableString.From(null);
SteveDunn commented 5 days ago

Thanks for trying it out @DPDmancul . It's an interesting scenario regarding wrapping nullable reference types. In my opinion, Vogen's primary purpose is to avoid nulls and strongly type things that were previously expressed as primitives (and/or nulls).

I've seen one other issue that was reported, and I think that was an issue relating to a nullable value type.

I still think that the best alternative is an Option/Maybe. Hopefully with discriminated unions comping up in C#, things like this will be easier to manage.

Thanks again for your feedback. I'll close this issue, but wait a week or so before releasing, just in case there's any reported issues.