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

Remove compiler downgrade #32474

Merged
merged 3 commits into from
Feb 20, 2020
Merged

Remove compiler downgrade #32474

merged 3 commits into from
Feb 20, 2020

Conversation

ViktorHofer
Copy link
Member

Let's see what's going to crash :)

@ViktorHofer
Copy link
Member Author

@safern would you mind pushing the necessary nullability fixes into this branch?

@safern
Copy link
Member

safern commented Feb 18, 2020

Of course. Working on it.

@@ -62,6 +62,7 @@ protected override void CreateCollection(ref ReadStack state)
{
string key = GetKeyName(enumerator.Current.Key, ref state, options);
writer.WritePropertyName(key);
Debug.Assert(enumerator.Current.Value != null);
Copy link
Member

Choose a reason for hiding this comment

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

I believe this assert is incorrect and will fail with this, so consider just using null-forgiving (!).

[Fact]
public static void WriteDictionaryNullValue()
{
    var input = new Dictionary<string, string> { { "a", null }, {"b", "foo" } };

    string json = JsonSerializer.Serialize(input);
}

Since this is calling the built-in JsonConverter<string?>, null is allowed and will work if passed in. The StringConverter overwrites the nullability, but I suppose it can't be expressed statically.

public override void Write(Utf8JsonWriter writer, string? value, JsonSerializerOptions options)
{
writer.WriteStringValue(value);
}

Is there something we can do here?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we can do anything here. Since the converter here is declared as the abstract type, the compiler will resolve the signature that contains the DisallowNull annotation. So I think we should just use a !. I think we can't declare the converter variable type as JsonConverter<string?>, right?

Copy link
Member

Choose a reason for hiding this comment

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

No, it has to remain generic. It just happens to be that all the Ts in the particular set of JsonConverter<T> work with null values (because our internal converters know how to deal with it).

I think, ideally, we will re-write parts of this to add special handling for null and honor the DisallowNull even internally.

Copy link
Member

Choose a reason for hiding this comment

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

I think that would make sense. Internal usage of these APIs should overall honor the intent and what we tell our customers. It would be great if the usage of ! was lower.

@safern safern force-pushed the ViktorHofer-patch-1 branch from 54ae3ce to c923dce Compare February 19, 2020 22:06
@safern safern force-pushed the ViktorHofer-patch-1 branch from c923dce to 50c206b Compare February 19, 2020 22:47
Copy link
Member

@ahsonkhan ahsonkhan left a comment

Choose a reason for hiding this comment

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

Thanks for doing this, @safern.

@safern safern force-pushed the ViktorHofer-patch-1 branch from 50c206b to ca2e3bf Compare February 19, 2020 23:20
@safern safern force-pushed the ViktorHofer-patch-1 branch from ca2e3bf to f2562bf Compare February 20, 2020 17:21
@safern safern merged commit 64fddce into master Feb 20, 2020
@safern safern deleted the ViktorHofer-patch-1 branch February 20, 2020 22:28
@ViktorHofer
Copy link
Member Author

Thanks Santi, you are my hero :)

@ghost ghost locked as resolved and limited conversation to collaborators Dec 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants