Rework TdsParserStateObjectManaged with nullable annotations

[Wraith2]

I was contacted by someone in the c# discord server who was experiencing #988 with version 3.0.1 of this library. If the previous fix was correct that shouldn't be possible so I had a look at the code in TdsParserStateObjectManaged.ReleasePacket:

internal override void ReleasePacket(PacketHandle syncReadPacket)
{
    SNIPacket packet = syncReadPacket.ManagedPacket;
    if (packet != null)
    {
        SNIHandle handle = Handle;
        handle.ReturnPacket(packet);
    }
}

There are only two variables involved and the only one we haven't checked for null is handle so it's pretty obvious what's going on. Looking at this it's clear that c#8 nullable reference type annotations would have shown this as a problem so I thought I'd rewrite the file with nullable enabled to see if it caused any interesting bugs or changes to show up. This PR is the result.

Nullable annotations are a compile time feature and do not add any runtime checking. It is possible to pass nulls to functions defined in nullable enabled files even if those functions are decorated with non-null context. That means that these changes only introduce new exceptions where an explicit throw is added, there are no hidden null checks.

[roji]

@Wraith2 it is pretty common practice to turn on NRTs only for recent TFMs, to avoid the issues above, e.g. since the netstandard2.0 isn't yet properly annotated. For nullability-related attributes which were only introduced later (e.g. [DoesNotReturn]), you can have those attributes defined in the project once behind #if, so that you don't need to do conditional compilation every time you want to use them (here's the Npgsql example).

This should land you in a place where you can use NRTs freely and without hacks or code smells, while getting almost perfect coverage through the new TFMs. There's always the chance of unverified code that's conditionally compiled only in older TFMs, but that's generally very rare, and code quality is usually more important.

[Wraith2]

you can have those attributes defined in the project once behind #if, so that you don't need to do conditional compilation every time you want to use them (here's the Npgsql example).

I considered this but my concern is what happens if multiple assemblies do the same thing? Say for example npgsql and sqlclient get loaded into the same appdomain/alc, doesn't it cause a crash because it's multiply defined?

[roji]

you can have those attributes defined in the project once behind #if, so that you don't need to do conditional compilation every time you want to use them (here's the Npgsql example).

I considered this but my concern is what happens if multiple assemblies do the same thing? Say for example npgsql and sqlclient get loaded into the same appdomain/alc, doesn't it cause a crash because it's multiply defined?

Having multiple types in the same namespace isn't an issue, provided they live in different assemblies. .NET types are fully-qualified by their assembly name. These would just be internal types which never get used or exposed externally.

In fact, the NRT NullableAttribute (which is what gets generated based on NRT annotations) is synthesized by Roslyn into assemblies it builds (there's no NullableAttribute in the BCL), so there's already lots of copies of that type all around (see dotnet/runtime#29039).

[Wraith2]

Ok, that works for me. I've integrated the definitions into the shared codebase so when we get to merging the managed implementation into netfx we'll be able to use the same file. This removes all the postfix non-null assertion decorations which is pleasing.

[Wraith2]

@David-Engel I know this is queue jumping but it would be nice to try and get this into 5.0.0 because it fixes a user reported issue and is a stability improvement.

[codecov]

Codecov Report

Merging #1555 (5a8a902) into main (9b1996a) will decrease coverage by 0.15%.
The diff coverage is 90.14%.

❗ Current head 5a8a902 differs from pull request most recent head fd380d9. Consider uploading reports for the commit fd380d9 to get more accurate results

@@            Coverage Diff             @@
##             main    #1555      +/-   ##
==========================================
- Coverage   71.40%   71.25%   -0.16%     
==========================================
  Files         295      295              
  Lines       61195    61208      +13     
==========================================
- Hits        43697    43611      -86     
- Misses      17498    17597      +99     

Flag	Coverage Δ
addons	92.38% <ø> (ø)
netcore	74.79% <90.14%> (-0.17%)	⬇️
netfx	69.17% <ø> (-0.10%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...soft/Data/SqlClient/TdsParserStateObjectManaged.cs	83.91% <90.14%> (-3.01%)	⬇️
...ata/SqlClient/SqlConnectionTimeoutErrorInternal.cs	30.35% <0.00%> (-11.61%)	⬇️
...src/Microsoft/Data/SqlClient/SqlMetadataFactory.cs	81.66% <0.00%> (-10.84%)	⬇️
...ActiveDirectoryAuthenticationTimeoutRetryHelper.cs	56.81% <0.00%> (-6.82%)	⬇️
...rc/Microsoft/Data/SqlClient/SQLFallbackDNSCache.cs	90.62% <0.00%> (-6.25%)	⬇️
...re/src/Microsoft/Data/SqlClient/SNI/SNINpHandle.cs	73.29% <0.00%> (-5.12%)	⬇️
.../src/Microsoft/Data/SqlClient/SNI/SNIMarsHandle.cs	87.89% <0.00%> (-4.49%)	⬇️
...nt/src/Microsoft/Data/ProviderBase/TimeoutTimer.cs	71.42% <0.00%> (-3.58%)	⬇️
...Microsoft/Data/ProviderBase/DbConnectionFactory.cs	17.04% <0.00%> (-3.41%)	⬇️
...e/src/Microsoft/Data/SqlClient/SNI/SNITcpHandle.cs	59.01% <0.00%> (-2.46%)	⬇️
... and 9 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 031afe8...fd380d9. Read the comment docs.

[David-Engel]

LGTM