[dotnet-svcutil] Remove forked framework codedom and use System.CodeDom package#5889
[dotnet-svcutil] Remove forked framework codedom and use System.CodeDom package#5889imcarolwang wants to merge 7 commits intodotnet:mainfrom
Conversation
| implMethod.StartDirectives.Add(ifStart); | ||
| implMethod.EndDirectives.Add(ifEnd); | ||
| // System.CodeDom doesn't provide a CodeIfDirectiveStatement; we use a region directive as a stable marker | ||
| // and post-process the generated text to convert it into the desired conditional compilation directives. |
There was a problem hiding this comment.
I asked copilot about this because it seems like there has to be an easier way than emitting this, and then doing a string replacement in CodeSerializer, and it looks like there might be. I think you can do this in the VisitClientClass method:
type.Members.Add(new CodeSnippetTypeMember(
"#if !NET6_0_OR_GREATER\r\n"
));
type.Members.Add(GenerateTaskBasedAsyncMethod("Close", nameScope));
type.Members.Add(new CodeSnippetTypeMember(
"#endif\r\n"
));And obviously switching it to #If Not NET6_0_OR_GREATER Then\r\n and #End If\r\n for Visual Basic.
There was a problem hiding this comment.
Thank you for the suggestion. Using CodeSnippetTypeMember to emit #if/#endif around the generated CloseAsync method was considered. During validation, the CodeDOM provider did not always keep snippet members adjacent to the CloseAsync method, and the directives could be emitted at the start of the class instead of directly around the method.
To avoid that, the current change keeps the existing pattern: a unique #region marker is attached to the CloseAsync member via StartDirectives/EndDirectives, and during serialization only that marked region is rewritten into the corresponding conditional compilation directives (#if/#endif for C#, #If/#End If for VB). This keeps the rewrite small and focused on the intended member.
| // Microsoft.CodeDom | ||
| var mscodedomTypes = TypeLoader.LoadTypes(typeof(Microsoft.CodeDom.CodeObject).GetTypeInfo().Assembly, Verbosity.Silent); | ||
| var mscodedomTypes = TypeLoader.LoadTypes(typeof(System.CodeDom.CodeObject).GetTypeInfo().Assembly, Verbosity.Silent); | ||
|
|
There was a problem hiding this comment.
This code isn't right. You are reflecting over all the types in the System.CodeDom assembly, and if the type contains "Microsoft.CodeDom", add it to the _codeDomTypes dictionary. None of the CodeDom fixups in this visitor are needed any more. The visitor is still needed for fixing up Microsoft.Xml to System.Xml for now, but you can strip out the codedom pieces from this class as no fixup is needed.
There was a problem hiding this comment.
Great catch! Agreed. The Microsoft.CodeDom fixup in NamespaceFixup was incorrect and no longer needed, I'll update to remove it.
|
|
||
| using Microsoft.CodeDom; | ||
| using System.CodeDom; | ||
| using System.Runtime.Serialization; |
There was a problem hiding this comment.
Out of scope for this PR, but do we still need this? I presume this is an artifact from the early days when IExtensibleDataObject wasn't in .NET Core 1.0 or 1.1, but they've been there since 2.0. I'm not finding equivalent code in .NET Framework to remove these. Can you open an issue and as a follow up task, work out if this is needed and if not, remove it.
| using System.ComponentModel; | ||
| using Microsoft.CodeDom; | ||
| using System.CodeDom; | ||
| using System.Text; |
There was a problem hiding this comment.
We shipped a WCF package called System.Web.Services.Description a few years ago. It doesn't have everything in there that is in this fork, but what's there is standalone, so you should be able to remove a lot of the code in this folder. This might be better done as a follow up PR to avoid adding too many changes to this PR and it taking a long time to merge.
...l/lib/tests/Baselines/TFMBootstrap/tfmDefault/SvcutilBootstrapper/SvcutilBootstrapper.csproj
Show resolved
Hide resolved
.../lib/tests/Baselines/TFMBootstrap/tfmNetstd20/SvcutilBootstrapper/SvcutilBootstrapper.csproj
Show resolved
Hide resolved
…tem.CodeDom codegen diffs; update tests to net10.0
1. Pin System.CodeDom to 8.0.0 to avoid pulling newer (net10-rc aligned) builds that can fail at runtime on older supported runtimes. 2. Ensure the generated SvcutilBootstrapper project can always load System.CodeDom when the tool is referenced by path (global-tool/bootstrapper flow) by adding an explicit System.CodeDom.dll reference when present next to dotnet-svcutil-lib.dll, update TFMBootstrap/TFMBootstrapGlobal test baselines accordingly. 3. Remove the previously-added CodeDom output post-processing that was only needed for System.CodeDom 10.x.
1. Replace the SVCUTIL_CLOSEASYNC_WRAP comment marker with a CodeRegionDirective marker around the generated CloseAsync member. 2. Update CodeSerializer post-processing to convert the matching #region/#endregion (or VB #Region/#End Region) into #if/#endif (or #If/#End If), avoiding brace-based member-end parsing. 3. Use CodeRegionMode (netstandard2.0 compatible) for the region directive start/end.
…TNET_VERSION paths for baseline comparison
imcarolwang
force-pushed
the
removeForkedCodeDom
branch
from
b5bb5c0 to
e24575c
Compare
2 participants
No description provided.