reactiveui / ReactiveUI.SourceGenerators

Use source generators to generate objects.
MIT License
31 stars 3 forks source link

[Bug]: ObservableAsProperty attribute does not respect nullability annotations for reference types #124

Closed PremekTill closed 2 weeks ago

PremekTill commented 2 weeks ago

Describe the bug 🐞

When using the [ObservableAsProperty] attribute with an Observable which transmits nullable reference types, the backing field and public property are incorrectly generated without the nullability annotations.

Step to reproduce

When using Observables which pass nullable values, in some cases, the generator drops the annotations. Consider the following combinations:

[ObservableAsProperty]
private IObservable<int> ValueTypeObservable { get; }

[ObservableAsProperty]
private IObservable<int?> NullableValueTypeObservable { get; }

[ObservableAsProperty]
private IObservable<object> ReferenceTypeObservable { get; }

[ObservableAsProperty]
private IObservable<object?> NullableReferenceTypeObservable { get; }

These will result in the following backing fields:

private int _valueTypeObservableProperty;
private ReactiveUI.ObservableAsPropertyHelper<int>? _valueTypeObservablePropertyHelper;

private int? _nullableValueTypeObservableProperty;
private ReactiveUI.ObservableAsPropertyHelper<int?>? _nullableValueTypeObservablePropertyHelper;

private object _referenceTypeObservableProperty;
private ReactiveUI.ObservableAsPropertyHelper<object>? _referenceTypeObservablePropertyHelper;

private object _nullableReferenceTypeObservableProperty;
private ReactiveUI.ObservableAsPropertyHelper<object>? _nullableReferenceTypeObservablePropertyHelper;

public object NullableReferenceTypeObservableProperty { get => _nullableReferenceTypeObservableProperty = _nullableReferenceTypeObservablePropertyHelper?.Value ?? _nullableReferenceTypeObservableProperty; }

With the non-nullable value type (int), a non-nullable field is correctly generated. It is implicitly populated with the default value and doesn't cause any issues.

Similarly, with the nullable value type (int?), a nullable field is correctly generated. It is implicitly populated with null and doesn't cause any issues.

With the non-nullable reference type (object), a non-nullable field is correctly generated. It is implicitly populated with null and causes a "CS8618 Non-nullable variable must contain a value" warning, which is to be expected, since the generator doesn't have any value to populate it with and the object might not be trivially constructible.

However, with the nullable reference type (object?), a non-nullable field is incorrectly generated. This breaks the nullability contract and causes a CS8618 warning to be emitted, even though null should be a valid value. The same is true for the generated public property, which breaks nullability analysis wherever the property is used.

The generated warnings can be silenced by doing this inside the user-code constructor:

_referenceTypeObservableProperty = new(); // Or any other value which we want to initialize with.
_nullableReferenceTypeObservableProperty = null!;

The non-nullable version makes sense, since we have to actually give the compiler a valid value to initialize with. It might be nice to include an optional DefaultValue property on the attribute which would do this without having to explicitly reference the auto-generated field, but that would be just a nice-to-have syntactic sugar.

For the nullable version, the correct behaviour would be simply generating an object? field and property, which would remove the warnings entirely. Tricking the nullability analyser by passing null! does remove the warning, but it is fairly hacky and unintuitive.

Reproduction repository

No response

Expected behavior

The backing field and property should be generated with the correct annotation.

Screenshots 🖼️

No response

IDE

Rider Windows

Operating system

No response

Version

2.0.9

Device

No response

ReactiveUI Version

20.1.63

Additional information ℹ️

As a small side-note, when looking at the generated _<propertyName>ObservablePropertyHelper backing fields, these are generated as nullable and always set in the InitializeOAPH() call, which must be made from the constructor for the generator to have a defined behaviour.

It might be possible to generate these as non-nullable required fields instead, which would both lead to improved nullability annotations (since these should never be actually null) and better compiler analytics (since instead of relying on the user calling InitializeOAPH() when needed and otherwise failing at run time, the compiler could enforce its usage at compile time). This is however just an idea, I don't know enough about the internals of source generators to know about its feasability.

github-actions[bot] commented 1 day ago

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.