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

Fixed erroneous PSUseDeclaredVarsMoreThanAssignments for some globals variables #2013

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

John-Leitch
Copy link

@John-Leitch John-Leitch commented Jul 11, 2024

PR Summary

This PR fixes the following longstanding issue: #1300 (Global variables are erroneously reported as "never used")

The issue exists because Helper.IsVariableGlobalOrEnvironment uses an Ast to key into VariableAnalysisDictionary, calling VariableAnalysis.IsGlobalOrEnvironment on the value returned to determine whether or not the variable is a global. However, in some cases, the key does not exist in the dictionary, resulting in the method returning false. Fortunately, VariableAnalysis.IsGlobalOrEnvironment depends on no instance members, instead relying solely on the properties of VariableExpressionAst, which is passed in. As such, VariableAnalysis.IsGlobalOrEnvironment can be made static with no consequence, allowing Helper.IsVariableGlobalOrEnvironment to call the method without the dictionary operations.

Also of note, the current implementation is somewhat redundant, with UseDeclaredVarsMoreThanAssignments.AnalyzeScriptBlockAst checking VariablePath.DriveName separately, which is why the issue does not affect environment variables despite VariableAnalysis.IsGlobalOrEnvironment erroneously returning false in some cases.

PR Checklist

@liamjpeters
Copy link
Contributor

Really clear explanation, thanks for getting to the root of the issue! 🙂 LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants