Enable nullability in ResourceCodeDomSerializer.SerializationResourceManager

[halgab]

As dicussed here

Proposed changes

• Enable nullability in ResourceCodeDomSerializer.SerializationResourceManager
• Fix what looks to be a bug introduced in Refactor SerializationResourceManager to use Dictionary instead of HashTable #8332

Microsoft Reviewers: Open in CodeFlow

[halgab]

Before #8332 the sentinel value used to be an object. As ResourceTable was a Hashtable, it was fine putting both dictionaries and simple objects in it. But changing it to a strongly typed dictionary, the type of the sentinel was also changed to a dictionary. The problem with that approach is that now we're actually updating the sentinel dictionary. Replacing the sentinel object by null should solve it

[halgab]

@elachlan

[elachlan]

Thank you for taking this on.

[halgab]

Note that if we merge this in .Net 9 timeframe, we are going to keep a bug that was not here in .Net 7. Is this deemed less risky than taking a fix so late? @dreddy-work

[halgab]

@dreddy-work do you want me to send over a PR just with the bugfix for .Net 8?

[dreddy-work]

@dreddy-work do you want me to send over a PR just with the bugfix for .Net 8?

Yes please. We are trying to mitigate risk by limiting what we ship in .NET 8.0.

[elachlan]

@dreddy-work do you want me to send over a PR just with the bugfix for .Net 8?

I'd just submit the PR and let them decide.

[halgab]

I've just pushed #9642. After taking another look, I found another introduced by #8332. I guess there's no point in updating this PR accordingly for now

[halgab]

Now that #9642 was merged, I rebased this one, removing the second commit that was about the bugfix.

[lonitra]

I added a few comments. Appreciate your patience here

[lonitra]

Please add comment of how do we know these are not null

[halgab]

Well, I don't know the actual reason... I just know the call sites of those properties always expect them not to be null as they are used as parameters in manager.Context.Push calls. That's another case of inconsistent checks I think

[lonitra]

I see, it looks like these are used as arguments to manager.Context.Push only when isMetadata = false. So it seems that it is OK for ResourceEntry.PropertyDescription and ResourceEntry.ExpressionContext to be nullable and should be annotated as so. If these are null when isMetadata = false, we should actually throw ArgumentException here since manager did not have the non-nullables required instead of later getting an ArgumentNullException from manager.Context.Push which might be confusing.

[halgab]

I've pushed something to address this. I've tried to stick to what you indicated. Let me know if this needs some more tweaking

[lonitra]

How do we know this is not nullable?

[halgab]

GetResourceSet returns null only in an error scenario. Which I know is not satisfying... As often in internal classes, checks are pretty inconsistent

[lonitra]

Generally we are moving to add comments whenever ! is used, but given that the codebase is so old, it can be hard to determine when exactly things are null in which case we should throw a specific exception instead of getting an NRE. For this case I think the right answer would be to throw an InvalidOperationException, but of course if there was more context, we should throw exception that would be most helpful.

if (invariant is null) {
    throw new InvalidOperationException();
}

invariant.Remove(resourceName);

[halgab]

I was too quick in my first reply. Turns out that we can perfectly explain why invariant can't be null. I've added a comment near the null forgiving operator (which move earlier in the code).

[lonitra]

Awesome