Skip to content
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

PSAvoidAssignmentToAutomaticVariable triggers on property assignment of automatic variable #1012

Closed
SeeminglyScience opened this issue Jun 4, 2018 · 3 comments · Fixed by #1008
Assignees
Milestone

Comments

@SeeminglyScience
Copy link
Contributor

Steps to reproduce

Invoke-ScriptAnalyzer -ScriptDefinition '$ExecutionContext.SessionState.Module.OnRemove = {}'

Expected behavior

No output.

Actual behavior

RuleName                            Severity     ScriptName Line  Message
--------                            --------     ---------- ----  -------
PSAvoidAssignmentToAutomaticVariable Error                   1     The Variable 'ExecutionContext' cannot be assigned
                                                                  since it is a readonly automatic variable that is
                                                                  built into PowerShell, please use a different name.

Environment data

> $PSVersionTable
 
Name                           Value                                                                                                                                                         
----                           -----                                                                                                                                                         
PSVersion                      5.1.16299.251                                                                                                                                                 
PSEdition                      Desktop                                                                                                                                                       
PSCompatibleVersions           {1.0, 2.0, 3.0, 4.0...}                                                                                                                                       
BuildVersion                   10.0.16299.251                                                                                                                                                
CLRVersion                     4.0.30319.42000                                                                                                                                               
WSManStackVersion              3.0                                                                                                                                                           
PSRemotingProtocolVersion      2.3                                                                                                                                                           
SerializationVersion           1.1.0.1

> (Get-Module -ListAvailable PSScriptAnalyzer).Version | ForEach-Object { $_.ToString() }
1.17.0
1.13.0
@bergmeister
Copy link
Collaborator

bergmeister commented Jun 4, 2018

@SeeminglyScience This sounds more like a special case that needs human judgement and should therefore be suppressed by the user rather than having to convolute the code with lots of special cases and maintaining it (especially should behaviour change in a different PS version).
For example the following demonstrates that the majority of properties are still read-only:

PS > $ExecutionContext.SessionState.UseFullLanguageModeInDebugger = $true
'UseFullLanguageModeInDebugger' is a ReadOnly property.
At line:1 char:1
+ $ExecutionContext.SessionState.UseFullLanguageModeInDebugger = $true
+ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ CategoryInfo          : InvalidOperation: (:) [], RuntimeException
+ FullyQualifiedErrorId : PropertyAssignmentException

What are your thoughts?
@JamesWTruher

@SeeminglyScience
Copy link
Contributor Author

Well, there's only a couple assignable AST types that would hold a variable in a way this rule would be interested in. Here's a bit of code I wrote for something else with the same goal.

internal class VariableOps
{
    internal static IEnumerable<VariableExpressionAst> GetVariablesFromAssignment(AssignmentStatementAst assignmentStatementAst)
    {
        if (assignmentStatementAst.Left is ArrayLiteralAst arrayLiteral)
        {
            return GetVariablesFromArrayLiteral(arrayLiteral);
        }

        var singleVariable = GetVariableFromSingleAssignableAst(assignmentStatementAst.Left);
        if (singleVariable == null)
        {
            return Enumerable.Empty<VariableExpressionAst>();
        }

        return new[] { singleVariable };
    }

    private static VariableExpressionAst GetVariableFromSingleAssignableAst(ExpressionAst expressionAst)
    {
        if (expressionAst is VariableExpressionAst variableExpression)
        {
            return variableExpression;
        }

        if (expressionAst is ConvertExpressionAst convertExpression)
        {
            return GetVariableFromSingleAssignableAst(convertExpression.Child);
        }

        if (expressionAst is AttributedExpressionAst attributedExpression)
        {
            return GetVariableFromSingleAssignableAst(attributedExpression.Child);
        }

        return null;
    }

    private static IEnumerable<VariableExpressionAst> GetVariablesFromArrayLiteral(ArrayLiteralAst arrayLiteralAst)
    {
        for (var i = 0; i < arrayLiteralAst.Elements.Count; i++)
        {
            var foundVariable = GetVariableFromSingleAssignableAst(arrayLiteralAst.Elements[i]);
            if (foundVariable != null)
            {
                yield return foundVariable;
            }
        }
    }
}

Catches [pstypename()] $Host = $null, [int] $Host = 0, $Host, $ExecutionPolicy = $null or any combination of them. More code than Ast.FindAll but more explicit and probably a bit faster.

For example the following demonstrates that the majority of properties are still read-only:

Of $ExecutionContext yeah. Here's a more common example found in a lot of profiles

Invoke-ScriptAnalyzer -ScriptDefinition '$Host.PrivateData["ErrorBackgroundColor"] = "Black"'

@bergmeister
Copy link
Collaborator

bergmeister commented Jun 4, 2018

Hmm, yes maybe you are right and it is better to just exclude all properties on automatic variables. I will have a think overnight, I fixed your other raised issue as part of PR 1008 by the way.
@SeeminglyScience I had a quick chat with James and we decided to fix this as well, PR is below

bergmeister added a commit to bergmeister/PSScriptAnalyzer that referenced this issue Jun 4, 2018
@bergmeister bergmeister added this to the 1.17.1 milestone Jun 4, 2018
@bergmeister bergmeister self-assigned this Jun 4, 2018
bergmeister added a commit that referenced this issue Jun 5, 2018
… when assigning a .Net property and only look at the LHS (#1008)

* Fix NullReferenceException in AvoidAssignmentToAutomaticVariable rule when assigning a net property to a .Net property

* when variableExpressionAst is null, then continue loop to reduce nesting

* fix indentation for cleaner diff

* Fix issue #1013 as well by making sure the rule looks only the LHS

* Fix issue #1012 as well

* Simplify test that checks that PSSA does not throw to address PR review
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment