Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 55 additions & 16 deletions src/features/interpreterSelection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,10 +44,10 @@ export interface PriorityChainResult {
export interface SettingResolutionError {
/** The setting that failed */
setting: 'pythonProjects' | 'defaultEnvManager' | 'defaultInterpreterPath';
/** Structured failure kind, used for localization and stale-error re-checks. */
kind: 'managerNotRegistered' | 'pathUnresolvedVariables' | 'pathCannotResolve';
/** The configured value */
configuredValue: string;
/** Reason for failure */
reason: string;
}

/**
Expand Down Expand Up @@ -81,8 +81,8 @@ async function resolvePriorityChainCore(
}
const error: SettingResolutionError = {
setting: 'pythonProjects',
kind: 'managerNotRegistered',
configuredValue: projectManagerId,
reason: `Environment manager '${projectManagerId}' is not registered`,
};
errors.push(error);
traceWarn(`${logPrefix} pythonProjects[] manager '${projectManagerId}' not found, trying next priority`);
Expand All @@ -99,8 +99,8 @@ async function resolvePriorityChainCore(
}
const error: SettingResolutionError = {
setting: 'defaultEnvManager',
kind: 'managerNotRegistered',
configuredValue: userConfiguredManager,
reason: `Environment manager '${userConfiguredManager}' is not registered`,
};
Comment thread
StellaHuang95 marked this conversation as resolved.
errors.push(error);
traceWarn(`${logPrefix} defaultEnvManager '${userConfiguredManager}' not found, trying next priority`);
Expand All @@ -118,8 +118,8 @@ async function resolvePriorityChainCore(
);
const error: SettingResolutionError = {
setting: 'defaultInterpreterPath',
kind: 'pathUnresolvedVariables',
configuredValue: userInterpreterPath,
reason: l10n.t('Path contains unresolved variables'),
};
errors.push(error);
} else {
Expand All @@ -138,8 +138,8 @@ async function resolvePriorityChainCore(
);
const error: SettingResolutionError = {
setting: 'defaultInterpreterPath',
kind: 'pathUnresolvedVariables',
configuredValue: userInterpreterPath,
reason: l10n.t('Path contains unresolved variables'),
};
errors.push(error);
} else {
Expand All @@ -155,8 +155,8 @@ async function resolvePriorityChainCore(
}
const error: SettingResolutionError = {
setting: 'defaultInterpreterPath',
kind: 'pathCannotResolve',
configuredValue: userInterpreterPath,
reason: `Could not resolve interpreter path '${userInterpreterPath}'`,
};
errors.push(error);
traceWarn(
Expand Down Expand Up @@ -383,8 +383,9 @@ export async function applyInitialEnvironmentSelection(
}
resolveGlobalScope()
.then(async (globalErrors) => {
if (globalErrors.length > 0) {
await notifyUserOfSettingErrors(globalErrors);
const liveGlobalErrors = filterLiveSettingErrors(globalErrors, envManagers);
if (liveGlobalErrors.length > 0) {
await notifyUserOfSettingErrors(liveGlobalErrors);
}
})
.catch((err) => traceError(`[interpreterSelection] Background global scope resolution failed: ${err}`));
Expand All @@ -397,9 +398,11 @@ export async function applyInitialEnvironmentSelection(
allErrors.push(...globalErrors);
}

// Notify user if any settings could not be applied (workspace + global when awaited)
if (allErrors.length > 0) {
await notifyUserOfSettingErrors(allErrors);
// Drop errors that became stale during the awaits above; see filterLiveSettingErrors.
const liveErrors = filterLiveSettingErrors(allErrors, envManagers);

if (liveErrors.length > 0) {
await notifyUserOfSettingErrors(liveErrors);
}

// Numeric values must go via the measures argument (properties are dropped).
Expand All @@ -409,7 +412,7 @@ export async function applyInitialEnvironmentSelection(
duration: selectionStopWatch.elapsedTime,
workspaceFolderCount: folders.length,
resolvedFolderCount,
settingErrorCount: allErrors.length,
settingErrorCount: liveErrors.length,
},
{
globalScopeDeferred: workspaceFolderResolved,
Expand All @@ -430,7 +433,42 @@ export function resetSettingWarnings(): void {
warnedSettings.clear();
}

function localizedReasonFor(error: SettingResolutionError): string {
switch (error.kind) {
case 'managerNotRegistered':
return l10n.t("Environment manager '{0}' is not registered", error.configuredValue);
case 'pathUnresolvedVariables':
return l10n.t('Path contains unresolved variables');
case 'pathCannotResolve':
return l10n.t("Could not resolve interpreter path '{0}'", error.configuredValue);
}
}

/**
* Re-check the live registry to drop `managerNotRegistered` errors that became
* stale while later awaits ran — a third-party manager extension may have
* registered in the meantime, in which case the warning would be a false positive.
*/
function filterLiveSettingErrors(
errors: SettingResolutionError[],
envManagers: EnvironmentManagers,
): SettingResolutionError[] {
return errors.filter((e) => {
if (e.kind === 'managerNotRegistered' && envManagers.getEnvironmentManager(e.configuredValue)) {
traceVerbose(
`[interpreterSelection] Manager '${e.configuredValue}' is now registered; suppressing stale warning`,
);
return false;
}
return true;
});
}

async function notifyUserOfSettingErrors(errors: SettingResolutionError[]): Promise<void> {
if (errors.length === 0) {
return;
}

// Group errors by setting type to avoid spamming the user
const uniqueSettings = [...new Set(errors.map((e) => e.setting))];

Expand All @@ -442,6 +480,7 @@ async function notifyUserOfSettingErrors(errors: SettingResolutionError[]): Prom

const settingErrors = errors.filter((e) => e.setting === setting);
const firstError = settingErrors[0];
const reason = localizedReasonFor(firstError);

let message: string;
let settingKey: string;
Expand All @@ -451,23 +490,23 @@ async function notifyUserOfSettingErrors(errors: SettingResolutionError[]): Prom
message = l10n.t(
"Python project setting for environment manager '{0}' could not be applied: {1}",
firstError.configuredValue,
firstError.reason,
reason,
);
settingKey = 'python-envs.pythonProjects';
break;
case 'defaultEnvManager':
message = l10n.t(
"Default environment manager '{0}' could not be applied: {1}",
firstError.configuredValue,
firstError.reason,
reason,
);
settingKey = 'python-envs.defaultEnvManager';
break;
case 'defaultInterpreterPath':
message = l10n.t(
"Default interpreter path '{0}' could not be resolved: {1}",
firstError.configuredValue,
firstError.reason,
reason,
);
settingKey = 'python.defaultInterpreterPath';
break;
Expand Down
91 changes: 91 additions & 0 deletions src/test/features/interpreterSelection.unit.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -968,6 +968,97 @@ suite('Interpreter Selection - applyInitialEnvironmentSelection', () => {
const warningMessage = showWarnStub.firstCall.args[0] as string;
assert.ok(warningMessage.includes('/nonexistent/python'), 'Warning message should include the configured path');
});

test('should suppress stale warning when manager registers during await (issue #1517)', async () => {
// Simulate the race: defaultEnvManager is set to 'pypa.hatch:hatch', which is not
// registered at priority-chain evaluation time, but becomes registered before
// notifyUserOfSettingErrors runs. The warning must NOT be shown — neither from the
// awaited workspace path nor from the deferred global `.then(...)` path.
const hatchManagerId = 'pypa.hatch:hatch';

sandbox.stub(workspaceApis, 'getWorkspaceFolders').returns([{ uri: testUri, name: 'test', index: 0 }]);
sandbox.stub(workspaceApis, 'getConfiguration').returns(createMockConfig([]) as WorkspaceConfiguration);
sandbox.stub(helpers, 'getUserConfiguredSetting').callsFake((section: string, key: string) => {
if (section === 'python-envs' && key === 'defaultEnvManager') {
return hatchManagerId;
}
return undefined;
});

// Hatch is not registered when the priority chain runs (returns undefined),
// but by the time notifyUserOfSettingErrors is called, it is available.
const mockHatchManager = {
id: hatchManagerId,
name: 'hatch',
displayName: 'Hatch',
get: sandbox.stub().resolves(undefined),
set: sandbox.stub().resolves(),
} as unknown as sinon.SinonStubbedInstance<InternalEnvironmentManager>;

let hatchRegistered = false;
// Override getEnvironmentManager: returns undefined until hatchRegistered is set, then returns the manager
mockEnvManagers.getEnvironmentManager.callsFake((scope: unknown) => {
const id = typeof scope === 'string' ? scope : undefined;
if (id === hatchManagerId && hatchRegistered) {
return mockHatchManager;
}
if (id === 'ms-python.python:venv') {
return mockVenvManager;
}
if (id === 'ms-python.python:system') {
return mockSystemManager;
}
return undefined;
});

// Simulate Hatch registering during the venv discovery await
mockVenvManager.get.callsFake(async () => {
hatchRegistered = true; // Hatch "registers" while venv discovery is awaited
return mockVenvEnv;
});

// The workspace folder will resolve (venv found), so global scope is deferred to
// a background `.then(...)`. Use a deferred promise to deterministically wait for
// that background work to complete before asserting on showWarningMessage —
// otherwise a regression in the deferred-path filtering would fire AFTER the test
// returned and silently slip through.
let resolveGlobalDone!: () => void;
const globalDone = new Promise<void>((resolve) => {
resolveGlobalDone = resolve;
});
mockEnvManagers.setEnvironments.callsFake(async () => {
resolveGlobalDone();
});

const showWarnStub = sandbox.stub(windowApis, 'showWarningMessage').resolves(undefined);

await applyInitialEnvironmentSelection(
mockEnvManagers as unknown as EnvironmentManagers,
mockProjectManager as unknown as PythonProjectManager,
mockNativeFinder as unknown as NativePythonFinder,
mockApi as unknown as PythonEnvironmentApi,
);

// Wait for the background global scope to call setEnvironments, then flush
// microtasks so the `.then(...)` handler that would call notifyUserOfSettingErrors
// has a chance to run before we assert.
await globalDone;
await new Promise<void>((resolve) => process.nextTick(resolve));

// Workspace folder should have resolved (venv found) and global scope was attempted
assert.ok(mockEnvManagers.setEnvironment.called, 'setEnvironment should be called for workspace folder');
assert.ok(
mockEnvManagers.setEnvironments.called,
'setEnvironments should be called for deferred global scope',
);

// Warning must NOT be shown because the error was stale on both paths
assert.strictEqual(
showWarnStub.called,
false,
'showWarningMessage must NOT be called when manager registered before notification',
);
Comment thread
StellaHuang95 marked this conversation as resolved.
});
});

suite('Interpreter Selection - resolveGlobalEnvironmentByPriority', () => {
Expand Down
Loading