(GH-879) Add filtering for CodeLens and References

[Glenn Sarti (glennsarti)]

Fixes #879

Code sites

LanguageSevice.cs : HandleDefinitionRequestAsync (DefinitionRequest Handler)
LanguageSevice.cs : GetDefinitionOfSymbolAsync
- Calls EnumeratePSFiles AFTER it parses currently loaded files

LanguageSevice.cs : HandleReferencesRequestAsync (ReferencesRequest Handler)
LanguageSevice.cs : FindReferencesOfSymbolAsync
- Calls EnumeratePSFiles

This is used in the CodeLensFeature, PesterCodeLensProvider and ReferencesCodeLensProvider

ReferencesCodeLensProvider.cs : ResolveCodeLensAsync
LanguageSevice.cs : FindReferencesOfSymbolAsync
- Calls EnumeratePSFiles

Not used

LanguageSevice.cs : HandleWorkspaceSymbolRequestAsync (WorkspaceSymbolRequest Handler)
- This is weird. It only looks for symbols in the workspace OPENED files, not ALL files
in the workspace. Seems like an oversight here.

As per ba1c40e this is known broken.

---

Previously there were no tests for the Workspace.EnumeratePSFiles method. This
commit adds tests for EnumeratePSFiles method using a set of static fixture
test files.

Note that the behaviour of EnumeratePSFiles changes depending on the .Net
Framework edition

---

Previously the Workspace.EnumeratePSFiles method could only filter files
based on file extension (*.ps1, *.psm1, *.psd1). However editor settings tend
to use file glob patterns, but Editor Services did not have a library that could
parse them.

This commit:

• Updates Editor Services to use the Microsoft.Extensions.FileSystemGlobbing
  library

• Updated the build process to include the new FileSystemGlobbing DLL

• The FileSystemGlobbing library uses an abstract file system to search, not
  an actual System.IO.FileSystem object. So to implement the same error handling
  and maximum depth recursion, a WorkspaceFileSystemWrapperFactory is used to
  create the Directory and File objects needed for the globbing library

  The WorkspaceFileSystemWrapperFactory can filter on:

  • Maximum recursion depth
  • Reparse points (Note that these aren't strictly Symlinks on windows. There
    are many other types of filesystem items which are reparse points
  • File system extension
  • Gracefully ignores any file access errors

• The EnumeratePSFiles has two method signatures. One with no arguments which
  uses the Workspace object's default values and another where all arguments
  must be specified when enumerating the files

• Adds tests for the EnumeratePSFiles method to ensure that it filters on glob
  and recursion depth.

---

Previously the EnumeratePSFiles method was modified to be able to use globbing
patterns to filter workspace files. This commit

• Modifies the LanguageServerSettings class to capture the 'files' and 'search'
  Settings in order to determine the correct list of glob patterns to use when
  searching. Currently the 'files.exclude' and 'search.exclude' are merged
  together to generate the list of globs and then set the Workspace settings
  appropriately

• Uses the 'search.followSymlinks' setting to determine whether to ignore
  reparse points

Note that the LanguageClient must be configured to send these settings during
the didChangeConfiguration events otherwise it will default to include
everything.

---

Work in Progress

• Use a globbing library
• Enforce a maxdepth rescursion
• Don't care about errors. Just Carry on!
• Ignore symlinks (Partial. reparse points are too generic BUT they're Windows only so 🤷‍♂️...)
• Inject files.exclude
• More tests to ensure depth and pattern searching are legit
• Remove redundant code
• Address TODOs
• Investigate the ReferencesCodeLensProvider call sites

[Glenn Sarti (glennsarti)]

Looks like I may need to issue workspace/configuration requests to get files.exclude information as it's not within the "usual" powershell scope.

TODO - Do I get a DidChangeConfiguration Notification when I change files.exclude? Yes, BUT the changed settings in the API call are for powershell only. Need to issue additional calls to get out-of-extension scope config info.
TODO - What about search.exclude?

https://microsoft.github.io/language-server-protocol/specification#workspace_configuration

[Glenn Sarti (glennsarti)]

TylerLeonhardt Rob Holt (@rjmholt) Keith Hill (@rkeithhill)

So I've reached a bit of an impasse. The EnumeratePSFiles method can now take include and exclude globs etc. however getting the globs from the Language Client is a little curious.

So far I see a couple of options

1. The Language Server sends a request to client to request the current configuration
   https://microsoft.github.io/language-server-protocol/specification#workspace_configuration

This could be ok for VSCode, but PSES should be editor agnostic therefore it could be 'files.exclude' on VSCode and 'exclude.files' of Atom (or something else). The advantage for this is that it's a Lang. Server. only change so it doesn't require any client changes.

2. Pass the exclude glob on the command when starting PSES

It would work, but if the user changes it, it will require PSES to restart. Not a great UX

3. The Language Server will depend on the Language Client to send the configuration using a custom notification/request

This would make the client responsible for updating the server, which may be ok?. It does mean that until the server receives the client message it will use the "defaults"

Any thoughts on what would be the best way to proceed?

[TylerLeonhardt]

That's a very good question...

Quick question... does number 1 only happen at startup and when values are changed?

I like number 1 personally... because we might be able to make it a powershell extension specific config like:

powershell.codelens.references.exclude

which could somehow inherit from files.exclude (maybe?)

[Glenn Sarti (glennsarti)]

In my testing, VSCode will send a DidChangeConfiguration notification for any settings change, but it will only send the language specific settings in the payload (in our case powershell).

My thinking was to send a workspace/configuration request whenever I receive a DidChangeConfiguration notification, kind of like an out-of-band request. However the configuration settings I request are just text strings and like I said, my not be consistent across editors.

As per your comment:

I like number 1 personally... because we might be able to make it a powershell extension specific config like:

powershell.codelens.references.exclude

Because that settings is within the language "namespace" it will appear in the DidChangeConfiguration` notification so there's no need to request more information.

[TylerLeonhardt]

Glenn Sarti (@glennsarti) can listen for files.exclude changes on the client side?

vscode.workspace.onDidChangeConfiguration(evt => {
   if (evt.affectsConfiguration('files.exclude')) {
     console.log('configuration updated');
     console.log(vscode.workspace.getConfiguration(undefined, null).get('files.exclude'));
   }
});

Then maybe we update our powershell.codelens.references.exclude from there

[Glenn Sarti (glennsarti)]

Then maybe we update our powershell.codelens.references.exclude from there

That's the hard bit. Do you change the settings in the user's settings? workspace settings? per language settings?

There is an update method in WorkspaceConfiguration From the docs and what I know, there is no "in process" view where an extension could inject volatile settings. There is an inspect method which may be useful.

This is kinda why I was leaning towards making the language client send a notification and be as "dumb" as possible eg.

  PowerShellLanguageClientExtraSettings : {
    'filesExclude': [ 'glob1', 'glob2'],
    'searchExclude': [ 'glob1', 'glob2'],
  }

Then let the Language Server decide when and what to do with it e.g.
It could smash together filesExclude and searchExclude and powershell.codelens.references.exclude to generate a full list.

Side note - This doesn't just affect codelens either, also affects Peek and Goto Definitions and Workspace Symbols. Perhaps powershell.excludeWorkspaceFiles = ['glob1', 'glob2']

[Glenn Sarti (glennsarti)]

From TylerLeonhardt

---

A.. simpler-to-implement option that doesn’t fix everything is having a separate setting:
powershell.files.excludes

and the default value of the setting is files.excludes

but naturally the user would need to be educated to include what’s in their files.excludes if they make changes to powershell.files.excludes

[Glenn Sarti (glennsarti)]

OOo...super interesting...You can also call commands

https://code.visualstudio.com/docs/editor/variables-reference#_command-variables

[TylerLeonhardt]

Right so the default would be the value of files.exclude...

I think the UX is that, you would see a box in the settings pane that gives you the default value (rather than starting from an empty box)...

So they would just tack on to the list the things they want to ignore.

This means that if they change files.exclude in the future, those changes wouldn't be in there since they specified a value for our setting.

This... Might be the way to go.

[Glenn Sarti (glennsarti)]

impasse surmounted. Language Client can send other settings on the OnDidConfigurationChange notification

[Glenn Sarti (glennsarti)]

Bouncing the PR due to the flaky debounce tests

Starting test execution, please wait...
ERROR: [xUnit.net 00:00:01.9798911]     Microsoft.PowerShell.EditorServices.Test.Protocol.Server.OutputDebouncerTests.OutputDebouncerAggregatesOutputEvents [FAIL]
At C:\projects\powershelleditorservices\PowerShellEditorServices.build.ps1:379 char:16
+ ...      exec { & $script:dotnetExe test -f $script:TestRuntime.Core (Dot ...
+                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
At C:\projects\powershelleditorservices\PowerShellEditorServices.build.ps1:371 char:1

[Glenn Sarti (glennsarti)]

Look at the code lens call sites for FindReferencesOfSymbolAsync they all seem legit and ok. Will cleanup the PR and then it's ready for merge.

[TylerLeonhardt]

Not sure how I feel about Codacy commenting all over PRs...

I think I can disable that and keep the gate at least.

[TylerLeonhardt]

Glenn Sarti (@glennsarti) let me know if there are any more outstanding questions from your side. I think the 2 concerns are settled.

[TylerLeonhardt]

Go ahead and rebase from master so that we can run CI (in Azure DevOps now!)

[Glenn Sarti (glennsarti)]

Rebased

[TylerLeonhardt]

Glenn Sarti (@glennsarti) are you waiting on us now?

[Glenn Sarti (glennsarti)]

TylerLeonhardt Yes, although I now see a merge conflict. I'll fix that up soon enough

[Glenn Sarti (glennsarti)]

Rebased

[Rob Holt (rjmholt)]

As far as I know, / should work on all OSes we support. .NET's FS APIs interpret it as the same as \ I'm pretty sure.

[Glenn Sarti (glennsarti)]

Rob Holt (@rjmholt) Unfortunately I can't add a comment to this ...

As far as I know, / should work on all OSes we support. .NET's FS APIs interpret it as the same as \ I'm > pretty sure.

So technically the output is from the Globbing library, not the System.IO... namespace so it's not the same. However, I noticed that the filepaths are being converted to URI when output from the Language Server so any \ is already being converted there. I'll add a maint commit because strictly speaking this is changing the behaviour of the EnumeratePSFiles method

[TylerLeonhardt]

LGTM (+nits) looking forward to getting this in, Glenn Sarti (@glennsarti)!

[Rob Holt (rjmholt)]

Looks really good! Thanks Glenn Sarti (@glennsarti)!

[Glenn Sarti (glennsarti)]

Rob Holt (@rjmholt) TylerLeonhardt Nits and changes addressed.

[TylerLeonhardt]

Patrick Meinecke (@SeeminglyScience) did you want to give this one more look since you requested changes a while back? Otherwise, we can merge.

[TylerLeonhardt]

Awesome work Glenn :)