Adds tests and documetation for the UseToExportFieldsInManifest

Author: Kapil Borle

RuleDocumentation/UseToExportFieldsInManifest.md

@@ -0,0 +1,28 @@
+#UseToExportFieldsInManifest 
+**Severity Level: Warning**
+
+
+##Description
+
+In a module manifest, AliasesToExport, CmdletsToExport, FunctionsToExport and VariablesToExport fields should not use wildcards or $null in their entries. During module auto-discovery, if any of these entries are missing or $null or wildcard, PowerShell does some potentially expensive work to analyze the rest of the module.
+
+##How to Fix
+
+Please consider using an explicit list. 
+
+##Example 1
+
+Wrong： 
+ 	FunctionsToExport = $null
+
+Correct: 
+	FunctionToExport = @()
+
+##Example 2
+Suppose there are only two functions in your module, Get-Foo and Set-Foo that you want to export. Then,
+
+Wrong:	
+	FunctionsToExport = '*'
+
+Correct:
+	FunctionToExport = @(Get-Foo, Set-Foo)	

Rules/ScriptAnalyzerBuiltinRules.csproj

@@ -95,7 +95,7 @@
       <DependentUpon>Strings.resx</DependentUpon>
     </Compile>
     <Compile Include="UseBOMForUnicodeEncodedFile.cs" />
-    <Compile Include="UseManifestExportFields.cs" />
+    <Compile Include="UseToExportFieldsInManifest.cs" />
     <Compile Include="UseOutputTypeCorrectly.cs" />
     <Compile Include="MissingModuleManifestField.cs" />
     <Compile Include="PossibleIncorrectComparisonWithNull.cs" />

Rules/Strings.Designer.cs

@@ -798,16 +798,16 @@
   <data name="AvoidNullOrEmptyHelpMessageAttributeName" xml:space="preserve">
     <value>AvoidNullOrEmptyHelpMessageAttribute</value>
   </data>
-  <data name="UseManifestExportFieldsCommonName" xml:space="preserve">
+  <data name="UseToExportFieldsInManifestCommonName" xml:space="preserve">
     <value>Use the *ToExport module manifest fields.</value>
   </data>
-  <data name="UseManifestExportFieldsDescription" xml:space="preserve">
-    <value>Specify entries for AliasToExport, CmdletsToExport, FunctionsToExport and do not use wildcards and $null in these entries. During module auto-discovery, if any of these entries are missing or $null, PowerShell do some potentially expensive work to analyze the rest of the module</value>
+  <data name="UseToExportFieldsInManifestDescription" xml:space="preserve">
+    <value>In a module manifest, AliasesToExport, CmdletsToExport, FunctionsToExport and VariablesToExport fields should not use wildcards or $null in their entries. During module auto-discovery, if any of these entries are missing or $null or wildcard, PowerShell does some potentially expensive work to analyze the rest of the module.</value>
   </data>
-  <data name="UseManifestExportFieldsError" xml:space="preserve">
-    <value>Do not use wildcards and $null in these entry. Explicitly specify a list for {0}.  </value>
+  <data name="UseToExportFieldsInManifestError" xml:space="preserve">
+    <value>Do not use wildcard or $null in this field. Explicitly specify a list for {0}.  </value>
   </data>
-  <data name="UseManifestExportFieldsName" xml:space="preserve">
-    <value>UseManifestExportFields</value>
+  <data name="UseToExportFieldsInManifestName" xml:space="preserve">
+    <value>UseToExportFieldsInManifest</value>
   </data>
 </root>

Rules/Strings.resx

@@ -17,17 +17,20 @@
 using Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic;
 using System.ComponentModel.Composition;
 using System.Globalization;
+using System.Text.RegularExpressions;
 
 namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules
 {
     /// <summary>
-    /// UseManifestExportFields: Run Test Module Manifest to check that no deprecated fields are being used.
+    /// UseToExportFieldsInManifest: Checks if AliasToExport, CmdletsToExport, FunctionsToExport and VariablesToExport 
+    /// fields do not use wildcards and $null in their entries. 
     /// </summary>
     [Export(typeof(IScriptRule))]
-    public class UseManifestExportFields : IScriptRule
+    public class UseToExportFieldsInManifest : IScriptRule
     {
         /// <summary>
-        /// AnalyzeScript: Run Test Module Manifest to check that no deprecated fields are being used.
+        /// AnalyzeScript: Analyzes the AST to check if AliasToExport, CmdletsToExport, FunctionsToExport 
+        /// and VariablesToExport fields do not use wildcards and $null in their entries. 
         /// </summary>
         /// <param name="ast">The script's ast</param>
         /// <param name="fileName">The script's file name</param>
@@ -44,37 +47,62 @@ public IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName)
                 yield break;
             }
 
+            if (!IsValidManifest(ast, fileName))
+            {
+                yield break;
+            }
+
             String[] manifestFields = {"FunctionsToExport", "CmdletsToExport", "VariablesToExport", "AliasesToExport"};
             var hashtableAst = ast.Find(x => x is HashtableAst, false) as HashtableAst;
-
+            
             if (hashtableAst == null)
-            {
-                //Should we emit a warning if the parser cannot find a hashtable?
+            {                                
                 yield break;
             }
 
             foreach(String field in manifestFields)
             {
                 IScriptExtent extent;
-                if (!HasAcceptableExportField(field, hashtableAst, out extent) && extent != null)
+                if (!HasAcceptableExportField(field, hashtableAst, ast.Extent.Text, out extent) && extent != null)
                 {
                     yield return new DiagnosticRecord(GetError(field), extent, GetName(), DiagnosticSeverity.Warning, fileName);
                 }
             }                               
 
         }
 
-        private bool HasAcceptableExportField(string key, HashtableAst hast, out IScriptExtent extent)
+        /// <summary>
+        /// Checks if the manifest file is valid. 
+        /// </summary>
+        /// <param name="ast"></param>
+        /// <param name="fileName"></param>
+        /// <returns>A boolean value indicating the validity of the manifest file.</returns>
+        private bool IsValidManifest(Ast ast, string fileName)
+        {
+            var missingManifestRule = new MissingModuleManifestField();
+            return !missingManifestRule.AnalyzeScript(ast, fileName).GetEnumerator().MoveNext();
+                    
+        }
+                
+        /// <summary>
+        /// Checks if the *ToExport fields are explicitly set to lists, @(...) 
+        /// </summary>
+        /// <param name="key"></param>
+        /// <param name="hast"></param>
+        /// <param name="scriptText"></param>
+        /// <param name="extent"></param>
+        /// <returns>A boolean value indicating if the the ToExport fields are explicitly set to lists or not.</returns>
+        private bool HasAcceptableExportField(string key, HashtableAst hast, string scriptText, out IScriptExtent extent)
         {
             extent = null;
             foreach (var pair in hast.KeyValuePairs)
             {
                 if (key.Equals(pair.Item1.Extent.Text.Trim(), StringComparison.OrdinalIgnoreCase))
                 {
-                    var arrayAst = pair.Item2.Find(x => x is ArrayLiteralAst, true) as ArrayLiteralAst;
+                    var arrayAst = pair.Item2.Find(x => x is ArrayLiteralAst || x is ArrayExpressionAst, true);
                     if (arrayAst == null)
                     {
-                        extent = GetScriptExtent(pair);
+                        extent = GetScriptExtent(pair, scriptText);
                         return false;
                     }
                     else
@@ -85,23 +113,31 @@ private bool HasAcceptableExportField(string key, HashtableAst hast, out IScript
             }
             return true;
         }
-
-
-        private ScriptExtent GetScriptExtent(Tuple<ExpressionAst, StatementAst> pair)
+        
+        /// <summary>
+        /// Gets the script extent. 
+        /// </summary>
+        /// <param name="pair"></param>
+        /// <param name="scriptText"></param>
+        /// <returns></returns>
+        private ScriptExtent GetScriptExtent(Tuple<ExpressionAst, StatementAst> pair, string scriptText)
         {
-            return new ScriptExtent(new ScriptPosition(pair.Item1.Extent.StartScriptPosition.File,
-                                                                        pair.Item1.Extent.StartScriptPosition.LineNumber,
-                                                                        pair.Item1.Extent.StartScriptPosition.Offset,
-                                                                        pair.Item1.Extent.StartScriptPosition.Line),
-                                                  new ScriptPosition(pair.Item2.Extent.EndScriptPosition.File,
-                                                                        pair.Item2.Extent.EndScriptPosition.LineNumber,
-                                                                        pair.Item2.Extent.EndScriptPosition.Offset,
-                                                                        pair.Item2.Extent.EndScriptPosition.Line));
+            string[] scriptLines = Regex.Split(scriptText, "\r\n|\r|\n");
+            return new ScriptExtent(new ScriptPosition(pair.Item1.Extent.File,
+                                                            pair.Item1.Extent.StartLineNumber,
+                                                            pair.Item1.Extent.StartColumnNumber,
+                                                            scriptLines[pair.Item1.Extent.StartLineNumber - 1]), //line number begins with 1
+                                        new ScriptPosition(pair.Item2.Extent.File,
+                                                            pair.Item2.Extent.EndLineNumber,
+                                                            pair.Item2.Extent.EndColumnNumber,
+                                                            scriptLines[pair.Item2.Extent.EndLineNumber - 1]));   //line number begins with 1
+
+            
         }
 
         public string GetError(string field)
         {
-            return string.Format(CultureInfo.CurrentCulture, Strings.UseManifestExportFieldsError, field);
+            return string.Format(CultureInfo.CurrentCulture, Strings.UseToExportFieldsInManifestError, field);
         }
 
         /// <summary>
@@ -110,7 +146,7 @@ public string GetError(string field)
         /// <returns>The name of this rule</returns>
         public string GetName()
         {
-            return string.Format(CultureInfo.CurrentCulture, Strings.NameSpaceFormat, GetSourceName(), Strings.UseManifestExportFieldsName);
+            return string.Format(CultureInfo.CurrentCulture, Strings.NameSpaceFormat, GetSourceName(), Strings.UseToExportFieldsInManifestName);
         }
 
         /// <summary>
@@ -119,7 +155,7 @@ public string GetName()
         /// <returns>The common name of this rule</returns>
         public string GetCommonName()
         {
-            return String.Format(CultureInfo.CurrentCulture, Strings.UseManifestExportFieldsCommonName);
+            return String.Format(CultureInfo.CurrentCulture, Strings.UseToExportFieldsInManifestCommonName);
         }
 
         /// <summary>
@@ -128,7 +164,7 @@ public string GetCommonName()
         /// <returns>The description of this rule</returns>
         public string GetDescription()
         {
-            return String.Format(CultureInfo.CurrentCulture, Strings.UseManifestExportFieldsDescription);
+            return String.Format(CultureInfo.CurrentCulture, Strings.UseToExportFieldsInManifestDescription);
         }
 
         /// <summary>

Rules/UseManifestExportFields.cs renamed to Rules/UseToExportFieldsInManifest.cs

@@ -56,7 +56,7 @@ Describe "Test Name parameters" {
 
         It "Get Rules with no parameters supplied" {
 			$defaultRules = Get-ScriptAnalyzerRule
-			$defaultRules.Count | Should be 40
+			$defaultRules.Count | Should be 41
 		}
     }