diff --git a/Engine/Helper.cs b/Engine/Helper.cs index 0e59ad803..16304ae86 100644 --- a/Engine/Helper.cs +++ b/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; } diff --git a/RuleDocumentation/UseOutputTypeCorrectly.md b/RuleDocumentation/UseOutputTypeCorrectly.md new file mode 100644 index 000000000..577f430cb --- /dev/null +++ b/RuleDocumentation/UseOutputTypeCorrectly.md @@ -0,0 +1,37 @@ +#UseOutputTypeCorrectly +**Severity Level: Information** + + +##Description + +If a return type is declared, the cmdlet must return that type. If a type is returned, a return type must be declared. + +##How to Fix + +To fix a violation of this rule, please check the OuputType attribute lists and the types that are returned in your code. You can get more details by running “Get-Help about_Functions_OutputTypeAttribute” command in Windows PowerShell. + +##Example + +##Example +Wrong: + + function Get-Foo + { + [CmdletBinding()] + [OutputType([String])] + Param( + ) + return "4 + } + +Correct: + + function Get-Foo + { + [CmdletBinding()] + [OutputType([String])] + Param( + ) + + return "bar" + } diff --git a/Rules/ScriptAnalyzerBuiltinRules.csproj b/Rules/ScriptAnalyzerBuiltinRules.csproj index e85c892d0..eef54841b 100644 --- a/Rules/ScriptAnalyzerBuiltinRules.csproj +++ b/Rules/ScriptAnalyzerBuiltinRules.csproj @@ -68,6 +68,7 @@ + diff --git a/Rules/Strings.Designer.cs b/Rules/Strings.Designer.cs index 1ec732308..3f1eb4469 100644 --- a/Rules/Strings.Designer.cs +++ b/Rules/Strings.Designer.cs @@ -1545,6 +1545,42 @@ internal static string UseIdenticalParametersDSCName { } } + /// + /// Looks up a localized string similar to Use OutputType Correctly. + /// + internal static string UseOutputTypeCorrectlyCommonName { + get { + return ResourceManager.GetString("UseOutputTypeCorrectlyCommonName", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to The return types of a cmdlet should be declared using the OutputType attribute.. + /// + internal static string UseOutputTypeCorrectlyDescription { + get { + return ResourceManager.GetString("UseOutputTypeCorrectlyDescription", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to The cmdlet '{0}' returns an object of type '{1}' but this type is not declared in the OutputType attribute.. + /// + internal static string UseOutputTypeCorrectlyError { + get { + return ResourceManager.GetString("UseOutputTypeCorrectlyError", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to UseOutputTypeCorrectly. + /// + internal static string UseOutputTypeCorrectlyName { + get { + return ResourceManager.GetString("UseOutputTypeCorrectlyName", resourceCulture); + } + } + /// /// Looks up a localized string similar to PSCredential. /// diff --git a/Rules/Strings.resx b/Rules/Strings.resx index f7501a45c..1d548b83a 100644 --- a/Rules/Strings.resx +++ b/Rules/Strings.resx @@ -678,4 +678,16 @@ AvoidUsingWMIObjectCmdlet + + Use OutputType Correctly + + + The return types of a cmdlet should be declared using the OutputType attribute. + + + The cmdlet '{0}' returns an object of type '{1}' but this type is not declared in the OutputType attribute. + + + UseOutputTypeCorrectly + \ No newline at end of file diff --git a/Rules/UseOutputTypeCorrectly.cs b/Rules/UseOutputTypeCorrectly.cs new file mode 100644 index 000000000..f79ccea15 --- /dev/null +++ b/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 +{ + /// + /// ProvideCommentHelp: Checks that objects return in a cmdlet have their types declared in OutputType Attribute + /// + [Export(typeof(IScriptRule))] + public class UseOutputTypeCorrectly : SkipTypeDefinition, IScriptRule + { + private IEnumerable _classes; + + /// + /// AnalyzeScript: Checks that objects return in a cmdlet have their types declared in OutputType Attribute + /// + /// The script's ast + /// The name of the script + /// A List of diagnostic results of this rule + public IEnumerable 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(); + + ast.Visit(this); + + return DiagnosticRecords; + } + + /// + /// 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) + /// + /// + /// + 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 outputTypes = new HashSet(); + + 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> returnTypes = FindPipelineOutput.OutputTypes(funcAst, _classes); + + foreach (Tuple 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.Information, fileName)); + } + } + + return AstVisitAction.Continue; + } + + /// + /// GetName: Retrieves the name of this rule. + /// + /// The name of this rule + public string GetName() + { + return string.Format(CultureInfo.CurrentCulture, Strings.NameSpaceFormat, GetSourceName(), Strings.UseOutputTypeCorrectlyName); + } + + /// + /// GetCommonName: Retrieves the common name of this rule. + /// + /// The common name of this rule + public string GetCommonName() + { + return string.Format(CultureInfo.CurrentCulture, Strings.UseOutputTypeCorrectlyCommonName); + } + + /// + /// GetDescription: Retrieves the description of this rule. + /// + /// The description of this rule + public string GetDescription() + { + return string.Format(CultureInfo.CurrentCulture, Strings.UseOutputTypeCorrectlyDescription); + } + + /// + /// Method: Retrieves the type of the rule: builtin, managed or module. + /// + public SourceType GetSourceType() + { + return SourceType.Builtin; + } + + /// + /// GetSeverity: Retrieves the severity of the rule: error, warning of information. + /// + /// + public RuleSeverity GetSeverity() + { + return RuleSeverity.Information; + } + + /// + /// Method: Retrieves the module/assembly name the rule is from. + /// + public string GetSourceName() + { + return string.Format(CultureInfo.CurrentCulture, Strings.SourceName); + } + } +} diff --git a/Tests/Rules/BadCmdlet.ps1 b/Tests/Rules/BadCmdlet.ps1 index 546dc31a8..2958a0778 100644 --- a/Tests/Rules/BadCmdlet.ps1 +++ b/Tests/Rules/BadCmdlet.ps1 @@ -7,6 +7,8 @@ ConfirmImpact='Medium')] [Alias()] [OutputType([String])] + [OutputType("System.Int32", ParameterSetName="ID")] + Param ( # Param1 help description @@ -46,6 +48,14 @@ } Process { + $a = 4.5 + + if ($true) + { + $a + } + + return @{"hash"="true"} } End { @@ -53,6 +63,7 @@ } # 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" diff --git a/Tests/Rules/GoodCmdlet.ps1 b/Tests/Rules/GoodCmdlet.ps1 index 7c1dfa240..73ea8399c 100644 --- a/Tests/Rules/GoodCmdlet.ps1 +++ b/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 { diff --git a/Tests/Rules/UseOutputTypeCorrectly.tests.ps1 b/Tests/Rules/UseOutputTypeCorrectly.tests.ps1 new file mode 100644 index 000000000..666d281d0 --- /dev/null +++ b/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 + } + } +} \ No newline at end of file