Nullable Reference Types for main library #2041
Conversation
The tooling caught the GeoPosition?[] => GeoPosition[] regression - excellent!
Revising what the user experience is like with NRTs here.
| } | ||
| return i % 2 == 0 ? null : Task.CompletedTask; | ||
| // TODO: What the freaking hell was this ever trying to do? | ||
| return i % 2 == 0 ? null! : Task.CompletedTask; |
There was a problem hiding this comment.
It's a test so... two scenarios for the price of one?? Maybe?
NickCraver
changed the title
|
@slorello89 I'd love thoughts here anywhere, but specifically on return types (e.g. from IDatabase and IServer and async interfaces) as to what's nullable and if any strike you as incorrect. I know that's a big ask, but if you had the time to scroll those and give thoughts on any tweaks, I'd appreciate it! |
This removed the parent bits and just accepts a null case, code before this was likely an iteration that just didn't get all the way to simplest.
In prep for more PRs and easier reviews, turning a lot of the language features on that I've addressed as part of NRTs but doing further here while having to review all changes anyhow. These are minor, but align on styles.
slorello89
left a comment
slorello89
left a comment
There was a problem hiding this comment.
Hey @NickCraver,
I went through the rest of what was in here, had a couple of questions about things that were null and a few other minor comments.
There is an awful lot of what looks like code cleanup changes in the Wrapper classes.
I scrolled through the entirety of IDatabase and IServer, for potentially missing threads to pull on, I only found a few inconsistencies in there (my questions in IServer are fleshed out in the other comments.
But a few things in IDatabase:
- KeyRandom can return a null key if Redis is empty
- It looks Like the HashEntry[] result for
HashGetAllMIGHT be null? - theParsemethod in the ResultProcessor - looks like it can pull back a null array. - Is the result of
IdentifyEndpointpotentially null?
Overall looks really good, obviously, there are questions as to whether or not this would be a minor or major version revision for the library. Putting that aside, I don't think I saw anything that I would insist on changing.
Also: think it's missing release notes :)
There are things we're removing completing in 3.0 which means actually binary breaks potentially, vs. people opting into warnings as errors on a 2.x release - decision is to go with a minor here and embrace warnings as warnings. If you want to hard break on warnings (like our existing `[Obsolete(error = false)]` additions), so be it.
Return an empty array instead (key is already the null indicator - an empty array is good here).
If we can't parse a response, properly throw here and go not nullable.
slorello89
left a comment
slorello89
left a comment
There was a problem hiding this comment.
1 weird comment that looks like it was probably an accidental copy/paste out of a different PR.
Otherwise, I think this looks pretty set.
I think calling this a Minor vs Major release is the right move, emitting warnings is fair in a minor IMO.
| return db; | ||
| } | ||
|
|
||
| /// <summary> |
There was a problem hiding this comment.
this comment seem like it was accidentally pasted?
There was a problem hiding this comment.
wooops, what the hell Nick?
mgravell
left a comment
mgravell
left a comment
There was a problem hiding this comment.
I've looked through this, focusing on the stuff under Interfaces (the primary public API), and it looks good to me. A few nice tweaks around Try etc. I'm not personally too concerned if we need to tweak some of the NRT annotations later, but: looks good to me. Nice job, sir.
| /// </summary> | ||
| public override string ToString() | ||
| { | ||
| if (toString != null) return toString; |
To help in reviewing this, I've provided an overview to listen along while reviewing.
This is exploring what NRTs (nullable reference types) would look like on the main library. This covers the migration + test migration (to get a feel for what users would see). Highlights are:
nullcould have happened before to keep it non-nullable (e.g. forFireAndForgeton a fetch).RedisKey/RedisValue/friends arestring?which changes a lot of(string)casts.IConvertibleisobjectand notobject?and soRedisValueandRedisResultare...just lying. They can be null. I have no idea why the decision on not-nullable is the way it is.IConvertible.ToType, lying again.[return: NotNullIfNotNull("field")]doesn't work like you'd think onTask<T>/Task<T?>, so we have 2 versions ofExecuteAsynctrees for nullable vs. non-nullable (e.g. non-nullable default value provided, like with the arrays mentioned above).where T : defaulthere. Don't ask for details. Because I don't really understand why..IsNullOrEmpty()and.IsNullOrWhiteSpace()extensions are just easier due to lack of annotations on full framework.TryGetException? Don't want to make the 99.9% success case more expensive though.GetBridge(message)!, which isn't null ifcreateistrue, so maybe break this into 2 methods with nocreatearg, one of which isn't nullable? (TryGetBridgeisn't great because this is most often chained).