Dapper icon indicating copy to clipboard operation
Dapper copied to clipboard

Nullable reference types

Open jnm2 opened this issue 4 years ago • 4 comments

I annotated the public API in https://github.com/StackExchange/Dapper/pull/1392I for fun and in case it could be helpful. I don't have a use case for this feature as a Dapper user right now, and I'm not asking for it.

@NickCraver had good thoughts and asked me to open an issue for discussion. There are some big drawbacks:

  • It would hose pretty much all open PRs.
  • Fixing the new warnings results in a big change to review. (Flip side: there are definitely bugs that got caught.)
  • Using C# 8 with any target framework other than .NET Core 3+ is officially unsupported by Microsoft, even though they tell library authors to use this unsupported configuration for the purpose of NRT annotation.
  • If I'm paraphrasing correctly, Nick is concerned that enabling NRTs could make it easier for nulls to leak in or out of Dapper APIs in places where they are currently supposed to never be null.

Is there anything that Dapper would stand to gain by paying these costs?

📝 https://github.com/tunnelvisionlabs/ReferenceAssemblyAnnotator aims to fill the tooling gap by copying nullability annotations from .NET Core 3+ into the reference assemblies you compile against and by providing internal nullability attributes as source. I use it for active projects from net35 to net48 and netstandard2.0. There aren't any technical limitations I know of compared to the NRT experience on .NET Core 3 itself. (Besides bugs in this community tool, of course.)

jnm2 avatar Jan 05 '20 01:01 jnm2

Tagging this for consideration in v3 - we're not sure when the net4x drop will happen but Dapper is very popular and this one doesn't cross the boundary well so...we need to see.

NickCraver avatar May 02 '20 13:05 NickCraver

Update here: we will be dropping net4x support in vNext.

NickCraver avatar May 09 '21 17:05 NickCraver

Dapper not being annotated means that I don't have a way to know whether Dapper might pass null to either value parameter:

internal sealed class FooTypeHandler : SqlMapper.TypeHandler<Foo>
{
    public override Foo Parse(object value)
    {
        throw new NotImplementedException();
    }

    public override void SetValue(IDbDataParameter parameter, Foo value)
    {
        throw new NotImplementedException();
    }
}

jnm2 avatar Aug 13 '21 14:08 jnm2

Looking at the Microsoft blog the advise for library authors is quite clear. So I'd like to see the annotations in the library as well. It's a bit unfortunate though that you need to turn on C# 8 for the netstandard2.0 target which is "unsupported" but then most things work anyway (including NRT).

roederja avatar Jul 23 '22 20:07 roederja