search

manuelroemer / Nullable

A source code only package which allows you to use .NET's nullable attributes in older target frameworks like .NET Standard 2.0 or the "old" .NET Framework.
https://www.nuget.org/packages/Nullable
MIT License
186 stars 8 forks source link

Can't build on older targets without explicit LangVersion #22

Closed Taudris closed 3 years ago

Taudris commented 3 years ago

I'm trying to create a NuGet package which contains 100% manual NRT attributes (with no C# 8.0 syntax) so that I can multitarget netstandard2.0 and netstandard2.1, remain in an MS-supported configuration (employer requirement), and get correct nullability warnings in projects which use C# 8.0+.

When I try to build against netstandard2.0, I get build errors on the generated .cs files in the obj folder because they include the #nullable preprocessor statement. If this preprocessor statement was not present (and the attributes had their own explicit annotations, if that's possible/needed), the files would otherwise compile.

Do you think you could make this change?

Taudris commented 3 years ago

Apparently NRT depends on the C# 8.0 compiler for things like nullable generic type arguments. The attributes that are compiler-generated, Nullable(byte/byte[]) and NullableContext(byte), are quite difficult to annotate code with by hand. So one must either use an unsupported TargetFramework/LangVersion combination, or contend with incomplete nullable support.

It would be nice to at least have the option for the latter guarded by a #define and some documented caveats.

manuelroemer commented 3 years ago

Hey there! As you wrote, manually annotating the attributes with Nullable/NullableContext is not something I would want to try/do as that is too complex and fragile for my taste.

A possible solution would be to nest the #nullable enable/#nullable restores in a NULLABLE_ATTRIBUTES_DISABLE_NULLABILITY preprocessor variable. That would allow you to manually remove them (and just end up with ambiguous nullability). At the moment, not a single attribute uses a single ? anyway, so the effort would be manageable.

I'm not so sure about whether I want to make this change though. I would generally assume users of the package to use C# 8.0+ (it's even a requirement in the README) and I'm not certain whether I want to relax that requirement.

Since you already wrote about the issue with Nullable/NullableContext: Do you think that you will continue with your plan to migrate your company's files to NRTs? You will, after all, have the same issue mentioned above with your custom code and it already sounded like this is not really an option for you either. If you do continue, is it an option for you to simply copy the attribute files to your project(s) manually and remove the nullable annotations? Then you would not depend on any change in this package.

Taudris commented 3 years ago

I tried doing exactly that. While it is compatible and does seem to work, it raises the question of complexity vs usefulness. You're getting an unknown subset of features, potentially breaking IDEs which depend on NullableAttribute and NullableContextAttribute (VS2019 currently treats referenced projects that don't have NRT enabled in an NRT project as having all non-nullable references - I hope/assume this is a bug that'll get fixed using information from these attributes), and creating an extra burden for other devs to learn on top of learning regular NRT.

I ended up multitargeting netstandard2.0 and 2.1, adding a preprocessor definition NULLABLE_ENABLED, and gating the new syntax with it. It's real ugly, but it's the only way to be 100% safe while getting NRT. I just wish the preprocessor was enough more powerful in some way to help deal with it.