Merge pull request #32 from PowerShell/FixProvideCommentHelp

Fix a bug where provide comment help is raised for non exported functions

Author: quoctruong

Engine/Helper.cs

@@ -241,10 +241,11 @@ public bool PositionalParameterUsed(CommandAst cmdAst)
         /// Given a command's name, checks whether it exists
         /// </summary>
         /// <param name="name"></param>
+        /// <param name="commandType"></param>
         /// <returns></returns>
-        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);
         }
 
         /// <summary>
@@ -259,6 +260,29 @@ public IEnumerable<Ast> DscResourceFunctions(Ast ast)
                 && resourceFunctionNames.Contains((item as FunctionDefinitionAst).Name, StringComparer.OrdinalIgnoreCase), true);            
         }
 
+        /// <summary>
+        /// Gets all the strings contained in an array literal ast
+        /// </summary>
+        /// <param name="alAst"></param>
+        /// <returns></returns>
+        public List<string> GetStringsFromArrayLiteral(ArrayLiteralAst alAst)
+        {
+            List<string> result = new List<string>();
+
+            if (alAst != null && alAst.Elements != null)
+            {
+                foreach (ExpressionAst eAst in alAst.Elements)
+                {
+                    if (eAst is StringConstantExpressionAst)
+                    {
+                        result.Add((eAst as StringConstantExpressionAst).Value);
+                    }
+                }
+            }
+
+            return result;
+        }
+
         /// <summary>
         /// Returns true if the block should be skipped as it has a name
         /// that matches keyword

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<string> exportedFunctions;
+
         /// <summary>
         /// AnalyzeScript: Analyzes the ast to check that cmdlets have help.
         /// </summary>
@@ -42,6 +45,131 @@ public IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName) {
 
             DiagnosticRecords.Clear();
             this.fileName = fileName;
+            exportedFunctions = new HashSet<string>(StringComparer.OrdinalIgnoreCase);
+            List<string> exportFunctionsCmdlet = Helper.Instance.CmdletNameAndAliases("export-modulemember");
+
+            // find functions exported
+            IEnumerable<Ast> 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<ParameterMetadata> switchParams = (exportMM != null) ? exportMM.Parameters.Values.Where<ParameterMetadata>(pm => pm.SwitchParameter) : Enumerable.Empty<ParameterMetadata>();
+
+            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(

Tests/Rules/BadCmdlet.ps1

@@ -50,4 +50,17 @@
     End
     {
     }
-}
+}
+
+# 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

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" {