A question about the new nullable enable and [MaybeNullWhen(false)]

[StefH]

Hi,

I see that you have added/changed some methods like:

bool TryResolveHelper(PathInfo name, Type? targetType, [MaybeNullWhen(false)] out IHelperDescriptor<HelperOptions> helper);

May personal preference, and also the way Microsoft defines the Try methods is to change the signature to:

bool TryResolveHelper(PathInfo name, Type? targetType, [NotNullWhen(true)] out IHelperDescriptor<HelperOptions>? helper);

Did you consider this?
And can you explain your choice?

[rexm]

Thanks for catching this - it was a miss on my part. I saw it followed the Dictionary-style TryGet contract and didn't think hard enough about it. What do you suggest? This version has only been out for a few hours.

[StefH]

I suggest to fix it. And double check all other code which involves TryGet pattern.

And it's not a critical change, so I guess it's not needed to unlist the nuget packages.

[TheConstructor]

Hey @StefH, the PR was mine, so I guess I put my two cents here: for the most part [MaybeNullWhen(false)] and [NotNullWhen(true)] is the same, and I don't mind, if it get's changed now or in the future.

That being said Dictionary<TKey, TValue> couldn't use [NotNullWhen(true)] as null might be a valid value for TValue. If it's not valid for TValue, you effectively get [NotNullWhen(true)]. Now looking just add IHelperResolver.TryResolveHelper you can choose either attribute, as TValue would be IHelperDescriptor<HelperOptions> - if you backed it by an Dictionary<TKey, TValue>, but I liked [MaybeNullWhen(false)] better, as it just forwards the underlying guarantee.

I also feel like [MaybeNullWhen(false)] out IHelperDescriptor<HelperOptions> better communicates the expected result. We expect that most of the times, when we actually call this, the user was right and there is a helper. But we want to gracefully handle the case, were there is no helper.

Again: it's up to you, but I would consider it just a boring change, not a fix unless I overlooked something.