Nullability annotations for System.Data

[roji]

Following ec73c56.

• No (intentional) behavioral changes have been introduced.
• To avoid making this huge PR even bigger, I've deliberately refrained from cleaning up/simplifying code as much as possible
• I've concentrated especially on the public surface: private/internal areas may contain some imprecise annotating. This shouldn't have an impact - we can always come back and improve these if we want.
• The APIs are generally very lax about nulls, accepting them (and doing nothing) in places where they don't really make sense (this is inconsistent across the codebase). I've stuck to annotating the current behavior - if a method accepts null (e.g. and does nothing), that parameter is annotated as nullable.
• I've found various bugs; lots of simple cases where checks are missing and an NRE would be thrown, some other suspicious cases for actual bugs. I've made some notes, but as this is almost entirely legacy we should probably be reactive here and wait for bug reports to be filed.
• A few corners have been left unannotated because of dependencies.

Part of #2339

[roji]

The following 0 count means the array never gets enumerated anyway

[roji]

/cc @ajcvickers @jeffhandley

[stephentoub]

I see call sites for some of these virtuals checking for and allowing null. Is non-nullable correct for the return value for all of these?

[roji]

The default implementation coalesces to string.Empty, and it's supposed to be a string you can always concatenate before/after the string. It's true that it's virtual so anything's possible, though I'm only seeing IsNullOrEmpty in the Odbc and OleDb providers, and SqlClient just calls base.QuotePrefix.

Where are you seeing null checks, anything persuasive? I can make it nullable if you think that's safer.

[stephentoub]

Where are you seeing null checks

The IsNullOrEmpty checks you mentioned. But it's not just in Odbc and OleDb, e.g. DBCommandBuilder allows null in both private void BuildInformation(DataTable schemaTable) and private string QuotedColumn(string column).

[roji]

OK, yeah - I see them though they're also IsNullOrEmpty. I think the point is to check for length 0 rather than for null (the coalescing behavior of the property getters is a good indication), but if you think we should annotate these as nullable I can do that. We could also do a more thorough search outside the BCL to see who's overriding DbCommandBuilder and what they're doing, let me know.

[stephentoub]

I only spot checked and I'm exhausted; thanks for slogging through this.

[ghost]

Tagging subscribers to this area: @roji, @ajcvickers
Notify danmosemsft if you want to be subscribed.

[roji]

@stephentoub have merged this to be able to continue working on Odbc etc., thanks for taking the time to review this (it was most definitely a slog).

If there are any outstanding points (e.g. #38810 (comment)) let me know and I'll do fix-up to this in a later PR.