diff --git a/Engine/Helper.cs b/Engine/Helper.cs index c39aa2ae8..70bd69f6b 100644 --- a/Engine/Helper.cs +++ b/Engine/Helper.cs @@ -241,10 +241,11 @@ public bool PositionalParameterUsed(CommandAst cmdAst) /// Given a command's name, checks whether it exists /// /// + /// /// - public CommandInfo GetCommandInfo(string name) + public CommandInfo GetCommandInfo(string name, CommandTypes commandType=CommandTypes.All) { - return Helper.Instance.MyCmdlet.InvokeCommand.GetCommand(name, CommandTypes.All); + return Helper.Instance.MyCmdlet.InvokeCommand.GetCommand(name, commandType); } /// @@ -259,6 +260,29 @@ public IEnumerable DscResourceFunctions(Ast ast) && resourceFunctionNames.Contains((item as FunctionDefinitionAst).Name, StringComparer.OrdinalIgnoreCase), true); } + /// + /// Gets all the strings contained in an array literal ast + /// + /// + /// + public List GetStringsFromArrayLiteral(ArrayLiteralAst alAst) + { + List result = new List(); + + if (alAst != null && alAst.Elements != null) + { + foreach (ExpressionAst eAst in alAst.Elements) + { + if (eAst is StringConstantExpressionAst) + { + result.Add((eAst as StringConstantExpressionAst).Value); + } + } + } + + return result; + } + /// /// Returns true if the block should be skipped as it has a name /// that matches keyword diff --git a/Rules/ProvideCommentHelp.cs b/Rules/ProvideCommentHelp.cs index 1e8eec481..b6f0e4c02 100644 --- a/Rules/ProvideCommentHelp.cs +++ b/Rules/ProvideCommentHelp.cs @@ -22,6 +22,7 @@ using System.Globalization; using System.Threading; using System.Reflection; +using System.Management.Automation; namespace Microsoft.Windows.Powershell.ScriptAnalyzer.BuiltinRules { @@ -31,6 +32,8 @@ namespace Microsoft.Windows.Powershell.ScriptAnalyzer.BuiltinRules [Export(typeof(IScriptRule))] public class ProvideCommentHelp : SkipTypeDefinition, IScriptRule { + private HashSet exportedFunctions; + /// /// AnalyzeScript: Analyzes the ast to check that cmdlets have help. /// @@ -42,6 +45,131 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) { DiagnosticRecords.Clear(); this.fileName = fileName; + exportedFunctions = new HashSet(StringComparer.OrdinalIgnoreCase); + List exportFunctionsCmdlet = Helper.Instance.CmdletNameAndAliases("export-modulemember"); + + // find functions exported + IEnumerable cmdAsts = ast.FindAll(item => item is CommandAst + && exportFunctionsCmdlet.Contains((item as CommandAst).GetCommandName(), StringComparer.OrdinalIgnoreCase), true); + + CommandInfo exportMM = Helper.Instance.GetCommandInfo("export-modulemember", CommandTypes.Cmdlet); + + // switch parameters + IEnumerable switchParams = (exportMM != null) ? exportMM.Parameters.Values.Where(pm => pm.SwitchParameter) : Enumerable.Empty(); + + if (exportMM == null) + { + return DiagnosticRecords; + } + + foreach (CommandAst cmdAst in cmdAsts) + { + if (cmdAst.CommandElements == null || cmdAst.CommandElements.Count < 2) + { + continue; + } + + int i = 1; + + while (i < cmdAst.CommandElements.Count) + { + CommandElementAst ceAst = cmdAst.CommandElements[i]; + ExpressionAst exprAst = null; + + if (ceAst is CommandParameterAst) + { + var paramAst = ceAst as CommandParameterAst; + var param = exportMM.ResolveParameter(paramAst.ParameterName); + + if (param == null) + { + i += 1; + continue; + } + + if (string.Equals(param.Name, "function", StringComparison.OrdinalIgnoreCase)) + { + // checks for the case of -Function:"verb-nouns" + if (paramAst.Argument != null) + { + exprAst = paramAst.Argument; + } + // checks for the case of -Function "verb-nouns" + else if (i < cmdAst.CommandElements.Count - 1) + { + i += 1; + exprAst = cmdAst.CommandElements[i] as ExpressionAst; + } + } + // some other parameter. we just checks whether the one after this is positional + else if (i < cmdAst.CommandElements.Count - 1) + { + // the next element is a parameter like -module so just move to that one + if (cmdAst.CommandElements[i + 1] is CommandParameterAst) + { + i += 1; + continue; + } + + // not a switch parameter so the next element is definitely the argument to this parameter + if (paramAst.Argument == null && !switchParams.Contains(param)) + { + // skips the next element + i += 1; + } + + i += 1; + continue; + } + } + else if (ceAst is ExpressionAst) + { + exprAst = ceAst as ExpressionAst; + } + + if (exprAst != null) + { + // One string so just add this to the list + if (exprAst is StringConstantExpressionAst) + { + exportedFunctions.Add((exprAst as StringConstantExpressionAst).Value); + } + // Array of the form "v-n", "v-n1" + else if (exprAst is ArrayLiteralAst) + { + exportedFunctions.UnionWith(Helper.Instance.GetStringsFromArrayLiteral(exprAst as ArrayLiteralAst)); + } + // Array of the form @("v-n", "v-n1") + else if (exprAst is ArrayExpressionAst) + { + ArrayExpressionAst arrExAst = exprAst as ArrayExpressionAst; + if (arrExAst.SubExpression != null && arrExAst.SubExpression.Statements != null) + { + foreach (StatementAst stAst in arrExAst.SubExpression.Statements) + { + if (stAst is PipelineAst) + { + PipelineAst pipeAst = stAst as PipelineAst; + if (pipeAst.PipelineElements != null) + { + foreach (CommandBaseAst cmdBaseAst in pipeAst.PipelineElements) + { + if (cmdBaseAst is CommandExpressionAst) + { + exportedFunctions.UnionWith(Helper.Instance.GetStringsFromArrayLiteral((cmdBaseAst as CommandExpressionAst).Expression as ArrayLiteralAst)); + } + } + } + } + } + } + } + } + + i += 1; + } + } + ast.Visit(this); return DiagnosticRecords; @@ -59,10 +187,8 @@ public override AstVisitAction VisitFunctionDefinition(FunctionDefinitionAst fun return AstVisitAction.SkipChildren; } - if (!string.Equals(funcAst.Name, "Get-TargetResource", StringComparison.OrdinalIgnoreCase) && !string.Equals(funcAst.Name, "Set-TargetResource", StringComparison.OrdinalIgnoreCase) && - !string.Equals(funcAst.Name, "Test-TargetResource", StringComparison.OrdinalIgnoreCase)) + if (exportedFunctions.Contains(funcAst.Name)) { - if (funcAst.GetHelpContent() == null) { DiagnosticRecords.Add( diff --git a/Tests/Rules/BadCmdlet.ps1 b/Tests/Rules/BadCmdlet.ps1 index b93658d0b..546dc31a8 100644 --- a/Tests/Rules/BadCmdlet.ps1 +++ b/Tests/Rules/BadCmdlet.ps1 @@ -50,4 +50,17 @@ End { } -} \ No newline at end of file +} + +# Provide comment help should not be raised here because this is not exported +function NoComment +{ + Write-Verbose "No Comment" +} + +function Comment +{ + Write-Verbose "Should raise providecommenthelp error" +} + +Export-ModuleMember Verb-Files, Comment \ No newline at end of file diff --git a/Tests/Rules/ProvideCommentHelp.tests.ps1 b/Tests/Rules/ProvideCommentHelp.tests.ps1 index 6ea13d0ca..30a183710 100644 --- a/Tests/Rules/ProvideCommentHelp.tests.ps1 +++ b/Tests/Rules/ProvideCommentHelp.tests.ps1 @@ -8,8 +8,8 @@ $noViolations = Invoke-ScriptAnalyzer $directory\GoodCmdlet.ps1 | Where-Object { Describe "ProvideCommentHelp" { Context "When there are violations" { - It "has 1 provide comment help violation" { - $violations.Count | Should Be 1 + It "has 2 provide comment help violations" { + $violations.Count | Should Be 2 } It "has the correct description message" {