diff --git a/Rules/AlignAssignmentStatement.cs b/Rules/AlignAssignmentStatement.cs index 1d79870f2..5b941924a 100644 --- a/Rules/AlignAssignmentStatement.cs +++ b/Rules/AlignAssignmentStatement.cs @@ -21,34 +21,45 @@ namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules #endif public 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. + /// Check the key value pairs of a hashtable, including DSC configurations. /// - /// [ConfigurableRuleProperty(defaultValue: true)] public bool CheckHashtable { get; set; } - private readonly char whitespaceChar = ' '; + /// + /// Whether to include hashtable key-value pairs where there is a comment + /// between the key and the equals sign in alignment. + /// + [ConfigurableRuleProperty(defaultValue: true)] + public bool AlignHashtableKvpWithInterveningComment { get; set; } - private List>> violationFinders - = new List>>(); + /// + /// Check the members of an enum. + /// + [ConfigurableRuleProperty(defaultValue: true)] + public bool CheckEnums { get; set; } /// - /// Sets the configurable properties of this rule. + /// Include enum members without explicit values in the width calculation. /// - /// 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); - } - } + [ConfigurableRuleProperty(defaultValue: true)] + public bool IncludeValuelessEnumMembers { get; set; } + + /// + /// Whether to include enum members where there is a comment + /// between the name and the equals sign in alignment. + /// + [ConfigurableRuleProperty(defaultValue: true)] + public bool AlignEnumMemberWithInterveningComment { get; set; } + /// + /// A mapping of line numbers to the indices of assignment operator + /// tokens on those lines. + /// + private readonly Dictionary> assignmentOperatorIndicesByLine = + new Dictionary>(); /// /// Analyzes the given ast to find if consecutive assignment statements are aligned. @@ -60,279 +71,640 @@ public override IEnumerable AnalyzeScript(Ast ast, string file { if (ast == null) { - throw new ArgumentNullException("ast"); + throw new ArgumentNullException(nameof(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) + // The high-level approach of the rule is to find all of the + // Key-Value pairs in a hashtable, or the members of an enum. + // For all of these assignments, we want to locate where both the + // left-hand-side (LHS) ends and where the equals sign is. + // Looking at all of these assignments for a particular structure, + // we can then decide where the equals sign _should_ be. It should + // be in the column after the longest LHS. + // + // Looking at where it _is_ vs where it _should_ be, we can then + // generate diagnostics and corrections. + + // As an optimisation, we first build a dictionary of all of the + // assignment operators in the script, keyed by line number. We do + // this by doing a single scan of the tokens. This makes it trvially + // fast to find the `Equals` token for a given assignment. + + // Note: In instances where there is a parse error, we do not have + // access to the tokens, so we can't build this dictionary. + // This is relevant for the DSC configuration parsing. + LocateAssignmentOperators(); + + if (CheckHashtable) { - foreach (var diagnosticRecord in violationFinder(tokenOps)) + // Find all hashtables + var hashtableAsts = ast.FindAll( + a => a is HashtableAst, true + ).Cast(); + foreach (var hashtableAst in hashtableAsts) { - yield return diagnosticRecord; + // For each hashtable find all assignment sites that meet + // our criteria for alignment checking + var hashtableAssignmentSites = ParseHashtable(hashtableAst); + + // Check alignment of the assignment sites and emit a + // diagnostic for each misalignment found. + foreach (var diag in CheckAlignment(hashtableAssignmentSites)) + { + yield return diag; + } + } + + // DSC does design time checking of available resource nodes. + // If a resource is not available at design time, the parser + // will error. A DSC Resource definition for a resource which is + // not found will not successfully be parsed and appear in the + // AST as a hashtable. The below is a best-effort attempt to + // find these assignment statements and consistently align them. + + // Find all ConfigurationDefinitionAsts + var dscConfigDefAsts = ast.FindAll( + a => a is ConfigurationDefinitionAst, true + ).Cast(); + foreach (var dscConfigDefAst in dscConfigDefAsts) + { + // Within each ConfigurationDefinitionAst, there can be many + // nested NamedBlocks, each of which can contain many nested + // CommandAsts. The CommandAsts which have 3 command + // elements, with the middle one being an equals sign, are + // the ones we're interested in. `ParseDscConfigDef` will + // emit parsed lists of these CommandAsts that share the + // same parent (and so should be aligned with one another). + foreach (var group in ParseDscConfigDef(dscConfigDefAst, ast)) + { + // Check alignment of the assignment sites and emit a + // diagnostic for each misalignment found. + foreach (var diag in CheckAlignment(group)) + { + yield return diag; + } + } } } - } - /// - /// Retrieves the common name of this rule. - /// - public override string GetCommonName() - { - return string.Format(CultureInfo.CurrentCulture, Strings.AlignAssignmentStatementCommonName); - } + if (CheckEnums) + { + // Find all enum TypeDefinitionAsts + var EnumTypeDefAsts = ast.FindAll( + a => a is TypeDefinitionAst t && t.IsEnum, true + ).Cast(); + foreach (var enumTypeDefAst in EnumTypeDefAsts) + { + // For each enum TypeDef find all assignment sites that meet + // our criteria for alignment checking + var enumAssignmentSites = ParseEnums(enumTypeDefAst); - /// - /// Retrieves the description of this rule. - /// - public override string GetDescription() - { - return string.Format(CultureInfo.CurrentCulture, Strings.AlignAssignmentStatementDescription); + // Check alignment of the assignment sites and emit a + // diagnostic for each misalignment found. + foreach (var diag in CheckAlignment(enumAssignmentSites)) + { + yield return diag; + } + } + } } /// - /// Retrieves the name of this rule. + /// Locate all the assignment tokens in the script and store their + /// indices in the assignmentOperatorIndicesByLine dictionary. /// - public override string GetName() + private void LocateAssignmentOperators() { - return string.Format( - CultureInfo.CurrentCulture, - Strings.NameSpaceFormat, - GetSourceName(), - Strings.AlignAssignmentStatementName); - } + // Clear any existing entries + assignmentOperatorIndicesByLine.Clear(); - /// - /// Retrieves the severity of the rule: error, warning or information. - /// - public override RuleSeverity GetSeverity() - { - return RuleSeverity.Warning; + var tokens = Helper.Instance.Tokens; + // Iterate through all tokens, looking for Equals tokens + for (int i = 0; i < tokens.Length; i++) + { + if (tokens[i].Kind == TokenKind.Equals) + { + // When an equals token is found, check if the dictionary + // has an entry for this line number, and if not create one. + int lineNumber = tokens[i].Extent.StartLineNumber; + if (!assignmentOperatorIndicesByLine.ContainsKey(lineNumber)) + { + assignmentOperatorIndicesByLine[lineNumber] = new List(); + } + // Add the index of this token to the list for this line + assignmentOperatorIndicesByLine[lineNumber].Add(i); + } + } } /// - /// Gets the severity of the returned diagnostic record: error, warning, or information. + /// Parse a hashtable's key-value pairs into a list of tuples which are + /// later used to verify and correct alignment of assignment operators. /// - /// - public DiagnosticSeverity GetDiagnosticSeverity() + /// The hashtable AST to parse. + /// + /// A list of tuples, where each tuple is a (lhsTokenExtent, equalsExtent) + /// pair representing the extent of the token immediately before the '=' + /// (effectively the key/rightmost key token) and the extent of the '=' itself. + /// Only includes pairs where an '=' token is found on the same line as the key. + /// Implicitly skips line continuations. + /// + private List> ParseHashtable(HashtableAst hashtableAst) { - return DiagnosticSeverity.Warning; - } + var assignmentSites = new List>(); - /// - /// Retrieves the name of the module/assembly the rule is from. - /// - public override string GetSourceName() - { - return string.Format(CultureInfo.CurrentCulture, Strings.SourceName); + if (hashtableAst == null) { return assignmentSites; } + + // Enumerate the KeyValuePairs of this hashtable + // Each KVP is a Tuple + foreach (var kvp in hashtableAst.KeyValuePairs) + { + // If the assignmentOperator dictionary has no entry for the + // line that the key ends on, skip this KVP + if (!assignmentOperatorIndicesByLine.ContainsKey(kvp.Item1.Extent.EndLineNumber)) + { + continue; + } + + // Next we need to find the location of the equals sign for this + // Key-Value pair. We know the line it should be on. We can + // search all of the equals signs on that line for the one that + // lives between the end of the key and the start of the value. + + int equalsTokenIndex = -1; + foreach (var index in assignmentOperatorIndicesByLine[kvp.Item1.Extent.EndLineNumber]) + { + if (Helper.Instance.Tokens[index].Extent.StartOffset >= kvp.Item1.Extent.EndOffset && + Helper.Instance.Tokens[index].Extent.EndOffset <= kvp.Item2.Extent.StartOffset + ) + { + equalsTokenIndex = index; + break; + } + } + + // If we didn't find the equals sign - skip this KVP + if (equalsTokenIndex == -1) + { + continue; + } + + // Normally a Key-Value pair looks like: + // + // Key = Value + // + // But the below is also valid: + // + // Key <#Inline Comment#> = Value + // + // We can still use this KVP for alignment - we simply treat + // the end of the token before the equals sign as the Left-Hand + // Side (LHS) of the assignment. We expose a user setting for + // this. + // If the user has not chosen to align such KVPs and the token + // before the equals sign does not end at the same offset as + // the key, we skip this KVP. + if (!AlignHashtableKvpWithInterveningComment && + Helper.Instance.Tokens[equalsTokenIndex - 1].Extent.EndOffset != kvp.Item1.Extent.EndOffset + ) + { + continue; + } + + assignmentSites.Add(new Tuple( + Helper.Instance.Tokens[equalsTokenIndex - 1].Extent, + Helper.Instance.Tokens[equalsTokenIndex].Extent + )); + } + + return assignmentSites; } /// - /// Retrieves the type of the rule, Builtin, Managed or Module. + /// Parse a DSC configuration definition's resource/property blocks into + /// a list of tuples which are later used to verify and correct alignment of + /// assignment operators. /// - public override SourceType GetSourceType() - { - return SourceType.Builtin; - } - private IEnumerable FindHashtableViolations(TokenOperations tokenOps) + /// The ConfigurationDefinitionAst to parse. + /// + /// An enumeration of lists of tuples, where each tuple is a (lhsTokenExtent, equalsExtent) + /// pair representing the extent of the token immediately before the '=' + /// (effectively the key/rightmost key token) and the extent of the '=' itself. + /// Only includes pairs where an '=' token is found on the same line as the key. + /// Implicitly skips line continuations. + /// + private IEnumerable>> ParseDscConfigDef( + ConfigurationDefinitionAst configDefAst, + Ast ast + ) { - var hashtableAsts = tokenOps.Ast.FindAll(ast => ast is HashtableAst, true); - var groups = new List>>(); - if (hashtableAsts != null) + + if (configDefAst == null) { yield break; } + + // Find command asts shaped like: = + var commandAsts = configDefAst.FindAll( + a => + a is CommandAst c && + c.CommandElements.Count == 3 && + c.CommandElements[1].Extent?.Text == "=", + true + ).Cast(); + + // Group by grandparent NamedBlock (commandAst.Parent is PipelineAst) + var grouped = commandAsts.GroupBy( + c => c.Parent?.Parent + ); + + foreach (var group in grouped) { - foreach (var astItem in hashtableAsts) + var assignmentSites = new List>(); + + foreach (var cmd in group) { - groups.Add(GetExtents(tokenOps, (HashtableAst)astItem)); - } - } + var lhs = cmd.CommandElements[0].Extent; + var eq = cmd.CommandElements[1].Extent; -#if !PSV3 - var configAsts = tokenOps.Ast.FindAll(ast => ast is ConfigurationDefinitionAst, true); - if (configAsts != null) - { - // There are probably parse errors caused by an "Undefined DSC resource" - // which prevents the parser from detecting the property value pairs as - // hashtable. Hence, this is a workaround to format configurations which - // have "Undefined DSC resource" parse errors. - - // find all commandAsts of the form "prop" "=" "val" that have the same parent - // and format those pairs. - foreach (var configAst in configAsts) + if (lhs.EndLineNumber != eq.StartLineNumber) + { + // Skip if the key and equals sign are not on the same + // line + continue; + } + + // Note: We can't use the token dictionary here like we do + // for hashtables/enums, as we get here typically + // because there's a parse error. i.e. + // ModuleNotFoundDuringParse and ResourceNotDefined + // Helper.Instance.Tokens is unavailable when there's + // a parse error so we can only use the ast. + + // In lieu of being able to check tokens, we check the + // source text between the end of the lhs and the start of + // the equals sign for non-whitespace characters. + // + // key <#comment#> = value + // ^ ^ + // | | + // ------------- + // | + // We check for non-whitespace characters here + // + // If there are any, we extend the lhs extent to include + // them, so that the alignment is to the end of the + // rightmost non-whitespace characters. + + // We get the text between between lhs and eq, trim it from + // the end (so we keep the right-most non-whitespace + // characters). It's length is how much we need to extend + // the lhs extent by. + var nonWhitespaceLength = + ast.Extent.Text.Substring( + lhs.EndOffset, + eq.StartOffset - lhs.EndOffset + ).TrimEnd().Length; + + // If there's any non-whitespace characters between the + // key and the equals sign, and the user has chosen to + // ignore such cases, skip this KVP. + if (nonWhitespaceLength > 0 && !AlignHashtableKvpWithInterveningComment) + { + continue; + } + + IScriptExtent leftExtent = null; + if (nonWhitespaceLength == 0) + { + // When there is no intervening comment, we use the + // key's extent as the LHS extent. + leftExtent = lhs; + } + else + { + // When there is an intervening comment, we extend + // the key's extent to include it. + leftExtent = new ScriptExtent( + new ScriptPosition( + lhs.File, + lhs.StartLineNumber, + lhs.StartColumnNumber, + null + ), + new ScriptPosition( + lhs.File, + lhs.EndLineNumber, + lhs.EndColumnNumber + nonWhitespaceLength, + null + ) + ); + } + + assignmentSites.Add(new Tuple( + leftExtent, + eq + )); + } + if (assignmentSites.Count > 0) { - groups.AddRange(GetCommandElementExtentGroups(configAst)); + yield return assignmentSites; } } -#endif + } - // 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 extentTuples in groups) + /// + /// Parse an enum's members into a list of tuples which are later used to + /// verify and correct alignment of assignment operators. + /// + /// The enum TypeDefinitionAst to parse. + /// + /// A list of tuples, where each tuple is a (lhsTokenExtent, equalsExtent) + /// pair representing the extent of the token immediately before the '=' + /// (effectively the member name) and the extent of the '=' itself. + /// Implicitly skips line continuations. + /// + private List> ParseEnums( + TypeDefinitionAst enumTypeDefAst + ) + { + var assignmentSites = new List>(); + if (enumTypeDefAst == null) { return assignmentSites; } + + // Ensure we're only processing enums + if (!enumTypeDefAst.IsEnum) { return assignmentSites; } + + // Enumerate Enum Members that are PropertyMemberAst + foreach ( + var member in enumTypeDefAst.Members.Where( + m => m is PropertyMemberAst + ).Cast() + ) { - if (!HasPropertiesOnSeparateLines(extentTuples)) + + // Enums can have members with or without explicit values. + + // If InitialValue is null, this member has no explicit + // value and so should have no equals sign. + if (member.InitialValue == null) { + if (!IncludeValuelessEnumMembers) + { + continue; + } + + if (member.Extent.StartLineNumber != member.Extent.EndLineNumber) + { + // This member spans multiple lines - skip it + continue; + } + + // We include this member in the alignment check, but + // with a null equalsExtent. This will be ignored in + // CheckAlignment, but will ensure that this member + // is included in the calculation of the target column. + assignmentSites.Add(new Tuple( + member.Extent, + null + )); continue; } - if (extentTuples == null - || extentTuples.Count == 0 - || !extentTuples.All(t => t.Item1.StartLineNumber == t.Item2.EndLineNumber)) + // If the assignmentOperator dictionary has no entry for the + // line of the member name - skip this member; it should + // have an explicit value, so must have an equals sign. + // It's possible that the equals sign is on a different + // line thanks to line continuations (`). We skip such + // members. + if (!assignmentOperatorIndicesByLine.ContainsKey(member.Extent.StartLineNumber)) { continue; } - var expectedStartColumnNumber = extentTuples.Max(x => x.Item1.EndColumnNumber) + 1; - foreach (var extentTuple in extentTuples) + // Next we need to find the location of the equals sign for this + // member. We know the line it should be on. We can + // search all of the equals signs on that line. + // + // Unlike hashtables, we don't have an extent for the LHS and + // RHS of the member. We have the extent of the entire + // member, the name of the member, and the extent of the + // InitialValue (RHS). We can use these to find the equals + // sign. We know the equals sign must be after the + // member name, and before the InitialValue. + + int equalsTokenIndex = -1; + foreach (var index in assignmentOperatorIndicesByLine[member.Extent.StartLineNumber]) { - if (extentTuple.Item2.StartColumnNumber != expectedStartColumnNumber) + if (Helper.Instance.Tokens[index].Extent.StartOffset >= (member.Extent.StartColumnNumber + member.Name.Length) && + Helper.Instance.Tokens[index].Extent.EndOffset < member.InitialValue.Extent.StartOffset + ) { - yield return new DiagnosticRecord( - GetError(), - extentTuple.Item2, - GetName(), - GetDiagnosticSeverity(), - extentTuple.Item1.File, - null, - GetHashtableCorrections(extentTuple, expectedStartColumnNumber).ToList()); + equalsTokenIndex = index; + break; } } - } - } - private List>> GetCommandElementExtentGroups(Ast configAst) - { - var result = new List>>(); - var commandAstGroups = GetCommandElementGroups(configAst); - foreach (var commandAstGroup in commandAstGroups) - { - var list = new List>(); - foreach (var commandAst in commandAstGroup) + // If we didn't find the equals sign - skip, it's likely on a + // different line due to line continuations. + if (equalsTokenIndex == -1) { - var elems = commandAst.CommandElements; - list.Add(new Tuple(elems[0].Extent, elems[1].Extent)); + continue; } - result.Add(list); - } + // Normally a member with a value looks like: + // + // Name = Value + // + // But the below is also valid: + // + // Name <#Inline Comment#> = Value + // + // We can still use this member for alignment - we simply treat + // the end of the token before the equals sign as the Left-Hand + // Side (LHS) of the assignment. We expose a user setting for + // this. + // If the user has not chosen to align such members and the + // token before the equals sign is a comment, we skip this + // member. + if (!AlignEnumMemberWithInterveningComment && + Helper.Instance.Tokens[equalsTokenIndex - 1].Kind == TokenKind.Comment + ) + { + continue; + } - return result; + assignmentSites.Add(new Tuple( + Helper.Instance.Tokens[equalsTokenIndex - 1].Extent, + Helper.Instance.Tokens[equalsTokenIndex].Extent + )); + } + return assignmentSites; } - private List> GetCommandElementGroups(Ast configAst) + /// + /// Check alignment of assignment operators in the provided list of + /// (lhsTokenExtent, equalsExtent) tuples, and return diagnostics for + /// any misalignments found. + /// + /// From the lhsTokenExtent, we can determine the target column for + /// alignment (the column after the longest key). We then compare the + /// equalsExtent's start column to the target column, and if they + /// differ, we have a misalignment and return a diagnostic. + /// + /// + /// A list of tuples, where each tuple is a (lhsTokenExtent, equalsExtent) + /// pair representing the extent of the token immediately before the '=' + /// and the extent of the '=' itself. + /// Only includes pairs where an '=' token is found on the same line as + /// the key. + /// + /// + /// An enumerable of DiagnosticRecords, one for each misaligned + /// assignment operator found. + /// + private IEnumerable CheckAlignment( + List> assignmentSites + ) { - var result = new List>(); - var astsFound = configAst.FindAll(ast => IsPropertyValueCommandAst(ast), true); - if (astsFound == null) + if (assignmentSites == null || assignmentSites.Count == 0) { - return result; + yield break; } - var parentChildrenGroup = from ast in astsFound - select (CommandAst)ast into commandAst - group commandAst by commandAst.Parent.Parent; // parent is pipeline and pipeline's parent is namedblockast - foreach (var group in parentChildrenGroup) + // Filter out everything from assignmentSites that is not on + // it's own line. Do this by grouping by the start line number + // of the lhsTokenExtent, and only keeping groups with a count + // of 1. + assignmentSites = assignmentSites + .GroupBy(t => t.Item1.StartLineNumber) + .Where(g => g.Count() == 1) + .Select(g => g.First()) + .ToList(); + + // If, after filtering, we have no assignment sites, exit + if (assignmentSites == null || assignmentSites.Count == 0) { - result.Add(group.ToList()); + yield break; } - return result; + // The target column for this hashtable is longest key plus one + // space. + var targetColumn = assignmentSites + .Max(t => t.Item1.EndColumnNumber) + 1; + + // Check each element of the hashtable to see if it's aligned + foreach (var site in assignmentSites) + { + // If the equalsExtent is null, this is a member without + // an explicit value. We include such members in the + // calculation of the target column, but we don't + // generate diagnostics for them. + if (site.Item2 == null) + { + continue; + } + + // If the equals sign is already at the target column, + // no diagnostic is needed. + if (site.Item2.StartColumnNumber == targetColumn) + { + continue; + } + + yield return new DiagnosticRecord( + string.Format(CultureInfo.CurrentCulture, Strings.AlignAssignmentStatementError), + site.Item2, + GetName(), + DiagnosticSeverity.Warning, + site.Item1.File, + null, + GetCorrectionExtent( + site.Item1, + site.Item2, + targetColumn + ) + ); + } } - private bool IsPropertyValueCommandAst(Ast ast) + /// + /// Generate the correction extent to align the assignment operator + /// to the target column. + /// + /// The extent of the token immediately before the '=' + /// The extent of the '=' token + /// The target column to align to + /// An enumerable of CorrectionExtents, one for each correction + private List GetCorrectionExtent( + IScriptExtent lhsExtent, + IScriptExtent equalsExtent, + int targetColumn + ) { - var commandAst = ast as CommandAst; - return commandAst != null - && commandAst.CommandElements.Count() == 3 - && commandAst.CommandElements[1].Extent.Text.Equals("="); + // We generate a correction extent which replaces the text between + // the end of the lhs and the start of the equals sign with the + // appropriate number of spaces to align the equals sign to the + // target column. + return new List + { + new CorrectionExtent( + lhsExtent.EndLineNumber, + equalsExtent.StartLineNumber, + lhsExtent.EndColumnNumber, + equalsExtent.StartColumnNumber, + new string(' ', targetColumn - lhsExtent.EndColumnNumber), + string.Format(CultureInfo.CurrentCulture, Strings.AlignAssignmentStatementError) + ) + }; } - private IEnumerable GetHashtableCorrections( - Tuple extentTuple, - int expectedStartColumnNumber) + /// + /// Retrieves the common name of this rule. + /// + public override string GetCommonName() { - 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()); + return string.Format(CultureInfo.CurrentCulture, Strings.AlignAssignmentStatementCommonName); } - private string GetError() + /// + /// Retrieves the description of this rule. + /// + public override string GetDescription() { - return String.Format(CultureInfo.CurrentCulture, Strings.AlignAssignmentStatementError); + return string.Format(CultureInfo.CurrentCulture, Strings.AlignAssignmentStatementDescription); } - private static List> GetExtents( - TokenOperations tokenOps, - HashtableAst hashtableAst) + /// + /// Retrieves the name of this rule. + /// + public override string GetName() { - var nodeTuples = new List>(); - foreach (var kvp in hashtableAst.KeyValuePairs) - { - var keyStartOffset = kvp.Item1.Extent.StartOffset; - bool keyStartOffSetReached = false; - var keyTokenNode = tokenOps.GetTokenNodes( - token => - { - if (keyStartOffSetReached) - { - return token.Kind == TokenKind.Equals; - } - if (token.Extent.StartOffset == keyStartOffset) - { - keyStartOffSetReached = true; - } - return false; - }).FirstOrDefault(); - if (keyTokenNode == null || keyTokenNode.Value == null) - { - continue; - } - var assignmentToken = keyTokenNode.Value.Extent; - - nodeTuples.Add(new Tuple( - kvp.Item1.Extent, assignmentToken)); - } + return string.Format( + CultureInfo.CurrentCulture, + Strings.NameSpaceFormat, + GetSourceName(), + Strings.AlignAssignmentStatementName); + } - return nodeTuples; + /// + /// Retrieves the severity of the rule: error, warning or information. + /// + public override RuleSeverity GetSeverity() + { + return RuleSeverity.Warning; } - private bool HasPropertiesOnSeparateLines(IEnumerable> tuples) + /// + /// Retrieves the name of the module/assembly the rule is from. + /// + public override string GetSourceName() { - if (tuples.Count() == 1) - { - // If the hashtable has just a single key-value pair, it does not have properties on separate lines - return false; - } - var lines = new HashSet(); - foreach (var kvp in tuples) - { - if (lines.Contains(kvp.Item1.StartLineNumber)) - { - return false; - } - else - { - lines.Add(kvp.Item1.StartLineNumber); - } - } + return string.Format(CultureInfo.CurrentCulture, Strings.SourceName); + } - return true; + /// + /// Retrieves the type of the rule, Builtin, Managed or Module. + /// + public override SourceType GetSourceType() + { + return SourceType.Builtin; } } } diff --git a/Tests/Rules/AlignAssignmentStatement.tests.ps1 b/Tests/Rules/AlignAssignmentStatement.tests.ps1 index 7558abf88..1262acdf8 100644 --- a/Tests/Rules/AlignAssignmentStatement.tests.ps1 +++ b/Tests/Rules/AlignAssignmentStatement.tests.ps1 @@ -2,164 +2,956 @@ # Licensed under the MIT License. BeforeAll { - $testRootDirectory = Split-Path -Parent $PSScriptRoot - Import-Module (Join-Path $testRootDirectory "PSScriptAnalyzerTestHelper.psm1") + function New-AlignAssignmentSettings { + [OutputType([hashtable])] + [CmdletBinding()] + param( + [Parameter()] + [bool] + $CheckHashtable = $false, + [Parameter()] + [bool] + $AlignHashtableKvpWithInterveningComment = $false, + [Parameter()] + [bool] + $CheckEnums = $false, + [Parameter()] + [bool] + $IncludeValuelessEnumMembers = $false, + [Parameter()] + [bool] + $AlignEnumMemberWithInterveningComment = $false + ) + return @{ + IncludeRules = @('PSAlignAssignmentStatement') + Rules = @{ + PSAlignAssignmentStatement = @{ + Enable = $true + CheckHashtable = $CheckHashtable + AlignHashtableKvpWithInterveningComment = $AlignHashtableKvpWithInterveningComment + CheckEnums = $CheckEnums + IncludeValuelessEnumMembers = $IncludeValuelessEnumMembers + AlignEnumMemberWithInterveningComment = $AlignEnumMemberWithInterveningComment + } + } + } + } - $ruleConfiguration = @{ - Enable = $true - CheckHashtable = $true + function Get-NonParseDiagnostics { + [OutputType([object[]])] + [CmdletBinding()] + param( + [Parameter(Mandatory, ValueFromPipeline)] + [object[]] + $Diagnostics + ) + process { + $Diagnostics | Where-Object { + $_.RuleName -eq 'PSAlignAssignmentStatement' + } + } } - $settings = @{ - IncludeRules = @("PSAlignAssignmentStatement") - Rules = @{ - PSAlignAssignmentStatement = $ruleConfiguration + function Apply-Corrections { + [OutputType([string])] + [CmdletBinding()] + param( + [string] + $Original, + [object[]] + $Diagnostics + ) + # Note: This only works to apply the correction extents because all of + # our corrections are simple, single line operations. + $lines = $Original -split "`n" + foreach ($Diagnostic in $Diagnostics) { + if (-not $Diagnostic.SuggestedCorrections) { + continue + } + foreach ($extent in $Diagnostic.SuggestedCorrections) { + $lineIndex = $extent.StartLineNumber - 1 + $prefix = $lines[$lineIndex].Substring( + 0, $extent.StartColumnNumber - 1 + ) + $suffix = $lines[$lineIndex].Substring( + $extent.EndColumnNumber - 1 + ) + $lines[$lineIndex] = $prefix + $extent.Text + $suffix + + } } + return ($lines -join "`n") } } -Describe "AlignAssignmentStatement" { - Context "When assignment statements are in hashtable" { - It "Should find violation when assignment statements are not aligned (whitespace needs to be added)" { +Describe 'AlignAssignmentStatement' { + + Context 'When checking Hashtables is disabled' { + + It 'Should not find violations in mis-aligned hashtables' { + $def = @' +@{ + 'Key' = 'Value' + 'LongerKey' = 'Value' +} +'@ + $settings = New-AlignAssignmentSettings + + Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings | + Get-NonParseDiagnostics | + Should -BeNullOrEmpty + + } + + It 'Should not find violations in DSC configuration blocks' { + $def = @' +Configuration C1 { + Node localhost { + NonExistentResource X { + Ensure = '"Present"' + Name = '"RSAT"' + } + } +} +'@ + $settings = New-AlignAssignmentSettings + + Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings | + Get-NonParseDiagnostics | + Should -BeNullOrEmpty + + } -Skip:($IsLinux -or $IsMacOS) + + } + + Context 'When Hashtable checking is enabled' { + + It 'Should not find violations in empty single-line hashtable' { + $def = '@{}' + + $settings = New-AlignAssignmentSettings -CheckHashtable $true + + Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings | + Get-NonParseDiagnostics | + Should -BeNullOrEmpty + } + + It 'Should not find violations in empty multi-line hashtable' { + $def = @' +@{ + +} +'@ + + $settings = New-AlignAssignmentSettings -CheckHashtable $true + + Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings | + Get-NonParseDiagnostics | + Should -BeNullOrEmpty + } + + It 'Should not find violation in aligned, single-line, single-kvp hashtable' { + $def = '@{"Key" = "Value"}' + + $settings = New-AlignAssignmentSettings -CheckHashtable $true + + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings | + Get-NonParseDiagnostics + $violations | Should -BeNullOrEmpty + } + + It 'Should find violation in mis-aligned, single-line, single-kvp hashtable' { + $def = '@{"Key" = "Value"}' + $expected = '@{"Key" = "Value"}' + + $settings = New-AlignAssignmentSettings -CheckHashtable $true + + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings | + Get-NonParseDiagnostics + $violations | Should -HaveCount 1 + + $corrected = Apply-Corrections -Original $def -Diagnostics $violations + + $corrected | Should -BeExactly $expected + } + + It 'Should not find violations in mis-aligned hashtable with multiple kvp on a single line' { + $def = '@{"Key1" = "Value1";"Key2"="Value2"}' + + $settings = New-AlignAssignmentSettings -CheckHashtable $true + + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings | + Get-NonParseDiagnostics + $violations | Should -BeNullOrEmpty + } + + It 'Should not find violations in well aligned, multi-line, multi-kvp hashtable' { + $def = @' +@{ + 'Key1' = 'Value1' + 'Key2' = 'Value2' + 'Key3' = 'Value3' +} +'@ + + $settings = New-AlignAssignmentSettings -CheckHashtable $true + + Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings | + Get-NonParseDiagnostics | + Should -BeNullOrEmpty + } + + It 'Should find violations in mis-aligned, multi-line, multi-kvp hashtable' { + $def = @' +@{ + 'Key1'= 'Value1' + 'Key12' = 'Value2' + 'Key123' = 'Value3' +} +'@ + + $expected = @' +@{ + 'Key1' = 'Value1' + 'Key12' = 'Value2' + 'Key123' = 'Value3' +} +'@ + + $settings = New-AlignAssignmentSettings -CheckHashtable $true + + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings | + Get-NonParseDiagnostics + $violations | Should -HaveCount 2 + + $corrected = Apply-Corrections -Original $def -Diagnostics $violations + $corrected | Should -BeExactly $expected + } + + It 'Should ignore lines with intervening comments when AlignHashtableKvpWithInterveningComment is false' { + $def = @' +@{ + 'Key1' <#comment#>= 'Value1' + 'Key12' = 'Value2' + 'Key123' = 'Value3' +} +'@ + + $expected = @' +@{ + 'Key1' <#comment#>= 'Value1' + 'Key12' = 'Value2' + 'Key123' = 'Value3' +} +'@ + + $settings = New-AlignAssignmentSettings -CheckHashtable $true + + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings | + Get-NonParseDiagnostics + $violations | Should -HaveCount 1 + + $corrected = Apply-Corrections -Original $def -Diagnostics $violations + $corrected | Should -BeExactly $expected + } + + It 'Should align lines with intervening comments when AlignHashtableKvpWithInterveningComment is true' { $def = @' -$hashtable = @{ - property1 = "value" - anotherProperty = "another value" +@{ + 'Key1' <#comment#>= 'Value1' + 'Key12' = 'Value2' + 'Key123' = 'Value3' } '@ - # Expected output after correction should be the following - # $hashtable = @{ - # property1 = "value" - # anotherProperty = "another value" - # } + $expected = @' +@{ + 'Key1' <#comment#> = 'Value1' + 'Key12' = 'Value2' + 'Key123' = 'Value3' +} +'@ + + $settings = New-AlignAssignmentSettings -CheckHashtable $true -AlignHashtableKvpWithInterveningComment $true + + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings | + Get-NonParseDiagnostics + $violations | Should -HaveCount 3 - $violations = Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings - $violations.Count | Should -Be 1 - Test-CorrectionExtentFromContent $def $violations 1 ' ' ' ' + $corrected = Apply-Corrections -Original $def -Diagnostics $violations + $corrected | Should -BeExactly $expected } - It "Should find violation when assignment statements are not aligned (whitespace needs to be removed)" { + It 'Should not find violations when intervening comment is already aligned and AlignHashtableKvpWithInterveningComment is true' { $def = @' -$hashtable = @{ - property1 = "value" - anotherProperty = "another value" +@{ + 'Key1' <#comment#> = 'Value1' + 'Key2' = 'Value2' + 'Key3' = 'Value3' } '@ - # Expected output should be the following - # $hashtable = @{ - # property1 = "value" - # anotherProperty = "another value" - # } + $settings = New-AlignAssignmentSettings -CheckHashtable $true -AlignHashtableKvpWithInterveningComment $true - $violations = Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings - $violations.Count | Should -Be 1 - Test-CorrectionExtentFromContent $def $violations 1 ' ' ' ' + Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings | + Get-NonParseDiagnostics | + Should -BeNullOrEmpty } - It "Should not crash if property name reaches further to the right than the longest property name (regression test for issue 1067)" { + It 'Should not find violations when intervening comment is on right of equals sign and AlignHashtableKvpWithInterveningComment is true' { $def = @' -$hashtable = @{ property1 = "value" - anotherProperty = "another value" +@{ + 'Key1' = <#comment#> 'Value1' + 'Key2' = 'Value2' + 'Key3' = 'Value3' } '@ - $violations = Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings -ErrorAction Stop | Get-Count | Should -Be 0 + $settings = New-AlignAssignmentSettings -CheckHashtable $true -AlignHashtableKvpWithInterveningComment $true + + Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings | + Get-NonParseDiagnostics | + Should -BeNullOrEmpty } - It "Should ignore if a hashtable is empty" { + It 'Should ignore kvp with a line continuation between key and equals sign' { $def = @' -$x = @{ } +@{ + 'LongerKey' ` + = <#comment#> 'Value1' + 'Key1' = 'Value2' + 'Key12' = 'Value3' +} +'@ + + $expected = @' +@{ + 'LongerKey' ` + = <#comment#> 'Value1' + 'Key1' = 'Value2' + 'Key12' = 'Value3' +} '@ - Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings | Get-Count | Should -Be 0 + $settings = New-AlignAssignmentSettings -CheckHashtable $true + + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings | + Get-NonParseDiagnostics + $violations | Should -HaveCount 1 + + $corrected = Apply-Corrections -Original $def -Diagnostics $violations + $corrected | Should -BeExactly $expected } - It "Should ignore if a hashtable has a single key-value pair on a single line" { + It 'Should correctly align kvp when key is a string containing an equals sign' { $def = @' -$x = @{ 'key'="value" } +@{ + 'key1=5' = 'Value1' + 'Key1' = 'Value2' + 'Key12' = 'Value3' +} '@ - Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings | Get-Count | Should -Be 0 + $expected = @' +@{ + 'key1=5' = 'Value1' + 'Key1' = 'Value2' + 'Key12' = 'Value3' +} +'@ + + $settings = New-AlignAssignmentSettings -CheckHashtable $true + + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings | + Get-NonParseDiagnostics + $violations | Should -HaveCount 3 + + $corrected = Apply-Corrections -Original $def -Diagnostics $violations + $corrected | Should -BeExactly $expected } - It "Should ignore if a hashtable has a single key-value pair across multiple lines" { + It 'Should correctly align kvp when key is an expression containing an assignment' { + # Note: `($key='key1')` defines the variable `$key` and sets it's + # value to 'key1'. The entire expression evaluates to 'key1' + # which is then used as the hashtable key. So the first key + # at runtime is equal to the string 'key1'. $def = @' -$x = @{ - 'key'="value" +@{ + ($key='key1') = 'Value1' + 'Key2' = 'Value2' + 'Key3' = 'Value3' +} +'@ + + $expected = @' +@{ + ($key='key1') = 'Value1' + 'Key2' = 'Value2' + 'Key3' = 'Value3' } '@ - Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings | Get-Count | Should -Be 0 + $settings = New-AlignAssignmentSettings -CheckHashtable $true + + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings | + Get-NonParseDiagnostics + $violations | Should -HaveCount 3 + + $corrected = Apply-Corrections -Original $def -Diagnostics $violations + $corrected | Should -BeExactly $expected } - It "Should ignore if a hashtable has multiple key-value pairs on a single line" { + It 'Should correctly align hashtables independantly when nested' { $def = @' -$x = @{ 'key'="value"; 'key2'="value2"; 'key3WithLongerName'="value3" } +@{ + 'key1' = 5 + 'key12' = @{ + 'nestedKey1' = 'Value1' + 'nestedKey12'= 'Value2' + 'nestedKey123'= @{ + 'superNestedKey1' = 'Value1' + 'superNestedKey12'='Value2' + } + } + 'key123' = 'Value3' +} '@ - Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings | Get-Count | Should -Be 0 + $expected = @' +@{ + 'key1' = 5 + 'key12' = @{ + 'nestedKey1' = 'Value1' + 'nestedKey12' = 'Value2' + 'nestedKey123' = @{ + 'superNestedKey1' = 'Value1' + 'superNestedKey12' ='Value2' } + } + 'key123' = 'Value3' +} +'@ + + $settings = New-AlignAssignmentSettings -CheckHashtable $true + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings | + Get-NonParseDiagnostics + $violations | Should -HaveCount 8 + + $corrected = Apply-Corrections -Original $def -Diagnostics $violations + $corrected | Should -BeExactly $expected + } + + It 'Should not find violations in aligned DSC configuration blocks' { + $def = @' +Configuration C1 { + Node localhost { + NonExistentResource X { + Ensure = '"Present"' + Name = '"RSAT"' + } } +} +'@ + $settings = New-AlignAssignmentSettings -CheckHashtable $true + + Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings | + Get-NonParseDiagnostics | + Should -BeNullOrEmpty + + } -Skip:($IsLinux -or $IsMacOS) - Context "When assignment statements are in DSC Configuration" { - It "Should find violations when assignment statements are not aligned" -skip:($IsLinux -or $IsMacOS) { + It 'Should find violations in mis-aligned DSC configuration blocks' { $def = @' -Configuration MyDscConfiguration { +Configuration C1 { + Node localhost { + NonExistentResource X { + Ensure = '"Present"' + Name = '"RSAT"' + } + } +} +'@ - param( - [string[]]$ComputerName="localhost" - ) - Node $ComputerName { - WindowsFeature MyFeatureInstance { - Ensure = "Present" - Name = "RSAT" + $expected = @' +Configuration C1 { + Node localhost { + NonExistentResource X { + Ensure = '"Present"' + Name = '"RSAT"' } - WindowsFeature My2ndFeatureInstance { - Ensure = "Present" - Name = "Bitlocker" + } +} +'@ + + $settings = New-AlignAssignmentSettings -CheckHashtable $true + + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings | + Get-NonParseDiagnostics + $violations | Should -HaveCount 1 + + $corrected = Apply-Corrections -Original $def -Diagnostics $violations + $corrected | Should -BeExactly $expected + + } -Skip:($IsLinux -or $IsMacOS) + + It 'Should ignore lines in DSC configuration blocks with intervening comments when AlignHashtableKvpWithInterveningComment is false' { + $def = @' +Configuration C1 { + Node localhost { + NonExistentResource X { + Ensure = '"Present"' + Name <#asdasd#>= '"RSAT"' + Other = 'Value' } } } '@ - Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings | Get-Count | Should -Be 2 + + $expected = @' +Configuration C1 { + Node localhost { + NonExistentResource X { + Ensure = '"Present"' + Name <#asdasd#>= '"RSAT"' + Other = 'Value' } } +} +'@ + + $settings = New-AlignAssignmentSettings -CheckHashtable $true -AlignHashtableKvpWithInterveningComment $false - if ($PSVersionTable.PSVersion.Major -ge 5) { - Context "When assignment statements are in DSC Configuration that has parse errors" { - It "Should find violations when assignment statements are not aligned" -skip:($IsLinux -or $IsMacOS) { - $def = @' -Configuration Sample_ChangeDescriptionAndPermissions -{ - Import-DscResource -Module NonExistentModule - # A Configuration block can have zero or more Node blocks - Node $NodeName - { - # Next, specify one or more resource blocks + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings | + Get-NonParseDiagnostics + $violations | Should -HaveCount 1 - NonExistentModule MySMBShare - { - Ensure = "Present" - Name = "MyShare" - Path = "C:\Demo\Temp" - ReadAccess = "author" - FullAccess = "some other author" - Description = "This is an updated description for this share" + $corrected = Apply-Corrections -Original $def -Diagnostics $violations + $corrected | Should -BeExactly $expected + } -Skip:($IsLinux -or $IsMacOS) + + It 'Should align lines in DSC configuration blocks with intervening comments when AlignHashtableKvpWithInterveningComment is true' { + $def = @' +Configuration C1 { + Node localhost { + NonExistentResource X { + Ensure = '"Present"' + Name <#asdasd#>= '"RSAT"' + Other = 'Value' } } } '@ - # This invocation will throw parse error caused by "Undefined DSC resource" because - # NonExistentModule is not really avaiable to load. Therefore we set erroraction to - # SilentlyContinue - Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings -ErrorAction SilentlyContinue | - Where-Object { $_.Severity -ne "ParseError" } | - Get-Count | - Should -Be 4 - } + + $expected = @' +Configuration C1 { + Node localhost { + NonExistentResource X { + Ensure = '"Present"' + Name <#asdasd#> = '"RSAT"' + Other = 'Value' } } } +'@ + + $settings = New-AlignAssignmentSettings -CheckHashtable $true -AlignHashtableKvpWithInterveningComment $true + + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings | + Get-NonParseDiagnostics + $violations | Should -HaveCount 3 + + $corrected = Apply-Corrections -Original $def -Diagnostics $violations + $corrected | Should -BeExactly $expected + } -Skip:($IsLinux -or $IsMacOS) + + It 'Should ignore lines with a line continuation in DSC configuration blocks' { + $def = @' +Configuration C1 { + Node localhost { + NonExistentResource X { + Ensure = '"Present"' + Name ` + = '"RSAT"' + Other = 'Value' + } + } +} +'@ + + $expected = @' +Configuration C1 { + Node localhost { + NonExistentResource X { + Ensure = '"Present"' + Name ` + = '"RSAT"' + Other = 'Value' + } + } +} +'@ + + $settings = New-AlignAssignmentSettings -CheckHashtable $true -AlignHashtableKvpWithInterveningComment $false + + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings | + Get-NonParseDiagnostics + $violations | Should -HaveCount 1 + + $corrected = Apply-Corrections -Original $def -Diagnostics $violations + $corrected | Should -BeExactly $expected + } -Skip:($IsLinux -or $IsMacOS) + + } + + Context 'When Enum checking is disabled' { + + It 'Should not find violations in mis-aligned enums' { + $def = @' +enum E1 { + Short = 1 + Longer = 2 + Longest = 3 +} +'@ + $settings = New-AlignAssignmentSettings + + Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings | + Get-NonParseDiagnostics | + Should -BeNullOrEmpty + + } + + } + + Context 'When Enum checking is enabled' { + + It 'Should not find violations in empty single-line enum' { + $def = 'enum E1 {}' + + $settings = New-AlignAssignmentSettings -CheckEnums $true + + Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings | + Get-NonParseDiagnostics | + Should -BeNullOrEmpty + } + + It 'Should not find violations in empty multi-line enum' { + $def = @' +enum E1 { + +} +'@ + + $settings = New-AlignAssignmentSettings -CheckEnums $true + + Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings | + Get-NonParseDiagnostics | + Should -BeNullOrEmpty + } + + It 'Should not find violations in single-member, valueless, single-line enum' { + $def = 'enum E1 { Member }' + + $settings = New-AlignAssignmentSettings -CheckEnums $true + + Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings | + Get-NonParseDiagnostics | + Should -BeNullOrEmpty + } + + It 'Should not find violations in aligned single-member, explicitly valued, single-line enum' { + $def = 'enum E1 { Member = 1 }' + + $settings = New-AlignAssignmentSettings -CheckEnums $true + + Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings | + Get-NonParseDiagnostics | + Should -BeNullOrEmpty + } + + It 'Should find violations in mis-aligned single-member, explicitly valued, single-line enum' { + $def = 'enum E1 { Member = 1 }' + + $expected = 'enum E1 { Member = 1 }' + + $settings = New-AlignAssignmentSettings -CheckEnums $true + + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings | + Get-NonParseDiagnostics + $violations | Should -HaveCount 1 + + $corrected = Apply-Corrections -Original $def -Diagnostics $violations + $corrected | Should -BeExactly $expected + } + + It 'Should find violations in mis-aligned single-member, explicitly valued, multi-line enum' { + $def = @' +enum E1 { + Member = 1 +} +'@ + + $expected = @' +enum E1 { + Member = 1 +} +'@ + + $settings = New-AlignAssignmentSettings -CheckEnums $true + + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings | + Get-NonParseDiagnostics + $violations | Should -HaveCount 1 + + $corrected = Apply-Corrections -Original $def -Diagnostics $violations + $corrected | Should -BeExactly $expected + } + + It 'Should not find violations in aligned, multi-member enum' { + $def = @' +enum E1 { + Member1 = 1 + Member2 = 2 + Member3 = 3 + Member4 = 4 +} +'@ + + $settings = New-AlignAssignmentSettings -CheckEnums $true + + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings | + Get-NonParseDiagnostics + $violations | Should -BeNullOrEmpty + } + + It 'Should find violations in mis-aligned, multi-member enum' { + $def = @' +enum E1 { + Member1 = 1 + Member12 = 2 + Member123 = 3 + Member1234 = 4 +} +'@ + + $expected = @' +enum E1 { + Member1 = 1 + Member12 = 2 + Member123 = 3 + Member1234 = 4 +} +'@ + + $settings = New-AlignAssignmentSettings -CheckEnums $true + + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings | + Get-NonParseDiagnostics + $violations | Should -HaveCount 3 + + $corrected = Apply-Corrections -Original $def -Diagnostics $violations + $corrected | Should -BeExactly $expected + } + + It 'Should find violations in mis-aligned, multi-member, mixed-valued enum' { + $def = @' +enum E1 { + Member1 = 1 + Member12 + Member123 = 3 + Member1234 +} +'@ + + $expected = @' +enum E1 { + Member1 = 1 + Member12 + Member123 = 3 + Member1234 +} +'@ + + $settings = New-AlignAssignmentSettings -CheckEnums $true + + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings | + Get-NonParseDiagnostics + $violations | Should -HaveCount 1 + + $corrected = Apply-Corrections -Original $def -Diagnostics $violations + $corrected | Should -BeExactly $expected + } + + It 'Should ignore lines with intervening comments when AlignEnumMemberWithInterveningComment is false' { + $def = @' +enum E1 { + Member1 = 1 + Member12 = 2 + Member123 <#Comment#>= 3 + Member1234 = 4 +} +'@ + + $expected = @' +enum E1 { + Member1 = 1 + Member12 = 2 + Member123 <#Comment#>= 3 + Member1234 = 4 +} +'@ + + $settings = New-AlignAssignmentSettings -CheckEnums $true -AlignEnumMemberWithInterveningComment $false + + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings | + Get-NonParseDiagnostics + $violations | Should -HaveCount 2 + + $corrected = Apply-Corrections -Original $def -Diagnostics $violations + $corrected | Should -BeExactly $expected + } + + It 'Should align lines with intervening comments when AlignHashtableKvpWithInterveningComment is true' { + $def = @' +enum E1 { + Member1 = 1 + Member12 = 2 + Member123 <#Comment#>= 3 + Member1234 = 4 +} +'@ + + $expected = @' +enum E1 { + Member1 = 1 + Member12 = 2 + Member123 <#Comment#> = 3 + Member1234 = 4 +} +'@ + + $settings = New-AlignAssignmentSettings -CheckEnums $true -AlignEnumMemberWithInterveningComment $true + + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings | + Get-NonParseDiagnostics + $violations | Should -HaveCount 4 + + $corrected = Apply-Corrections -Original $def -Diagnostics $violations + $corrected | Should -BeExactly $expected + } + + It 'Should not find violations when intervening comment is already aligned and AlignEnumMemberWithInterveningComment is true' { + $def = @' +enum E1 { + Member1 = 1 + Member12 = 2 + Member123 <#Comment#> = 3 + Member1234 = 4 +} +'@ + + $settings = New-AlignAssignmentSettings -CheckEnums $true -AlignEnumMemberWithInterveningComment $true + + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings | + Get-NonParseDiagnostics + $violations | Should -BeNullOrEmpty + } + + It 'Should not find violations when intervening comment is on right of equals sign and AlignEnumMemberWithInterveningComment is true' { + $def = @' +enum E1 { + Member1 = 1 + Member12 = 2 + Member123 = <#Comment#> 3 + Member1234 = 4 +} +'@ + + $settings = New-AlignAssignmentSettings -CheckEnums $true -AlignEnumMemberWithInterveningComment $true + + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings | + Get-NonParseDiagnostics + $violations | Should -BeNullOrEmpty + } + + It 'Should ignore member with a line continuation between name and equals sign' { + $def = @' +enum E1 { + Member1 = 1 + Member12 ` + = 2 + Member123 = 3 + Member1234 = 4 +} +'@ + + $expected = @' +enum E1 { + Member1 = 1 + Member12 ` + = 2 + Member123 = 3 + Member1234 = 4 +} +'@ + + $settings = New-AlignAssignmentSettings -CheckEnums $true + + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings | + Get-NonParseDiagnostics + $violations | Should -HaveCount 2 + + $corrected = Apply-Corrections -Original $def -Diagnostics $violations + $corrected | Should -BeExactly $expected + } + + It 'Should use valueless members for alignment when IncludeValuelessEnumMembers is true' { + $def = @' +enum E1 { + Member1 = 1 + Member12 + Member123 = 3 + Member1234 +} +'@ + + $expected = @' +enum E1 { + Member1 = 1 + Member12 + Member123 = 3 + Member1234 +} +'@ + + $settings = New-AlignAssignmentSettings -CheckEnums $true -IncludeValuelessEnumMembers $true + + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings | + Get-NonParseDiagnostics + $violations | Should -HaveCount 2 + + $corrected = Apply-Corrections -Original $def -Diagnostics $violations + $corrected | Should -BeExactly $expected + } + + It 'Should not find violations where all members are valueless and IncludeValuelessEnumMembers is true' { + $def = @' +enum E1 { + Member1 + Member12 + Member123 + Member1234 +} +'@ + + $settings = New-AlignAssignmentSettings -CheckEnums $true -IncludeValuelessEnumMembers $true + + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings | + Get-NonParseDiagnostics + $violations | Should -BeNullOrEmpty + } + + } + +} \ No newline at end of file diff --git a/docs/Rules/AlignAssignmentStatement.md b/docs/Rules/AlignAssignmentStatement.md index c2709ac1a..28ca9f47f 100644 --- a/docs/Rules/AlignAssignmentStatement.md +++ b/docs/Rules/AlignAssignmentStatement.md @@ -10,38 +10,55 @@ title: AlignAssignmentStatement ## 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. +Consecutive assignment statements are more readable when they're aligned. +Assignments are considered aligned when their `equals` signs line up vertically. -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. +This rule looks at the key-value pairs in hashtables (including DSC +configurations) as well as enum definitions. + +Consider the following example which has a hashtable and enum which are not +aligned. ```powershell $hashtable = @{ - property1 = 'value' + property = 'value' anotherProperty = 'another value' } + +enum Enum { + member = 1 + anotherMember = 2 +} ``` Alignment in this case would look like the following. ```powershell $hashtable = @{ - property1 = 'value' + property = 'value' anotherProperty = 'another value' } + +enum Enum { + member = 1 + anotherMember = 2 +} ``` -The rule ignores hashtables in which the assignment statements are on the same line. For example, -the rule ignores `$h = {a = 1; b = 2}`. +The rule ignores any assignments within hashtables and enums which are on the +same line as others. For example, the rule ignores `$h = @{a = 1; b = 2}`. ## Configuration ```powershell Rules = @{ PSAlignAssignmentStatement = @{ - Enable = $true - CheckHashtable = $true + Enable = $true + CheckHashtable = $true + AlignHashtableKvpWithInterveningComment = $true + CheckEnum = $true + AlignEnumMemberWithInterveningComment = $true + IncludeValuelessEnumMembers = $true } } ``` @@ -52,8 +69,123 @@ Rules = @{ Enable or disable the rule during ScriptAnalyzer invocation. -#### CheckHashtable: bool (Default value is `$false`) +#### CheckHashtable: bool (Default value is `$true`) + +Enforce alignment of assignment statements in a hashtable and in a DSC +Configuration. There is only one setting for hashtable and DSC configuration +because the property value pairs in a DSC configuration are parsed as key-value +pairs of a hashtable. + +#### AlignHashtableKvpWithInterveningComment: bool (Default value is `$true`) + +Include key-value pairs in the alignment that have an intervening comment - that +is to say a comment between the key name and the equals sign. + +Consider the following: + +```powershell +$hashtable = @{ + property = 'value' + anotherProperty <#A Comment#> = 'another value' + anotherDifferentProperty = 'yet another value' +} +``` + +With this setting disabled, the line with the comment is ignored, and it would +be aligned like so: + +```powershell +$hashtable = @{ + property = 'value' + anotherProperty <#A Comment#> = 'another value' + anotherDifferentProperty = 'yet another value' +} +``` + +With it enabled, the comment line is included in alignment: + +```powershell +$hashtable = @{ + property = 'value' + anotherProperty <#A Comment#> = 'another value' + anotherDifferentProperty = 'yet another value' +} +``` + +#### CheckEnum: bool (Default value is `$true`) + +Enforce alignment of assignment statements of an Enum definition. + +#### AlignEnumMemberWithInterveningComment: bool (Default value is `$true`) + +Include enum members in the alignment that have an intervening comment - that +is to say a comment between the member name and the equals sign. -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. +Consider the following: + +```powershell +enum Enum { + member = 1 + anotherMember <#A Comment#> = 2 + anotherDifferentMember = 3 +} +``` + +With this setting disabled, the line with the comment is ignored, and it would +be aligned like so: + +```powershell +enum Enum { + member = 1 + anotherMember <#A Comment#> = 2 + anotherDifferentMember = 3 +} +``` + +With it enabled, the comment line is included in alignment: + +```powershell +enum Enum { + member = 1 + anotherMember <#A Comment#> = 2 + anotherDifferentMember = 3 +} +``` + +#### IncludeValuelessEnumMembers: bool (Default value is `$true`) + +Include enum members in the alignment that don't have an initial value - that +is to say they don't have an equals sign. Enum's don't need to be given a value +when they're defined. + +Consider the following: + +```powershell +enum Enum { + member = 1 + anotherMember = 2 + anotherDifferentMember +} +``` + +With this setting disabled the third line which has no value is not considered +when choosing where to align assignments. It would be aligned like so: + +```powershell +enum Enum { + member = 1 + anotherMember = 2 + anotherDifferentMember +} +``` + +With it enabled, the valueless member is included in alignment as if it had a +value: + +```powershell +enum Enum { + member = 1 + anotherMember = 2 + anotherDifferentMember +} +``` \ No newline at end of file