Skip to content

Commit

Permalink
Annotate config.GetValue() with [NotNullIfNotNull] (dotnet#101336)
Browse files Browse the repository at this point in the history
* Annotate GetValue() with [NotNullIfNotNull]

* Avoid duplicate NullableAttributes.cs

* Annotate generated GetValue() with [NotNullIfNotNull]
  • Loading branch information
dahlbyk authored and michaelgsharp committed May 8, 2024
1 parent 7d273fe commit 0bdc91e
Show file tree
Hide file tree
Showing 13 changed files with 49 additions and 6 deletions.
1 change: 1 addition & 0 deletions src/libraries/Directory.Build.targets
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,7 @@
<!-- Adds Nullable annotation attributes to C# non .NETCoreApp builds. -->
<ItemGroup Condition="'$(Nullable)' != '' and
'$(Nullable)' != 'disable' and
'$(SkipIncludeNullableAttributes)' != 'true' and
'$(MSBuildProjectExtension)' == '.csproj' and
'$(TargetFrameworkIdentifier)' != '.NETCoreApp'">
<Compile Include="$(CoreLibSharedDir)System\Diagnostics\CodeAnalysis\NullableAttributes.cs" Link="System\Diagnostics\CodeAnalysis\NullableAttributes.cs" />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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()
};
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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}<T>(this {Identifier.IConfiguration} {Identifier.configuration}, string {Identifier.key}, T {Identifier.defaultValue}) => " +
$"(T?)({expressionForGetValueCore}({Identifier.configuration}, typeof(T), {Identifier.key}) ?? {Identifier.defaultValue});");
}
Expand All @@ -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))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand Down Expand Up @@ -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");
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ public sealed record SourceGenerationSpec
public required ImmutableEquatableArray<TypeSpec> 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; }
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,12 @@ public static void Bind(this Microsoft.Extensions.Configuration.IConfiguration c
[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.")]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,7 @@ public static void Bind(this IConfiguration configuration, object? instance, Act
/// <param name="defaultValue">The default value to use if no value is found.</param>
/// <returns>The converted value.</returns>
[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);
Expand Down Expand Up @@ -198,6 +199,7 @@ public static void Bind(this IConfiguration configuration, object? instance, Act
/// <param name="defaultValue">The default value to use if no value is found.</param>
/// <returns>The converted value.</returns>
[RequiresUnreferencedCode(TrimmingWarningMessage)]
[return: NotNullIfNotNull(nameof(defaultValue))]
public static object? GetValue(
this IConfiguration configuration,
[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -326,11 +326,15 @@ public void CanBindToObjectProperty()
[Fact]
public void GetNullValue()
{
var dic = new Dictionary<string, string>
#nullable enable
#pragma warning disable IDE0004 // Cast is redundant

var dic = new Dictionary<string, string?>
{
{"Integer", null},
{"Boolean", null},
{"Nested:Integer", null},
{"String", null},
{"Object", null }
};
var configurationBuilder = new ConfigurationBuilder();
Expand All @@ -341,13 +345,16 @@ public void GetNullValue()
Assert.False(config.GetValue<bool>("Boolean"));
Assert.Equal(0, config.GetValue<int>("Integer"));
Assert.Equal(0, config.GetValue<int>("Nested:Integer"));
Assert.Null(config.GetValue<string>("String"));
Assert.Null(config.GetValue<ComplexOptions>("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"));
Expand All @@ -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<bool>());
Assert.Equal(0, config.GetSection("Integer").Get<int>());
Assert.Equal(0, config.GetSection("Nested:Integer").Get<int>());
Assert.Null(config.GetSection("Object").Get<ComplexOptions>());

#pragma warning restore IDE0004
#nullable restore
}

[Fact]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ namespace Microsoft.Extensions.Configuration.Binder.SourceGeneration

/// <summary>Extracts the value with the specified key and converts it to the specified type.</summary>
[InterceptsLocation(@"src-0.cs", 16, 24)]
[return: global::System.Diagnostics.CodeAnalysis.NotNullIfNotNull(nameof(defaultValue))]
public static T? GetValue<T>(this IConfiguration configuration, string key, T defaultValue) => (T?)(BindingExtensions.GetValueCore(configuration, typeof(T), key) ?? defaultValue);

/// <summary>Extracts the value with the specified key and converts it to the specified type.</summary>
Expand All @@ -47,6 +48,7 @@ namespace Microsoft.Extensions.Configuration.Binder.SourceGeneration

/// <summary>Extracts the value with the specified key and converts it to the specified type.</summary>
[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.

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ namespace Microsoft.Extensions.Configuration.Binder.SourceGeneration
#region IConfiguration extensions.
/// <summary>Extracts the value with the specified key and converts it to the specified type.</summary>
[InterceptsLocation(@"src-0.cs", 12, 20)]
[return: global::System.Diagnostics.CodeAnalysis.NotNullIfNotNull(nameof(defaultValue))]
public static T? GetValue<T>(this IConfiguration configuration, string key, T defaultValue) => (T?)(BindingExtensions.GetValueCore(configuration, typeof(T), key) ?? defaultValue);
#endregion IConfiguration extensions.

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ namespace Microsoft.Extensions.Configuration.Binder.SourceGeneration
#region IConfiguration extensions.
/// <summary>Extracts the value with the specified key and converts it to the specified type.</summary>
[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.

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@
<NoWarn>$(NoWarn);SYSLIB1103,SYSLIB1104</NoWarn>
<InterceptorsPreviewNamespaces>$(InterceptorsPreviewNamespaces);Microsoft.Extensions.Configuration.Binder.SourceGeneration</InterceptorsPreviewNamespaces>
<EnableConfigurationBindingGenerator>true</EnableConfigurationBindingGenerator>

<!-- Avoid conflict with Microsoft.Extensions.Configuration.Binder (via InternalsVisibleTo) -->
<SkipIncludeNullableAttributes>true</SkipIncludeNullableAttributes>
</PropertyGroup>

<PropertyGroup>
Expand Down

0 comments on commit 0bdc91e

Please sign in to comment.