URIs in context keys

[roblourens]

We have a 'resource' context key which has the full URI of the open editor. Even though it's a URI type, it's actually used as a string:

vscode/src/vs/workbench/contrib/preferences/browser/preferences.contribution.ts

Line 254 in 204d024

when: ContextKeyExpr.and(ResourceContextKey.Resource.isEqualTo(that.environmentService.settingsResource.toString()), ContextKeyExpr.not('isInDiffEditor')),

and it only works as a string. The equals expr coerces the real URI to a string:

vscode/src/vs/platform/contextkey/common/contextkey.ts

Line 457 in 4a130c4

return (context.getValue(this.key) == this.value);

and the "in" expr, which I want to use, doesn't work with URI objects because it's checking object equality.

So I suggest that we change the type to just be a string that only contains the toStringified URI. Does that make sense or am I missing something about how this is meant to be used?

[roblourens]

Basically in #146686 I added a notebookCellResource context key which is a URI toString and I'm wondering whether to make 'resource' work the same. The alternative is changing some context expression types to know how to check URI equality.

Actually, it just occurred to me that this won't work correctly for remote EHs. The extension needs to set the key to a real URI so it gets transformed, so really we need to make the expressions know how to check URI equality (or forcibly coerce both sides to a string), right?

[roblourens]

This issue has gotten a little chaotic but here's one proposal: #147734

[roblourens]

#147926 makes it clear that we only want a string value in a context key value. To close this out, I still need a way for an extension to set a URI in a context key but have it stringified on the frontend so that it goes through uri-transformation and can be compared to something like my new notebookCellResource key. See #146686 (comment) for a usage example.

So I guess I would propose stringifying it in the setContext command at this point

vscode/src/vs/platform/contextkey/browser/contextKeyService.ts

Line 578 in 93aad56

accessor.get(IContextKeyService).createKey(String(contextKey), contextValue);

and similar to my original PR, I'm not sure whether we want to special-case URIs or if there is something more generic to do there.

[alexdima]

@roblourens I'm super sorry, but with things going in so many directions I'm having trouble understanding what the latest proposal is and the motivation behind it.

If I understand correctly, we have done a big adoption with #147926 and we have now eliminated non-primitive values from the context keys defined by VS Code core. So there will be no context keys built in to VS Code that use URIs.

setContext is exposed as a command for extensions. Coercing the passed in value to a string might cause breakage if the passed in value is read somewhere via IContextKeyService.getContextKeyValue and passed back as an argument to a command defined by the same extension. We could introduce a warning if the passed in value is not a primitive, but I'm not sure it is safe to coerce the value to a string.

Would it be possible to identify the problematic extension that calls setContext and change the extension code to pass in a string instead of a URI?

[roblourens]

Yeah this set of issues has taken some turns. cc @jrieken. I am still trying to accomplish basically what I described in #146686 (comment). Let me update that for what has happened and summarize here. If it's hard to follow, call me any morning.

• I am trying to give an extension the ability to contribute a menu button to the toolbar for a particular notebook cell. The extension contributes a menu item with "when": "notebookCellResource in jupyter.runByLineCells" where notebookCellResource is a built-in context key which is the (stringified) URI of a notebook cell
• The extension runs something like
  executeCommand('setContext', 'jupyter.runByLineCells', [cell.uri]) - they do not stringify the URI at this point because it needs to go through the URI transformer so that it will match the uri on the frontend

The problem is, that one in operator did not work for URIs. So we had this conversation about whether to support URIs or other objects as context key values, or only primitives, and it seems the intent is that context key values should only be primitives. My original idea was that the 'setContext' command would provide the service of coercing URIs to strings (or maybe any invalid value to a primitive) on the frontend so that the scenario above can work. Alternatively, we can still declare URIs to a primitive for the purposes of evaluating context key expressions.

and passed back as an argument to a command defined by the same extension

How does a context value get passed back to an extension command?

[jrieken]

My original idea was that the 'setContext' command would provide the service of coercing URIs to strings (or maybe any invalid value to a primitive) on the frontend so that the scenario above can work

I like that better than treating URIs as primitives - either way to need to traverse/massage the values given via 'setContext' because URIs would need "reviving".

How does a context value get passed back to an extension command?

Arguments are independent of context key values. When are arguments are provided than that's usually dependent on the menu. E.g the (code) editor context menu passes the URI of the current document, tree view context menus pass the tree item etc. For notebook cells menus it would make sense to pass the cell for which the menu has showing etc.

[alexdima]

The extension runs something like
executeCommand('setContext', 'jupyter.runByLineCells', [cell.uri]) - they do not stringify the URI at this point because it needs to go through the URI transformer so that it will match the uri on the frontend

Aha! Thanks for clarifying that. I agree that URIs need to cross over as URIs to benefit from transforming, so therefore we need to revive them and .toString() them here. I agree that it might be best to just add a special case for URIs in the setContext command implementation.

[roblourens]

Looks perfect, thank you!

[roblourens]

Verification:

• Run code like this in an extension

const cell = vscode.workspace.notebookDocuments[0].getCells()[0];
vscode.commands.executeCommand('setContext', 'extArrKey', [cell.document.uri]);

• Use "Inspect context keys" to verify that there is a context key with that name, and the value is a stringified URI