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

Annotate config.GetValue() with [NotNullIfNotNull] #101336

Merged
merged 3 commits into from
Apr 29, 2024

Conversation

dahlbyk
Copy link
Contributor

@dahlbyk dahlbyk commented Apr 20, 2024

This is annoying:

// CS8600: Converting possible null value to non-nullable type.
string foo = config.GetValue("foo", "bar");

Before the fix, the updated tests fail to compile:

ConfigurationBinderTests.cs(357,31): error CS8600: Converting null literal or possible null value to non-nullable type.
ConfigurationBinderTests.cs(358,48): error CS8600: Converting null literal or possible null value to non-nullable type.
ConfigurationBinderTests.cs(368,25): error CS8605: Unboxing a possibly null value.
ConfigurationBinderTests.cs(369,29): error CS8605: Unboxing a possibly null value.
ConfigurationBinderTests.cs(370,29): error CS8605: Unboxing a possibly null value.
ConfigurationBinderTests.cs(372,31): error CS8600: Converting null literal or possible null value to non-nullable type.
ConfigurationBinderTests.cs(373,48): error CS8600: Converting null literal or possible null value to non-nullable type.

The source generator has been updated as well.

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Apr 20, 2024
Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-extensions-configuration
See info in area-owners.md if you want to be subscribed.

@dahlbyk
Copy link
Contributor Author

dahlbyk commented Apr 20, 2024

Hmm, tried adding System.Diagnostics.CodeAnalysis to the source generator: Update: Changed to fully-qualified reference instead of using.

private readonly SortedSet<string> _namespaces = new()
{
"System",
"System.CodeDom.Compiler",
"System.Globalization",
"System.Runtime.CompilerServices",
"Microsoft.Extensions.Configuration",
};

But build is failing with:

error CS0436: The type 'NotNullIfNotNullAttribute' in 'src\libraries\System.Private.CoreLib\src\System\Diagnostics\CodeAnalysis\NullableAttributes.cs' conflicts with the imported type 'NotNullIfNotNullAttribute' in 'Microsoft.Extensions.Configuration.Binder, Version=9.0.0.0, Culture=neutral, PublicKeyToken=adb9793829ddae60'. Using the type defined in 'src\libraries\System.Private.CoreLib\src\System\Diagnostics\CodeAnalysis\NullableAttributes.cs'. [src\libraries\Microsoft.Extensions.Configuration.Binder\tests\SourceGenerationTests\Microsoft.Extensions.Configura
tion.Binder.SourceGeneration.Tests.csproj::TargetFramework=net462]

@tarekgh
Copy link
Member

tarekgh commented Apr 21, 2024

@dahlbyk thanks for submitting the PR. I converted this to a draft PR as there is more need to be done before we can accept this PR. Adding the attribute to these APIs make sense, there is more need to be done here if we want to proceed with that.

  • This is kind of breaking change. you can see the app compatibility failures:
❌.packages\microsoft.dotnet.apicompat.task\9.0.100-preview.4.24215.1\build\Microsoft.DotNet.ApiCompat.ValidateAssemblies.Common.targets(16,5): error : API compatibility errors between 'ref/net9.0/Microsoft.Extensions.Configuration.Binder.dll' (left) and 'lib/net9.0/Microsoft.Extensions.Configuration.Binder.dll' (right):
❌.packages\microsoft.dotnet.apicompat.task\9.0.100-preview.4.24215.1\build\Microsoft.DotNet.ApiCompat.ValidateAssemblies.Common.targets(16,5): error CP0016: Cannot add attribute 'System.Diagnostics.CodeAnalysis.NotNullIfNotNullAttribute("defaultValue")' to 'Microsoft.Extensions.Configuration.ConfigurationBinder.GetValue(Microsoft.Extensions.Configuration.IConfiguration, System.Type, string, object?)'.
❌.packages\microsoft.dotnet.apicompat.task\9.0.100-preview.4.24215.1\build\Microsoft.DotNet.ApiCompat.ValidateAssemblies.Common.targets(16,5): error CP0016: Cannot add attribute 'System.Diagnostics.CodeAnalysis.NotNullIfNotNullAttribute("defaultValue")' to 'Microsoft.Extensions.Configuration.ConfigurationBinder.GetValue<T>(Microsoft.Extensions.Configuration.IConfiguration, string, T)'.

We need to decide if we should go with the breaking change and file a breaking change doc for it. Also, we'll need to suppress the app compatibility failure:

error : API breaking changes found. If those are intentional, the APICompat suppression file can be updated by rebuilding with '/p:ApiCompatGenerateSuppressionFile=true' [D:\a\_work\1\s\src\libraries\Microsoft.Extensions.Configuration.Binder\src\Microsoft.Extensions.Configuration.Binder.csproj]

CC @stephentoub @ericstj if they have any suggestion or recommendation about his change.

@dahlbyk
Copy link
Contributor Author

dahlbyk commented Apr 22, 2024

Thanks for the initial feedback! I've conditionally included NullableAttributes.cs and suppressed the compatibility failure for Microsoft.Extensions.Configuration.Binder.dll.

@dahlbyk
Copy link
Contributor Author

dahlbyk commented Apr 22, 2024

I took a stab at updating the source generator. Haven't figured out the CS0436 error mentioned above, but I did noticed it's specific to TargetFramework=net462.

@ericstj
Copy link
Member

ericstj commented Apr 22, 2024

I don't think making a nullable API less nullable should be actually breaking. I think the issue flagged by APICompat is just that the reference assembly needs to be updated. Please make sure to update reference source in the ref folder. We also don't have strongly typed rules for nullability attributes - so those are just going to get flagged no matter what. If we agree to the change we can suppress the diagnostic.

@dahlbyk dahlbyk force-pushed the config-defaultValue-nullability branch from 860871c to d44f69d Compare April 23, 2024 05:53
Comment on lines 21 to 23
<ItemGroup Condition="'$(TargetFrameworkIdentifier)' == 'netstandard2.0'">
<Compile Include="$(CoreLibSharedDir)System\Diagnostics\CodeAnalysis\NullableAttributes.cs" Link="System.Private.CoreLib\System\Diagnostics\CodeAnalysis\NullableAttributes.cs" />
</ItemGroup>
Copy link
Contributor Author

@dahlbyk dahlbyk Apr 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It turns out this is redundant, as NullableAttributes.cs is already included in most libraries automatically:

<!-- Adds Nullable annotation attributes to C# non .NETCoreApp builds. -->
<ItemGroup Condition="'$(Nullable)' != '' and
'$(Nullable)' != 'disable' and
'$(MSBuildProjectExtension)' == '.csproj' and
'$(TargetFrameworkIdentifier)' != '.NETCoreApp'">
<Compile Include="$(CoreLibSharedDir)System\Diagnostics\CodeAnalysis\NullableAttributes.cs" Link="System\Diagnostics\CodeAnalysis\NullableAttributes.cs" />
</ItemGroup>

This explains #101336 (comment):

error CS0436: The type 'NotNullIfNotNullAttribute' in 'src\libraries\System.Private.CoreLib\src\System\Diagnostics\CodeAnalysis\NullableAttributes.cs' conflicts with the imported type 'NotNullIfNotNullAttribute' in 'Microsoft.Extensions.Configuration.Binder, ...'.

Microsoft.Extensions.Configuration.Binder.SourceGeneration.Tests.csproj has its own NullableAttributes.cs that it has ignored, but when actually used it conflicts with internal one in Microsoft.Extensions.Configuration.Binder:

[assembly: InternalsVisibleTo("Microsoft.Extensions.Configuration.Binder.SourceGeneration.Tests, PublicKey=0024000004800000940000000602000000240000525341310004000001000100f33a29044fa9d740c9b3213a93e57c84b472c84e0b8a0e1ae48e67a9f8f6de9d5f7f3d52ac23e48ac51801f1dc950abe901da34d2a9e3baadb141a17c77ef3c565dd5ee5054b91cf63bb3c6ab83f72ab3aafe93d0fc3c2348b764fafb0b1c0733de51459aeab46580384bf9d74c4e28164b7cde247f891ba07891c9d872ad2bb")]

There may be a cleaner way, but I was able to work around this by allowing projects to opt out of the import.

@tarekgh
Copy link
Member

tarekgh commented Apr 26, 2024

@dahlbyk We shouldn't enforce the definition of the nullable attribute in the source generator as it could lead to complications and problems. The source generator only needs to verify if the attribute is defined during compilation and utilize it accordingly. If it's not defined, we should refrain from emitting any code that relies on this attribute. Users can choose to define the attribute in their code if they wish to support it, granting them flexibility in its usage.

@dahlbyk
Copy link
Contributor Author

dahlbyk commented Apr 26, 2024

The source generator only needs to verify if the attribute is defined during compilation and utilize it accordingly.

This seems reasonable but I'm new to SG so not sure what this looks like. Any examples/docs?

@tarekgh
Copy link
Member

tarekgh commented Apr 27, 2024

This seems reasonable but I'm new to SG so not sure what this looks like. Any examples/docs?

You may look at the code

Half = compilation.GetBestTypeByMetadataName("System.Half");
as example to see how you can search for a type in the compilation. You may look at the docs Compilation.GetTypesByMetadataName and Compilation.GetTypeByMetadataName too for more info.

You search for NotNullIfNotNullAttribute type then if it is found, utilize it.

@dahlbyk dahlbyk force-pushed the config-defaultValue-nullability branch from 001579c to 4ffdaa3 Compare April 27, 2024 05:34
@dahlbyk
Copy link
Contributor Author

dahlbyk commented Apr 27, 2024

  1. Updated to only emit [NotNullIfNotNull] if it exists
  2. Dropped all the churn from the branch
  3. Rebased onto latest main

Update: build is fine.

I'm curious if the build is going to reproduce local failures that seem related to #95830:

System.TypeLoadException: Method 'get_Keys' in type 'ImplementerOfIDictionaryClass`2' from assembly 'Microsoft.Extensions.Configuration.Binder.Tests, Version=9.0.0.0, Culture=neutral, PublicKeyToken=adb9793829ddae60' does not have an implementation.

System.TypeLoadException: Method 'get_Keys' in type 'ImplementerOfIDictionaryClass`2' from assembly 'Microsoft.Extensions.Configuration.Binder.SourceGeneration.Tests, Version=9.0.0.0, Culture=neutral, PublicKeyToken=adb9793829ddae60' does not have an implementation.

It does, in fact, have an implementation:

I might just need to do a full rebuild?

@dahlbyk
Copy link
Contributor Author

dahlbyk commented Apr 28, 2024

Restored NoIncludeNullableAttributes and rebased onto main again.

Copy link
Member

@tarekgh tarekgh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @dahlbyk, I added a minor comments, otherwise LGTM .

@dahlbyk dahlbyk force-pushed the config-defaultValue-nullability branch from 8b8b2be to 92d4c26 Compare April 29, 2024 01:04
@tarekgh
Copy link
Member

tarekgh commented Apr 29, 2024

Thanks @dahlbyk for your help getting this working and completing it.

@tarekgh tarekgh merged commit 92ca5f3 into dotnet:main Apr 29, 2024
79 of 84 checks passed
@dahlbyk dahlbyk deleted the config-defaultValue-nullability branch April 29, 2024 19:34
matouskozak pushed a commit to matouskozak/runtime that referenced this pull request Apr 30, 2024
* Annotate GetValue() with [NotNullIfNotNull]

* Avoid duplicate NullableAttributes.cs

* Annotate generated GetValue() with [NotNullIfNotNull]
michaelgsharp pushed a commit to michaelgsharp/runtime that referenced this pull request May 9, 2024
* Annotate GetValue() with [NotNullIfNotNull]

* Avoid duplicate NullableAttributes.cs

* Annotate generated GetValue() with [NotNullIfNotNull]
Ruihan-Yin pushed a commit to Ruihan-Yin/runtime that referenced this pull request May 30, 2024
* Annotate GetValue() with [NotNullIfNotNull]

* Avoid duplicate NullableAttributes.cs

* Annotate generated GetValue() with [NotNullIfNotNull]
@github-actions github-actions bot locked and limited conversation to collaborators May 31, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Extensions-Configuration community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants