M1

[GPTI314]

This PR will be imported into Gerrit with the title and first
comment (this text) used to generate the subject and body of
the Gerrit change.

Please ensure you adhere to every item in this list.

More info can be found at https://github.com/golang/go/wiki/CommitMessage

• The PR title is formatted as follows: frob the quux before blarfing
  • The part after the colon uses the verb tense + phrase that completes the blank in,
    "This change modifies Go to ___________"
  • Lowercase verb after the colon
  • No trailing period
  • Keep the title as short as possible. ideally under 76 characters or shorter
• No Markdown
• The first PR comment (this one) is wrapped at 76 characters, unless it's
  really needed (ASCII art, table, or long link)
• If there is a corresponding issue, add either Fixes golang/vscode-go#1234 or Updates golang/vscode-go#1234
  (the latter if this is not a complete fix) to this comment
• If referring to a repo, you can use the owner/repo#issue_number syntax:
  Fixes golang/tools#1234
• We do not use Signed-off-by lines in Go. Please don't add them.
  Our Gerrit server & GitHub bots enforce CLA compliance instead.
• Delete these instructions once you have read and applied them

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[Copilot]

Pull Request Overview

This PR modernizes the codebase by replacing deprecated String.substr() calls with String.substring() and String.slice(), converts synchronous file I/O to asynchronous operations, adds comprehensive JSDoc documentation, introduces type safety improvements, and consolidates duplicate configuration listeners.

Key Changes:

• Replaces all substr() calls with substring() or slice() throughout the codebase
• Converts file reading operations to async/await pattern in envUtils.ts and goCover.ts
• Adds detailed JSDoc documentation for key functions in util.ts, goMain.ts, and goLanguageServer.ts
• Introduces TypeScript type definitions (ExtensionCommand, ConfigurationObject) and constants for magic numbers
• Consolidates duplicate configuration change listeners in goMain.ts
• Improves error handling in several modules and adds cleanup handlers to prevent memory leaks

Reviewed Changes

Copilot reviewed 22 out of 22 changed files in this pull request and generated 13 comments.

Show a summary per file

File	Description
extension/src/utils/pathUtils.ts	Replaces substr() with substring(), fixes typo in comment, updates cache invalidation documentation
extension/src/utils/lsofProcessParser.ts	Replaces substr() with substring() for string parsing
extension/src/utils/envUtils.ts	Converts parseEnvFile and parseEnvFiles to async functions using fs.promises
extension/src/util.ts	Adds ExtensionCommand interface, comprehensive JSDoc comments, replaces substr() with substring(), improves error handling and logging
extension/src/testUtils.ts	Replaces substr() with substring()
extension/src/pickProcess.ts	Replaces substr() with substring()
extension/src/language/goLanguageServer.ts	Adds constants for timeouts/thresholds, introduces ConfigurationObject type, adds JSDoc documentation, improves type safety
extension/src/goVulncheck.ts	Adds event listener cleanup handlers to prevent memory leaks, improves error handling
extension/src/goVet.ts	Replaces substr() with substring()
extension/src/goTest/run.ts	Changes Promise.all to Promise.allSettled for better error handling
extension/src/goPackages.ts	Replaces substr() with substring()
extension/src/goModules.ts	Replaces substr() with substring()
extension/src/goMain.ts	Consolidates duplicate configuration listeners, adds JSDoc documentation for activation/deactivation functions, improves configuration change handling
extension/src/goLint.ts	Replaces substr() with substring()
extension/src/goInstall.ts	Replaces substr() with substring()
extension/src/goImport.ts	Replaces substr() with substring()
extension/src/goGenerateTests.ts	Replaces substr() with substring()
extension/src/goDebugConfiguration.ts	Adds event listener cleanup handlers to prevent memory leaks
extension/src/goCover.ts	Converts applyCodeCoverageToAllEditors to async function, refactors from Promise wrapper to async/await
extension/src/goBuild.ts	Replaces substr() with substring()
extension/src/diffUtils.ts	Replaces substr() with substring()
extension/src/debugAdapter/goDebug.ts	Replaces substr() with substring() throughout debug adapter

Comments suppressed due to low confidence (1)

extension/src/language/goLanguageServer.ts:78

• Unused variable GOPLS_SHUTDOWN_TIMEOUT_MS.

const GOPLS_SHUTDOWN_TIMEOUT_MS = 2000;

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

[nitpick] The ExtensionCommand interface is defined but the category field is marked as optional. However, when it's used in the filter on line 478, the code doesn't check if category exists before potentially using it. While this might work fine with VS Code's package.json structure, consider documenting what values category can have or when it's present vs absent.

Suggested change

	title: string;
	title: string;
	/**
	* The category of the command, as defined in VS Code's package.json.
	* This is used for grouping commands in the command palette.
	* Typical values include "Go", "Debug", etc.
	* May be absent for commands not shown in the palette or not grouped.
	*/

[Copilot]

The error event handler resolves with null instead of rejecting the promise. This could mask errors and make debugging harder. Consider either rejecting the promise with the error or at least logging it before resolving.

Suggested change:

p.on('error', (err) => {
    cleanup();
    reject(err); // or at minimum: console.error('vulncheck process error:', err);
});

[Copilot]

The cache invalidation logic has changed significantly. The old code checked if the value of getToolsGopath() changed by comparing it with getToolsGopath(false) (cached vs uncached). The new code only checks if the configuration changed via e.affectsConfiguration(). This could miss cases where toolsGopath is derived from other settings or environment variables that changed, potentially causing stale cache issues.

Consider whether this change captures all scenarios where the cache should be cleared, or if additional logic is needed to detect actual value changes rather than just configuration changes.

Suggested change

	// Clear tool cache if toolsGopath changed
	if (e.affectsConfiguration('go.toolsGopath') || e.affectsConfiguration('go.alternateTools')) {
	// Clear tool cache if toolsGopath changed (compare cached vs uncached value)
	const cachedToolsGopath = getToolsGopath();
	const uncachedToolsGopath = getToolsGopath(false);
	if (cachedToolsGopath !== uncachedToolsGopath) {

[Copilot]

The comment references go.toolsGopath and go.alternateTools as the settings that trigger cache invalidation, but the original comment mentioned go.goroot and go.alternateTool (singular). Please verify this is accurate and matches the actual cache invalidation logic in goMain.ts (lines 355-358), which checks for both go.toolsGopath and go.alternateTools.

[Copilot]

The outputChannel is referenced but may not be imported or defined in the current scope. Verify that outputChannel is available in this context. If this code is inside util.ts and outputChannel is not a global, this will cause a runtime error.

[Copilot]

The comment says "Log stderr but don't fail" but the code uses outputChannel.debug(). However, outputChannel may not be imported/available in this scope (see other comment). Additionally, changing from error() to outputChannel.debug() changes the behavior - the original code used the error() function which might do more than just logging. Verify this change is intentional and that it won't suppress important error information.

Suggested change

	outputChannel.debug(`stderr from "${goRuntimePath} version": ${stderr}`);
	outputChannel.info(`stderr from "${goRuntimePath} version": ${stderr}`);

[Copilot]

The outputChannel is referenced but may not be imported or defined in the current scope. Verify that outputChannel is available in this context. If this code is inside util.ts and outputChannel is not a global, this will cause a runtime error.

[Copilot]

Changing parseEnvFile from synchronous to asynchronous is a breaking change. All callers of this function must be updated to await the Promise. Verify that all call sites have been updated throughout the codebase to handle the async version, otherwise this will cause runtime errors.

[Copilot]

Changing applyCodeCoverageToAllEditors from synchronous to asynchronous is a breaking change. All callers of this function must be updated to await the Promise. Verify that all call sites have been updated throughout the codebase to handle the async version, otherwise this will cause runtime errors.

[Copilot]

The new constant GOPLS_SHUTDOWN_TIMEOUT_MS is defined but never used in the visible diff. Verify that this constant is actually being used somewhere in the file, otherwise it should be removed to avoid dead code.

Suggested change

const GOPLS_SHUTDOWN_TIMEOUT_MS = 2000;