Refactor the module path construction code to make it more robust and easier to maintain (#26565)

daxian-dbw · web-flow · commit 592668bd0b0e · 2025-12-12T09:43:08.000-08:00
diff --git a/src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleHost.cs b/src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleHost.cs
@@ -2201,7 +2201,6 @@ private void ReportException(Exception e, Executor exec)
             if (e1 != null)
             {
                 // that didn't work.  Write out the error ourselves as a last resort.
-
                 ReportExceptionFallback(e, null);
             }
         }
@@ -2222,15 +2221,17 @@ private void ReportExceptionFallback(Exception e, string header)
                 Console.Error.WriteLine(header);
             }
 
-            if (e == null)
+            if (e is null)
             {
                 return;
             }
 
             // See if the exception has an error record attached to it...
             ErrorRecord er = null;
             if (e is IContainsErrorRecord icer)
+            {
                 er = icer.ErrorRecord;
+            }
 
             if (e is PSRemotingTransportException)
             {
@@ -2247,8 +2248,22 @@ private void ReportExceptionFallback(Exception e, string header)
             }
 
             // Add the position message for the error if it's available.
-            if (er != null && er.InvocationInfo != null)
+            if (er?.InvocationInfo is { })
+            {
                 Console.Error.WriteLine(er.InvocationInfo.PositionMessage);
+            }
+
+            // Print the stack trace.
+            Console.Error.WriteLine($"\n--- {e.GetType().FullName} ---");
+            Console.Error.WriteLine(e.StackTrace);
+
+            Exception inner = e.InnerException;
+            while (inner is { })
+            {
+                Console.Error.WriteLine($"--- inner {inner.GetType().FullName} ---");
+                Console.Error.WriteLine(inner.StackTrace);
+                inner = inner.InnerException;
+            }
         }
 
         /// <summary>
diff --git a/src/System.Management.Automation/engine/Modules/ModuleIntrinsics.cs b/src/System.Management.Automation/engine/Modules/ModuleIntrinsics.cs
@@ -10,9 +10,7 @@
 using System.Management.Automation.Language;
 using System.Text;
 using System.Threading;
-
 using Microsoft.PowerShell.Commands;
-using Microsoft.Win32;
 
 using Dbg = System.Management.Automation.Diagnostics;
 
@@ -39,18 +37,22 @@ public class ModuleIntrinsics
         private static readonly string s_windowsPowerShellPSHomeModulePath =
             Path.Combine(System.Environment.SystemDirectory, "WindowsPowerShell", "v1.0", "Modules");
 
+        static ModuleIntrinsics()
+        {
+            // Initialize the module path.
+            SetModulePath();
+        }
+
         internal ModuleIntrinsics(ExecutionContext context)
         {
             _context = context;
-
-            // And initialize the module path...
-            SetModulePath();
+            ModuleTable = new Dictionary<string, PSModuleInfo>(StringComparer.OrdinalIgnoreCase);
         }
 
         private readonly ExecutionContext _context;
 
         // Holds the module collection...
-        internal Dictionary<string, PSModuleInfo> ModuleTable { get; } = new Dictionary<string, PSModuleInfo>(StringComparer.OrdinalIgnoreCase);
+        internal Dictionary<string, PSModuleInfo> ModuleTable { get; }
 
         private const int MaxModuleNestingDepth = 10;
 
@@ -176,11 +178,9 @@ private PSModuleInfo CreateModuleImplementation(string name, string path, object
 
                         sb.SessionState = ss;
                     }
-                    else
+                    else if (moduleCode is string sbText)
                     {
-                        var sbText = moduleCode as string;
-                        if (sbText != null)
-                            sb = ScriptBlock.Create(_context, sbText);
+                        sb = ScriptBlock.Create(_context, sbText);
                     }
                 }
 
@@ -1082,91 +1082,80 @@ internal static string GetExpandedEnvironmentVariable(string name, EnvironmentVa
             return result;
         }
 
-        /// <summary>
-        /// Checks if a particular string (path) is a member of 'combined path' string (like %Path% or %PSModulePath%)
-        /// </summary>
-        /// <param name="pathToScan">'Combined path' string to analyze; can not be null.</param>
-        /// <param name="pathToLookFor">Path to search for; can not be another 'combined path' (semicolon-separated); can not be null.</param>
-        /// <returns>Index of pathToLookFor in pathToScan; -1 if not found.</returns>
-        private static int PathContainsSubstring(string pathToScan, string pathToLookFor)
-        {
-            // we don't support if any of the args are null - parent function should ensure this; empty values are ok
-            Diagnostics.Assert(pathToScan != null, "pathToScan should not be null according to contract of the function");
-            Diagnostics.Assert(pathToLookFor != null, "pathToLookFor should not be null according to contract of the function");
-
-            int pos = 0; // position of the current substring in pathToScan
-            string[] substrings = pathToScan.Split(Path.PathSeparator, StringSplitOptions.None); // we want to process empty entries
-            string goodPathToLookFor = pathToLookFor.Trim().TrimEnd(Path.DirectorySeparatorChar); // trailing backslashes and white-spaces will mess up equality comparison
-            foreach (string substring in substrings)
-            {
-                string goodSubstring = substring.Trim().TrimEnd(Path.DirectorySeparatorChar);  // trailing backslashes and white-spaces will mess up equality comparison
-
-                // We have to use equality comparison on individual substrings (as opposed to simple 'string.IndexOf' or 'string.Contains')
-                // because of cases like { pathToScan="C:\Temp\MyDir\MyModuleDir", pathToLookFor="C:\Temp" }
-
-                if (string.Equals(goodSubstring, goodPathToLookFor, StringComparison.OrdinalIgnoreCase))
-                {
-                    return pos; // match found - return index of it in the 'pathToScan' string
-                }
-                else
-                {
-                    pos += substring.Length + 1; // '1' is for trailing semicolon
-                }
-            }
-            // if we are here, that means a match was not found
-            return -1;
-        }
-
         /// <summary>
         /// Adds paths to a 'combined path' string (like %Path% or %PSModulePath%) if they are not already there.
         /// </summary>
         /// <param name="basePath">Path string (like %Path% or %PSModulePath%).</param>
-        /// <param name="pathToAdd">Collection of individual paths to add.</param>
+        /// <param name="pathToAdd">An individual path to add, or multiple paths separated by the path separator character.</param>
         /// <param name="insertPosition">-1 to append to the end; 0 to insert in the beginning of the string; etc...</param>
         /// <returns>Result string.</returns>
-        private static string AddToPath(string basePath, string pathToAdd, int insertPosition)
+        private static string UpdatePath(string basePath, string pathToAdd, ref int insertPosition)
         {
             // we don't support if any of the args are null - parent function should ensure this; empty values are ok
-            Diagnostics.Assert(basePath != null, "basePath should not be null according to contract of the function");
-            Diagnostics.Assert(pathToAdd != null, "pathToAdd should not be null according to contract of the function");
+            Dbg.Assert(basePath != null, "basePath should not be null according to contract of the function");
+            Dbg.Assert(pathToAdd != null, "pathToAdd should not be null according to contract of the function");
+
+            // The 'pathToAdd' could be a 'combined path' (path-separator-separated).
+            string[] newPaths = pathToAdd.Split(
+                Path.PathSeparator,
+                StringSplitOptions.RemoveEmptyEntries | StringSplitOptions.TrimEntries);
 
-            StringBuilder result = new StringBuilder(basePath);
+            if (newPaths.Length is 0)
+            {
+                // The 'pathToAdd' doesn't really contain any paths to add.
+                return basePath;
+            }
+
+            var result = new StringBuilder(basePath, capacity: basePath.Length + pathToAdd.Length + newPaths.Length);
+            var addedPaths = new HashSet<string>(StringComparer.OrdinalIgnoreCase);
+            string[] initialPaths = basePath.Split(
+                Path.PathSeparator,
+                StringSplitOptions.RemoveEmptyEntries | StringSplitOptions.TrimEntries);
+
+            foreach (string p in initialPaths)
+            {
+                // Remove the trailing directory separators.
+                // Trailing white spaces were already removed by 'StringSplitOptions.TrimEntries'.
+                addedPaths.Add(Path.TrimEndingDirectorySeparator(p));
+            }
 
-            if (!string.IsNullOrEmpty(pathToAdd)) // we don't want to append empty paths
+            foreach (string subPathToAdd in newPaths)
             {
-                foreach (string subPathToAdd in pathToAdd.Split(Path.PathSeparator, StringSplitOptions.RemoveEmptyEntries)) // in case pathToAdd is a 'combined path' (semicolon-separated)
+                // Remove the trailing directory separators.
+                // Trailing white spaces were already removed by 'StringSplitOptions.TrimEntries'.
+                string normalizedPath = Path.TrimEndingDirectorySeparator(subPathToAdd);
+                if (addedPaths.Contains(normalizedPath))
                 {
-                    int position = PathContainsSubstring(result.ToString(), subPathToAdd); // searching in effective 'result' value ensures that possible duplicates in pathsToAdd are handled correctly
-                    if (position == -1) // subPathToAdd not found - add it
-                    {
-                        if (insertPosition == -1 || insertPosition > basePath.Length) // append subPathToAdd to the end
-                        {
-                            bool endsWithPathSeparator = false;
-                            if (result.Length > 0)
-                            {
-                                endsWithPathSeparator = (result[result.Length - 1] == Path.PathSeparator);
-                            }
+                    // The normalized sub path was already added - skip it.
+                    continue;
+                }
 
-                            if (endsWithPathSeparator)
-                            {
-                                result.Append(subPathToAdd);
-                            }
-                            else
-                            {
-                                result.Append(Path.PathSeparator + subPathToAdd);
-                            }
-                        }
-                        else if (insertPosition > result.Length)
-                        {
-                            // handle case where path is a singleton with no path separator already
-                            result.Append(Path.PathSeparator).Append(subPathToAdd);
-                        }
-                        else // insert at the requested location (this is used by DSC (<Program Files> location) and by 'user-specific location' (SpecialFolder.MyDocuments or EVT.User))
-                        {
-                            result.Insert(insertPosition, subPathToAdd + Path.PathSeparator);
-                        }
+                // The normalized sub path was not found - add it.
+                if (insertPosition is -1 || insertPosition >= result.Length)
+                {
+                    // Append the normalized sub path to the end.
+                    if (result.Length > 0 && result[^1] != Path.PathSeparator)
+                    {
+                        result.Append(Path.PathSeparator);
                     }
+
+                    result.Append(normalizedPath);
+                    // Next insertion should happen at the end.
+                    insertPosition = result.Length;
+                }
+                else
+                {
+                    // Insert at the requested location.
+                    // This is used by the user-specific module path, the shared module path (<Program Files> location), and the PSHome module path.
+                    string strToInsert = normalizedPath + Path.PathSeparator;
+                    result.Insert(insertPosition, strToInsert);
+
+                    // Next insertion should happen after the just inserted string.
+                    insertPosition += strToInsert.Length;
                 }
+
+                // Add it to the set.
+                addedPaths.Add(normalizedPath);
             }
 
             return result.ToString();
@@ -1218,7 +1207,7 @@ public static string GetModulePath(string currentProcessModulePath, string hklmM
             string psHomeModulePath = GetPSHomeModulePath(); // $PSHome\Modules location
 
             // If the variable isn't set, then set it to the default value
-            if (currentProcessModulePath == null)  // EVT.Process does Not exist - really corner case
+            if (string.IsNullOrEmpty(currentProcessModulePath))  // EVT.Process does Not exist - really corner case
             {
                 // Handle the default case...
                 if (string.IsNullOrEmpty(hkcuUserModulePath)) // EVT.User does Not exist -> set to <SpecialFolder.MyDocuments> location
@@ -1270,21 +1259,6 @@ public static string GetModulePath(string currentProcessModulePath, string hklmM
             return currentProcessModulePath;
         }
 
-        private static string UpdatePath(string path, string pathToAdd, ref int insertIndex)
-        {
-            if (!string.IsNullOrEmpty(pathToAdd))
-            {
-                path = AddToPath(path, pathToAdd, insertIndex);
-                insertIndex = path.IndexOf(Path.PathSeparator, PathContainsSubstring(path, pathToAdd));
-                if (insertIndex != -1)
-                {
-                    // advance past the path separator
-                    insertIndex++;
-                }
-            }
-            return path;
-        }
-
         /// <summary>
         /// Checks if $env:PSModulePath is not set and sets it as appropriate. Note - because these
         /// strings go through the provider, we need to escape any wildcards before passing them
@@ -1358,11 +1332,16 @@ private static string SetModulePath()
         {
             string currentModulePath = GetExpandedEnvironmentVariable(Constants.PSModulePathEnvVar, EnvironmentVariableTarget.Process);
 #if !UNIX
-            // if the current process and user env vars are the same, it means we need to append the machine one as it's incomplete
-            // otherwise, the user modified it and we should use the process one
+            // if the current process and user env vars are the same, it means we need to append the machine one as it's incomplete.
+            // Otherwise, the user modified it and we should use the process one.
             if (string.CompareOrdinal(GetExpandedEnvironmentVariable(Constants.PSModulePathEnvVar, EnvironmentVariableTarget.User), currentModulePath) == 0)
             {
-                currentModulePath = currentModulePath + Path.PathSeparator + GetExpandedEnvironmentVariable(Constants.PSModulePathEnvVar, EnvironmentVariableTarget.Machine);
+                string machineScopeValue = GetExpandedEnvironmentVariable(Constants.PSModulePathEnvVar, EnvironmentVariableTarget.Machine);
+                currentModulePath = string.IsNullOrEmpty(currentModulePath)
+                    ? machineScopeValue
+                    : string.IsNullOrEmpty(machineScopeValue)
+                        ? currentModulePath
+                        : string.Concat(currentModulePath, Path.PathSeparator, machineScopeValue);
             }
 #endif
             string allUsersModulePath = PowerShellConfig.Instance.GetModulePath(ConfigScope.AllUsers);
diff --git a/test/powershell/Host/ConsoleHost.Tests.ps1 b/test/powershell/Host/ConsoleHost.Tests.ps1
@@ -386,6 +386,33 @@ export $envVarName='$guid'
         }
     }
 
+    Context "-SettingsFile Commandline switch set 'PSModulePath'" {
+
+        BeforeAll {
+            $CustomSettingsFile = Join-Path -Path $TestDrive -ChildPath 'powershell.test.json'
+            $mPath1 = Join-Path $PSHOME 'Modules'
+            $mPath2 = Join-Path $TestDrive 'NonExist'
+            $pathSep = [System.IO.Path]::PathSeparator
+
+            ## Use multiple paths in the setting.
+            $ModulePath = "${mPath1}${pathSep}${mPath2}".Replace('\', "\\")
+            Set-Content -Path $CustomSettingsfile -Value "{`"Microsoft.PowerShell:ExecutionPolicy`":`"Unrestricted`", `"PSModulePath`": `"$ModulePath`" }" -ErrorAction Stop
+        }
+
+        It "Verify PowerShell PSModulePath should contain paths from config file" {
+            $psModulePath = & $powershell -NoProfile -SettingsFile $CustomSettingsFile -Command '$env:PSModulePath'
+
+            ## $mPath1 already exists in the value of env PSModulePath, so it won't be added again.
+            $index = $psModulePath.IndexOf("${mPath1}${pathSep}", [System.StringComparison]::OrdinalIgnoreCase)
+            $index | Should -BeGreaterThan 0
+            $index += $mPath1.Length
+            $psModulePath.IndexOf($mPath1, $index, [System.StringComparison]::OrdinalIgnoreCase) | Should -BeExactly -1
+
+            ## $mPath2 should be added at the index position 0.
+            $psModulePath.StartsWith("${mPath2}${pathSep}", [System.StringComparison]::OrdinalIgnoreCase) | Should -BeTrue
+        }
+    }
+
     Context "Pipe to/from powershell" {
         BeforeAll {
             if ($null -ne $PSStyle) {

Original file line number	Diff line number	Diff line change
`@@ -2201,7 +2201,6 @@ private void ReportException(Exception e, Executor exec)`
`2201`	`2201`	`if (e1 != null)`
`2202`	`2202`	`{`
`2203`	`2203`	`// that didn't work. Write out the error ourselves as a last resort.`
`2204`		`-`
`2205`	`2204`	`ReportExceptionFallback(e, null);`
`2206`	`2205`	`}`
`2207`	`2206`	`}`
`@@ -2222,15 +2221,17 @@ private void ReportExceptionFallback(Exception e, string header)`
`2222`	`2221`	`Console.Error.WriteLine(header);`
`2223`	`2222`	`}`
`2224`	`2223`
`2225`		`- if (e == null)`
	`2224`	`+ if (e is null)`
`2226`	`2225`	`{`
`2227`	`2226`	`return;`
`2228`	`2227`	`}`
`2229`	`2228`
`2230`	`2229`	`// See if the exception has an error record attached to it...`
`2231`	`2230`	`ErrorRecord er = null;`
`2232`	`2231`	`if (e is IContainsErrorRecord icer)`
	`2232`	`+ {`
`2233`	`2233`	`er = icer.ErrorRecord;`
	`2234`	`+ }`
`2234`	`2235`
`2235`	`2236`	`if (e is PSRemotingTransportException)`
`2236`	`2237`	`{`
`@@ -2247,8 +2248,22 @@ private void ReportExceptionFallback(Exception e, string header)`
`2247`	`2248`	`}`
`2248`	`2249`
`2249`	`2250`	`// Add the position message for the error if it's available.`
`2250`		`- if (er != null && er.InvocationInfo != null)`
	`2251`	`+ if (er?.InvocationInfo is { })`
	`2252`	`+ {`
`2251`	`2253`	`Console.Error.WriteLine(er.InvocationInfo.PositionMessage);`
	`2254`	`+ }`
	`2255`	`+`
	`2256`	`+ // Print the stack trace.`
	`2257`	`+ Console.Error.WriteLine($"\n--- {e.GetType().FullName} ---");`
	`2258`	`+ Console.Error.WriteLine(e.StackTrace);`
	`2259`	`+`
	`2260`	`+ Exception inner = e.InnerException;`
	`2261`	`+ while (inner is { })`
	`2262`	`+ {`
	`2263`	`+ Console.Error.WriteLine($"--- inner {inner.GetType().FullName} ---");`
	`2264`	`+ Console.Error.WriteLine(inner.StackTrace);`
	`2265`	`+ inner = inner.InnerException;`
	`2266`	`+ }`
`2252`	`2267`	`}`
`2253`	`2268`
`2254`	`2269`	`/// <summary>`