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

[release/7.0] Disable nullability warnings in JSON source generator #74861

Merged
merged 2 commits into from
Sep 1, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,9 @@ private void AddSource(string fileName, string source, bool isRootContextDef = f
bool isInGlobalNamespace = @namespace == JsonConstants.GlobalNamespaceValue;

StringBuilder sb = new(@"// <auto-generated/>
#nullable enable

#nullable enable annotations
#nullable disable warnings
Copy link
Member

Choose a reason for hiding this comment

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

Are we ever generating public API? With annotations enabled we will still be outputting nullability metadata, and if we have reason to believe that the annotations are incorrect, that'll mean incorrect nullability information surfaced to consumers of the APIs.

If this is all internal to the assembly, it's probably fine. If it's not, we might instead consider doing:

#nullable disable
#pragma warning disable CS8632

instead, or something along those lines.

Copy link
Member

Choose a reason for hiding this comment

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

@eiriktsarpalis do you want to resolve this question before I hit merge?

Copy link
Member

Choose a reason for hiding this comment

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

Yes let's please resolve this q first.

Copy link
Member

Choose a reason for hiding this comment

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

we do produce some public APIs - AFAIK they're correct though

Copy link
Member

Choose a reason for hiding this comment

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

they're correct though

Ok


// Suppress warnings about [Obsolete] member usage in generated code.
#pragma warning disable CS0618");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -380,6 +380,37 @@ public static void SupportsGenericParameterWithCustomConverterFactory()
Assert.Equal(@"[""Cee""]", json);
}

// Regression test for https://github.com/dotnet/runtime/issues/74652
[Fact]
public static void ClassWithStringValuesRoundtrips()
{
JsonSerializerOptions options = ClassWithStringValuesContext.Default.Options;

ClassWithStringValues obj = new()
{
StringValuesProperty = new(new[] { "abc", "def" })
};

string json = JsonSerializer.Serialize(obj, options);
Assert.Equal("""{"StringValuesProperty":["abc","def"]}""", json);
}

// Regression test for https://github.com/dotnet/runtime/issues/61734
[Fact]
public static void ClassWithDictionaryPropertyRoundtrips()
{
JsonSerializerOptions options = ClassWithDictionaryPropertyContext.Default.Options;

ClassWithDictionaryProperty obj = new(new Dictionary<string, object?>()
{
["foo"] = "bar",
["test"] = "baz",
});

string json = JsonSerializer.Serialize(obj, options);
Assert.Equal("""{"DictionaryProperty":{"foo":"bar","test":"baz"}}""", json);
}

[JsonConverter(typeof(JsonStringEnumConverter))]
public enum TestEnum
{
Expand All @@ -394,7 +425,16 @@ internal partial class GenericParameterWithCustomConverterFactoryContext : JsonS
[JsonSerializable(typeof(ClassWithPocoListDictionaryAndNullable))]
internal partial class ClassWithPocoListDictionaryAndNullablePropertyContext : JsonSerializerContext
{
}

[JsonSerializable(typeof(ClassWithStringValues))]
internal partial class ClassWithStringValuesContext : JsonSerializerContext
{
}

[JsonSerializable(typeof(ClassWithDictionaryProperty))]
internal partial class ClassWithDictionaryPropertyContext : JsonSerializerContext
{
}

internal class ClassWithPocoListDictionaryAndNullable
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,10 @@
<Compile Include="$(CoreLibSharedDir)System\Diagnostics\CodeAnalysis\DynamicallyAccessedMemberTypes.cs" />
</ItemGroup>

<ItemGroup>
<ProjectReference Include="$(LibrariesProjectRoot)Microsoft.Extensions.Primitives\src\Microsoft.Extensions.Primitives.csproj" />
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't feel right to me. I know these are tests, but having something in System depend on things in Microsoft.Extensions feels backwards.

Is there something about StringValues that we can isolate into a unit test type? Or do we actually have to use the StringValues type in order to test the scenario?

Copy link
Member

Choose a reason for hiding this comment

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

StringValues is what customer reported so it was added here as a test to make sure that specific scenario works. I think that should be fine for tests, we already depend on other things like JSON.NET

</ItemGroup>

<Target Name="FixIncrementalCoreCompileWithAnalyzers" BeforeTargets="CoreCompile">
<ItemGroup>
<CustomAdditionalCompileInputs Include="@(Analyzer)" />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

using System.Collections.Generic;
using System.Text.Json.Serialization;
using Microsoft.Extensions.Primitives;

namespace System.Text.Json.SourceGeneration.Tests.RepeatedTypes
{
Expand Down Expand Up @@ -275,4 +276,15 @@ public class PublicTestClass
{
internal class InternalNestedClass { }
}

public sealed class ClassWithStringValues
{
public StringValues StringValuesProperty { get; set; }
}

public class ClassWithDictionaryProperty
{
public ClassWithDictionaryProperty(Dictionary<string, object?> property) => DictionaryProperty = property;
public Dictionary<string, object?> DictionaryProperty { get; }
}
}