Resurrect support to resolve aliased references #1656
Conversation
In ancient times, commit 633e36b implemented support for aliases when finding references. This was removed during the OmniSharp rewrite because it needed the pipeline thread, which was not yet available. With that work complete, this can now be resurrected.
andyleejordan
force-pushed
the
andschwa/resolve-aliases
branch
from
d7e4fb6 to
0e9ea5a
Compare
src/PowerShellEditorServices/Services/PowerShell/Utility/CommandHelpers.cs
Outdated
Show resolved
Hide resolved
src/PowerShellEditorServices/Services/Symbols/Vistors/AstOperations.cs
Outdated
Show resolved
Hide resolved
src/PowerShellEditorServices/Services/Symbols/Vistors/FindReferencesVisitor.cs
Outdated
Show resolved
Hide resolved
src/PowerShellEditorServices/Services/Symbols/Vistors/FindReferencesVisitor.cs
Outdated
Show resolved
Hide resolved
andyleejordan
force-pushed
the
andschwa/resolve-aliases
branch
2 times, most recently
from
4194473 to
84ee6b2
Compare
| s_cmdletToAliasCache.AddOrUpdate( | ||
| aliasInfo.Definition, | ||
| (_) => new List<string> { aliasInfo.Name}, | ||
| (_, v) => { v.Add(aliasInfo.Name); return v; }); |
There was a problem hiding this comment.
This would beat the purpose, as these 2 lambdas are closures and thus new instances will be created for every aliasInfo object, so it will end up with more allocations.
The overload I suggested to use will generate static delegates can are reused for all the calls to AddOrUpdate here, and thus could reduce allocations.
There was a problem hiding this comment.
The suggested code didn't compile...what's the fix for it?
There was a problem hiding this comment.
I see this let me see if I can get it right.
There was a problem hiding this comment.
My suggestion is pseudo code :) I didn't try compiling it. I guess the following would work
s_cmdletToAliasCache.AddOrUpdate(
aliasInfo.Definition,
(_, arg) => new List<string> { arg },
(_, v, arg) => { v.Add(arg); return v; },
aliasInfo.Name);
There was a problem hiding this comment.
I'm getting this:
No overload for method 'AddOrUpdate' takes 4 arguments
Which is why I made the change I did instead. But I agree the documentation says this overload exists and takes four arguments, and we're using .NET 6 here.
There was a problem hiding this comment.
I see, so PSES is targeting netstandard2.0?
In that case, please keep your original code unchanged.
There was a problem hiding this comment.
we're using .NET 6 here
Just so I'm clear :) we are using .NET 6 SDK but the build targets netstandard 2.0, is that right?
There was a problem hiding this comment.
@daxian-dbw Since we're using netstandard2.0 and net461 in order to continue to support Windows PowerShell 5.1, I can't use this overload 😢
There was a problem hiding this comment.
That's OK. It's an optimization change anyway 😄
There was a problem hiding this comment.
I've added a TODO to improve this with the better delegate if/when we move to .NET 4.7.2 and/or .NET Standards 2.1 (so really, the day we drop PowerShell 5.1 support).
This consolidated the constructors so we don't have some unused (and confusing) boolean differentiating them, and instead just provide one constructor with default values. Also clean up `AstOperationsTests`.
Ironically this was clearly how it used to work, and was for some reason changed, even though it does need to be a task and was previously relying on synchronous side effects.
Which is where it might be accessed concurrently as a cache, but we can use `IDictionary` (and return a copy of the caches) to the rest of the users.
andyleejordan
force-pushed
the
andschwa/resolve-aliases
branch
from
84ee6b2 to
627e915
Compare
|
Thanks for your time reviewing @daxian-dbw! This resolves the last disabled test from the original set of Language Server tests. |
This work was spawned by a skipped unit test that "indicated a bug in the product code." The bug was that the support was removed, but could be brought back fairly simply now that the pipeline threading work is finished.