Implement UseOutputTypeCorrectly

Engine/Helper.cs

@@ -2085,6 +2085,11 @@ public object VisitConvertExpression(ConvertExpressionAst convAst)
         {
             if (convAst != null)
             {
+                if (convAst.Type.TypeName.GetReflectionType() != null)
+                {
+                    return convAst.Type.TypeName.GetReflectionType().FullName;
+                }
+
                 return convAst.Type.TypeName.FullName;
             }
 
@@ -2135,6 +2140,11 @@ public object VisitTypeConstraint(TypeConstraintAst typeAst)
         {
             if (typeAst != null)
             {
+                if (typeAst.TypeName.GetReflectionType() != null)
+                {
+                    return typeAst.TypeName.GetReflectionType().FullName;
+                }
+
                 return typeAst.TypeName.FullName;
             }
 
@@ -2160,6 +2170,11 @@ public object VisitTypeExpression(TypeExpressionAst typeExpressionAst)
         {
             if (typeExpressionAst != null)
             {
+                if (typeExpressionAst.TypeName.GetReflectionType() != null)
+                {
+                    return typeExpressionAst.TypeName.GetReflectionType().FullName;
+                }
+
                 return typeExpressionAst.TypeName.FullName;
             }
 

Rules/ScriptAnalyzerBuiltinRules.csproj

@@ -68,6 +68,7 @@
     <Compile Include="AvoidUsingPlainTextForPassword.cs" />
     <Compile Include="AvoidUsingWMIObjectCmdlet.cs" />
     <Compile Include="AvoidUsingWriteHost.cs" />
+    <Compile Include="UseOutputTypeCorrectly.cs" />
     <Compile Include="MissingModuleManifestField.cs" />
     <Compile Include="PossibleIncorrectComparisonWithNull.cs" />
     <Compile Include="ProvideCommentHelp.cs" />

Rules/Strings.resx

@@ -678,4 +678,16 @@
   <data name="AvoidUsingWMIObjectCmdletName" xml:space="preserve">
     <value>AvoidUsingWMIObjectCmdlet</value>
   </data>
+  <data name="UseOutputTypeCorrectlyCommonName" xml:space="preserve">
+    <value>Use OutputType Correctly</value>
+  </data>
+  <data name="UseOutputTypeCorrectlyDescription" xml:space="preserve">
+    <value>The return types of a cmdlet should be declared using the OutputType attribute.</value>
+  </data>
+  <data name="UseOutputTypeCorrectlyError" xml:space="preserve">
+    <value>The cmdlet '{0}' returns an object of type '{1}' but this type is not declared in the OutputType attribute.</value>
+  </data>
+  <data name="UseOutputTypeCorrectlyName" xml:space="preserve">
+    <value>UseOutputTypeCorrectly</value>
+  </data>
 </root>

Rules/UseOutputTypeCorrectly.cs

@@ -0,0 +1,179 @@
+//
+// Copyright (c) Microsoft Corporation.
+//
+// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+// THE SOFTWARE.
+//
+
+using System;
+using System.Collections.Generic;
+using System.Linq;
+using System.Management.Automation.Language;
+using Microsoft.Windows.Powershell.ScriptAnalyzer.Generic;
+using System.ComponentModel.Composition;
+using System.Globalization;
+using System.Management.Automation;
+
+namespace Microsoft.Windows.Powershell.ScriptAnalyzer.BuiltinRules
+{
+    /// <summary>
+    /// ProvideCommentHelp: Analyzes ast to check that cmdlets have help.
+    /// </summary>
+    [Export(typeof(IScriptRule))]
+    public class UseOutputTypeCorrectly : SkipTypeDefinition, IScriptRule
+    {
+        private IEnumerable<TypeDefinitionAst> _classes;
+
+        /// <summary>
+        /// AnalyzeScript: Analyzes the ast to check that cmdlets have help.
+        /// </summary>
+        /// <param name="ast">The script's ast</param>
+        /// <param name="fileName">The name of the script</param>
+        /// <returns>A List of diagnostic results of this rule</returns>
+        public IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName)
+        {
+            if (ast == null) throw new ArgumentNullException(Strings.NullAstErrorMessage);
+
+            DiagnosticRecords.Clear();
+            this.fileName = fileName;
+
+            _classes = ast.FindAll(item => item is TypeDefinitionAst && ((item as TypeDefinitionAst).IsClass), true).Cast<TypeDefinitionAst>();
+
+            ast.Visit(this);
+
+            return DiagnosticRecords;
+        }
+
+        /// <summary>
+        /// Visit function and checks that it is a cmdlet. If yes, then checks that any object returns must have a type declared
+        /// in the output type (the only exception is if the type is object)
+        /// </summary>
+        /// <param name="funcAst"></param>
+        /// <returns></returns>
+        public override AstVisitAction VisitFunctionDefinition(FunctionDefinitionAst funcAst)
+        {
+            if (funcAst == null || funcAst.Body == null || funcAst.Body.ParamBlock == null
+                || funcAst.Body.ParamBlock.Attributes == null || funcAst.Body.ParamBlock.Attributes.Count == 0
+                || !funcAst.Body.ParamBlock.Attributes.Any(attr => attr.TypeName.GetReflectionType() == typeof(CmdletBindingAttribute)))
+            {
+                return AstVisitAction.Continue;
+            }
+
+            HashSet<string> outputTypes = new HashSet<string>();
+
+            foreach (AttributeAst attrAst in funcAst.Body.ParamBlock.Attributes)
+            {
+                if (attrAst.TypeName != null && attrAst.TypeName.GetReflectionType() == typeof(OutputTypeAttribute)
+                    && attrAst.PositionalArguments != null)
+                {
+                    foreach (ExpressionAst expAst in attrAst.PositionalArguments)
+                    {
+                        if (expAst is StringConstantExpressionAst)
+                        {
+                            Type type = Type.GetType((expAst as StringConstantExpressionAst).Value);
+                            if (type != null)
+                            {
+                                outputTypes.Add(type.FullName);
+                            }
+                        }
+                        else
+                        {
+                            TypeExpressionAst typeAst = expAst as TypeExpressionAst;
+                            if (typeAst != null && typeAst.TypeName != null)
+                            {
+                                if (typeAst.TypeName.GetReflectionType() != null)
+                                {
+                                    outputTypes.Add(typeAst.TypeName.GetReflectionType().FullName);
+                                }
+                                else
+                                {
+                                    outputTypes.Add(typeAst.TypeName.FullName);
+                                }
+                            }
+                        }
+                    }
+                }
+            }
+
+            List<Tuple<string, StatementAst>> returnTypes = FindPipelineOutput.OutputTypes(funcAst, _classes);
+
+            foreach (Tuple<string, StatementAst> returnType in returnTypes)
+            {
+                string typeName = returnType.Item1;
+
+                if (String.IsNullOrEmpty(typeName)
+                    || String.Equals(typeof(Unreached).FullName, typeName, StringComparison.OrdinalIgnoreCase)
+                    || String.Equals(typeof(Undetermined).FullName, typeName, StringComparison.OrdinalIgnoreCase)
+                    || String.Equals(typeof(object).FullName, typeName, StringComparison.OrdinalIgnoreCase)
+                    || outputTypes.Contains(typeName, StringComparer.OrdinalIgnoreCase))
+                {
+                    continue;
+                }
+                else
+                {
+                    DiagnosticRecords.Add(new DiagnosticRecord(string.Format(CultureInfo.CurrentCulture, Strings.UseOutputTypeCorrectlyError,
+                        funcAst.Name, typeName), returnType.Item2.Extent, GetName(), DiagnosticSeverity.Warning, fileName));
+                }
+            }
+
+            return AstVisitAction.Continue;
+        }
+
+        /// <summary>
+        /// GetName: Retrieves the name of this rule.
+        /// </summary>
+        /// <returns>The name of this rule</returns>
+        public string GetName()
+        {
+            return string.Format(CultureInfo.CurrentCulture, Strings.NameSpaceFormat, GetSourceName(), Strings.UseOutputTypeCorrectlyName);
+        }
+
+        /// <summary>
+        /// GetCommonName: Retrieves the common name of this rule.
+        /// </summary>
+        /// <returns>The common name of this rule</returns>
+        public string GetCommonName()
+        {
+            return string.Format(CultureInfo.CurrentCulture, Strings.UseOutputTypeCorrectlyCommonName);
+        }
+
+        /// <summary>
+        /// GetDescription: Retrieves the description of this rule.
+        /// </summary>
+        /// <returns>The description of this rule</returns>
+        public string GetDescription()
+        {
+            return string.Format(CultureInfo.CurrentCulture, Strings.UseOutputTypeCorrectlyDescription);
+        }
+
+        /// <summary>
+        /// Method: Retrieves the type of the rule: builtin, managed or module.
+        /// </summary>
+        public SourceType GetSourceType()
+        {
+            return SourceType.Builtin;
+        }
+
+        /// <summary>
+        /// GetSeverity: Retrieves the severity of the rule: error, warning of information.
+        /// </summary>
+        /// <returns></returns>
+        public RuleSeverity GetSeverity()
+        {
+            return RuleSeverity.Information;
+        }
+
+        /// <summary>
+        /// Method: Retrieves the module/assembly name the rule is from.
+        /// </summary>
+        public string GetSourceName()
+        {
+            return string.Format(CultureInfo.CurrentCulture, Strings.SourceName);
+        }
+    }
+}

Tests/Rules/BadCmdlet.ps1

@@ -7,6 +7,8 @@
                   ConfirmImpact='Medium')]
     [Alias()]
     [OutputType([String])]
+    [OutputType("System.Int32", ParameterSetName="ID")]
+
     Param
     (
         # Param1 help description
@@ -46,13 +48,22 @@
     }
     Process
     {
+        $a = 4.5
+
+        if ($true)
+        {
+            $a
+        }
+
+        return @{"hash"="true"}
     }
     End
     {
     }
 }
 
 # Provide comment help should not be raised here because this is not exported
+#Output type rule should also not be raised here as this is not a cmdlet
 function NoComment
 {
     Write-Verbose "No Comment"

Tests/Rules/GoodCmdlet.ps1

@@ -28,7 +28,9 @@ function Get-File
                   HelpUri = 'http://www.microsoft.com/',
                   ConfirmImpact='Medium')]
     [Alias()]
-    [OutputType([String])]
+    [OutputType([String], [System.Double], [Hashtable])]
+    [OutputType("System.Int32", ParameterSetName="ID")]
+
     Param
     (
         # Param1 help description
@@ -75,6 +77,15 @@ function Get-File
             Write-Verbose "Write Verbose"
             Get-Process
         }
+
+        $a = 4.5
+
+        if ($true)
+        {
+            $a
+        }
+
+        return @{"hash"="true"}
     }
     End
     {

Tests/Rules/UseOutputTypeCorrectly.tests.ps1

@@ -0,0 +1,29 @@
+Import-Module PSScriptAnalyzer
+$violationMessage = "The cmdlet 'Verb-Files' returns an object of type 'System.Double' but this type is not declared in the OutputType attribute."
+$violationName = "PSUseOutputTypeCorrectly"
+$directory = Split-Path -Parent $MyInvocation.MyCommand.Path
+$violations = Invoke-ScriptAnalyzer $directory\BadCmdlet.ps1 | Where-Object {$_.RuleName -eq $violationName}
+$dscViolations = Invoke-ScriptAnalyzer -ErrorAction SilentlyContinue $directory\DSCResources\MyDscResource\MyDscResource.psm1 | Where-Object {$_.RuleName -eq $violationName}
+$noViolations = Invoke-ScriptAnalyzer $directory\GoodCmdlet.ps1 | Where-Object {$_.RuleName -eq $violationName}
+
+Describe "UseOutputTypeCorrectly" {
+    Context "When there are violations" {
+        It "has 2 Use OutputType Correctly violations" {
+            $violations.Count | Should Be 2
+        }
+
+        It "has the correct description message" {
+            $violations[0].Message | Should Match $violationMessage
+        }
+
+        It "Does not count violation in DSC class" {
+            $dscViolations.Count | Should Be 0
+        }
+    }
+
+    Context "When there are no violations" {
+        It "returns no violations" {
+            $noViolations.Count | Should Be 0
+        }
+    }
+}