chore(build/compiler): Enable a bunch of strict TS compiler options. (#6138)

This commit is contained in:
Richie Foreman 2025-08-13 16:17:38 -04:00 committed by GitHub
parent 8fae227e8d
commit a90aeb3d8f
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
28 changed files with 141 additions and 84 deletions

View File

@ -97,17 +97,17 @@ TypeScript's power lies in its ability to provide static type checking, catching
- **Preferring `unknown` over `any`**: When you absolutely cannot determine the type of a value at compile time, and you're tempted to reach for any, consider using unknown instead. unknown is a type-safe counterpart to any. While a variable of type unknown can hold any value, you must perform type narrowing (e.g., using typeof or instanceof checks, or a type assertion) before you can perform any operations on it. This forces you to handle the unknown type explicitly, preventing accidental runtime errors.
```
```ts
function processValue(value: unknown) {
if (typeof value === 'string') {
// value is now safely a string
console.log(value.toUpperCase());
} else if (typeof value === 'number') {
// value is now safely a number
console.log(value * 2);
}
// Without narrowing, you cannot access properties or methods on 'value'
// console.log(value.someProperty); // Error: Object is of type 'unknown'.
if (typeof value === 'string') {
// value is now safely a string
console.log(value.toUpperCase());
} else if (typeof value === 'number') {
// value is now safely a number
console.log(value * 2);
}
// Without narrowing, you cannot access properties or methods on 'value'
// console.log(value.someProperty); // Error: Object is of type 'unknown'.
}
```
@ -115,6 +115,27 @@ TypeScript's power lies in its ability to provide static type checking, catching
- **Bypassing Type Checking**: Like `any`, type assertions bypass TypeScript's safety checks. If your assertion is incorrect, you introduce a runtime error that TypeScript would not have warned you about.
- **Code Smell in Testing**: A common scenario where `any` or type assertions might be tempting is when trying to test "private" implementation details (e.g., spying on or stubbing an unexported function within a module). This is a strong indication of a "code smell" in your testing strategy and potentially your code structure. Instead of trying to force access to private internals, consider whether those internal details should be refactored into a separate module with a well-defined public API. This makes them inherently testable without compromising encapsulation.
### Type narrowing `switch` clauses
When authoring a switch clause over an enumeration or fixed list of items,
always prefer to use the `checkExhaustive` helper method within the default
clause of the switch. This will ensure that all of the possible options within
the value or enumeration are used.
This helper method can be found in `packages/cli/src/utils/checks.ts`
Here's an example of using the helper method properly:
```
switch (someValue) {
case 1:
case 2:
// ...
default:
return checkExhaustive(someValue);
}
```
### Embracing JavaScript's Array Operators
To further enhance code cleanliness and promote safe functional programming practices, leverage JavaScript's rich set of array operators as much as possible. Methods like `.map()`, `.filter()`, `.reduce()`, `.slice()`, `.sort()`, and others are incredibly powerful for transforming and manipulating data collections in an immutable and declarative way.

View File

@ -138,13 +138,11 @@ export const directoryCommand: SlashCommand = {
if (errors.length > 0) {
addItem(
{
type: MessageType.ERROR,
text: errors.join('\n'),
},
{ type: MessageType.ERROR, text: errors.join('\n') },
Date.now(),
);
}
return;
},
},
{

View File

@ -19,6 +19,7 @@ import {
findWordEndInLine,
} from './text-buffer.js';
import { cpLen, toCodePoints } from '../../utils/textUtils.js';
import { assumeExhaustive } from '../../../utils/checks.js';
// Check if we're at the end of a base word (on the last base character)
// Returns true if current position has a base character followed only by combining marks until non-word
@ -806,7 +807,7 @@ export function handleVimAction(
default: {
// This should never happen if TypeScript is working correctly
const _exhaustiveCheck: never = action;
assumeExhaustive(action);
return state;
}
}

View File

@ -0,0 +1,28 @@
/**
* @license
* Copyright 2025 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/
/* Fail to compile on unexpected values. */
export function assumeExhaustive(_value: never): void {}
/**
* Throws an exception on unexpected values.
*
* A common use case is switch statements:
* switch(enumValue) {
* case Enum.A:
* case Enum.B:
* break;
* default:
* checkExhaustive(enumValue);
* }
*/
export function checkExhaustive(
value: never,
msg = `unexpected value ${value}!`,
): never {
assumeExhaustive(value);
throw new Error(msg);
}

View File

@ -9,7 +9,6 @@ import { describe, it, expect, vi } from 'vitest';
import {
CoreToolScheduler,
ToolCall,
ValidatingToolCall,
convertToFunctionResponse,
} from './coreToolScheduler.js';
import {
@ -54,7 +53,9 @@ class MockModifiableTool
};
}
async shouldConfirmExecute(): Promise<ToolCallConfirmationDetails | false> {
override async shouldConfirmExecute(): Promise<
ToolCallConfirmationDetails | false
> {
if (this.shouldConfirm) {
return {
type: 'edit',
@ -121,8 +122,6 @@ describe('CoreToolScheduler', () => {
abortController.abort();
await scheduler.schedule([request], abortController.signal);
const _waitingCall = onToolCallsUpdate.mock
.calls[1][0][0] as ValidatingToolCall;
const confirmationDetails = await mockTool.shouldConfirmExecute(
{},
abortController.signal,
@ -394,7 +393,7 @@ describe('CoreToolScheduler edit cancellation', () => {
);
}
async shouldConfirmExecute(
override async shouldConfirmExecute(
_params: Record<string, unknown>,
_abortSignal: AbortSignal,
): Promise<ToolCallConfirmationDetails | false> {

View File

@ -91,7 +91,6 @@ export class MCPOAuthProvider {
private static readonly REDIRECT_PORT = 7777;
private static readonly REDIRECT_PATH = '/oauth/callback';
private static readonly HTTP_OK = 200;
private static readonly HTTP_REDIRECT = 302;
/**
* Register a client dynamically with the OAuth server.

View File

@ -45,7 +45,7 @@ export class MockTool extends BaseTool<{ [key: string]: unknown }, ToolResult> {
);
}
async shouldConfirmExecute(
override async shouldConfirmExecute(
_params: { [key: string]: unknown },
_abortSignal: AbortSignal,
): Promise<ToolCallConfirmationDetails | false> {

View File

@ -471,7 +471,7 @@ Expectation for required parameters:
* @param params Parameters to validate
* @returns Error message string or null if valid
*/
validateToolParams(params: EditToolParams): string | null {
override validateToolParams(params: EditToolParams): string | null {
const errors = SchemaValidator.validate(
this.schema.parametersJsonSchema,
params,

View File

@ -281,7 +281,7 @@ export class GlobTool extends BaseDeclarativeTool<GlobToolParams, ToolResult> {
/**
* Validates the parameters for the tool.
*/
validateToolParams(params: GlobToolParams): string | null {
override validateToolParams(params: GlobToolParams): string | null {
const errors = SchemaValidator.validate(
this.schema.parametersJsonSchema,
params,

View File

@ -614,7 +614,7 @@ export class GrepTool extends BaseDeclarativeTool<GrepToolParams, ToolResult> {
* @param params Parameters to validate
* @returns An error message string if invalid, null otherwise
*/
validateToolParams(params: GrepToolParams): string | null {
override validateToolParams(params: GrepToolParams): string | null {
const errors = SchemaValidator.validate(
this.schema.parametersJsonSchema,
params,

View File

@ -314,7 +314,7 @@ export class LSTool extends BaseDeclarativeTool<LSToolParams, ToolResult> {
* @param params Parameters to validate
* @returns An error message string if invalid, null otherwise
*/
validateToolParams(params: LSToolParams): string | null {
override validateToolParams(params: LSToolParams): string | null {
const errors = SchemaValidator.validate(
this.schema.parametersJsonSchema,
params,

View File

@ -70,7 +70,7 @@ class DiscoveredMCPToolInvocation extends BaseToolInvocation<
super(params);
}
async shouldConfirmExecute(
override async shouldConfirmExecute(
_abortSignal: AbortSignal,
): Promise<ToolCallConfirmationDetails | false> {
const serverAllowListKey = this.serverName;
@ -135,7 +135,7 @@ export class DiscoveredMCPTool extends BaseDeclarativeTool<
readonly serverName: string,
readonly serverToolName: string,
description: string,
readonly parameterSchema: unknown,
override readonly parameterSchema: unknown,
readonly timeout?: number,
readonly trust?: boolean,
nameOverride?: string,

View File

@ -180,7 +180,7 @@ class MemoryToolInvocation extends BaseToolInvocation<
return `in ${tildeifyPath(memoryFilePath)}`;
}
async shouldConfirmExecute(
override async shouldConfirmExecute(
_abortSignal: AbortSignal,
): Promise<ToolEditConfirmationDetails | false> {
const memoryFilePath = getGlobalMemoryFilePath();
@ -294,7 +294,7 @@ export class MemoryTool
);
}
validateToolParams(params: SaveMemoryParams): string | null {
override validateToolParams(params: SaveMemoryParams): string | null {
const errors = SchemaValidator.validate(
this.schema.parametersJsonSchema,
params,

View File

@ -198,7 +198,9 @@ export class ReadFileTool extends BaseDeclarativeTool<
);
}
protected validateToolParams(params: ReadFileToolParams): string | null {
protected override validateToolParams(
params: ReadFileToolParams,
): string | null {
const errors = SchemaValidator.validate(
this.schema.parametersJsonSchema,
params,

View File

@ -626,7 +626,9 @@ Use this tool when the user's query implies needing the content of several files
);
}
protected validateToolParams(params: ReadManyFilesParams): string | null {
protected override validateToolParams(
params: ReadManyFilesParams,
): string | null {
const errors = SchemaValidator.validate(
this.schema.parametersJsonSchema,
params,

View File

@ -67,7 +67,7 @@ class ShellToolInvocation extends BaseToolInvocation<
return description;
}
async shouldConfirmExecute(
override async shouldConfirmExecute(
_abortSignal: AbortSignal,
): Promise<ToolCallConfirmationDetails | false> {
const command = stripShellWrapper(this.params.command);
@ -343,7 +343,9 @@ export class ShellTool extends BaseDeclarativeTool<
);
}
protected validateToolParams(params: ShellToolParams): string | null {
protected override validateToolParams(
params: ShellToolParams,
): string | null {
const commandCheck = isCommandAllowed(params.command, this.config);
if (!commandCheck.allowed) {
if (!commandCheck.reason) {

View File

@ -19,8 +19,8 @@ export class DiscoveredTool extends BaseTool<ToolParams, ToolResult> {
constructor(
private readonly config: Config,
name: string,
readonly description: string,
readonly parameterSchema: Record<string, unknown>,
override readonly description: string,
override readonly parameterSchema: Record<string, unknown>,
) {
const discoveryCmd = config.getToolDiscoveryCommand()!;
const callCommand = config.getToolCallCommand()!;

View File

@ -284,13 +284,13 @@ export abstract class BaseTool<
* @param parameterSchema JSON Schema defining the parameters
*/
constructor(
readonly name: string,
readonly displayName: string,
readonly description: string,
readonly kind: Kind,
readonly parameterSchema: unknown,
readonly isOutputMarkdown: boolean = true,
readonly canUpdateOutput: boolean = false,
override readonly name: string,
override readonly displayName: string,
override readonly description: string,
override readonly kind: Kind,
override readonly parameterSchema: unknown,
override readonly isOutputMarkdown: boolean = true,
override readonly canUpdateOutput: boolean = false,
) {
super(
name,
@ -320,7 +320,7 @@ export abstract class BaseTool<
* @returns An error message string if invalid, null otherwise
*/
// eslint-disable-next-line @typescript-eslint/no-unused-vars
validateToolParams(params: TParams): string | null {
override validateToolParams(params: TParams): string | null {
// Implementation would typically use a JSON Schema validator
// This is a placeholder that should be implemented by derived classes
return null;

View File

@ -143,7 +143,9 @@ ${textContent}
return `Processing URLs and instructions from prompt: "${displayPrompt}"`;
}
async shouldConfirmExecute(): Promise<ToolCallConfirmationDetails | false> {
override async shouldConfirmExecute(): Promise<
ToolCallConfirmationDetails | false
> {
if (this.config.getApprovalMode() === ApprovalMode.AUTO_EDIT) {
return false;
}
@ -337,7 +339,9 @@ export class WebFetchTool extends BaseDeclarativeTool<
}
}
protected validateToolParams(params: WebFetchToolParams): string | null {
protected override validateToolParams(
params: WebFetchToolParams,
): string | null {
const errors = SchemaValidator.validate(
this.schema.parametersJsonSchema,
params,

View File

@ -103,7 +103,7 @@ export class WebSearchTool extends BaseTool<
return null;
}
getDescription(params: WebSearchToolParams): string {
override getDescription(params: WebSearchToolParams): string {
return `Searching the web for: "${params.query}"`;
}

View File

@ -102,11 +102,11 @@ export class WriteFileTool
);
}
toolLocations(params: WriteFileToolParams): ToolLocation[] {
override toolLocations(params: WriteFileToolParams): ToolLocation[] {
return [{ path: params.file_path }];
}
validateToolParams(params: WriteFileToolParams): string | null {
override validateToolParams(params: WriteFileToolParams): string | null {
const errors = SchemaValidator.validate(
this.schema.parametersJsonSchema,
params,
@ -144,7 +144,7 @@ export class WriteFileTool
return null;
}
getDescription(params: WriteFileToolParams): string {
override getDescription(params: WriteFileToolParams): string {
if (!params.file_path) {
return `Model did not provide valid parameters for write file tool, missing or empty "file_path"`;
}
@ -158,7 +158,7 @@ export class WriteFileTool
/**
* Handles the confirmation prompt for the WriteFile tool.
*/
async shouldConfirmExecute(
override async shouldConfirmExecute(
params: WriteFileToolParams,
abortSignal: AbortSignal,
): Promise<ToolCallConfirmationDetails | false> {

View File

@ -13,8 +13,6 @@ import { AuthType } from '../core/contentGenerator.js';
import { StructuredError } from '../core/turn.js';
describe('parseAndFormatApiError', () => {
const _enterpriseMessage =
'upgrade to a Gemini Code Assist Standard or Enterprise plan with higher limits';
const vertexMessage = 'request a quota increase through Vertex';
const geminiMessage = 'request a quota increase through AI Studio';

View File

@ -289,7 +289,7 @@ export class FileSearch {
* Builds the in-memory cache for fast pattern matching.
*/
private buildResultCache(): void {
this.resultCache = new ResultCache(this.allFiles, this.absoluteDir);
this.resultCache = new ResultCache(this.allFiles);
// The v1 algorithm is much faster since it only looks at the first
// occurence of the pattern. We use it for search spaces that have >20k
// files, because the v2 algorithm is just too slow in those cases.

View File

@ -4,7 +4,6 @@
* SPDX-License-Identifier: Apache-2.0
*/
import path from 'node:path';
import { test, expect } from 'vitest';
import { ResultCache } from './result-cache.js';
@ -17,7 +16,7 @@ test('ResultCache basic usage', async () => {
'subdir/other.js',
'subdir/nested/file.md',
];
const cache = new ResultCache(files, path.resolve('.'));
const cache = new ResultCache(files);
const { files: resultFiles, isExactMatch } = await cache.get('*.js');
expect(resultFiles).toEqual(files);
expect(isExactMatch).toBe(false);
@ -25,7 +24,7 @@ test('ResultCache basic usage', async () => {
test('ResultCache cache hit/miss', async () => {
const files = ['foo.txt', 'bar.js', 'baz.md'];
const cache = new ResultCache(files, path.resolve('.'));
const cache = new ResultCache(files);
// First call: miss
const { files: result1Files, isExactMatch: isExactMatch1 } =
await cache.get('*.js');
@ -44,7 +43,7 @@ test('ResultCache cache hit/miss', async () => {
test('ResultCache best base query', async () => {
const files = ['foo.txt', 'foobar.js', 'baz.md'];
const cache = new ResultCache(files, path.resolve('.'));
const cache = new ResultCache(files);
// Cache a broader query
cache.set('foo', ['foo.txt', 'foobar.js']);

View File

@ -13,10 +13,7 @@ export class ResultCache {
private hits = 0;
private misses = 0;
constructor(
private readonly allFiles: string[],
private readonly absoluteDir: string,
) {
constructor(private readonly allFiles: string[]) {
this.cache = new Map();
}

View File

@ -11,7 +11,7 @@ import { marked } from 'marked';
import { processImports, validateImportPath } from './memoryImportProcessor.js';
// Helper function to create platform-agnostic test paths
const testPath = (...segments: string[]) => {
function testPath(...segments: string[]): string {
// Start with the first segment as is (might be an absolute path on Windows)
let result = segments[0];
@ -27,9 +27,8 @@ const testPath = (...segments: string[]) => {
}
return path.normalize(result);
};
}
// Mock fs/promises
vi.mock('fs/promises');
const mockedFs = vi.mocked(fs);
@ -509,21 +508,21 @@ describe('memoryImportProcessor', () => {
expect(result.importTree.imports).toHaveLength(2);
// First import: nested.md
// Prefix with underscore to indicate they're intentionally unused
const _expectedNestedPath = testPath(projectRoot, 'src', 'nested.md');
const _expectedInnerPath = testPath(projectRoot, 'src', 'inner.md');
const _expectedSimplePath = testPath(projectRoot, 'src', 'simple.md');
// Check that the paths match using includes to handle potential absolute/relative differences
expect(result.importTree.imports![0].path).toContain('nested.md');
const expectedNestedPath = testPath(projectRoot, 'src', 'nested.md');
expect(result.importTree.imports![0].path).toContain(expectedNestedPath);
expect(result.importTree.imports![0].imports).toHaveLength(1);
const expectedInnerPath = testPath(projectRoot, 'src', 'inner.md');
expect(result.importTree.imports![0].imports![0].path).toContain(
'inner.md',
expectedInnerPath,
);
expect(result.importTree.imports![0].imports![0].imports).toBeUndefined();
// Second import: simple.md
expect(result.importTree.imports![1].path).toContain('simple.md');
const expectedSimplePath = testPath(projectRoot, 'src', 'simple.md');
expect(result.importTree.imports![1].path).toContain(expectedSimplePath);
expect(result.importTree.imports![1].imports).toBeUndefined();
});
@ -724,21 +723,20 @@ describe('memoryImportProcessor', () => {
expect(result.importTree.imports).toHaveLength(2);
// First import: nested.md
// Prefix with underscore to indicate they're intentionally unused
const _expectedNestedPath = testPath(projectRoot, 'src', 'nested.md');
const _expectedInnerPath = testPath(projectRoot, 'src', 'inner.md');
const _expectedSimplePath = testPath(projectRoot, 'src', 'simple.md');
const expectedNestedPath = testPath(projectRoot, 'src', 'nested.md');
const expectedInnerPath = testPath(projectRoot, 'src', 'inner.md');
const expectedSimplePath = testPath(projectRoot, 'src', 'simple.md');
// Check that the paths match using includes to handle potential absolute/relative differences
expect(result.importTree.imports![0].path).toContain('nested.md');
expect(result.importTree.imports![0].path).toContain(expectedNestedPath);
expect(result.importTree.imports![0].imports).toHaveLength(1);
expect(result.importTree.imports![0].imports![0].path).toContain(
'inner.md',
expectedInnerPath,
);
expect(result.importTree.imports![0].imports![0].imports).toBeUndefined();
// Second import: simple.md
expect(result.importTree.imports![1].path).toContain('simple.md');
expect(result.importTree.imports![1].path).toContain(expectedSimplePath);
expect(result.importTree.imports![1].imports).toBeUndefined();
});
@ -899,7 +897,7 @@ describe('memoryImportProcessor', () => {
// Test relative paths - resolve them against basePath
const relativePath = './file.md';
const _resolvedRelativePath = path.resolve(basePath, relativePath);
path.resolve(basePath, relativePath);
expect(validateImportPath(relativePath, basePath, [basePath])).toBe(true);
// Test parent directory access (should be allowed if parent is in allowed paths)
@ -907,12 +905,12 @@ describe('memoryImportProcessor', () => {
if (parentPath !== basePath) {
// Only test if parent is different
const parentRelativePath = '../file.md';
const _resolvedParentPath = path.resolve(basePath, parentRelativePath);
path.resolve(basePath, parentRelativePath);
expect(
validateImportPath(parentRelativePath, basePath, [parentPath]),
).toBe(true);
const _resolvedSubPath = path.resolve(basePath, 'sub');
path.resolve(basePath, 'sub');
const resultSub = validateImportPath('sub', basePath, [basePath]);
expect(resultSub).toBe(true);
}

View File

@ -261,7 +261,7 @@ export async function processImports(
// Process imports in reverse order to handle indices correctly
for (let i = imports.length - 1; i >= 0; i--) {
const { start, _end, path: importPath } = imports[i];
const { start, path: importPath } = imports[i];
// Skip if inside a code region
if (

View File

@ -3,7 +3,16 @@
"strict": true,
"esModuleInterop": true,
"skipLibCheck": true,
"noImplicitAny": true,
"noImplicitOverride": true,
"noImplicitReturns": true,
"noImplicitThis": true,
"forceConsistentCasingInFileNames": true,
"noUnusedLocals": true,
"strictBindCallApply": true,
"strictFunctionTypes": true,
"strictNullChecks": true,
"strictPropertyInitialization": true,
"resolveJsonModule": true,
"sourceMap": true,
"composite": true,