Powershell: `ImportPSModule` on `InitialSessionState` should be `params`.

[twitchax]

Functions in the automation API which take arrays should take them as params.

For example this should be:

        public void ImportPSModule(params string[] name)
        {
            if (name == null)
                throw new ArgumentNullException("name");
            foreach (string n in name)
            {
                ModuleSpecificationsToImport.Add(new ModuleSpecification(n));
            }
        }

A developer should not _have_ to call this by newing up an array of strings.

[iSazonov]

It seems we get extra allocation.

/cc @lzybkr @daxian-dbw

[twitchax]

@iSazonov, what do you mean you get extra allocation?

[iSazonov]

In both cases we do new string[]. Although it is so critical - we call this method rarely.

[twitchax]

Yeah. I meant more for the sake of developers. Regardless of the number of times PowerShell 6 calls it, writing an app which hosts PowerShell needs to explicitly new up a string[], which is not syntactically preferred.

[lzybkr]

FWIW, I believe adding params is a breaking change if you don't recompile your code - you could end up with a missing method exception. This might be a non-issue as there are so few hosts that this might affect.

A better change that might be sufficient is to add some overloads that take 1, 2, or 3 parameters. These overloads would avoid creating an array and cover the most common cases.

[twitchax]

How is adding params a breaking change?

Any caller will have to do

state.ImportPSModule(new string[] { "Module1", "Module2" });

That line will still compile with params. How often do developers upgrade their packages and not recompile? Also, I would need to look at the IL, but my impression was that adding params would not change the signature of the method: it would simply allow the caller to use the syntactic sugar. In the end, only the caller needs to take params into account when using the syntactic sugar call: in the case of a method that already news up an

It is totally cool to make methods that take 1, 2, or 3 parameters, but that only solves part of the problem, as their may be developers that need more. The BCL does this by providing methods with 1, 2, or 3 parameters, and then a fourth overload that takes a params as its fourth parameter.

[twitchax]

Actually, it looks like the IL method signature is the same...there is just an attribute applied for syntactic purposes (I would guess).

private static void WithArray(string[] array)
{
}

goes to

.method private hidebysig static 
    void WithArray (
        string[] 'array'
    ) cil managed 
{
    // Method begins at RVA 0x205e
    // Code size 2 (0x2)
    .maxstack 8

    IL_0000: nop
    IL_0001: ret
} // end of method Program::WithArray

And with params, this

private static void WithParams(params string[] array)
{
}

goes to

.method private hidebysig static 
    void WithParams (
        string[] 'array'
    ) cil managed 
{
    .param [1]
    .custom instance void [System.Runtime]System.ParamArrayAttribute::.ctor() = (
        01 00 00 00
    )
    // Method begins at RVA 0x2061
    // Code size 2 (0x2)
    .maxstack 8

    IL_0000: nop
    IL_0001: ret
} // end of method Program::WithParams

So, even if a developer did not rebuild against new binaries with params, it should still work exactly the same.

[lzybkr]

I'm assuming that the callsig in the reference will look different because of the attribute on the parameter.

For example, C++/CLI uses an attribute to distinguish between int and long when they map to the same CLI type.

[twitchax]

It seems to work fine. I just tried this scenario, and the call works fine, even if you do not rebuild after a dependency updates to use params.

[twitchax]

This change is obviously not required, it is just normally expected that methods like this take params since it just makes life easier for developers.

[lzybkr]

Thanks for checking, my memory was fuzzy. I might have been remembering modopt/modreq which is on the signature, not on the references.

[iSazonov]

It is public API - we should PowerShell Committee conclusion.
/cc @SteveL-MSFT

[powercode]

Maybe not on topic, but related:
Is is possible to filter the imported commands when adding a module to the InitialSessionState?

Like we have on Import-Module with for example -Cmdlet?

Otherwise I'd like an overload on ImportPSModule with filtering options.

[iSazonov]

Internally we use Import-Module cmdlet to import a module in InitialSessionState.

[SteveL-MSFT]

@PowerShell/powershell-committee reviewed this. We believe this is the right change and we should look at all the other public apis to see if this is applicable elsewhere. It doesn't appear to be a breaking change based on the discussion.

[iSazonov]

I found only public static void RemoveKeyHandler(string[] key) and public static int Main(string[] args). The first has still another overload. So I believe there's nothing more to fix.