Annotate `System.Speech` for nullable reference types #119564

RenderMichael · 2025-09-11T05:35:59Z

Contributes to #90400

dotnet-policy-service · 2025-09-11T05:37:45Z

Tagging subscribers to this area: @dotnet/area-system-speech
See info in area-owners.md if you want to be subscribed.

xtqqczze · 2025-09-11T09:43:46Z

src/libraries/System.Speech/src/Internal/AlphabetConverter.cs

                public string sapi;
                public string ups;
                public bool isDefault;
+                public ConversionUnit(string sapi, string ups, bool isDefault)


This seems a good candidate for C# 12 primary constructor.

I tried to minimize diffs where possible because of System.Speech’s support policy, but I would very much like to modernize a few things here.

src/libraries/System.Speech/ref/System.Speech.cs

stephentoub · 2025-09-11T12:43:33Z

src/libraries/System.Speech/ref/System.Speech.cs

        public System.Speech.AudioFormat.EncodingFormat EncodingFormat { get { throw null; } }
        public int SamplesPerSecond { get { throw null; } }
-        public override bool Equals(object obj) { throw null; }
+        public override bool Equals(object? obj) { throw null; }


Typically the obj parameter would have a [NotNullWhen(true)] attribute on it. You might need to include the internal version of that attribute when building for !NET

Thanks, I went through all these. For some reason, dotnet msbuild /t:GenerateReferenceAssemblySource didn't add this attribute and kept stripping it, even though I had it in source. Not sure what I did wrong here - [System.Diagnostics.CodeAnalysis.DisallowNullAttribute] sticks just fine.

I'm not seeing the nullability attributes imported on !NET, I'll look around elsewhere to see how that's done.

src/libraries/System.Speech/ref/System.Speech.cs

stephentoub · 2025-09-11T12:48:46Z

src/libraries/System.Speech/ref/System.Speech.cs

-        public System.Speech.Synthesis.TtsEngine.ContourPoint[] GetContourPoints() { throw null; }
-        public void SetContourPoints(System.Speech.Synthesis.TtsEngine.ContourPoint[] points) { }
+        public System.Speech.Synthesis.TtsEngine.ContourPoint[]? GetContourPoints() { throw null; }
+        public void SetContourPoints([System.Diagnostics.CodeAnalysis.NotNullAttribute] System.Speech.Synthesis.TtsEngine.ContourPoint[]? points) { }


I do not understand this attribution/annotation. It's allowing points to be null but then guaranteeing that it wasn't null?

Much like ArgumentNullException.ThrowIfNull and friends, this method validates that the contour points array is non-null.

Like most nullable annotations, this is retrofitted to work with the code it's already working with. It has one callsite:

runtime/src/libraries/System.Speech/src/Internal/Synthesis/SSmlParser.cs

Lines 881 to 885 in 99ed0ee

case "contour":

CheckForDuplicates(ref sContour, reader);

prosody.SetContourPoints(ParseContour(sContour));

if (prosody.GetContourPoints() == null) { isInvalidAttribute = true; }

break;

Where ParseContour(string?) returns null if the XML string is improperly shaped.

runtime/src/libraries/System.Speech/src/Internal/Synthesis/SSmlParser.cs

Lines 1581 to 1589 in 99ed0ee

catch (InvalidOperationException)

{

return null;

}

if (points.Count < 1)

{

return null;

}

We shouldn't be marking the public method's parameter as nullable if it invariably throws an ArgumentNullException when the argument is null. The call site should instead get a suppression.

Looking at the next line makes me think we should set isInvalidAttribute to true instead of suppressing + throwing. prosody.GetContourPoints() can never return null because the method would have thrown the line below.

I'll change it to that, happy to switch to something else if that's preferred.

Please just suppress it.

I know it's counterintuitive, but we're not trying to make the implementation better as part of this. The library is in maintenance mode, and every change we make has the possibility to regress. We do not have good test coverage for this.

At the end of the day, the primary purpose of adding annotations is for consumers of the library, and we're changing the implementation with annotations as a way to help validate that public surface area.

Reverted and suppressed, with a small explanatory comment

src/libraries/System.Speech/src/AudioFormat/AudioFormatConverter.cs

src/libraries/System.Speech/src/Internal/AlphabetConverter.cs

src/libraries/System.Speech/src/Internal/GrammarBuilding/BuilderElements.cs

src/libraries/System.Speech/src/Internal/SapiInterop/SapiProxy.cs

stephentoub · 2025-09-11T21:00:07Z

src/libraries/System.Speech/src/Internal/SrgsParser/SrgsDocumentParser.cs

-            IElement child = null;
-            IRule parentRule = parent as IRule;
-            IItem parentItem = parent as IItem;
+            IElement child;


Why isn't this one nullable? There's a child = null! below on a non-error path.

Suppressing nullability on one of the parsing branches was the lesser of all evils.

child is passed as the value to some implementation of IElementFactory.AddElement(IRule rule, IElement value) and of IElementFactory.AddElement(IItem item, IElement value). If that happens to be SrgsElementFactory, then it gets added to either SrgsRule.Elements or SrgsItem.Elements, respectively. Both of these collections call instance methods on their items.

It seems like when the element type isn't a SrgsText, we somehow don't go down this path (or at least, nobody's complained about it to the .NET team afaik).

I opted to suppress the null assignment, let me know if I should do something different.

In other words, I think it's an honest reflection of a hole in null handling in System.Speech. The two methods which child is passed into cannot necessarily handle nulls, yet there is a path where child can be null.

A single suppression on the affected branch seems apt.

src/libraries/System.Speech/src/Internal/SrgsParser/SrgsDocumentParser.cs

src/libraries/System.Speech/src/Internal/Synthesis/AudioFormatConverter.cs

src/libraries/System.Speech/src/Internal/Synthesis/PcmConverter.cs

…roblem

src/libraries/System.Speech/src/Result/RecognitionResult.cs

src/libraries/System.Speech/src/Internal/Synthesis/VoiceSynthesis.cs

stephentoub · 2025-09-11T21:29:41Z

src/libraries/System.Speech/src/Recognition/RecognizerBase.cs

-                {
-                    formatInfo = new SpeechAudioFormatInfo(16000, AudioBitsPerSample.Sixteen, AudioChannel.Mono);
+                    // If for some reason the GetFormat fails, assume 16 Kb, 16 bits, Audio.
+                    return new SpeechAudioFormatInfo(16000, AudioBitsPerSample.Sixteen, AudioChannel.Mono);


I understand why you changed L2595 to be a return. But why duplicate this line? It can just be after the empty catch block and the else block above re-removed, no?

The nullability (or definite assignment, for that matter) didn't fall out for SpeechAudioFormatInfo formatInfo. AudioFormatConverter.ToSpeechAudioFormatInfo would be called after the catch block; I didn't want to mess with a method that uses the Marshal type but I guess nothing seems to throw COMException.

I don't see a way to structure this that doesn't suppress null warnings somehow. Maybe that's from a lack of imagination though.

Suppressing is preferred.

Ideally the only changes in the whole PR are added ?s, !s, and attributes, with the rare pattern matching thrown in when it's helpful for ifs, and possibly new ctors necessary to reduce field annotations. Pretty much everything else should be undone.

src/libraries/System.Speech/src/Resources/Strings.resx

src/libraries/System.Speech/src/SRID.cs

stephentoub

Thanks for working on this.

RenderMichael · 2025-09-12T05:01:42Z

src/libraries/System.Speech/ref/System.Speech.csproj

-
-    <!-- TODO https://github.com/dotnet/runtime/issues/90400: Annotate for nullable reference types -->
-    <Nullable>disable</Nullable>
    <NoWarn>$(NoWarn);nullable</NoWarn>


For some reason when I remove this line, System.Speech.cs blows up every ? saying NRT is disabled.

Am I missing something? I'm not familiar enough with .NET internals to know what went wrong.

I would have expected the Nullable option to inherit a value of enable from this condition:

runtime/src/libraries/Directory.Build.props

Line 49 in 5878338

<Nullable Condition="'$(Nullable)' == '' and '$(IsTestProject)' != 'true'">enable</Nullable>

You could try adding the following to src/libraries/System.Speech/ref/System.Speech.csproj:

<Target Name="TestMessage" AfterTargets="Build" > <Message Text="Nullable: $(Nullable)" Importance="high"/> </Target>

And then build with:

dotnet build --verbosity detailed

Strange, the output gave me:

1:6>Target "TestMessage" in project "D:\Code\RenderMichael\runtime\src\libraries\System.Speech\ref\System.Speech.csproj" (entry point): 1:6>Task "Message" Nullable: enable

Maybe I have to rebuild something or Visual Studio 2022 isn't happy somehow.

I'll remove the suppression and see what the CI says.

The issue I am running into is reflected in CI.

Maybe src/libraries/System.Speech/ref/System.Speech.cs needs an explicit #nullable enable?

I got it working with that and suppressing CS8777 over a [NotNull]. I can push those changes.

Suppressing CS8777: Parameter must have a non-null value when exiting doesn't sound quite right.

Not quite sure what to make of it then.

What's strange is that these issues still happen even when <Nullable>enable</Nullable> is unconditionally added directly to the ref .csproj. Maybe there's something unique about the build process for this maintenance-only legacy library?

I've looked again. If the [NotNull] is required then surpassing CS8777 in src/libraries/System.Speech/ref/System.Speech.cs seems reasonable.

However, since the lexicalForm argument should not be null, I would actually make it non-nullable and use the null-suppressing operator at call-sites.

Done. that's much nicer,

…y merged PR

xtqqczze · 2025-09-14T12:06:05Z

src/libraries/System.Speech/ref/System.Speech.csproj

@@ -1,7 +1,6 @@
-<Project Sdk="Microsoft.NET.Sdk">
+<Project Sdk="Microsoft.NET.Sdk">


.csproj file should not contain Unicode byte-order-mark.

@RenderMichael Out of interest, which editor inserted this change?

I think this happened after I edited the file in VS2026 Insiders, but I can't reproduce it again.

I would have expected Visual Studio to respect the .editorconfig file:

runtime/.editorconfig

Lines 15 to 17 in 2bcadad

# Specify UTF-8 without byte-order mark

[*.{csproj,locproj,nativeproj,proj,resx,vbproj}]

charset = utf-8

Maybe there's some race condition on a fresh install with the new IDE. Luckily, it's not happening anymore.

src/libraries/System.Speech/src/Internal/Synthesis/VoiceSynthesis.cs

…sis.cs

xtqqczze · 2025-09-19T11:06:49Z

src/libraries/System.Speech/src/Internal/Synthesis/VoiceSynthesis.cs

                foreach (Parameters parameters in _pendingSpeakQueue)
                {
-                    ParametersSpeak paramSpeak = parameters._parameter as ParametersSpeak;
+                    ParametersSpeak paramSpeak = (ParametersSpeak)parameters._parameter;


We now might have an InvalidCastException when we didn't before.

I think that’s a better exception than the NRE that would have been thrown on the next line if the types didn’t line up.

I think a better approach is guarding the logic on whether the object is of type ParametersSpeak, like elsewhere in the code. But this may be best left alone as it is outside the scope of annotation for null reference types.

If it weren't for the System.Speech's support policy and the size of this PR, there are a lot of opportunistic niceties I would add. Looking through the associated area label, this library isn't dead yet!

RenderMichael added 3 commits September 11, 2025 00:41

Annotate System.Speech for nullable reference types

bdfe504

Contributes to #dotnet#90400

Clean up some changes

175e331

Use string resource for exception messages instead of hardcoding

bccf214

github-actions bot added the area-System.Speech label Sep 11, 2025

dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Sep 11, 2025

xtqqczze reviewed Sep 11, 2025

View reviewed changes