PowerShell / PSScriptAnalyzer

Download ScriptAnalyzer from PowerShellGallery
https://www.powershellgallery.com/packages/PSScriptAnalyzer/
MIT License
1.84k stars 372 forks source link

PSUseOutputTypeCorrectly complains "... type is not declared in the OutputType attribute" even though it is #1765

Open jantari opened 2 years ago

jantari commented 2 years ago

Steps to reproduce

PSScriptAnalyzer is complaining about a missing OutputType attribute, even though I did set it correctly. See PSScriptAnalyzer CI comment under my commits: https://github.com/jantari/LSUClient/commit/a6c2fcc1622aa221d0be73debb4a97baea7b36b5

Easiest way to reproduce is test with my exact file:

mkdir test-pssa
curl.exe "https://raw.githubusercontent.com/jantari/LSUClient/develop/public/Install-LSUpdate.ps1" -o .\test-pssa\Install-LSUpdate.ps1
Import-Module PSScriptAnalyzer -RequiredVersion 1.20.0 # Latest
Invoke-ScriptAnalyzer -Path .\test-pssa -ExcludeRule PSAvoidTrailingWhitespace -Recurse -Verbose

Expected behavior

PSScriptAnalyzer should have no complaints.

Actual behavior

RuleName                            Severity     ScriptName Line  Message
--------                            --------     ---------- ----  -------
PSUseOutputTypeCorrectly            Information  Install-LS 93    The cmdlet 'Install-LSUpdate' returns an object of type
                                                 Update.ps1       'PackageInstallResult' but this type is not declared in the
                                                                  OutputType attribute.
PSUseOutputTypeCorrectly            Information  Install-LS 125   The cmdlet 'Install-LSUpdate' returns an object of type
                                                 Update.ps1       'PackageInstallResult' but this type is not declared in the
                                                                  OutputType attribute.

Environment data

Issue is reproducible under both:

> $PSVersionTable
Name                           Value
----                           -----
PSVersion                      7.2.1
PSEdition                      Core
GitCommitId                    7.2.1
OS                             Microsoft Windows 10.0.22538
Platform                       Win32NT
PSCompatibleVersions           {1.0, 2.0, 3.0, 4.0…}
PSRemotingProtocolVersion      2.3
SerializationVersion           1.1.0.1
WSManStackVersion              3.0

> (Get-Module -ListAvailable PSScriptAnalyzer).Version | ForEach-Object { $_.ToString() }
1.20.0
1.19.1

and:

> $PSVersionTable
Name                           Value
----                           -----
PSVersion                      5.1.22538.1000
PSEdition                      Desktop
PSCompatibleVersions           {1.0, 2.0, 3.0, 4.0...}
BuildVersion                   10.0.22538.1000
CLRVersion                     4.0.30319.42000
WSManStackVersion              3.0
PSRemotingProtocolVersion      2.3
SerializationVersion           1.1.0.1

> (Get-Module -ListAvailable PSScriptAnalyzer).Version | ForEach-Object { $_.ToString() }
1.19.0
1.20.0
1.19.1
bergmeister commented 2 years ago

PSScriptAnalyzer is correct as you've not specified the attribute correctly, the PowerShell parser is just a bit lax and is not giving you an error. You need to specify a type but you've specified a string, i.e. it needs to be [OutputType([PackageInstallResult])] instead of [OutputType('PackageInstallResult')], see docs here: https://docs.microsoft.com/powershell/module/microsoft.powershell.core/about/about_functions_outputtypeattribute Consider the below as a proof that your way is incorrect and PowerShell does not recognise it as an OutputType on the function

function correct
{
    [OutputType([int])]
    Param()
    '1'
}

function wrong
{
    [OutputType('int')]
    Param()
    '1'
}

(Get-Command correct).OutputType
(Get-Command wrong).OutputType
jantari commented 2 years ago

Woah, that is silly. I had picked up the "specify the type name as a string" syntax somewhere a long time ago and this was the first time ever that PSSA complained about / didn't accept it.

I've been doing this wrong the entire time (and many many more times in other projects / scripts ....)

well damn ... but thanks a lot for clearing that up. I can't remember where I got this syntax from, but you've got me questioning everything now haha

SOLVED :tada:

jantari commented 2 years ago

No, wait a second, I'm not crazy. The article you linked does mention that specifying the TypeName as a string should be legitimate:

[OutputType([<TypeLiteral>], ParameterSetName="<Name>")]
[OutputType("<TypeNameString>", ParameterSetName="<Name>")]

and even uses that syntax in one of the examples:

https://docs.microsoft.com/en-us/powershell/module/microsoft.powershell.core/about/about_functions_outputtypeattribute?view=powershell-7.2#example-2-use-the-output-attribute-to-indicate-dynamic-output-types

so is it a problem in PSSA after all?

EDIT:

Yea, it works:

function not-wrong-after-all
{
     [OutputType('System.Int32')]
     Param()
     '1'
}

(Get-Command not-wrong-after-all).OutputType
bergmeister commented 2 years ago

Fair enough, it seems that if I added [CmdletBinding()] in my examples, it would've worked. However, what is happening here I think is that at the time of analysing it, it does not know about the type and cannot resolve it. If you imported the module that defined this type before running Invoke-ScriptAnalyzer you would not be getting this false positive, simple test by running class PackageInstallResult {} to define this type proves that. Using brackets instead of single quotes make ScriptAnalyzer not return a false positive either so definitely a workaround as well for now. I will take a detailed look later whether this rule could be tweaked.

jantari commented 2 years ago

I will take a detailed look later whether this rule could be tweaked.

Thanks, some additional context for then:

what is happening here I think is that at the time of analysing it, it does not know about the type and cannot resolve it. If you imported the module that defined this type before running Invoke-ScriptAnalyzer you would not be getting this false positive

That is not the case because it is not possible* to export classes from a module, unfortunately. Classes defined in a module are only accessible within the module when it is imported with Import-Module. When the half-baked using module statement** is used instead of Import-Module to load a module then defined classes are imported into the current (global) scope and are accessible, but there is no way to control class-exporting from within the module manifest, it is just up to how the user loads a module.

* Not in a controlled manner (e.g. pick the classes to export) and not without permanently polluting the global scope. Using Add-Type inside the module and defining the class in inline Csharp code makes it available in the global scope, but then there is no way to remove / unload it again other than closing and re-opening PowerShell which is a bad user-experience imo.

** using module has several drawbacks and cannot always be used so it is basically kind of DoA and to be ignored for any actual real-world use, but for the sake of testing this problem:

using module "C:\full\path\to\module\LSUClient.psd1"
Invoke-ScriptAnalyzer -Path .\ -ExcludeRule PSAvoidTrailingWhitespace -Recurse -Verbose

did get rid of the PSUseOutputTypeCorrectly complaints for Install-LSUpdate (and surfaced several others, but that was to be expected, now that the module is fully loaded it finds all my custom types and recognizes when they are being returned)

What this means for PSSA then is that:

Either it should fail the [OutputType([PackageInstallResult])] syntax as well, because the class can't be found even if the module is imported with Import-Module regardless of the syntax used in the OutputType attribute.

Or PSSA needs to do some of the magic that Pester is able to perform to run code / assertions inside the code scope of the module, which would give it visibility into the classes defined there. According to this StackOverflow answer that can also be done with the & (Get-Module 'ModuleName') { <# ScriptBlock #> } syntax. And indeed:

cd lsuclient-project-dir
Import-Module .\LSUClient.psd1
Import-Module PSScriptAnalyzer -RequiredVersion 1.20.0
# This yields dramatically different results:
Invoke-ScriptAnalyzer -Path .\ -ExcludeRule PSAvoidTrailingWhitespace -Recurse
# Than this:
& (Get-Module 'LSUClient') { Invoke-ScriptAnalyzer -Path .\ -ExcludeRule PSAvoidTrailingWhitespace -Recurse }

looks like i maybe opened the ugly can o' worms on how half-baked PowerShell modules and classes are. Oops.

jantari commented 2 years ago

Using my module as the grounds for experimentation, it appears that:

  1. PSScriptAnalyzer always recognizes built-in types (e.g. [System.Int32], [System.Collections.Hashtable] etc.) in the OutputType attribute regardless of how they are specified (type-notation in square brackets or the type name as a string)

  2. PSScriptAnalyzer does not recognize module-internal types reliably; neither in OutputType nor when they are being returned. This is evidenced by the fact that scripts or functions that return custom types (such as Get-PackagesInRepository) but do not declare them in an OutputType attribute aren't flagged / found by PSSA - except for the [PackageInstallResult] type being returned by Install-LSUpdate. I don't know what is going on there, because it doesn't find the custom types in any other function. That is, unless PPSA is explicitly invoked inside the module scope (where these types are loaded) with the relatively obsure & (Get-Module 'LSUClient') { Invoke-ScriptAnalyzer <# Params #> } syntax. This could be a consequence of PowerShells and Import-Module's handling of classes and scope rather than PSSA, but then how does it detect [PackageInstallResult] being returned from Install-LSUpdate even when scanning from out-of-module scope?

  3. Running PSScriptAnalyzer inside of the module scope also isn't a "solution" because it produces some undesired results - for example, it complains that there is no OutputType attribute set for my function Get-LSUpdate. However, when I add it, the function / cmdlet doesn't work at all anymore in PowerShell:

    jantari@AMDESKTOP:~\projectdir\path\lsuclient
    └─ PS> Import-Module .\LSUClient.psd1
    jantari@AMDESKTOP:~\projectdir\path\lsuclient
    └─ PS> Get-LSUpdate -Model 20U5 -All -Verbose
    Unable to find type [LenovoPackage].
    At C:\Users\jantari\projectdir\path\lsuclient\public\Get-LSUpdate.ps1:65 char:17
    +     [OutputType([LenovoPackage])]
    +                 ~~~~~~~~~~~~~~~
        + CategoryInfo          : InvalidOperation: (LenovoPackage:TypeName) [], RuntimeException
        + FullyQualifiedErrorId : TypeNotFound

    so that suggestion by PSSA wasn't great. It appears PowerShell itself also isn't aware of module-internal classes/types at the time of parsing the OutputType. However, PowerShell does accept it when I use TypeName string notation ( [OutputType('LenovoPackage')] ) - no clue why. So to summarize, I have to use [TypeName] notation for PSSA to accept it, but I have to use "TypeName" notation for the cmdlet to actually work 😅

  4. It is debatable whether the fact that PSScriptAnalyzer silently ignores unrecognized (thus seemingly invalid) types being returned by functions without an OutputType attribute is a bug or not. Personally, I would have kind of assumed that an apparent non-existant "garbgae" type being returned from a function would be a linting error, as it could be a typo or the result of a function not having been updated after a class was renamed or removed

  5. On top of that, something is for sure funky with PSSA in regards to accepting the "TypeNameString" OutputType attribute syntax though. It accepts it for built-in types as stated in point 1, but when PSSA is invoked via the & (Get-Module 'LSUClient') { Invoke-ScriptAnalyzer <# Params #> } syntax so it is able to see module-internal types, it will still only accept OutputType attributes for them in square-bracket type notation. This I believe is a bug in PSSA, as there is no longer a scoping issue preventing PSSA from seeing these types, it just doesn't parse or recognize them properly when they are specified by their TypeName. Why it works for built-in types I can't say, I'd guess the parser in PSSA takes a different, bug-free code path for those.