Skip to content

Commit 0921c70

Browse files
gregg-miskellygregg-miskelly
authored
Enable nullable reference types in MICore (#1603)
- Add <Nullable>enable</Nullable> to MICore.csproj - Add GlobalUsings.cs with NullableHelpers static import - Add NullableAttributes.cs shared file - Add nullable annotations (?) to all appropriate type declarations - Add [NotNullWhen], [DoesNotReturn] attributes - Add Debug.Assert statements for non-null invariants - Replace string.IsNullOrEmpty/IsNullOrWhiteSpace with NullableHelpers versions - Add using System.Diagnostics where needed for Debug class - Remove redundant using aliases replaced by GlobalUsings This also cleans up the code in `PipeTransport.ExecuteSyncCommand` as that code had the classic Process.Start deadlock
1 parent dd91e13 commit 0921c70

33 files changed

Lines changed: 721 additions & 630 deletions

‎docs/CodingStandards-CSharp-for-AI.md‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,3 +40,17 @@ These aren't in `.editorconfig` but show up everywhere — follow the existing c
4040
- **Host calls go through `DebugEngineHost`.** MIDebugEngine never references `Microsoft.VisualStudio.*` directly; use `HostLogger`, `HostMarshal`, `HostOutputWindow`, etc.
4141
- **AD7 surface lives on `AD7*` partial classes.** Keep VS-SDK COM concerns out of the core `Debugged*` classes.
4242
- **Worker thread discipline.** AD7 callbacks must not block. Use `Task.Run` for work and post results back via the engine's `WorkerThread` / `EngineCallback`. Mirror existing call sites; do not invent new threading patterns.
43+
44+
## Nullable reference types and `Debug`
45+
46+
Projects with nullable reference types enabled **must never** use `System.Diagnostics.Debug` directly. Do **not** add `using System.Diagnostics;` or `using Debug = System.Diagnostics.Debug;` in any file in a nullable-enabled project.
47+
48+
Instead, use `Microsoft.DebugEngineHost.NullableHelpers.Debug`, which is a wrapper that adds `[DoesNotReturn]` / `[DoesNotReturnIf(false)]` attributes so the C# nullable flow analyser understands that `Debug.Assert`/`Debug.Fail` stop execution. This wrapper is brought into scope via the file `GlobalUsings.cs` in each nullable-enabled project:
49+
50+
```csharp
51+
global using static global::Microsoft.DebugEngineHost.NullableHelpers;
52+
```
53+
54+
With that global using in place every `Debug.Assert(...)` and `Debug.Fail(...)` call in the project automatically resolves to `NullableHelpers.Debug` — no per-file `using` is needed. **Never shadow this with a per-file alias or a `using System.Diagnostics;` import.**
55+
56+
The same global using also brings `IsNullOrEmpty` and `IsNullOrWhiteSpace` into scope as drop-in replacements for `string.IsNullOrEmpty`/`string.IsNullOrWhiteSpace` with the proper `[NotNullWhen(false)]` annotation.

‎src/MICore/Checksum.cs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,8 +22,8 @@ public enum MIHashAlgorithmName
2222

2323
public class Checksum
2424
{
25-
private string _checksumString = null;
26-
private byte[] _bytes = null;
25+
private string? _checksumString = null;
26+
private byte[] _bytes;
2727

2828
public readonly MIHashAlgorithmName MIHashAlgorithmName;
2929

‎src/MICore/CommandFactories/MICommandFactory.cs‎

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@
33

44
using System;
55
using System.Collections.Generic;
6-
using System.Diagnostics;
76
using System.Threading.Tasks;
87
using System.IO;
98
using System.Text;
@@ -81,7 +80,7 @@ public static MICommandFactory GetInstance(MIMode mode, Debugger debugger)
8180
return commandFactory;
8281
}
8382

84-
public static string SpanNextAddr(string line, out ulong addr)
83+
public static string? SpanNextAddr(string line, out ulong addr)
8584
{
8685
addr = 0;
8786
char[] endOfNum = { ' ', '\t', '\"' };
@@ -483,7 +482,7 @@ internal bool PreparePath(string path, bool useUnixFormat, out string pathMI)
483482
return requiresQuotes;
484483
}
485484

486-
public virtual async Task<Results> BreakInsert(string filename, bool useUnixFormat, uint line, string condition, bool enabled, IEnumerable<Checksum> checksums = null, ResultClass resultClass = ResultClass.done)
485+
public virtual async Task<Results> BreakInsert(string filename, bool useUnixFormat, uint line, string condition, bool enabled, IEnumerable<Checksum>? checksums = null, ResultClass resultClass = ResultClass.done)
487486
{
488487
StringBuilder cmd = await BuildBreakInsert(condition, enabled);
489488

@@ -533,7 +532,7 @@ public virtual Task<Results> BreakWatch(string address, uint size, ResultClass r
533532

534533
public virtual bool SupportsDataBreakpoints { get { return false; } }
535534

536-
public virtual async Task<TupleValue> BreakInfo(string bkptno)
535+
public virtual async Task<TupleValue?> BreakInfo(string bkptno)
537536
{
538537
Results bindResult = await _debugger.CmdAsync("-break-info " + bkptno, ResultClass.None);
539538
if (bindResult.ResultClass != ResultClass.done)
@@ -563,7 +562,7 @@ public virtual async Task BreakDelete(string bkptno, ResultClass resultClass = R
563562

564563
public virtual async Task BreakCondition(string bkptno, string expr)
565564
{
566-
if (string.IsNullOrWhiteSpace(expr))
565+
if (IsNullOrWhiteSpace(expr))
567566
{
568567
expr = string.Empty;
569568
}
@@ -632,7 +631,7 @@ public virtual void DecodeExceptionReceivedProperties(Results miExceptionResult,
632631

633632
#region Miscellaneous
634633

635-
public virtual Task<string[]> AutoComplete(string command, int threadId, uint frameLevel)
634+
public virtual Task<string[]?> AutoComplete(string command, int threadId, uint frameLevel)
636635
{
637636
throw new NotImplementedException();
638637
}
@@ -708,9 +707,9 @@ public virtual AsyncBreakSignal GetAsyncBreakSignal(Results results)
708707
return MICore.AsyncBreakSignal.None;
709708
}
710709

711-
public Results IsModuleLoad(string cmd)
710+
public Results? IsModuleLoad(string cmd)
712711
{
713-
Results results = null;
712+
Results? results = null;
714713
if (cmd.StartsWith("library-loaded,", StringComparison.Ordinal))
715714
{
716715
MIResults res = new MIResults(_debugger.Logger);

‎src/MICore/CommandFactories/gdb.cs‎

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@
33

44
using System;
55
using System.Collections.Generic;
6-
using System.Diagnostics;
76
using System.Threading.Tasks;
87
using System.IO;
98
using System.Text;
@@ -57,7 +56,7 @@ protected override async Task<Results> ThreadFrameCmdAsync(string command, strin
5756
{
5857
// first aquire an exclusive lock. This is used as we don't want to fight with other commands that also require the current
5958
// thread to be set to a particular value
60-
ExclusiveLockToken lockToken = await _debugger.CommandLock.AquireExclusive();
59+
ExclusiveLockToken? lockToken = await _debugger.CommandLock.AquireExclusive();
6160

6261
try
6362
{
@@ -103,7 +102,7 @@ protected override async Task<Results> ThreadCmdAsync(string command, string arg
103102
{
104103
// first aquire an exclusive lock. This is used as we don't want to fight with other commands that also require the current
105104
// thread to be set to a particular value
106-
ExclusiveLockToken lockToken = await _debugger.CommandLock.AquireExclusive();
105+
ExclusiveLockToken? lockToken = await _debugger.CommandLock.AquireExclusive();
107106

108107
try
109108
{
@@ -193,7 +192,7 @@ public override async Task<List<ulong>> StartAddressesForLine(string file, uint
193192
{
194193
while (true)
195194
{
196-
string resultLine = stringReader.ReadLine();
195+
string? resultLine = stringReader.ReadLine();
197196
if (resultLine == null)
198197
break;
199198

@@ -281,7 +280,7 @@ public override TargetArchitecture ParseTargetArchitectureResult(string result)
281280
{
282281
while (true)
283282
{
284-
string resultLine = stringReader.ReadLine();
283+
string? resultLine = stringReader.ReadLine();
285284
if (resultLine == null)
286285
break;
287286

@@ -327,7 +326,7 @@ public override async Task Catch(string name, bool onlyOnce = false, ResultClass
327326
await _debugger.ConsoleCmdAsync(command + name, allowWhileRunning: false);
328327
}
329328

330-
public override async Task<string[]> AutoComplete(string command, int threadId, uint frameLevel)
329+
public override async Task<string[]?> AutoComplete(string command, int threadId, uint frameLevel)
331330
{
332331
string cmd = "-complete";
333332
string args = $"\"{command}\"";

‎src/MICore/CommandFactories/lldb.cs‎

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@
33

44
using System;
55
using System.Collections.Generic;
6-
using System.Diagnostics;
76
using System.Threading.Tasks;
87
using System.IO;
98
using System.Text;
@@ -116,13 +115,13 @@ protected override async Task<Results> ThreadCmdAsync(string command, string arg
116115

117116
public override Task<List<ulong>> StartAddressesForLine(string file, uint line)
118117
{
119-
return Task.FromResult<List<ulong>>(null);
118+
return Task.FromResult(new List<ulong>());
120119
}
121120

122121
public override Task EnableTargetAsyncOption()
123122
{
124123
// lldb-mi doesn't support target-async mode, and doesn't seem to need to
125-
return Task.FromResult((object)null);
124+
return Task.CompletedTask;
126125
}
127126

128127
public override string GetTargetArchitectureCommand()
@@ -136,7 +135,7 @@ public override TargetArchitecture ParseTargetArchitectureResult(string result)
136135
{
137136
while (true)
138137
{
139-
string resultLine = stringReader.ReadLine();
138+
string? resultLine = stringReader.ReadLine();
140139
if (resultLine == null)
141140
break;
142141

@@ -216,7 +215,7 @@ private async Task<bool> RequiresOnKeywordForBreakInsert()
216215
{
217216
// Query for the version.
218217
string version = await Version();
219-
if (!string.IsNullOrWhiteSpace(version) && version.Trim().Equals(OldLLDBMIVersionString, StringComparison.Ordinal))
218+
if (!IsNullOrWhiteSpace(version) && version.Trim().Equals(OldLLDBMIVersionString, StringComparison.Ordinal))
220219
{
221220
_requiresOnKeywordForBreakInsert = true;
222221
}

‎src/MICore/CommandLock.cs‎

Lines changed: 11 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@
33

44
using System;
55
using System.Collections.Generic;
6-
using System.Diagnostics;
76
using System.Linq;
87
using System.Text;
98
using System.Threading;
@@ -18,7 +17,7 @@ sealed public class ExclusiveLockToken : IDisposable
1817
// NOTE: I tried to make this a value object, but value objects don't work quite as expected in async methods and calling 'Close'
1918
// wasn't updating the backing value object which was stored in the state machine class
2019
{
21-
private CommandLock _commandLock;
20+
private CommandLock? _commandLock;
2221
private int _value;
2322

2423
internal ExclusiveLockToken(CommandLock commandLock, int value)
@@ -39,7 +38,7 @@ public static bool IsNullOrClosed(ExclusiveLockToken token)
3938
return (token == null || token._value == 0);
4039
}
4140

42-
public override bool Equals(object obj)
41+
public override bool Equals(object? obj)
4342
{
4443
throw new NotImplementedException(); // this method should never be called
4544
}
@@ -66,6 +65,7 @@ public void ConvertToSharedLock()
6665
_value = 0;
6766
_commandLock = null;
6867

68+
Debug.Assert(commandLock is not null, "Should be impossible. A non-zero _value implies _commandLock is set.");
6969
commandLock.ConvertExclusiveLockToShared(value);
7070
}
7171

@@ -78,6 +78,7 @@ public void Close()
7878
_value = 0;
7979
_commandLock = null;
8080

81+
Debug.Assert(commandLock is not null, "Should be impossible. A non-zero _value implies _commandLock is set.");
8182
commandLock.ReleaseExclusive(value);
8283
}
8384
}
@@ -99,9 +100,9 @@ sealed public class CommandLock
99100

100101
private int _prevExclusiveToken;
101102
private int _pendingSharedLockRequests;
102-
private TaskCompletionSource<int> _waitingSharedLockSource;
103+
private TaskCompletionSource<int>? _waitingSharedLockSource;
103104
private readonly Queue<TaskCompletionSource<ExclusiveLockToken>> _waitingExclusiveLockRequests = new Queue<TaskCompletionSource<ExclusiveLockToken>>();
104-
private string _closeMessage;
105+
private string _closeMessage = string.Empty;
105106

106107
public CommandLock()
107108
{
@@ -184,7 +185,7 @@ public Task AquireShared()
184185
// Internal method called from the ExclusiveLockToken class as part of closing an exclusive lock
185186
internal void ReleaseExclusive(int tokenValue)
186187
{
187-
Action actionAfterReleaseLock = null;
188+
Action? actionAfterReleaseLock = null;
188189

189190
lock (this.LockObject)
190191
{
@@ -212,7 +213,7 @@ internal void ReleaseExclusive(int tokenValue)
212213
// Internal method called from the ExclusiveLockToken class to convert an exclusive lock into a shared lock
213214
internal void ConvertExclusiveLockToShared(int tokenValue)
214215
{
215-
Action actionAfterReleaseLock = null;
216+
Action? actionAfterReleaseLock = null;
216217

217218
lock (this.LockObject)
218219
{
@@ -240,7 +241,7 @@ internal void ConvertExclusiveLockToShared(int tokenValue)
240241

241242
public void ReleaseShared()
242243
{
243-
Action actionAfterReleaseLock = null;
244+
Action? actionAfterReleaseLock = null;
244245

245246
lock (this.LockObject)
246247
{
@@ -269,7 +270,7 @@ public void ReleaseShared()
269270
}
270271

271272
// NOTE: This method MUST be called with this.LockObject held
272-
private Action GetAfterReleaseLockAction()
273+
private Action? GetAfterReleaseLockAction()
273274
{
274275
Debug.Assert(_lockStatus == StatusFree, "Why is GetAfterReleaseLockAction called when the lock is not free?");
275276

@@ -298,7 +299,7 @@ private ExclusiveLockToken GetNextExclusiveLockToken()
298299
}
299300

300301
// NOTE: This method MUST be called with this.LockObject held
301-
private Action MaybeSignalPendingSharedLockRequests()
302+
private Action? MaybeSignalPendingSharedLockRequests()
302303
{
303304
Debug.Assert(_lockStatus >= 0, "Why is MaybeGetSharedLockAction called when the lock is not free/reading?");
304305

0 commit comments

Comments
 (0)