Vscode: Consolidate variable substitution between debug and tasks

[weinand]

Currently we have (at least) two places where we substitute variables in jsons. One for regular variables (which are used in launch.json and tasks.json) and another one for command variables (only available in launch.json).

We should consolidate this:

• There should be only a single implementation for variable substitution (and it should not make a difference whether the variable comes from a command execution or from an environment variable).
• Command variables should be available for both launch.json and task.json (that means we have to generalise the 'variables' contribution point).
• variable substitution should be robust: special characters in a variable name should not break the substitution.

@isidorn please add more ...

[isidorn]

After looking into this it is a bigger change that @dbaeumer should also be included in.
Thus pushing to August.

[dbaeumer]

@isidorn I think we push that to September :-)

[bpasero]

@weinand @dbaeumer @isidorn (also @DonJayamanne fyi) I suggest we meet on this topic in September (maybe when Isi is back) to discuss how this could be supported centrally from the configuration service.

I have some concerns how this could work if we decide to push this down to the configuration service (where imho it would make sense to have it). My understanding is that:

• there are variables that have static values from either the process environment or some other globals in the context you are in (e.g. workspace path)
• there are variables which are dynamic based on the active opened file, file extension
• there are variables which are computed by running a contributed command

Now, the way the configuration service is being used is to give you access to the entire configuration JSON object (in the case of debug or tasks, scoped to launch.json or tasks.json). We currently do NOT have an API to give you the value of a specific key. It is not clear to me when and how variable substitution should take place. Obviously it cannot take place once on startup when the configuration is loaded because of the dynamic variables. Basically it would need to take place every time the configuration is being asked for, which can be rather expensive. Also, I think command execution is asynchronous, but we made sure that retrieving the configuration always returns synchronous and we should not go back to async imho. We could solve this by introducing a special method that for a given key returns its value with all variables replaced. But that is very inconsistent compared to getting the full JSON object and working with it.

Then, some configuration values are complex objects. Would we support variables inside these objects? I think the only place where variables should be supported is in simple string values.

Please let me know how you think this should work given the constraints of the configuration service.

[isidorn]

@bpasero your understanding regarding the variables is right, there are 3 'different types': static value from the env which are accessed by ${env.NAME}, dynamic based on workspace ${NAME} and ones based on commands ${command.NAME}.

Currently we also support variable substitution in nested objects. Common use case that the users asked for is substitution inside an env object in launch.json.

Could we leave the current API synchronous which does not do any replacment and introduce some asynchronous API which also returns the full JSON but with all the variables replaced? Do you find this too ugly?

These commands need to be replaced exactly at the time the configuration is asked for. E.g. we need to ask the user what process to connect every time he starts debugging (and then that replaces a variable in launch.json). Due to that I only see my previous proposal as a potential sollution at the moment.

[bpasero]

Yes, very ugly to do have a sync call without replacement and async with replacement...

[isidorn]

Because we support the command replacment the method which returns the replaced variables has to be async. Now how to make that API not very ugly is an open question.

[DonJayamanne]

@bpasero , I agree with @isidorn . Further to his comments, variable substitution with nested objects is also used by the debugger, when referencing settings from settings.json in launch.json.
For instance in my python extension i have the setting "python.pythonPath" = "abc". This value is accessed in launch.json as follows: ${config}.python.pythonPath.

[dbaeumer]

@bpasero I would not bake variable substitution into the configuration service. This makes things IMO very complicated and expensive in general. I would go down the path of saying the configuration service gives you exactly the values how they are in the corresponding json file. If a component has a setting that the component knows has variables in it we should offer another 'service' which does the replacement.

Otherwise a statement like configService.set('myKey', configService.get('myKey')) would alter the setting if the setting has a variable. Even worse the variable would be lost.

[bpasero]

After talking to @dbaeumer and @weinand I lean towards a new service that is able to resolve variables in configuration values and provides an API that is long running (because it potentially executes commands). That way the base configuration service stays as it is today and returns the truth as it is in the files (solves Dirks point on get/set issues).

This will solve the issue in debug and task land that both use different ways of resolving variables.

Wether or not we expose this new service to extensions is up for debate. If extensions only want the static variable replacement we could just offer a NPM module that extensions can require to get this functionality.

[DonJayamanne]

Wether or not we expose this new service to extensions is up for debate. If extensions only want the static variable replacement we could just offer a NPM module that extensions can require to get this functionality.

I would use such a module in my Python extension. Currently I have a number of settings where users can enter paths (e.g. arguments to some tools, etc). Here I have added support for variables such as ${workspaceRoot}.

Currently I enabled the use of ${workspaceRoot} by having my own copy of the SystemVariables class in the extension.

[weinand]

Variable substitution for dynamic variables cannot be done on the lowest layer because dynamic variables are long running and potentially require access to the UI. Triggering variable substitution can only be done in a context where the user expects UI popping up and is able to respond to it. In debug and tasks land this is when "launching" a debug session or "running" a task.

For this reason I think debug and tasks would have to introduce an additional way of resolving dynamic variables (on top of the already resolved system variables). But it would be great if we could reuse a "resolve" utility provided by the configurations service.

[bpasero]

All reuse can happen from the new resolver service that should also provide library functions if needed for others to use.

[bpasero]

Another idea that popped up after talking to @aeschli is to remove all knowledge of task.json and launch.json from the IConfigurationService and move it into a new service (that could well be the same service that also does the variables substitution).

Reasoning is that it would make the configuration service API easier with regards to writing (@jrieken fyi). Currently it is not clear how to support writing to tasks.json and launch.json from the same API that allows to write to global and workspace settings.

Since there are probably no clients of the configuration in tasks and debug land outside of tasks and debug, this change would be easy to adopt.

[weinand]

@bpasero Yes, task.json and launch.json are a different kind of beast than settings and I do not see a need to surface them under the IConfigurationService.

[jrieken]

Problem is that it would break the extension API which currently does the same IConfigurationService does - exposing everything with values as authored...

[bpasero]

Then maybe we should deprecate getConfiguration() and introduce new APIs to deal with configuration (global, workspace) and configuration in debug/tasks.

[jrieken]

Even if we deprecate the function it must still behave the same (not semantic breakage). Given that and the fact one ever complained I an eager on adding more API for that. The write/inspect challenge is real but we should come up with a _compatible_ solution

[bpasero]

It would be relatively easy to support the old way of the API by merging the results of the configuration with the values from debug and task (from the new service).

I am also fine leaving the current API as it is but I am leaning towards changing the (internal workbench) API to take out debug/tasks from configuration land and move it to the new service which also takes care of variables substitution.

[dbaeumer]

Even if we move out the tasks.json and launch.json to a separate service I opt for not mixing reading and writing these settings with variable substation. It simply complicates things since you need methods to read the raw value and the substituted value. The following code MUST result in a null op `config.set('key', config.get('key'));

And I do have requests were extension writers want to contribute to the tasks.json. This will get prominent when tasks will support more than one command. Then for example a CPP extension could contribute a task for running make into an already existing tasks.json file. Would be great if the CPP extension had API to do so.

In my opinion the variable substitution is best done by the task or debug service since they know what they do and they know how to optimize certain things (for example they can cache some values instead of recomputing them all the time).

[isidorn]

• created configurationResolverService that is now used by debug and tasks
• updated our unit tests to now work with configurationResolverService
• configurationResovlerService will also resolve ${env.name} and ${config.name} variables
• configurationResolverService is now in charge of resolving interactive variables