massage bogous snippets when reading them, show info-message, #27090

src/vs/editor/contrib/snippet/common/snippetVariables.ts

@@ -13,6 +13,18 @@ import { ISnippetVariableResolver } from './snippet';
 
 export class EditorSnippetVariableResolver {
 
+	static readonly VariableNames = Object.freeze({
+		'SELECTION': true,
+		'TM_SELECTED_TEXT': true,
+		'TM_CURRENT_LINE': true,
+		'TM_CURRENT_WORD': true,
+		'TM_LINE_INDEX': true,
+		'TM_LINE_NUMBER': true,
+		'TM_FILENAME': true,
+		'TM_DIRECTORY': true,
+		'TM_FILEPATH': true,
+	});
+
 	constructor(
 		private readonly _model: IModel,
 		private readonly _selection: Selection

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

@@ -15,6 +15,8 @@ import { ISnippetsService, ISnippet } from 'vs/workbench/parts/snippets/electron
 import { IModeService } from 'vs/editor/common/services/modeService';
 import { languagesExtPoint } from 'vs/editor/common/services/modeServiceImpl';
 import { LanguageIdentifier } from 'vs/editor/common/modes';
+import { SnippetParser, Marker, Placeholder, Variable, Text, walk } from 'vs/editor/contrib/snippet/common/snippetParser';
+import { EditorSnippetVariableResolver } from "vs/editor/contrib/snippet/common/snippetVariables";
 
 interface ISnippetsExtensionPoint {
 	language: string;
@@ -79,19 +81,23 @@ export class MainProcessTextMateSnippet implements IWorkbenchContribution {
 		let modeId = snippet.language;
 		let languageIdentifier = this._modeService.getLanguageIdentifier(modeId);
 		if (languageIdentifier) {
-			readAndRegisterSnippets(this._snippetService, languageIdentifier, normalizedAbsolutePath, extensionName);
+			readAndRegisterSnippets(this._snippetService, languageIdentifier, normalizedAbsolutePath, extensionName, collector);
 		}
 	}
 }
 
-export function readAndRegisterSnippets(snippetService: ISnippetsService, languageIdentifier: LanguageIdentifier, filePath: string, extensionName?: string): TPromise<void> {
+export function readAndRegisterSnippets(
+	snippetService: ISnippetsService, languageIdentifier: LanguageIdentifier, filePath: string,
+	extensionName?: string, collector?: ExtensionMessageCollector
+): TPromise<void> {
+
 	return readFile(filePath).then(fileContents => {
-		let snippets = parseSnippetFile(fileContents.toString(), extensionName);
+		let snippets = parseSnippetFile(fileContents.toString(), extensionName, collector);
 		snippetService.registerSnippets(languageIdentifier.id, snippets, filePath);
 	});
 }
 
-function parseSnippetFile(snippetFileContent: string, extensionName?: string): ISnippet[] {
+function parseSnippetFile(snippetFileContent: string, extensionName?: string, collector?: ExtensionMessageCollector): ISnippet[] {
 	let snippetsObj = parse(snippetFileContent);
 	if (!snippetsObj || typeof snippetsObj !== 'object') {
 		return [];
@@ -102,21 +108,34 @@ function parseSnippetFile(snippetFileContent: string, extensionName?: string): I
 
 	let processSnippet = (snippet: any, name: string) => {
 		let prefix = snippet['prefix'];
-		let bodyStringOrArray = snippet['body'];
+		let body = <string | string[]>snippet['body'];
 
-		if (Array.isArray(bodyStringOrArray)) {
-			bodyStringOrArray = bodyStringOrArray.join('\n');
+		if (Array.isArray(body)) {
+			body = body.join('\n');
 		}
 
-		if (typeof prefix === 'string' && typeof bodyStringOrArray === 'string') {
-			result.push({
-				name,
-				extensionName,
-				prefix,
-				description: snippet['description'] || name,
-				codeSnippet: bodyStringOrArray
-			});
+		if (typeof prefix !== 'string' || typeof body !== 'string') {
+			return;
+		}
+
+		snippet = {
+			name,
+			extensionName,
+			prefix,
+			description: snippet['description'] || name,
+			codeSnippet: body
+		};
+
+		const didRewrite = _rewriteBogousVariables(snippet);
+		if (didRewrite && collector) {
+			collector.info(nls.localize(
+				'badVariableUse',
+				"The \"{0}\"-snippet very likely confuses snippet-variables and snippet-placeholders. See https://code.visualstudio.com/docs/editor/userdefinedsnippets#_snippet-syntax for more details",
+				name
+			));
 		}
+
+		result.push(snippet);
 	};
 
 	topLevelProperties.forEach(topLevelProperty => {
@@ -132,3 +151,49 @@ function parseSnippetFile(snippetFileContent: string, extensionName?: string): I
 	});
 	return result;
 }
+
+function _rewriteBogousVariables(snippet: ISnippet): boolean {
+	const marker = new SnippetParser(true, false).parse(snippet.codeSnippet, false);
+
+	let placeholders = new Map<string, number>();
+	let placeholderMax = 0;
+	walk(marker, candidate => {
+		if (candidate instanceof Placeholder) {
+			placeholderMax = Math.max(placeholderMax, Number(candidate.index));
+		}
+		return true;
+	});
+
+	function fixBogousVariables(marker: Marker): string {
+		if (marker instanceof Text) {
+			return SnippetParser.escape(marker.string);
+
+		} else if (marker instanceof Placeholder) {
+			if (marker.defaultValue.length > 0) {
+				return `\${${marker.index}:${marker.defaultValue.map(fixBogousVariables).join('')}}`;
+			} else {
+				return `\$${marker.index}`;
+			}
+		} else if (marker instanceof Variable) {
+			if (marker.defaultValue.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.defaultValue.length > 0) {
+				return `\${${marker.name}:${marker.defaultValue.map(fixBogousVariables).join('')}}`;
+			} else {
+				return `\$${marker.name}`;
+			}
+		} else {
+			throw new Error('unexpected marker: ' + marker);
+		}
+	}
+
+	const placeholderCountBefore = placeholderMax;
+	snippet.codeSnippet = marker.map(fixBogousVariables).join('');
+	return placeholderCountBefore !== placeholderMax;
+}