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

WithEnforceNullabilty() has unpredictable behavior with classes having both nullable reference type and value type #1018

Closed
popemkt opened this issue Dec 3, 2024 · 5 comments · Fixed by #1025

Comments

@popemkt
Copy link

popemkt commented Dec 3, 2024

Describe the bug

When deserializing to a class with both nullable reference and value types members, YamlDotNet considers the nullabe value type to not accept null values.

I suspect it's coming from the reflection checks only checking if a member allows null using the presence of NullableAttribute, which fails if it's a TValueType? (this won't generate a NullableAttribute). But I've seen strange cases where the compiler doesn't even generate NullableAttribute on a reference type, so I won't be too sure about that.

To Reproduce
Using latest version: 16.2.1

Create a new console project with .net8
Program.cs

#nullable enable
using YamlDotNet.Serialization;

var deserializer = new DeserializerBuilder().WithEnforceNullability().Build();
var yaml = @"
TestString: null
TestBool: null
";
var test = deserializer.Deserialize<NonNullableClass>(yaml);

public class NonNullableClass
{
    public string? TestString { get; set; }
    public bool? TestBool { get; set; }
}

Result:

Unhandled exception. (Line: 2, Col: 1, Idx: 2) - (Line: 2, Col: 11, Idx: 12): Strict nullability enforcement error.

@popemkt
Copy link
Author

popemkt commented Dec 4, 2024

Update, the example will give an error even when I use the exact setup as in one of the tests, but with a ? notation on the property

#nullable enable
using YamlDotNet.Serialization;

var deserializer = new DeserializerBuilder().WithEnforceNullability().Build();
var yaml = @"
TestString: null
";
var test = deserializer.Deserialize<NonNullableClass>(yaml);

public class NonNullableClass
{
    public string? TestString { get; set; }
}

Result: Unhandled exception. (Line: 2, Col: 1, Idx: 2) - (Line: 2, Col: 11, Idx: 12): Strict nullability enforcement error.
Here's the lowered version of the class (no NullableAttribute on the property)

[NullableContext(2)]
[Nullable(0)]
public class NonNullableClass
{
  [CompilerGenerated]
  [DebuggerBrowsable(DebuggerBrowsableState.Never)]
  private string <TestString>k__BackingField;

  public string TestString
  {
    [CompilerGenerated] get
    {
      return this.<TestString>k__BackingField;
    }
    [CompilerGenerated] set
    {
      this.<TestString>k__BackingField = value;
    }
  }

  public NonNullableClass()
  {
    base..ctor();
  }
}

I think this is because way the compiler generate NullableAttribute is really unreliable to base the checks on.

@EdwardCooke
Copy link
Collaborator

I’m not at a computer for a bit. But. In your second example, what happens when you put a non nullable, no question mark, string in there? What is generated? I wonder if there’s an optimization where if all properties/members are nullable then it only puts the nullable attributes at the class level and not on individual properties and fields. That’s just a guess though.

@popemkt
Copy link
Author

popemkt commented Dec 6, 2024

I guess you might be right,
Here's the version without ?

[NullableContext(1)]
[Nullable(0)]
public class NonNullableClass
{
  [CompilerGenerated]
  [DebuggerBrowsable(DebuggerBrowsableState.Never)]
  private string <TestString>k__BackingField;

  public string TestString
  {
    [CompilerGenerated] get
    {
      return this.<TestString>k__BackingField;
    }
    [CompilerGenerated] set
    {
      this.<TestString>k__BackingField = value;
    }
  }

  public NonNullableClass()
  {
    this.<TestString>k__BackingField = "Default";
    base..ctor();
  }
}

@popemkt
Copy link
Author

popemkt commented Dec 23, 2024

@EdwardCooke did you have any time to look at this?

@EdwardCooke
Copy link
Collaborator

Thanks for the reminder. I just did some digging. Looks like there is some optimization that happens with nullable properties at the compiler level.

dotnet/roslyn#39133

I have implemented the required changes so we should be good once I cut a release for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants