Skip to content

Conversation

@StephenMolloy
Copy link
Member

@StephenMolloy StephenMolloy commented Aug 11, 2025

Fixes #78704.

CodeExporter was ported from NetFx code where DataContract and all its various sub-types were all internally visible. Also being code from the old world, it leveraged many beliefs that it assumed to be true based on knowing what sub-type of DataContract was being worked with. Now that this export code lives in a separate assembly and has minimal access to the internals of DataContract and virtually no access to any of its subtypes, it's time to start explicitly stating the things that CodeExporter assumes to be true.

This pull request improves code safety and robustness in CodeExporter.cs by adding Debug.Assert statements to validate key assumptions about non-null values and object state throughout the codebase. These assertions help catch errors earlier during development and clarify expectations for future maintainers.

Assertions for required object state:

  • Added Debug.Assert checks to ensure that TypeDeclaration and TypeReference are non-null before using them in nested type generation, class contract export, and serializable contract export. [1] [2] [3] [4]
    • These asserts rely on the knowledge that class and collection data contracts should always have TypeReference/Declarations - which will be reflected in the "info" objects being worked with here. (Not all sub-classes of DataContract do.)
  • Added assertions to verify that BaseContract is not null for CollectionDataContract and EnumDataContract scenarios, including collection name checks and item contract usage. [1] [2] [3] [4] [5]
    • Again, knowing the specific sub-type of DataContract being worked with here allows us to assert that a 'BaseContract' exists.

Validation of attribute and reference values:

  • Added Debug.Assert to confirm assembly name is not null when generating code attributes, and to ensure base type references and item names are present when exporting collection data contracts. [1] [2]

These changes collectively make the code more defensive and easier to debug by catching unexpected null values and invalid states early in the development cycle.

@StephenMolloy StephenMolloy added this to the 10.0.0 milestone Aug 11, 2025
@StephenMolloy StephenMolloy self-assigned this Aug 11, 2025
Copilot AI review requested due to automatic review settings August 11, 2025 22:37
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR enhances code safety in the CodeExporter.cs by adding Debug.Assert statements to validate assumptions that were previously implicit when the code had internal access to DataContract subtypes. The changes make the code more defensive and help catch issues during development by explicitly verifying expected object states and non-null values.

Key changes:

  • Added assertions to validate that TypeDeclaration and TypeReference properties are non-null before use
  • Added assertions to confirm BaseContract is not null for specific DataContract subtypes where this is guaranteed
  • Added validation for assembly names and item names that are expected to be non-null in specific contexts

@mconnew
Copy link
Member

mconnew commented Aug 27, 2025

What was the rationale behind using Debug.Assert over throwing an exception? Is it the case that we're very sure this isn't possible with the current code and you're adding these to protect against regressions if/when things are refactored? If it's possible for an end user to do something to cause any of these assertions to be incorrect, then we should throw an exception.

@StephenMolloy
Copy link
Member Author

StephenMolloy commented Aug 28, 2025

What was the rationale behind using Debug.Assert over throwing an exception? Is it the case that we're very sure this isn't possible with the current code and you're adding these to protect against regressions if/when things are refactored? If it's possible for an end user to do something to cause any of these assertions to be incorrect, then we should throw an exception.

The code here came over from NetFx. I don't know for certain that it's impossible for and end user to cause these assertions to be wrong - the type structure certainly doesn't prescribe that these asserts are always true. With that said, I think the reason many of these assertions need to be made is because this is an external package, and only knows about the base DataContract type, so it can't officially know that "Collection" contracts always have an item type and stuff like that. I think this would be cleaner if the exporter code lived in the same assembly as DataContracts, but it doesn't.

Mostly, asserts don't change behavior while exceptions potentially could. I suppose that was the mindset - don't change behavior to work around the syntactic compiler sugar warnings. If you feel strongly about putting teeth behind the nullable notations, my only concern would be that were already past the preview phase for .Net 10.

@StephenMolloy StephenMolloy modified the milestones: 10.0.0, 11.0.0 Sep 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Review usage of nullable suppressions (especially !) in CodeExporter.cs

2 participants