Merge pull request #9 from PowerShell/dscresource-quoc

quoctruong · quoctruong · commit c94632c4961f · 2015-04-06T14:15:27.000-07:00
Implement Return Correct Types For DSC Functions rule
diff --git a/Engine/Helper.cs b/Engine/Helper.cs
diff --git a/Engine/VariableAnalysisBase.cs b/Engine/VariableAnalysisBase.cs
@@ -1205,7 +1205,7 @@ public AssignmentTarget(AssignmentStatementAst ast)
                     _rightHandSideVariable = new VariableTarget(cmExAst.Expression as VariableExpressionAst);
                     if (String.Equals((exprAst as VariableExpressionAst).VariablePath.UserPath, "this", StringComparison.OrdinalIgnoreCase))
                     {
-
+                        Constant = SpecialVars.ThisVariable;
                     }
                 }
                 //Store the type info for variable assignment from .Net type
diff --git a/Rules/ReturnCorrectTypesForDSCFunctions.cs b/Rules/ReturnCorrectTypesForDSCFunctions.cs
@@ -30,8 +30,55 @@ public class ReturnCorrectTypesForDSCFunctions : IDSCResourceRule
         /// <returns>The results of the analysis</returns>
         public IEnumerable<DiagnosticRecord> AnalyzeDSCResource(Ast ast, string fileName)
         {
+            if (ast == null) throw new ArgumentNullException(Strings.NullAstErrorMessage);
+
             // TODO: Add logic for DSC Resources
-            return Enumerable.Empty<DiagnosticRecord>();
+
+            IEnumerable<Ast> functionDefinitionAsts = Helper.Instance.DscResourceFunctions(ast);
+
+            IEnumerable<TypeDefinitionAst> classes = ast.FindAll(item =>
+                item is TypeDefinitionAst
+                && ((item as TypeDefinitionAst).IsClass), true).Cast<TypeDefinitionAst>();
+
+            foreach (FunctionDefinitionAst func in functionDefinitionAsts)
+            {
+                Helper.Instance.InitializeVariableAnalysis(func);
+                List<Tuple<string, StatementAst>> outputTypes = FindPipelineOutput.OutputTypes(func, classes);
+
+                if (String.Equals(func.Name, "Set-TargetResource", StringComparison.OrdinalIgnoreCase))
+                {
+                    foreach (Tuple<string, StatementAst> outputType in outputTypes)
+                    {
+                        yield return new DiagnosticRecord(string.Format(CultureInfo.CurrentCulture, Strings.ReturnCorrectTypesForSetTargetResourceFunctionsDSCError),
+                            outputType.Item2.Extent, GetName(), DiagnosticSeverity.Strict, fileName);
+                    }
+                }
+                else
+                {
+                    Dictionary<string, string> returnTypes = new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase);
+                    returnTypes["Get-TargetResource"] = typeof(System.Collections.Hashtable).FullName;
+                    returnTypes["Test-TargetResource"] = typeof(bool).FullName;
+
+                    foreach (Tuple<string, StatementAst> outputType in outputTypes)
+                    {
+                        string type = outputType.Item1;
+
+                        if (String.IsNullOrEmpty(type)
+                            || String.Equals(typeof(Unreached).FullName, type, StringComparison.OrdinalIgnoreCase)
+                            || String.Equals(typeof(Undetermined).FullName, type, StringComparison.OrdinalIgnoreCase)
+                            || String.Equals(typeof(object).FullName, type, StringComparison.OrdinalIgnoreCase)
+                            || String.Equals(type, returnTypes[func.Name], StringComparison.OrdinalIgnoreCase))
+                        {
+                            continue;
+                        }
+                        else
+                        {
+                            yield return new DiagnosticRecord(string.Format(CultureInfo.CurrentCulture, Strings.ReturnCorrectTypesForGetTestTargetResourceFunctionsDSCResourceError,
+                                func.Name, returnTypes[func.Name], type), outputType.Item2.Extent, GetName(), DiagnosticSeverity.Strict, fileName);
+                        }
+                    }
+                }
+            }
         }
 
         /// <summary>
@@ -44,7 +91,6 @@ public IEnumerable<DiagnosticRecord> AnalyzeDSCClass(Ast ast, string fileName)
         {
             if (ast == null) throw new ArgumentNullException(Strings.NullAstErrorMessage);
 
-
             IEnumerable<TypeDefinitionAst> classes = ast.FindAll(item =>
                 item is TypeDefinitionAst
                 && ((item as TypeDefinitionAst).IsClass), true).Cast<TypeDefinitionAst>();
@@ -56,8 +102,8 @@ item is TypeDefinitionAst
             foreach (TypeDefinitionAst dscClass in dscClasses)
             {
                 Dictionary<string, string> returnTypes = new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase);
-                returnTypes["Get"] = dscClass.Name;
                 returnTypes["Test"] = typeof(bool).FullName;
+                returnTypes["Get"] = dscClass.Name;
 
                 foreach (var member in dscClass.Members)
                 {
@@ -103,7 +149,7 @@ item is TypeDefinitionAst
                                     ret.Extent, GetName(), DiagnosticSeverity.Strict, fileName);
                             }
 
-                            string typeName = Helper.Instance.GetTypeFromReturnStatementAst(funcAst, ret, classes, ast);
+                            string typeName = Helper.Instance.GetTypeFromReturnStatementAst(funcAst, ret, classes);
 
                             // This also includes the case of return $this because the type of this is unreached.
                             if (String.IsNullOrEmpty(typeName)
diff --git a/Rules/Strings.Designer.cs b/Rules/Strings.Designer.cs
diff --git a/Rules/Strings.resx b/Rules/Strings.resx
@@ -648,4 +648,10 @@
   <data name="ReturnCorrectTypesForSetFunctionsDSCError" xml:space="preserve">
     <value>Set function in DSC Class {0} should not return anything</value>
   </data>
+  <data name="ReturnCorrectTypesForGetTestTargetResourceFunctionsDSCResourceError" xml:space="preserve">
+    <value>{0} function in DSC Resource should return object of type {1} instead of {2}</value>
+  </data>
+  <data name="ReturnCorrectTypesForSetTargetResourceFunctionsDSCError" xml:space="preserve">
+    <value>Set-TargetResource function in DSC Resource should not output anything to the pipeline.</value>
+  </data>
 </root>
diff --git a/Rules/UseIdenticalMandatoryParametersDSC.cs b/Rules/UseIdenticalMandatoryParametersDSC.cs
@@ -26,9 +26,9 @@ public IEnumerable<DiagnosticRecord> AnalyzeDSCResource(Ast ast, string fileName
             if (ast == null) throw new ArgumentNullException(Strings.NullAstErrorMessage);
 
             // Expected TargetResource functions in the DSC Resource module
-            List<string> expectedTargetResourceFunctionNames = new List<string>(new string[] { "Set-TargetResource", "Test-TargetResource", "Get-TargetResource" });                       
-            
-            IEnumerable<Ast> functionDefinitionAsts = ast.FindAll(item => item is FunctionDefinitionAst && expectedTargetResourceFunctionNames.Contains((item as FunctionDefinitionAst).Name, StringComparer.OrdinalIgnoreCase), true);
+            List<string> expectedTargetResourceFunctionNames = new List<string>(new string[] { "Set-TargetResource", "Test-TargetResource", "Get-TargetResource" });
+
+            IEnumerable<Ast> functionDefinitionAsts = Helper.Instance.DscResourceFunctions(ast);
 
             // Dictionary to keep track of Mandatory parameters and their presence in Get/Test/Set TargetResource cmdlets
             Dictionary<string, List<string>> mandatoryParameters = new Dictionary<string, List<string>>(StringComparer.OrdinalIgnoreCase);
diff --git a/Rules/UseIdenticalParametersDSC.cs b/Rules/UseIdenticalParametersDSC.cs
@@ -32,14 +32,10 @@ public IEnumerable<DiagnosticRecord> AnalyzeDSCResource(Ast ast, string fileName
         {
             if (ast == null) throw new ArgumentNullException(Strings.NullAstErrorMessage);
 
-            // Expected TargetResource functions in the DSC Resource module
-            List<string> expectedTargetResourceFunctionNames = new List<string>(new string[] { "Set-TargetResource", "Test-TargetResource" });
-
             // parameters
             Dictionary<string, ParameterAst> paramNames = new Dictionary<string, ParameterAst>(StringComparer.OrdinalIgnoreCase);
 
-            IEnumerable<Ast> functionDefinitionAsts = ast.FindAll(item => item is FunctionDefinitionAst
-                && expectedTargetResourceFunctionNames.Contains((item as FunctionDefinitionAst).Name, StringComparer.OrdinalIgnoreCase), true);
+            IEnumerable<Ast> functionDefinitionAsts = Helper.Instance.DscResourceFunctions(ast);
 
             if (functionDefinitionAsts.Count() == 2)
             {
diff --git a/Tests/Rules/DSCResources/BadDscResource/BadDscResource.psm1 b/Tests/Rules/DSCResources/BadDscResource/BadDscResource.psm1
@@ -103,10 +103,14 @@ class FileResource
         {
             return $present
         }
-        else
+        elseif ($true)
         {
             return -not $present
         }
+        else
+        {
+            return 5
+        }
     }
     
 
@@ -119,20 +123,27 @@ class FileResource
     [FileResource] Get()
     {
         $present = $this.TestFilePath($this.Path)        
+
+        $hashtable = @{"3"="4"}
         
         if ($present)
         {
             $file = Get-ChildItem -LiteralPath $this.Path
             $this.CreationTime = $file.CreationTime
             $this.Ensure = [Ensure]::Present
+            return $this.TestFilePath($this.Path)
         }
-        else
+        elseif ($true)
         {
             $this.CreationTime = $null
             $this.Ensure = [Ensure]::Absent
-        }        
+            return $present
+        }
+        else
+        {
+            return $hashtable
+        }
 
-        return $this
     }
 
     <#
diff --git a/Tests/Rules/DSCResources/MSFT_WaitForAll/MSFT_WaitForAll.psm1 b/Tests/Rules/DSCResources/MSFT_WaitForAll/MSFT_WaitForAll.psm1
@@ -94,6 +94,12 @@ function Set-TargetResource
                -RetryCount $RetryCount `
                -ThrottleLimit $ThrottleLimit 
     }
+
+    $true -and $false
+    
+    if ($true) {
+        return 4;
+    }
 }
 
 # 
@@ -118,14 +124,22 @@ function Test-TargetResource
 
     Import-Module $PSScriptRoot\..\..\PSDSCxMachine.psm1
 
-    return PSDSCxMachine\Test-_InternalPSDscXMachineTR `
-             -RemoteResourceId $ResourceName `
-             -RemoteMachine $NodeName `
-             -RemoteCredential $Credential `
-             -MinimalNumberOfMachineInState $NodeName.Count `
-             -RetryIntervalSec $RetryIntervalSec `
-             -RetryCount $RetryCount `
-             -ThrottleLimit $ThrottleLimit
+    $b = @{"Test"=3}
+    $b
+
+    $c = [Math]::Sin(3)
+    $c
+
+    $d = [bool[]]@($true, $false)
+    
+    $d
+
+    foreach ($d in $b)
+    {
+         $test
+    }
+
+    return $true
 }
 
 
diff --git a/Tests/Rules/DSCResources/MSFT_WaitForAny/MSFT_WaitForAny.psm1 b/Tests/Rules/DSCResources/MSFT_WaitForAny/MSFT_WaitForAny.psm1
@@ -30,15 +30,22 @@ function Get-TargetResource
     )
 
     Import-Module $PSScriptRoot\..\..\PSDSCxMachine.psm1
+    
+    $b = @{"hash" = "table"}
 
-    return PSDSCxMachine\Get-_InternalPSDscXMachineTR `
-               -RemoteResourceId $ResourceName `
-               -RemoteMachine $NodeName `
-               -RemoteCredential $Credential `
-               -MinimalNumberOfMachineInState 1 `
-               -RetryIntervalSec $RetryIntervalSec `
-               -RetryCount $RetryCount `
-               -ThrottleLimit $ThrottleLimit
+    if ($true)
+    {
+        return $b;
+    }
+    elseif ($c)
+    {
+        return @{"hash2"="table2"}
+    }
+    else
+    {
+        # can't determine type of c so error should not be raised as we're trying to be conservative
+        return $c;
+    }
 }
 
 #
@@ -125,14 +132,25 @@ function Test-TargetResource
 
     Import-Module $PSScriptRoot\..\..\PSDSCxMachine.psm1
 
-    return PSDSCxMachine\Test-_InternalPSDscXMachineTR `
-               -RemoteResourceId $ResourceName `
-               -RemoteMachine $NodeName `
-               -RemoteCredential $Credential `
-               -MinimalNumberOfMachineInState 1 `
-               -RetryIntervalSec $RetryIntervalSec `
-               -RetryCount $RetryCount `
-               -ThrottleLimit $ThrottleLimit
+    $a = $true
+    $a
+
+    if ($true)
+    {
+        $false;
+    }
+    elseif ($b)
+    {
+        return $a -or $true
+    }
+    elseif ($c)
+    {
+        return $false;
+    }
+    else
+    {
+        return $true
+    }
 }
 
 
diff --git a/Tests/Rules/ReturnCorrectTypesForDSCFunctions.ps1 b/Tests/Rules/ReturnCorrectTypesForDSCFunctions.ps1
@@ -0,0 +1,46 @@
+﻿Import-Module -Verbose ScriptAnalyzer
+
+$violationMessageDSCResource = "Test-TargetResource function in DSC Resource should return object of type System.Boolean instead of System.Collections.Hashtable"
+$violationMessageDSCClass = "Test function in DSC Class FileResource should return object of type System.Boolean instead of type System.Int32"
+$violationName = "PSDSCReturnCorrectTypesForDSCFunctions"
+$directory = Split-Path -Parent $MyInvocation.MyCommand.Path
+$violations = Invoke-ScriptAnalyzer $directory\DSCResources\MSFT_WaitForAll\MSFT_WaitForAll.psm1 | Where-Object {$_.RuleName -eq $violationName}
+$noViolations = Invoke-ScriptAnalyzer $directory\DSCResources\MSFT_WaitForAny\MSFT_WaitForAny.psm1 | Where-Object {$_.RuleName -eq $violationName}
+$classViolations = Invoke-ScriptAnalyzer -ErrorAction SilentlyContinue $directory\DSCResources\BadDscResource\BadDscResource.psm1 | Where-Object {$_.RuleName -eq $violationName}
+$noClassViolations = Invoke-ScriptAnalyzer -ErrorAction SilentlyContinue $directory\DSCResources\MyDscResource\MyDscResource.psm1 | Where-Object {$_.RuleName -eq $violationName}
+
+Describe "ReturnCorrectTypesForDSCFunctions" {
+    Context "When there are violations" {
+        It "has 5 return correct types for DSC functions violations" {
+            $violations.Count | Should Be 5
+        }
+
+        It "has the correct description message" {
+            $violations[2].Message | Should Match $violationMessageDSCResource
+        }
+    }
+
+    Context "When there are no violations" {
+        It "returns no violations" {
+            $noViolations.Count | Should Be 0
+        }
+    }
+}
+
+Describe "StandardDSCFunctionsInClass" {
+    Context "When there are violations" {
+        It "has 4 return correct types for DSC functions violations" {
+            $classViolations.Count | Should Be 4
+        }
+
+        It "has the correct description message" {
+            $classViolations[0].Message | Should Match $violationMessageDSCClass
+        }
+    }
+
+    Context "When there are no violations" {
+        It "returns no violations" {
+            $noClassViolations.Count | Should Be 0
+        }
+    }
+}
diff --git a/Tests/Rules/UseDSCResourceFunctions.tests.ps1 b/Tests/Rules/UseDSCResourceFunctions.tests.ps1

Original file line number	Diff line number	Diff line change
`@@ -1205,7 +1205,7 @@ public AssignmentTarget(AssignmentStatementAst ast)`
`1205`	`1205`	`_rightHandSideVariable = new VariableTarget(cmExAst.Expression as VariableExpressionAst);`
`1206`	`1206`	`if (String.Equals((exprAst as VariableExpressionAst).VariablePath.UserPath, "this", StringComparison.OrdinalIgnoreCase))`
`1207`	`1207`	`{`
`1208`		`-`
	`1208`	`+ Constant = SpecialVars.ThisVariable;`
`1209`	`1209`	`}`
`1210`	`1210`	`}`
`1211`	`1211`	`//Store the type info for variable assignment from .Net type`
Original file line number	Diff line number	Diff line change
`@@ -32,14 +32,10 @@ public IEnumerable<DiagnosticRecord> AnalyzeDSCResource(Ast ast, string fileName`
`32`	`32`	`{`
`33`	`33`	`if (ast == null) throw new ArgumentNullException(Strings.NullAstErrorMessage);`
`34`	`34`
`35`		`- // Expected TargetResource functions in the DSC Resource module`
`36`		`- List<string> expectedTargetResourceFunctionNames = new List<string>(new string[] { "Set-TargetResource", "Test-TargetResource" });`
`37`		`-`
`38`	`35`	`// parameters`
`39`	`36`	`Dictionary<string, ParameterAst> paramNames = new Dictionary<string, ParameterAst>(StringComparer.OrdinalIgnoreCase);`
`40`	`37`
`41`		`- IEnumerable<Ast> functionDefinitionAsts = ast.FindAll(item => item is FunctionDefinitionAst`
`42`		`- && expectedTargetResourceFunctionNames.Contains((item as FunctionDefinitionAst).Name, StringComparer.OrdinalIgnoreCase), true);`
	`38`	`+ IEnumerable<Ast> functionDefinitionAsts = Helper.Instance.DscResourceFunctions(ast);`
`43`	`39`
`44`	`40`	`if (functionDefinitionAsts.Count() == 2)`
`45`	`41`	`{`
Original file line number	Diff line number	Diff line change
`@@ -103,10 +103,14 @@ class FileResource`
`103`	`103`	`{`
`104`	`104`	`return $present`
`105`	`105`	`}`
`106`		`- else`
	`106`	`+ elseif ($true)`
`107`	`107`	`{`
`108`	`108`	`return -not $present`
`109`	`109`	`}`
	`110`	`+ else`
	`111`	`+ {`
	`112`	`+ return 5`
	`113`	`+ }`
`110`	`114`	`}`
`111`	`115`
`112`	`116`
`@@ -119,20 +123,27 @@ class FileResource`
`119`	`123`	`[FileResource] Get()`
`120`	`124`	`{`
`121`	`125`	`$present = $this.TestFilePath($this.Path)`
	`126`	`+`
	`127`	`+ $hashtable = @{"3"="4"}`
`122`	`128`
`123`	`129`	`if ($present)`
`124`	`130`	`{`
`125`	`131`	`$file = Get-ChildItem -LiteralPath $this.Path`
`126`	`132`	`$this.CreationTime = $file.CreationTime`
`127`	`133`	`$this.Ensure = [Ensure]::Present`
	`134`	`+ return $this.TestFilePath($this.Path)`
`128`	`135`	`}`
`129`		`- else`
	`136`	`+ elseif ($true)`
`130`	`137`	`{`
`131`	`138`	`$this.CreationTime = $null`
`132`	`139`	`$this.Ensure = [Ensure]::Absent`
`133`		`- }`
	`140`	`+ return $present`
	`141`	`+ }`
	`142`	`+ else`
	`143`	`+ {`
	`144`	`+ return $hashtable`
	`145`	`+ }`
`134`	`146`
`135`		`- return $this`
`136`	`147`	`}`
`137`	`148`
`138`	`149`	`<#`