Self-review fixes

Author: gregg-miskelly

src/DebugEngineHost.Common/HostLogChannel.cs

@@ -50,15 +50,15 @@ public interface ILogChannel
 
     public class HostLogChannel : ILogChannel
     {
-        private readonly Action<string> _log;
+        private readonly Action<string>? _log;
         private StreamWriter? _logFile;
         private LogLevel _minLevelToBeLogged;
 
         private readonly object _lock = new object();
 
-        private HostLogChannel() { _log = null!; }
+        private HostLogChannel() { }
 
-        public HostLogChannel(Action<string> logAction, string? file, LogLevel logLevel)
+        public HostLogChannel(Action<string>? logAction, string? file, LogLevel logLevel)
         {
             _log = logAction;
 

src/DebugEngineHost.Stub/DebugEngineHost.ref.cs

@@ -259,7 +259,7 @@ public static class HostLogger
         /// </summary>
         /// <param name="callback">The callback to use to send the engine log.</param>
         /// <param name="level">The level of the log to filter the channel on.</param>
-        public static void EnableHostLogging(Action<string> callback, LogLevel level = LogLevel.Verbose)
+        public static void EnableHostLogging(Action<string>? callback, LogLevel level = LogLevel.Verbose)
         {
             throw new NotImplementedException();
         }
@@ -527,7 +527,7 @@ public static bool IsRunInTerminalAvailable()
         /// <returns>true if the message is sent, false if not.</returns>
         [System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Naming", "CA1704:IdentifiersShouldBeSpelledCorrectly", MessageId = "cwd")]
         [System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Design", "CA1006:DoNotNestGenericTypesInMemberSignatures")]
-        public static void RunInTerminal(string title, string workingDirectory, bool useExternalConsole, IReadOnlyList<string> commandArgs, IReadOnlyDictionary<string, string> environmentVariables, Action<int?> success, Action<string> failure)
+        public static void RunInTerminal(string title, string workingDirectory, bool useExternalConsole, IReadOnlyList<string> commandArgs, IReadOnlyDictionary<string, string?> environmentVariables, Action<int?> success, Action<string> failure)
         {
             throw new NotImplementedException();
         }

src/DebugEngineHost.VSCode/HostLogger.cs

@@ -21,7 +21,7 @@ public static void EnableNatvisDiagnostics(Action<string> callback, LogLevel lev
             }
         }
 
-        public static void EnableHostLogging(Action<string> callback, LogLevel level = LogLevel.Verbose)
+        public static void EnableHostLogging(Action<string>? callback, LogLevel level = LogLevel.Verbose)
         {
             if (s_engineLogChannel is null)
             {

src/DebugEngineHost.VSCode/HostRunInTerminal.cs

@@ -9,7 +9,7 @@ namespace Microsoft.DebugEngineHost
 {
     public static class HostRunInTerminal
     {
-        private static Action<string, string, bool, List<string>, Dictionary<string, object>, Action<int?>, Action<string>>? s_runInTerminalCallback;
+        private static Action<string, string, bool, List<string>, Dictionary<string, object?>, Action<int?>, Action<string>>? s_runInTerminalCallback;
 
         /// <summary>
         /// Checks to see if RunInTerminal is available
@@ -23,11 +23,11 @@ public static bool IsRunInTerminalAvailable()
         /// <summary>
         /// Passes the call to the UI to attempt to RunInTerminal if possible.
         /// </summary>
-        public static void RunInTerminal(string title, string cwd, bool useExternalConsole, IReadOnlyList<string> commandArgs, IReadOnlyDictionary<string, string> environmentVars, Action<int?> success, Action<string> failure)
+        public static void RunInTerminal(string title, string cwd, bool useExternalConsole, IReadOnlyList<string> commandArgs, IReadOnlyDictionary<string, string?> environmentVars, Action<int?> success, Action<string> failure)
         {
             if (s_runInTerminalCallback is not null)
             {
-                Dictionary<string, object> env = new Dictionary<string, object>();
+                Dictionary<string, object?> env = new Dictionary<string, object?>();
                 foreach (var item in environmentVars)
                 {
                     env.Add(item.Key, item.Value);
@@ -41,7 +41,7 @@ public static void RunInTerminal(string title, string cwd, bool useExternalConso
         /// Registers callback to call when RunInTerminal is called
         /// </summary>
         /// <param name="runInTerminalCallback">Callback for RunInTerminal</param>
-        public static void RegisterRunInTerminalCallback(Action<string, string, bool, List<string>, Dictionary<string, object>, Action<int?>, Action<string>> runInTerminalCallback)
+        public static void RegisterRunInTerminalCallback(Action<string, string, bool, List<string>, Dictionary<string, object?>, Action<int?>, Action<string>> runInTerminalCallback)
         {
             Debug.Assert(runInTerminalCallback != null, "Callback should not be null.");
             s_runInTerminalCallback = runInTerminalCallback;

src/DebugEngineHost/HostLogger.cs

@@ -19,7 +19,7 @@ public static class HostLogger
         private static FeedbackLogBuffer? s_circularBuffer;
         private static VSFeedbackLogger? s_feedbackLogger;
 
-        public static void EnableHostLogging(Action<string> callback, LogLevel level = LogLevel.Verbose)
+        public static void EnableHostLogging(Action<string>? callback, LogLevel level = LogLevel.Verbose)
         {
             if (s_engineLogChannel is null)
             {

src/DebugEngineHost/HostRunInTerminal.cs

@@ -22,7 +22,7 @@ public static bool IsRunInTerminalAvailable()
 
         [System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Naming", "CA1704:IdentifiersShouldBeSpelledCorrectly", MessageId = "cwd")]
         [System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Design", "CA1006:DoNotNestGenericTypesInMemberSignatures")]
-        public static void RunInTerminal(string title, string workingDirectory, bool useExternalConsole, IReadOnlyList<string> commandArgs, IReadOnlyDictionary<string, string> environmentVariables, Action<int?> success, Action<string> failure)
+        public static void RunInTerminal(string title, string workingDirectory, bool useExternalConsole, IReadOnlyList<string> commandArgs, IReadOnlyDictionary<string, string?> environmentVariables, Action<int?> success, Action<string> failure)
         {
             throw new NotImplementedException();
         }

src/MICore/JsonLaunchOptions.cs

@@ -4,7 +4,6 @@
 using System;
 using System.Collections.Generic;
 using System.Globalization;
-using System.Runtime.CompilerServices;
 using System.Runtime.Serialization;
 using Newtonsoft.Json;
 using Newtonsoft.Json.Converters;

src/MICore/LaunchOptions.cs

@@ -300,14 +300,14 @@ public EnvironmentEntry(Xml.LaunchOptions.EnvironmentEntry xmlEntry)
 
         public EnvironmentEntry(Json.LaunchOptions.Environment jsonEntry)
         {
-            this.Name = LaunchOptions.RequireAttribute(jsonEntry.Name, nameof(jsonEntry.Name));
-            this.Value = jsonEntry.Value ?? string.Empty;
+            this.Name = LaunchOptions.RequireAttribute(jsonEntry.Name, "environment.name");
+            this.Value = jsonEntry.Value;
         }
 
         public EnvironmentEntry(string name, string? value)
         {
             this.Name = name;
-            this.Value = value ?? string.Empty;
+            this.Value = value;
         }
 
         /// <summary>
@@ -316,9 +316,9 @@ public EnvironmentEntry(string name, string? value)
         public string Name { get; private set; }
 
         /// <summary>
-        /// [Required] Value of the environment variable
+        /// Value of the environment variable, or null to delete it
         /// </summary>
-        public string Value { get; private set; }
+        public string? Value { get; private set; }
     }
 
     public sealed class SourceMapEntry
@@ -380,9 +380,14 @@ public static ReadOnlyCollection<SourceMapEntry> CreateCollection(Xml.LaunchOpti
 
         public static ReadOnlyCollection<SourceMapEntry> CreateCollection(Dictionary<string, object>? source)
         {
-            var sourceMaps = new List<SourceMapEntry>(source?.Keys.Count ?? 0);
+            int count = source?.Keys.Count ?? 0;
+            if (count == 0)
+            {
+                return new ReadOnlyCollection<SourceMapEntry>(Array.Empty<SourceMapEntry>());
+            }
 
-            foreach (var item in source ?? new Dictionary<string, object>())
+            var sourceMaps = new List<SourceMapEntry>();
+            foreach (var item in source!)
             {
                 string compileTimePath = item.Key;
                 string? editorPath = null;
@@ -880,7 +885,7 @@ protected set
         private string? _exePath;
 
         /// <summary>
-        /// [Required] Path to the executable file. This could be a path on the remote machine (for Pipe transport)
+        /// Path to the executable file. This could be a path on the remote machine (for Pipe transport)
         /// or the local machine (Local transport).
         /// </summary>
         public virtual string? ExePath
@@ -1383,19 +1388,25 @@ public static LaunchOptions GetInstance(HostConfigurationStore? configStore, str
 
                     // if the customLauncher element is present then try using the custom launcher implementation from the config store
                     JToken? customLauncherToken = parsedOptions["customLauncher"];
-                    if (customLauncherToken is not null && !IsNullOrWhiteSpace(customLauncherToken.Value<string>()))
+                    if (customLauncherToken is not null && customLauncherToken.Value<string>() is string customLauncherName && !IsNullOrWhiteSpace(customLauncherName))
                     {
-                        string customLauncherName = customLauncherToken.Value<string>() ?? string.Empty;
-                        var jsonLauncher = configStore?.GetCustomLauncher(customLauncherName);
+                        if (configStore is null || eventCallback is null)
+                        {
+                            throw new InvalidLaunchOptionsException(MICoreResources.Error_UnknownLaunchOptions);
+                        }
+
+                        var jsonLauncher = configStore.GetCustomLauncher(customLauncherName);
                         if (jsonLauncher == null)
                         {
                             throw new ArgumentException(string.Format(CultureInfo.CurrentCulture, MICoreResources.Error_UnknownCustomLauncher, customLauncherName));
                         }
-                        if (jsonLauncher as IPlatformAppLauncher == null)
+
+                        if (jsonLauncher is not IPlatformAppLauncher platformAppLauncher)
                         {
                             throw new ArgumentException(string.Format(CultureInfo.CurrentCulture, MICoreResources.Error_LauncherNotFound, customLauncherName));
                         }
-                        launchOptions = ExecuteLauncher(configStore, (IPlatformAppLauncher)jsonLauncher, exePath, args, dir, parsedOptions, eventCallback, targetEngine, logger);
+
+                        launchOptions = ExecuteLauncher(configStore, platformAppLauncher, exePath, args, dir, parsedOptions, eventCallback, targetEngine, logger);
                     }
                     else if (parsedOptions["pipeTransport"] is JToken pipeTransportToken && pipeTransportToken.HasValues)
                     {
@@ -1508,7 +1519,7 @@ public static LaunchOptions GetInstance(HostConfigurationStore? configStore, str
 
             if (clsidLauncher != Guid.Empty)
             {
-                if (launcherXmlOptions is null)
+                if (launcherXmlOptions is null || configStore is null)
                 {
                     throw new InvalidLaunchOptionsException(MICoreResources.Error_UnknownLaunchOptions);
                 }
@@ -1517,7 +1528,7 @@ public static LaunchOptions GetInstance(HostConfigurationStore? configStore, str
             }
             else if (launcher != null)
             {
-                if (launcherXmlOptions is null)
+                if (launcherXmlOptions is null || configStore is null || eventCallback is null)
                 {
                     throw new InvalidLaunchOptionsException(MICoreResources.Error_UnknownLaunchOptions);
                 }
@@ -1560,8 +1571,8 @@ public static LaunchOptions CreateForAttachRequest(Microsoft.VisualStudio.Debugg
                                                             Logger logger)
         {
             var suppOptions = GetOptionsFromFile(logger);
-            string connection;
-            ((IDebugPort2)unixPort).GetPortName(out connection!);
+            string? connection;
+            ((IDebugPort2)unixPort).GetPortName(out connection);
             AttachOptionsForConnection? attachOptions = null;
             if (suppOptions != null && suppOptions.AttachOptions != null)
             {
@@ -2175,13 +2186,8 @@ public static int RequirePortAttribute(int attributeValue, string attributeName)
             return attributeValue;
         }
 
-        private static LaunchOptions ExecuteLauncher(HostConfigurationStore? configStore, Guid clsidLauncher, string exePath, string? args, string? dir, object launcherXmlOptions, IDeviceAppLauncherEventCallback? eventCallback, TargetEngine targetEngine, Logger? logger)
+        private static LaunchOptions ExecuteLauncher(HostConfigurationStore configStore, Guid clsidLauncher, string exePath, string? args, string? dir, object launcherXmlOptions, IDeviceAppLauncherEventCallback? eventCallback, TargetEngine targetEngine, Logger? logger)
         {
-            if (configStore is null)
-            {
-                throw new ArgumentNullException(nameof(configStore));
-            }
-
             var deviceAppLauncher = HostLoader.VsCoCreateManagedObject(configStore, clsidLauncher) as IPlatformAppLauncher;
             if (deviceAppLauncher == null)
             {
@@ -2190,15 +2196,15 @@ private static LaunchOptions ExecuteLauncher(HostConfigurationStore? configStore
             return ExecuteLauncher(configStore, deviceAppLauncher, exePath, args, dir, launcherXmlOptions, eventCallback, targetEngine, logger);
         }
 
-        private static LaunchOptions ExecuteLauncher(HostConfigurationStore? configStore, IPlatformAppLauncher deviceAppLauncher, string exePath, string? args, string? dir, object launcherOptions, IDeviceAppLauncherEventCallback? eventCallback, TargetEngine targetEngine, Logger? logger)
+        private static LaunchOptions ExecuteLauncher(HostConfigurationStore configStore, IPlatformAppLauncher deviceAppLauncher, string exePath, string? args, string? dir, object launcherOptions, IDeviceAppLauncherEventCallback eventCallback, TargetEngine targetEngine, Logger? logger)
         {
             bool success = false;
 
             try
             {
                 try
                 {
-                    deviceAppLauncher.Initialize(configStore ?? throw new ArgumentNullException(nameof(configStore)), eventCallback ?? throw new ArgumentNullException(nameof(eventCallback)));
+                    deviceAppLauncher.Initialize(configStore, eventCallback);
                     deviceAppLauncher.SetLaunchOptions(exePath, args ?? string.Empty, dir ?? string.Empty, launcherOptions, targetEngine);
                 }
                 catch (Exception e) when (!(e is InvalidLaunchOptionsException) && ExceptionHelper.BeforeCatch(e, logger, reportOnlyCorrupting: true))

src/MICore/Logger.cs

@@ -84,10 +84,7 @@ public static void LoadMIDebugLogger()
             {   // command configured log file
                 HostLogger.Reset();
                 HostLogger.SetEngineLogFile(CmdLogInfo.logFile);
-                if (CmdLogInfo.logToOutput != null)
-                {
-                    HostLogger.EnableHostLogging(CmdLogInfo.logToOutput);
-                }
+                HostLogger.EnableHostLogging(CmdLogInfo.logToOutput);
             }
 
             s_isEnabled = true;

src/MICore/PlatformUtilities.cs

@@ -73,7 +73,7 @@ public static bool IsLinux()
         }
 
         // Abstract API call to add an environment variable to a new process
-        public static void SetEnvironmentVariable(this ProcessStartInfo processStartInfo, string key, string value)
+        public static void SetEnvironmentVariable(this ProcessStartInfo processStartInfo, string key, string? value)
         {
             processStartInfo.Environment[key] = value;
         }