Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Confusing use of NotNullAttribute #1412

Closed
BCSharp opened this issue Apr 20, 2022 · 2 comments · Fixed by #1420
Closed

Confusing use of NotNullAttribute #1412

BCSharp opened this issue Apr 20, 2022 · 2 comments · Fixed by #1420
Assignees

Comments

@BCSharp
Copy link
Member

BCSharp commented Apr 20, 2022

NotNullAttribute comes from two sources:

  1. Originally from the DLR (namespace Microsoft.Scripting.Runtime) to mark parameters that do not accept a dynamic null value.
  2. Newer use to support nullable annotations (namespace System.Diagnostics.CodeAnalysis).

This creates a name clash when both attributes need to be used in the same file or even when only one of the attributes is used but both namespaces are imported. Various compilation units in the codebase handle the name clash in various ways: sometimes NotNull for the code analysis is renamed to NotNullOnReturn, and sometimes the DLR NotNull is renamed to NotDynamicNull. If there is no clash but just ambiguity, the attribute may not be renamed at all, just redeclared.

Even when there is no name clash or ambiguity, this is still confusing: when reading the code, it is not immediately obvious which attribute (hence which meaning) is applied. This has been bugging me already for a while.

The issue has been discussed before resulting in a compromise of using NotDynamicNull for the DLR NotNull. This requires renaming the attribute in the prologue of every .cs file where such attribute is used (and it is used frequently). My understanding was that, with time, NotDynamicNull will be supported directly by the DLR and all those renames polluting .cs files could be removed without requiring further changes to the IronPython code. Because this was supposed to be a DLR attribute, that was my reason for rejecting the alternative name NotNoneAttribute.

But now with global using, the whole issue can be easily handled exclusively at the IronPython level; no need to drag DLR into this. As I have stated before, I prefer NotNone over NotDynamicNull, primarily because is as short as NotNull and length matters since this attribute is used in method declarations where most often the whole declaration is put on one line.

Therefore I propose the following solution:

  1. NotNull always means System.Diagnostics.CodeAnalysis.NotNullAttribute.
  2. Microsoft.Scripting.Runtime.NotNullAttribute is renamed globally to NotNone. The code uses solely the renamed version.
  3. If possible, IronPythonAnalyzer is used to spot usage of not-renamed DLR NotNull.
  4. All existing other aliases for those two attributes are abolished, together with the corresponding file-scope using declarations for those aliases.
  5. The rename of DLR NotNnull is done with global using so that:
    1. It is readilly available without extra file-scope using (this attribute is much more often used than the one for code analysis)
    2. If the DLR ever does change the name for NotNull, it will require update in only one place (per project).

If this proposal gets a go ahead, I am prepared to implement the changes throughout the whole codebase.

@slozier
Copy link
Contributor

slozier commented Apr 21, 2022

Very cool, didn't know global using was a thing! Sounds good to me, I propose we use the project file version of it (instead of sticking it in a random cs file):

<ItemGroup>
  <Using Include="Microsoft.Scripting.Runtime.NotNullAttribute" Alias="NotNoneAttribute" />
</ItemGroup>

Seems to even work if we put in in Directory.Build.props...

@BCSharp
Copy link
Member Author

BCSharp commented Apr 24, 2022

Directory.Build.props looks like a good place.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants