diff --git a/Engine/Settings/CodeFormatting.psd1 b/Engine/Settings/CodeFormatting.psd1 index 89dfd787c..ba5d3a1aa 100644 --- a/Engine/Settings/CodeFormatting.psd1 +++ b/Engine/Settings/CodeFormatting.psd1 @@ -2,8 +2,9 @@ IncludeRules = @( 'PSPlaceOpenBrace', 'PSPlaceCloseBrace', + 'PSUseConsistentWhitespace', 'PSUseConsistentIndentation', - 'PSUseConsistentWhitespace' + 'PSAlignAssignmentStatement' ) Rules = @{ @@ -30,5 +31,10 @@ CheckSeparator = $true } + PSAlignAssignmentStatement = @{ + Enable = $true + CheckHashtable = $true + CheckDSCConfiguration = $true + } } } diff --git a/Engine/TokenOperations.cs b/Engine/TokenOperations.cs index 35d4b6828..e6040e324 100644 --- a/Engine/TokenOperations.cs +++ b/Engine/TokenOperations.cs @@ -272,6 +272,11 @@ public IEnumerable> GetOpenParensWithWhiteSpacesBefore() return GetTokenAndPrecedingWhitespace(TokenKind.LParen); } + public static int GetExtentWidth(IScriptExtent extent) + { + return extent.EndOffset - extent.StartOffset; + } + private bool OnSameLine(Token token1, Token token2) { return token1.Extent.StartLineNumber == token2.Extent.EndLineNumber; diff --git a/RuleDocumentation/AlignAssignmentStatement.md b/RuleDocumentation/AlignAssignmentStatement.md new file mode 100644 index 000000000..1462bd035 --- /dev/null +++ b/RuleDocumentation/AlignAssignmentStatement.md @@ -0,0 +1,48 @@ +# AlignAssignmentStatement + +**Severity Level: Warning** + +## Description + +Consecutive assignment statements are more readable if they are aligned. By aligned, we imply that the `equal` sign for all the assignment statements should be in the same column. + +The rule looks for key (property) value pairs in a hashtable (DSC configuration) to check if they are aligned or not. Consider the following example in which the key value pairs are not aligned. + +```powershell +$hashtable = @{ + property1 = "value" + anotherProperty = "another value" +} +``` + +Alignment in this case would look like the following. + +```powershell +$hashtable = @{ + property1 = "value" + anotherProperty = "another value" +} +``` + +The rule will ignore hashtables in which the assignment statements are on the same line. For example, the rule will ignore `$h = {a = 1; b = 2}`. + +## Configuration + +```powershell + Rules = @{ + PSAlignAssignmentStatement = @{ + Enable = $true + CheckHashtable = $true + } + } +``` + +### Parameters + +#### Enable: bool (Default value is `$false`) + +Enable or disable the rule during ScriptAnalyzer invocation. + +#### CheckHashtable: bool (Default value is `$false`) + +Enforce alignment of assignment statements in a hashtable and in a DSC Configuration. There is only one switch for hasthable and DSC configuration because the property value pairs in a DSC configuration are parsed as key value pairs of a hashtable. diff --git a/Rules/AlignAssignmentStatement.cs b/Rules/AlignAssignmentStatement.cs new file mode 100644 index 000000000..531cb9654 --- /dev/null +++ b/Rules/AlignAssignmentStatement.cs @@ -0,0 +1,271 @@ +// 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; +#if !CORECLR +using System.ComponentModel.Composition; +#endif +using System.Globalization; +using System.Linq; +using System.Management.Automation.Language; +using Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic; + +namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules +{ + /// + /// A class to walk an AST to check if consecutive assignment statements are aligned. + /// +#if !CORECLR + [Export(typeof(IScriptRule))] +#endif + class AlignAssignmentStatement : ConfigurableRule + { + // We keep this switch even though the rule has only one switch (this) as of now, because we want + // to let the rule be expandable in the future to allow formatting assignments even + // in variable assignments. But for now we will stick to only one option. + /// + /// Check if key value pairs in a hashtable are aligned or not. + /// + /// + [ConfigurableRuleProperty(defaultValue: true)] + public bool CheckHashtable { get; set; } + + private readonly char whitespaceChar = ' '; + + private List>> violationFinders + = new List>>(); + + /// + /// Sets the configurable properties of this rule. + /// + /// A dictionary that maps parameter name to it value. Must be non-null + public override void ConfigureRule(IDictionary paramValueMap) + { + base.ConfigureRule(paramValueMap); + if (CheckHashtable) + { + violationFinders.Add(FindHashtableViolations); + } + } + + + /// + /// Analyzes the given ast to find if consecutive assignment statements are aligned. + /// + /// AST to be analyzed. This should be non-null + /// Name of file that corresponds to the input AST. + /// A an enumerable type containing the violations + public override IEnumerable AnalyzeScript(Ast ast, string fileName) + { + if (ast == null) + { + throw new ArgumentNullException("ast"); + } + // only handles one line assignments + // if the rule encounters assignment statements that are multi-line, the rule will ignore that block + + var tokenOps = new TokenOperations(Helper.Instance.Tokens, ast); + foreach (var violationFinder in violationFinders) + { + foreach (var diagnosticRecord in violationFinder(tokenOps)) + { + yield return diagnosticRecord; + } + } + } + + /// + /// Retrieves the common name of this rule. + /// + public override string GetCommonName() + { + return string.Format(CultureInfo.CurrentCulture, Strings.AlignAssignmentStatementCommonName); + } + + /// + /// Retrieves the description of this rule. + /// + public override string GetDescription() + { + return string.Format(CultureInfo.CurrentCulture, Strings.AlignAssignmentStatementDescription); + } + + /// + /// Retrieves the name of this rule. + /// + public override string GetName() + { + return string.Format( + CultureInfo.CurrentCulture, + Strings.NameSpaceFormat, + GetSourceName(), + Strings.AlignAssignmentStatementName); + } + + /// + /// Retrieves the severity of the rule: error, warning or information. + /// + public override RuleSeverity GetSeverity() + { + return RuleSeverity.Warning; + } + + /// + /// Gets the severity of the returned diagnostic record: error, warning, or information. + /// + /// + public DiagnosticSeverity GetDiagnosticSeverity() + { + return DiagnosticSeverity.Warning; + } + + /// + /// Retrieves the name of the module/assembly the rule is from. + /// + public override string GetSourceName() + { + return string.Format(CultureInfo.CurrentCulture, Strings.SourceName); + } + + /// + /// Retrieves the type of the rule, Builtin, Managed or Module. + /// + public override SourceType GetSourceType() + { + return SourceType.Builtin; + } + private IEnumerable FindHashtableViolations(TokenOperations tokenOps) + { + var hashtableAsts = tokenOps.Ast.FindAll(ast => ast is HashtableAst, true); + if (hashtableAsts == null) + { + yield break; + } + + // it is probably much easier have a hashtable writer that formats the hashtable and writes it + // but it makes handling comments hard. So we need to use this approach. + + // This is how the algorithm actually works: + // if each key value pair are on a separate line + // find all the assignment operators + // if all the assignment operators are aligned (check the column number of each assignment operator) + // skip + // else + // find the distance between the assignment operators and their corresponding LHS + // find the longest left expression + // make sure all the assignment operators are in the same column as that of the longest left hand. + + foreach (var astItem in hashtableAsts) + { + var hashtableAst = (HashtableAst)astItem; + if (!HasKeysOnSeparateLines(hashtableAst)) + { + continue; + } + + var extentTuples = GetExtents(tokenOps, hashtableAst); + if (extentTuples == null + || extentTuples.Count == 0 + || !extentTuples.All(t => t.Item1.StartLineNumber == t.Item2.EndLineNumber)) + { + continue; + } + + var widestKeyExtent = extentTuples + .Select(t => t.Item1) + .Aggregate((t1, tAggregate) => { + return TokenOperations.GetExtentWidth(tAggregate) > TokenOperations.GetExtentWidth(t1) + ? tAggregate + : t1; + }); + var expectedStartColumnNumber = widestKeyExtent.EndColumnNumber + 1; + foreach (var extentTuple in extentTuples) + { + if (extentTuple.Item2.StartColumnNumber != expectedStartColumnNumber) + { + yield return new DiagnosticRecord( + GetError(), + extentTuple.Item2, + GetName(), + GetDiagnosticSeverity(), + extentTuple.Item1.File, + null, + GetHashtableCorrections(extentTuple, expectedStartColumnNumber).ToList()); + } + } + } + } + + private IEnumerable GetHashtableCorrections( + Tuple extentTuple, + int expectedStartColumnNumber) + { + var equalExtent = extentTuple.Item2; + var lhsExtent = extentTuple.Item1; + var columnDiff = expectedStartColumnNumber - equalExtent.StartColumnNumber; + yield return new CorrectionExtent( + lhsExtent.EndLineNumber, + equalExtent.StartLineNumber, + lhsExtent.EndColumnNumber, + equalExtent.StartColumnNumber, + new String(whitespaceChar, expectedStartColumnNumber - lhsExtent.EndColumnNumber), + GetError()); + } + + private string GetError() + { + return String.Format(CultureInfo.CurrentCulture, Strings.AlignAssignmentStatementError); + } + + private static IList> GetExtents( + TokenOperations tokenOps, + HashtableAst hashtableAst) + { + var nodeTuples = new List>(); + foreach (var kvp in hashtableAst.KeyValuePairs) + { + var keyStartOffset = kvp.Item1.Extent.StartOffset; + var keyTokenNode = tokenOps.GetTokenNodes( + token => token.Extent.StartOffset == keyStartOffset).FirstOrDefault(); + if (keyTokenNode == null + || keyTokenNode.Next == null + || keyTokenNode.Next.Value.Kind != TokenKind.Equals) + { + return null; + } + + nodeTuples.Add(new Tuple( + kvp.Item1.Extent, + keyTokenNode.Next.Value.Extent)); + } + + return nodeTuples; + } + + private bool HasKeysOnSeparateLines(HashtableAst hashtableAst) + { + var lines = new HashSet(); + foreach (var kvp in hashtableAst.KeyValuePairs) + { + if (lines.Contains(kvp.Item1.Extent.StartLineNumber)) + { + return false; + } + else + { + lines.Add(kvp.Item1.Extent.StartLineNumber); + } + } + + return true; + } + } +} diff --git a/Rules/ScriptAnalyzerBuiltinRules.csproj b/Rules/ScriptAnalyzerBuiltinRules.csproj index cd5c3da95..9fdc256ef 100644 --- a/Rules/ScriptAnalyzerBuiltinRules.csproj +++ b/Rules/ScriptAnalyzerBuiltinRules.csproj @@ -121,6 +121,7 @@ + diff --git a/Rules/Strings.resx b/Rules/Strings.resx index bb4f9abbc..6cea74a59 100644 --- a/Rules/Strings.resx +++ b/Rules/Strings.resx @@ -942,4 +942,16 @@ Use space after a semicolon. + + AlignAssignmentStatement + + + Align assignment statement + + + Line up assignment statements such that the assignment operator are aligned. + + + Assignment statements are not aligned + \ No newline at end of file diff --git a/Tests/Engine/GetScriptAnalyzerRule.tests.ps1 b/Tests/Engine/GetScriptAnalyzerRule.tests.ps1 index a81bf3634..40adc257d 100644 --- a/Tests/Engine/GetScriptAnalyzerRule.tests.ps1 +++ b/Tests/Engine/GetScriptAnalyzerRule.tests.ps1 @@ -61,7 +61,7 @@ Describe "Test Name parameters" { It "get Rules with no parameters supplied" { $defaultRules = Get-ScriptAnalyzerRule - $expectedNumRules = 49 + $expectedNumRules = 50 if ((Test-PSEditionCoreClr) -or (Test-PSVersionV3) -or (Test-PSVersionV4)) { # for PSv3 PSAvoidGlobalAliases is not shipped because @@ -70,7 +70,7 @@ Describe "Test Name parameters" { # for PowerShell Core, PSUseSingularNouns is not # shipped because it uses APIs that are not present # in dotnet core. - $expectedNumRules = 48 + $expectedNumRules = 49 } $defaultRules.Count | Should be $expectedNumRules } diff --git a/Tests/Rules/AlignAssignmentStatement.tests.ps1 b/Tests/Rules/AlignAssignmentStatement.tests.ps1 new file mode 100644 index 000000000..207b5f088 --- /dev/null +++ b/Tests/Rules/AlignAssignmentStatement.tests.ps1 @@ -0,0 +1,91 @@ +$directory = Split-Path -Parent $MyInvocation.MyCommand.Path +$testRootDirectory = Split-Path -Parent $directory + +Import-Module PSScriptAnalyzer +Import-Module (Join-Path $testRootDirectory "PSScriptAnalyzerTestHelper.psm1") + +$ruleConfiguration = @{ + Enable = $true + CheckHashtable = $true +} + +$settings = @{ + IncludeRules = @("PSAlignAssignmentStatement") + Rules = @{ + PSAlignAssignmentStatement = $ruleConfiguration + } +} + +Describe "AlignAssignmentStatement" { + Context "When assignment statements are in hashtable" { + It "Should find violation when assignment statements are not aligned (whitespace needs to be added)" { + $def = @' +$hashtable = @{ + property1 = "value" + anotherProperty = "another value" +} +'@ + +# Expected output after correction should be the following +# $hashtable = @{ +# property1 = "value" +# anotherProperty = "another value" +# } + + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings + $violations.Count | Should Be 1 + Test-CorrectionExtentFromContent $def $violations 1 ' ' ' ' + } + + It "Should find violation when assignment statements are not aligned (whitespace needs to be removed)" { + $def = @' +$hashtable = @{ + property1 = "value" + anotherProperty = "another value" +} +'@ + +# Expected output should be the following +# $hashtable = @{ +# property1 = "value" +# anotherProperty = "another value" +# } + + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings + $violations.Count | Should Be 1 + Test-CorrectionExtentFromContent $def $violations 1 ' ' ' ' + } + + It "Should ignore if a hashtable is empty" { + $def = @' +$x = @{ } +'@ + Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings | Get-Count | Should Be 0 + + } + } + + Context "When assignment statements are in DSC Configuration" { + It "Should find violations when assignment statements are not aligned" { + $def = @' +Configuration MyDscConfiguration { + + param( + [string[]]$ComputerName="localhost" + ) + Node $ComputerName { + WindowsFeature MyFeatureInstance { + Ensure = "Present" + Name = "RSAT" + } + WindowsFeature My2ndFeatureInstance { + Ensure = "Present" + Name = "Bitlocker" + } + } +} +'@ + Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings | Get-Count | Should Be 2 + } +} +}