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
185 stars 8 forks source link

Why are the attributes marked as internal? #2

Closed mrdnote closed 5 years ago

mrdnote commented 5 years ago

I've added the project to my solution, but I can't use the attributes since they are marked as internal. When I change them to public it works brilliantly.

manuelroemer commented 5 years ago

The attributes are internal, so that no name conflicts can happen. If the attributes were public, naming conflicts should happen. For example, have a look at this (very professional looking) constellation: grafik

The library at the top uses the package. If the attribute is public it becomes visible to other projects which use this library. A problem occurs when there are two attributes with the same name, e.g. for a .NET Core 3.0 app. Now there are two attributes in the same namespace with the same name - one from .NET Core and one from the library. The compiler will be unable to choose the correct one and thus won't compile.

By making the attribute internal, it is not visible to others, hence no conflict can occur (in this example, the .NET Core app would directly use the attribute defined by the .NET Core BCL).

I am curious why your project doesn't work if the attributes are internal though. Could you share your exact project setup, so that I can have a look at it? Or can you tell me how I can reproduce the issue?

Sorry to hear that you seem to have such a rough time getting this to work - let's solve these problems!

manuelroemer commented 5 years ago

By the way, I am thinking of introducing a compiler constant like NULLABLE_ATTRIBUTES_PUBLIC which change the modifiers from internal to public. This is something that LibLog does (LibLog also adds source code files to projects). This would 100% be opt-in though, because internal should always be the default.

mrdnote commented 5 years ago

Classic .NET framework projects don't have conflicts, because the System.Diagnostics.CodeAnalysis namespace does not exist there.

And this is also why I can't use the attributes, because I don't have standard System.Diagnostics.CodeAnalysis. And the internal classes cannot be specified because they are internal to the Nullable project.

Out of curiosity, in a .NET Core app situation, how come your attributes are being used while the are marked as inaccessible, so in effect the code is referencing the .NET Core framework attributes instead of your Nullable package's attributes? Or are they just there to "fool" the compiler?

manuelroemer commented 5 years ago

Even classic .NET Framework projects could in theory get these conflicts. Here is an example:

LibraryA:
  - public class AllowNullAttribute

LibraryB:
  - public class AllowNullAttribute

App:
  * Uses LibraryA
  * Uses LibraryB

If you then wrote this code in App, you would also get a compiler error, because the compiler doesn't know if he should use the attribute from LibraryA or LibraryB:

using System.Diagnostics.CodeAnalysis;

[AllowNull] public string Foo { get; set; }

This is why the attributes are defined as internal - if they don't "leak out" of the project, there will not be any conflict.

Or are they just there to "fool" the compiler?

That's basically it. The whole idea works, because the C# compiler simply checks if there is an attribute with the same name and namespace when doing nullable analysis, but the attribute does not have to come from the official .NET BCL. Because of this, it even works if the attribute is internal. And if you use this package with a .NET Core 3.0 app, the Nullable.cs file simply doesn't get added to the project, because .NET already defines these attributes for you.

Ideally, projects using this package are setup like this:

LibraryA:
  * References Nullable NuGet package
  - internal class AllowNullAttribute

LibraryB: 
  * References Nullable NuGet package
  - internal class AllowNullAttribute

App:
  * Uses LibraryA
  * Uses LibraryB
  * References Nullable NuGet package
  - internal class AllowNullAttribute

This way, the attributes exist in every project, but don't leak out. The compiler can still do nullable analysis, because each project still defines the attributes internally.

To get back to the original problem: Are your projects set-up like above, i.e. does each project reference the NuGet package separately? If not, how did you add the attributes?

mrdnote commented 5 years ago

I know how conflicts work, I'm just saying that I don't have them in this case, because the AllowNullAttribute is not defined for .NET Framework. Your definition is the first and only one from the perspective of my project.

My project is setup so each library and app references your Nullable project.

Attached you can find a test project which demonstrates the issue. NullableTestConsoleApp.zip

manuelroemer commented 5 years ago

Thanks, that project is very helpful! 👍

The reason for why it doesn't work is that the Nullable project has not been added via NuGet. Try the following steps for a testing project:

  1. Create a new .NET Console project using the old .NET FW.

  2. Go into the Visual Studio Options > NuGet Package Manager and select the "Allow format selection on first package install" checkbox. This allows you to choose if you want to use a packages.config file or package references when you install your first package.
    grafik

  3. Go into the package manager by right clicking "References" > "Manage NuGet packages...". Search for "Nullable" and install the package. In the dialog which pops up, choose the "Package reference in project file" option.
    grafik

  4. Enable C# 8.0 so that you have Nullable support by unloading and editing your project and then editing the XML like in the third image: grafik
    grafik
    grafik

  5. Reload your project - you should now be able to use the attributes in existing code.
    grafik

  6. Try it out by, for example, pasting the code below and checking if the warnings are shown: grafik

Furthermore, you can put the cursor on the attribute and press F12 - this way, you can see if the file actually got added to your project:

Click F12 here: grafik

and you will see this: grafik

You can download and test the finished demo project here: NullableNetFw.zip

I hope that this helps (and works) for you!

Edit: One last note - these are a lot of steps to get this working. This is not the case for projects which use the new SDK style .csproj format. For these, you can simply do a dotnet add package Nullable and voilà, it works. Just adding this in case other people find this issue and wonder why there are so many required steps.

mrdnote commented 5 years ago

Yeah, in my other issue https://github.com/manuelroemer/Nullable/issues/1 I already gave away that I would add Nullable.csproj as a project to my solution. Now I understand the miscommunication about internal/public and nuget package references :D

Edit: For me, the easiest solution is to add the project to my solution and change the access modifiers to public. I don't expect your project to be high maintenance with a lot of updates.

manuelroemer commented 5 years ago

Okay awesome, I'm glad that everything is settled now! :)

I don't expect your project to be high maintenance with a lot of updates.

Yeah, I agree - there are not going to be many changes here.

One last tip: You can consider to just create a Shared Project in Visual Studio and then copy only the NullableAttributes.cs file and nothing else to it. This way, you won't even have to change the internal to public and you won't have to deal with NuGet packages - less work overall! That is, in my opinion, the best out of both worlds.