runtime icon indicating copy to clipboard operation
runtime copied to clipboard

DCS alignment to 4.8 with schema support

Open StephenMolloy opened this issue 2 years ago • 1 comments

Edit: Less drafty now. Just about ready to go. API reviews for the new System.Runtime.Serialization.Schema stuff (#72243) and the new API's exposed from DCS in the runtime to allow that package to do it's thing (#72242) have been approved.


Very, very drafty. But I need to get this out so you guys can get eyes on it. Several places I had questions or clarifications or just noticed something off... all those are marked with comments that have "TODO smolloy" so I can easily find them - and you can too.

I am probably not packaging the Schema project or including CodeDom in the correct way, but I figure project niggles can get sorted out later.

One issue that I still need to look deeper into is how we create DataContract's when importing schema. In 4.8 we just created the appropriate DataContract type with a default constructor and went on our merry way. In 5+ we are nullable notated and we don't have default constructors anymore and we expect every DC to have a non-null underlying type... which is what using default constructors avoided in 4.8. Undoing the non-nullability of UnderlyingType and all the code surrounding it was untenable starting out. So I create contracts with a dummy 'SchemaDefinedType' type. I still need to look into what problems that might cause and how to fix them.

Also note that we don't have any tests. I have not tested any of this schema code. The full suite of serialization tests as it exists in Core passes on my machine, but there is obviously a gaping coverage hole there wrt the schema work.

StephenMolloy avatar Jul 07 '22 06:07 StephenMolloy

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

Nit, indentation


Refers to: src/libraries/System.Runtime.Serialization.Schema/src/System/Runtime/Serialization/Schema/CodeExporter.cs:305 in 7462bea. [](commit_id = 7462bea6f2e00ccb9405f02fccbe2ae7fe5ef8ef, deletion_comment = False)

mconnew avatar Aug 12 '22 17:08 mconnew

This code previously had a constraint on dataContract.GenericInfo != null. Now it's always running. Are you relying on the earlier condition in the previous if block? Is it possible for dataContract.GenericInfo to be null and it to still be a generic type definition?


Refers to: src/libraries/System.Runtime.Serialization.Schema/src/System/Runtime/Serialization/Schema/CodeExporter.cs:575 in 7462bea. [](commit_id = 7462bea6f2e00ccb9405f02fccbe2ae7fe5ef8ef, deletion_comment = False)

mconnew avatar Aug 12 '22 18:08 mconnew

I know it's equivalent, but I think the code would be easier to read if the comparison was >= 2 as there's an extra step when reading > 1 to parse that as "at least 2"


Refers to: src/libraries/System.Runtime.Serialization.Schema/src/System/Runtime/Serialization/Schema/CodeExporter.cs:664 in 7462bea. [](commit_id = 7462bea6f2e00ccb9405f02fccbe2ae7fe5ef8ef, deletion_comment = False)

mconnew avatar Aug 12 '22 18:08 mconnew

@mconnew

This code previously had a constraint on dataContract.GenericInfo != null. Now it's always running. Are you relying on the earlier condition in the previous if block? Is it possible for dataContract.GenericInfo to be null and it to still be a generic type definition?

Refers to: src/libraries/System.Runtime.Serialization.Schema/src/System/Runtime/Serialization/Schema/CodeExporter.cs:575 in 7462bea. [](commit_id = 7462bea, deletion_comment = False)

You somehow got three comments in a row here that I can't resolve or reply to. Hrrmmm. Anyway, consider the nits before and after this one resolved.

As for this one, this was part of the decoupling referred to in this feedback item. The logic has been twisted to keep GenericInfo internal, and it is now handled in the call to DataContractSet.GetReferencedType(). In the end it should be equivallent. But this is the schema side of that untangling that you asked about in DCS.

StephenMolloy avatar Aug 12 '22 20:08 StephenMolloy

Ack on the latest diff, all looks good to me.

mconnew avatar Aug 12 '22 23:08 mconnew