-
Notifications
You must be signed in to change notification settings - Fork 748
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
Improve performance of NoUnusedVariablesRule & NoUnusedParametersRule #8853
Conversation
ed44a01
to
8fdd65f
Compare
@@ -22,7 +22,6 @@ va | |||
// unassigned variable | |||
var foo | |||
//@[04:07) [BCP028 (Error)] Identifier "foo" is declared multiple times. Remove or rename the duplicates. (CodeDescription: none) |foo| | |||
//@[04:07) [no-unused-vars (Warning)] Variable "foo" is declared but never used. (CodeDescription: bicep core(https://aka.ms/bicep/linter/no-unused-vars)) |foo| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is because I removed the OfType<VariableAccessSyntax>()
filter when looking for references. Technically foo
is being used in this file, but incorrectly as a function foo()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚢
// TODO: Performance: Use a visitor to visit VariableAccesssyntax and collects the non-error symbols into a list. | ||
// Then do a symbol visitor to go through all the symbols that exist and compare. | ||
// Same issue for unused-params rule. | ||
var invertedBindings = model.Binder.Bindings.ToLookup(kvp => kvp.Value, kvp => kvp.Key); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nit]: Since all the NoUnused*Rule
s use the same binding inversion, would it make sense to just expose this as a property of the binder? It looks like a cheap thing to calculate, but it could be done once and cached as an ImmutableDictionary<Symbol, ImmutableArray<SyntaxBase>>
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think in the long run, we'll want a more robust replacement for SemanticModel.FindReferences()
, but it'll have to be semanticmodel-driven rather than binder-driven (for example, this approach wouldn't work for namespaced functions, as the binder won't bind them to a FunctionSymbol
).
If I have time, I'll give this a go. For now, I didn't want to encourage this pattern in other places by making it available through the IBinder
interface.
Noticed when implementing #8844 -
NoUnusedVariablesRule
is consuming ~34% total CPU time when running benchmarks. This PR drops that down to ~2.5%.Microsoft Reviewers: Open in CodeFlow