re-escape choice elements when checking for bogous snippets. fixes #31521

A marker now has a `toTextmateString` method which must return the escaped input string to generate itself.

src/vs/editor/contrib/snippet/browser/snippetParser.ts

@@ -129,14 +129,6 @@ export class Scanner {
 
 export abstract class Marker {
 
-	static toString(marker?: Marker[]): string {
-		let result = '';
-		for (const m of marker) {
-			result += m.toString();
-		}
-		return result;
-	}
-
 	readonly _markerBrand: any;
 
 	public parent: Marker;
@@ -181,9 +173,11 @@ export abstract class Marker {
 	}
 
 	toString() {
-		return '';
+		return this.children.reduce((prev, cur) => prev + cur.toString(), '');
 	}
 
+	abstract toTextmateString(): string;
+
 	len(): number {
 		return 0;
 	}
@@ -198,6 +192,9 @@ export class Text extends Marker {
 	toString() {
 		return this.value;
 	}
+	toTextmateString(): string {
+		return this.value.replace(/\$|}|\\/g, '\\$&');
+	}
 	len(): number {
 		return this.value.length;
 	}
@@ -238,8 +235,14 @@ export class Placeholder extends Marker {
 			: undefined;
 	}
 
-	toString() {
-		return Marker.toString(this.children);
+	toTextmateString(): string {
+		if (this.children.length === 0) {
+			return `\$${this.index}`;
+		} else if (this.choice) {
+			return `\${${this.index}|${this.choice.toTextmateString()}|}`;
+		} else {
+			return `\${${this.index}:${this.children.map(child => child.toTextmateString()).join('')}}`;
+		}
 	}
 
 	clone(): Placeholder {
@@ -265,6 +268,12 @@ export class Choice extends Marker {
 		return this.options[0].value;
 	}
 
+	toTextmateString(): string {
+		return this.options
+			.map(option => option.value.replace(/\||,/g, '\\$&'))
+			.join(',');
+	}
+
 	len(): number {
 		return this.options[0].len();
 	}
@@ -291,8 +300,12 @@ export class Variable extends Marker {
 		return false;
 	}
 
-	toString() {
-		return Marker.toString(this.children);
+	toTextmateString(): string {
+		if (this.children.length === 0) {
+			return `\${${this.name}}`;
+		} else {
+			return `\${${this.name}:${this.children.map(child => child.toTextmateString()).join('')}}`;
+		}
 	}
 
 	clone(): Variable {
@@ -375,10 +388,6 @@ export class TextmateSnippet extends Marker {
 		return ret;
 	}
 
-	get text() {
-		return Marker.toString(this.children);
-	}
-
 	resolveVariables(resolver: VariableResolver): this {
 		this.walk(candidate => {
 			if (candidate instanceof Variable) {
@@ -401,6 +410,10 @@ export class TextmateSnippet extends Marker {
 		return super.replace(child, others);
 	}
 
+	toTextmateString(): string {
+		return this.children.reduce((prev, cur) => prev + cur.toTextmateString(), '');
+	}
+
 	clone(): TextmateSnippet {
 		let ret = new TextmateSnippet();
 		this._children = this.children.map(child => child.clone());
@@ -422,7 +435,7 @@ export class SnippetParser {
 	private _token: Token;
 
 	text(value: string): string {
-		return this.parse(value).text;
+		return this.parse(value).toString();
 	}
 
 	parse(value: string, insertFinalTabstop?: boolean, enforceFinalTabstop?: boolean): TextmateSnippet {

src/vs/editor/contrib/snippet/browser/snippetSession.ts

@@ -299,12 +299,12 @@ export class SnippetSession {
 				.resolveVariables(new EditorSnippetVariableResolver(model, selection));
 
 			const offset = model.getOffsetAt(start) + delta;
-			delta += snippet.text.length - model.getValueLengthInRange(snippetSelection);
+			delta += snippet.toString().length - model.getValueLengthInRange(snippetSelection);
 
 			// store snippets with the index of their originating selection.
 			// that ensures the primiary cursor stays primary despite not being
 			// the one with lowest start position
-			edits[idx] = EditOperation.replace(snippetSelection, snippet.text);
+			edits[idx] = EditOperation.replace(snippetSelection, snippet.toString());
 			snippets[idx] = new OneSnippet(editor, snippet, offset);
 		}
 

src/vs/editor/contrib/snippet/test/browser/snippetParser.test.ts

@@ -231,6 +231,60 @@ suite('SnippetParser', () => {
 		});
 	});
 
+	test('Snippet choices: unable to escape comma and pipe, #31521', function () {
+		assertTextAndMarker('console.log(${1|not\\, not, five, 5, 1   23|});', 'console.log(not, not);', Text, Placeholder, Text);
+	});
+
+	test('Marker, toTextmateString()', function () {
+
+		function assertTextsnippetString(input: string, expected: string): void {
+			const snippet = new SnippetParser().parse(input);
+			const actual = snippet.toTextmateString();
+			assert.equal(actual, expected);
+		}
+
+		assertTextsnippetString('$1', '$1');
+		assertTextsnippetString('\\$1', '\\$1');
+		assertTextsnippetString('console.log(${1|not\\, not, five, 5, 1   23|});', 'console.log(${1|not\\, not, five, 5, 1   23|});');
+		assertTextsnippetString('console.log(${1|not\\, not, \\| five, 5, 1   23|});', 'console.log(${1|not\\, not, \\| five, 5, 1   23|});');
+		assertTextsnippetString('this is text', 'this is text');
+		assertTextsnippetString('this ${1:is ${2:nested with $var}}', 'this ${1:is ${2:nested with ${var}}}');
+		assertTextsnippetString('this ${1:is ${2:nested with $var}}}', 'this ${1:is ${2:nested with ${var}}}\\}');
+	});
+
+	test('Marker, toTextmateString() <-> identity', function () {
+
+		function assertIdent(input: string): void {
+			// full loop: (1) parse input, (2) generate textmate string, (3) parse, (4) ensure both trees are equal
+			const snippet = new SnippetParser().parse(input);
+			const input2 = snippet.toTextmateString();
+			const snippet2 = new SnippetParser().parse(input2);
+
+			function checkCheckChildren(marker1: Marker, marker2: Marker) {
+				assert.ok(marker1 instanceof Object.getPrototypeOf(marker2).constructor);
+				assert.ok(marker2 instanceof Object.getPrototypeOf(marker1).constructor);
+
+				assert.equal(marker1.children.length, marker2.children.length);
+				assert.equal(marker1.toString(), marker2.toString());
+
+				for (let i = 0; i < marker1.children.length; i++) {
+					checkCheckChildren(marker1.children[i], marker2.children[i]);
+				}
+			}
+
+			checkCheckChildren(snippet, snippet2);
+		}
+
+		assertIdent('$1');
+		assertIdent('\\$1');
+		assertIdent('console.log(${1|not\\, not, five, 5, 1   23|});');
+		assertIdent('console.log(${1|not\\, not, \\| five, 5, 1   23|});');
+		assertIdent('this is text');
+		assertIdent('this ${1:is ${2:nested with $var}}');
+		assertIdent('this ${1:is ${2:nested with $var}}}');
+		assertIdent('this ${1:is ${2:nested with $var}} and repeating $1');
+	});
+
 	test('Parser, choise marker', () => {
 		const { placeholders } = new SnippetParser().parse('${1|one,two,three|}');
 
@@ -418,7 +472,7 @@ suite('SnippetParser', () => {
 		let nested = new SnippetParser().parse('ddd$1eee$0', true);
 		snippet.replace(second, nested.children);
 
-		assert.equal(snippet.text, 'aaabbbdddeee');
+		assert.equal(snippet.toString(), 'aaabbbdddeee');
 		assert.equal(snippet.placeholders.length, 4);
 		assert.equal(snippet.placeholders[0].index, '1');
 		assert.equal(snippet.placeholders[1].index, '1');
@@ -441,7 +495,7 @@ suite('SnippetParser', () => {
 		let nested = new SnippetParser().parse('dddeee$0', true);
 		snippet.replace(second, nested.children);
 
-		assert.equal(snippet.text, 'aaabbbdddeee');
+		assert.equal(snippet.toString(), 'aaabbbdddeee');
 		assert.equal(snippet.placeholders.length, 3);
 	});
 

src/vs/editor/contrib/snippet/test/browser/snippetVariables.test.ts

@@ -100,16 +100,16 @@ suite('Snippet Variables Resolver', function () {
 
 	test('TextmateSnippet, resolve variable', function () {
 		const snippet = new SnippetParser().parse('"$TM_CURRENT_WORD"', true);
-		assert.equal(snippet.text, '""');
+		assert.equal(snippet.toString(), '""');
 		snippet.resolveVariables(resolver);
-		assert.equal(snippet.text, '"this"');
+		assert.equal(snippet.toString(), '"this"');
 
 	});
 
 	test('TextmateSnippet, resolve variable with default', function () {
 		const snippet = new SnippetParser().parse('"${TM_CURRENT_WORD:foo}"', true);
-		assert.equal(snippet.text, '"foo"');
+		assert.equal(snippet.toString(), '"foo"');
 		snippet.resolveVariables(resolver);
-		assert.equal(snippet.text, '"this"');
+		assert.equal(snippet.toString(), '"this"');
 	});
 });

src/vs/workbench/parts/snippets/electron-browser/TMSnippets.ts

@@ -15,7 +15,7 @@ import { ISnippetsService, ISnippet } from 'vs/workbench/parts/snippets/electron
 import { IModeService } from 'vs/editor/common/services/modeService';
 import { languagesExtPoint } from 'vs/workbench/services/mode/common/workbenchModeService';
 import { LanguageIdentifier } from 'vs/editor/common/modes';
-import { SnippetParser, Marker, Placeholder, Variable, Text } from 'vs/editor/contrib/snippet/browser/snippetParser';
+import { SnippetParser, Placeholder, Variable, Text } from 'vs/editor/contrib/snippet/browser/snippetParser';
 import { EditorSnippetVariableResolver } from 'vs/editor/contrib/snippet/browser/snippetVariables';
 
 interface ISnippetsExtensionPoint {
@@ -158,7 +158,7 @@ function parseSnippetFile(snippetFileContent: string, extensionName?: string, co
 	return result;
 }
 
-function _rewriteBogousVariables(snippet: ISnippet): boolean {
+export function _rewriteBogousVariables(snippet: ISnippet): boolean {
 	const textmateSnippet = new SnippetParser().parse(snippet.codeSnippet, false);
 
 	let placeholders = new Map<string, number>();
@@ -167,40 +167,33 @@ function _rewriteBogousVariables(snippet: ISnippet): boolean {
 		placeholderMax = Math.max(placeholderMax, placeholder.index);
 	}
 
-	function fixBogousVariables(marker: Marker): string {
-		if (marker instanceof Text) {
-			return SnippetParser.escape(marker.value);
+	let didChange = false;
+	let stack = [...textmateSnippet.children];
 
-		} else if (marker instanceof Placeholder) {
-			if (marker.choice) {
-				return `\${${marker.index}|${marker.choice.options.map(fixBogousVariables).join(',')}|}`;
+	while (stack.length > 0) {
+		let marker = stack.shift();
 
-			} else if (marker.children.length > 0) {
-				return `\${${marker.index}:${marker.children.map(fixBogousVariables).join('')}}`;
+		if (
+			marker instanceof Variable
+			&& marker.children.length === 0
+			&& !EditorSnippetVariableResolver.VariableNames[marker.name]
+		) {
+			// a 'variable' without a default value and not being one of our supported
+			// variables is automatically turing into a placeholder. This is to restore
+			// a bug we had before. So `${foo}` becomes `${N:foo}`
+			const index = placeholders.has(marker.name) ? placeholders.get(marker.name) : ++placeholderMax;
+			placeholders.set(marker.name, index);
+
+			const synthetic = new Placeholder(index).appendChild(new Text(marker.name));
+			textmateSnippet.replace(marker, [synthetic]);
+			didChange = true;
 
-			} else {
-				return `\$${marker.index}`;
-			}
-		} else if (marker instanceof Variable) {
-			if (marker.children.length === 0 && !EditorSnippetVariableResolver.VariableNames[marker.name]) {
-				// a 'variable' without a default value and not being one of our supported
-				// variables is automatically turing into a placeholder. This is to restore
-				// a bug we had before. So `${foo}` becomes `${N:foo}`
-				let index = placeholders.has(marker.name) ? placeholders.get(marker.name) : ++placeholderMax;
-				placeholders.set(marker.name, index);
-				return `\${${index++}:${marker.name}}`;
-
-			} else if (marker.children.length > 0) {
-				return `\${${marker.name}:${marker.children.map(fixBogousVariables).join('')}}`;
-			} else {
-				return `\${${marker.name}}`;
-			}
 		} else {
-			throw new Error('unexpected marker: ' + marker);
+			// recurse
+			stack.push(...marker.children);
 		}
 	}
 
-	const placeholderCountBefore = placeholderMax;
-	snippet.codeSnippet = textmateSnippet.children.map(fixBogousVariables).join('');
-	return placeholderCountBefore !== placeholderMax;
+	snippet.codeSnippet = textmateSnippet.toTextmateString();
+	return didChange;
 }

src/vs/workbench/parts/snippets/test/electron-browser/snippetsRewrite.test.ts

+/*---------------------------------------------------------------------------------------------
+ *  Copyright (c) Microsoft Corporation. All rights reserved.
+ *  Licensed under the MIT License. See License.txt in the project root for license information.
+ *--------------------------------------------------------------------------------------------*/
+
+'use strict';
+
+import * as assert from 'assert';
+import { _rewriteBogousVariables } from 'vs/workbench/parts/snippets/electron-browser/TMSnippets';
+
+suite('TMSnippets', function () {
+
+	function assertRewrite(input: string, expected: string): void {
+		let snippet = { codeSnippet: input, description: undefined, extensionName: undefined, name: undefined, prefix: undefined };
+		_rewriteBogousVariables(snippet);
+		assert.equal(snippet.codeSnippet, expected);
+	}
+
+	test('bogous variable rewrite', function () {
+
+		assertRewrite('foo', 'foo');
+		assertRewrite('hello $1 world$0', 'hello $1 world$0');
+
+		assertRewrite('$foo and $foo', '${1:foo} and ${1:foo}');
+		assertRewrite('$1 and $SELECTION and $foo', '$1 and ${SELECTION} and ${2:foo}');
+
+
+		assertRewrite(
+			[
+				'for (var ${index} = 0; ${index} < ${array}.length; ${index}++) {',
+				'\tvar ${element} = ${array}[${index}];',
+				'\t$0',
+				'}'
+			].join('\n'),
+			[
+				'for (var ${1:index} = 0; ${1:index} < ${2:array}.length; ${1:index}++) {',
+				'\tvar ${3:element} = ${2:array}[${1:index}];',
+				'\t$0',
+				'\\}'
+			].join('\n')
+		);
+	});
+
+	test('Snippet choices: unable to escape comma and pipe, #31521', function () {
+		assertRewrite('console.log(${1|not\\, not, five, 5, 1   23|});', 'console.log(${1|not\\, not, five, 5, 1   23|});');
+	});
+});