From e6e5856995525c808ad52547beb8fe19802207ea Mon Sep 17 00:00:00 2001 From: Connor Peet Date: Wed, 18 Dec 2024 12:39:57 -0800 Subject: [PATCH] debug: fix unexpected behaviors with duplicate `name`s in `launch.json` (#236513) Suffix duplicated launch configs with a config at the time they're read. In debug we assume the names are unique, so this should fix #231377 and probably other hidden issues as well. --- .../browser/debugConfigurationManager.ts | 61 ++++++++++++------- 1 file changed, 39 insertions(+), 22 deletions(-) diff --git a/src/vs/workbench/contrib/debug/browser/debugConfigurationManager.ts b/src/vs/workbench/contrib/debug/browser/debugConfigurationManager.ts index 0cc18f401f1..89edaf1e668 100644 --- a/src/vs/workbench/contrib/debug/browser/debugConfigurationManager.ts +++ b/src/vs/workbench/contrib/debug/browser/debugConfigurationManager.ts @@ -10,7 +10,6 @@ import { Emitter, Event } from '../../../../base/common/event.js'; import * as json from '../../../../base/common/json.js'; import { IJSONSchema } from '../../../../base/common/jsonSchema.js'; import { DisposableStore, IDisposable, dispose } from '../../../../base/common/lifecycle.js'; -import * as objects from '../../../../base/common/objects.js'; import * as resources from '../../../../base/common/resources.js'; import { ThemeIcon } from '../../../../base/common/themables.js'; import { URI as uri } from '../../../../base/common/uri.js'; @@ -27,16 +26,16 @@ import { IStorageService, StorageScope, StorageTarget } from '../../../../platfo import { IUriIdentityService } from '../../../../platform/uriIdentity/common/uriIdentity.js'; import { IWorkspaceContextService, IWorkspaceFolder, IWorkspaceFoldersChangeEvent, WorkbenchState } from '../../../../platform/workspace/common/workspace.js'; import { IEditorPane } from '../../../common/editor.js'; -import { debugConfigure } from './debugIcons.js'; -import { CONTEXT_DEBUG_CONFIGURATION_TYPE, DebugConfigurationProviderTriggerKind, IAdapterManager, ICompound, IConfig, IConfigPresentation, IConfigurationManager, IDebugConfigurationProvider, IGlobalConfig, IGuessedDebugger, ILaunch } from '../common/debug.js'; -import { launchSchema } from '../common/debugSchemas.js'; -import { getVisibleAndSorted } from '../common/debugUtils.js'; import { launchSchemaId } from '../../../services/configuration/common/configuration.js'; import { ACTIVE_GROUP, IEditorService } from '../../../services/editor/common/editorService.js'; import { IExtensionService } from '../../../services/extensions/common/extensions.js'; import { IHistoryService } from '../../../services/history/common/history.js'; import { IPreferencesService } from '../../../services/preferences/common/preferences.js'; import { ITextFileService } from '../../../services/textfile/common/textfiles.js'; +import { CONTEXT_DEBUG_CONFIGURATION_TYPE, DebugConfigurationProviderTriggerKind, IAdapterManager, ICompound, IConfig, IConfigPresentation, IConfigurationManager, IDebugConfigurationProvider, IGlobalConfig, IGuessedDebugger, ILaunch } from '../common/debug.js'; +import { launchSchema } from '../common/debugSchemas.js'; +import { getVisibleAndSorted } from '../common/debugUtils.js'; +import { debugConfigure } from './debugIcons.js'; const jsonRegistry = Registry.as(JSONExtensions.JSONContribution); jsonRegistry.registerSchema(launchSchemaId, launchSchema); @@ -509,7 +508,7 @@ abstract class AbstractLaunch implements ILaunch { ) { } getCompound(name: string): ICompound | undefined { - const config = this.getConfig(); + const config = this.getDeduplicatedConfig(); if (!config || !config.compounds) { return undefined; } @@ -518,7 +517,7 @@ abstract class AbstractLaunch implements ILaunch { } getConfigurationNames(ignoreCompoundsAndPresentation = false): string[] { - const config = this.getConfig(); + const config = this.getDeduplicatedConfig(); if (!config || (!Array.isArray(config.configurations) && !Array.isArray(config.compounds))) { return []; } else { @@ -540,21 +539,22 @@ abstract class AbstractLaunch implements ILaunch { getConfiguration(name: string): IConfig | undefined { // We need to clone the configuration in order to be able to make changes to it #42198 - const config = objects.deepClone(this.getConfig()); + const config = this.getDeduplicatedConfig(); if (!config || !config.configurations) { return undefined; } const configuration = config.configurations.find(config => config && config.name === name); - if (configuration) { - if (this instanceof UserLaunch) { - configuration.__configurationTarget = ConfigurationTarget.USER; - } else if (this instanceof WorkspaceLaunch) { - configuration.__configurationTarget = ConfigurationTarget.WORKSPACE; - } else { - configuration.__configurationTarget = ConfigurationTarget.WORKSPACE_FOLDER; - } + if (!configuration) { + return; + } + + if (this instanceof UserLaunch) { + return { ...configuration, __configurationTarget: ConfigurationTarget.USER }; + } else if (this instanceof WorkspaceLaunch) { + return { ...configuration, __configurationTarget: ConfigurationTarget.WORKSPACE }; + } else { + return { ...configuration, __configurationTarget: ConfigurationTarget.WORKSPACE_FOLDER }; } - return configuration; } async getInitialConfigurationContent(folderUri?: uri, type?: string, useInitialConfigs?: boolean, token?: CancellationToken): Promise { @@ -575,9 +575,28 @@ abstract class AbstractLaunch implements ILaunch { return content; } + get hidden(): boolean { return false; } + + private getDeduplicatedConfig(): IGlobalConfig | undefined { + const original = this.getConfig(); + return original && { + version: original.version, + compounds: original.compounds && distinguishConfigsByName(original.compounds), + configurations: original.configurations && distinguishConfigsByName(original.configurations), + }; + } +} + +function distinguishConfigsByName(things: readonly T[]): T[] { + const seen = new Map(); + return things.map(thing => { + const no = seen.get(thing.name) || 0; + seen.set(thing.name, no + 1); + return no === 0 ? thing : { ...thing, name: `${thing.name} (${no})` }; + }); } class Launch extends AbstractLaunch implements ILaunch { @@ -655,11 +674,9 @@ class Launch extends AbstractLaunch implements ILaunch { } async writeConfiguration(configuration: IConfig): Promise { - const fullConfig = objects.deepClone(this.getConfig()!); - if (!fullConfig.configurations) { - fullConfig.configurations = []; - } - fullConfig.configurations.push(configuration); + // note: we don't get the deduplicated config since we don't want that to 'leak' into the file + const fullConfig: Partial = this.getConfig() || {}; + fullConfig.configurations = [...fullConfig.configurations || [], configuration]; await this.configurationService.updateValue('launch', fullConfig, { resource: this.workspace.uri }, ConfigurationTarget.WORKSPACE_FOLDER); } }