Add support for exporting aliases using `*-Alias` in public scripts

[johlju]

This PR adds to the behavior of exporting aliases. It will export aliases declared with [Alias()] but it is now also possible to export aliases using New-Alias and Set-Alias that are defined in separate script file (at script level) in a public script (in Public folder). It will also handle if Remove-Alias is used and will assume that alias should not be exported.

Prior to this change GetCommandAlias was passed an AST for the entire built module and parsed the AST for [Alias()]. In this change the AST is also parsed for the commands New-Alias, Set-Alias, and Remove-Alias. If a New-Alias, Set-Alias is part of a file that is placed in the folder Public and have the same basename as the alias name it will be exported (assuming the file name is filtered as part of the PublicFilter).

Re-opening this PR as a new PR for a new attempt to get this merged. I have some time to work on this now if there are any needed changes. Previous PR was here https://github.com/PoshCode/ModuleBuilder/pull/104, I was unable to re-open the previous PR.

[Jaykul]

Hey @gaelcolas happy with this?

[Jaykul]

Actually, what do you two think about this, for GetCommandAlias:

using namespace System.Management.Automation.Language
using namespace System.Collections.Generic

# This is used only to parse the parameters to New|Set|Remove-Alias
class AliasParameterVisitor : AstVisitor {
    [string]$Parameter = $null
    [string]$Command = $null
    [string]$Name = $null
    [string]$Value = $null
    [string]$Scope = $null

    # Parameter Names
    [AstVisitAction] VisitCommandParameter([CommandParameterAst]$ast) {
        $this.Parameter = $ast.ParameterName
        return [AstVisitAction]::Continue
    }

    # Parameter Values
    [AstVisitAction] VisitStringConstantExpression([StringConstantExpressionAst]$ast) {
        # The FIRST command element is always the command name
        if (!$this.Command) {
            $this.Command = $ast.Value
            return [AstVisitAction]::Continue
        } else {
            switch ($this.Parameter) {
                "Scope" {
                    $this.Scope = $ast.Value
                }
                "Name" {
                    $this.Name = $ast.Value
                }
                "Value" {
                    $this.Value = $ast.Value
                }
                default {
                    if (!$this.Parameter) {
                        # For bare arguments, the order is Name, Value:
                        if (!$this.Name) {
                            $this.Name = $ast.Value
                        } else {
                            $this.Value = $ast.Value
                        }
                    }
                }
            }

            $this.Parameter = $null

            # If we have enough information, stop the visit
            # For -Scope global, it might as well be Remove-Alias, we don't want to export these
            if ($this.Name -and ($this.Command -eq "Remove-Alias" -or $this.Scope -eq "Global")) {
                $this.Command = "Remove-Alias"
                return [AstVisitAction]::StopVisit
            }
            return [AstVisitAction]::Continue
        }
    }

    [AliasParameterVisitor] Clear() {
        $this.Command = $null
        $this.Parameter = $null
        $this.Name = $null
        $this.Value = $null
        $this.Scope = $null
        return $this
    }
}

# This visits everything at the top level of the script
class AliasVisitor : AstVisitor {
    [HashSet[String]]$Aliases = @()
    [AliasParameterVisitor]$Parameters = @{}

    # The [Alias(...)] attribute on functions matters, but we can't export aliases that are defined inside a function
    [AstVisitAction] VisitFunctionDefinition([FunctionDefinitionAst]$ast) {
        @($ast.Body.ParamBlock.Attributes.Where{ $_.TypeName.Name -eq "Alias" }.PositionalArguments.Value).ForEach({ $this.Aliases.Add($_) })
        return [AstVisitAction]::SkipChildren
    }

    # Top-level commands matter, but only if they're alias commands
    [AstVisitAction] VisitCommand([CommandAst]$ast) {
        if ($ast.CommandElements[0].Value -match "(New|Set|Remove)-Alias") {
            $ast.Visit($this.Parameters.Clear())
            if ($this.Parameters.Command -eq "Remove-Alias") {
                $this.Aliases.Remove($this.Parameters.Name)
            } else {
                $this.Aliases.Add($this.Parameters.Name)
            }
        }
        return [AstVisitAction]::SkipChildren
    }
}

function GetCommandAlias {
    <#
        .SYNOPSIS
            Parses one or more files for aliases and returns a list of alias names.
    #>
    [CmdletBinding()]
    param(
        # The AST to find aliases in
        [Parameter(Mandatory, ValueFromPipelineByPropertyName, ValueFromPipeline)]
        [System.Management.Automation.Language.Ast]$AST
    )
    begin {
        $Visitor = [AliasVisitor]::new()
    }
    process {
        $Ast.Visit($Visitor)
    }
    end {
        $Visitor.Aliases
        }
}

It seems to be faster, but is it also easy to follow? Oh, and I think it will also handle unusual but valid parameter order like:

New-Alias -Value Remove-Alias rma
New-Alias -Name rmal Remove-Alias 
Set-Alias -Scope Global rma Remove-Alias
Set-Alias -Force rmal Remove-Alias

[johlju]

@Jaykul replacing GetCommandAlias with the code snippet you provided fails both the unit tests and integration tests. :/ I pushed the changes as per the other review comments.

[johlju]

I did a bit of modification and got the snippet above working.

One note. It was not possible to have using statement in the file GetCommandalias.ps1. It fails one that using statement is not first in the modulebuilder.psm1 file, and if I move the to the source modulebuilder.psm1 the file GetCommandAlias.ps1 cannot be parsed during build, so ended up using the full types in the source file directly.

[johlju]

Ready for review again. Thanks @Jaykul.

[Jaykul]

Oh hey, you didn't need to integrate it necessarily -- I hope that means you actually liked it ;-)

[Jaykul]

The way I had written it, it returned an array, instead of a hashtable, so it was pretty different.

Also, re. the using not working, that's because our build uses a very simplified version of ModuleBuilder to build ModuleBuilder ;-) and it didn't handle moving using statements. But we can fix that by moving the classes to their own file(s). I'm going to merge this and then refactor that back.

[johlju]

Oh hey, you didn't need to integrate it necessarily -- I hope that means you actually liked it ;-)

I learned something new, so that way I liked it. 😊 Not sure if it was more readable though, somewhat higher level to understand for contributors, but since it had the potential to handle more scenarios than I had in the tests I implemented it 😃

Thanks for merging. When releases this will simplify the build process for a few modules. 😃

[Jaykul]

@johlju I am not comfortable with the FunctionsToExport being limited by AliasesToExport.

I mean, I know you're doing it because you used a "Public" function to define an Alias in, but it breaks the fundamental concept of the "Public" filter and the 1:1 file -> function behavior.

You can define aliases anywhere. At the bottom of each function file, all in the "footer" in build.psd1, in a separate "aliases" folder ... or whatever -- but the idea that an alias prevents exporting a function that's otherwise supposed to be exported gives me a problem.

Are you ok with that?

[johlju]

@johlju I am not comfortable with the FunctionsToExport being limited by AliasesToExport.

I mean, I know you're doing it because you used a "Public" function to define an Alias in, but it breaks the fundamental concept of the "Public" filter and the 1:1 file -> function behavior.

You can define aliases anywhere. At the bottom of each function file, all in the "footer" in build.psd1, in a separate "aliases" folder ... or whatever -- but the idea that an alias prevents exporting a function that's otherwise supposed to be exported gives me a problem.

Are you ok with that?

It should be possible to add an alias any where an it should be added to AliasToExport. But it should not add one of those aliases to the FunctionsToExport property. That was my intent. but I can look into if it will remove a function if an alias is in a public file, it is not supposed to do that.

[Jaykul]

You can have aliases anywhere -- but -- you cannot have a file in the "public" filter that doesn't have a function that gets exported.

[Jaykul]

The way it was written, there's a line of code that says, basically:

for all the public function files, if there's not an alias with that name, export them.

To me, that breaks the whole concept, because the core premise is:

for each public function file, export the function

[johlju]

Working on it, sending in a draft PR shortly, need help on it.

[Jaykul]

Don't worry about it, I'm refactoring a bit anyway. Let me send a PR, and you tell me if it still satisfies ;-)

[johlju]

Ah, sent in a draft PR now, but having issues with avoiding getting aliases from the private functions. I will see if your PR fixes it 😉 This is the draft PR: https://github.com/PoshCode/ModuleBuilder/pull/115#issuecomment-1159517409