ContextProvider resolve handler improvements #13296

lukka · 2025-02-20T17:48:04Z

Changes:
-new feature: add traits for C++ lang version
-telemetry: fix cancellation events.
-telemetry: more diag event for registration failure.

Extension/src/LanguageServer/copilotCompletionContextProvider.ts

Extension/src/LanguageServer/client.ts

sean-mcmanus

Should SnippetEntry be removed? It doesn't appear to be referenced anymore?

Extension/src/LanguageServer/copilotCompletionContextProvider.ts

kuchungmsft · 2025-02-24T19:44:50Z

FYI, I have a PR opened to clean up the compiler argument traits, which may impact this PR, see #13278.

sean-mcmanus · 2025-02-24T20:46:27Z

FYI, I have a PR opened to clean up the compiler argument traits, which may impact this PR, see #13278.

Should your PR get checked in first, right?

Extension/src/LanguageServer/copilotCompletionContextProvider.ts

sean-mcmanus

cppCodeSnippetsFeatureNames doesn't work if it's lowercased -- that seems a little error prone and unexpected, because the other C_Cpp settings values are case insensitive I believe.

sean-mcmanus

Oh, but that doesn't appear to be a regression -- it looks like I modified the setting value to be lowercase and expected it to work.

Extension/src/LanguageServer/copilotCompletionContextProvider.ts

sean-mcmanus

getEnabledFeatureNames splits by "," and breaks if ", " with a space after the comma is used -- that seems error prone. Should it be changed to handle space after a comma?

Extension/src/LanguageServer/copilotCompletionContextProvider.ts

-telemetry: fix cancellation events. -telemetry: more diag event for registration failure. -add fallback to SimilarFiles providers such as openTabs and/or related-files

sean-mcmanus · 2025-02-26T23:34:27Z

Extension/src/LanguageServer/copilotCompletionContextProvider.ts

+                }
+                telemetry.addTraitsMetadata(projectContext, telemetryProperties, telemetryMetrics);
+
+                return copilotCompletionContext;
            } else {
                logMessage += `, result is missing`;
                telemetry.addResponseMetadata(true);
                return undefined;


This returning undefined is a little strange because it's losing information that the caller seems to want to check.

sean-mcmanus · 2025-02-26T23:37:36Z

Extension/src/LanguageServer/copilotCompletionContextProvider.ts

            }
            telemetry.addCompletionContextKind(copilotCompletionContextKind);
            telemetry.addResponseMetadata(copilotCompletionContext?.isResultMissing ?? true,
                copilotCompletionContext?.snippets?.length, copilotCompletionContext?.translationUnitUri,
                copilotCompletionContext?.caretOffset, copilotCompletionContext?.featureFlag);
+            logMessage += ` (id:${copilotCompletionContext?.requestId}) `;


This is odd/confusing, because ${copilotCompletionContext?.requestId will be undefined if isResultMissing.

By returning the undefined as mention earlier, you're losing the requestId. Maybe that doesn't really matter but it makes the logging a little confusing.

sean-mcmanus

@lukka Should the snippets be trimmed for whitespace?

sean-mcmanus

I still see unnecessary stuff sent to TypeScript like endLine. Can you remove that?

sean-mcmanus

I'm approving but it seems like some of the data sent to TypeScript could be cleaned up on the cpptools side.

lukka marked this pull request as ready for review February 20, 2025 18:16

lukka requested a review from a team as a code owner February 20, 2025 18:16

lukka changed the title ~~ContextProvider resolved improvements~~ ContextProvider resolve handler improvements Feb 20, 2025

sean-mcmanus approved these changes Feb 20, 2025

View reviewed changes

benmcmorran previously approved these changes Feb 20, 2025

View reviewed changes