Do not trim whitespace when part of strings or regexes (microsoft#198164

)

* Do not trim whitespace when part of strings or regexes

Fixes microsoft#195010. If the token to be removed is part of a string or a regex, then we do not want to remove the character, as it is very likely semantically important.

* Address initial feedback:

1. Add a new setting to control whether to trim whitespace in multiline strings/regexes. This setting is then piped through to everywhere that is calling trim whitespace, which is file save editing, the trim whitespace command, and notebooks.
2. Force line tokenization to complete when the setting is enabled so that trim is accurate.

* Don't force tokenization; instead, check to see if there are tokens for a given line and do not format if there are none.

* Look for syntactical tokens

* Fix compilation errors

* Add a test

---------

Co-authored-by: Alex Dima <[email protected]>

src/vs/editor/common/commands/trimTrailingWhitespaceCommand.ts

@@ -9,22 +9,25 @@ import { Position } from 'vs/editor/common/core/position';
 import { Range } from 'vs/editor/common/core/range';
 import { Selection } from 'vs/editor/common/core/selection';
 import { ICommand, ICursorStateComputerData, IEditOperationBuilder } from 'vs/editor/common/editorCommon';
+import { StandardTokenType } from 'vs/editor/common/encodedTokenAttributes';
 import { ITextModel } from 'vs/editor/common/model';
 
 export class TrimTrailingWhitespaceCommand implements ICommand {
 
 	private readonly _selection: Selection;
 	private _selectionId: string | null;
 	private readonly _cursors: Position[];
+	private readonly _trimInRegexesAndStrings: boolean;
 
-	constructor(selection: Selection, cursors: Position[]) {
+	constructor(selection: Selection, cursors: Position[], trimInRegexesAndStrings: boolean) {
 		this._selection = selection;
 		this._cursors = cursors;
 		this._selectionId = null;
+		this._trimInRegexesAndStrings = trimInRegexesAndStrings;
 	}
 
 	public getEditOperations(model: ITextModel, builder: IEditOperationBuilder): void {
-		const ops = trimTrailingWhitespace(model, this._cursors);
+		const ops = trimTrailingWhitespace(model, this._cursors, this._trimInRegexesAndStrings);
 		for (let i = 0, len = ops.length; i < len; i++) {
 			const op = ops[i];
 
@@ -42,7 +45,7 @@ export class TrimTrailingWhitespaceCommand implements ICommand {
 /**
  * Generate commands for trimming trailing whitespace on a model and ignore lines on which cursors are sitting.
  */
-export function trimTrailingWhitespace(model: ITextModel, cursors: Position[]): ISingleEditOperation[] {
+export function trimTrailingWhitespace(model: ITextModel, cursors: Position[], trimInRegexesAndStrings: boolean): ISingleEditOperation[] {
 	// Sort cursors ascending
 	cursors.sort((a, b) => {
 		if (a.lineNumber === b.lineNumber) {
@@ -96,6 +99,22 @@ export function trimTrailingWhitespace(model: ITextModel, cursors: Position[]):
 			continue;
 		}
 
+		if (!trimInRegexesAndStrings) {
+			if (!model.tokenization.hasAccurateTokensForLine(lineNumber)) {
+				// We don't want to force line tokenization, as that can be expensive, but we also don't want to trim
+				// trailing whitespace in lines that are not tokenized yet, as that can be wrong and trim whitespace from
+				// lines that the user requested we don't. So we bail out if the tokens are not accurate for this line.
+				continue;
+			}
+
+			const lineTokens = model.tokenization.getLineTokens(lineNumber);
+			const fromColumnType = lineTokens.getStandardTokenType(lineTokens.findTokenIndexAtOffset(fromColumn));
+
+			if (fromColumnType === StandardTokenType.String || fromColumnType === StandardTokenType.RegEx) {
+				continue;
+			}
+		}
+
 		fromColumn = Math.max(minEditColumn, fromColumn);
 		r[rLen++] = EditOperation.delete(new Range(
 			lineNumber, fromColumn,

src/vs/editor/common/model/textModelTokens.ts

@@ -128,6 +128,11 @@ export class TokenizerWithStateStoreAndTextModel<TState extends IState = IState>
 		return lineTokens;
 	}
 
+	public hasAccurateTokensForLine(lineNumber: number): boolean {
+		const firstInvalidLineNumber = this.store.getFirstInvalidEndStateLineNumberOrMax();
+		return (lineNumber < firstInvalidLineNumber);
+	}
+
 	public isCheapToTokenize(lineNumber: number): boolean {
 		const firstInvalidLineNumber = this.store.getFirstInvalidEndStateLineNumberOrMax();
 		if (lineNumber < firstInvalidLineNumber) {

src/vs/editor/common/model/tokenizationTextModelPart.ts

@@ -142,6 +142,11 @@ export class TokenizationTextModelPart extends TextModelPart implements ITokeniz
 		this.grammarTokens.forceTokenization(lineNumber);
 	}
 
+	public hasAccurateTokensForLine(lineNumber: number): boolean {
+		this.validateLineNumber(lineNumber);
+		return this.grammarTokens.hasAccurateTokensForLine(lineNumber);
+	}
+
 	public isCheapToTokenize(lineNumber: number): boolean {
 		this.validateLineNumber(lineNumber);
 		return this.grammarTokens.isCheapToTokenize(lineNumber);
@@ -568,6 +573,13 @@ class GrammarTokens extends Disposable {
 		this._defaultBackgroundTokenizer?.checkFinished();
 	}
 
+	public hasAccurateTokensForLine(lineNumber: number): boolean {
+		if (!this._tokenizer) {
+			return true;
+		}
+		return this._tokenizer.hasAccurateTokensForLine(lineNumber);
+	}
+
 	public isCheapToTokenize(lineNumber: number): boolean {
 		if (!this._tokenizer) {
 			return true;

src/vs/editor/common/tokenizationTextModelPart.ts

@@ -56,6 +56,12 @@ export interface ITokenizationTextModelPart {
 	 */
 	tokenizeIfCheap(lineNumber: number): void;
 
+	/**
+	 * Check if tokenization information is accurate for `lineNumber`.
+	 * @internal
+	 */
+	hasAccurateTokensForLine(lineNumber: number): boolean;
+
 	/**
 	 * Check if calling `forceTokenization` for this `lineNumber` will be cheap (time-wise).
 	 * This is based on a heuristic.

src/vs/editor/contrib/linesOperations/browser/linesOperations.ts

@@ -25,6 +25,7 @@ import * as nls from 'vs/nls';
 import { MenuId } from 'vs/platform/actions/common/actions';
 import { KeybindingWeight } from 'vs/platform/keybinding/common/keybindingsRegistry';
 import { ILanguageConfigurationService } from 'vs/editor/common/languages/languageConfigurationRegistry';
+import { IConfigurationService } from 'vs/platform/configuration/common/configuration';
 
 // copy lines
 
@@ -402,7 +403,11 @@ export class TrimTrailingWhitespaceAction extends EditorAction {
 			return;
 		}
 
-		const command = new TrimTrailingWhitespaceCommand(selection, cursors);
+		const config = _accessor.get(IConfigurationService);
+		const model = editor.getModel();
+		const trimInRegexAndStrings = config.getValue<boolean>('files.trimTrailingWhitespaceInRegexAndStrings', { overrideIdentifier: model?.getLanguageId(), resource: model?.uri });
+
+		const command = new TrimTrailingWhitespaceCommand(selection, cursors, trimInRegexAndStrings);
 
 		editor.pushUndoStop();
 		editor.executeCommands(this.id, [command]);

src/vs/editor/test/browser/commands/trimTrailingWhitespaceCommand.test.ts

@@ -4,14 +4,19 @@
  *--------------------------------------------------------------------------------------------*/
 
 import * as assert from 'assert';
+import { DisposableStore } from 'vs/base/common/lifecycle';
 import { ensureNoDisposablesAreLeakedInTestSuite } from 'vs/base/test/common/utils';
 import { TrimTrailingWhitespaceCommand, trimTrailingWhitespace } from 'vs/editor/common/commands/trimTrailingWhitespaceCommand';
 import { ISingleEditOperation } from 'vs/editor/common/core/editOperation';
 import { Position } from 'vs/editor/common/core/position';
 import { Range } from 'vs/editor/common/core/range';
 import { Selection } from 'vs/editor/common/core/selection';
+import { MetadataConsts, StandardTokenType } from 'vs/editor/common/encodedTokenAttributes';
+import { EncodedTokenizationResult, ITokenizationSupport, TokenizationRegistry } from 'vs/editor/common/languages';
+import { ILanguageService } from 'vs/editor/common/languages/language';
+import { NullState } from 'vs/editor/common/languages/nullTokenize';
 import { getEditOperation } from 'vs/editor/test/browser/testCommand';
-import { withEditorModel } from 'vs/editor/test/common/testTextModel';
+import { createModelServices, instantiateTextModel, withEditorModel } from 'vs/editor/test/common/testTextModel';
 
 /**
  * Create single edit operation
@@ -36,21 +41,31 @@ function createSingleEditOp(text: string | null, positionLineNumber: number, pos
 
 function assertTrimTrailingWhitespaceCommand(text: string[], expected: ISingleEditOperation[]): void {
 	return withEditorModel(text, (model) => {
-		const op = new TrimTrailingWhitespaceCommand(new Selection(1, 1, 1, 1), []);
+		const op = new TrimTrailingWhitespaceCommand(new Selection(1, 1, 1, 1), [], true);
 		const actual = getEditOperation(model, op);
 		assert.deepStrictEqual(actual, expected);
 	});
 }
 
 function assertTrimTrailingWhitespace(text: string[], cursors: Position[], expected: ISingleEditOperation[]): void {
 	return withEditorModel(text, (model) => {
-		const actual = trimTrailingWhitespace(model, cursors);
+		const actual = trimTrailingWhitespace(model, cursors, true);
 		assert.deepStrictEqual(actual, expected);
 	});
 }
 
 suite('Editor Commands - Trim Trailing Whitespace Command', () => {
 
+	let disposables: DisposableStore;
+
+	setup(() => {
+		disposables = new DisposableStore();
+	});
+
+	teardown(() => {
+		disposables.dispose();
+	});
+
 	ensureNoDisposablesAreLeakedInTestSuite();
 
 	test('remove trailing whitespace', function () {
@@ -102,4 +117,73 @@ suite('Editor Commands - Trim Trailing Whitespace Command', () => {
 		]);
 	});
 
+	test('skips strings and regex if configured', function () {
+		const instantiationService = createModelServices(disposables);
+		const languageService = instantiationService.get(ILanguageService);
+		const languageId = 'testLanguageId';
+		const languageIdCodec = languageService.languageIdCodec;
+		disposables.add(languageService.registerLanguage({ id: languageId }));
+		const encodedLanguageId = languageIdCodec.encodeLanguageId(languageId);
+
+		const otherMetadata = (
+			(encodedLanguageId << MetadataConsts.LANGUAGEID_OFFSET)
+			| (StandardTokenType.Other << MetadataConsts.TOKEN_TYPE_OFFSET)
+			| (MetadataConsts.BALANCED_BRACKETS_MASK)
+		) >>> 0;
+		const stringMetadata = (
+			(encodedLanguageId << MetadataConsts.LANGUAGEID_OFFSET)
+			| (StandardTokenType.String << MetadataConsts.TOKEN_TYPE_OFFSET)
+			| (MetadataConsts.BALANCED_BRACKETS_MASK)
+		) >>> 0;
+
+		const tokenizationSupport: ITokenizationSupport = {
+			getInitialState: () => NullState,
+			tokenize: undefined!,
+			tokenizeEncoded: (line, hasEOL, state) => {
+				switch (line) {
+					case 'const a = `  ': {
+						const tokens = new Uint32Array([
+							0, otherMetadata,
+							10, stringMetadata,
+						]);
+						return new EncodedTokenizationResult(tokens, state);
+					}
+					case '  a string  ': {
+						const tokens = new Uint32Array([
+							0, stringMetadata,
+						]);
+						return new EncodedTokenizationResult(tokens, state);
+					}
+					case '`;  ': {
+						const tokens = new Uint32Array([
+							0, stringMetadata,
+							1, otherMetadata
+						]);
+						return new EncodedTokenizationResult(tokens, state);
+					}
+				}
+				throw new Error(`Unexpected`);
+			}
+		};
+
+		disposables.add(TokenizationRegistry.register(languageId, tokenizationSupport));
+
+		const model = disposables.add(instantiateTextModel(
+			instantiationService,
+			[
+				'const a = `  ',
+				'  a string  ',
+				'`;  ',
+			].join('\n'),
+			languageId
+		));
+
+		model.tokenization.forceTokenization(1);
+		model.tokenization.forceTokenization(2);
+		model.tokenization.forceTokenization(3);
+
+		const op = new TrimTrailingWhitespaceCommand(new Selection(1, 1, 1, 1), [], false);
+		const actual = getEditOperation(model, op);
+		assert.deepStrictEqual(actual, [createSingleEditOp(null, 3, 3, 3, 5)]);
+	});
 });

src/vs/workbench/contrib/codeEditor/browser/saveParticipants.ts

@@ -47,12 +47,14 @@ export class TrimWhitespaceParticipant implements ITextFileSaveParticipant {
 			return;
 		}
 
-		if (this.configurationService.getValue('files.trimTrailingWhitespace', { overrideIdentifier: model.textEditorModel.getLanguageId(), resource: model.resource })) {
-			this.doTrimTrailingWhitespace(model.textEditorModel, context.reason === SaveReason.AUTO);
+		const trimTrailingWhitespaceOption = this.configurationService.getValue<boolean>('files.trimTrailingWhitespace', { overrideIdentifier: model.textEditorModel.getLanguageId(), resource: model.resource });
+		const trimInRegexAndStrings = this.configurationService.getValue<boolean>('files.trimTrailingWhitespaceInRegexAndStrings', { overrideIdentifier: model.textEditorModel.getLanguageId(), resource: model.resource });
+		if (trimTrailingWhitespaceOption) {
+			this.doTrimTrailingWhitespace(model.textEditorModel, context.reason === SaveReason.AUTO, trimInRegexAndStrings);
 		}
 	}
 
-	private doTrimTrailingWhitespace(model: ITextModel, isAutoSaved: boolean): void {
+	private doTrimTrailingWhitespace(model: ITextModel, isAutoSaved: boolean, trimInRegexesAndStrings: boolean): void {
 		let prevSelection: Selection[] = [];
 		let cursors: Position[] = [];
 
@@ -72,7 +74,7 @@ export class TrimWhitespaceParticipant implements ITextFileSaveParticipant {
 			}
 		}
 
-		const ops = trimTrailingWhitespace(model, cursors);
+		const ops = trimTrailingWhitespace(model, cursors, trimInRegexesAndStrings);
 		if (!ops.length) {
 			return; // Nothing to do
 		}

src/vs/workbench/contrib/files/browser/files.contribution.ts

@@ -230,6 +230,12 @@ configurationRegistry.registerConfiguration({
 			'description': nls.localize('trimTrailingWhitespace', "When enabled, will trim trailing whitespace when saving a file."),
 			'scope': ConfigurationScope.LANGUAGE_OVERRIDABLE
 		},
+		'files.trimTrailingWhitespaceInRegexAndStrings': {
+			'type': 'boolean',
+			'default': true,
+			'description': nls.localize('trimTrailingWhitespaceInRegexAndStrings', "When enabled, trailing whitespace will be removed from multiline strings and regexes will be removed on save or when executing 'editor.action.trimTrailingWhitespace'. This can cause whitespace to not be trimmed from lines when there isn't up-to-date token information."),
+			'scope': ConfigurationScope.LANGUAGE_OVERRIDABLE
+		},
 		'files.insertFinalNewline': {
 			'type': 'boolean',
 			'default': false,

src/vs/workbench/contrib/notebook/browser/contrib/saveParticipants/saveParticipants.ts

@@ -110,12 +110,14 @@ class TrimWhitespaceParticipant implements IStoredFileWorkingCopySaveParticipant
 	) { }
 
 	async participate(workingCopy: IStoredFileWorkingCopy<IStoredFileWorkingCopyModel>, context: IStoredFileWorkingCopySaveParticipantContext, progress: IProgress<IProgressStep>, _token: CancellationToken): Promise<void> {
-		if (this.configurationService.getValue<boolean>('files.trimTrailingWhitespace')) {
-			await this.doTrimTrailingWhitespace(workingCopy, context.reason === SaveReason.AUTO, progress);
+		const trimTrailingWhitespaceOption = this.configurationService.getValue<boolean>('files.trimTrailingWhitespace');
+		const trimInRegexAndStrings = this.configurationService.getValue<boolean>('files.trimTrailingWhitespaceInRegexAndStrings');
+		if (trimTrailingWhitespaceOption) {
+			await this.doTrimTrailingWhitespace(workingCopy, context.reason === SaveReason.AUTO, trimInRegexAndStrings, progress);
 		}
 	}
 
-	private async doTrimTrailingWhitespace(workingCopy: IStoredFileWorkingCopy<IStoredFileWorkingCopyModel>, isAutoSaved: boolean, progress: IProgress<IProgressStep>) {
+	private async doTrimTrailingWhitespace(workingCopy: IStoredFileWorkingCopy<IStoredFileWorkingCopyModel>, isAutoSaved: boolean, trimInRegexesAndStrings: boolean, progress: IProgress<IProgressStep>) {
 		if (!workingCopy.model || !(workingCopy.model instanceof NotebookFileWorkingCopyModel)) {
 			return;
 		}
@@ -150,7 +152,7 @@ class TrimWhitespaceParticipant implements IStoredFileWorkingCopySaveParticipant
 					}
 				}
 
-				const ops = trimTrailingWhitespace(model, cursors);
+				const ops = trimTrailingWhitespace(model, cursors, trimInRegexesAndStrings);
 				if (!ops.length) {
 					return []; // Nothing to do
 				}