Skip to content

Commit d2b5550

Browse files
StephenMolloyCopilotCopilot
authored
Explicitly asserting CodeExporter beliefs about specific types of DataContracts (#118616)
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]](diffhunk://#diff-aed5d73b49483bcc99c574cae5c5e7635ba4dcb43fa6ecc1404e6d95e1a9feebL489-R490) [[2]](diffhunk://#diff-aed5d73b49483bcc99c574cae5c5e7635ba4dcb43fa6ecc1404e6d95e1a9feebL505-R507) [[3]](diffhunk://#diff-aed5d73b49483bcc99c574cae5c5e7635ba4dcb43fa6ecc1404e6d95e1a9feebL822-R829) [[4]](diffhunk://#diff-aed5d73b49483bcc99c574cae5c5e7635ba4dcb43fa6ecc1404e6d95e1a9feebL1155-R1165) - 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]](diffhunk://#diff-aed5d73b49483bcc99c574cae5c5e7635ba4dcb43fa6ecc1404e6d95e1a9feebR613) [[2]](diffhunk://#diff-aed5d73b49483bcc99c574cae5c5e7635ba4dcb43fa6ecc1404e6d95e1a9feebR636) [[3]](diffhunk://#diff-aed5d73b49483bcc99c574cae5c5e7635ba4dcb43fa6ecc1404e6d95e1a9feebR655) [[4]](diffhunk://#diff-aed5d73b49483bcc99c574cae5c5e7635ba4dcb43fa6ecc1404e6d95e1a9feebR1078) [[5]](diffhunk://#diff-aed5d73b49483bcc99c574cae5c5e7635ba4dcb43fa6ecc1404e6d95e1a9feebL1205-R1216) - 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]](diffhunk://#diff-aed5d73b49483bcc99c574cae5c5e7635ba4dcb43fa6ecc1404e6d95e1a9feebL521-R525) [[2]](diffhunk://#diff-aed5d73b49483bcc99c574cae5c5e7635ba4dcb43fa6ecc1404e6d95e1a9feebR1248-R1258) 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. --------- Co-authored-by: Copilot <copilot@github.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
1 parent 8c9b352 commit d2b5550

1 file changed

Lines changed: 31 additions & 16 deletions

File tree

  • src/libraries/System.Runtime.Serialization.Schema/src/System/Runtime/Serialization/Schema

src/libraries/System.Runtime.Serialization.Schema/src/System/Runtime/Serialization/Schema/CodeExporter.cs

Lines changed: 31 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -312,7 +312,7 @@ internal CodeTypeReference GetCodeTypeReference(DataContract dataContract)
312312

313313
// This is supposed to be set by GenerateType. If it wasn't, there is a problem.
314314
Debug.Assert(contractCodeDomInfo.TypeReference != null);
315-
return contractCodeDomInfo.TypeReference!;
315+
return contractCodeDomInfo.TypeReference;
316316
}
317317

318318
private CodeTypeReference GetCodeTypeReference(Type type)
@@ -493,7 +493,8 @@ private void GenerateType(DataContract dataContract, ContractCodeDomInfo contrac
493493
if (containingContractCodeDomInfo.ReferencedTypeExists)
494494
return null;
495495

496-
CodeTypeDeclaration containingType = containingContractCodeDomInfo.TypeDeclaration!; // Nested types by definition have containing types.
496+
Debug.Assert(containingContractCodeDomInfo.TypeDeclaration != null, "Nested types have containing types by definition - types with declaration");
497+
CodeTypeDeclaration containingType = containingContractCodeDomInfo.TypeDeclaration;
497498
if (TypeContainsNestedType(containingType, nestedTypeName))
498499
{
499500
for (int i = 1; ; i++)
@@ -509,7 +510,8 @@ private void GenerateType(DataContract dataContract, ContractCodeDomInfo contrac
509510

510511
CodeTypeDeclaration type = CreateTypeDeclaration(nestedTypeName, dataContract);
511512
containingType.Members.Add(type);
512-
contractCodeDomInfo.TypeReference = new CodeTypeReference(containingContractCodeDomInfo.TypeReference!.BaseType + "+" + nestedTypeName); // Again, nested types by definition have containing types.
513+
Debug.Assert(containingContractCodeDomInfo.TypeReference != null, "Nested types have containing types by definition - types with reference");
514+
contractCodeDomInfo.TypeReference = new CodeTypeReference(containingContractCodeDomInfo.TypeReference.BaseType + "+" + nestedTypeName);
513515

514516
if (GenerateInternalTypes)
515517
type.TypeAttributes = TypeAttributes.NestedAssembly;
@@ -525,8 +527,9 @@ private static CodeTypeDeclaration CreateTypeDeclaration(string typeName, DataCo
525527
CodeAttributeDeclaration generatedCodeAttribute = new CodeAttributeDeclaration(typeof(GeneratedCodeAttribute).FullName!);
526528

527529
AssemblyName assemblyName = Assembly.GetExecutingAssembly().GetName();
528-
generatedCodeAttribute.Arguments.Add(new CodeAttributeArgument(new CodePrimitiveExpression(assemblyName.Name!)));
529-
generatedCodeAttribute.Arguments.Add(new CodeAttributeArgument(new CodePrimitiveExpression(assemblyName.Version?.ToString()!)));
530+
Debug.Assert(assemblyName.Name != null, $"Current executing assembly name is not expected to be null in {nameof(CodeExporter)}.{nameof(CreateTypeDeclaration)} scenario");
531+
generatedCodeAttribute.Arguments.Add(new CodeAttributeArgument(new CodePrimitiveExpression(assemblyName.Name)));
532+
generatedCodeAttribute.Arguments.Add(new CodeAttributeArgument(new CodePrimitiveExpression(assemblyName.Version?.ToString())));
530533

531534
// System.Diagnostics.DebuggerStepThroughAttribute not allowed on enums
532535
// ensure that the attribute is only generated on types that are not enums
@@ -614,7 +617,8 @@ private static CodeTypeDeclaration CreateTypeDeclaration(string typeName, DataCo
614617
if (!TryGetReferencedDictionaryType(collectionContract, out typeReference))
615618
{
616619
// ItemContract - aka BaseContract - is never null for CollectionDataContract
617-
DataContract itemContract = collectionContract.BaseContract!;
620+
Debug.Assert(collectionContract.BaseContract != null, "BaseContract should not be null for CollectionDataContract");
621+
DataContract itemContract = collectionContract.BaseContract;
618622
if (collectionContract.IsDictionaryLike(out _, out _, out _))
619623
{
620624
GenerateKeyValueType(itemContract.As(DataContractType.ClassDataContract));
@@ -635,10 +639,11 @@ private static CodeTypeDeclaration CreateTypeDeclaration(string typeName, DataCo
635639
[RequiresUnreferencedCode(ImportGlobals.SerializerTrimmerWarning)]
636640
private bool HasDefaultCollectionNames(DataContract collectionContract)
637641
{
642+
// ItemContract - aka BaseContract - is never null for CollectionDataContract
638643
Debug.Assert(collectionContract.Is(DataContractType.CollectionDataContract));
644+
Debug.Assert(collectionContract.BaseContract != null, "BaseContract should not be null for CollectionDataContract");
639645

640-
// ItemContract - aka BaseContract - is never null for CollectionDataContract
641-
DataContract itemContract = collectionContract.BaseContract!;
646+
DataContract itemContract = collectionContract.BaseContract;
642647
bool isDictionary = collectionContract.IsDictionaryLike(out string? keyName, out string? valueName, out string? itemName);
643648
if (itemName != itemContract.XmlName.Name)
644649
return false;
@@ -654,6 +659,7 @@ private bool HasDefaultCollectionNames(DataContract collectionContract)
654659
private bool TryGetReferencedDictionaryType(DataContract collectionContract, [NotNullWhen(true)] out CodeTypeReference? typeReference)
655660
{
656661
Debug.Assert(collectionContract.Is(DataContractType.CollectionDataContract));
662+
Debug.Assert(collectionContract.BaseContract != null, "BaseContract should not be null for CollectionDataContract");
657663

658664
// Check if it is a dictionary and use referenced dictionary type if present
659665
if (collectionContract.IsDictionaryLike(out _, out _, out _)
@@ -662,7 +668,7 @@ private bool TryGetReferencedDictionaryType(DataContract collectionContract, [No
662668
Type? type = _dataContractSet.GetReferencedType(GenericDictionaryName, GenericDictionaryContract, out DataContract? _, out object[]? _) ?? typeof(Dictionary<,>);
663669

664670
// ItemContract - aka BaseContract - is never null for CollectionDataContract
665-
DataContract? itemContract = collectionContract.BaseContract!.As(DataContractType.ClassDataContract);
671+
DataContract? itemContract = collectionContract.BaseContract.As(DataContractType.ClassDataContract);
666672

667673
// A dictionary should have a Key/Value item contract that has at least two members: key and value.
668674
Debug.Assert(itemContract != null);
@@ -693,7 +699,7 @@ private bool TryGetReferencedListType(DataContract itemContract, bool isItemType
693699
if (type != null)
694700
{
695701
typeReference = GetCodeTypeReference(type);
696-
typeReference.TypeArguments.Add(GetElementTypeReference(itemContract, isItemTypeNullable)!); // Lists have an item type
702+
typeReference.TypeArguments.Add(GetElementTypeReference(itemContract, isItemTypeNullable)); // Lists have an item type
697703
return true;
698704
}
699705
}
@@ -826,7 +832,8 @@ private void ExportClassDataContract(DataContract classDataContract, ContractCod
826832
{
827833
ContractCodeDomInfo baseContractCodeDomInfo = GetContractCodeDomInfo(classDataContract.BaseContract);
828834
Debug.Assert(baseContractCodeDomInfo.IsProcessed, "Cannot generate code for type if code for base type has not been generated");
829-
type.BaseTypes.Add(baseContractCodeDomInfo.TypeReference!);
835+
Debug.Assert(baseContractCodeDomInfo.TypeReference != null, "Class data contracts should have non-null TypeReference");
836+
type.BaseTypes.Add(baseContractCodeDomInfo.TypeReference);
830837
AddBaseMemberNames(baseContractCodeDomInfo, contractCodeDomInfo);
831838
if (baseContractCodeDomInfo.ReferencedTypeExists)
832839
{
@@ -1075,7 +1082,8 @@ private void ExportEnumDataContract(DataContract enumDataContract, ContractCodeD
10751082

10761083
CodeTypeDeclaration type = contractCodeDomInfo.TypeDeclaration;
10771084
// BaseContract is never null for EnumDataContract
1078-
Type baseType = enumDataContract.BaseContract!.UnderlyingType;
1085+
Debug.Assert(enumDataContract.BaseContract != null, "BaseContract should not be null for EnumDataContract");
1086+
Type baseType = enumDataContract.BaseContract.UnderlyingType;
10791087
type.IsEnum = true;
10801088
type.BaseTypes.Add(baseType);
10811089
if (baseType.IsDefined(typeof(FlagsAttribute), false))
@@ -1159,7 +1167,9 @@ private void ExportISerializableDataContract(DataContract classDataContract, Con
11591167
{
11601168
ContractCodeDomInfo baseContractCodeDomInfo = GetContractCodeDomInfo(classDataContract.BaseContract);
11611169
GenerateType(classDataContract.BaseContract, baseContractCodeDomInfo);
1162-
type.BaseTypes.Add(baseContractCodeDomInfo.TypeReference!);
1170+
1171+
Debug.Assert(baseContractCodeDomInfo.TypeReference != null, "Class data contracts should have non-null TypeReference");
1172+
type.BaseTypes.Add(baseContractCodeDomInfo.TypeReference);
11631173
if (baseContractCodeDomInfo.ReferencedTypeExists)
11641174
{
11651175
Type? actualType = (Type?)baseContractCodeDomInfo.TypeReference?.UserData[s_codeUserDataActualTypeKey];
@@ -1210,6 +1220,7 @@ private void ExportCollectionDataContract(DataContract collectionContract, Contr
12101220
collectionContract.XmlName.Namespace)));
12111221

12121222
// ItemContract - aka BaseContract - is never null for CollectionDataContract
1223+
Debug.Assert(collectionContract.BaseContract != null, "BaseContract should not be null for CollectionDataContract");
12131224
DataContract itemContract = collectionContract.BaseContract!;
12141225
bool isItemTypeNullable = GetCollectionItemNullability(collectionContract);
12151226
bool isDictionary = collectionContract.IsDictionaryLike(out string? keyName, out string? valueName, out string? itemName);
@@ -1242,15 +1253,17 @@ private void ExportCollectionDataContract(DataContract collectionContract, Contr
12421253

12431254
// This is supposed to be set by GenerateType. If it wasn't, there is a problem.
12441255
Debug.Assert(contractCodeDomInfo.TypeDeclaration != null);
1256+
Debug.Assert(baseTypeReference != null, "Base type reference should not be null for Dictionary/List collection data contracts");
12451257

12461258
CodeTypeDeclaration generatedType = contractCodeDomInfo.TypeDeclaration;
1247-
generatedType.BaseTypes.Add(baseTypeReference!);
1259+
generatedType.BaseTypes.Add(baseTypeReference);
12481260
CodeAttributeDeclaration collectionContractAttribute = new CodeAttributeDeclaration(GetClrTypeFullName(typeof(CollectionDataContractAttribute)));
12491261
collectionContractAttribute.Arguments.Add(new CodeAttributeArgument(ImportGlobals.NameProperty, new CodePrimitiveExpression(dataContractName)));
12501262
collectionContractAttribute.Arguments.Add(new CodeAttributeArgument(ImportGlobals.NamespaceProperty, new CodePrimitiveExpression(collectionContract.XmlName.Namespace)));
12511263
if (collectionContract.IsReference != ImportGlobals.DefaultIsReference)
12521264
collectionContractAttribute.Arguments.Add(new CodeAttributeArgument(ImportGlobals.IsReferenceProperty, new CodePrimitiveExpression(collectionContract.IsReference)));
1253-
collectionContractAttribute.Arguments.Add(new CodeAttributeArgument(ImportGlobals.ItemNameProperty, new CodePrimitiveExpression(GetNameForAttribute(itemName!)))); // ItemName is never null for Collection contracts.
1265+
Debug.Assert(itemName != null, "ItemName is never null for Collection contracts.");
1266+
collectionContractAttribute.Arguments.Add(new CodeAttributeArgument(ImportGlobals.ItemNameProperty, new CodePrimitiveExpression(GetNameForAttribute(itemName))));
12541267
if (foundDictionaryBase)
12551268
{
12561269
// These are not null if we are working with a dictionary. See CollectionDataContract.IsDictionary
@@ -1440,7 +1453,9 @@ private static string GetClrIdentifier(string identifier, string defaultIdentifi
14401453

14411454
internal static string GetClrTypeFullName(Type type)
14421455
{
1443-
return !type.IsGenericTypeDefinition && type.ContainsGenericParameters ? type.Namespace + "." + type.Name : type.FullName!;
1456+
// Type.FullName can be null for types that contain unassigned generic parameters and for generic type parameters,
1457+
// so construct a fallback name only when FullName is unavailable.
1458+
return type.FullName ?? (type.Namespace == null ? type.Name : type.Namespace + "." + type.Name);
14441459
}
14451460

14461461
private static string AppendToValidClrIdentifier(string identifier, string appendString)

0 commit comments

Comments
 (0)