Support for nullable reference types

[runceel]

• [x] Update csharp lang version to 10.0
• [x] Enable nullable
• [ ] Fix all errors and warnings
• [ ] Run all unit tests with all green results

Analyzer

• [ ] ReactivePropertySlim constructor
• [ ] ToReadOnlyReactivePropertySlim

[Sod-Almighty]

Any movement on this?

[runceel]

This was harder than I thought, so I was focusing other issues.
I will try it after releasing .net 6.

[runceel]

I have no idea to add nullability for that...

class ReactiveProperty<T>
{
    public T Value 
    { 
        get; 
        set; 
    }

    public ReactiveProperty() : this(default)
    {
    }

    public ReactiveProperty(T initialValue)
    {
        Value = initialValue;
    }
}

[soi013]

In My Opinion, There is no way to avoid big breaking changes.

class ReactiveProperty<T>
{
    public T Value { get; set; }

//    public ReactiveProperty() : this(default){}

    public ReactiveProperty(T initialValue)
    {
        Value = initialValue;
    }
}

usage

//public ReactiveProperty<string> RName { get; } = new(); //compile error
public ReactiveProperty<string> RName { get; } = new(string.Empty);
public ReactiveProperty<string?> RName { get; } = new(default(string));

[runceel]

@soi013 Thank you for the comment. I totally agree your suggestion. However, as you writing, it is a breaking change.

[soi013]

I considered another way.

How about using a partial nullable disablle and an Analyzer?

Here is the Analyzer repository and nupkg that I made as a demo.

https://github.com/soi013/ReactivePropertyAnalyzer/ https://github.com/soi013/ReactivePropertyAnalyzer/releases/tag/v1.0

This Analyzer also works with the current version of ReactiveProperty, and shows warnings where it is initialized with null.

Partial Disabling nullable

class ReactiveProperty<T>
{
    public T Value { get; set; }

#nullable disable 
    public ReactiveProperty() : this(default){}
#nullable restore

    public ReactiveProperty(T initialValue)
    {
        Value = initialValue;
    }
}

[runceel]

That's great!! This is a best way to add nullability support with keeping compatibility.

[xin9le]

I hadn't thought of that approach. It's very nice!!

[runceel]

@soi013 I cloned the repository, and I tried it. It is really interesting. I just added a few lines to improve the analyzer.

private void AnalyzeObjectCreationSyntax(SyntaxNodeAnalysisContext context)
{
    if (context.Node is not ObjectCreationExpressionSyntax invocationExpr)
        return;

    if (invocationExpr.Type is not GenericNameSyntax geneSynatx)
        return;

    if (geneSynatx.Identifier.ValueText is not nameof(ReactiveProperty) or nameof(ReadOnlyReactivePropertySlim))
        return;

    var typeGenerigArg = geneSynatx.TypeArgumentList.Arguments.FirstOrDefault();

    if (typeGenerigArg == null)
        return;

    var symbolGeneric = context.SemanticModel.GetSymbolInfo(typeGenerigArg).Symbol as INamedTypeSymbol;

    if (symbolGeneric == null) return;
    if (symbolGeneric.IsValueType != false) return;
    if (typeGenerigArg is NullableTypeSyntax) return; // add

    var arguments = invocationExpr.ArgumentList?.Arguments;

    if (arguments?.Count != 0)
        return;

    var diagnostic = Diagnostic.Create(Rule, invocationExpr.GetLocation(), invocationExpr.ToString());
    context.ReportDiagnostic(diagnostic);
}

private void AnalyzeImplicitObjectCreationSyntax(SyntaxNodeAnalysisContext context)
{
    if (context.Node is not ImplicitObjectCreationExpressionSyntax invocationExpr)
        return;

    var typeSymbol = context.SemanticModel.GetTypeInfo(invocationExpr, context.CancellationToken).Type;

    if (typeSymbol?.Name is not nameof(ReactiveProperty) or nameof(ReadOnlyReactivePropertySlim))
        return;

    var symbolGeneric = (typeSymbol as INamedTypeSymbol)?.TypeArguments.FirstOrDefault();

    if (symbolGeneric?.IsValueType != false)
        return;

    if (symbolGeneric.NullableAnnotation == NullableAnnotation.Annotated) return; // add

    var arguments = invocationExpr.ArgumentList?.Arguments;
    if (arguments?.Count != 0)
        return;

    var diagnostic = Diagnostic.Create(Rule, invocationExpr.GetLocation(), invocationExpr.ToString());
    context.ReportDiagnostic(diagnostic);
}

I succeeded detecting nullability.

I will work for supporting nullablity for ReactiveProperty. But it is still too hard. 😓

[soi013]

LGTM!

But how to analyze of ReadOnlyReactiveProperty ? I have not any idea.

var source1 = new ReactiveProperty<string>("initialValue");
var roName1 = source1.ToReadOnlyReactiveProperty<string>(); //.Value is not null

var source2 = new ReactiveProperty<string>("initialValue", mode: ReactivePropertyMode.None);
var roNameNull = source2.ToReadOnlyReactiveProperty<string>(); //.Value is null

var roName2 = source2.ToReadOnlyReactiveProperty<string>("initialValue"); //.Value is not null

[runceel]

😥

[runceel]

I also have not...

[runceel]

This task is not interesting for me. So, please expect delay for this. If anyone interest it, please feel free fork and pullrequest for this.

[gmkado]

@runceel any update on this one? Is it still low on your priority list?

[runceel]

@gmkado I had added nullable annotations for minimum necessary usecases. If you are facing warnings of nullable reference. Please let me know.

[gmkado]

Hi @runceel , thanks for responding. Should the following interactive script give me compiler warnings?

#r "nuget: ReactiveProperty, 9.1.2"
#nullable enable

using Reactive.Bindings;

public class Foo
{
	public IReadOnlyReactiveProperty<string> Bar => _bar;
	private readonly ReactivePropertySlim<string> _bar = new();  // <-- should this complain?
}

var x = new Foo();
x.Bar.Value.Contains("uh oh");

[runceel]

@gmkado This is one of the problems that I could not solve. Due to historical reasons, the ReactiveProperty class has a default constructor. However, when using this default constructor, the Value property becomes null in the case of reference types. Ideally, a warning should be issued, but I am not aware of a solution to address this issue. If you know of any solution, please let me know so that I can incorporate it.

[gmkado]

@runceel I've never worked with CodeAnalyzer's before, but could you add a similar rule to the invocation of ToReadOnlyReactiveProperty?

Alternatively, if you wanted to phase out the default constructor you could mark it as [Obsolete], with a message like "Provide an initial value to the constructor". You could overload the ToReadOnlyReactiveProperty extension method instead of having the optional initialValue and mark the one without the initialValue argument as [Obsolete] as well.

[runceel]

Thank you for your opinion. We had tried the analyzer solution, but there are a lot of patterns to detect nullable reference errors, so it was so difficult...

And I don't want to add [Obsolete] attribute for this. Because I don't want to write new ReactiveProperty<string?>(null);, I just want to write new ReactiveProperty<string?>();. Adding [Obsolete] attribute to the default constructor would make for a bad developer experience, I think.

I expect that the C# compiler will be improved in the future to detect more nullable warnings correctly.

[gmkado]

but there are a lot of patterns to detect nullable reference errors, so it was so difficult...

Are you referring to this?

If so, is there any harm in adding the analyzer that catches this scenario? It may not catch 100% of null references, but it would definitely help catch most of them.

Or are there more corner cases that you ran into that aren't in this thread?

[runceel]

@gmkado

Are you referring to https://github.com/runceel/ReactiveProperty/issues/241#issuecomment-1001372776?

Yes, it is.

Or are there more corner cases that you ran into that aren't in this thread?

I am not inclined to implement this as I would originally want the C# compiler to detect it. If someone can make a PR for this, I can merge it and publish it to NuGet.