Port System.Speech to .NET Core

[danmoseley]

Fix https://github.com/dotnet/wpf/issues/2935

This ports System.Speech to .NET Core. We are porting it to enable existing code that uses System.Speech on .NET Framework to continue to work on .NET Core, including within Powershell Core scripts. This is a Windows-only legacy tech that will not receive new investment or features.

The work is broken into commits.

1. Rather than start with the poorly commented product code, I built System.Speech.dll then decompiled it in such a way that it picked up all the XML intellisense comments. This lost the names of locals, but the names are reasonable and it seems a better result overall given we won't be updating the code. Added license headers.

2. The main commit that needs review is the commit adding the project and packaging. @safern helped me with this. @ericstj perhaps you could help look at this.

3. For resources I did not convert to our regular SR.cs generation scheme because the code indexes directly into the resources for certain SAPI error codes - I also could not find any way to trigger that codepath to satisfy myself that improving this would not break something - I actually think it's dead. So I left as is.

4. Fixed the simple use of reflection to get a registry key handle to just access the handle. This was the one bug that prevented the .NET Framework binary from just working as is. Also cleaned up to use SafeHandle.

5. Fixed a hang running tests in XUnit. This was because XUnit runs tests in its own synch context, which it uses to ensure any async void tests have completed their async operations before returning results. The Speech code creates an AsyncOperation around whatever sync context it is run in, calls OperationStarted() on that AO, but never calls OperationCompleted(). This means that the XUnit sync context will wait indefinitely. I experimented with adding a Dispose scheme to call OperationCompleted() but this caused other threading problems. Discussed with @stephentoub and simply removed the use of AsyncOperation as it is not adding any value here. Note - an alternative hacky approach, that would not have required this change, would have been to have each of the tests switch out the synch context during their calls.

6. Fixed all the warnings and whitespace.

7. Removed dead code using Gendarme.

8. Added some end to end tests. These get only about 50% coverage. However, much of this assembly is concerned with building, compiling, reading and writing grammars and such: I am guessing most folks just need the speech synthesis and possibly recognition. The coverage should be sufficient to verify that, given we do not plan to change any of this code. I did go through the old tests, but they are very verbose, use a custom test harness, have checked in wav files, etc so I didn't end up using them.

The code references CodeDOM in order to build certain compiled grammars. In .NET Core, compiling with CodeDOM isn't supported, so these code paths will just throw. I experimented with stripping them all out and removing the dependency, but it seemed to add churn for not much value again in code that we don't want to change.

Remaining work: add it to the Windows Compat pack, which I can either push to this PR (with guidance) or do in a follow up.

Edit:

Restarted work using the original sources, instead of decompiled code. Repeated all the above, plus

9. Removed CodeDOM usage.

10. Fix many other minor things as shown in the commit list including spelling, formatting, a couple of clear bugs, etc.

Successfully tested consuming it both as Speech nupkg and as part of the Windows Compatibility Package nupkg.

[Dotnet-GitSync-Bot]

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.

[Dotnet-GitSync-Bot]

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

[terrajobst]

I haven't really reviewed the code, I assume it's a straight port. My only question is about what TFMs this targets. I think we'd want to be consistent with the rest of the Windows Compat Pack, which means:

1. Target .NET Standard 2.0. Installing this package in .NET Framework should simply add a reference to the (in-box) framework assembly.

2. Someone should be able to target .NET Core, reference a .NET Framework binary that uses System.Speech and get it to work by using this package. IOW, this package needs to satisfy bind requests via the .NET Framework identity. To avoid the need for type forwarders we should just match the .NET Framework assembly name and token.

3. Mark the assembly as Windows-specific.

4. We should reference this package from Microsoft.Windows.Compatibility.

I don't know enough about our .pkgproj infrastructure to understand whether this is fulfilled by this PR or not.

[ericstj]

I put together a port of this to netstandard2.0 danmoseley#8.

[birbilis]

"The code references CodeDOM in order to build certain compiled grammars. In .NET Core, compiling with CodeDOM isn't supported, so these code paths will just throw" - not sure if it has interpreter too for the speech recognition grammars, if it doesn't this could be a problem, unless it could spit out source file via codedom and have the caller compile it on the fly via alternative way

[danmoseley]

Still need to review the use of AppDomain that Eric removed in https://github.com/danmosemsft/runtime/pull/8/files

[danmoseley]

@safern it looks like I'm getting Publishing build artifacts failed with an error: Not found PathtoPublish: /Users/runner/work/1/s/artifacts/log/ in the iOS leg - known? I don't see an issue.

[ericstj]

Rather than start with the poorly commented product code, I built System.Speech.dll then decompiled it in such a way that it picked up all the XML intellisense comments. This lost the names of locals, but the names are reasonable and it seems a better result overall given we won't be updating the code. Added license headers.

FWIW @carlossanlop is working on a tool that takes intellisense and applies it to source (for our doc efforts in 6.0). If there was any value in the .NETFramework source / comments that you'd want to merge this is an alternative.

[stephentoub]

then decompiled it

This is placing a lot of faith in 100% accuracy of the decompiler. I personally don't have such absolute faith in any decompiler I've used.

How extensive / close to 100% is the test suite and its code coverage?

[stephentoub]

Rather than start with the poorly commented product code ... that it picked up all the XML intellisense comments ... we won't be updating the code.

If we're not going to be changing the code, I'm not sure why inline XML comments matter, either. We should do the thing that has the greatest fidelity to the original code that's been in production for a long time.

[danmoseley]

@ericstj if I started over with the source that has no XML docs, would intellisense still have text, same as it does for all our other code without XML docs? (Or do you have access to @carlossanlop's tool, if it's ready)

[safern]

@danmosemsft -- @carlossanlop tool is under development but you might get lucky with the changes on his branch here: https://github.com/carlossanlop/DocsPortingTool/tree/TripleSlashRoslyn and be able to port the comments, I believe he got it working for some projects in dotnet/runtime.

@safern it looks like I'm getting Publishing build artifacts failed with an error: Not found PathtoPublish: /Users/runner/work/1/s/artifacts/log/ in the iOS leg - known? I don't see an issue.

I don't see the error anymore, do you have the build? Usually when this fails, is when there is a failure before building (i.e, downloading the checkout bundle, installing native dependencies) and it usually hides the real error in the pipeline timeline. The reason why this is the behavior, is because we want the step to upload logs to run always (even when previous steps failed) as that is specially when we need to upload the logs :).

[danmoseley]

@safern thanks for that info, I'll look if I see it again.

[danmoseley]

For this PR, I plan to start over using the original sources as suggested, but it shouldn't take that long - I should be able to do it this week.

[danmoseley]

OK, I have restarted using the original sources, rather than decompiled ones. As well as the changes from before,

• Did a lot of code formatting cleanup, deleting bad/broken XML comments, etc using various fixers, the code formatter tool, dead code finders, etc. It's not "perfect" but I don't plan to take this further. I tried to leave XML comments that are not useless.
• Removed all code that required support for compiling to IL. This removed one of the two uses of AppDomain.

Testing

• Unit tests are 43% line coverage. That covers the basics of synthesis and recognition; it doesn't cover all the various event handlers, the details of creating, manipulating, loading and saving grammars, etc.
• Consumed the package in a .NET 5.0 project and in a .NET Framework 4.7.2 project and verified it produces speech in both cases.
• Decompiled this version and my previous version (without pdb's, so match locals names) and broadly verified diffs were expected. Eg., debug-only code is now present and was not previously.

To do

• Test acquisition through Windows Compat Pack (Edit: done)

Future

• If/when @carlossanlop has a good tool for adding XML comments into code, I would be happy to run it. It does not seem particularly important since we do not expect to change this code in the future. It will be static unless we discover a porting error, or some bug that would meet the .NET Framework bar, and in those sources the XML comments are no better.

Please re-review (mainly packaging/build)

[danmoseley]

Addressed feedback, rebased. Retested NS2.0 class library, netcoreapp2.0 (build failure), netcoreapp5.0, net472, with compat pack and with direct package reference.

[ghost]

Hello @danmosemsft!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

[danmoseley]

Incidentally /t:GenerateReferenceAssemblySource breaks the ref: it is missing some overloads. It also removes the _dummy entries. So I didn't use that.

[danmoseley]

/backport to release/5.0

[github-actions]

Started backporting to release/5.0: https://github.com/dotnet/runtime/actions/runs/470649374

[github-actions]

@danmosemsft backporting to release/5.0 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: Initial sources, with banners
Applying: Run the code formatter
Applying: Fix hang in XUnit due to failing to complete all AsyncOperation-s
Applying: Remove reflection over RegistryKey
Applying: Add ref and packaging
Applying: Add tests
Applying: Add sln
Applying: Fix CS1584
Applying: Fix CA1823
Applying: Fix CA1834
Applying: Unnecessary suppressions
Applying: Fix SA1028
Applying: Fix CA1507
Applying: Fix CA1810
Applying: Fix CA1825
Applying: Fix CA1825
Applying: Unnecessary suppressions
Applying: Fix CA1805
Applying: Fix IDE0004
Applying: Fix IDE0090
Applying: Remove CAS
Applying: Remove tabs and dead code
Applying: Unnecessary suppressions
Applying: Fix SA1212
Applying: Fix SA1121
Applying: Disable SA1129
Applying: Fix SA1206
Applying: Fix SA1518
Applying: Fix SA1617
Applying: Fix SA1001
Applying: Fix CS0618
Applying: Remove unnecessary comments
Applying: Remove unnecessary whitespace
Applying: Remove low value xml doc comments
Applying: Unused usings
Applying: dead files
Applying: Remove CAS
Applying: More junk
Applying: Fix obvious original bug
Applying: Remove/insert newlines
Applying: Remove reference to old design document
Applying: Fix spacing
Applying: Fix typo name
Applying: Fix file casing
Applying: Remove dead code
Applying: Add to compat pack
Applying: Remove AppDomain etc
Applying: Fix casing of .NET
Applying: Remove low value XML docs
Applying: Remove code that relies on compiling assemblies
Applying: Fix inadvertently removed padding
Applying: Use EDI to preserve stack when rethrowing
Applying: Fix misaligned resource ID's to match sperror.h
Applying: Skip SpeechRecognitionEngine tests if no installed recognizers
Applying: Fix misformatted string bug
Applying: Logging for CI error
Applying: Fix NRE trying to map phonemes for voice for culture for which we do not have phoneme map
Applying: Fix 153 spelling errors in comments using `Visual Studio Spell Checker`
Applying: Remove extraneous file
Applying: Fix spacing
Applying: Fix project reference
Applying: Reorder properties in csproj
Applying: Change from netcoreapp2.0 to netcoreapp2.1
Applying: Update src/libraries/System.Speech/pkg/System.Speech.pkgproj
Applying: Add build error for targeting netcoreapp2.0
Applying: Suppress new error during packaging testing
error: sha1 information is lacking or useless (src/libraries/pkg/test/project.csproj.template).
error: could not build fake ancestor
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0066 Suppress new error during packaging testing
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!