Make ICommandHandler default to ...args: unknown[]
#270134
Conversation
This catches cases where the command definition doesn't specify any parameter types. I also updated `registerCommand` to not produce errors for any handlers that do have explicit types
📬 CODENOTIFYThe following users are being notified based on files changed in this PR: @bpaseroMatched files:
|
|
Pinging area owners. Please check out this PR and push fixes to this branch for your area during debt week
For most cases, the best fix is to add a runtime type check to make sure the passed in argument is the type you expect |
There was a problem hiding this comment.
Pull Request Overview
This PR enhances type safety for command handlers by changing the default parameter type from any[] to unknown[] in the ICommandHandler interface. This modification helps catch cases where command definitions don't specify parameter types and requires explicit type checking or casting when accessing command arguments.
Key changes:
- Modified
ICommandHandlerto default tounknown[]parameter types instead ofany[] - Updated command registrations throughout the codebase to handle the stricter typing
- Added type guards and assertions to validate command arguments at runtime
Reviewed Changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/vs/platform/commands/common/commands.ts | Updated ICommandHandler and ICommand interfaces to use generic type parameters with unknown[] default |
| src/vs/platform/keybinding/common/keybindingsRegistry.ts | Added generic type parameters to keybinding command interfaces |
| src/vs/workbench/contrib/timeline/browser/timeline.contribution.ts | Added URI import and type guard for command handler argument |
| src/vs/workbench/contrib/tasks/common/tasks.ts | Added new ITaskConfig interface definition |
| src/vs/workbench/contrib/tasks/browser/abstractTaskService.ts | Updated command handlers with explicit parameter types and imported ITaskConfig |
| src/vs/workbench/contrib/remote/browser/tunnelView.ts | Replaced property checks with isRemoteTunnel type guard |
| src/vs/workbench/contrib/files/browser/fileCommands.ts | Changed parameter types from URI | object to unknown |
| src/vs/workbench/contrib/files/browser/editors/textFileSaveErrorHandler.ts | Added URI type guard in command handlers |
| src/vs/workbench/contrib/debug/browser/debugCommands.ts | Updated parameter type from string to unknown |
| src/vs/workbench/contrib/chat/browser/promptSyntax/promptToolsCodeLensProvider.ts | Added explicit type casting and model import |
| src/vs/workbench/contrib/chat/browser/contrib/chatInputCompletions.ts | Added assertType usage for runtime validation |
| src/vs/workbench/browser/parts/editor/editorCommands.ts | Added type casting and runtime type checks |
| src/vs/workbench/api/browser/mainThreadNotebook.ts | Updated command with explicit parameter types |
| src/vs/platform/tunnel/common/tunnel.ts | Added isRemoteTunnel type guard function |
| src/vs/editor/contrib/smartSelect/browser/smartSelect.ts | Added array validation with new isArrayOf utility |
| src/vs/editor/contrib/links/browser/getLinks.ts | Added explicit type casting |
| src/vs/editor/contrib/format/browser/format.ts | Added isFormattingOptions type guard |
| src/vs/editor/contrib/colorPicker/browser/colorPickerContribution.ts | Added null check and explicit type casting |
| src/vs/base/common/types.ts | Added isArrayOf utility function and refactored isStringArray |
| src/vs/base/test/common/types.test.ts | Added comprehensive tests for new isArrayOf and isStringArray functions |
| this.updateTools(first, Range.lift(second), third); | ||
| const model = first as IEditorModel; | ||
| if (isITextModel(model) && Range.isIRange(second) && Array.isArray(third)) { | ||
| this.updateTools(model as ITextModel, Range.lift(second), third); |
There was a problem hiding this comment.
Double type casting is unnecessary and confusing. Since isITextModel(model) already validates that model is an ITextModel, the second cast model as ITextModel is redundant.
| this.updateTools(model as ITextModel, Range.lift(second), third); | |
| this.updateTools(model, Range.lift(second), third); |
Copilot uses AI. Check for mistakes.
|
|
||
| // resolve links | ||
| for (let i = 0; i < Math.min(resolveCount, list.links.length); i++) { | ||
| for (let i = 0; i < Math.min(resolveCount as number, list.links.length); i++) { |
There was a problem hiding this comment.
The type assertion resolveCount as number should be replaced with proper type validation using a type guard to ensure runtime safety.
| for (let i = 0; i < Math.min(resolveCount as number, list.links.length); i++) { | |
| for (let i = 0; i < Math.min(resolveCount, list.links.length); i++) { |
Copilot uses AI. Check for mistakes.
| CommandsRegistry.registerCommand('_executeColorPresentationProvider', function (accessor, ...args) { | ||
| const [color, context] = args; | ||
| const { uri, range } = context; | ||
| if (!context) { | ||
| return; | ||
| } | ||
|
|
||
| const { uri, range } = context as { uri?: unknown; range?: unknown }; | ||
| if (!(uri instanceof URI) || !Array.isArray(color) || color.length !== 4 || !Range.isIRange(range)) { |
There was a problem hiding this comment.
Using type assertion with optional properties doesn't provide meaningful type safety. Consider defining a proper interface for the expected context structure or using more specific type guards.
See below for a potential fix:
interface IColorPresentationContext {
uri: URI;
range: unknown;
}
function isColorPresentationContext(obj: any): obj is IColorPresentationContext {
return obj
&& obj.uri instanceof URI
&& obj.range !== undefined;
}
CommandsRegistry.registerCommand('_executeColorPresentationProvider', function (accessor, ...args) {
const [color, context] = args;
if (!isColorPresentationContext(context)) {
return;
}
const { uri, range } = context;
if (!Array.isArray(color) || color.length !== 4 || !Range.isIRange(range)) {
Copilot uses AI. Check for mistakes.
For #270132
This catches cases where the command definition doesn't specify any parameter types. I also updated
registerCommandto not produce errors for any handlers that do have explicit typesWork in progress. There are about 30 errors after making this change