Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: add eslint rule to forbid instanceof #20564

Closed
wants to merge 3 commits into from
Closed
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
2 changes: 1 addition & 1 deletion packages/@aws-cdk/core/lib/asset-staging.ts
Original file line number Diff line number Diff line change
Expand Up @@ -555,7 +555,7 @@ function calculateCacheKey<A extends object>(props: A): string {
* Recursively sort object keys
*/
function sortObject(object: { [key: string]: any }): { [key: string]: any } {
if (typeof object !== 'object' || object instanceof Array) {
if (typeof object !== 'object' || Array.isArray(object)) {
return object;
}
const ret: { [key: string]: any } = {};
Expand Down
1 change: 1 addition & 0 deletions packages/@aws-cdk/core/lib/private/cloudformation-lang.ts
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,7 @@ function tokenAwareStringify(root: any, space: number, ctx: IResolveContext) {
if (Array.isArray(obj)) {
return renderCollection('[', ']', obj, recurse);
}
// eslint-disable-next-line no-instanceof/no-instanceof
if (typeof obj === 'object' && obj != null && !(obj instanceof Date)) {
// Treat as an intrinsic if this LOOKS like a CFN intrinsic (`{ Ref: ... }`)
// AND it's the result of a token resolution. Otherwise, we just treat this
Expand Down
1 change: 1 addition & 0 deletions packages/@aws-cdk/core/lib/private/resolve.ts
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,7 @@ export function resolve(obj: any, options: IResolveOptions): any {
// primitives - as-is
//

// eslint-disable-next-line no-instanceof/no-instanceof
if (typeof(obj) !== 'object' || obj instanceof Date) {
return obj;
}
Expand Down
2 changes: 1 addition & 1 deletion packages/@aws-cdk/core/lib/private/synthesis.ts
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,7 @@ function synthesizeTree(root: IConstruct, builder: cxapi.CloudAssemblyBuilder, v

if (Stack.isStack(construct)) {
construct.synthesizer.synthesize(session);
} else if (construct instanceof TreeMetadata) {
} else if (TreeMetadata.isTreeMetadata(construct)) {
construct._synthesizeTree(session);
} else {
const custom = getCustomSynthesis(construct);
Expand Down
18 changes: 18 additions & 0 deletions packages/@aws-cdk/core/lib/private/tree-metadata.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,22 @@ import { ConstructInfo, constructInfoFromConstruct } from './runtime-info';

const FILE_PATH = 'tree.json';

const TREE_METADATA_SYMBOL = Symbol.for('@aws-cdk/core.TreeMetadata');

/**
* Construct that is automatically attached to the top-level `App`.
* This generates, as part of synthesis, a file containing the construct tree and the metadata for each node in the tree.
* The output is in a tree format so as to preserve the construct hierarchy.
*
*/
export class TreeMetadata extends Construct {
/**
* Return whether the given object is a TreeMetadata.
*/
public static isTreeMetadata(x: any): x is TreeMetadata {
return x !== null && typeof(x) === 'object' && TREE_METADATA_SYMBOL in x;
}

constructor(scope: Construct) {
super(scope, 'Tree');
}
Expand Down Expand Up @@ -98,3 +107,12 @@ interface Node {
*/
readonly constructInfo?: ConstructInfo;
}

/**
* Mark all instances of 'TreeMetadata'.
*/
Object.defineProperty(TreeMetadata.prototype, TREE_METADATA_SYMBOL, {
value: true,
enumerable: false,
writable: false,
});
1 change: 1 addition & 0 deletions packages/@aws-cdk/core/lib/runtime.ts
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,7 @@ export function validateBoolean(x: any): ValidationResult {
}

export function validateDate(x: any): ValidationResult {
// eslint-disable-next-line no-instanceof/no-instanceof
if (canInspect(x) && !(x instanceof Date)) {
return new ValidationResult(`${JSON.stringify(x)} should be a Date`);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -239,7 +239,7 @@ function evalCFN(value: any) {
}

function isAssetManifest(x: cxapi.CloudArtifact): x is cxapi.AssetManifestArtifact {
return x instanceof cxapi.AssetManifestArtifact;
return cxapi.AssetManifestArtifact.isAssetManifestArtifact(x);
}

function getAssetManifest(asm: cxapi.CloudAssembly): cxapi.AssetManifestArtifact {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -393,7 +393,7 @@ function evalCFN(value: any) {
}

function isAssetManifest(x: cxapi.CloudArtifact): x is cxapi.AssetManifestArtifact {
return x instanceof cxapi.AssetManifestArtifact;
return cxapi.AssetManifestArtifact.isAssetManifestArtifact(x);
}

function getAssetManifest(asm: cxapi.CloudAssembly): cxapi.AssetManifestArtifact {
Expand Down
2 changes: 1 addition & 1 deletion packages/@aws-cdk/core/test/stage.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ describe('stage', () => {
// THEN -- app manifest contains a nested cloud assembly
const appAsm = app.synth();

const artifact = appAsm.artifacts.find(x => x instanceof cxapi.NestedCloudAssemblyArtifact);
const artifact = appAsm.artifacts.find(x => cxapi.NestedCloudAssemblyArtifact.isNestedCloudAssemblyArtifact(x));
expect(artifact).toBeDefined();


Expand Down
18 changes: 18 additions & 0 deletions packages/@aws-cdk/cx-api/lib/artifacts/cloudformation-artifact.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,16 @@ import { CloudArtifact } from '../cloud-artifact';
import type { CloudAssembly } from '../cloud-assembly';
import { Environment, EnvironmentUtils } from '../environment';

const CLOUDFORMATION_STACK_ARTIFACT_SYMBOL = Symbol.for('@aws-cdk/cx-api.CloudFormationStackArtifact');

export class CloudFormationStackArtifact extends CloudArtifact {
/**
* Return whether the given object is a CloudFormationStackArtifact.
*/
public static isCloudFormationStackArtifact(x: any): x is CloudFormationStackArtifact {
return x !== null && typeof(x) === 'object' && CLOUDFORMATION_STACK_ARTIFACT_SYMBOL in x;
}

/**
* The file name of the template.
*/
Expand Down Expand Up @@ -183,3 +192,12 @@ export class CloudFormationStackArtifact extends CloudArtifact {
return ret;
}
}

/**
* Mark all instances of 'CloudFormationStackArtifact'.
*/
Object.defineProperty(CloudFormationStackArtifact.prototype, CLOUDFORMATION_STACK_ARTIFACT_SYMBOL, {
value: true,
enumerable: false,
writable: false,
});
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,19 @@ import * as cxschema from '@aws-cdk/cloud-assembly-schema';
import { CloudArtifact } from '../cloud-artifact';
import type { CloudAssembly } from '../cloud-assembly';

const NESTED_CLOUD_ASSEMBLY_ARTIFACT_SYMBOL = Symbol.for('@aws-cdk/cx-api.NestedCloudAssemblyArtifact');

/**
* Asset manifest is a description of a set of assets which need to be built and published
*/
export class NestedCloudAssemblyArtifact extends CloudArtifact {
/**
* Return whether the given object is a NestedCloudAssemblyArtifact.
*/
public static isNestedCloudAssemblyArtifact(x: any): x is NestedCloudAssemblyArtifact {
return x !== null && typeof(x) === 'object' && NESTED_CLOUD_ASSEMBLY_ARTIFACT_SYMBOL in x;
}

/**
* The relative directory name of the asset manifest
*/
Expand Down Expand Up @@ -40,4 +49,13 @@ export interface NestedCloudAssemblyArtifact {
readonly nestedAssembly: CloudAssembly;

// Declared in a different file
}
}

/**
* Mark all instances of 'NestedCloudAssemblyArtifact'.
*/
Object.defineProperty(NestedCloudAssemblyArtifact.prototype, NESTED_CLOUD_ASSEMBLY_ARTIFACT_SYMBOL, {
value: true,
enumerable: false,
writable: false,
});
20 changes: 19 additions & 1 deletion packages/@aws-cdk/cx-api/lib/artifacts/tree-cloud-artifact.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,16 @@ import * as cxschema from '@aws-cdk/cloud-assembly-schema';
import { CloudArtifact } from '../cloud-artifact';
import { CloudAssembly } from '../cloud-assembly';

const TREE_CLOUD_ARTIFACT_SYMBOL = Symbol.for('@aws-cdk/cx-api.TreeCloudArtifact');

export class TreeCloudArtifact extends CloudArtifact {
/**
* Return whether the given object is a TreeCloudArtifact.
*/
public static isTreeCloudArtifact(x: any): x is TreeCloudArtifact {
return x !== null && typeof(x) === 'object' && TREE_CLOUD_ARTIFACT_SYMBOL in x;
}

public readonly file: string;

constructor(assembly: CloudAssembly, name: string, artifact: cxschema.ArtifactManifest) {
Expand All @@ -14,4 +23,13 @@ export class TreeCloudArtifact extends CloudArtifact {
}
this.file = properties.file;
}
}
}

/**
* Mark all instances of 'TreeCloudArtifact'.
*/
Object.defineProperty(TreeCloudArtifact.prototype, TREE_CLOUD_ARTIFACT_SYMBOL, {
value: true,
enumerable: false,
writable: false,
});
20 changes: 6 additions & 14 deletions packages/@aws-cdk/cx-api/lib/cloud-assembly.ts
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ export class CloudAssembly {
* @returns a `CloudFormationStackArtifact` object.
*/
public getStackByName(stackName: string): CloudFormationStackArtifact {
const artifacts = this.artifacts.filter(a => a instanceof CloudFormationStackArtifact && a.stackName === stackName);
const artifacts = this.artifacts.filter(a => CloudFormationStackArtifact.isCloudFormationStackArtifact(a) && a.stackName === stackName);
if (!artifacts || artifacts.length === 0) {
throw new Error(`Unable to find stack with stack name "${stackName}"`);
}
Expand Down Expand Up @@ -115,7 +115,7 @@ export class CloudAssembly {
throw new Error(`Unable to find artifact with id "${artifactId}"`);
}

if (!(artifact instanceof CloudFormationStackArtifact)) {
if (!CloudFormationStackArtifact.isCloudFormationStackArtifact(artifact)) {
throw new Error(`Artifact ${artifactId} is not a CloudFormation stack`);
}

Expand Down Expand Up @@ -154,7 +154,7 @@ export class CloudAssembly {
throw new Error(`Unable to find artifact with id "${artifactId}"`);
}

if (!(artifact instanceof NestedCloudAssemblyArtifact)) {
if (!NestedCloudAssemblyArtifact.isNestedCloudAssemblyArtifact(artifact)) {
throw new Error(`Found artifact '${artifactId}' but it's not a nested cloud assembly`);
}

Expand Down Expand Up @@ -184,7 +184,7 @@ export class CloudAssembly {
}
const tree = trees[0];

if (!(tree instanceof TreeCloudArtifact)) {
if (!TreeCloudArtifact.isTreeCloudArtifact(tree)) {
throw new Error('"Tree" artifact is not of expected type');
}

Expand All @@ -195,22 +195,14 @@ export class CloudAssembly {
* @returns all the CloudFormation stack artifacts that are included in this assembly.
*/
public get stacks(): CloudFormationStackArtifact[] {
return this.artifacts.filter(isCloudFormationStackArtifact);

function isCloudFormationStackArtifact(x: any): x is CloudFormationStackArtifact {
return x instanceof CloudFormationStackArtifact;
}
return this.artifacts.filter(CloudFormationStackArtifact.isCloudFormationStackArtifact);
}

/**
* The nested assembly artifacts in this assembly
*/
public get nestedAssemblies(): NestedCloudAssemblyArtifact[] {
return this.artifacts.filter(isNestedCloudAssemblyArtifact);

function isNestedCloudAssemblyArtifact(x: any): x is NestedCloudAssemblyArtifact {
return x instanceof NestedCloudAssemblyArtifact;
}
return this.artifacts.filter(NestedCloudAssemblyArtifact.isNestedCloudAssemblyArtifact);
}

private validateDeps() {
Expand Down
5 changes: 5 additions & 0 deletions tools/@aws-cdk/cdk-build-tools/config/eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ module.exports = {
'import',
'@aws-cdk',
'jest',
'no-instanceof',
],
parser: '@typescript-eslint/parser',
parserOptions: {
Expand Down Expand Up @@ -199,6 +200,10 @@ module.exports = {
],
}],

// 'instanceof' must be avoided, it cannot be made to work properly in the face of symlinks and transitive dependencies
// (it may only be used for built-ins and classes that are guaranteed to not escape the scope)
'no-instanceof/no-instanceof': 'error',

// Overrides for plugin:jest/recommended
"jest/expect-expect": "off",
"jest/no-conditional-expect": "off",
Expand Down
1 change: 1 addition & 0 deletions tools/@aws-cdk/cdk-build-tools/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
"@aws-cdk/node-bundle": "0.0.0",
"@typescript-eslint/eslint-plugin": "^4.33.0",
"@typescript-eslint/parser": "^4.33.0",
"eslint-plugin-no-instanceof": "^1.0.1",
"awslint": "0.0.0",
"chalk": "^4",
"eslint": "^7.32.0",
Expand Down
2 changes: 1 addition & 1 deletion tools/@aws-cdk/cfn2ts/lib/genspec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ export class Attribute {
* Example: AWS::EC2 -> ec2
*/
export function packageName(module: SpecName | string): string {
if (module instanceof SpecName) {
if (SpecName.isSpecName(module)) {
module = module.module;
}

Expand Down
19 changes: 19 additions & 0 deletions tools/@aws-cdk/cfn2ts/lib/spec-utils.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,21 @@
import { schema } from '@aws-cdk/cfnspec';
import { joinIf } from './util';

const SPEC_NAME_SYMBOL = Symbol.for('@aws-cdk/cfn2ts.SpecName');

/**
* Name of an object in the CloudFormation spec
*
* This refers to a Resource, parsed from a string like 'AWS::S3::Bucket'.
*/
export class SpecName {
/**
* Return whether the given object is a SpecName.
*/
public static isSpecName(x: any): x is SpecName {
return x !== null && typeof(x) === 'object' && SPEC_NAME_SYMBOL in x;
}

/**
* Parse a string representing a name from the CloudFormation spec to a CfnName object
*/
Expand Down Expand Up @@ -36,6 +45,7 @@ export class SpecName {
}
}


/**
* Name of a property type or attribute in the CloudFormation spec.
*
Expand Down Expand Up @@ -121,3 +131,12 @@ function primitiveScalarTypeNames(spec: schema.ScalarProperty): string[] {
}
return [];
}

/**
* Mark all instances of 'SpecName'.
*/
Object.defineProperty(SpecName.prototype, SPEC_NAME_SYMBOL, {
value: true,
enumerable: false,
writable: false,
});
5 changes: 5 additions & 0 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -4538,6 +4538,11 @@ eslint-plugin-jest@^24.7.0:
dependencies:
"@typescript-eslint/experimental-utils" "^4.0.1"

eslint-plugin-no-instanceof@^1.0.1:
version "1.0.1"
resolved "https://registry.npmjs.org/eslint-plugin-no-instanceof/-/eslint-plugin-no-instanceof-1.0.1.tgz#5d9fc86d160df6991b654b294a62390207f1bb97"
integrity sha512-zlqQ7EsfzbRO68uI+p8FIE7zYB4njs+nNbkNjSb5QmLi2et67zQLqSeaao5U9SpnlZTTJC87nS2oyHo2ACtajw==

eslint-plugin-node@^11.1.0:
version "11.1.0"
resolved "https://registry.npmjs.org/eslint-plugin-node/-/eslint-plugin-node-11.1.0.tgz#c95544416ee4ada26740a30474eefc5402dc671d"
Expand Down