Fix incorrect name check when autoloading required modules

[rjmholt]

PR Summary

Fixes #8204.

The change I made in #7125 made the check too strict on the module we try to autoload it from requires and rejected it based on name.

This PR removes that name check but also sets up a fix for the more general problem that a ModuleSpecification can always specify a path rather than a name. I added some reusable logic to make this easier to service.

The main contribution is regression testing for RequiredModules, #requires -Modules and other cmdlets that can use ModuleSpecifications.

However, the general pervasive problem is that ModuleSpecifications contain raw user input and we don't do very much about handling paths that might be given as module names, namely:

• Normalising directory separators
• Resolving relative paths (which should be resolved differently depending on context)
• Dealing with case-sensitivity
• Handling trailing slashes
• Matching on module directories accounting for the versioned directory structure
• Working with special directory names like . and ..

The methods I've added deal with these and I've applied them in one place to help correct the immediate problem and make it more efficient.

But the fact remains that we have no way in the type system to identify when this resolution has already occurred; there are over 120 uses of the ModuleSpecification type in the code base, and we don't have a way to differentiate when they contain raw user input and when they can be trusted to have a canonical path. The way the module cmdlets are written currently, we redo a fair amount of this validation and normalisation logic, and it's probably somewhere we could find serious perf improvements.

PR Checklist

• PR has a meaningful title
  • Use the present tense and imperative mood when describing your changes
• Summarized changes
• Change is not breaking
• Make sure all .h, .cpp, .cs, .ps1 and .psm1 files have the correct copyright header
• This PR is ready to merge and is not Work in Progress.
  • If the PR is work in progress, please add the prefix WIP: to the beginning of the title and remove the prefix when the PR is ready.
• User-facing changes
  • Not Applicable
  • OR
  • User-facing Documentation needed
    • Issue filed - Issue link:
• Testing - New and feature
  • Not Applicable or can only be tested interactively
  • OR
  • Make sure you've added a new test if existing tests do not effectively test the code changed
    • Add [feature] if the change is significant or affects feature tests

[bergmeister]

Thanks for handling the issue. I'd consider a regression test for this important feature to be essential.

[rjmholt]

Agreed that it's essential -- I'm pretty surprised we didn't have one already.

I'm halfway through writing some already.

[bergmeister]

Ok, feel free to add the test in a 2nd PR as I can understand that this is a time-pressing issue that can initially be verified with some manual testing (please include Windows as test platform though)

[rjmholt]

We also seem to do required module autoloading in #requires pragmas:

PowerShell/src/System.Management.Automation/engine/CommandDiscovery.cs

Lines 307 to 313 in 4118fd2

	ModuleCmdletBase.LoadRequiredModule(
	context: context,
	currentModule: null,
	requiredModuleSpecification: requiredModule,
	moduleManifestPath: null,
	manifestProcessingFlags: ModuleCmdletBase.ManifestProcessingFlags.LoadElements | ModuleCmdletBase.ManifestProcessingFlags.WriteErrors,
	error: out error);

If those are supposed to allow paths, I'll need to do the same path normalisation logic there.

[adityapatwardhan]

@rjmholt 1 new test is failing on Linux and macOS. Please have a look.

[rjmholt]

The failing AppVeyor test is passing on my machine. Looking into it.

[daxian-dbw]

What's the concern about putting this fix in place? Regression? Perf degradation?

[rjmholt]

Just keeping the fix constrained in this PR. It shouldn't cause regressions or perf degradations, but to ensure there are no regressions (since this PR is a regression fix itself) I just left it as a comment.

[daxian-dbw]

Will you open issue to track all those todo comments? They can be easily forgotten :)

[daxian-dbw]

Did you open an issue to track those todo's?

[rjmholt]

Next on my list

[daxian-dbw]

Did you open an issue to track those todo's?

[daxian-dbw]

Only pending action item left is to open an issue tracking all "TODO" comments.

[adityapatwardhan]

Since the CodeFactor issues are for existing methods, re-ordering will cause larger diffs. We can ignore CodeFactor issues for this PR.