Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MVUX] Better support for nullability annotations #1605

Closed
5 of 6 tasks
dr1rrb opened this issue Jun 29, 2023 · 4 comments · Fixed by #2080
Closed
5 of 6 tasks

[MVUX] Better support for nullability annotations #1605

dr1rrb opened this issue Jun 29, 2023 · 4 comments · Fixed by #2080
Labels
kind/enhancement New feature or request.

Comments

@dr1rrb
Copy link
Member

dr1rrb commented Jun 29, 2023

What would you like to be added:

Feed.Async<T> where T : notnull should accept an AsyncFunc<T?> and automatically interpret a null as None.

Same for other operators like .Select, and .SelectAsync.

Why is this needed:

The language now have the nullable annotations which are commonly used by devs. Doing such would improve user experience and help to use the None state which is often not used.

For which Platform:

  • iOS
  • Android
  • WebAssembly
  • WebAssembly renders for Xamarin.Forms
  • Windows
  • Build tasks

Anything else we need to know?

  • The Option<T> should be hidden from "first level public API" (could be usable only with "advanced" operators)
  • We already have such support for State.Update
  • This could be a breaking change (compile time)
@dr1rrb dr1rrb added kind/enhancement New feature or request. triage/untriaged Indicates an issue requires triaging or verification. labels Jun 29, 2023
@nickrandolph nickrandolph removed the triage/untriaged Indicates an issue requires triaging or verification. label Nov 10, 2023
@nickrandolph
Copy link
Contributor

@dr1rrb is this something we can get done in 5.1? If so, can you update the release number

@dr1rrb
Copy link
Member Author

dr1rrb commented Nov 16, 2023

About "factories", adding a notnull type constraint would drive to :

// Confusion possible between cases 3 and 7
var int1 = Feed.Async(_ => default(int)); // => Some(0)
var int2 = Feed.Async(_ => 42); // => Some(42)
var int3  = Feed.Async(_ => default(int?)); // ==> None // currently: Some(null)
var int4 = Feed.Async(_ => 42); // => Some(42)
var int5 = Feed<int>.Async(_ => default(int)); // => Some(0)
var int6 = Feed<int>.Async(_ => 42); // => Some(42)
var int7 = Feed<int?>.Async(_ => default(int?)); // => Some(null)
var int8 = Feed<int?>.Async(_ => 42); // => Some(42)

// Confusion possible between cases 3 and 7
var struct1 = Feed.Async(_ => default(MyStruct)); // => Some(0)
var struct2 = Feed.Async(_ => new MyStruct(42)); // => Some(42)
var struct3 = Feed.Async(_ => default(MyStruct?));  // ==> None // currently: Some(null)
var struct4 = Feed.Async(_ => new MyStruct(42)); // => Some(42)
var struct5 = Feed<MyStruct>.Async(_ => default(MyStruct)); // => Some(0)
var struct6 = Feed<MyStruct>.Async(_ => new MyStruct(42)); // => Some(42)
var struct7 = Feed<MyStruct?>.Async(_ => default(MyStruct?)); // => Some(null)
var struct8 = Feed<MyStruct?>.Async(_ => new MyStruct(42)); // => Some(42)

// Confusion possible between cases 1,3 and 7
var str1 = Feed.Async(_ => default(string)); // => None  // currently: Some(null)
var str2 = Feed.Async(_ => "42"); // => Some("42")
var str3 = Feed.Async(_ => default(string?)); // => None  // currently: Some(null)
var str4 = Feed.Async(_ => "42"); // => Some("42")
// var str5 = Feed<string>.Async(_ => default(string)); // compilation issue
var str6 = Feed<string>.Async(_ => "42"); // => Some("42")
var str7 = Feed<string?>.Async(_ => default(string?)); // => Some(null)
var str8 = Feed<string?>.Async(_ => "42"); // => Some("42")

// Confusion possible between cases 1,3 and 7
var obj1 = Feed.Async(_ => default(MyObject)); // => None  // currently: Some(null)
var obj2 = Feed.Async(_ => new MyObject(42)); // => Some(42)
var obj3 = Feed.Async(_ => default(MyObject?)); // => None  // currently: Some(null)
var obj4 = Feed.Async(_ => new MyObject(42)); // => Some(42)
// var obj5 = Feed<MyObject>.Async(_ => default(MyObject)); // compilation issue
var obj6 = Feed<MyObject>.Async(_ => new MyObject(42)); // => Some(42)
var obj7 = Feed<MyObject?>.Async(_ => default(MyObject?)); // => Some(null)
var obj8 = Feed<MyObject?>.Async(_ => new MyObject(42)); // => Some(42)

Proposed changed

The common issue is with the case 7, so we propose to change the current behavior also in it to return a None if value is null, a.k.a. "null as None".

  • This breaks the current behavior where null is never interpreted in anyway in Feed and State, but would match the behavior of ListFeed (which already implement "null or empty as None")
  • If user doesn't want None, they can use Feed<T>.Async(_ => Option<T>.None()) which is already available
  • This semantic would be present only for "entry points" (a.k.a. "factories") of reactive framework (i.e. Feed.Async[Enumerable])
  • [BREAKING] This will change returned type of Feed.Async(_ => default(T?)) from Feed<T?> to Feed<T> (compile time).
  • New behavior could be disabled using a feature flag (new behavior enabled by default) ==> still breaks compilation, but restoring previous behavior would only require to add type in feed factories that allow null from Feed.Async to Feed<T?>.Async

Note

Feed of value-types are usually only used for binding purposes where anyway None is converted to default(T) for the view.

@dr1rrb
Copy link
Member Author

dr1rrb commented Nov 16, 2023

When we look at "operators", we have 2 cases to consider:

  1. The input type parameter (Feed.Select; Feed.SelectAsync; Feed.Where; State.Update; State.ForEachAsync; GetAwaiter)
  2. The return type (Feed.Select; Feed.SelectAsync; State.Update)

Case 1 could be named "None as null", while case 2 is "null as None".

Proposal

To match the "new behavior" suggested for "factories", we should also follow the "null as None". This means that we should constraint the "return type" on all operators to notnull and convert the null to None (still following the feature flag).

In order to allow users to return Some(null) we would also have to add .SelectData(Func<Option<TSource>, Option<TResult>>) (using the Data suffix like we already have to State.Data), SelectDataAsync and WhereData.

However we should NOT implement the "None as null", this is to avoid to implicitly convert None(struct) to Some(default(struct)) (e.g. having the "None as null" would allow Feed<int>.Select(i => i + 1) which would implicitly convert None<int>() to Some(0))

Special cases of imperative operators

Currently Feed.GetAwaiter, State.Update and State.ForEachAsync are already constraints to notnull on "input parameter".

State.Update

Input and output types are the same, implementing the "null as None" somehow forces us to also implement the "None as null".

This is not as problematic as for Feed.Select as the Update is an imperative operation where we anyway expect to change the current value. Getting a default instead of None is actually the expected behavior.

We suggest to keep it as-is (i.e. both "None as null" and "null as None")

State.ForEach

Same here, this is designed to execute imperative code, converting None to default is somehow the expected behavior. We should however add an ForEachDataAsync in order to allow user to make distinction between default and None is needed.

We suggest to keep it as-is (i.e. both "None as null") and add ForEachDataAsync

Feed.GetAwaiter

Same here, this is designed to execute imperative code, converting None to default is somehow the expected behavior. User can already do await feed.Data() to get an Option<T>.

We suggest to keep it as-is (i.e. both "None as null") and add ForEachDataAsync

Examples

IState<int> intState = null!;
await intState.Update(v => v + 1); // Dangerous case: user will get 0 in case of None
await intState.Update(i => default(int?));
var intValue = await intState; // int // Dangerous case: user will get 0 in case of None

IState<MyStruct> structState = null!;
await structState.Update(v => v with { Value = 43 }); // Dangerous case: user will get default(MyStruct) in case of None
await structState.Update(v => default(MyStruct?));
var structValue = await structState; // default(MyStruct) // Dangerous case: user will get default(MyStruct) in case of None

IState<string> strState = null!;
await strState.Update(v => (v ?? "").ToString());
await strState.Update(v => default(string?));
var strValue = await strState; // string?

IState<MyObject> objectState = null!;
await objectState.Update(v => (v ?? new(42)) with { Value = 43 });
await objectState.Update(v => default(MyObject?));
var objectValue = await objectState; //MyObject?

@dr1rrb
Copy link
Member Author

dr1rrb commented Nov 16, 2023

To summarized the suggested changes:

  • None as null (parameter value): Only for imperative operators (State.Update; State.ForEachAsync; Feed.GetAwaiter)
  • null as None (return value): Everywhere

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement New feature or request.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants