Relative patterns should use URIs and not fsPath for pattern matching

[jdneo]

Version: 1.46.0 (system setup)
Commit: a5d1cc2
Date: 2020-06-10T09:03:20.462Z
Electron: 7.3.1
Chrome: 78.0.3904.130
Node.js: 12.8.1
V8: 7.8.279.23-electron.0
OS: Windows_NT x64 10.0.18363

Steps to Reproduce:

1. Using the code lens sample: https://github.com/microsoft/vscode-extension-samples/tree/master/codelens-sample

2. Add a DocumentSelector when register the code lens provider
   

3. Open the folder which containing the file pattern in Remote (i.e. WSL).

4. The CodeLensProvider#provideCodeLenses() won't get hitted.

[jrieken]

Can you check if it works when not having the pattern?

[jdneo]

@jrieken Yes, the codelens works if the document selector only has scheme.

[jrieken]

Hm... The pattern specifies an absolute path and I wonder if that's the problem. On what machine should that path exist? On the local or remote machine? I'd assume they have different folder hierarchies.

Also, know that you can create a RelativePattern for a workspace root folder:

vscode/src/vs/vscode.d.ts

Line 1880 in 5b35173

constructor(base: WorkspaceFolder | string, pattern: string)

. That might be helpful.

[jdneo]

The path is on the remote machine. Actually you can just try with the code lens extension sample to repro that.

[jrieken]

The path is on the remote machine.

Ok, that explains it. The path will be evaluated on the UI side, e.g. inside the render we need to make the decision if code lens providers apply or not

[jdneo]

So take the WSL as an example, VS Code will evaluate the file's real path (which is a Windows path) to /home/ubuntu/....., which causes the Code Lens not appear, Am I right?

If that is the case, I think I need to cache the pattern and compare it inside the CodeLensProvider, instead of using the DocumentSelector.

[jrieken]

Unsure about WSL and path shapes... Maybe @aeschli can help.

[aeschli]

This should work. The code lens engine should see the document as vscode-remote://wsl+Ubuntu/home/ubuntu/... and I guess the filter should work.

[jdneo]

@jrieken @aeschli Is it possible to make the document selector for the code lens work in remote as well?

I understand that code lens belongs to the UI part but since the document selector is dealing with the file path, I think it should be treated as a 'workspace part' (just my opinion).

[jrieken]

When using this toy extension and @aeschli test resolver I get it to work (in local and remote)

	vscode.languages.registerCodeLensProvider({
		scheme: 'file',
		pattern: new vscode.RelativePattern('/Users/jrieken/Code/_samples/devfest/foo/', '**/*.txt')
	}, new class implements vscode.CodeLensProvider {

		onDidChangeCodeLenses?: vscode.Event<void> | undefined;

		provideCodeLenses(document: vscode.TextDocument, token: vscode.CancellationToken) {
			console.log('I have been called!')
			return [];
		}
	})

@jdneo From your remote test file, can you invoke Copy Path and check that it's really the right path? Alternatively you can debug into this. Set a breakpoint here, select the text file tab, and debug into the has function

[jdneo]

Hi @jrieken

I still cannot see the Code Lens in Remote: WSL. Is there anything I missed?

When no pattern specified:

When pattern specified:

[jrieken]

idk - my only suggestion would be to debug this in the aforementioned location. Adding @aeschli who does WSL and @bpasero who does glob pattern matching

[bpasero]

@jdneo I believe this could be due to the file scheme you put in. Remote resources are having a vscode-remote resource scheme as far as I know. Can you try to change your code to this instead:

const selector: DocumentSelector = {
    pattern: new RelativePattern(workspace.workspaceFolders![0], '**/*.txt')
};

So that you pick the workspace folder and not have to specify any URI.

[jdneo]

@bpasero This actually makes me more confused.

If I'm using workspace.workspaceFolders![0], no matter I have scheme: 'file' or not, the code lens will show in both cases.

And you can see the first picture I attached. If the document selector has only the scheme: 'file', the code lens will still appear in remote.

Seems that it's not because of the file scheme, but the RelativePattern.

[bpasero]

@jdneo yeah not sure, in principle you should never write the pattern as file path but use that workspace folder if you want to point to it.

I will defer to @jrieken or @aeschli to explain where this language selector is being evaluated, the code is here:

vscode/src/vs/editor/common/modes/languageSelector.ts

Line 86 in b8f63e9

if (pattern === candidateUri.fsPath || matchGlobPattern(pattern, candidateUri.fsPath)) {

But I am not sure if the file path that goes there is of vscode-remote or file scheme. I guess it is file given that you see it working now with file scheme.

One theory I have is that maybe the use of URI.fsPath converts the slashes to backslashes and thus the pattern does not match anymore because of it.

[bpasero]

Upon further investigation, I figured that this seems to be related to the fsPath usage in the languageSelector and is possibly a missing remote adoption.

The following works for me:

vscode.RelativePattern('\\home\\ticino\\workspaces\\foo', '**/*.txt')

It looks like we are having a remote WSL path that will have backslashes when fsPath is used on Windows and as such the pattern later fails to properly be detected here:

vscode/src/vs/base/common/glob.ts

Line 334 in b8f63e9

if (!extpath.isEqualOrParent(path, arg2.base)) {

I think the only solution here would be to push URI support all the way down to glob to not fall over comparing backslash with slash.

In any case, using the workspace folder is the best way out still.

[jdneo]

What if we want to resolve code lenses in part of the folders in the workspace? I tried to move the file path to the pattern but no success. 🤔

[jrieken]

One theory I have is that maybe the use of URI.fsPath converts the slashes to backslashes and thus the pattern does not match anymore because of it.

Yes, on windows fsPath is using backslash and my assumption was that glob is doing the same. A quick fix might be use path which always uses /

[bpasero]

@jrieken

A quick fix might be use path which always uses /

Yeah, though this would break any existing code that defines the path with backslashes. We could document this though, e.g. always require POSIX paths for relative glob patterns.

[jrieken]

Yeah, though this would break any existing code that defines the path with backslashes.

Not sure that I understand correctly? Do you mean uri#path or the glob pattern?

[bpasero]

@jrieken yeah so glob patterns always requires POSIX paths (though we internally treat / and \ the same). But RelativePattern allows to specify a base which we use to do a check on the path of the file and this check is not using glob patterns but simply isEqualOrParent. I would think that extensions have been putting windows and posix paths in there without thinking much about it.

I can do a fix where I support both slash and backslash, maybe that is the safest option.

[jdneo]

Tested in latest Insider and the Code Lens shows as expected.

Thank you for fixing this. :)