From 092effaaebf07df9d38e11c8609094e592358bb6 Mon Sep 17 00:00:00 2001 From: Henning Dieterichs Date: Fri, 11 Jul 2025 21:21:55 +0200 Subject: [PATCH] Fixes https://github.com/microsoft/vscode/issues/255127 (#255428) * Fixes https://github.com/microsoft/vscode/issues/255127 * Fixes tests * Fixes CI --- src/vs/editor/common/core/edits/textEdit.ts | 25 ++++++ .../editor/common/core/text/abstractText.ts | 5 ++ .../common/core/text/positionToOffsetImpl.ts | 4 +- .../browser/model/inlineCompletionsModel.ts | 86 +++++++++++-------- .../browser/model/inlineSuggestionItem.ts | 4 +- .../inlineCompletions/browser/utils.ts | 2 +- .../test/browser/getSecondaryEdits.test.ts | 10 +-- .../test/browser/inlineCompletions.test.ts | 8 +- .../common/platformObservableUtils.ts | 38 +++++--- 9 files changed, 115 insertions(+), 67 deletions(-) diff --git a/src/vs/editor/common/core/edits/textEdit.ts b/src/vs/editor/common/core/edits/textEdit.ts index 2e1d490bc7e..f12c562a242 100644 --- a/src/vs/editor/common/core/edits/textEdit.ts +++ b/src/vs/editor/common/core/edits/textEdit.ts @@ -347,6 +347,12 @@ export class TextReplacement { return this.extendToCoverRange(newRange, initialValue); } + public removeCommonPrefixAndSuffix(text: AbstractText): TextReplacement { + const prefix = this.removeCommonPrefix(text); + const suffix = prefix.removeCommonSuffix(text); + return suffix; + } + public removeCommonPrefix(text: AbstractText): TextReplacement { const normalizedOriginalText = text.getValueOfRange(this.range).replaceAll('\r\n', '\n'); const normalizedModifiedText = this.text.replaceAll('\r\n', '\n'); @@ -360,6 +366,19 @@ export class TextReplacement { return new TextReplacement(range, newText); } + public removeCommonSuffix(text: AbstractText): TextReplacement { + const normalizedOriginalText = text.getValueOfRange(this.range).replaceAll('\r\n', '\n'); + const normalizedModifiedText = this.text.replaceAll('\r\n', '\n'); + + const commonSuffixLen = commonSuffixLength(normalizedOriginalText, normalizedModifiedText); + const end = TextLength.ofText(normalizedOriginalText.substring(0, normalizedOriginalText.length - commonSuffixLen)) + .addToPosition(this.range.getStartPosition()); + + const newText = normalizedModifiedText.substring(0, normalizedModifiedText.length - commonSuffixLen); + const range = Range.fromPositions(this.range.getStartPosition(), end); + return new TextReplacement(range, newText); + } + public isEffectiveDeletion(text: AbstractText): boolean { let newText = this.text.replaceAll('\r\n', '\n'); let existingText = text.getValueOfRange(this.range).replaceAll('\r\n', '\n'); @@ -372,6 +391,12 @@ export class TextReplacement { return newText === ''; } + + public toString(): string { + const start = this.range.getStartPosition(); + const end = this.range.getEndPosition(); + return `(${start.lineNumber},${start.column} -> ${end.lineNumber},${end.column}): "${this.text}"`; + } } function rangeFromPositions(start: Position, end: Position): Range { diff --git a/src/vs/editor/common/core/text/abstractText.ts b/src/vs/editor/common/core/text/abstractText.ts index fe99d9f818d..71ae73e12ea 100644 --- a/src/vs/editor/common/core/text/abstractText.ts +++ b/src/vs/editor/common/core/text/abstractText.ts @@ -10,6 +10,7 @@ import { PositionOffsetTransformer } from './positionToOffsetImpl.js'; import { Range } from '../range.js'; import { LineRange } from '../ranges/lineRange.js'; import { TextLength } from '../text/textLength.js'; +import { OffsetRange } from '../ranges/offsetRange.js'; export abstract class AbstractText { abstract getValueOfRange(range: Range): string; @@ -27,6 +28,10 @@ export abstract class AbstractText { return this.getValueOfRange(this.length.toRange()); } + getValueOfOffsetRange(range: OffsetRange): string { + return this.getValueOfRange(this.getTransformer().getRange(range)); + } + getLineLength(lineNumber: number): number { return this.getValueOfRange(new Range(lineNumber, 1, lineNumber, Number.MAX_SAFE_INTEGER)).length; } diff --git a/src/vs/editor/common/core/text/positionToOffsetImpl.ts b/src/vs/editor/common/core/text/positionToOffsetImpl.ts index deb847b1322..75c0ea80ee9 100644 --- a/src/vs/editor/common/core/text/positionToOffsetImpl.ts +++ b/src/vs/editor/common/core/text/positionToOffsetImpl.ts @@ -39,12 +39,12 @@ export abstract class PositionOffsetTransformerBase { return new Deps.deps.StringReplacement(this.getOffsetRange(edit.range), edit.text); } - getSingleTextEdit(edit: StringReplacement): TextReplacement { + getTextReplacement(edit: StringReplacement): TextReplacement { return new Deps.deps.TextReplacement(this.getRange(edit.replaceRange), edit.newText); } getTextEdit(edit: StringEdit): TextEdit { - const edits = edit.replacements.map(e => this.getSingleTextEdit(e)); + const edits = edit.replacements.map(e => this.getTextReplacement(e)); return new Deps.deps.TextEdit(edits); } } diff --git a/src/vs/editor/contrib/inlineCompletions/browser/model/inlineCompletionsModel.ts b/src/vs/editor/contrib/inlineCompletions/browser/model/inlineCompletionsModel.ts index e206d0376c3..100eede4291 100644 --- a/src/vs/editor/contrib/inlineCompletions/browser/model/inlineCompletionsModel.ts +++ b/src/vs/editor/contrib/inlineCompletions/browser/model/inlineCompletionsModel.ts @@ -5,11 +5,11 @@ import { mapFindFirst } from '../../../../../base/common/arraysFind.js'; import { itemsEquals } from '../../../../../base/common/equals.js'; -import { BugIndicatingError, onUnexpectedError, onUnexpectedExternalError } from '../../../../../base/common/errors.js'; +import { BugIndicatingError, onUnexpectedExternalError } from '../../../../../base/common/errors.js'; import { Emitter } from '../../../../../base/common/event.js'; import { Disposable } from '../../../../../base/common/lifecycle.js'; import { IObservable, IObservableWithChange, IReader, ITransaction, autorun, constObservable, derived, derivedHandleChanges, derivedOpts, mapObservableArrayCached, observableFromEvent, observableSignal, observableValue, recomputeInitiallyAndOnChange, subtransaction, transaction } from '../../../../../base/common/observable.js'; -import { commonPrefixLength, firstNonWhitespaceIndex } from '../../../../../base/common/strings.js'; +import { firstNonWhitespaceIndex } from '../../../../../base/common/strings.js'; import { isDefined } from '../../../../../base/common/types.js'; import { IAccessibilityService } from '../../../../../platform/accessibility/common/accessibility.js'; import { ICommandService } from '../../../../../platform/commands/common/commands.js'; @@ -33,7 +33,7 @@ import { IFeatureDebounceInformation } from '../../../../common/services/languag import { ILanguageFeaturesService } from '../../../../common/services/languageFeatures.js'; import { IModelContentChangedEvent } from '../../../../common/textModelEvents.js'; import { SnippetController2 } from '../../../snippet/browser/snippetController2.js'; -import { addPositions, getEndPositionsAfterApplying, removeTextReplacementCommonSuffixPrefix, substringPos, subtractPositions } from '../utils.js'; +import { getEndPositionsAfterApplying, removeTextReplacementCommonSuffixPrefix } from '../utils.js'; import { AnimatedValue, easeOutCubic, ObservableAnimatedValue } from './animation.js'; import { computeGhostText } from './computeGhostText.js'; import { GhostText, GhostTextOrReplacement, ghostTextOrReplacementEquals, ghostTextsOrReplacementsEqual } from './ghostText.js'; @@ -48,6 +48,8 @@ import { ICodeEditorService } from '../../../../browser/services/codeEditorServi import { InlineCompletionViewData, InlineCompletionViewKind } from '../view/inlineEdits/inlineEditsViewInterface.js'; import { IInlineCompletionsService } from '../../../../browser/services/inlineCompletionsService.js'; import { TypingInterval } from './typingSpeed.js'; +import { StringReplacement } from '../../../../common/core/edits/stringEdit.js'; +import { OffsetRange } from '../../../../common/core/ranges/offsetRange.js'; export class InlineCompletionsModel extends Disposable { private readonly _source; @@ -576,10 +578,12 @@ export class InlineCompletionsModel extends Disposable { const mode = this._suggestPreviewMode.read(reader); const positions = this._positions.read(reader); - const edits = [fullEdit, ...getSecondaryEdits(this.textModel, positions, fullEdit)]; - const ghostTexts = edits - .map((edit, idx) => computeGhostText(edit, model, mode, positions[idx], fullEditPreviewLength)) - .filter(isDefined); + const allPotentialEdits = [fullEdit, ...getSecondaryEdits(this.textModel, positions, fullEdit)]; + const validEditsAndGhostTexts = allPotentialEdits + .map((edit, idx) => ({ edit, ghostText: edit ? computeGhostText(edit, model, mode, positions[idx], fullEditPreviewLength) : undefined })) + .filter(({ edit, ghostText }) => edit !== undefined && ghostText !== undefined); + const edits = validEditsAndGhostTexts.map(({ edit }) => edit!); + const ghostTexts = validEditsAndGhostTexts.map(({ ghostText }) => ghostText!); const primaryGhostText = ghostTexts[0] ?? new GhostText(fullEdit.range.endLineNumber, []); return { kind: 'ghostText', edits, primaryGhostText, ghostTexts, inlineCompletion: augmentation?.completion, suggestItem }; } else { @@ -590,10 +594,12 @@ export class InlineCompletionsModel extends Disposable { const replacement = inlineCompletion.getSingleTextEdit(); const mode = this._inlineSuggestMode.read(reader); const positions = this._positions.read(reader); - const edits = [replacement, ...getSecondaryEdits(this.textModel, positions, replacement)]; - const ghostTexts = edits - .map((edit, idx) => computeGhostText(edit, model, mode, positions[idx], 0)) - .filter(isDefined); + const allPotentialEdits = [replacement, ...getSecondaryEdits(this.textModel, positions, replacement)]; + const validEditsAndGhostTexts = allPotentialEdits + .map((edit, idx) => ({ edit, ghostText: edit ? computeGhostText(edit, model, mode, positions[idx], 0) : undefined })) + .filter(({ edit, ghostText }) => edit !== undefined && ghostText !== undefined); + const edits = validEditsAndGhostTexts.map(({ edit }) => edit!); + const ghostTexts = validEditsAndGhostTexts.map(({ ghostText }) => ghostText!); if (!ghostTexts[0]) { return undefined; } return { kind: 'ghostText', edits, primaryGhostText: ghostTexts[0], ghostTexts, inlineCompletion, suggestItem: undefined }; } @@ -955,7 +961,7 @@ export class InlineCompletionsModel extends Disposable { const replaceRange = Range.fromPositions(cursorPosition, ghostTextPos); const newText = editor.getModel()!.getValueInRange(replaceRange) + partialGhostTextVal; const primaryEdit = new TextReplacement(replaceRange, newText); - const edits = [primaryEdit, ...getSecondaryEdits(this.textModel, positions, primaryEdit)]; + const edits = [primaryEdit, ...getSecondaryEdits(this.textModel, positions, primaryEdit)].filter(isDefined); const selections = getEndPositionsAfterApplying(edits).map(p => Selection.fromPositions(p)); editor.edit(TextEdit.fromParallelReplacementsUnsorted(edits), this._getMetadata(completion, type)); @@ -1045,36 +1051,40 @@ export enum VersionIdChangeReason { Other, } -export function getSecondaryEdits(textModel: ITextModel, positions: readonly Position[], primaryEdit: TextReplacement): TextReplacement[] { +export function getSecondaryEdits(textModel: ITextModel, positions: readonly Position[], primaryTextRepl: TextReplacement): (TextReplacement | undefined)[] { if (positions.length === 1) { // No secondary cursor positions return []; } - const primaryPosition = positions[0]; - const secondaryPositions = positions.slice(1); - const primaryEditStartPosition = primaryEdit.range.getStartPosition(); - const primaryEditEndPosition = primaryEdit.range.getEndPosition(); - const replacedTextAfterPrimaryCursor = textModel.getValueInRange( - Range.fromPositions(primaryPosition, primaryEditEndPosition) - ); - const positionWithinTextEdit = subtractPositions(primaryPosition, primaryEditStartPosition); - if (positionWithinTextEdit.lineNumber < 1) { - onUnexpectedError(new BugIndicatingError( - `positionWithinTextEdit line number should be bigger than 0. - Invalid subtraction between ${primaryPosition.toString()} and ${primaryEditStartPosition.toString()}` - )); - return []; - } - const secondaryEditText = substringPos(primaryEdit.text, positionWithinTextEdit); - return secondaryPositions.map(pos => { - const posEnd = addPositions(subtractPositions(pos, primaryEditStartPosition), primaryEditEndPosition); - const textAfterSecondaryCursor = textModel.getValueInRange( - Range.fromPositions(pos, posEnd) - ); - const l = commonPrefixLength(replacedTextAfterPrimaryCursor, textAfterSecondaryCursor); - const range = Range.fromPositions(pos, pos.delta(0, l)); - return new TextReplacement(range, secondaryEditText); - }); + const text = new TextModelText(textModel); + const textTransformer = text.getTransformer(); + const primaryOffset = textTransformer.getOffset(positions[0]); + const secondaryOffsets = positions.slice(1).map(pos => textTransformer.getOffset(pos)); + + primaryTextRepl = primaryTextRepl.removeCommonPrefixAndSuffix(text); + const primaryStringRepl = textTransformer.getStringReplacement(primaryTextRepl); + + const deltaFromOffsetToRangeStart = primaryStringRepl.replaceRange.start - primaryOffset; + const primaryContextRange = primaryStringRepl.replaceRange.join(OffsetRange.emptyAt(primaryOffset)); + const primaryContextValue = text.getValueOfOffsetRange(primaryContextRange); + + const replacements = secondaryOffsets.map(secondaryOffset => { + const newRangeStart = secondaryOffset + deltaFromOffsetToRangeStart; + const newRangeEnd = newRangeStart + primaryStringRepl.replaceRange.length; + const range = new OffsetRange(newRangeStart, newRangeEnd); + + const contextRange = range.join(OffsetRange.emptyAt(secondaryOffset)); + const contextValue = text.getValueOfOffsetRange(contextRange); + if (contextValue !== primaryContextValue) { + return undefined; + } + + const stringRepl = new StringReplacement(range, primaryStringRepl.newText); + const repl = textTransformer.getTextReplacement(stringRepl); + return repl; + }).filter(isDefined); + + return replacements; } class FadeoutDecoration extends Disposable { diff --git a/src/vs/editor/contrib/inlineCompletions/browser/model/inlineSuggestionItem.ts b/src/vs/editor/contrib/inlineCompletions/browser/model/inlineSuggestionItem.ts index d09df6d1959..f95905ceb75 100644 --- a/src/vs/editor/contrib/inlineCompletions/browser/model/inlineSuggestionItem.ts +++ b/src/vs/editor/contrib/inlineCompletions/browser/model/inlineSuggestionItem.ts @@ -191,7 +191,7 @@ export class InlineCompletionItem extends InlineSuggestionItemBase { const edit = reshapeInlineCompletion(new StringReplacement(transformer.getOffsetRange(data.range), insertText), textModel); const trimmedEdit = edit.removeCommonSuffixAndPrefix(textModel.getValue()); - const textEdit = transformer.getSingleTextEdit(edit); + const textEdit = transformer.getTextReplacement(edit); const displayLocation = data.displayLocation ? InlineSuggestDisplayLocation.create(data.displayLocation) : undefined; @@ -242,7 +242,7 @@ export class InlineCompletionItem extends InlineSuggestionItemBase { } const newEdit = new StringReplacement(newEditRange[0], this._textEdit.text); const positionOffsetTransformer = getPositionOffsetTransformerFromTextModel(textModel); - const newTextEdit = positionOffsetTransformer.getSingleTextEdit(newEdit); + const newTextEdit = positionOffsetTransformer.getTextReplacement(newEdit); let newDisplayLocation = this.displayLocation; if (newDisplayLocation) { diff --git a/src/vs/editor/contrib/inlineCompletions/browser/utils.ts b/src/vs/editor/contrib/inlineCompletions/browser/utils.ts index 0115130cea8..44694055aa9 100644 --- a/src/vs/editor/contrib/inlineCompletions/browser/utils.ts +++ b/src/vs/editor/contrib/inlineCompletions/browser/utils.ts @@ -52,7 +52,7 @@ export function removeTextReplacementCommonSuffixPrefix(edits: readonly TextRepl const text = textModel.getValue(); const stringReplacements = edits.map(edit => transformer.getStringReplacement(edit)); const minimalStringReplacements = stringReplacements.map(replacement => replacement.removeCommonSuffixPrefix(text)); - return minimalStringReplacements.map(replacement => transformer.getSingleTextEdit(replacement)); + return minimalStringReplacements.map(replacement => transformer.getTextReplacement(replacement)); } export function convertItemsToStableObservables(items: IObservable, store: DisposableStore): IObservable[]> { diff --git a/src/vs/editor/contrib/inlineCompletions/test/browser/getSecondaryEdits.test.ts b/src/vs/editor/contrib/inlineCompletions/test/browser/getSecondaryEdits.test.ts index 1512dd4a06f..291c17bf0ae 100644 --- a/src/vs/editor/contrib/inlineCompletions/test/browser/getSecondaryEdits.test.ts +++ b/src/vs/editor/contrib/inlineCompletions/test/browser/getSecondaryEdits.test.ts @@ -5,10 +5,11 @@ import assert from 'assert'; import { Position } from '../../../../common/core/position.js'; import { getSecondaryEdits } from '../../browser/model/inlineCompletionsModel.js'; -import { TextReplacement } from '../../../../common/core/edits/textEdit.js'; +import { TextEdit, TextReplacement } from '../../../../common/core/edits/textEdit.js'; import { createTextModel } from '../../../../test/common/testTextModel.js'; import { Range } from '../../../../common/core/range.js'; import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../../../base/test/common/utils.js'; +import { isDefined } from '../../../../../base/common/types.js'; suite('getSecondaryEdits', () => { @@ -51,12 +52,7 @@ suite('getSecondaryEdits', () => { '}' ].join('\n')); const secondaryEdits = getSecondaryEdits(textModel, positions, primaryEdit); - assert.deepStrictEqual(secondaryEdits, [new TextReplacement( - new Range(4, 1, 4, 1), [ - ' return 0;', - '}' - ].join('\n') - )]); + assert.deepStrictEqual(TextEdit.fromParallelReplacementsUnsorted(secondaryEdits.filter(isDefined)).toString(textModel.getValue()), "...ction fib(❰\n↦) {\n\t... 0;\n}❱"); textModel.dispose(); }); diff --git a/src/vs/editor/contrib/inlineCompletions/test/browser/inlineCompletions.test.ts b/src/vs/editor/contrib/inlineCompletions/test/browser/inlineCompletions.test.ts index a8e0d0ea8b0..2bd599875cb 100644 --- a/src/vs/editor/contrib/inlineCompletions/test/browser/inlineCompletions.test.ts +++ b/src/vs/editor/contrib/inlineCompletions/test/browser/inlineCompletions.test.ts @@ -621,7 +621,7 @@ suite('Multi Cursor Support', () => { editor.getValue(), [ `console.log("hello");`, - `console.log("hello");`, + `console.log`, `` ].join('\n') ); @@ -650,7 +650,7 @@ suite('Multi Cursor Support', () => { editor.getValue(), [ `console.log("hello");`, - `console.warn("hello");`, + `console.warn`, `` ].join('\n') ); @@ -721,7 +721,7 @@ suite('Multi Cursor Support', () => { editor.getValue(), [ `for (let i)`, - `for (let i`, + `for `, `` ].join('\n') ); @@ -754,7 +754,7 @@ suite('Multi Cursor Support', () => { editor.getValue(), [ `console.log("hello" + )`, - `console.warnnnn("hello" + `, + `console.warnnnn`, `` ].join('\n') ); diff --git a/src/vs/platform/observable/common/platformObservableUtils.ts b/src/vs/platform/observable/common/platformObservableUtils.ts index 225357badc9..f6c9bb9116b 100644 --- a/src/vs/platform/observable/common/platformObservableUtils.ts +++ b/src/vs/platform/observable/common/platformObservableUtils.ts @@ -3,28 +3,40 @@ * Licensed under the MIT License. See License.txt in the project root for license information. *--------------------------------------------------------------------------------------------*/ -import { IDisposable } from '../../../base/common/lifecycle.js'; -import { autorunOpts, IObservable, IReader, observableFromEventOpts } from '../../../base/common/observable.js'; +import { DisposableStore, IDisposable } from '../../../base/common/lifecycle.js'; +import { derivedOpts, IObservable, IReader, observableFromEventOpts } from '../../../base/common/observable.js'; import { IConfigurationService } from '../../configuration/common/configuration.js'; import { ContextKeyValue, IContextKeyService, RawContextKey } from '../../contextkey/common/contextkey.js'; /** Creates an observable update when a configuration key updates. */ export function observableConfigValue(key: string, defaultValue: T, configurationService: IConfigurationService): IObservable { - return observableFromEventOpts({ debugName: () => `Configuration Key "${key}"`, }, - (handleChange) => configurationService.onDidChangeConfiguration(e => { - if (e.affectsConfiguration(key)) { - handleChange(e); - } - }), - () => configurationService.getValue(key) ?? defaultValue - ); + function compute_$show2FramesUp() { + return observableFromEventOpts({ debugName: () => `Configuration Key "${key}"`, }, + (handleChange) => configurationService.onDidChangeConfiguration(e => { + if (e.affectsConfiguration(key)) { + handleChange(e); + } + }), + () => configurationService.getValue(key) ?? defaultValue + ); + } + return compute_$show2FramesUp(); } /** Update the configuration key with a value derived from observables. */ export function bindContextKey(key: RawContextKey, service: IContextKeyService, computeValue: (reader: IReader) => T): IDisposable { const boundKey = key.bindTo(service); - return autorunOpts({ debugName: () => `Set Context Key "${key.key}"` }, reader => { - boundKey.set(computeValue(reader)); - }); + + function compute_$show2FramesUp() { + const store = new DisposableStore(); + derivedOpts({ debugName: () => `Set Context Key "${key.key}"` }, reader => { + const value = computeValue(reader); + boundKey.set(value); + return value; + }).recomputeInitiallyAndOnChange(store); + return store; + } + + return compute_$show2FramesUp(); }