@cmkb3 please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.

@microsoft-github-policy-service agree [company="{your company}"]

Options:

• (default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.

@microsoft-github-policy-service agree

• (when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.

@microsoft-github-policy-service agree company="Microsoft"

Contributor License Agreement

@microsoft-github-policy-service agree

Thanks for finding and patching the issue.

I suggest to discuss replacing Progress = 16, with Progress = 8, in internal enum PreferenceVariable for performance reason (with the PR the tuple size jumps from 16 to 32).

If I understand correctly, the enum value must be unique within the combination of both AutomaticVariable and PreferenceVariable internal enum sets. Value 8 already exists within AutomaticVariable as PSCommandPath.

    internal enum AutomaticVariable
     {
        Underbar = 0,
        Args = 1,
        This = 2,
        Input = 3,
        PSCmdlet = 4,
        PSBoundParameters = 5,
        MyInvocation = 6,
        PSScriptRoot = 7,
        PSCommandPath = 8,
        NumberOfAutomaticVariables // 1 + the last, used to initialize global scope.
    }

    internal enum PreferenceVariable
    {
        Debug = 9,
        Verbose = 10,
        Error = 11,
        WhatIf = 12,
        Warning = 13,
        Information = 14,
        Confirm = 15,
        Progress = 16,
    }

If I understand correctly,

Yes, I see. Could you please add new Assert like in line 718 for PreferenceVariableTypes?

If we can not use value less 16 we will allocate double size tuples so we have to think about whether we need to fix the performance issue and how.

If we can not use value less 16 we will allocate double size tuples so we have to think about whether we need to fix the performance issue and how.

While we should think about this the damage is already done by accepting and implementing the new parameter in 7.4 . We should fix this issue first then investigate the performance side after not hold off a bugfix for a problem today.

Yes, I see. Could you please add new Assert like in line 718 for PreferenceVariableTypes?

What is this in reference to? Neither of the changed files are above 500 lines; searching for 'Assert' in either returned no results for me.

Yes, I see. Could you please add new Assert like in line 718 for PreferenceVariableTypes?

What is this in reference to? Neither of the changed files are above 500 lines; searching for 'Assert' in either returned no results for me.

Ah, sorry!

If we can not use value less 16 we will allocate double size tuples so we have to think about whether we need to fix the performance issue and how.

While we should think about this the damage is already done by accepting and implementing the new parameter in 7.4 . We should fix this issue first then investigate the performance side after not hold off a bugfix for a problem today.

Currently we haven't the performance issue because the feature doesn't work. We should either revert #18887 or fix without any (perf) regression.

Currently we haven't the performance issue because the feature doesn't work. We should either revert #18887 or fix without any (perf) regression.

It works just fine for binary cmdlets, it’s advanced functions where it fails and this fixes. We only loose out on the size 16 tuple if the function didn’t have any paramaters or variables (or was un-optimisable). I would say the vast majority of advanced functions will have at least one parameter or one variable making it go beyond 16 in even without this addition.

Ah, sorry!

PowerShell/src/System.Management.Automation/engine/parser/VariableAnalysis.cs

Lines 718 to 721 in c2f1ff0

	Diagnostics.Assert(!_disableOptimizations
	|| orderedLocals.Length == (int)AutomaticVariable.NumberOfAutomaticVariables +
	(scriptCmdlet ? SpecialVariables.PreferenceVariables.Length : 0),
	"analysis is incorrectly allocating number of locals when optimizations are disabled.");

Are you looking for something like this (@iSazonov)? Asserts that the enums match the associated array names/types lengths.

Diagnostics.Assert(AutomaticVariable.NumberOfAutomaticVariables == SpecialVariables.AutomaticVariables.Length
    && AutomaticVariable.NumberOfAutomaticVariables == SpecialVariables.AutomaticVariableTypes.Length,
    "automatic variable enum length does not match both automatic variable and automatic variable types length.");

Diagnostics.Assert(SpecialVariables.PreferenceVariable.Length == SpecialVariables.PreferenceVariables.Length
    && SpecialVariables.PreferenceVariable.Length == SpecialVariables.PreferenceVariableTypes.Length,
    "preference variable enum length does not match both preference variable and preference variable types length.").

Are you looking for something like this (@iSazonov)? Asserts that the enums match the associated array names/types lengths.

Yes. We should add check for types you change in the PR.

Have you checked the existing assert in DEBUG mode? I guess it will throw now.

Have you checked the existing assert in DEBUG mode? I guess it will throw now.

The debug statements

Diagnostics.Assert(AutomaticVariable.NumberOfAutomaticVariables == SpecialVariables.AutomaticVariables.Length
    && AutomaticVariable.NumberOfAutomaticVariables == SpecialVariables.AutomaticVariableTypes.Length,
    "automatic variable enum length does not match both automatic variable and automatic variable types length.");

Diagnostics.Assert(SpecialVariables.PreferenceVariable.Length == SpecialVariables.PreferenceVariables.Length
    && SpecialVariables.PreferenceVariable.Length == SpecialVariables.PreferenceVariableTypes.Length,
    "preference variable enum length does not match both preference variable and preference variable types length.").

Do not throw as they are just checking that the slots used for the automatic vars and action preferences match up together. In this case the enum wasn't populated with the progress preference var so the slots still match up with what was in the enum.

If we wanted to add a debug assertion we could potentially do add something to the below which somehow checks the enums defined here. I don't know off a good check for this off the top of my head though.

The debug statements

[...]

Do not throw as they are just checking that the slots used for the automatic vars and action preferences match up together. In this case the enum wasn't populated with the progress preference var so the slots still match up with what was in the enum.

If the second debug statement I proposed existed in the current codebase (pre-merge), it should have thrown, if I understand correctly.

Edit: accidentally copied the same section twice 🙃

// Enum, length of 8
internal enum PreferenceVariable
{
    Debug = 9,
    Verbose = 10,
    Error = 11,
    WhatIf = 12,
    Warning = 13,
    Information = 14,
    Confirm = 15,
    Progress = 16,
};

// Array of preference values, length of 7
internal static readonly string[] PreferenceVariables =
{
    SpecialVariables.DebugPreference,
    SpecialVariables.VerbosePreference,
    SpecialVariables.ErrorActionPreference,
    SpecialVariables.WhatIfPreference,
    SpecialVariables.WarningPreference,
    SpecialVariables.InformationPreference,
    SpecialVariables.ConfirmPreference,
};

// Array of preference variable types, length of 7
internal static readonly Type[] PreferenceVariableTypes =
{
    /* DebugPreference */                         typeof(ActionPreference),
    /* VerbosePreference */                       typeof(ActionPreference),
    /* ErrorPreference */                         typeof(ActionPreference),
    /* WhatIfPreference */                        typeof(SwitchParameter),
    /* WarningPreference */                       typeof(ActionPreference),
    /* InformationPreference */                   typeof(ActionPreference),
    /* ConfirmPreference */                       typeof(ConfirmImpact),
};

// Debug assert, effectively becomes "Assert 8 == 7 && 8 == 7"
Diagnostics.Assert(SpecialVariables.PreferenceVariable.Length == SpecialVariables.PreferenceVariables.Length
    && SpecialVariables.PreferenceVariable.Length == SpecialVariables.PreferenceVariableTypes.Length,
    "preference variable enum length does not match both preference variable and preference variable types length.").

Ah my apologies I misread things.

@cmkb3 I wonder why do you change nuget configs? Do you use Start-PSBuild -UseNuGetOrg?

@cmkb3 I wonder why do you change nuget configs? Do you use Start-PSBuild -UseNuGetOrg?

Yes, I'm a git idiot and accidentally pushed those configs when trying to not do that. Tried to undo it and I've only made it worse so far, I'm afraid 😩

@cmkb3 You can close the PR and open new one with clean commit.

📣 Hey @cmkb3, how did we do? We would love to hear your feedback with the link below! 🗣️

🔗 https://aka.ms/PSRepoFeedback

Microsoft Forms