Allow EnvironmentVariableCollection API to apply changes via shell integration

[Tyriar]

A problem the Python extension has is that it sets environment variables but they get overridden during shell startup scripts. This issue would allow adding an option to the variable mutator for the timing a particular environment variable is applied.

[karrtikr]

@Tyriar FYI none of the integration scripts work if VSCODE_ENV_* variable is set, each shell fails with its own type of error.

[sandy081]

Moving this to next milestone as this is a feature request and we are late in the release cycle to make this happen.

[Tyriar]

Proposal:

export enum EnvironmentVariableMutatorTiming {
	/**
	 * Apply to the environment just before the process is created.
	 */
	ProcessCreation = 1,

	/**
	 * Apply to the environment in the shell integration script. Note that this _will not_ apply
	 * the mutator if shell integration is disabled or not working for some reason.
	 */
	ShellIntegration = 2,

	/**
	 * Apply to the environment just before the process is created and then again in the shell
	 * integration script. Note that this _will not_ apply the mutator if shell integration is
	 * disabled or not working for some reason.
	 */
	Both = 3
}

export interface EnvironmentVariableMutatorOptions {
	/**
	 * When the mutator will be applied to the environment.
	 */
	timing?: EnvironmentVariableMutatorTiming;
}

/**
 * A type of mutation and its value to be applied to an environment variable.
 */
export interface EnvironmentVariableMutator {
	/**
	 * Options applied to the mutator.
	 */
	readonly options?: EnvironmentVariableMutatorOptions;
}

export interface EnvironmentVariableCollection extends Iterable<[variable: string, mutator: EnvironmentVariableMutator]> {
	/**
	 * @param options Options applied to the mutator.
	 */
	replace(variable: string, value: string, options?: EnvironmentVariableMutatorOptions): void;

	/**
	 * @param options Options applied to the mutator.
	 */
	append(variable: string, value: string, options?: EnvironmentVariableMutatorOptions): void;

	/**
	 * @param options Options applied to the mutator.
	 */
	prepend(variable: string, value: string, options?: EnvironmentVariableMutatorOptions): void;
}

The timing property will be implemented by the specially handled VSCODE_ENV_* variables.

[Tyriar]

Feedback from API sync:

• "Timing" makes it seems ms/performance-related. Some other ideas: phase, stage, sequence. Think about it and look at existing APIs for something similar
• timing should be readonly
• Make values on EnvironmentVariableMutator non-falsy and initialize with defaults instead of undefined

[Tyriar]

Make values on EnvironmentVariableMutator non-falsy and initialize with defaults instead of undefined

I couldn't find many examples of similar things, and when I did it's optional everywhere like in ProcessExecutionOptions:

vscode/src/vscode-dts/vscode.d.ts

Lines 7538 to 7572 in a9d434a

	export class ProcessExecution {
	/**
	* Creates a process execution.
	*
	* @param process The process to start.
	* @param options Optional options for the started process.
	*/
	constructor(process: string, options?: ProcessExecutionOptions);
	/**
	* Creates a process execution.
	*
	* @param process The process to start.
	* @param args Arguments to be passed to the process.
	* @param options Optional options for the started process.
	*/
	constructor(process: string, args: string[], options?: ProcessExecutionOptions);
	/**
	* The process to be executed.
	*/
	process: string;
	/**
	* The arguments passed to the process. Defaults to an empty array.
	*/
	args: string[];
	/**
	* The process options used when the process is executed.
	* Defaults to undefined.
	*/
	options?: ProcessExecutionOptions;
	}

I'll keep it as is for now as we can make it non-undefined later in a non-breaking change.

[Tyriar]

Updated proposal after feedback:

export interface EnvironmentVariableMutatorOptions {
	/**
	 * Apply to the environment just before the process is created.
	 * 
	 * Defaults to true.
	 */
	applyAtProcessCreation?: boolean;
	
	/**
	 * Apply to the environment in the shell integration script. Note that this _will not_ apply
	 * the mutator if shell integration is disabled or not working for some reason.
	 * 
	 * Defaults to false.
	 */
	applyAtShellIntegration?: boolean;
}

/**
 * A type of mutation and its value to be applied to an environment variable.
 */
export interface EnvironmentVariableMutator {
	/**
	 * Options applied to the mutator.
	 */
	readonly options: EnvironmentVariableMutatorOptions;
}

export interface EnvironmentVariableCollection extends Iterable<[variable: string, mutator: EnvironmentVariableMutator]> {
	/**
	 * @param options Options applied to the mutator.
	 */
	replace(variable: string, value: string, options?: EnvironmentVariableMutatorOptions): void;

	/**
	 * @param options Options applied to the mutator.
	 */
	append(variable: string, value: string, options?: EnvironmentVariableMutatorOptions): void;

	/**
	 * @param options Options applied to the mutator.
	 */
	prepend(variable: string, value: string, options?: EnvironmentVariableMutatorOptions): void;
}

Comments:

• I split out the timing enum into 2 booleans as that better future proofs us (what would Both mean if we had a 3rd timing?) and is simpler in a way.
• For EnvironmentVariableMutator.options we can have it return a Required<EnvironmentVariableMutatorOptions>, I don't see us using Required<> anywhere in the API though so I'll leave is as the less strict EnvironmentVariableMutatorOptions.

[Tyriar]

Currently blocked on the changes to workspace scope moving to a EnvironmentVariableCollection function as replace, append, prepend prototypes conflict.

[Tyriar]

TPI #182970

[Tyriar]

Question for API sync: #182883 (review)

[Tyriar]

Feedback to bring to sync:

• Should the default of applyAtProcessCreation be false?

[karrtikr]

Another option: Rename it to doNotApplyAtProcessCreation and keep default as false.

[Tyriar]

Bug report for zsh: #188875
Bug report for fish: #184009

[Tyriar]

The bugs don't block this as they shouldn't change the API shape.

[Tyriar]

We do need to finalize how the defaults work for applyAtProcessCreation though.

[Tyriar]

Let's treat undefined as false and update the jsdoc to explain this. Good to finalize

[karrtikr]

Particularly for Python extension, it's easy to miss to set or not set it when replacing or prepending:

envVarCollection.replace(
    'PATH',
    `${path.dirname(interpreter.path)}${path.delimiter}${process.env[pathVarName]}`,
    { applyAtShellIntegration: true, applyAtProcessCreation: true },
);

or

envVarCollection.prepend('PS1', value, {
    applyAtShellIntegration: true,
    applyAtProcessCreation: false,
});

Maybe we should consider making it a required parameter as it is always something which we look at.

[Tyriar]

@karrtikr the options arg can't be required due to backwards compatibility, the individual option properties should be required as then it would be inconsistent if we added a new property (which must be optional).

I think the undefined is treated as false thing fixes most of the API ux problems.

[karrtikr]

To be clear, I'm suggesting just to make applyAtProcessCreation required when using options. As that is the confusing property which changes value when options is used.