fix #31040

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

@@ -380,27 +380,33 @@ export class SnippetParser {
 
 		// * fill in default for empty placeHolders
 		// * compact sibling Text markers
-		function walk(marker: Marker[], placeholderDefaultValues: Map<number, Marker[]>) {
+		const placeholderDefaultValues = new Map<number, Marker[]>();
+		const incompletePlaceholders: Placeholder[] = [];
+
+		function walk(marker: Marker[]) {
 
 			for (let i = 0; i < marker.length; i++) {
 				const thisMarker = marker[i];
 
+				// fill in default values for repeated placeholders
+				// like `${1:foo}and$1` becomes `${1:foo}and${1:foo}`
+				// and `$1and${1:foo}` becomes `${1:foo}and${1:foo}`
 				if (thisMarker instanceof Placeholder) {
-					// fill in default values for repeated placeholders
-					// like `${1:foo}and$1` becomes ${1:foo}and${1:foo}
-					if (!placeholderDefaultValues.has(thisMarker.index)) {
+					// the first placeholder that has a value define the
+					// value for *all* placeholders of this index
+					if (thisMarker.isFinalTabstop) {
+						placeholderDefaultValues.set(0);
+					} else if (!placeholderDefaultValues.has(thisMarker.index) && thisMarker.children.length > 0) {
 						placeholderDefaultValues.set(thisMarker.index, thisMarker.children);
-						walk(thisMarker.children, placeholderDefaultValues);
+						walk(thisMarker.children);
 
-					} else if (thisMarker.children.length === 0) {
-						// copy children from first placeholder definition, no need to
-						// recurse on them because they have been visited already
-						thisMarker.children = placeholderDefaultValues.get(thisMarker.index).map(child => child.clone());
+					} else {
+						incompletePlaceholders.push(thisMarker);
 					}
 
 
 				} else if (thisMarker instanceof Variable) {
-					walk(thisMarker.children, placeholderDefaultValues);
+					walk(thisMarker.children);
 
 				} else if (i > 0 && thisMarker instanceof Text && marker[i - 1] instanceof Text) {
 					(<Text>marker[i - 1]).string += (<Text>marker[i]).string;
@@ -409,9 +415,14 @@ export class SnippetParser {
 				}
 			}
 		}
+		walk(marker);
 
-		const placeholderDefaultValues = new Map<number, Marker[]>();
-		walk(marker, placeholderDefaultValues);
+		// fill in defaults
+		for (const placeholder of incompletePlaceholders) {
+			if (placeholderDefaultValues.has(placeholder.index)) {
+				placeholder.children = placeholderDefaultValues.get(placeholder.index).map(m => m.clone());
+			}
+		}
 
 		if (
 			!placeholderDefaultValues.has(0) && // there is no final tabstop

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

@@ -231,10 +231,10 @@ suite('SnippetController2', function () {
 		model.setValue('');
 		editor.setSelection(new Selection(1, 1, 1, 1));
 
-		ctrl.insert('import ${2:${1:module}} from \'${1: module }\'$0');
+		ctrl.insert('import ${2:${1:module}} from \'${1:module}\'$0');
 
 		assertContextKeys(contextKeys, true, false, true);
-		assertSelections(editor, new Selection(1, 8, 1, 14), new Selection(1, 21, 1, 29));
+		assertSelections(editor, new Selection(1, 8, 1, 14), new Selection(1, 21, 1, 27));
 
 		ctrl.insert('foo');
 		assertSelections(editor, new Selection(1, 11, 1, 11), new Selection(1, 21, 1, 21));

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

@@ -259,7 +259,13 @@ suite('SnippetParser', () => {
 		assert.equal((<Placeholder>p2).index, '1');
 		assert.equal((<Placeholder>p2).children.length, '1');
 		assert.equal((<Text>(<Placeholder>p2).children[0]), 'err');
+	});
 
+	test('Repeated snippet placeholder should always inherit, #31040', function () {
+		assertText('${1:foo}-abc-$1', 'foo-abc-foo');
+		assertText('${1:foo}-abc-${1}', 'foo-abc-foo');
+		assertText('${1:foo}-abc-${1:bar}', 'foo-abc-foo');
+		assertText('${1}-abc-${1:foo}', 'foo-abc-foo');
 	});
 
 	test('backspace esapce in TM only, #16212', () => {