Functional.Maybe
Functional.Maybe copied to clipboard
Complete implementation of NRT compatibility
Hi,
This is a PR that does, as I understand it, a complete handling of NRTs in Functional.Maybe. I closed the earlier PR thinking there is a better way to do it, but I now believe that for the principles I adopted, there isn't one.
The principles for the feature design that I adopted are:
-
T
inMaybe<T>
isnotnull
, otherwise users could createMaybe<string?>
, which:
- wouldn't make sense as Maybe is a replacement for
null
, not a container around it, - wouldn lead to "possible null" warnings when invoking
.Value
on such a maybe (which is especially problematic in generic code, which doesn't know if e.g.Maybe<T>
is aMaybe<string>
or aMaybe<string?>
).
- User should never have to suppress nullability warnings when invoking normal API features
- I consider suppressing NRT warnings a workaround, so IMO an API should not rely on it because the reader will later have difficulties between telling valid API usage from nasty workaround.
These two points led to changes some of which can be considered controversial:
- Many methods needed to be annotated with
notnull
. This is only a breaking change if someone has enabled NRT analysis and treats nullable warnings as errors. - In some methods, I added another generic parameter e.g. on OrElse, which is now
TResult OrElse<T, TResult>(this Maybe<T> a, Func<TResult> @default) where T : notnull, TResult
. This is because when done this way, a single method can handle both NRTs and non-NRTs. This is a breaking change, but only if someone explicitly specified the generic parameters, which, IMO, should be a rare case. - In other methods, like
Lookup
, a single method could not handle both NRTs and non-NRTs. Going further with the example ofLookup
, I couldn't find a way (and I'd argue it doesn't exist) to pass bothDictionary<string, string>
andDictionary<string, string?>
to the same method without warnings. This is because the following code gives warnings:Dictionary<string, string?> d = new Dictionary<string, string>()
. In these cases, I really had no choice but to create two methods, one forDictionary<string, string>
and one forDictionary<string, string?>
. In cases like this one, I decided to name the "more NRT-friendly" version with a "Nrt" suffix as I assumed that the "non-NRT" versions will be used more often. These additional methods aren't what I wanted to end up with, but I see no other way - overloads don't work in this case, becauseT
andT?
aren't different types. There are total of 6 methods that required this kind of addition and I regret that I couldn't do it another way (though I tried, see https://stackoverflow.com/questions/68551157/c-sharp-nullable-reference-types-can-this-function-accept-a-dictionary-with-bot and https://twitter.com/GGalezowski/status/1420108618536472577).
I welcome feedback if you don't like the design decisions I made for this PR.
I also added some unit tests around the areas where I wasn't sure how they will work with NRTs. This isn't by any means a complete coverage, just the cases I needed to get the feedback while modifying the code. Also, I did not know which naming convention to follow for the test cases, so I kind of guessed. Any feedback is welcome. My incertainty about conventions applies to indentation as well. I didn't know which to follow as there are multiple conventions in use, sometimes even in a single file. Again, let me know what you prefer and I am ready to change it.
By the way, apologies for the size of this PR. I could not find a way to make it smaller.
Just noticed that I could turn the *Nrt
methods into overloads, but that would be at the expense of additional generic parameter that could not be inferred.
So instead of: dictionary.LookupNrt("a")
, I could have dictionary.Lookup<string, string, string?>("a")
. The first generic arg is for the key type, the second for the resulting maybe type and the third for dictionary value type. The implementation would then involve a MaybeCast
from string
to string?
.
I now think that this might be a better way than using an arbitrary convention - users would be able to find the overload more easily and if they don't want to type all the generic parameters every time, they could just write their own extension methods over the overload.
As mentioned, changed the proposed code to remove the *Nrt methods and instead introduced new overloads that can be used to either convert from nullable to non-nullable or attempt at downcasting (which was not the point, but doesn't hurt, I even added two unit tests to show that). The users of the library can recreate the *Nrt methods easily (and apply their own naming conventions if they wish), for example, the old version of Wrap
is currently defined using the new overload like this:
public static Func<TK, Maybe<TR>> Wrap<TK, TR>(TryGet<TK, TR> tryer)
where TR : notnull => Wrap<TK, TR, TR>(tryer);
and a version compatible with NRTs can be defined also using the new overload as sth. like this:
public static Func<TK, Maybe<TR>> WrapNrt<TK, TR>(TryGet<TK, TR?> tryer)
where TR : notnull => Wrap<TK, TR?, TR>(tryer);
The code is ready for review. I will be happy to answer questions. If you don't want this feature or this scope of feature, please reject.
Hi, any news on this one?
Hi, any luck reviewing this?
Hi, any news?