Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Modules containing colliding names causes side-effects that are visible in other modules #12036

Closed
alx9r opened this issue Mar 5, 2020 · 7 comments
Labels
Issue-Question ideally support can be provided via other mechanisms, but sometimes folks do open an issue to get a Resolution-By Design The reported behavior is by design. WG-Engine core PowerShell engine, interpreter, and runtime

Comments

@alx9r
Copy link

alx9r commented Mar 5, 2020

The way that PowerShell resolves commands with colliding names seems to have changed sometime between PowerShell 5.1 and 7. The new behavior is problematic because it introduces the possibility of the following happening:

Invoking a command in one module can have the side effect that changes the command to which a different name in an unrelated module resolves.

In other words, (at least by default in PowerShell 7, it seems) one can never be sure that code invoked in a different module will not inadvertently change the command to which a name in your module will resolve.

This seems like something I need to take into careful account when designing modules. Accordingly, I'm hoping to find answers to the following questions:

  1. Is this the design intent of PowerShell? If so, how should modules be designed to prevent other modules from inadvertently changing the commands to which names resolve?
  2. Is there some way to override this behavior so that a module imported in one module doesn't appear in another?

Other notes:

Steps to reproduce

Create the following well-defined module q in $Env:PSModulePath:

# q.psd1
@{
    ModuleVersion     = '0.1.0'
    RootModule        = 'q.psm1'
    FunctionsToExport = 'Get-FileHash','q1'
}
# q.psm1
function Get-FileHash {}
function q1 {}

Invoke the following:

New-Module m {
    function m1 {
        [pscustomobject]@{
            Call        = $MyInvocation.MyCommand.Name
            CommandType = Get-Command Get-FileHash | % CommandType
            Module      = Get-Command Get-FileHash | % Module | % {[System.IO.FileInfo]::new($_.Path).Name}
        }
    }
    function m2 {q1} # this call should not alter the Get-FileHash that is visible in module p
} | Import-Module
New-Module p {
    function p1 {
        [pscustomobject]@{
            Call        = $MyInvocation.MyCommand.Name
            CommandType = Get-Command Get-FileHash | % CommandType
            Module      = Get-Command Get-FileHash | % Module | % {[System.IO.FileInfo]::new($_.Path).Name}
        }
    }
} | Import-Module

m1
p1
m2
m1
p1

Expected behavior

Either this behavior (which is how PowerShell 5.1 behaves)

Call CommandType Module
---- ----------- ------
m1      Function q.psm1
p1      Function q.psm1
m1      Function q.psm1
p1      Function q.psm1

or this behavior

Call CommandType Module
---- ----------- ------
m1        Cmdlet Microsoft.PowerShell.Utility.psd1
p1        Cmdlet Microsoft.PowerShell.Utility.psd1
m1      Function q.psm1
p1        Cmdlet Microsoft.PowerShell.Utility.psd1

Actual behavior

Call CommandType Module
---- ----------- ------
m1        Cmdlet Microsoft.PowerShell.Utility.psd1
p1        Cmdlet Microsoft.PowerShell.Utility.psd1
m1      Function q.psm1
p1      Function q.psm1

Environment data


Name                           Value
----                           -----
PSVersion                      7.0.0
PSEdition                      Core
GitCommitId                    7.0.0
OS                             Microsoft Windows 6.3.9600
Platform                       Win32NT
PSCompatibleVersions           {1.0, 2.0, 3.0, 4.0…}
PSRemotingProtocolVersion      2.3
SerializationVersion           1.1.0.1
WSManStackVersion              3.0
@alx9r alx9r added the Issue-Question ideally support can be provided via other mechanisms, but sometimes folks do open an issue to get a label Mar 5, 2020
@iSazonov
Copy link
Collaborator

iSazonov commented Mar 5, 2020

It is expected behavior that latest module exports names in global and overloads previously defined names.

@alx9r
Copy link
Author

alx9r commented Mar 5, 2020

It is expected behavior that latest module exports names in global and overloads previously defined names.

@iSazonov Got it. Do you think I can rely on the shadowing that occurs when Import-Module is invoked within a module? This is the example:

New-Module m {
    Import-Module q
    function m1 {
        [pscustomobject]@{
            Call        = $MyInvocation.MyCommand.Name
            CommandType = Get-Command Get-FileHash | % CommandType
            Module      = Get-Command Get-FileHash | % Module | % {[System.IO.FileInfo]::new($_.Path).Name}
        }
    }
    function m2 {q1}
} | Import-Module
New-Module p {
    Import-Module Microsoft.PowerShell.Utility
    function p1 {
        [pscustomobject]@{
            Call        = $MyInvocation.MyCommand.Name
            CommandType = Get-Command Get-FileHash | % CommandType
            Module      = Get-Command Get-FileHash | % Module | % {[System.IO.FileInfo]::new($_.Path).Name}
        }
    }
} | Import-Module

m1
p1
m2
m1
p1
m1
p1

which outputs

Call CommandType Module
---- ----------- ------
m1      Function q.psm1
p1      Function Microsoft.PowerShell.Utility.psd1
m1      Function q.psm1
p1      Function Microsoft.PowerShell.Utility.psd1
m1      Function q.psm1
p1      Function Microsoft.PowerShell.Utility.psd1

Note that in this example the command to which each call site is resolved is the same for each call at that site.

@iSazonov
Copy link
Collaborator

iSazonov commented Mar 5, 2020

If you ask about workaround, yes, this is good because you use a module context to avoid name collisions.

@alx9r
Copy link
Author

alx9r commented Mar 5, 2020

Can you give an example where this new behaviour is causing an issue? It would help us pin down the change and the issue

@rjmholt This repro is an abstraction of how this caused an issue. I've got a repo of 50 or so modules that I'm transitioning from Windows PowerShell. I'm relying on the 20k or so unit and integration tests from those modules to hopefully get this to happen smoothly. Under PowerShell 5.1 none of the modules had Import-Module calls that corresponded to the modules required. Instead they seemed to have relied only on the automatic loading resulting from RequiredModules in the manifest. Per my OP here that results in consistent results under PowerShell 5.1 because, it seems, under 5.1 no matter what the order that other code is invoked, from the perspective of each module, command resolution remained the same. Under PowerShell 7, though, that doesn't hold, and the order that modules auto-load their requirements matters (or matters more?).

In my case I had an inadvertent name collision on Add-SqlLogin (between this implementation and in one of our internal modules. A third module that called Add-SqlLogin either successfully called the internal implementation or failed trying to call the one from the public SqlServer module. The failure depended on whether a command from a fourth module that auto-imports the public SqlServer module was invoked before or after the third module. That's no fun.

The reliance for name resolution by modules on the global scope is, I think, a mistake that happened to not manifest in 5.1. My task currently is to determine how to ensure name resolution occurs reliably within each module without ever reaching the global scope. Hopefully I can find such a solution.

My intuition is that this will probably be a one-time cleanup with some internal policy changes around importing of required modules. I think I prefer the new command resolution behavior, but I've still got a bit of work to do to determine whether I can get that to stabilize with our modules.

@alx9r
Copy link
Author

alx9r commented Apr 7, 2020

...this will probably be a one-time cleanup with some internal policy changes around importing of required modules.

Here is a summary of the policy I implemented to mitigate this issue.

Policy

  1. Each module shall invoke Import-Module for each of the modules specified by the RequiredModules entry in the module manifest.
  2. Import-Module in (1) shall be invoked at the top scope of the module's session state.
  3. $PSModuleAutoloadingPreference shall be None during Import-Module in (1).
  4. During the invokation of automated tests $PSModuleAutoloadingPreference shall be None.

Reasoning

(1) and (2) adequately shadow command names to ensure name resolution occurs within the module. (3) and (4) ensure that automated tests will fail for nearly all commands if they are not imported per (1).

Limitations

If a command that is already imported when PowerShell launches is not imported per (1), then that command is still vulnerable to this issue.

@iSazonov iSazonov added the WG-Engine core PowerShell engine, interpreter, and runtime label Apr 8, 2020
@alx9r
Copy link
Author

alx9r commented Apr 8, 2020

@iSazonov I just reviewed this issue. I think the key point is this statement you made:

It is expected behavior that latest module exports names in global and overloads previously defined names.

I think then, that the behavior I described in my OP is by design. If you agree, I'd like to close this issue. I think #12014 sufficiently covers the outstanding issue with surprising command name resolution.

@iSazonov
Copy link
Collaborator

iSazonov commented Apr 9, 2020

Ok, we can reopen in any time if needed.

@iSazonov iSazonov added the Resolution-By Design The reported behavior is by design. label Apr 9, 2020
@alx9r alx9r closed this as completed Apr 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue-Question ideally support can be provided via other mechanisms, but sometimes folks do open an issue to get a Resolution-By Design The reported behavior is by design. WG-Engine core PowerShell engine, interpreter, and runtime
Projects
None yet
Development

No branches or pull requests

2 participants