calculate snapshot id of symbol at reading to prevent unexpected behavior if modifying at inspecting (#1336)

* fix: instrumentSimpleOperation swallows exception

* adjust the position and initial range of inline chat for fixing.

* adjust the selection of revealing the first inspection.

* calculate snapshot id at reading symbol, because symbol may change.

* rename versionId to snapshotId

* fix messages.

* refine prompt

* prevent duplicate inspecting.

* refine prompt

* handle error `response got filtered`

* handle invalid inspection comments

* return original java version value getting from ls.

* refactor: Improve inspection message handling and error handling

* Refactor InspectionCache to handle snapshot inspections with non inspections.

* remove `Ignore ...` code action.

* add support for inspections of fields

* fix messages.

* enhance prompts to better support context info and refactor

* fix error message to provide more details.

* fix prompt

Author: wangmingliang-ms

src/copilot/Copilot.ts

@@ -1,6 +1,6 @@
 import { LanguageModelChatMessage, lm, Disposable, CancellationToken, LanguageModelChatRequestOptions, LanguageModelChatMessageRole, LanguageModelChatSelector } from "vscode";
-import { instrumentSimpleOperation, sendInfo } from "vscode-extension-telemetry-wrapper";
-import { logger } from "./utils";
+import { fixedInstrumentSimpleOperation, logger } from "./utils";
+import { sendInfo } from "vscode-extension-telemetry-wrapper";
 
 export default class Copilot {
     public static readonly DEFAULT_END_MARK = '<|endofresponse|>';
@@ -10,6 +10,7 @@ export default class Copilot {
     public static readonly NOT_CANCELLABEL: CancellationToken = { isCancellationRequested: false, onCancellationRequested: () => Disposable.from() };
 
     public constructor(
+        private readonly systemMessagesOrSamples: LanguageModelChatMessage[],
         private readonly modelSelector: LanguageModelChatSelector = Copilot.DEFAULT_MODEL,
         private readonly modelOptions: LanguageModelChatRequestOptions = Copilot.DEFAULT_MODEL_OPTIONS,
         private readonly maxRounds: number = Copilot.DEFAULT_MAX_ROUNDS,
@@ -18,14 +19,13 @@ export default class Copilot {
     }
 
     private async doSend(
-        systemMessagesOrSamples: LanguageModelChatMessage[],
         userMessage: string,
         modelOptions: LanguageModelChatRequestOptions = Copilot.DEFAULT_MODEL_OPTIONS,
         cancellationToken: CancellationToken = Copilot.NOT_CANCELLABEL
     ): Promise<string> {
         let answer: string = '';
         let rounds: number = 0;
-        const messages = [...systemMessagesOrSamples];
+        const messages = [...this.systemMessagesOrSamples];
         const _send = async (message: string): Promise<boolean> => {
             rounds++;
             logger.debug(`User: \n`, message);
@@ -37,15 +37,18 @@ export default class Copilot {
             try {
                 const model = (await lm.selectChatModels(this.modelSelector))?.[0];
                 if (!model) {
-                    throw new Error('No model selected');
+                    const models = await lm.selectChatModels();
+                    throw new Error(`No suitable model, available models: [${models.map(m => m.name).join(', ')}]. Please make sure you have installed the latest "GitHub Copilot Chat" (v0.16.0 or later).`);
                 }
                 const response = await model.sendRequest(messages, modelOptions ?? this.modelOptions, cancellationToken);
                 for await (const item of response.text) {
                     rawAnswer += item;
                 }
             } catch (e) {
-                logger.error(`Failed to send request to copilot`, e);
-                throw new Error(`Failed to send request to copilot: ${e}`);
+                //@ts-ignore
+                const cause = e.cause || e;
+                logger.error(`Failed to chat with copilot`, cause);
+                throw cause;
             }
             messages.push(new LanguageModelChatMessage(LanguageModelChatMessageRole.Assistant, rawAnswer));
             logger.debug(`Copilot: \n`, rawAnswer);
@@ -63,11 +66,10 @@ export default class Copilot {
     }
 
     public async send(
-        systemMessagesOrSamples: LanguageModelChatMessage[],
         userMessage: string,
         modelOptions: LanguageModelChatRequestOptions = Copilot.DEFAULT_MODEL_OPTIONS,
         cancellationToken: CancellationToken = Copilot.NOT_CANCELLABEL
     ): Promise<string> {
-        return instrumentSimpleOperation("java.copilot.sendRequest", this.doSend.bind(this))(systemMessagesOrSamples, userMessage, modelOptions, cancellationToken);
+        return fixedInstrumentSimpleOperation("java.copilot.sendRequest", this.doSend.bind(this))(userMessage, modelOptions, cancellationToken);
     }
 }

src/copilot/inspect/InspectActionCodeLensProvider.ts

@@ -22,7 +22,7 @@ export class InspectActionCodeLensProvider implements CodeLensProvider {
         const topLevelCodeLenses: CodeLens[] = [];
         const classes = await getTopLevelClassesOfDocument(document);
         classes.map(clazz => new CodeLens(clazz.range, {
-            title: "Rewrite with new Java syntax",
+            title: "✨ Rewrite with new Java syntax",
             command: COMMAND_INSPECT_CLASS,
             arguments: [document, clazz]
         })).forEach(codeLens => topLevelCodeLenses.push(codeLens));

src/copilot/inspect/Inspection.ts

@@ -39,7 +39,7 @@ export namespace Inspection {
     export function revealFirstLineOfInspection(inspection: Inspection) {
         inspection.document && void workspace.openTextDocument(inspection.document.uri).then(document => {
             void window.showTextDocument(document).then(editor => {
-                const range = document.lineAt(inspection.problem.position.line).range;
+                const range = getIndicatorRangeOfInspection(inspection.problem);
                 editor.selection = new Selection(range.start, range.end);
                 editor.revealRange(range);
             });

src/copilot/inspect/InspectionCache.ts

@@ -1,14 +1,13 @@
 import { SymbolKind, TextDocument } from 'vscode';
-import { METHOD_KINDS, getClassesAndMethodsContainedInRange, getClassesAndMethodsOfDocument, logger } from '../utils';
+import { METHOD_KINDS, getSymbolsContainedInRange, getSymbolsOfDocument, logger } from '../utils';
 import { Inspection } from './Inspection';
-import * as crypto from "crypto";
 import { SymbolNode } from './SymbolNode';
 
 /**
  * A map based cache for inspections of a document.
- * format: `Map<documentKey, Map<symbolQualifiedName, [symbolVersionId, Inspection[]]`
+ * format: `Map<documentKey, Map<symbolQualifiedName, [symbolSnapshotId, Inspection[]]`
  */
-const DOC_SYMBOL_VERSION_INSPECTIONS: Map<string, Map<string, [string, Inspection[]]>> = new Map();
+const DOC_SYMBOL_SNAPSHOT_INSPECTIONS: Map<string, Map<string, [string, Inspection[]]>> = new Map();
 
 export default class InspectionCache {
     /**
@@ -18,15 +17,14 @@ export default class InspectionCache {
     public static async hasCache(document: TextDocument, symbol?: SymbolNode): Promise<boolean> {
         const documentKey = document.uri.fsPath;
         if (!symbol) {
-            return DOC_SYMBOL_VERSION_INSPECTIONS.has(documentKey);
+            return DOC_SYMBOL_SNAPSHOT_INSPECTIONS.has(documentKey);
         }
-        const symbolInspections = DOC_SYMBOL_VERSION_INSPECTIONS.get(documentKey);
+        const symbolInspections = DOC_SYMBOL_SNAPSHOT_INSPECTIONS.get(documentKey);
         // check if the symbol or its contained symbols are cached
-        const symbols = await getClassesAndMethodsContainedInRange(symbol.range, document);
+        const symbols = await getSymbolsContainedInRange(symbol.range, document);
         for (const s of symbols) {
-            const versionInspections = symbolInspections?.get(s.qualifiedName);
-            const symbolVersionId = InspectionCache.calculateSymbolVersionId(document, s);
-            if (versionInspections?.[0] === symbolVersionId) {
+            const snapshotInspections = symbolInspections?.get(s.qualifiedName);
+            if (snapshotInspections?.[0] === s.snapshotId && snapshotInspections[1].length > 0) {
                 return true;
             }
         }
@@ -39,7 +37,7 @@ export default class InspectionCache {
      * their content has changed.
      */
     public static async getCachedInspectionsOfDoc(document: TextDocument): Promise<Inspection[]> {
-        const symbols: SymbolNode[] = await getClassesAndMethodsOfDocument(document);
+        const symbols: SymbolNode[] = await getSymbolsOfDocument(document);
         const inspections: Inspection[] = [];
         // we don't get cached inspections directly from the cache, because we need to filter out outdated symbols
         for (const symbol of symbols) {
@@ -54,12 +52,11 @@ export default class InspectionCache {
      */
     public static getCachedInspectionsOfSymbol(document: TextDocument, symbol: SymbolNode): Inspection[] {
         const documentKey = document.uri.fsPath;
-        const symbolInspections = DOC_SYMBOL_VERSION_INSPECTIONS.get(documentKey);
-        const versionInspections = symbolInspections?.get(symbol.qualifiedName);
-        const symbolVersionId = InspectionCache.calculateSymbolVersionId(document, symbol);
-        if (versionInspections?.[0] === symbolVersionId) {
+        const symbolInspections = DOC_SYMBOL_SNAPSHOT_INSPECTIONS.get(documentKey);
+        const snapshotInspections = symbolInspections?.get(symbol.qualifiedName);
+        if (snapshotInspections?.[0] === symbol.snapshotId) {
             logger.debug(`cache hit for ${SymbolKind[symbol.kind]} ${symbol.qualifiedName} of ${document.uri.fsPath}`);
-            const inspections = versionInspections[1];
+            const inspections = snapshotInspections[1];
             inspections.forEach(s => {
                 s.document = document;
                 s.problem.position.line = s.problem.position.relativeLine + symbol.range.start.line;
@@ -78,7 +75,7 @@ export default class InspectionCache {
                 return isMethod ?
                     // NOTE: method inspections are inspections whose `position.line` is within the method's range
                     inspectionLine >= symbol.range.start.line && inspectionLine <= symbol.range.end.line :
-                    // NOTE: class inspections are inspections whose `position.line` is exactly the first line number of the class
+                    // NOTE: class/field inspections are inspections whose `position.line` is exactly the first line number of the class/field
                     inspectionLine === symbol.range.start.line;
             });
             // re-calculate `relativeLine` of method inspections, `relativeLine` is the relative line number to the start of the method
@@ -93,13 +90,13 @@ export default class InspectionCache {
      */
     public static invalidateInspectionCache(document?: TextDocument, symbol?: SymbolNode, inspeciton?: Inspection): void {
         if (!document) {
-            DOC_SYMBOL_VERSION_INSPECTIONS.clear();
+            DOC_SYMBOL_SNAPSHOT_INSPECTIONS.clear();
         } else if (!symbol) {
             const documentKey = document.uri.fsPath;
-            DOC_SYMBOL_VERSION_INSPECTIONS.delete(documentKey);
+            DOC_SYMBOL_SNAPSHOT_INSPECTIONS.delete(documentKey);
         } else if (!inspeciton) {
             const documentKey = document.uri.fsPath;
-            const symbolInspections = DOC_SYMBOL_VERSION_INSPECTIONS.get(documentKey);
+            const symbolInspections = DOC_SYMBOL_SNAPSHOT_INSPECTIONS.get(documentKey);
             // remove the cached inspections of the symbol
             symbolInspections?.delete(symbol.qualifiedName);
             // remove the cached inspections of contained symbols
@@ -110,11 +107,10 @@ export default class InspectionCache {
             });
         } else {
             const documentKey = document.uri.fsPath;
-            const symbolInspections = DOC_SYMBOL_VERSION_INSPECTIONS.get(documentKey);
-            const versionInspections = symbolInspections?.get(symbol.qualifiedName);
-            const symbolVersionId = InspectionCache.calculateSymbolVersionId(document, symbol);
-            if (versionInspections?.[0] === symbolVersionId) {
-                const inspections = versionInspections[1];
+            const symbolInspections = DOC_SYMBOL_SNAPSHOT_INSPECTIONS.get(documentKey);
+            const snapshotInspections = symbolInspections?.get(symbol.qualifiedName);
+            if (snapshotInspections?.[0] === symbol.snapshotId) {
+                const inspections = snapshotInspections[1];
                 // remove the inspection
                 inspections.splice(inspections.indexOf(inspeciton), 1);
             }
@@ -124,22 +120,13 @@ export default class InspectionCache {
     private static cacheSymbolInspections(document: TextDocument, symbol: SymbolNode, inspections: Inspection[]): void {
         logger.debug(`cache ${inspections.length} inspections for ${SymbolKind[symbol.kind]} ${symbol.qualifiedName} of ${document.uri.fsPath}`);
         const documentKey = document.uri.fsPath;
-        const symbolVersionId = InspectionCache.calculateSymbolVersionId(document, symbol);
-        const cachedSymbolInspections = DOC_SYMBOL_VERSION_INSPECTIONS.get(documentKey) ?? new Map();
+        const cachedSymbolInspections = DOC_SYMBOL_SNAPSHOT_INSPECTIONS.get(documentKey) ?? new Map();
         inspections.forEach(s => {
             s.document = document;
             s.symbol = symbol;
         });
         // use qualified name to prevent conflicts between symbols with the same signature in same document
-        cachedSymbolInspections.set(symbol.qualifiedName, [symbolVersionId, inspections]);
-        DOC_SYMBOL_VERSION_INSPECTIONS.set(documentKey, cachedSymbolInspections);
-    }
-
-    /**
-     * generate a unique id for the symbol based on its content, so that we can detect if the symbol has changed
-     */
-    private static calculateSymbolVersionId(document: TextDocument, symbol: SymbolNode): string {
-        const body = document.getText(symbol.range);
-        return crypto.createHash('md5').update(body).digest("hex")
+        cachedSymbolInspections.set(symbol.qualifiedName, [symbol.snapshotId, inspections]);
+        DOC_SYMBOL_SNAPSHOT_INSPECTIONS.set(documentKey, cachedSymbolInspections);
     }
 }