Should Script Analyzer deal with binary modules?

[KirkMunro]

Here's a thought that just came to mind while discussing pluralized nouns in more Azure cmdlets with another MVP:

If a module author creates a binary module with cmdlets that have pluralized nouns, I think (haven't specifically tested these scenarios myself) they can package that up in a module in one of two lazy ways:

1. Drop the dll into a folder with the same name as the dll (minus the extension), and voila, it's auto-discoverable via PowerShell if it's in the right path.
2. Drop the dll into a folder with the same name as the dll, along with a manifest with the same name so that you get versioning, have the manifest load the binary module, but leave the default '*' values for all Export_whatever_ manifest attributes so that all cmdlets are exported.

In both of these scenarios, there is nothing that PSScriptAnalyzer can do to flag issues like pluralized nouns or non-standard verbs, is there? Does PSScriptAnalyzer even check a manifest for pluralized nouns in the exported command names, if they are listed?

I started thinking about this because I was wondering how/why so many pluralized nouns were getting included in released modules from the AzureRM group, and if they use binary cmdlets, this may be one of the reasons why. I'm looking forward to PSScriptAnalyzer evolving into simply PSAnalyzer or perhaps adopting a PSModuleAnalyzer sibling that can check for these types of issues, either by loading modules as part of the inspection or, preferably, by inspecting with reflection or other means the contents of binary modules for issues with command naming and attribute use.

[joeyaiello]

Looping in @JKeithB here. I agree this seems like a bit of a hole in our moderation strategy.

There's a couple approaches that we could take here, with one leveraging FxCop to do static analysis and the other simply analyzing the manifest and Get-Command output of the module after it's imported. I believe @raghushantha will be following with some thoughts on the benefits of each.

[ghost]

Have to agree with y'all, it's a hole in our story. Not as easy to deal with binary modules purely within PSSA, but some of the things you are mentioning appear to be feasible. @raghushantha - we should discuss figure out what options exist that we can use on the Gallery.

[Jaykul]

When it comes to "moderation" ... at a minimum you should be able to validate the command names and existence of help content 😬

[KirkMunro]

Anything related to command metadata should be candidates for evaluation on binary modules. Parameter attributes (ensuring that parameters of type PSCredential have a matching Credential type conversion attribute on them, for example), cmdlet attributes, OutputType, verb, noun, SupportsShouldProcess, etc.

[joeyaiello]

Next step here is to enumerate all rules which can be analyzed without source (i.e. via only reflection/Get-Command).

[rjmholt]

From the community call:

Is there a PSScriptAnalyzer for binary modules?

Not today -- it's not quite the same use-case as PSScriptAnalyzer today, since the analysis must be mostly of C# and due to the compilation step there's an added question of what do you run the analysis on; the C# source or the built module asset. There are possible advantages to the latter, but there's more established tooling for the former.

We've discussed using Roslyn analysers to improve the module authoring experience, but just in a theoretical "wouldn't it be nice if" way.

Certainly since that issue [this issue] (which predates my joining the PS team) I haven't heard much in the way of requests for it, but I'd love to have the discussion about:

• What are people looking for in terms of binary module linting/analysis, and who wants it?
• What stage should we run it at and what existing tooling should be leverage?
• Should it be part of PSScriptAnalyzer or a separate tool?

[KirkMunro]

• What are people looking for in terms of binary module linting/analysis, and who wants it?

I want it, because14 years into this PowerShell adventure I'm still policing others, including Microsoft partner teams, about it.

I think you're breaking it up wrong though.

There is cmdlet linting/analysis, which looks at cmdlet metadata, checks for the obvious things: pluralized nouns, SupportsShouldProcess usage for verbs where it should be used, approved verbs, proper use of pipeline input (you can just throw ValueFromPipeline on every parameter), but that's wrong, etc.

And then there is module analysis, which isn't about binary modules specifically but includes binary modules, and checks the manifest for improper use of wildcards in exports, looks at verb pairings, looks for manual Import-Module usage in a psm1 file to recommend RequiredModules or NestedModules usage a manifest instead, etc. All of this is about external inspection of a module design. Internal inspection via Roslyn is a completely different ball game, and I would put that as secondary. Let them write crappy internal code if they want (as long as it's open source so that we can call out performance issues and get them corrected), but at least make the public interface clean, consistent, and free from the obvious flaws. Given the number of module bundles that come from companies these days, because module bundling is a better design (faster loading, smaller individual module footprint), perhaps the inspection should include inspection across modules. In a perfect world, maybe it could pick up on duplicate files in modules that could be put into a shared module. I doubt it would ever pick up on these design issues though, although it could inspect the required size of comment-based help or markdown help or both and promote updateable help instead.

• What stage should we run it at and what existing tooling should be leverage?

Post build. Leverage PowerShell's natural ability to inspect and interrogate itself.

• Should it be part of PSScriptAnalyzer or a separate tool?

Either evolve PSScriptingAnalyzer into just PSAnalyzer, or create a PSModuleAnalyzer. Either would work. The point is for there to be a tool, not for that tool to be a multi-functional tool.

[rjmholt]

Interestingly the discussion in PowerShell/PowerShell-RFC#263 has evolved in the opposite direction, suggesting:

• We should implement Roslyn analysers first
• The tooling for those exists, is easy to integrate with existing toolchains, and can check/fix many simple existing issues
• Rolling another analysis engine on our own creates overhead for everyone

I think those points make a lot of sense, although I'd also like to be able to check for things beyond the C# like the module layout and the manifest, and in a way that we could apply it to script modules as well.

[SeeminglyScience]

Another benefit to the roslyn analyzer approach is method body analysis. While it's definitely possible to disassemble the IL after the fact, it becomes a lot more complicated. The use cases I have in mind off the top of my head are:

1. Use WriteObject(object, bool). A really easy place to trip up is to send an array or other IEnumerable to the pipeline as is since WriteObject(object) defaults to not enumerating. This rule would only trigger if object implements IEnumerable (and isn't string, XmlNode, IDictionary, or whatever the other hard coded one is).

2. Implement StopProcessing. Another common slip up I see is folks using the Console.CancelKeyPress event instead of implementing StopProcessing because they don't know it's built in. It looks like it works, and does in most scenarios, but fails when VSCode or other editors/hosts try to initiate a pipeline stop.

Both of those should be fairly simple to implement as well.

[KirkMunro]

If the Roslyn analyzers are easy, then implementing those should absolutely be considered. They will bring a lot of value to the community.

That said, binary modules are more often created by C# developers than PowerShell scripters. Developers get programming, and (often) know how to read/learn an SDK, but the PowerShell cmdlet design, as well as how to properly set up a module using a manifest, those are things that are very clearly not obvious to them. Without a module analyzer or well written documentation that provides the proper guidance, neither of which have ever been provided, well, you end up with community policing after the fact to try and bring modules in line.

If I were to look at where there have been more problems historically, and consider whether those problems lie in improperly configured (binary) cmdlets, in improper module design, or in incorrect implementation details binary cmdlets, while considering the impact to those issues on the community and how they can be corrected, the cmdlet configuration problems as well as improper module design have been more common and their impact has been greater due to common usage scenarios not working as they should, and complexity in correcting those problems due to public interface changes. The internal issues are also important, but I'm 100% sure that the pool of developers who do not know PowerShell very well at all but whose job requires them to create binary modules needs much more help on the design (public interface) of cmdlets and the modules that export those cmdlets more than they need help on the internals. Once something is in place that helps them get the public interfaces correct, then I think going further and helping them with internal code analysis is the right thing to do.

[SeeminglyScience]

If the Roslyn analyzers are easy, then implementing those should absolutely be considered. They will bring a lot of value to the community.

Easier I should say. The API seems pretty simple, but I haven't made one so I can't vouch for it.

That said, binary modules are more often created by C# developers than PowerShell scripters. Developers get programming, and (often) know how to read/learn an SDK, but the PowerShell cmdlet design, as well as how to properly set up a module using a manifest, those are things that are very clearly not obvious to them.

Absolutely. I think a combination of roslyn analyzers and the dotnet new template will do a lot of good there. I wonder if the dotnet team would be open to adding the PowerShell template as a default... @rjmholt

A default dotnet new template with pre-loaded analyzers, and an example cmdlet with the manifest already filled out for it? Could be great.

If I were to look at where there have been more problems historically, and consider whether those problems lie in improperly configured (binary) cmdlets, in improper module design, or in incorrect implementation details binary cmdlets, while considering the impact to those issues on the community and how they can be corrected, the cmdlet configuration problems as well as improper module design have been more common and their impact has been greater due to common usage scenarios not working as they should, and complexity in correcting those problems due to public interface changes. The internal issues are also important, but I'm 100% sure that the pool of developers who do not know PowerShell very well at all but whose job requires them to create binary modules needs much more help on the design (public interface) of cmdlets and the modules that export those cmdlets more than they need help on the internals. Once something is in place that helps them get the public interfaces correct, then I think going further and helping them with internal code analysis is the right thing to do.

Yeah I'm with ya, we're trying to solve the same problems. Is there a scenario you're thinking of where a roslyn analyzer wouldn't work but a custom tool would?

[KirkMunro]

Yeah I'm with ya, we're trying to solve the same problems. Is there a scenario you're thinking of where a Roslyn analyzer wouldn't work but a custom tool would?

Not that I can think of, no. The details of how this is implemented are far less important to me than the functionality that is made available to guide all module authors in the right direction, with priority given towards where guidance is needed the most.

[bergmeister]

A roslyn analyser would be great to warn e.g. in the case when a binary cmdlet declares that it implements supportsshouldprocess (because someone probably copy pasted from the web) but never calls the API to check for that, meaning, it definitely does not implement it. With that embarrassing bugs like this one could be avoided: Azure/azure-powershell#13231 (comment)