Skip to content

Do not trigger UseDeclaredVarsMoreThanAssignment for variables being used via Get-Variable #925

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 21 additions & 9 deletions Rules/UseDeclaredVarsMoreThanAssignments.cs
Original file line number Diff line number Diff line change
@@ -1,12 +1,5 @@
// 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.
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

using System;
using System.Collections.Generic;
Expand All @@ -16,6 +9,7 @@
#endif
using System.Globalization;
using Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic;
using System.Linq;

namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules
{
Expand Down Expand Up @@ -204,6 +198,24 @@ private IEnumerable<DiagnosticRecord> AnalyzeScriptBlockAst(ScriptBlockAst scrip
}
}

// Detect usages of Get-Variable
var getVariableCmdletNamesAndAliases = Helper.Instance.CmdletNameAndAliases("Get-Variable");
IEnumerable<Ast> getVariableCommandAsts = scriptBlockAst.FindAll(testAst => testAst is CommandAst commandAst &&
getVariableCmdletNamesAndAliases.Contains(commandAst.GetCommandName(), StringComparer.OrdinalIgnoreCase), true);
foreach (CommandAst getVariableCommandAst in getVariableCommandAsts)
{
var commandElements = getVariableCommandAst.CommandElements.ToList();
// The following extracts the variable name only in the simplest possibe case 'Get-Variable variableName'
if (commandElements.Count == 2 && commandElements[1] is StringConstantExpressionAst constantExpressionAst)
{
var variableName = constantExpressionAst.Value;
if (assignments.ContainsKey(variableName))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a comment indicating this is case insensitive? (I missed that until I saw the entire file - maybe it's ok)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments can get out of date quickly, therefore I renamed the dictionary variable assignments to assignmentsDictionary_OrdinalIgnoreCase.

{
assignments.Remove(variableName);
}
}
}

foreach (string key in assignments.Keys)
{
yield return new DiagnosticRecord(
Expand Down
5 changes: 5 additions & 0 deletions Tests/Rules/UseDeclaredVarsMoreThanAssignments.tests.ps1
Original file line number Diff line number Diff line change
Expand Up @@ -73,5 +73,10 @@ function MyFunc2() {
It "returns no violations" {
$noViolations.Count | Should -Be 0
}

It "Using a variable via 'Get-Variable' does not trigger a warning" {
$noViolations = Invoke-ScriptAnalyzer -ScriptDefinition '$a=4; get-variable a' | Where-Object { $_.RuleName -eq $violationName }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is the where-object needed?
could this just be

Invoke-ScriptAnalyzer -ScriptDefinition '$a=4;Get-Variable a' | Should -BeNullOrEmpty

Copy link
Collaborator Author

@bergmeister bergmeister Mar 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the Where-Object now since this seems to be your preferred approach.

$noViolations.Count | Should -Be 0
}
}
}