Skip to content

Commit

Permalink
chore: enable unbound-method eslint rule (#33605)
Browse files Browse the repository at this point in the history
Unbound method calls using `this` are likely to have unintended effects.

Here is an example with static methods, but the same thing applies to object methods:

```ts
class Class {
  public static staticMethod() {
    this.otherStaticMethod()
  }

  public static otherStaticMethod() { }
}

// ✅ valid
Class.staticMethod();

// ❌ boom
const x = Class.staticMethod;
x();
```

When assigning a method to a variable, you need to take extra care and this linter rule is going to remind you.

This rule also catches a lot of cases were we meant to call a function but accidentally didn't:

```ts
// Actual examples of unintentional code in our code base
list.map(x => x.toString).join(', ')

expect(x).toBeTruthy
```

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
rix0rrr authored Feb 28, 2025
1 parent 127059e commit f44209e
Show file tree
Hide file tree
Showing 47 changed files with 123 additions and 101 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -178,12 +178,12 @@ customerDS.createResolver('MutationRemoveCustomer', {
responseMappingTemplate: MappingTemplate.dynamoDbResultItem(),
});

const ops = [
{ suffix: 'Eq', op: KeyCondition.eq },
{ suffix: 'Lt', op: KeyCondition.lt },
{ suffix: 'Le', op: KeyCondition.le },
{ suffix: 'Gt', op: KeyCondition.gt },
{ suffix: 'Ge', op: KeyCondition.ge },
const ops: Array<{ suffix: string; op: (x: string, y: string) => KeyCondition }> = [
{ suffix: 'Eq', op: (x, y) => KeyCondition.eq(x, y) },
{ suffix: 'Lt', op: (x, y) => KeyCondition.lt(x, y) },
{ suffix: 'Le', op: (x, y) => KeyCondition.le(x, y) },
{ suffix: 'Gt', op: (x, y) => KeyCondition.gt(x, y) },
{ suffix: 'Ge', op: (x, y) => KeyCondition.ge(x, y) },
];
for (const { suffix, op } of ops) {
orderDS.createResolver(`QueryGetCustomerOrders${suffix}`, {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -633,7 +633,7 @@ function getPossibleAmiTypes(instanceTypes: InstanceType[]): NodegroupAmiType[]
const architectures: Set<AmiArchitecture> = new Set(instanceTypes.map(typeToArch));

if (architectures.size === 0) { // protective code, the current implementation will never result in this.
throw new Error(`Cannot determine any ami type compatible with instance types: ${instanceTypes.map(i => i.toString).join(', ')}`);
throw new Error(`Cannot determine any ami type compatible with instance types: ${instanceTypes.map(i => i.toString()).join(', ')}`);
}

if (architectures.size > 1) {
Expand Down
12 changes: 6 additions & 6 deletions packages/@aws-cdk/aws-glue-alpha/lib/triggers/workflow.ts
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ export abstract class WorkflowBase extends cdk.Resource implements IWorkflow {
...options,
workflowName: this.workflowName,
type: 'ON_DEMAND',
actions: options.actions?.map(this.renderAction),
actions: options.actions?.map(this.renderAction.bind(this)),
description: options.description || undefined,
});

Expand All @@ -162,7 +162,7 @@ export abstract class WorkflowBase extends cdk.Resource implements IWorkflow {
...options,
workflowName: this.workflowName,
type: 'SCHEDULED',
actions: options.actions?.map(this.renderAction),
actions: options.actions?.map(this.renderAction.bind(this)),
schedule: dailySchedule.expressionString,
startOnCreation: options.startOnCreation ?? false,
});
Expand All @@ -189,7 +189,7 @@ export abstract class WorkflowBase extends cdk.Resource implements IWorkflow {
...options,
workflowName: this.workflowName,
type: 'SCHEDULED',
actions: options.actions?.map(this.renderAction),
actions: options.actions?.map(this.renderAction.bind(this)),
schedule: weeklySchedule.expressionString,
startOnCreation: options.startOnCreation ?? false,
});
Expand All @@ -210,7 +210,7 @@ export abstract class WorkflowBase extends cdk.Resource implements IWorkflow {
...options,
workflowName: this.workflowName,
type: 'SCHEDULED',
actions: options.actions?.map(this.renderAction),
actions: options.actions?.map(this.renderAction.bind(this)),
schedule: options.schedule.expressionString,
startOnCreation: options.startOnCreation ?? false,
});
Expand All @@ -231,7 +231,7 @@ export abstract class WorkflowBase extends cdk.Resource implements IWorkflow {
...options,
workflowName: this.workflowName,
type: 'EVENT',
actions: options.actions?.map(this.renderAction),
actions: options.actions?.map(this.renderAction.bind(this)),
eventBatchingCondition: this.renderEventBatchingCondition(options),
description: options.description ?? undefined,
});
Expand All @@ -253,7 +253,7 @@ export abstract class WorkflowBase extends cdk.Resource implements IWorkflow {
...options,
workflowName: this.workflowName,
type: 'CONDITIONAL',
actions: options.actions?.map(this.renderAction),
actions: options.actions?.map(this.renderAction.bind(this)),
predicate: this.renderPredicate(options),
eventBatchingCondition: this.renderEventBatchingCondition(options),
description: options.description ?? undefined,
Expand Down
1 change: 1 addition & 0 deletions packages/@aws-cdk/aws-iot-alpha/test/topic-rule.test.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
/* eslint-disable @typescript-eslint/unbound-method */
import { Template } from 'aws-cdk-lib/assertions';
import * as cdk from 'aws-cdk-lib';
import * as iot from '../lib';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -857,31 +857,31 @@ describe('Application', () => {

// Table driven test with: [method, metricName, default statistic]
const assertions: Array<[(options?: cloudwatch.MetricOptions) => cloudwatch.Metric, string, string]> = [
[flinkApp.metricKpus, 'KPUs', 'Average'],
[flinkApp.metricDowntime, 'downtime', 'Average'],
[flinkApp.metricUptime, 'uptime', 'Average'],
[flinkApp.metricFullRestarts, 'fullRestarts', 'Sum'],
[flinkApp.metricNumberOfFailedCheckpoints, 'numberOfFailedCheckpoints', 'Sum'],
[flinkApp.metricLastCheckpointDuration, 'lastCheckpointDuration', 'Maximum'],
[flinkApp.metricLastCheckpointSize, 'lastCheckpointSize', 'Maximum'],
[flinkApp.metricCpuUtilization, 'cpuUtilization', 'Average'],
[flinkApp.metricHeapMemoryUtilization, 'heapMemoryUtilization', 'Average'],
[flinkApp.metricOldGenerationGCTime, 'oldGenerationGCTime', 'Sum'],
[flinkApp.metricOldGenerationGCCount, 'oldGenerationGCCount', 'Sum'],
[flinkApp.metricThreadsCount, 'threadsCount', 'Average'],
[flinkApp.metricNumRecordsIn, 'numRecordsIn', 'Average'],
[flinkApp.metricNumRecordsInPerSecond, 'numRecordsInPerSecond', 'Average'],
[flinkApp.metricNumRecordsOut, 'numRecordsOut', 'Average'],
[flinkApp.metricNumRecordsOutPerSecond, 'numRecordsOutPerSecond', 'Average'],
[flinkApp.metricNumLateRecordsDropped, 'numLateRecordsDropped', 'Sum'],
[flinkApp.metricCurrentInputWatermark, 'currentInputWatermark', 'Maximum'],
[flinkApp.metricCurrentOutputWatermark, 'currentOutputWatermark', 'Maximum'],
[flinkApp.metricManagedMemoryUsed, 'managedMemoryUsed', 'Average'],
[flinkApp.metricManagedMemoryTotal, 'managedMemoryTotal', 'Average'],
[flinkApp.metricManagedMemoryUtilization, 'managedMemoryUtilization', 'Average'],
[flinkApp.metricIdleTimeMsPerSecond, 'idleTimeMsPerSecond', 'Average'],
[flinkApp.metricBackPressuredTimeMsPerSecond, 'backPressuredTimeMsPerSecond', 'Average'],
[flinkApp.metricBusyTimePerMsPerSecond, 'busyTimePerMsPerSecond', 'Average'],
[x => flinkApp.metricKpus(x), 'KPUs', 'Average'],
[x => flinkApp.metricDowntime(x), 'downtime', 'Average'],
[x => flinkApp.metricUptime(x), 'uptime', 'Average'],
[x => flinkApp.metricFullRestarts(x), 'fullRestarts', 'Sum'],
[x => flinkApp.metricNumberOfFailedCheckpoints(x), 'numberOfFailedCheckpoints', 'Sum'],
[x => flinkApp.metricLastCheckpointDuration(x), 'lastCheckpointDuration', 'Maximum'],
[x => flinkApp.metricLastCheckpointSize(x), 'lastCheckpointSize', 'Maximum'],
[x => flinkApp.metricCpuUtilization(x), 'cpuUtilization', 'Average'],
[x => flinkApp.metricHeapMemoryUtilization(x), 'heapMemoryUtilization', 'Average'],
[x => flinkApp.metricOldGenerationGCTime(x), 'oldGenerationGCTime', 'Sum'],
[x => flinkApp.metricOldGenerationGCCount(x), 'oldGenerationGCCount', 'Sum'],
[x => flinkApp.metricThreadsCount(x), 'threadsCount', 'Average'],
[x => flinkApp.metricNumRecordsIn(x), 'numRecordsIn', 'Average'],
[x => flinkApp.metricNumRecordsInPerSecond(x), 'numRecordsInPerSecond', 'Average'],
[x => flinkApp.metricNumRecordsOut(x), 'numRecordsOut', 'Average'],
[x => flinkApp.metricNumRecordsOutPerSecond(x), 'numRecordsOutPerSecond', 'Average'],
[x => flinkApp.metricNumLateRecordsDropped(x), 'numLateRecordsDropped', 'Sum'],
[x => flinkApp.metricCurrentInputWatermark(x), 'currentInputWatermark', 'Maximum'],
[x => flinkApp.metricCurrentOutputWatermark(x), 'currentOutputWatermark', 'Maximum'],
[x => flinkApp.metricManagedMemoryUsed(x), 'managedMemoryUsed', 'Average'],
[x => flinkApp.metricManagedMemoryTotal(x), 'managedMemoryTotal', 'Average'],
[x => flinkApp.metricManagedMemoryUtilization(x), 'managedMemoryUtilization', 'Average'],
[x => flinkApp.metricIdleTimeMsPerSecond(x), 'idleTimeMsPerSecond', 'Average'],
[x => flinkApp.metricBackPressuredTimeMsPerSecond(x), 'backPressuredTimeMsPerSecond', 'Average'],
[x => flinkApp.metricBusyTimePerMsPerSecond(x), 'busyTimePerMsPerSecond', 'Average'],
];

assertions.forEach(([method, metricName, defaultStatistic]) => {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
/* eslint-disable @typescript-eslint/unbound-method */
import * as child_process from 'child_process';
import * as os from 'os';
import * as path from 'path';
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
/* eslint-disable @typescript-eslint/unbound-method */
import * as path from 'path';
import { Template } from 'aws-cdk-lib/assertions';
import { Runtime } from 'aws-cdk-lib/aws-lambda';
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
/* eslint-disable @typescript-eslint/unbound-method */
import * as fs from 'fs';
import * as path from 'path';
import { Architecture, Code, Runtime } from 'aws-cdk-lib/aws-lambda';
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
/* eslint-disable @typescript-eslint/unbound-method */
import * as path from 'path';
import { Template } from 'aws-cdk-lib/assertions';
import { Code, Runtime, Architecture } from 'aws-cdk-lib/aws-lambda';
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
/* eslint-disable @typescript-eslint/unbound-method */
import * as path from 'path';
import { Runtime } from 'aws-cdk-lib/aws-lambda';
import { DockerImage, Stack } from 'aws-cdk-lib';
Expand Down
1 change: 1 addition & 0 deletions packages/@aws-cdk/aws-pipes-alpha/test/pipe.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -469,6 +469,7 @@ describe('Pipe', () => {
expect(pipe.pipeRole).toBe(role);
expect(source.grantRead).toHaveBeenCalledWith(role);
expect(target.grantPush).toHaveBeenCalledWith(role);
// eslint-disable-next-line @typescript-eslint/unbound-method
expect(enrichment.grantInvoke).toHaveBeenCalledWith(role);
});

Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import * as child_process from 'child_process';
import * as path from 'path';
import * as fs from 'fs-extra';
import { snapshotTestWorker } from '../../lib/workers/extract';
Expand Down Expand Up @@ -33,7 +32,6 @@ describe('Snapshot tests', () => {

test('has snapshot', () => {
// WHEN
jest.spyOn(child_process, 'spawnSync').mockResolvedValue;
const test = {
fileName: path.join(directory, 'xxxxx.test-with-snapshot.js'),
discoveryRoot: directory,
Expand All @@ -46,7 +44,6 @@ describe('Snapshot tests', () => {

test('failed snapshot', () => {
// WHEN
jest.spyOn(child_process, 'spawnSync').mockRejectedValue;
const test = {
fileName: path.join(directory, 'xxxxx.test-with-snapshot-assets-diff.js'),
discoveryRoot: directory,
Expand Down
2 changes: 1 addition & 1 deletion packages/aws-cdk-lib/aws-backup/lib/plan.ts
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,7 @@ export class BackupPlan extends Resource implements IBackupPlan {
});
}

private planCopyActions(props: BackupPlanCopyActionProps): CfnBackupPlan.CopyActionResourceTypeProperty {
private planCopyActions(this: void, props: BackupPlanCopyActionProps): CfnBackupPlan.CopyActionResourceTypeProperty {
return {
destinationBackupVaultArn: props.destinationBackupVault.backupVaultArn,
lifecycle: (props.deleteAfter || props.moveToColdStorageAfter) && {
Expand Down
4 changes: 2 additions & 2 deletions packages/aws-cdk-lib/aws-cloudfront/lib/geo-restriction.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ export class GeoRestriction {
* that you want to allow. Include one element for each country.
* See ISO 3166-1-alpha-2 code on the *International Organization for Standardization* website
*/
public static allowlist(...locations: string[]) {
public static allowlist(this: void, ...locations: string[]) {
return new GeoRestriction('whitelist', GeoRestriction.validateLocations(locations));
}

Expand All @@ -22,7 +22,7 @@ export class GeoRestriction {
* that you want to deny. Include one element for each country.
* See ISO 3166-1-alpha-2 code on the *International Organization for Standardization* website
*/
public static denylist(...locations: string[]) {
public static denylist(this: void, ...locations: string[]) {
return new GeoRestriction('blacklist', GeoRestriction.validateLocations(locations));
}

Expand Down
1 change: 1 addition & 0 deletions packages/aws-cdk-lib/aws-config/lib/rule.ts
Original file line number Diff line number Diff line change
Expand Up @@ -449,6 +449,7 @@ export class CustomRule extends RuleNew {
}
const hash = createHash('sha256')
.update(JSON.stringify({
/* eslint-disable-next-line @typescript-eslint/unbound-method *//* REMOVEME: this is a latent bug */
fnName: props.lambdaFunction.functionName.toString,
accountId: Stack.of(this).resolve(this.env.account),
region: Stack.of(this).resolve(this.env.region),
Expand Down
12 changes: 6 additions & 6 deletions packages/aws-cdk-lib/aws-ec2/lib/cfn-init-elements.ts
Original file line number Diff line number Diff line change
Expand Up @@ -655,42 +655,42 @@ export class InitPackage extends InitElement {
/**
* Install an RPM from an HTTP URL or a location on disk
*/
public static rpm(location: string, options: LocationPackageOptions = {}): InitPackage {
public static rpm(this: void, location: string, options: LocationPackageOptions = {}): InitPackage {
return new InitPackage('rpm', [location], options.key, options.serviceRestartHandles);
}

/**
* Install a package using Yum
*/
public static yum(packageName: string, options: NamedPackageOptions = {}): InitPackage {
public static yum(this: void, packageName: string, options: NamedPackageOptions = {}): InitPackage {
return new InitPackage('yum', options.version ?? [], packageName, options.serviceRestartHandles);
}

/**
* Install a package from RubyGems
*/
public static rubyGem(gemName: string, options: NamedPackageOptions = {}): InitPackage {
public static rubyGem(this: void, gemName: string, options: NamedPackageOptions = {}): InitPackage {
return new InitPackage('rubygems', options.version ?? [], gemName, options.serviceRestartHandles);
}

/**
* Install a package from PyPI
*/
public static python(packageName: string, options: NamedPackageOptions = {}): InitPackage {
public static python(this: void, packageName: string, options: NamedPackageOptions = {}): InitPackage {
return new InitPackage('python', options.version ?? [], packageName, options.serviceRestartHandles);
}

/**
* Install a package using APT
*/
public static apt(packageName: string, options: NamedPackageOptions = {}): InitPackage {
public static apt(this: void, packageName: string, options: NamedPackageOptions = {}): InitPackage {
return new InitPackage('apt', options.version ?? [], packageName, options.serviceRestartHandles);
}

/**
* Install an MSI package from an HTTP URL or a location on disk
*/
public static msi(location: string, options: LocationPackageOptions = {}): InitPackage {
public static msi(this: void, location: string, options: LocationPackageOptions = {}): InitPackage {
// The MSI package version must be a string, not an array.
return new class extends InitPackage {
protected renderPackageVersions() { return location; }
Expand Down
6 changes: 3 additions & 3 deletions packages/aws-cdk-lib/aws-ec2/test/ip-addresses.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,12 @@ describe('Cidr vpc allocation', () => {

test('Default Cidr returns ipv4IpamPoolId as undefined', () => {
const ipAddresses = IpAddresses.cidr('10.0.0.0/16');
expect(ipAddresses.allocateVpcCidr().ipv4IpamPoolId).toBeUndefined;
expect(ipAddresses.allocateVpcCidr().ipv4IpamPoolId).toBeUndefined();
});

test('Default Cidr returns ipv4NetmaskLength as undefined', () => {
const ipAddresses = IpAddresses.cidr('10.0.0.0/16');
expect(ipAddresses.allocateVpcCidr().ipv4NetmaskLength).toBeUndefined;
expect(ipAddresses.allocateVpcCidr().ipv4NetmaskLength).toBeUndefined();
});
});

Expand Down Expand Up @@ -102,7 +102,7 @@ describe('AwsIpam vpc allocation', () => {

test('AwsIpam returns cidrBlock as undefined', () => {
const ipAddresses = IpAddresses.awsIpamAllocation(awsIpamProps);
expect(ipAddresses.allocateVpcCidr().cidrBlock).toBeUndefined;
expect(ipAddresses.allocateVpcCidr().cidrBlock).toBeUndefined();
});

test('AwsIpam returns the correct vpc ipv4IpamPoolId', () => {
Expand Down
1 change: 1 addition & 0 deletions packages/aws-cdk-lib/aws-ec2/test/vpc.from-lookup.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -380,6 +380,7 @@ interface MockVpcContextResponse {
function mockVpcContextProviderWith(
response: MockVpcContextResponse,
paramValidator?: (options: cxschema.VpcContextQuery) => void) {
// eslint-disable-next-line @typescript-eslint/unbound-method
const previous = ContextProvider.getValue;
ContextProvider.getValue = (_scope: Construct, options: GetContextValueOptions) => {
// do some basic sanity checks
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -710,7 +710,7 @@ describe('ApplicationLoadBalancedFargateService', () => {
// THEN
expect(() => {
service.internalDesiredCount;
}).toBeTruthy;
}).toBeTruthy();
});

test('multiple capacity provider strategies are set', () => {
Expand Down
1 change: 1 addition & 0 deletions packages/aws-cdk-lib/aws-ecs/test/cluster.test.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
/* eslint-disable @typescript-eslint/unbound-method */
import { testDeprecated } from '@aws-cdk/cdk-build-tools';
import { Annotations, Match, Template } from '../../assertions';
import * as autoscaling from '../../aws-autoscaling';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -352,7 +352,7 @@ describe('fargate task definition', () => {
// THEN
expect(() => {
taskDefinition.ephemeralStorageGiB;
}).toBeTruthy;
}).toBeTruthy();
});

test('runtime testing for windows container', () => {
Expand Down
2 changes: 1 addition & 1 deletion packages/aws-cdk-lib/aws-eks/lib/managed-nodegroup.ts
Original file line number Diff line number Diff line change
Expand Up @@ -646,7 +646,7 @@ function getPossibleAmiTypes(instanceTypes: InstanceType[]): NodegroupAmiType[]
const architectures: Set<AmiArchitecture> = new Set(instanceTypes.map(typeToArch));

if (architectures.size === 0) { // protective code, the current implementation will never result in this.
throw new Error(`Cannot determine any ami type compatible with instance types: ${instanceTypes.map(i => i.toString).join(', ')}`);
throw new Error(`Cannot determine any ami type compatible with instance types: ${instanceTypes.map(i => i.toString()).join(', ')}`);
}

if (architectures.size > 1) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,14 @@ export class ListenerCertificate implements IListenerCertificate {
/**
* Use an ACM certificate as a listener certificate
*/
public static fromCertificateManager(acmCertificate: acm.ICertificate) {
public static fromCertificateManager(this: void, acmCertificate: acm.ICertificate) {
return new ListenerCertificate(acmCertificate.certificateArn);
}

/**
* Use any certificate, identified by its ARN, as a listener certificate
*/
public static fromArn(certificateArn: string) {
public static fromArn(this: void, certificateArn: string) {
return new ListenerCertificate(certificateArn);
}

Expand Down
4 changes: 2 additions & 2 deletions packages/aws-cdk-lib/aws-iam/lib/principals.ts
Original file line number Diff line number Diff line change
Expand Up @@ -92,14 +92,14 @@ export class ComparablePrincipal {
/**
* Whether or not the given principal is a comparable principal
*/
public static isComparablePrincipal(x: IPrincipal): x is IComparablePrincipal {
public static isComparablePrincipal(this: void, x: IPrincipal): x is IComparablePrincipal {
return 'dedupeString' in x;
}

/**
* Return the dedupeString of the given principal, if available
*/
public static dedupeStringFor(x: IPrincipal): string | undefined {
public static dedupeStringFor(this: void, x: IPrincipal): string | undefined {
return ComparablePrincipal.isComparablePrincipal(x) ? x.dedupeString() : undefined;
}
}
Expand Down
Loading

0 comments on commit f44209e

Please sign in to comment.