From 17cb04a7c500b8c9168ee8997762bc7b6bb5f84a Mon Sep 17 00:00:00 2001 From: Oleg Solomko Date: Fri, 21 Feb 2025 13:01:10 -0800 Subject: [PATCH 1/5] II of the `MarkersProvider` class --- .../utils/createPromptFile.ts | 2 +- .../languageFeatures/markersProvider.ts | 144 ++++++++++++++++++ .../chat/common/promptFileReferenceErrors.ts | 28 ++-- .../promptSyntax/parsers/basePromptParser.ts | 1 - .../promptSyntax/utils/promptFilesLocator.ts | 2 +- 5 files changed, 160 insertions(+), 17 deletions(-) create mode 100644 src/vs/workbench/contrib/chat/browser/promptSyntax/languageFeatures/markersProvider.ts diff --git a/src/vs/workbench/contrib/chat/browser/promptSyntax/contributions/createPromptCommand/utils/createPromptFile.ts b/src/vs/workbench/contrib/chat/browser/promptSyntax/contributions/createPromptCommand/utils/createPromptFile.ts index d449726d917d2..710637d2fdfff 100644 --- a/src/vs/workbench/contrib/chat/browser/promptSyntax/contributions/createPromptCommand/utils/createPromptFile.ts +++ b/src/vs/workbench/contrib/chat/browser/promptSyntax/contributions/createPromptCommand/utils/createPromptFile.ts @@ -9,8 +9,8 @@ import { assert } from '../../../../../../../../base/common/assert.js'; import { VSBuffer } from '../../../../../../../../base/common/buffer.js'; import { dirname } from '../../../../../../../../base/common/resources.js'; import { IFileService } from '../../../../../../../../platform/files/common/files.js'; -import { isPromptFile, PROMPT_FILE_EXTENSION } from '../../../../../../../../platform/prompts/common/constants.js'; import { ICommandService } from '../../../../../../../../platform/commands/common/commands.js'; +import { isPromptFile, PROMPT_FILE_EXTENSION } from '../../../../../../../../platform/prompts/common/constants.js'; /** * Options for the {@link createPromptFile} utility. diff --git a/src/vs/workbench/contrib/chat/browser/promptSyntax/languageFeatures/markersProvider.ts b/src/vs/workbench/contrib/chat/browser/promptSyntax/languageFeatures/markersProvider.ts new file mode 100644 index 0000000000000..a58c56320b8a8 --- /dev/null +++ b/src/vs/workbench/contrib/chat/browser/promptSyntax/languageFeatures/markersProvider.ts @@ -0,0 +1,144 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +import { assert } from '../../../../../../base/common/assert.js'; +import { ITextModel } from '../../../../../../editor/common/model.js'; +import { assertDefined } from '../../../../../../base/common/types.js'; +import { Disposable } from '../../../../../../base/common/lifecycle.js'; +import { IPromptsService } from '../../../common/promptSyntax/service/types.js'; +import { IPromptFileReference } from '../../../common/promptSyntax/parsers/types.js'; +import { TextModelPromptParser } from '../../../common/promptSyntax/parsers/textModelPromptParser.js'; +import { IMarkerData, IMarkerService, MarkerSeverity } from '../../../../../../platform/markers/common/markers.js'; +import { FailedToResolveContentsStream, NotPromptFile, ParseError, RecursiveReference } from '../../../common/promptFileReferenceErrors.js'; + +/** + * TODO: @legomushroom + */ +const MARKERS_OWNER_ID = 'reusable-prompts-syntax'; + +/** + * TODO: @legomushroom + */ +// TODO: @lego - move under /common +export class MarkersProvider extends Disposable { + /** + * TODO: @legomushroom + */ + private readonly parser: TextModelPromptParser; + + constructor( + private readonly editor: ITextModel, + @IMarkerService private readonly markerService: IMarkerService, + @IPromptsService private readonly promptsService: IPromptsService, + ) { + super(); + + this.parser = this.promptsService + .getSyntaxParserFor(this.editor) + .onUpdate(this.updateMarkers.bind(this)) + .onDispose(this.dispose.bind(this)); + + // TODO @legomushroom - uncomment? + // this.updateMarkers(); + } + + /** + * TODO: @legomushroom + */ + private async updateMarkers() { + // ensure that parsing process is settled + await this.parser.allSettled(); + + // clean up all previously added markers + this.markerService.remove(MARKERS_OWNER_ID, [this.editor.uri]); + + const markers: IMarkerData[] = []; + for (const link of this.parser.allReferences) { + const { errorCondition, linkRange } = link; + + if (!errorCondition || !linkRange) { + continue; + } + + // the `NotPromptFile` error is allowed because we allow users + // to include non-prompt file links in the prompt files + // note! this check also handles the `FolderReference` error + if (errorCondition instanceof NotPromptFile) { + continue; + } + + markers.push(toMarker(link)); + } + + this.markerService.changeOne( + MARKERS_OWNER_ID, + this.editor.uri, + markers, + ); + } +} + +/** + * TODO: @legomushroom + */ +// TODO: @legomushroom - add @throws info +const toMarker = ( + link: IPromptFileReference, +): IMarkerData => { + const { errorCondition, linkRange } = link; + + // a sanity check because this function must be + // used only if these attributes are present + assertDefined( + errorCondition, + 'Error condition must to be defined.', + ); + assertDefined( + linkRange, + 'Link range must to be defined.', + ); + assert( + !(errorCondition instanceof NotPromptFile), + 'Error must not be of "not prompt file" type.', + ); + + // `FailedToResolveContentsStream` error check takes into account the `OpenFailed` error too + const severity = (errorCondition instanceof FailedToResolveContentsStream) + ? MarkerSeverity.Error + : MarkerSeverity.Warning; + + return { + message: getErrorMessage(errorCondition), + severity, + ...linkRange, + }; +}; + +/** + * TODO: @legomushroom + */ +// TODO: @legomushroom - add @throws info +// TODO: @legomushroom - localize or reuse existing messages +const getErrorMessage = ( + error: ParseError, +): string => { + // a sanity check because this function must be + // used only if error has correct type + assert( + !(error instanceof NotPromptFile), + 'Error must not be of "not prompt file" type.', + ); + + // `FailedToResolveContentsStream` error takes into account the `OpenFailed` error too + if (error instanceof FailedToResolveContentsStream) { + return `The file '${error.uri.fsPath}' is not found.`; + } + + if (error instanceof RecursiveReference) { + return 'The file contains recursive child reference that will be ignored.'; + } + + return `An unexpected error occurred: ${error.message}`; +}; diff --git a/src/vs/workbench/contrib/chat/common/promptFileReferenceErrors.ts b/src/vs/workbench/contrib/chat/common/promptFileReferenceErrors.ts index 84e7d62551b64..5f75ba3da0d26 100644 --- a/src/vs/workbench/contrib/chat/common/promptFileReferenceErrors.ts +++ b/src/vs/workbench/contrib/chat/common/promptFileReferenceErrors.ts @@ -41,36 +41,36 @@ export abstract class ParseError extends Error { } /** - * A generic error for failing to resolve prompt contents stream. + * Base resolve error class used when file reference resolution fails. */ -export class FailedToResolveContentsStream extends ParseError { - public override errorType = 'FailedToResolveContentsStream'; +export abstract class ResolveError extends ParseError { + public abstract override errorType: string; constructor( public readonly uri: URI, - public readonly originalError: unknown, - message: string = `Failed to resolve prompt contents stream for '${uri.toString()}': ${originalError}.`, + message?: string, + options?: ErrorOptions, ) { - super(message); + super(message, options); } } - /** - * Base resolve error class used when file reference resolution fails. + * A generic error for failing to resolve prompt contents stream. */ -export abstract class ResolveError extends ParseError { - public abstract override errorType: string; +export class FailedToResolveContentsStream extends ResolveError { + public override errorType = 'FailedToResolveContentsStream'; constructor( - public readonly uri: URI, - message?: string, - options?: ErrorOptions, + uri: URI, + public readonly originalError: unknown, + message: string = `Failed to resolve prompt contents stream for '${uri.toString()}': ${originalError}.`, ) { - super(message, options); + super(uri, message); } } + /** * Error that reflects the case when attempt to open target file fails. */ diff --git a/src/vs/workbench/contrib/chat/common/promptSyntax/parsers/basePromptParser.ts b/src/vs/workbench/contrib/chat/common/promptSyntax/parsers/basePromptParser.ts index 937c7e6b1bb00..b53f4109cb322 100644 --- a/src/vs/workbench/contrib/chat/common/promptSyntax/parsers/basePromptParser.ts +++ b/src/vs/workbench/contrib/chat/common/promptSyntax/parsers/basePromptParser.ts @@ -452,7 +452,6 @@ export abstract class BasePromptParser extend return undefined; } - // if the first error is the error of the root reference, // then return it as an `error` otherwise use `warning` const [firstError, ...restErrors] = errors; diff --git a/src/vs/workbench/contrib/chat/common/promptSyntax/utils/promptFilesLocator.ts b/src/vs/workbench/contrib/chat/common/promptSyntax/utils/promptFilesLocator.ts index 2d787402fbe16..348b9b6280151 100644 --- a/src/vs/workbench/contrib/chat/common/promptSyntax/utils/promptFilesLocator.ts +++ b/src/vs/workbench/contrib/chat/common/promptSyntax/utils/promptFilesLocator.ts @@ -13,7 +13,7 @@ import { IWorkspaceContextService } from '../../../../../../platform/workspace/c import { IConfigurationService } from '../../../../../../platform/configuration/common/configuration.js'; /** - * Class to locate prompt files. + * Utility class to locate prompt files. */ export class PromptFilesLocator { constructor( From 068ba7281041d92ffb9968a77beec2c6c01f665e Mon Sep 17 00:00:00 2001 From: Oleg Solomko Date: Fri, 21 Feb 2025 14:21:42 -0800 Subject: [PATCH 2/5] II of the `PromptsLinkDiagnosticsProvider` contribution --- .../contrib/chat/browser/chat.contribution.ts | 1 + .../languageFeatures/markersProvider.ts | 144 ------------ .../promptLinkDiagnosticsProvider.ts | 220 ++++++++++++++++++ 3 files changed, 221 insertions(+), 144 deletions(-) delete mode 100644 src/vs/workbench/contrib/chat/browser/promptSyntax/languageFeatures/markersProvider.ts create mode 100644 src/vs/workbench/contrib/chat/common/promptSyntax/languageFeatures/promptLinkDiagnosticsProvider.ts diff --git a/src/vs/workbench/contrib/chat/browser/chat.contribution.ts b/src/vs/workbench/contrib/chat/browser/chat.contribution.ts index 29bd893d14999..3536107163edf 100644 --- a/src/vs/workbench/contrib/chat/browser/chat.contribution.ts +++ b/src/vs/workbench/contrib/chat/browser/chat.contribution.ts @@ -42,6 +42,7 @@ import { ILanguageModelStatsService, LanguageModelStatsService } from '../common import { ILanguageModelToolsService } from '../common/languageModelToolsService.js'; import '../common/promptSyntax/languageFeatures/promptLinkProvider.js'; import '../common/promptSyntax/languageFeatures/promptPathAutocompletion.js'; +import '../common/promptSyntax/languageFeatures/promptLinkDiagnosticsProvider.js'; import { PromptsService } from '../common/promptSyntax/service/promptsService.js'; import { IPromptsService } from '../common/promptSyntax/service/types.js'; import './promptSyntax/contributions/usePromptCommand.js'; diff --git a/src/vs/workbench/contrib/chat/browser/promptSyntax/languageFeatures/markersProvider.ts b/src/vs/workbench/contrib/chat/browser/promptSyntax/languageFeatures/markersProvider.ts deleted file mode 100644 index a58c56320b8a8..0000000000000 --- a/src/vs/workbench/contrib/chat/browser/promptSyntax/languageFeatures/markersProvider.ts +++ /dev/null @@ -1,144 +0,0 @@ -/*--------------------------------------------------------------------------------------------- - * Copyright (c) Microsoft Corporation. All rights reserved. - * Licensed under the MIT License. See License.txt in the project root for license information. - *--------------------------------------------------------------------------------------------*/ - -import { assert } from '../../../../../../base/common/assert.js'; -import { ITextModel } from '../../../../../../editor/common/model.js'; -import { assertDefined } from '../../../../../../base/common/types.js'; -import { Disposable } from '../../../../../../base/common/lifecycle.js'; -import { IPromptsService } from '../../../common/promptSyntax/service/types.js'; -import { IPromptFileReference } from '../../../common/promptSyntax/parsers/types.js'; -import { TextModelPromptParser } from '../../../common/promptSyntax/parsers/textModelPromptParser.js'; -import { IMarkerData, IMarkerService, MarkerSeverity } from '../../../../../../platform/markers/common/markers.js'; -import { FailedToResolveContentsStream, NotPromptFile, ParseError, RecursiveReference } from '../../../common/promptFileReferenceErrors.js'; - -/** - * TODO: @legomushroom - */ -const MARKERS_OWNER_ID = 'reusable-prompts-syntax'; - -/** - * TODO: @legomushroom - */ -// TODO: @lego - move under /common -export class MarkersProvider extends Disposable { - /** - * TODO: @legomushroom - */ - private readonly parser: TextModelPromptParser; - - constructor( - private readonly editor: ITextModel, - @IMarkerService private readonly markerService: IMarkerService, - @IPromptsService private readonly promptsService: IPromptsService, - ) { - super(); - - this.parser = this.promptsService - .getSyntaxParserFor(this.editor) - .onUpdate(this.updateMarkers.bind(this)) - .onDispose(this.dispose.bind(this)); - - // TODO @legomushroom - uncomment? - // this.updateMarkers(); - } - - /** - * TODO: @legomushroom - */ - private async updateMarkers() { - // ensure that parsing process is settled - await this.parser.allSettled(); - - // clean up all previously added markers - this.markerService.remove(MARKERS_OWNER_ID, [this.editor.uri]); - - const markers: IMarkerData[] = []; - for (const link of this.parser.allReferences) { - const { errorCondition, linkRange } = link; - - if (!errorCondition || !linkRange) { - continue; - } - - // the `NotPromptFile` error is allowed because we allow users - // to include non-prompt file links in the prompt files - // note! this check also handles the `FolderReference` error - if (errorCondition instanceof NotPromptFile) { - continue; - } - - markers.push(toMarker(link)); - } - - this.markerService.changeOne( - MARKERS_OWNER_ID, - this.editor.uri, - markers, - ); - } -} - -/** - * TODO: @legomushroom - */ -// TODO: @legomushroom - add @throws info -const toMarker = ( - link: IPromptFileReference, -): IMarkerData => { - const { errorCondition, linkRange } = link; - - // a sanity check because this function must be - // used only if these attributes are present - assertDefined( - errorCondition, - 'Error condition must to be defined.', - ); - assertDefined( - linkRange, - 'Link range must to be defined.', - ); - assert( - !(errorCondition instanceof NotPromptFile), - 'Error must not be of "not prompt file" type.', - ); - - // `FailedToResolveContentsStream` error check takes into account the `OpenFailed` error too - const severity = (errorCondition instanceof FailedToResolveContentsStream) - ? MarkerSeverity.Error - : MarkerSeverity.Warning; - - return { - message: getErrorMessage(errorCondition), - severity, - ...linkRange, - }; -}; - -/** - * TODO: @legomushroom - */ -// TODO: @legomushroom - add @throws info -// TODO: @legomushroom - localize or reuse existing messages -const getErrorMessage = ( - error: ParseError, -): string => { - // a sanity check because this function must be - // used only if error has correct type - assert( - !(error instanceof NotPromptFile), - 'Error must not be of "not prompt file" type.', - ); - - // `FailedToResolveContentsStream` error takes into account the `OpenFailed` error too - if (error instanceof FailedToResolveContentsStream) { - return `The file '${error.uri.fsPath}' is not found.`; - } - - if (error instanceof RecursiveReference) { - return 'The file contains recursive child reference that will be ignored.'; - } - - return `An unexpected error occurred: ${error.message}`; -}; diff --git a/src/vs/workbench/contrib/chat/common/promptSyntax/languageFeatures/promptLinkDiagnosticsProvider.ts b/src/vs/workbench/contrib/chat/common/promptSyntax/languageFeatures/promptLinkDiagnosticsProvider.ts new file mode 100644 index 0000000000000..b3efbf3d20cca --- /dev/null +++ b/src/vs/workbench/contrib/chat/common/promptSyntax/languageFeatures/promptLinkDiagnosticsProvider.ts @@ -0,0 +1,220 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +import { IPromptsService } from '../service/types.js'; +import { IPromptFileReference } from '../parsers/types.js'; +import { assert } from '../../../../../../base/common/assert.js'; +import { NotPromptFile } from '../../promptFileReferenceErrors.js'; +import { ITextModel } from '../../../../../../editor/common/model.js'; +import { assertDefined } from '../../../../../../base/common/types.js'; +import { Disposable } from '../../../../../../base/common/lifecycle.js'; +import { IEditor } from '../../../../../../editor/common/editorCommon.js'; +import { ObjectCache } from '../../../../../../base/common/objectCache.js'; +import { TextModelPromptParser } from '../parsers/textModelPromptParser.js'; +import { Registry } from '../../../../../../platform/registry/common/platform.js'; +import { PromptsConfig } from '../../../../../../platform/prompts/common/config.js'; +import { isPromptFile } from '../../../../../../platform/prompts/common/constants.js'; +import { LifecyclePhase } from '../../../../../services/lifecycle/common/lifecycle.js'; +import { IEditorService } from '../../../../../services/editor/common/editorService.js'; +import { ObservableDisposable } from '../../../../../../base/common/observableDisposable.js'; +import { IWorkbenchContributionsRegistry, Extensions } from '../../../../../common/contributions.js'; +import { IInstantiationService } from '../../../../../../platform/instantiation/common/instantiation.js'; +import { IConfigurationService } from '../../../../../../platform/configuration/common/configuration.js'; +import { IMarkerData, IMarkerService, MarkerSeverity } from '../../../../../../platform/markers/common/markers.js'; + +/** + * TODO: @legomushroom - list + * - improve error messages + */ + +/** + * TODO: @legomushroom + */ +const MARKERS_OWNER_ID = 'reusable-prompts-syntax'; + +/** + * TODO: @legomushroom + */ +class PromptLinkDiagnosticsProvider extends ObservableDisposable { + /** + * TODO: @legomushroom + */ + private readonly parser: TextModelPromptParser; + + constructor( + private readonly editor: ITextModel, + @IMarkerService private readonly markerService: IMarkerService, + @IPromptsService private readonly promptsService: IPromptsService, + ) { + super(); + + this.parser = this.promptsService + .getSyntaxParserFor(this.editor) + .onUpdate(this.updateMarkers.bind(this)) + .onDispose(this.dispose.bind(this)) + .start(); + + // initialize markers + this.updateMarkers(); + } + + /** + * TODO: @legomushroom + */ + private async updateMarkers() { + // ensure that parsing process is settled + await this.parser.allSettled(); + + // clean up all previously added markers + this.markerService.remove(MARKERS_OWNER_ID, [this.editor.uri]); + + const markers: IMarkerData[] = []; + for (const link of this.parser.references) { + const { topError, linkRange } = link; + + if (!topError || !linkRange) { + continue; + } + + // the `NotPromptFile` error is allowed because we allow users + // to include non-prompt file links in the prompt files + // note! this check also handles the `FolderReference` error + if (topError instanceof NotPromptFile) { + continue; + } + + markers.push(toMarker(link)); + } + + this.markerService.changeOne( + MARKERS_OWNER_ID, + this.editor.uri, + markers, + ); + } +} + +/** + * TODO: @legomushroom + */ +// TODO: @legomushroom - add @throws info +const toMarker = ( + link: IPromptFileReference, +): IMarkerData => { + const { topError, linkRange } = link; + + // a sanity check because this function must be + // used only if these attributes are present + assertDefined( + topError, + 'Error condition must to be defined.', + ); + assertDefined( + linkRange, + 'Link range must to be defined.', + ); + assert( + !(topError instanceof NotPromptFile), + 'Error must not be of "not prompt file" type.', + ); + + // use `error` severity if the error relates to the link itself, and use + // the `warning` severity if the error is related to one of its children + const severity = (topError.isRootError) + ? MarkerSeverity.Error + : MarkerSeverity.Warning; + + return { + message: topError.message, + severity, + ...linkRange, + }; +}; + +/** + * TODO: @legomushroom + */ +export class PromptsLinkDiagnosticsProvider extends Disposable { + /** + * TODO: @legomushroom + */ + private readonly providers: ObjectCache; + + constructor( + @IEditorService editorService: IEditorService, + @IInstantiationService initService: IInstantiationService, + @IConfigurationService configService: IConfigurationService, + ) { + super(); + + // cache of prompt marker providers + this.providers = this._register( + new ObjectCache((editor: ITextModel) => { + const parser: PromptLinkDiagnosticsProvider = initService.createInstance( + PromptLinkDiagnosticsProvider, + editor, + ); + + // this is a sanity check and the contract of the object cache, + // we must return a non-disposed object from this factory function + parser.assertNotDisposed( + 'Created prompt parser must not be disposed.', + ); + + return parser; + }), + ); + + // if the feature is disabled, do not create any providers + if (!PromptsConfig.enabled(configService)) { + return; + } + + // subscribe to changes of the active editor + this._register(editorService.onDidActiveEditorChange(() => { + const { activeTextEditorControl } = editorService; + if (!activeTextEditorControl) { + return; + } + + this.handleNewEditor(activeTextEditorControl); + })); + + // handle existing visible text editors + editorService + .visibleTextEditorControls + .forEach(this.handleNewEditor.bind(this)); + } + + /** + * TODO: @legomushroom + */ + private handleNewEditor(editor: IEditor): this { + const model = editor.getModel(); + if (!model) { + return this; + } + + // we support only `text editors` for now so filter out `diff` ones + if ('modified' in model || 'model' in model) { + return this; + } + + // enable this only for prompt file editors + if (!isPromptFile(model.uri)) { + return this; + } + + // note! calling `get` also creates a provider if it does not exist; + // and the provider is auto-removed when the model is disposed + this.providers.get(model); + + return this; + } +} + +// register the provider as a workbench contribution +Registry.as(Extensions.Workbench) + .registerWorkbenchContribution(PromptsLinkDiagnosticsProvider, LifecyclePhase.Eventually); From de22815af8aca822e8b565a80957b1ab6e0e8324 Mon Sep 17 00:00:00 2001 From: Oleg Solomko Date: Fri, 21 Feb 2025 18:38:20 -0800 Subject: [PATCH 3/5] improve error messages --- .../promptAttachmentWidget.ts | 2 +- .../chat/common/promptFileReferenceErrors.ts | 2 +- .../filePromptContentsProvider.ts | 4 +- .../promptContentsProviderBase.ts | 12 +- .../promptLinkDiagnosticsProvider.ts | 14 +- .../promptSyntax/parsers/basePromptParser.ts | 185 ++++++++++++++---- .../common/promptSyntax/parsers/types.d.ts | 33 +++- .../testUtils/expectedReference.ts | 8 +- 8 files changed, 195 insertions(+), 65 deletions(-) diff --git a/src/vs/workbench/contrib/chat/browser/attachments/promptAttachments/promptAttachmentWidget.ts b/src/vs/workbench/contrib/chat/browser/attachments/promptAttachments/promptAttachmentWidget.ts index 8c1068988f56e..cb082eaae6d24 100644 --- a/src/vs/workbench/contrib/chat/browser/attachments/promptAttachments/promptAttachmentWidget.ts +++ b/src/vs/workbench/contrib/chat/browser/attachments/promptAttachments/promptAttachmentWidget.ts @@ -118,7 +118,7 @@ export class PromptAttachmentWidget extends Disposable { // add the issue details in the hover title for the attachment, one // error/warning at a time because there is a limited space available if (topError) { - const { isRootError, message: details } = topError; + const { isRootError, localizedMessage: details } = topError; const isWarning = !isRootError; this.domNode.classList.add( diff --git a/src/vs/workbench/contrib/chat/common/promptFileReferenceErrors.ts b/src/vs/workbench/contrib/chat/common/promptFileReferenceErrors.ts index 5f75ba3da0d26..40cec4f0503ff 100644 --- a/src/vs/workbench/contrib/chat/common/promptFileReferenceErrors.ts +++ b/src/vs/workbench/contrib/chat/common/promptFileReferenceErrors.ts @@ -8,7 +8,7 @@ import { URI } from '../../../../base/common/uri.js'; /** * Base prompt parsing error class. */ -export abstract class ParseError extends Error { +abstract class ParseError extends Error { /** * Error type name. */ diff --git a/src/vs/workbench/contrib/chat/common/promptSyntax/contentProviders/filePromptContentsProvider.ts b/src/vs/workbench/contrib/chat/common/promptSyntax/contentProviders/filePromptContentsProvider.ts index 512c29e98c23f..4f35897df0a99 100644 --- a/src/vs/workbench/contrib/chat/common/promptSyntax/contentProviders/filePromptContentsProvider.ts +++ b/src/vs/workbench/contrib/chat/common/promptSyntax/contentProviders/filePromptContentsProvider.ts @@ -11,7 +11,7 @@ import { CancellationError } from '../../../../../../base/common/errors.js'; import { PromptContentsProviderBase } from './promptContentsProviderBase.js'; import { VSBufferReadableStream } from '../../../../../../base/common/buffer.js'; import { CancellationToken } from '../../../../../../base/common/cancellation.js'; -import { OpenFailed, NotPromptFile, ParseError, FolderReference } from '../../promptFileReferenceErrors.js'; +import { OpenFailed, NotPromptFile, ResolveError, FolderReference } from '../../promptFileReferenceErrors.js'; import { FileChangesEvent, FileChangeType, IFileService } from '../../../../../../platform/files/common/files.js'; /** @@ -81,7 +81,7 @@ export class FilePromptContentProvider extends PromptContentsProviderBase()); + private readonly onContentChangedEmitter = this._register(new Emitter()); /** * Event that fires when the prompt contents change. The event is either * a `VSBufferReadableStream` stream with changed contents or an instance of - * the `ParseError` class representing a parsing failure case. + * the `ResolveError` class representing a parsing failure case. * * `Note!` this field is meant to be used by the external consumers of the prompt * contents provider that the classes that extend this abstract class. @@ -112,7 +112,7 @@ export abstract class PromptContentsProviderBase< this.onContentChangedEmitter.fire(stream); }) .catch((error) => { - if (error instanceof ParseError) { + if (error instanceof ResolveError) { this.onContentChangedEmitter.fire(error); return; diff --git a/src/vs/workbench/contrib/chat/common/promptSyntax/languageFeatures/promptLinkDiagnosticsProvider.ts b/src/vs/workbench/contrib/chat/common/promptSyntax/languageFeatures/promptLinkDiagnosticsProvider.ts index b3efbf3d20cca..114687d46d785 100644 --- a/src/vs/workbench/contrib/chat/common/promptSyntax/languageFeatures/promptLinkDiagnosticsProvider.ts +++ b/src/vs/workbench/contrib/chat/common/promptSyntax/languageFeatures/promptLinkDiagnosticsProvider.ts @@ -78,10 +78,12 @@ class PromptLinkDiagnosticsProvider extends ObservableDisposable { continue; } + const { originalError } = topError; + // the `NotPromptFile` error is allowed because we allow users // to include non-prompt file links in the prompt files // note! this check also handles the `FolderReference` error - if (topError instanceof NotPromptFile) { + if (originalError instanceof NotPromptFile) { continue; } @@ -106,17 +108,19 @@ const toMarker = ( const { topError, linkRange } = link; // a sanity check because this function must be - // used only if these attributes are present + // used only if these link attributes are present assertDefined( topError, - 'Error condition must to be defined.', + 'Top error must to be defined.', ); assertDefined( linkRange, 'Link range must to be defined.', ); + + const { originalError } = topError; assert( - !(topError instanceof NotPromptFile), + !(originalError instanceof NotPromptFile), 'Error must not be of "not prompt file" type.', ); @@ -127,7 +131,7 @@ const toMarker = ( : MarkerSeverity.Warning; return { - message: topError.message, + message: topError.localizedMessage, severity, ...linkRange, }; diff --git a/src/vs/workbench/contrib/chat/common/promptSyntax/parsers/basePromptParser.ts b/src/vs/workbench/contrib/chat/common/promptSyntax/parsers/basePromptParser.ts index b53f4109cb322..f9fba66926716 100644 --- a/src/vs/workbench/contrib/chat/common/promptSyntax/parsers/basePromptParser.ts +++ b/src/vs/workbench/contrib/chat/common/promptSyntax/parsers/basePromptParser.ts @@ -8,7 +8,6 @@ import { URI } from '../../../../../../base/common/uri.js'; import { ChatPromptCodec } from '../codecs/chatPromptCodec.js'; import { Emitter } from '../../../../../../base/common/event.js'; import { assert } from '../../../../../../base/common/assert.js'; -import { IPromptFileReference, IResolveError } from './types.js'; import { FileReference } from '../codecs/tokens/fileReference.js'; import { ChatPromptDecoder } from '../codecs/chatPromptDecoder.js'; import { IRange } from '../../../../../../editor/common/core/range.js'; @@ -17,22 +16,71 @@ import { IPromptContentsProvider } from '../contentProviders/types.js'; import { DeferredPromise } from '../../../../../../base/common/async.js'; import { ILogService } from '../../../../../../platform/log/common/log.js'; import { basename, extUri } from '../../../../../../base/common/resources.js'; +import { IErrorWithLink, IPromptFileReference, IResolveError } from './types.js'; import { VSBufferReadableStream } from '../../../../../../base/common/buffer.js'; import { isPromptFile } from '../../../../../../platform/prompts/common/constants.js'; import { ObservableDisposable } from '../../../../../../base/common/observableDisposable.js'; import { FilePromptContentProvider } from '../contentProviders/filePromptContentsProvider.js'; import { IInstantiationService } from '../../../../../../platform/instantiation/common/instantiation.js'; import { MarkdownLink } from '../../../../../../editor/common/codecs/markdownCodec/tokens/markdownLink.js'; -import { OpenFailed, NotPromptFile, RecursiveReference, FolderReference, ParseError, FailedToResolveContentsStream } from '../../promptFileReferenceErrors.js'; +import { OpenFailed, NotPromptFile, RecursiveReference, FolderReference, ResolveError, FailedToResolveContentsStream } from '../../promptFileReferenceErrors.js'; + +/** + * TODO: @legomushroom - list + * + * - fix recursive reference case: + * - it should not be an error, but warning + * - [in `brokenChild`]: fix the error message + */ /** * Well-known localized error messages. */ const errorMessages = { - recursion: localize('chatPromptInstructionsRecursiveReference', 'Recursive reference found'), - fileOpenFailed: localize('chatPromptInstructionsFileOpenFailed', 'Failed to open file'), - streamOpenFailed: localize('chatPromptInstructionsStreamOpenFailed', 'Failed to open contents stream'), - brokenChild: localize('chatPromptInstructionsBrokenReference', 'Contains a broken reference that will be ignored'), + // TODO: @legomushroom - update localization IDs + recursion: localize('chatPromptInstructionsRecursiveReference', 'Recursive reference'), + openFailed: localize('chatPromptInstructionsFileOpenFailed', 'Cannot open'), + streamOpenFailed: localize('chatPromptInstructionsStreamOpenFailed', 'Cannot read'), + + /** + * TODO: @legomushroom - add @throws tag + */ + brokenChild: ( + error: IErrorWithLink, + isDirectChild: boolean, + moreErrorsCount: number, + ): string => { + // sanity check + assert( + moreErrorsCount >= 0, + `Additional errors count must be non-negative, got '${moreErrorsCount}'.`, + ); + + const subjectName = (isDirectChild) + ? localize('chatPromptInstructionsBrokenReferenceFile', "Contains") + : localize( + 'chatPromptInstructionsBrokenChildReference', + "Indirectly referenced file '{0}' contains", + basename(error.link.uri), + ); + + const linkErrorType = (error.error instanceof RecursiveReference) + ? localize('chatPromptInstructionsBrokenReferenceFileRecursive', "recursive") + : localize('chatPromptInstructionsBrokenChildReferenceBroken', "broken"); + + const moreErrorsNote = (moreErrorsCount > 0) + ? localize('chatPromptInstructionsBrokenReferenceSuffix', " (+{0} more warnings)", moreErrorsCount) + : ''; + + return localize( + 'chatPromptInstructionsBrokenReference', + "{0} a {1} link '{2}' that will be ignored.{3}", + subjectName, + linkErrorType, + error.link.text, + moreErrorsNote, + ); + }, }; /** @@ -65,13 +113,13 @@ export abstract class BasePromptParser extend return this; } - private _errorCondition?: ParseError; + private _errorCondition?: ResolveError; /** * If file reference resolution fails, this attribute will be set * to an error instance that describes the error condition. */ - public get errorCondition(): ParseError | undefined { + public get errorCondition(): ResolveError | undefined { return this._errorCondition; } @@ -196,7 +244,7 @@ export abstract class BasePromptParser extend * references recursion. */ private onContentsChanged( - streamOrError: VSBufferReadableStream | ParseError, + streamOrError: VSBufferReadableStream | ResolveError, seenReferences: string[], ): void { // dispose and cleanup the previously received stream @@ -209,7 +257,7 @@ export abstract class BasePromptParser extend this.disposeReferences(); // if an error received, set up the error condition and stop - if (streamOrError instanceof ParseError) { + if (streamOrError instanceof ResolveError) { this._errorCondition = streamOrError; this._onUpdate.fire(); @@ -399,12 +447,47 @@ export abstract class BasePromptParser extend .map(child => child.uri); } + /** + * TODO: @legomushroom + */ + public get errors(): readonly IErrorWithLink[] { + // collect error conditions of direct child references + const childErrors = this + // get immediate references + .references + // filter out children without error conditions or + // the ones that are non-prompt snippet files + .filter((childReference) => { + const { errorCondition } = childReference; + + return errorCondition && !(errorCondition instanceof NotPromptFile); + }) + // map to error condition objects + .map((childReference): IErrorWithLink => { + const { errorCondition } = childReference; + + // `must` always be `true` because of the `filter` call above + assertDefined( + errorCondition, + `Error condition must be present for '${childReference.uri.path}'.`, + ); + + return { + link: childReference, + error: errorCondition, + }; + }); + + return childErrors; + } + /** * List of all errors that occurred while resolving the current * reference including all possible errors of nested children. */ - public get allErrors(): ParseError[] { - const result: ParseError[] = []; + // TODO: @legomushroom - refactor to use `errors()` method above + public get allErrors(): readonly IErrorWithLink[] { + const result: IErrorWithLink[] = []; // collect error conditions of all child references const childErrorConditions = this @@ -418,7 +501,7 @@ export abstract class BasePromptParser extend return errorCondition && !(errorCondition instanceof NotPromptFile); }) // map to error condition objects - .map((childReference): ParseError => { + .map((childReference): IErrorWithLink => { const { errorCondition } = childReference; // `must` always be `true` because of the `filter` call above @@ -427,7 +510,10 @@ export abstract class BasePromptParser extend `Error condition must be present for '${childReference.uri.path}'.`, ); - return errorCondition; + return { + link: childReference, + error: errorCondition, + }; }); result.push(...childErrorConditions); @@ -440,49 +526,64 @@ export abstract class BasePromptParser extend * possible child reference errors. */ public get topError(): IResolveError | undefined { - // get all errors, including error of this object - const errors = []; if (this.errorCondition) { - errors.push(this.errorCondition); + return { + localizedMessage: this.getErrorMessage(this.errorCondition), + originalError: this.errorCondition, + isRootError: true, + }; } - errors.push(...this.allErrors); - // if no errors, nothing to do - if (errors.length === 0) { - return undefined; + const childErrors: IErrorWithLink[] = []; + for (const reference of this.references) { + const { errorCondition } = reference; + + if (!errorCondition) { + continue; + } + + childErrors.push({ + link: reference, + error: errorCondition, + }); + } + + const nestedErrors: IErrorWithLink[] = []; + for (const reference of this.references) { + nestedErrors.push(...reference.allErrors); } - // if the first error is the error of the root reference, - // then return it as an `error` otherwise use `warning` - const [firstError, ...restErrors] = errors; - const isRootError = (firstError === this.errorCondition); + if (childErrors.length === 0 && nestedErrors.length === 0) { + return undefined; + } - // if a child error - the error is somewhere in the nested references tree, - // then use message prefix to highlight that this is not a root error - const prefix = (!isRootError) - ? `${errorMessages.brokenChild}: ` - : ''; + const firstChildError = childErrors[0] || nestedErrors[0]; + const isDirectChildError = (childErrors.length > 0); + const totalErrorsCount = childErrors.length + nestedErrors.length; - const moreSuffix = restErrors.length > 0 - ? `\n-\n +${restErrors.length} more error${restErrors.length > 1 ? 's' : ''}` - : ''; + const localizedMessage = errorMessages + .brokenChild( + firstChildError, + isDirectChildError, + totalErrorsCount - 1, + ); - const errorMessage = this.getErrorMessage(firstError); return { - isRootError, - message: `${prefix}${errorMessage}${moreSuffix}`, + localizedMessage, + originalError: firstChildError.error, + isRootError: false, }; } /** * Get message for the provided error condition object. * - * @param error Error object that extends {@link ParseError}. + * @param error Error object that extends {@link ResolveError}. * @returns Error message. */ - protected getErrorMessage(error: TError): string { + protected getErrorMessage(error: TError): string { if (error instanceof OpenFailed) { - return `${errorMessages.fileOpenFailed} '${error.uri.path}'.`; + return `${errorMessages.openFailed} '${error.uri.path}'.`; } if (error instanceof FailedToResolveContentsStream) { @@ -599,10 +700,10 @@ export class PromptFileReference extends BasePromptParser Date: Mon, 24 Feb 2025 14:49:47 -0800 Subject: [PATCH 4/5] fix error messages content for both editor and in-chat attachment cases --- .../promptAttachmentWidget.ts | 16 +- .../chat/common/promptFileReferenceErrors.ts | 52 ++++- .../promptSyntax/contentProviders/types.d.ts | 6 +- .../promptLinkDiagnosticsProvider.ts | 10 +- .../promptSyntax/parsers/basePromptParser.ts | 220 ++++-------------- .../common/promptSyntax/parsers/topError.ts | 96 ++++++++ .../common/promptSyntax/parsers/types.d.ts | 38 +-- 7 files changed, 226 insertions(+), 212 deletions(-) create mode 100644 src/vs/workbench/contrib/chat/common/promptSyntax/parsers/topError.ts diff --git a/src/vs/workbench/contrib/chat/browser/attachments/promptAttachments/promptAttachmentWidget.ts b/src/vs/workbench/contrib/chat/browser/attachments/promptAttachments/promptAttachmentWidget.ts index cb082eaae6d24..d7b6b71cccea6 100644 --- a/src/vs/workbench/contrib/chat/browser/attachments/promptAttachments/promptAttachmentWidget.ts +++ b/src/vs/workbench/contrib/chat/browser/attachments/promptAttachments/promptAttachmentWidget.ts @@ -22,8 +22,8 @@ import { ILanguageService } from '../../../../../../editor/common/languages/lang import { FileKind, IFileService } from '../../../../../../platform/files/common/files.js'; import { IMenuService, MenuId } from '../../../../../../platform/actions/common/actions.js'; import { getCleanPromptName } from '../../../../../../platform/prompts/common/constants.js'; -import { ChatPromptAttachmentModel } from '../../chatAttachmentModel/chatPromptAttachmentModel.js'; import { IContextKeyService } from '../../../../../../platform/contextkey/common/contextkey.js'; +import { ChatPromptAttachmentModel } from '../../chatAttachmentModel/chatPromptAttachmentModel.js'; import { IContextMenuService } from '../../../../../../platform/contextview/browser/contextView.js'; import { getDefaultHoverDelegate } from '../../../../../../base/browser/ui/hover/hoverDelegateFactory.js'; import { getFlatContextMenuActions } from '../../../../../../platform/actions/browser/menuEntryActionViewItem.js'; @@ -118,18 +118,18 @@ export class PromptAttachmentWidget extends Disposable { // add the issue details in the hover title for the attachment, one // error/warning at a time because there is a limited space available if (topError) { - const { isRootError, localizedMessage: details } = topError; - const isWarning = !isRootError; + const { errorSubject: subject } = topError; + const isError = (subject === 'root'); this.domNode.classList.add( - (isWarning) ? 'warning' : 'error', + (isError) ? 'error' : 'warning', ); - const errorCaption = (isWarning) - ? localize('warning', "Warning") - : localize('error', "Error"); + const severity = (isError) + ? localize('error', "Error") + : localize('warning', "Warning"); - title += `\n-\n[${errorCaption}]: ${details}`; + title += `\n[${severity}]: ${topError.localizedMessage}`; } const fileWithoutExtension = getCleanPromptName(file); diff --git a/src/vs/workbench/contrib/chat/common/promptFileReferenceErrors.ts b/src/vs/workbench/contrib/chat/common/promptFileReferenceErrors.ts index 40cec4f0503ff..dd1b93be160bb 100644 --- a/src/vs/workbench/contrib/chat/common/promptFileReferenceErrors.ts +++ b/src/vs/workbench/contrib/chat/common/promptFileReferenceErrors.ts @@ -4,6 +4,8 @@ *--------------------------------------------------------------------------------------------*/ import { URI } from '../../../../base/common/uri.js'; +import { basename } from '../../../../base/common/path.js'; +import { assert, assertNever } from '../../../../base/common/assert.js'; /** * Base prompt parsing error class. @@ -89,6 +91,12 @@ export class OpenFailed extends FailedToResolveContentsStream { } } +/** + * Character use to join filenames/paths in a chain of references that + * lead to recursion. + */ +const DEFAULT_RECURSIVE_PATH_JOIN_CHAR = ' -> '; + /** * Error that reflects the case when attempt resolve nested file * references failes due to a recursive reference, e.g., @@ -106,23 +114,53 @@ export class OpenFailed extends FailedToResolveContentsStream { export class RecursiveReference extends ResolveError { public override errorType = 'RecursiveReferenceError'; + /** + * Default string representation of the recursive path. + */ + public readonly recursivePathString: string; + constructor( uri: URI, public readonly recursivePath: string[], ) { - const references = recursivePath.join(' -> '); + // sanity check - a recursive path must always have at least + // two items in the list, otherwise it is not a recursive loop + assert( + recursivePath.length >= 2, + `Recursive path must contain at least two paths, got '${recursivePath.length}'.`, + ); + const pathString = recursivePath.join(DEFAULT_RECURSIVE_PATH_JOIN_CHAR); super( uri, - `Recursive references found: ${references}.`, + `Recursive references found: ${pathString}.`, ); + this.recursivePathString = pathString; } /** * Returns a string representation of the recursive path. */ - public get recursivePathString(): string { - return this.recursivePath.join(' -> '); + public getRecursivePathString( + filename: 'basename' | 'fullpath', + pathJoinCharacter: string = DEFAULT_RECURSIVE_PATH_JOIN_CHAR, + ): string { + return this.recursivePath + .map((path) => { + if (filename === 'fullpath') { + return `'${path}'`; + } + + if (filename === 'basename') { + return `'${basename(path)}'`; + } + + assertNever( + filename, + `Unknown filename format '${filename}'.`, + ); + }) + .join(pathJoinCharacter); } /** @@ -138,6 +176,12 @@ export class RecursiveReference extends ResolveError { return false; } + // performance optimization - if the paths lengths don't match, + // no need to compare entire strings as they must be different + if (this.recursivePathString.length !== other.recursivePathString.length) { + return false; + } + return this.recursivePathString === other.recursivePathString; } diff --git a/src/vs/workbench/contrib/chat/common/promptSyntax/contentProviders/types.d.ts b/src/vs/workbench/contrib/chat/common/promptSyntax/contentProviders/types.d.ts index 5d03ef2848cea..3ff8c569b6dc7 100644 --- a/src/vs/workbench/contrib/chat/common/promptSyntax/contentProviders/types.d.ts +++ b/src/vs/workbench/contrib/chat/common/promptSyntax/contentProviders/types.d.ts @@ -4,7 +4,7 @@ *--------------------------------------------------------------------------------------------*/ import { URI } from '../../../../../../base/common/uri.js'; -import { ParseError } from '../../promptFileReferenceErrors.js'; +import { ResolveError } from '../../promptFileReferenceErrors.js'; import { IDisposable } from '../../../../../../base/common/lifecycle.js'; import { VSBufferReadableStream } from '../../../../../../base/common/buffer.js'; @@ -27,9 +27,9 @@ export interface IPromptContentsProvider extends IDisposable { /** * Event that fires when the prompt contents change. The event is either a * {@linkcode VSBufferReadableStream} stream with changed contents or - * an instance of the {@linkcode ParseError} error. + * an instance of the {@linkcode ResolveError} error. */ onContentChanged( - callback: (streamOrError: VSBufferReadableStream | ParseError) => void, + callback: (streamOrError: VSBufferReadableStream | ResolveError) => void, ): IDisposable; } diff --git a/src/vs/workbench/contrib/chat/common/promptSyntax/languageFeatures/promptLinkDiagnosticsProvider.ts b/src/vs/workbench/contrib/chat/common/promptSyntax/languageFeatures/promptLinkDiagnosticsProvider.ts index 114687d46d785..590c4ecf92dd4 100644 --- a/src/vs/workbench/contrib/chat/common/promptSyntax/languageFeatures/promptLinkDiagnosticsProvider.ts +++ b/src/vs/workbench/contrib/chat/common/promptSyntax/languageFeatures/promptLinkDiagnosticsProvider.ts @@ -24,11 +24,6 @@ import { IInstantiationService } from '../../../../../../platform/instantiation/ import { IConfigurationService } from '../../../../../../platform/configuration/common/configuration.js'; import { IMarkerData, IMarkerService, MarkerSeverity } from '../../../../../../platform/markers/common/markers.js'; -/** - * TODO: @legomushroom - list - * - improve error messages - */ - /** * TODO: @legomushroom */ @@ -124,9 +119,8 @@ const toMarker = ( 'Error must not be of "not prompt file" type.', ); - // use `error` severity if the error relates to the link itself, and use - // the `warning` severity if the error is related to one of its children - const severity = (topError.isRootError) + // `error` severity for the link itself, `warning` for any of its children + const severity = (topError.errorSubject === 'root') ? MarkerSeverity.Error : MarkerSeverity.Warning; diff --git a/src/vs/workbench/contrib/chat/common/promptSyntax/parsers/basePromptParser.ts b/src/vs/workbench/contrib/chat/common/promptSyntax/parsers/basePromptParser.ts index f9fba66926716..d5a93b8455788 100644 --- a/src/vs/workbench/contrib/chat/common/promptSyntax/parsers/basePromptParser.ts +++ b/src/vs/workbench/contrib/chat/common/promptSyntax/parsers/basePromptParser.ts @@ -3,7 +3,7 @@ * Licensed under the MIT License. See License.txt in the project root for license information. *--------------------------------------------------------------------------------------------*/ -import { localize } from '../../../../../../nls.js'; +import { TopError } from './topError.js'; import { URI } from '../../../../../../base/common/uri.js'; import { ChatPromptCodec } from '../codecs/chatPromptCodec.js'; import { Emitter } from '../../../../../../base/common/event.js'; @@ -15,73 +15,15 @@ import { assertDefined } from '../../../../../../base/common/types.js'; import { IPromptContentsProvider } from '../contentProviders/types.js'; import { DeferredPromise } from '../../../../../../base/common/async.js'; import { ILogService } from '../../../../../../platform/log/common/log.js'; +import { IResolveError, IPromptFileReference, ITopError } from './types.js'; import { basename, extUri } from '../../../../../../base/common/resources.js'; -import { IErrorWithLink, IPromptFileReference, IResolveError } from './types.js'; import { VSBufferReadableStream } from '../../../../../../base/common/buffer.js'; import { isPromptFile } from '../../../../../../platform/prompts/common/constants.js'; import { ObservableDisposable } from '../../../../../../base/common/observableDisposable.js'; import { FilePromptContentProvider } from '../contentProviders/filePromptContentsProvider.js'; import { IInstantiationService } from '../../../../../../platform/instantiation/common/instantiation.js'; import { MarkdownLink } from '../../../../../../editor/common/codecs/markdownCodec/tokens/markdownLink.js'; -import { OpenFailed, NotPromptFile, RecursiveReference, FolderReference, ResolveError, FailedToResolveContentsStream } from '../../promptFileReferenceErrors.js'; - -/** - * TODO: @legomushroom - list - * - * - fix recursive reference case: - * - it should not be an error, but warning - * - [in `brokenChild`]: fix the error message - */ - -/** - * Well-known localized error messages. - */ -const errorMessages = { - // TODO: @legomushroom - update localization IDs - recursion: localize('chatPromptInstructionsRecursiveReference', 'Recursive reference'), - openFailed: localize('chatPromptInstructionsFileOpenFailed', 'Cannot open'), - streamOpenFailed: localize('chatPromptInstructionsStreamOpenFailed', 'Cannot read'), - - /** - * TODO: @legomushroom - add @throws tag - */ - brokenChild: ( - error: IErrorWithLink, - isDirectChild: boolean, - moreErrorsCount: number, - ): string => { - // sanity check - assert( - moreErrorsCount >= 0, - `Additional errors count must be non-negative, got '${moreErrorsCount}'.`, - ); - - const subjectName = (isDirectChild) - ? localize('chatPromptInstructionsBrokenReferenceFile', "Contains") - : localize( - 'chatPromptInstructionsBrokenChildReference', - "Indirectly referenced file '{0}' contains", - basename(error.link.uri), - ); - - const linkErrorType = (error.error instanceof RecursiveReference) - ? localize('chatPromptInstructionsBrokenReferenceFileRecursive', "recursive") - : localize('chatPromptInstructionsBrokenChildReferenceBroken', "broken"); - - const moreErrorsNote = (moreErrorsCount > 0) - ? localize('chatPromptInstructionsBrokenReferenceSuffix', " (+{0} more warnings)", moreErrorsCount) - : ''; - - return localize( - 'chatPromptInstructionsBrokenReference', - "{0} a {1} link '{2}' that will be ignored.{3}", - subjectName, - linkErrorType, - error.link.text, - moreErrorsNote, - ); - }, -}; +import { OpenFailed, NotPromptFile, RecursiveReference, FolderReference, ResolveError } from '../../promptFileReferenceErrors.js'; /** * Error conditions that may happen during the file reference resolution. @@ -205,7 +147,10 @@ export abstract class BasePromptParser extend if (seenReferences.includes(this.uri.path)) { seenReferences.push(this.uri.path); - this._errorCondition = new RecursiveReference(this.uri, seenReferences); + this._errorCondition = new RecursiveReference( + this.uri, + seenReferences, + ); this._onUpdate.fire(); this.firstParseResult.complete(); @@ -450,7 +395,7 @@ export abstract class BasePromptParser extend /** * TODO: @legomushroom */ - public get errors(): readonly IErrorWithLink[] { + public get errors(): readonly ResolveError[] { // collect error conditions of direct child references const childErrors = this // get immediate references @@ -463,7 +408,7 @@ export abstract class BasePromptParser extend return errorCondition && !(errorCondition instanceof NotPromptFile); }) // map to error condition objects - .map((childReference): IErrorWithLink => { + .map((childReference): ResolveError => { const { errorCondition } = childReference; // `must` always be `true` because of the `filter` call above @@ -472,10 +417,7 @@ export abstract class BasePromptParser extend `Error condition must be present for '${childReference.uri.path}'.`, ); - return { - link: childReference, - error: errorCondition, - }; + return errorCondition; }); return childErrors; @@ -485,38 +427,22 @@ export abstract class BasePromptParser extend * List of all errors that occurred while resolving the current * reference including all possible errors of nested children. */ - // TODO: @legomushroom - refactor to use `errors()` method above - public get allErrors(): readonly IErrorWithLink[] { - const result: IErrorWithLink[] = []; + public get allErrors(): readonly IResolveError[] { + const result: IResolveError[] = []; - // collect error conditions of all child references - const childErrorConditions = this - // get entire reference tree - .allReferences - // filter out children without error conditions or - // the ones that are non-prompt snippet files - .filter((childReference) => { - const { errorCondition } = childReference; - - return errorCondition && !(errorCondition instanceof NotPromptFile); - }) - // map to error condition objects - .map((childReference): IErrorWithLink => { - const { errorCondition } = childReference; - - // `must` always be `true` because of the `filter` call above - assertDefined( - errorCondition, - `Error condition must be present for '${childReference.uri.path}'.`, - ); + for (const reference of this.references) { + const { errorCondition } = reference; - return { - link: childReference, - error: errorCondition, - }; - }); + if (errorCondition && (!(errorCondition instanceof NotPromptFile))) { + result.push({ + originalError: errorCondition, + parentUri: this.uri, + }); + } - result.push(...childErrorConditions); + // recursively collect all possible errors of its children + result.push(...reference.allErrors); + } return result; } @@ -525,30 +451,17 @@ export abstract class BasePromptParser extend * The top most error of the current reference or any of its * possible child reference errors. */ - public get topError(): IResolveError | undefined { + public get topError(): ITopError | undefined { if (this.errorCondition) { - return { - localizedMessage: this.getErrorMessage(this.errorCondition), + return new TopError({ + errorSubject: 'root', + errorsCount: 1, originalError: this.errorCondition, - isRootError: true, - }; - } - - const childErrors: IErrorWithLink[] = []; - for (const reference of this.references) { - const { errorCondition } = reference; - - if (!errorCondition) { - continue; - } - - childErrors.push({ - link: reference, - error: errorCondition, }); } - const nestedErrors: IErrorWithLink[] = []; + const childErrors: ResolveError[] = [...this.errors]; + const nestedErrors: IResolveError[] = []; for (const reference of this.references) { nestedErrors.push(...reference.allErrors); } @@ -557,54 +470,29 @@ export abstract class BasePromptParser extend return undefined; } - const firstChildError = childErrors[0] || nestedErrors[0]; - const isDirectChildError = (childErrors.length > 0); - const totalErrorsCount = childErrors.length + nestedErrors.length; - - const localizedMessage = errorMessages - .brokenChild( - firstChildError, - isDirectChildError, - totalErrorsCount - 1, - ); - - return { - localizedMessage, - originalError: firstChildError.error, - isRootError: false, - }; - } + const firstDirectChildError = childErrors[0]; + const firstNestedChildError = nestedErrors[0]; + const hasDirectChildError = (firstDirectChildError !== undefined); - /** - * Get message for the provided error condition object. - * - * @param error Error object that extends {@link ResolveError}. - * @returns Error message. - */ - protected getErrorMessage(error: TError): string { - if (error instanceof OpenFailed) { - return `${errorMessages.openFailed} '${error.uri.path}'.`; - } - - if (error instanceof FailedToResolveContentsStream) { - return `${errorMessages.streamOpenFailed} '${error.uri.path}'.`; - } - - // if a recursion, provide the entire recursion path so users - // can use it for the debugging purposes - if (error instanceof RecursiveReference) { - const { recursivePath } = error; + const firstChildError = (hasDirectChildError) + ? { + originalError: firstDirectChildError, + parentUri: this.uri, + } + : firstNestedChildError; - const recursivePathString = recursivePath - .map((path) => { - return basename(URI.file(path)); - }) - .join(' -> '); + const totalErrorsCount = childErrors.length + nestedErrors.length; - return `${errorMessages.recursion}:\n${recursivePathString}`; - } + const subject = (hasDirectChildError) + ? 'child' + : 'indirect-child'; - return error.message; + return new TopError({ + errorSubject: subject, + originalError: firstChildError.originalError, + parentUri: firstChildError.parentUri, + errorsCount: totalErrorsCount, + }); } /** @@ -696,18 +584,6 @@ export class PromptFileReference extends BasePromptParser, + ) { } + + + public get localizedMessage(): string { + // TODO: @legomushroom - update localization IDs + const { originalError, parentUri, errorSubject: subject, errorsCount } = this; + + assert( + errorsCount >= 1, + `Error count must be at least 1, got '${errorsCount}'.`, + ); + + // a note about how many more link issues are there + const moreIssuesNote = (errorsCount > 1) + ? localize('chatPromptInstructionsBrokenReferenceSuffix222222', "\n(+{0} more issues)", errorsCount - 1) + : ''; + + if (subject === 'root') { + if (originalError instanceof OpenFailed) { + return localize( + 'chatPromptInstructionsFileOpenFailed111', + "Cannot open '{0}'.{1}", + originalError.uri.path, + moreIssuesNote, + ); + } + + if (originalError instanceof FailedToResolveContentsStream) { + return localize( + 'chatPromptInstructionsStreamOpenFailed1111', + "Cannot read '{0}'.{1}", + originalError.uri.path, + moreIssuesNote, + ); + } + + if (originalError instanceof RecursiveReference) { + return localize( + 'chatPromptInstructionsSelfRecursion4444', + "Recursion to itself.", + ); + } + + return originalError.message + moreIssuesNote; + } + + // a sanity check - because the error subject is not `root`, the parent must set + assertDefined( + parentUri, + 'Parent URI must be defined for error of non-root link.', + ); + + const errorMessageStart = (subject === 'child') + ? localize('chatPromptInstructionsBrokenReferenceFile', "Contains") + : localize( + 'chatPromptInstructionsBrokenChildReference', + "Indirectly referenced prompt '{0}' contains", + parentUri.path, + ); + + const linkIssueName = (originalError instanceof RecursiveReference) + ? localize('chatPromptInstructionsBrokenChildRecursiveLink', "recursive") + : localize('chatPromptInstructionsBrokenChildBrokenLink', "broken"); + + return localize( + 'chatPromptInstructionsBrokenReference111', + "{0} a {1} link to '{2}' that will be ignored.{3}", + errorMessageStart, + linkIssueName, + originalError.uri.path, + moreIssuesNote, + ); + } +} diff --git a/src/vs/workbench/contrib/chat/common/promptSyntax/parsers/types.d.ts b/src/vs/workbench/contrib/chat/common/promptSyntax/parsers/types.d.ts index b439e78a43ebe..65818cc17a439 100644 --- a/src/vs/workbench/contrib/chat/common/promptSyntax/parsers/types.d.ts +++ b/src/vs/workbench/contrib/chat/common/promptSyntax/parsers/types.d.ts @@ -11,37 +11,41 @@ import { IRange, Range } from '../../../../../../editor/common/core/range.js'; /** * TODO: @legomushroom */ -export interface IErrorWithLink { +export interface IResolveError { /** - * TODO: @legomushroom + * Original error instance. */ - readonly error: ResolveError; + readonly originalError: ResolveError; /** - * TODO: @legomushroom + * URI of the parent that references this error. */ - readonly link: IPromptReference; + readonly parentUri?: URI; } /** - * Interface for a resolve error. + * Top most error of the reference tree. */ -export interface IResolveError { +export interface ITopError extends IResolveError { /** - * Localized error message. + * Where does the error belong to: + * + * - `root` - the error is the top most error of the entire tree + * - `child` - the error is a child of the root error + * - `indirect-child` - the error is a child of a child of the root error */ - localizedMessage: string; + readonly errorSubject: 'root' | 'child' | 'indirect-child'; /** - * Original error object. + * Total number of all errors in the references tree, including the error + * of the current reference and all possible errors of its children. */ - originalError: ResolveError; + readonly errorsCount: number; /** - * Whether this error is for the root reference - * object, or for one of its possible children. + * Localized error message. */ - isRootError: boolean; + readonly localizedMessage: string; } /** @@ -110,19 +114,19 @@ export interface IPromptReference extends IDisposable { /** * TODO: @legomushroom */ - readonly errors: readonly IErrorWithLink[]; + readonly errors: readonly ResolveError[]; /** * List of all errors that occurred while resolving the current * reference including all possible errors of nested children. */ - readonly allErrors: readonly IErrorWithLink[]; + readonly allErrors: readonly IResolveError[]; /** * The top most error of the current reference or any of its * possible child reference errors. */ - readonly topError: IResolveError | undefined; + readonly topError: ITopError | undefined; /** * Direct references of the current reference. From 0926d48de41e2fe4bba339078be14d5f8eb382b1 Mon Sep 17 00:00:00 2001 From: Oleg Solomko Date: Mon, 24 Feb 2025 15:15:30 -0800 Subject: [PATCH 5/5] add more doc comments --- .../promptLinkDiagnosticsProvider.ts | 28 +++++++++------ .../promptSyntax/parsers/basePromptParser.ts | 34 ++++++------------ .../common/promptSyntax/parsers/topError.ts | 36 ++++++++++--------- .../common/promptSyntax/parsers/types.d.ts | 4 +-- 4 files changed, 48 insertions(+), 54 deletions(-) diff --git a/src/vs/workbench/contrib/chat/common/promptSyntax/languageFeatures/promptLinkDiagnosticsProvider.ts b/src/vs/workbench/contrib/chat/common/promptSyntax/languageFeatures/promptLinkDiagnosticsProvider.ts index 590c4ecf92dd4..7d46c72dbbe03 100644 --- a/src/vs/workbench/contrib/chat/common/promptSyntax/languageFeatures/promptLinkDiagnosticsProvider.ts +++ b/src/vs/workbench/contrib/chat/common/promptSyntax/languageFeatures/promptLinkDiagnosticsProvider.ts @@ -25,16 +25,16 @@ import { IConfigurationService } from '../../../../../../platform/configuration/ import { IMarkerData, IMarkerService, MarkerSeverity } from '../../../../../../platform/markers/common/markers.js'; /** - * TODO: @legomushroom + * Unique ID of the markers provider class. */ const MARKERS_OWNER_ID = 'reusable-prompts-syntax'; /** - * TODO: @legomushroom + * Prompt links diagnostics provider for a single text model. */ class PromptLinkDiagnosticsProvider extends ObservableDisposable { /** - * TODO: @legomushroom + * Reference to the current prompt syntax parser instance. */ private readonly parser: TextModelPromptParser; @@ -56,7 +56,7 @@ class PromptLinkDiagnosticsProvider extends ObservableDisposable { } /** - * TODO: @legomushroom + * Update diagnostic markers for the current editor. */ private async updateMarkers() { // ensure that parsing process is settled @@ -94,9 +94,14 @@ class PromptLinkDiagnosticsProvider extends ObservableDisposable { } /** - * TODO: @legomushroom + * Convert a prompt link with an issue to a marker data. + * + * @throws + * - if there is no link issue (e.g., `topError` undefined) + * - if there is no link range to highlight (e.g., `linkRange` undefined) + * - if the original error is of `NotPromptFile` type - we don't want to + * show diagnostic markers for non-prompt file links in the prompts */ -// TODO: @legomushroom - add @throws info const toMarker = ( link: IPromptFileReference, ): IMarkerData => { @@ -132,11 +137,12 @@ const toMarker = ( }; /** - * TODO: @legomushroom + * The class that manages creation and disposal of {@link PromptLinkDiagnosticsProvider} + * classes for each specific editor text model. */ -export class PromptsLinkDiagnosticsProvider extends Disposable { +export class PromptLinkDiagnosticsInstanceManager extends Disposable { /** - * TODO: @legomushroom + * Currently available {@link PromptLinkDiagnosticsProvider} instances. */ private readonly providers: ObjectCache; @@ -187,7 +193,7 @@ export class PromptsLinkDiagnosticsProvider extends Disposable { } /** - * TODO: @legomushroom + * Initialize a new {@link PromptLinkDiagnosticsProvider} for the given editor. */ private handleNewEditor(editor: IEditor): this { const model = editor.getModel(); @@ -215,4 +221,4 @@ export class PromptsLinkDiagnosticsProvider extends Disposable { // register the provider as a workbench contribution Registry.as(Extensions.Workbench) - .registerWorkbenchContribution(PromptsLinkDiagnosticsProvider, LifecyclePhase.Eventually); + .registerWorkbenchContribution(PromptLinkDiagnosticsInstanceManager, LifecyclePhase.Eventually); diff --git a/src/vs/workbench/contrib/chat/common/promptSyntax/parsers/basePromptParser.ts b/src/vs/workbench/contrib/chat/common/promptSyntax/parsers/basePromptParser.ts index d5a93b8455788..1f92962ff75f8 100644 --- a/src/vs/workbench/contrib/chat/common/promptSyntax/parsers/basePromptParser.ts +++ b/src/vs/workbench/contrib/chat/common/promptSyntax/parsers/basePromptParser.ts @@ -393,32 +393,18 @@ export abstract class BasePromptParser extend } /** - * TODO: @legomushroom + * Get list of errors for the direct links of the current reference. */ public get errors(): readonly ResolveError[] { - // collect error conditions of direct child references - const childErrors = this - // get immediate references - .references - // filter out children without error conditions or - // the ones that are non-prompt snippet files - .filter((childReference) => { - const { errorCondition } = childReference; - - return errorCondition && !(errorCondition instanceof NotPromptFile); - }) - // map to error condition objects - .map((childReference): ResolveError => { - const { errorCondition } = childReference; - - // `must` always be `true` because of the `filter` call above - assertDefined( - errorCondition, - `Error condition must be present for '${childReference.uri.path}'.`, - ); - - return errorCondition; - }); + const childErrors: ResolveError[] = []; + + for (const reference of this.references) { + const { errorCondition } = reference; + + if (errorCondition && (!(errorCondition instanceof NotPromptFile))) { + childErrors.push(errorCondition); + } + } return childErrors; } diff --git a/src/vs/workbench/contrib/chat/common/promptSyntax/parsers/topError.ts b/src/vs/workbench/contrib/chat/common/promptSyntax/parsers/topError.ts index 983c383a1e13e..0e6e4a9ad9c07 100644 --- a/src/vs/workbench/contrib/chat/common/promptSyntax/parsers/topError.ts +++ b/src/vs/workbench/contrib/chat/common/promptSyntax/parsers/topError.ts @@ -10,11 +10,11 @@ import { assertDefined } from '../../../../../../base/common/types.js'; import { OpenFailed, RecursiveReference, FailedToResolveContentsStream } from '../../promptFileReferenceErrors.js'; /** - * TODO: @lego + * The top-most error of the reference tree. */ export class TopError implements ITopError { - public readonly errorSubject = this.options.errorSubject; public readonly originalError = this.options.originalError; + public readonly errorSubject = this.options.errorSubject; public readonly errorsCount = this.options.errorsCount; public readonly parentUri = this.options.parentUri; @@ -24,7 +24,6 @@ export class TopError implements ITopError { public get localizedMessage(): string { - // TODO: @legomushroom - update localization IDs const { originalError, parentUri, errorSubject: subject, errorsCount } = this; assert( @@ -33,37 +32,37 @@ export class TopError implements ITopError { ); // a note about how many more link issues are there - const moreIssuesNote = (errorsCount > 1) - ? localize('chatPromptInstructionsBrokenReferenceSuffix222222', "\n(+{0} more issues)", errorsCount - 1) + const moreIssuesLabel = (errorsCount > 1) + ? localize('workbench.reusable-prompts.top-error.more-issues-label', "\n(+{0} more issues)", errorsCount - 1) : ''; if (subject === 'root') { if (originalError instanceof OpenFailed) { return localize( - 'chatPromptInstructionsFileOpenFailed111', + 'workbench.reusable-prompts.top-error.open-failed', "Cannot open '{0}'.{1}", originalError.uri.path, - moreIssuesNote, + moreIssuesLabel, ); } if (originalError instanceof FailedToResolveContentsStream) { return localize( - 'chatPromptInstructionsStreamOpenFailed1111', + 'workbench.reusable-prompts.top-error.cannot-read', "Cannot read '{0}'.{1}", originalError.uri.path, - moreIssuesNote, + moreIssuesLabel, ); } if (originalError instanceof RecursiveReference) { return localize( - 'chatPromptInstructionsSelfRecursion4444', + 'workbench.reusable-prompts.top-error.recursive-reference', "Recursion to itself.", ); } - return originalError.message + moreIssuesNote; + return originalError.message + moreIssuesLabel; } // a sanity check - because the error subject is not `root`, the parent must set @@ -73,24 +72,27 @@ export class TopError implements ITopError { ); const errorMessageStart = (subject === 'child') - ? localize('chatPromptInstructionsBrokenReferenceFile', "Contains") + ? localize( + 'workbench.reusable-prompts.top-error.child.direct', + "Contains", + ) : localize( - 'chatPromptInstructionsBrokenChildReference', + 'workbench.reusable-prompts.top-error.child.indirect', "Indirectly referenced prompt '{0}' contains", parentUri.path, ); const linkIssueName = (originalError instanceof RecursiveReference) - ? localize('chatPromptInstructionsBrokenChildRecursiveLink', "recursive") - : localize('chatPromptInstructionsBrokenChildBrokenLink', "broken"); + ? localize('recursive', "recursive") + : localize('broken', "broken"); return localize( - 'chatPromptInstructionsBrokenReference111', + 'workbench.reusable-prompts.top-error.child.final-message', "{0} a {1} link to '{2}' that will be ignored.{3}", errorMessageStart, linkIssueName, originalError.uri.path, - moreIssuesNote, + moreIssuesLabel, ); } } diff --git a/src/vs/workbench/contrib/chat/common/promptSyntax/parsers/types.d.ts b/src/vs/workbench/contrib/chat/common/promptSyntax/parsers/types.d.ts index 65818cc17a439..92de54e9f6c62 100644 --- a/src/vs/workbench/contrib/chat/common/promptSyntax/parsers/types.d.ts +++ b/src/vs/workbench/contrib/chat/common/promptSyntax/parsers/types.d.ts @@ -9,7 +9,7 @@ import { IDisposable } from '../../../../../../base/common/lifecycle.js'; import { IRange, Range } from '../../../../../../editor/common/core/range.js'; /** - * TODO: @legomushroom + * A resolve error with a parent prompt URI, if any. */ export interface IResolveError { /** @@ -112,7 +112,7 @@ export interface IPromptReference extends IDisposable { readonly errorCondition: ResolveError | undefined; /** - * TODO: @legomushroom + * Get list of errors for the direct links of the current reference. */ readonly errors: readonly ResolveError[];