From 0bdc91e7c3b81c65a3f13f87562e383e75f84f3f Mon Sep 17 00:00:00 2001 From: Keith Dahlby Date: Mon, 29 Apr 2024 10:49:01 -0500 Subject: [PATCH] Annotate `config.GetValue()` with `[NotNullIfNotNull]` (#101336) * Annotate GetValue() with [NotNullIfNotNull] * Avoid duplicate NullableAttributes.cs * Annotate generated GetValue() with [NotNullIfNotNull] --- src/libraries/Directory.Build.targets | 1 + .../ConfigurationBindingGenerator.Emitter.cs | 2 ++ .../ConfigurationBindingGenerator.Parser.cs | 1 + .../gen/Emitter/ConfigurationBinder.cs | 10 ++++++++ .../gen/Parser/KnownTypeSymbols.cs | 4 +++ .../gen/Specs/SourceGenerationSpec.cs | 1 + ...crosoft.Extensions.Configuration.Binder.cs | 2 ++ .../src/ConfigurationBinder.cs | 2 ++ .../tests/Common/ConfigurationBinderTests.cs | 25 ++++++++++++++----- .../GetValue.generated.txt | 2 ++ .../GetValue_T_Key_DefaultValue.generated.txt | 1 + ...alue_TypeOf_Key_DefaultValue.generated.txt | 1 + ...ation.Binder.SourceGeneration.Tests.csproj | 3 +++ 13 files changed, 49 insertions(+), 6 deletions(-) diff --git a/src/libraries/Directory.Build.targets b/src/libraries/Directory.Build.targets index e3d599b2d8276..5a3fff449e7bb 100644 --- a/src/libraries/Directory.Build.targets +++ b/src/libraries/Directory.Build.targets @@ -170,6 +170,7 @@ diff --git a/src/libraries/Microsoft.Extensions.Configuration.Binder/gen/ConfigurationBindingGenerator.Emitter.cs b/src/libraries/Microsoft.Extensions.Configuration.Binder/gen/ConfigurationBindingGenerator.Emitter.cs index d8c5a8925b46b..1dceb31a0cc8a 100644 --- a/src/libraries/Microsoft.Extensions.Configuration.Binder/gen/ConfigurationBindingGenerator.Emitter.cs +++ b/src/libraries/Microsoft.Extensions.Configuration.Binder/gen/ConfigurationBindingGenerator.Emitter.cs @@ -15,6 +15,7 @@ private sealed partial class Emitter private readonly TypeIndex _typeIndex; private readonly bool _emitEnumParseMethod; private readonly bool _emitGenericParseEnum; + private readonly bool _emitNotNullIfNotNull; private readonly bool _emitThrowIfNullMethod; private readonly SourceWriter _writer = new(); @@ -26,6 +27,7 @@ public Emitter(SourceGenerationSpec sourceGenSpec) _typeIndex = new TypeIndex(sourceGenSpec.ConfigTypes); _emitEnumParseMethod = sourceGenSpec.EmitEnumParseMethod; _emitGenericParseEnum = sourceGenSpec.EmitGenericParseEnum; + _emitNotNullIfNotNull = sourceGenSpec.EmitNotNullIfNotNull; _emitThrowIfNullMethod = sourceGenSpec.EmitThrowIfNullMethod; } diff --git a/src/libraries/Microsoft.Extensions.Configuration.Binder/gen/ConfigurationBindingGenerator.Parser.cs b/src/libraries/Microsoft.Extensions.Configuration.Binder/gen/ConfigurationBindingGenerator.Parser.cs index cb39cd788cfd7..3d6e2b6503101 100644 --- a/src/libraries/Microsoft.Extensions.Configuration.Binder/gen/ConfigurationBindingGenerator.Parser.cs +++ b/src/libraries/Microsoft.Extensions.Configuration.Binder/gen/ConfigurationBindingGenerator.Parser.cs @@ -57,6 +57,7 @@ internal sealed partial class Parser(CompilationData compilationData) ConfigTypes = _createdTypeSpecs.Values.OrderBy(s => s.TypeRef.FullyQualifiedName).ToImmutableEquatableArray(), EmitEnumParseMethod = _emitEnumParseMethod, EmitGenericParseEnum = _emitGenericParseEnum, + EmitNotNullIfNotNull = _typeSymbols.NotNullIfNotNullAttribute is not null, EmitThrowIfNullMethod = IsThrowIfNullMethodToBeEmitted() }; } diff --git a/src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Emitter/ConfigurationBinder.cs b/src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Emitter/ConfigurationBinder.cs index c6f23779ec93d..f49cabc8bfc0c 100644 --- a/src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Emitter/ConfigurationBinder.cs +++ b/src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Emitter/ConfigurationBinder.cs @@ -73,6 +73,7 @@ private void EmitGetValueMethods() if (ShouldEmitMethods(MethodsToGen.ConfigBinder_GetValue_T_key_defaultValue)) { EmitStartDefinition_Get_Or_GetValue_Overload(MethodsToGen.ConfigBinder_GetValue_T_key_defaultValue, documentation); + EmitNotNullIfNotNull(Identifier.defaultValue); _writer.WriteLine($"public static T? {Identifier.GetValue}(this {Identifier.IConfiguration} {Identifier.configuration}, string {Identifier.key}, T {Identifier.defaultValue}) => " + $"(T?)({expressionForGetValueCore}({Identifier.configuration}, typeof(T), {Identifier.key}) ?? {Identifier.defaultValue});"); } @@ -87,11 +88,20 @@ private void EmitGetValueMethods() if (ShouldEmitMethods(MethodsToGen.ConfigBinder_GetValue_TypeOf_key_defaultValue)) { EmitStartDefinition_Get_Or_GetValue_Overload(MethodsToGen.ConfigBinder_GetValue_TypeOf_key_defaultValue, documentation); + EmitNotNullIfNotNull(Identifier.defaultValue); _writer.WriteLine($"public static object? {Identifier.GetValue}(this {Identifier.IConfiguration} {Identifier.configuration}, Type {Identifier.type}, string {Identifier.key}, object? {Identifier.defaultValue}) => " + $"{expressionForGetValueCore}({Identifier.configuration}, {Identifier.type}, {Identifier.key}) ?? {Identifier.defaultValue};"); } } + private void EmitNotNullIfNotNull(string parameterName) + { + if (_emitNotNullIfNotNull) + { + _writer.WriteLine($"[return: global::System.Diagnostics.CodeAnalysis.NotNullIfNotNull(nameof({parameterName}))]"); + } + } + private void EmitBindMethods_ConfigurationBinder() { if (!ShouldEmitMethods(MethodsToGen.ConfigBinder_Bind)) diff --git a/src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Parser/KnownTypeSymbols.cs b/src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Parser/KnownTypeSymbols.cs index 89ff70db196b0..2f084ec7bdfce 100644 --- a/src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Parser/KnownTypeSymbols.cs +++ b/src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Parser/KnownTypeSymbols.cs @@ -64,6 +64,7 @@ internal sealed class KnownTypeSymbols public INamedTypeSymbol? MemberInfo { get; } public INamedTypeSymbol? ParameterInfo { get; } public INamedTypeSymbol? Delegate { get; } + public INamedTypeSymbol? NotNullIfNotNullAttribute { get; } public KnownTypeSymbols(CSharpCompilation compilation) { @@ -132,6 +133,9 @@ public KnownTypeSymbols(CSharpCompilation compilation) IntPtr = Compilation.GetSpecialType(SpecialType.System_IntPtr); UIntPtr = Compilation.GetSpecialType(SpecialType.System_UIntPtr); Delegate = Compilation.GetSpecialType(SpecialType.System_Delegate); + + // Only generate nullable attributes if available + NotNullIfNotNullAttribute = compilation.GetBestTypeByMetadataName("System.Diagnostics.CodeAnalysis.NotNullIfNotNullAttribute"); } } } diff --git a/src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Specs/SourceGenerationSpec.cs b/src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Specs/SourceGenerationSpec.cs index c4fc5a6079ad9..e435ffa0cd0b2 100644 --- a/src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Specs/SourceGenerationSpec.cs +++ b/src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Specs/SourceGenerationSpec.cs @@ -12,6 +12,7 @@ public sealed record SourceGenerationSpec public required ImmutableEquatableArray ConfigTypes { get; init; } public required bool EmitEnumParseMethod { get; set; } public required bool EmitGenericParseEnum { get; set; } + public required bool EmitNotNullIfNotNull { get; set; } public required bool EmitThrowIfNullMethod { get; set; } } } diff --git a/src/libraries/Microsoft.Extensions.Configuration.Binder/ref/Microsoft.Extensions.Configuration.Binder.cs b/src/libraries/Microsoft.Extensions.Configuration.Binder/ref/Microsoft.Extensions.Configuration.Binder.cs index 0f28d4f2fb9af..c0ee592ed6b87 100644 --- a/src/libraries/Microsoft.Extensions.Configuration.Binder/ref/Microsoft.Extensions.Configuration.Binder.cs +++ b/src/libraries/Microsoft.Extensions.Configuration.Binder/ref/Microsoft.Extensions.Configuration.Binder.cs @@ -32,10 +32,12 @@ public static partial class ConfigurationBinder [System.Diagnostics.CodeAnalysis.RequiresUnreferencedCodeAttribute("In case the type is non-primitive, the trimmer cannot statically analyze the object's type so its members may be trimmed.")] public static object? GetValue(this Microsoft.Extensions.Configuration.IConfiguration configuration, [System.Diagnostics.CodeAnalysis.DynamicallyAccessedMembersAttribute(System.Diagnostics.CodeAnalysis.DynamicallyAccessedMemberTypes.All)] System.Type type, string key) { throw null; } [System.Diagnostics.CodeAnalysis.RequiresUnreferencedCodeAttribute("In case the type is non-primitive, the trimmer cannot statically analyze the object's type so its members may be trimmed.")] + [return: System.Diagnostics.CodeAnalysis.NotNullIfNotNull(nameof(defaultValue))] public static object? GetValue(this Microsoft.Extensions.Configuration.IConfiguration configuration, [System.Diagnostics.CodeAnalysis.DynamicallyAccessedMembersAttribute(System.Diagnostics.CodeAnalysis.DynamicallyAccessedMemberTypes.All)] System.Type type, string key, object? defaultValue) { throw null; } [System.Diagnostics.CodeAnalysis.RequiresUnreferencedCodeAttribute("In case the type is non-primitive, the trimmer cannot statically analyze the object's type so its members may be trimmed.")] public static T? GetValue<[System.Diagnostics.CodeAnalysis.DynamicallyAccessedMembersAttribute(System.Diagnostics.CodeAnalysis.DynamicallyAccessedMemberTypes.All)] T>(this Microsoft.Extensions.Configuration.IConfiguration configuration, string key) { throw null; } [System.Diagnostics.CodeAnalysis.RequiresUnreferencedCodeAttribute("In case the type is non-primitive, the trimmer cannot statically analyze the object's type so its members may be trimmed.")] + [return: System.Diagnostics.CodeAnalysis.NotNullIfNotNull(nameof(defaultValue))] public static T? GetValue<[System.Diagnostics.CodeAnalysis.DynamicallyAccessedMembersAttribute(System.Diagnostics.CodeAnalysis.DynamicallyAccessedMemberTypes.All)] T>(this Microsoft.Extensions.Configuration.IConfiguration configuration, string key, T defaultValue) { throw null; } [System.Diagnostics.CodeAnalysis.RequiresDynamicCodeAttribute("Binding strongly typed objects to configuration values requires generating dynamic code at runtime, for example instantiating generic types.")] [System.Diagnostics.CodeAnalysis.RequiresUnreferencedCodeAttribute("In case the type is non-primitive, the trimmer cannot statically analyze the object's type so its members may be trimmed.")] diff --git a/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs b/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs index c6783555ca475..658653c28c0fe 100644 --- a/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs +++ b/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs @@ -167,6 +167,7 @@ public static void Bind(this IConfiguration configuration, object? instance, Act /// The default value to use if no value is found. /// The converted value. [RequiresUnreferencedCode(TrimmingWarningMessage)] + [return: NotNullIfNotNull(nameof(defaultValue))] public static T? GetValue<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)] T>(this IConfiguration configuration, string key, T defaultValue) { return (T?)GetValue(configuration, typeof(T), key, defaultValue); @@ -198,6 +199,7 @@ public static void Bind(this IConfiguration configuration, object? instance, Act /// The default value to use if no value is found. /// The converted value. [RequiresUnreferencedCode(TrimmingWarningMessage)] + [return: NotNullIfNotNull(nameof(defaultValue))] public static object? GetValue( this IConfiguration configuration, [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)] diff --git a/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/Common/ConfigurationBinderTests.cs b/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/Common/ConfigurationBinderTests.cs index 95585b7623923..22691e7571426 100644 --- a/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/Common/ConfigurationBinderTests.cs +++ b/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/Common/ConfigurationBinderTests.cs @@ -326,11 +326,15 @@ public void CanBindToObjectProperty() [Fact] public void GetNullValue() { - var dic = new Dictionary + #nullable enable + #pragma warning disable IDE0004 // Cast is redundant + + var dic = new Dictionary { {"Integer", null}, {"Boolean", null}, {"Nested:Integer", null}, + {"String", null}, {"Object", null } }; var configurationBuilder = new ConfigurationBuilder(); @@ -341,13 +345,16 @@ public void GetNullValue() Assert.False(config.GetValue("Boolean")); Assert.Equal(0, config.GetValue("Integer")); Assert.Equal(0, config.GetValue("Nested:Integer")); + Assert.Null(config.GetValue("String")); Assert.Null(config.GetValue("Object")); // Generic overloads with default value. - Assert.True(config.GetValue("Boolean", true)); - Assert.Equal(1, config.GetValue("Integer", 1)); - Assert.Equal(1, config.GetValue("Nested:Integer", 1)); - Assert.Equal(new NestedConfig(""), config.GetValue("Object", new NestedConfig(""))); + Assert.True((bool)config.GetValue("Boolean", true)); + Assert.Equal(1, (int)config.GetValue("Integer", 1)); + Assert.Equal(1, (int)config.GetValue("Nested:Integer", 1)); + // [NotNullIfNotNull] avoids CS8600: Converting possible null value to non-nullable type. + Assert.Equal("s", (string)config.GetValue("String", "s")); + Assert.Equal(new NestedConfig(""), (NestedConfig)config.GetValue("Object", new NestedConfig(""))); // Type overloads. Assert.Null(config.GetValue(typeof(bool), "Boolean")); @@ -356,16 +363,22 @@ public void GetNullValue() Assert.Null(config.GetValue(typeof(ComplexOptions), "Object")); // Type overloads with default value. + // [NotNullIfNotNull] avoids CS8605: Unboxing a possibly null value. Assert.True((bool)config.GetValue(typeof(bool), "Boolean", true)); Assert.Equal(1, (int)config.GetValue(typeof(int), "Integer", 1)); Assert.Equal(1, (int)config.GetValue(typeof(int), "Nested:Integer", 1)); - Assert.Equal(new NestedConfig(""), config.GetValue("Object", new NestedConfig(""))); + // [NotNullIfNotNull] avoids CS8600: Converting possible null value to non-nullable type. + Assert.Equal("s", (string)config.GetValue(typeof(string), "String", "s")); + Assert.Equal(new NestedConfig(""), (NestedConfig)config.GetValue("Object", new NestedConfig(""))); // GetSection tests. Assert.False(config.GetSection("Boolean").Get()); Assert.Equal(0, config.GetSection("Integer").Get()); Assert.Equal(0, config.GetSection("Nested:Integer").Get()); Assert.Null(config.GetSection("Object").Get()); + + #pragma warning restore IDE0004 + #nullable restore } [Fact] diff --git a/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/SourceGenerationTests/Baselines/netcoreapp/ConfigurationBinder/GetValue.generated.txt b/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/SourceGenerationTests/Baselines/netcoreapp/ConfigurationBinder/GetValue.generated.txt index 67df05b508e89..72edaaf8c6fba 100644 --- a/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/SourceGenerationTests/Baselines/netcoreapp/ConfigurationBinder/GetValue.generated.txt +++ b/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/SourceGenerationTests/Baselines/netcoreapp/ConfigurationBinder/GetValue.generated.txt @@ -39,6 +39,7 @@ namespace Microsoft.Extensions.Configuration.Binder.SourceGeneration /// Extracts the value with the specified key and converts it to the specified type. [InterceptsLocation(@"src-0.cs", 16, 24)] + [return: global::System.Diagnostics.CodeAnalysis.NotNullIfNotNull(nameof(defaultValue))] public static T? GetValue(this IConfiguration configuration, string key, T defaultValue) => (T?)(BindingExtensions.GetValueCore(configuration, typeof(T), key) ?? defaultValue); /// Extracts the value with the specified key and converts it to the specified type. @@ -47,6 +48,7 @@ namespace Microsoft.Extensions.Configuration.Binder.SourceGeneration /// Extracts the value with the specified key and converts it to the specified type. [InterceptsLocation(@"src-0.cs", 17, 24)] + [return: global::System.Diagnostics.CodeAnalysis.NotNullIfNotNull(nameof(defaultValue))] public static object? GetValue(this IConfiguration configuration, Type type, string key, object? defaultValue) => BindingExtensions.GetValueCore(configuration, type, key) ?? defaultValue; #endregion IConfiguration extensions. diff --git a/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/SourceGenerationTests/Baselines/netcoreapp/ConfigurationBinder/GetValue_T_Key_DefaultValue.generated.txt b/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/SourceGenerationTests/Baselines/netcoreapp/ConfigurationBinder/GetValue_T_Key_DefaultValue.generated.txt index b546b86ea1b9b..c383398924de6 100644 --- a/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/SourceGenerationTests/Baselines/netcoreapp/ConfigurationBinder/GetValue_T_Key_DefaultValue.generated.txt +++ b/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/SourceGenerationTests/Baselines/netcoreapp/ConfigurationBinder/GetValue_T_Key_DefaultValue.generated.txt @@ -35,6 +35,7 @@ namespace Microsoft.Extensions.Configuration.Binder.SourceGeneration #region IConfiguration extensions. /// Extracts the value with the specified key and converts it to the specified type. [InterceptsLocation(@"src-0.cs", 12, 20)] + [return: global::System.Diagnostics.CodeAnalysis.NotNullIfNotNull(nameof(defaultValue))] public static T? GetValue(this IConfiguration configuration, string key, T defaultValue) => (T?)(BindingExtensions.GetValueCore(configuration, typeof(T), key) ?? defaultValue); #endregion IConfiguration extensions. diff --git a/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/SourceGenerationTests/Baselines/netcoreapp/ConfigurationBinder/GetValue_TypeOf_Key_DefaultValue.generated.txt b/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/SourceGenerationTests/Baselines/netcoreapp/ConfigurationBinder/GetValue_TypeOf_Key_DefaultValue.generated.txt index 772160a53bdd5..8371cd0b05ca1 100644 --- a/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/SourceGenerationTests/Baselines/netcoreapp/ConfigurationBinder/GetValue_TypeOf_Key_DefaultValue.generated.txt +++ b/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/SourceGenerationTests/Baselines/netcoreapp/ConfigurationBinder/GetValue_TypeOf_Key_DefaultValue.generated.txt @@ -35,6 +35,7 @@ namespace Microsoft.Extensions.Configuration.Binder.SourceGeneration #region IConfiguration extensions. /// Extracts the value with the specified key and converts it to the specified type. [InterceptsLocation(@"src-0.cs", 11, 20)] + [return: global::System.Diagnostics.CodeAnalysis.NotNullIfNotNull(nameof(defaultValue))] public static object? GetValue(this IConfiguration configuration, Type type, string key, object? defaultValue) => BindingExtensions.GetValueCore(configuration, type, key) ?? defaultValue; #endregion IConfiguration extensions. diff --git a/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/SourceGenerationTests/Microsoft.Extensions.Configuration.Binder.SourceGeneration.Tests.csproj b/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/SourceGenerationTests/Microsoft.Extensions.Configuration.Binder.SourceGeneration.Tests.csproj index eccbe7d759b41..c19f91354cba5 100644 --- a/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/SourceGenerationTests/Microsoft.Extensions.Configuration.Binder.SourceGeneration.Tests.csproj +++ b/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/SourceGenerationTests/Microsoft.Extensions.Configuration.Binder.SourceGeneration.Tests.csproj @@ -8,6 +8,9 @@ $(NoWarn);SYSLIB1103,SYSLIB1104 $(InterceptorsPreviewNamespaces);Microsoft.Extensions.Configuration.Binder.SourceGeneration true + + + true