Annotate System.Data.{Odbc,OleDb} for nullability

[roji]

• As before, no behavior changes were done, only annotations (but see comments below)
• Nullability warnings are disabled for OleDb, see https://github.com/roji/runtime/tree/SystemDataOleDbBuildHack for a csproj hack to make these appear
• Unfortunately there don't seem to be any warnings about discrepancies between implementation (src) and interface (ref). I did the best I could to keep them in sync, but it would be better to have automatic verification.
• Also marked System.Data.DataSetExtensions as annotated - it's a facade with no actual types.

/cc @jeffhandley @ajcvickers

[ghost]

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

[roji]

@stephentoub @jeffhandley the build failures seem to be because System.Data.Odbc is being built for older TFMs (netfx, netstandard2.0) where nothing is annotated, so Debug.Assert isn't recognized (when building locally, presumably for .NET Core, there are no errors/warnings). Nullability warnings/errors should probably be disabled for older TFMs here, right?

[jeffhandley]

Nullability warnings/errors should probably be disabled for older TFMs here, right?

I'm not sure myself. @ericstj, do you know?

[jeffhandley]

I'm at 37/200 files viewed. So far so good. And thanks for fixing the spelling errors in comments!

[ericstj]

Nullability warnings/errors should probably be disabled for older TFMs here, right?

I'm not sure myself. @ericstj, do you know

Looks to me like folks are suppressing the warnings on other frameworks:

runtime/src/libraries/Directory.Build.targets

Lines 9 to 10 in 3f7202b

	<NoWarn Condition="'$(TargetFrameworkIdentifier)' == '.NETFramework' or '$(TargetFrameworkIdentifier)' == '.NETStandard' or '$(TargetFramework)' == 'netcoreapp2.1'">$(NoWarn);nullable</NoWarn>
	<NoWarn Condition="'$(GeneratePlatformNotSupportedAssembly)' == 'true' or '$(GeneratePlatformNotSupportedAssemblyMessage)' != ''">$(NoWarn);nullable</NoWarn>

[krwq]

what does is { } mean?

[roji]

This is just a way to use pattern making to check if a value isn't null

[stephentoub]

!= null or is not null would be better.

[roji]

!= null is the check being replaced, the problem is that since we're checking an array element, the compiler doesn't track the nullability status of that, and any additional reference is flagged again (that's why I made this change).

is not null is C# 9, right? Is it OK to use this already in runtime? If not, I can just use a local variable here instead (or replace this with is not null when that becomes available).

[roji]

FYI am going to merge as-is, but we can definitely replace these with the nicer is not null once we can use C# 9 here.

[jeffhandley]

There's a tiny benefit of capturing restriction, that will be realized more when using is not null.

[stephentoub]

Sure, but neither { } nor is not null are necessary to achieve that:

if (restrictions[i] is string restriction)
{
    ...
}

[jeffhandley]

Ah, yeah that'd be better indeed.

[roji]

The slight disadvantage of restrictions[i] is string restriction is that it's ambiguous with regards to whether we're checking that something isn't null, or that it's a string (as opposed to something else). FWIW Resharper/Rider warn about this.

I tried restrictions[i] is not null restriction, assuming that's how the C# 9 feature looks like, but it doesn't seem to compile. I'll just switch to restrictions[i] is string restriction for now as suggested.

[stephentoub]

If you see it as a disadvantage, then just do the super simple thing:

foreach (string? restriction in restrictions)
{
    if (restriction != null)
    {
        tmp[count++] = restriction;
    }
}

or

for (int i = 0; i < restrictions.Length; i++)
{
    string? restriction = restrictions[i];
    if (restriction != null)
    {
        tmp[count++] = restriction;
    }
}

or just suppress the warning:

for (int i = 0; i < restrictions.Length; i++)
{
    if (restrictions[i] != null)
    {
        tmp[count++] = restrictions[i]!; // TODO-NULLABLE: Indexer nullability tracked (https://github.com/dotnet/roslyn/issues/34644)
    }
}

as we do in dozens of other places in dotnet/runtime... the more we work around it, the less likely it'll be the compiler "fixes" it.
cc: @cston

Not everything needs to use fancy syntax.

[krwq]

is null buffer actually allowed or is it only for zero length?

[roji]

A null buffer makes the method just return the number of characters in the column, without returning it in any way (this is even in the docs... doesn't happen that often in System.Data :)).

[krwq]

I'm at 65/120 and have not seen anything blocking so far - my eyes got a bit blurry with question marks flying so will likely not finish today.

The only comment is to replace all TODO with TODO-NULLABLE for easier tracking

[roji]

@ericstj (or anyone else) any idea about the nullability errors in the build? It's odd that they're only in that one config, and I don't get them when building locally...

[stephentoub]

(or anyone else) any idea about the nullability errors in the build? It's odd that they're only in that one config, and I don't get them when building locally...

The reason we suppress nullability warnings in

runtime/src/libraries/Directory.Build.targets

Lines 9 to 10 in 3f7202b

	<NoWarn Condition="'$(TargetFrameworkIdentifier)' == '.NETFramework' or '$(TargetFrameworkIdentifier)' == '.NETStandard' or '$(TargetFramework)' == 'netcoreapp2.1'">$(NoWarn);nullable</NoWarn>
	<NoWarn Condition="'$(GeneratePlatformNotSupportedAssembly)' == 'true' or '$(GeneratePlatformNotSupportedAssemblyMessage)' != ''">$(NoWarn);nullable</NoWarn>

for downlevel targets is because those surface areas don't have nullable annotations / attributes. That includes Debug.Assert, which means if you have code like Debug.Assert(foo != null); foo.Blah();, without the [DoesNotReturnIf(false)] attribute on the assert, the compiler doesn't know that foo.Blah() is valid and warns that foo might be null. So, the most likely cause here is this library is building against a target that is not yet included in that Directory.Build.targets exclusion, and the fix would be to extend those conditions to make sure it's included.

[ericstj]

Are you doing an -allconfigurations build locally? The other legs are all for our vertical configurations where we execute tests, whereas allconfigurations includes other configurations which ship in the package. From the looks of the log the framework you're failing on is 'netcoreapp2.0-FreeBSD`.

We should probably just change the condition on nowarn to suppress on '$(TargetFrameworkIdentifier)' == '.NETCoreApp' and $([MSBuild]::VersionLessThan($(TargetFrameworkVersion), '3.0') instead of only netcoreapp2.1 as it does today.

[ericstj]

I went ahead and pushed a commit to suppress nullable warnings on all old netcoreapp frameworks as I suggested.

[roji]

Thanks @ericstj!

[ericstj]

Thanks @ericstj!

Sure thing! @krwq @jeffhandley I think this is still pending review of annotations.

[jeffhandley]

I'm at 80/121 files (although I started moving bottom up here recently). Will keep going tomorrow.

[jeffhandley]

A bit more progress reviewing but still not done.

[jeffhandley]

Finished reviewed. There were several places where you might want to use TODO-NULLABLE instead of TODO (following the pattern seen in other places in this PR). Just suggestions though.

I think what @ericstj spotted in DbMetaDataFactory.cs is a lurking regression. I don't know if there's existing test coverage that should have caught it though.

[roji]

@jeffhandley @krwq and the others, thanks for the thorough reviewing, I've integrated all your comments.

[jeffhandley]

Thank you for all of the effort you put into this!!