Skip to content

Commit

Permalink
fix(awslint): linting is slow (#27860)
Browse files Browse the repository at this point in the history
Reduces runtime of `awslint` against `aws-cdk-lib` from ~70s down to ~15s.

Speed up 1: Reduce rule definitions (~1s)
Speed up 2: Make core checks fqn based only (~5s)
Speed up 3: Optimize code paths to defer expensive checks (~4s)
Speed up 4: Locked typesystem (~25s)
Speed up 5: Faster camel casing (~15s)


![giphy](https://github.com/aws/aws-cdk/assets/379814/4a8eb1dd-d045-48a2-8a4d-0a1e36db8b3b)


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
mrgrain committed Nov 17, 2023
1 parent 4713547 commit 0607b2c
Show file tree
Hide file tree
Showing 13 changed files with 278 additions and 2,107 deletions.
2,095 changes: 50 additions & 2,045 deletions packages/aws-cdk-lib/awslint.json

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion packages/awslint/.eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ module.exports = {
parserOptions: {
ecmaVersion: '2018',
sourceType: 'module',
project: './tsconfig.json',
project: __dirname + '/tsconfig.json',
},
extends: [
'plugin:import/typescript',
Expand Down
10 changes: 7 additions & 3 deletions packages/awslint/bin/awslint.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,8 @@ async function main() {

if (args._.length > 1) {
argv.showHelp();
process.exit(1);
process.exitCode = 1;
return;
}

const command = args._[0] || 'lint';
Expand Down Expand Up @@ -204,7 +205,7 @@ async function main() {
}

if (errors && !args.save) {
process.exit(1);
process.exitCode = 1;
}

return;
Expand Down Expand Up @@ -241,14 +242,17 @@ main().catch(e => {
if (stackTrace) {
console.error(e.stack);
}
process.exit(1);
process.exitCode = 1;
});

async function loadModule(dir: string) {
const ts = new reflect.TypeSystem();
await ts.load(dir, { validate: false }); // Don't validate to save 66% of execution time (20s vs 1min).
// We run 'awslint' during build time, assemblies are guaranteed to be ok.

// We won't load any more assemblies. Lock the typesystem to benefit from performance improvements.
ts.lock();

if (ts.roots.length !== 1) {
throw new Error('Expecting only a single root assembly');
}
Expand Down
18 changes: 18 additions & 0 deletions packages/awslint/lib/case.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import * as changeCase from 'change-case';

function cachedTransform(transform: (x: string) => string): (x: string) => string {
const CACHE = new Map<string, string>();
return (x) => {
const prev = CACHE.get(x);
if (prev) {
return prev;
}

const transformed = transform(x);
CACHE.set(x, transformed);
return transformed;
};
}

export const camelize = cachedTransform((x: string) => changeCase.camelCase(x));
export const pascalize = cachedTransform((x: string) => changeCase.pascalCase(x));
2 changes: 1 addition & 1 deletion packages/awslint/lib/rules/api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ apiLinter.add({
return;
}

if (type.type.fqn === e.ctx.core.constructClass.fqn) {
if (type.type.fqn === e.ctx.core.baseConstructClassFqn) {
return;
}

Expand Down
4 changes: 2 additions & 2 deletions packages/awslint/lib/rules/cfn-resource.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import * as camelcase from 'camelcase';
import * as reflect from 'jsii-reflect';
import { CoreTypes } from './core-types';
import { ResourceReflection } from './resource';
import { pascalize } from '../case';
import { Linter } from '../linter';

const cfnResourceTagName = 'cloudformationResource';
Expand Down Expand Up @@ -100,6 +100,6 @@ export class CfnResourceReflection {
return 'Id';
}

return camelcase(name, { pascalCase: true });
return pascalize(name);
}
}
19 changes: 3 additions & 16 deletions packages/awslint/lib/rules/construct.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,6 @@ export class ConstructReflection {
return typeRef.fqn;
}

/**
* @deprecated - use `CoreTypes.constructClass()` or `CoreTypes.baseConstructClass()` as appropriate
*/
public readonly ROOT_CLASS: reflect.ClassType; // constructs.Construct

public readonly fqn: string;
public readonly interfaceFqn: string;
public readonly propsFqn: string;
Expand All @@ -43,7 +38,6 @@ export class ConstructReflection {
this.fqn = classType.fqn;
this.sys = classType.system;
this.core = new CoreTypes(this.sys);
this.ROOT_CLASS = this.sys.findClass(this.core.constructClass.fqn);
this.interfaceFqn = `${this.typePrefix(classType)}.I${classType.name}`;
this.propsFqn = `${this.typePrefix(classType)}.${classType.name}Props`;
this.interfaceType = this.tryFindInterface();
Expand Down Expand Up @@ -99,16 +93,9 @@ constructLinter.add({
}

const expectedParams = new Array<MethodSignatureParameterExpectation>();

let baseType;
if (process.env.AWSLINT_BASE_CONSTRUCT && !CoreTypes.isCfnResource(e.ctx.classType)) {
baseType = e.ctx.core.baseConstructClass;
} else {
baseType = e.ctx.core.constructClass;
}
expectedParams.push({
name: 'scope',
type: baseType.fqn,
type: e.ctx.core.baseConstructClassFqn,
});

expectedParams.push({
Expand Down Expand Up @@ -184,7 +171,7 @@ constructLinter.add({
message: 'construct interface must extend core.IConstruct',
eval: e => {
if (!e.ctx.interfaceType) { return; }
const interfaceBase = e.ctx.sys.findInterface(e.ctx.core.constructInterface.fqn);
const interfaceBase = e.ctx.sys.findInterface(e.ctx.core.baseConstructInterfaceFqn);
e.assert(e.ctx.interfaceType.extends(interfaceBase), e.ctx.interfaceType.fqn);
},
});
Expand Down Expand Up @@ -247,7 +234,7 @@ constructLinter.add({

const found = (fqn && e.ctx.sys.tryFindFqn(fqn));
if (found) {
e.assert(!(fqn === e.ctx.core.tokenInterface.fqn), `${e.ctx.propsFqn}.${property.name}`);
e.assert(!(fqn === e.ctx.core.tokenInterfaceFqn), `${e.ctx.propsFqn}.${property.name}`);
}
}
},
Expand Down
59 changes: 44 additions & 15 deletions packages/awslint/lib/rules/core-types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -100,50 +100,85 @@ export class CoreTypes {
* @deprecated - use `baseConstructClass()`
*/
public get constructClass() {
return this.sys.findClass(CoreTypesFqn.Construct);
return this.baseConstructClass;
}

/**
* @returns `classType` for the core type Construct
*/
public get baseConstructClass() {
return this.sys.findClass(CoreTypesFqn.Construct);
return this.sys.findClass(this.baseConstructClassFqn);
}

/**
* @returns `classType` for the core type Construct
*/
public get baseConstructClassFqn() {
return CoreTypesFqn.Construct;
}

/**
* @returns `interfacetype` for the core type Construct
* @deprecated - use `baseConstructInterface()`
*/
public get constructInterface() {
return this.sys.findInterface(CoreTypesFqn.ConstructInterface);
return this.baseConstructInterface;
}

/**
* @returns `interfacetype` for the core type Construct
*/
public get baseConstructInterface() {
return this.sys.findInterface(CoreTypesFqn.ConstructInterface);
return this.sys.findInterface(this.baseConstructInterfaceFqn);
}

/**
* @returns `classType` for the core type Construct
* @returns fqn for for the core Construct interface
*/
public get baseConstructInterfaceFqn() {
return CoreTypesFqn.ConstructInterface;
}

/**
* @returns `classType` for the core type Resource
*/
public get resourceClass() {
return this.sys.findClass(CoreTypesFqn.Resource);
return this.sys.findClass(this.resourceClassFqn);
}

/**
* @returns `interfaceType` for the core type Resource
* @returns fqn for the core type Resource
*/
public get resourceClassFqn() {
return CoreTypesFqn.Resource;
}

/**
* @returns fqn for the core Resource interface
*/
public get resourceInterface() {
return this.sys.findInterface(CoreTypesFqn.ResourceInterface);
return this.sys.findInterface(this.resourceInterfaceFqn);
}

/**
* @returns `interfaceType` for the core type Resource
*/
public get resourceInterfaceFqn() {
return CoreTypesFqn.ResourceInterface;
}

/**
* @returns `classType` for the core type Token
*/
public get tokenInterface() {
return this.sys.findInterface(CoreTypesFqn.ResolvableInterface);
return this.sys.findInterface(this.tokenInterfaceFqn);
}

/**
* @returns fqn for the core type Token
*/
public get tokenInterfaceFqn() {
return CoreTypesFqn.ResolvableInterface;
}

public get physicalNameClass() {
Expand All @@ -158,11 +193,5 @@ export class CoreTypes {
// disable-all-checks
return;
}

for (const fqn of Object.values(CoreTypesFqn)) {
if (!this.sys.tryFindFqn(fqn)) {
throw new Error(`core FQN type not found: ${fqn}`);
}
}
}
}
6 changes: 2 additions & 4 deletions packages/awslint/lib/rules/imports.ts
Original file line number Diff line number Diff line change
Expand Up @@ -70,8 +70,7 @@ importsLinter.add({
// "fromRoleArn" => "roleArn"
const argName = e.ctx.resource.basename[0].toLocaleLowerCase() + method.name.slice('from'.length + 1);

const baseType = process.env.AWSLINT_BASE_CONSTRUCT ? e.ctx.resource.core.baseConstructClass :
e.ctx.resource.core.constructClass;
const baseType = e.ctx.resource.core.baseConstructClass;
e.assertSignature(method, {
parameters: [
{ name: 'scope', type: baseType },
Expand All @@ -92,8 +91,7 @@ importsLinter.add({
return;
}

const baseType = process.env.AWSLINT_BASE_CONSTRUCT ? e.ctx.resource.core.baseConstructClass
: e.ctx.resource.core.constructClass;
const baseType = e.ctx.resource.core.baseConstructClass;
e.assertSignature(e.ctx.fromAttributesMethod, {
parameters: [
{ name: 'scope', type: baseType },
Expand Down
21 changes: 10 additions & 11 deletions packages/awslint/lib/rules/resource.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import * as camelcase from 'camelcase';
import * as reflect from 'jsii-reflect';
import { CfnResourceReflection } from './cfn-resource';
import { ConstructReflection } from './construct';
import { CoreTypes } from './core-types';
import { getDocTag } from './util';
import { camelize, pascalize } from '../case';
import { Linter } from '../linter';

const GRANT_RESULT_FQN = '@aws-cdk/aws-iam.Grant';
Expand Down Expand Up @@ -31,10 +31,9 @@ export class ResourceReflection {
return []; // not part of the dep stack
}

return ConstructReflection
.findAllConstructs(assembly)
.filter(c => CoreTypes.isResourceClass(c.classType))
.map(c => new ResourceReflection(c));
return assembly.allClasses
.filter(c => CoreTypes.isConstructClass(c) && CoreTypes.isResourceClass(c))
.map(c => new ResourceReflection(new ConstructReflection(c)));
}

public readonly attributes: Attribute[]; // actual attribute props
Expand Down Expand Up @@ -71,7 +70,7 @@ export class ResourceReflection {
return undefined;
}

const resourceName = camelcase(this.cfn.basename);
const resourceName = camelize(this.cfn.basename);

// if resource name ends with "Name" (e.g. DomainName, then just use it as-is, otherwise append "Name")
const physicalNameProp = resourceName.endsWith('Name') ? resourceName : `${resourceName}Name`;
Expand All @@ -89,7 +88,7 @@ export class ResourceReflection {
continue; // skip any protected properties
}

const basename = camelcase(this.cfn.basename);
const basename = camelize(this.cfn.basename);

// an attribute property is a property which starts with the type name
// (e.g. "bucketXxx") and/or has an @attribute doc tag.
Expand All @@ -108,7 +107,7 @@ export class ResourceReflection {
// okay, we don't have an explicit CFN attribute name, so we'll guess it
// from the name of the property.

const name = camelcase(p.name, { pascalCase: true });
const name = pascalize(p.name);
if (this.cfn.attributeNames.includes(name)) {
// special case: there is a cloudformation resource type in the attribute name
// for example 'RoleId'.
Expand Down Expand Up @@ -158,7 +157,7 @@ resourceLinter.add({
code: 'resource-class-extends-resource',
message: 'resource classes must extend "cdk.Resource" directly or indirectly',
eval: e => {
const resourceBase = e.ctx.sys.findClass(e.ctx.core.resourceClass.fqn);
const resourceBase = e.ctx.sys.findClass(e.ctx.core.resourceClassFqn);
e.assert(e.ctx.construct.classType.extends(resourceBase), e.ctx.construct.fqn);
},
});
Expand All @@ -179,7 +178,7 @@ resourceLinter.add({
const resourceInterface = e.ctx.construct.interfaceType;
if (!resourceInterface) { return; }

const resourceInterfaceFqn = e.ctx.core.resourceInterface.fqn;
const resourceInterfaceFqn = e.ctx.core.resourceInterfaceFqn;
const interfaceBase = e.ctx.sys.findInterface(resourceInterfaceFqn);
e.assert(resourceInterface.extends(interfaceBase), resourceInterface.fqn);
},
Expand Down Expand Up @@ -266,7 +265,7 @@ function tryResolveCfnResource(resourceClass: reflect.ClassType): CfnResourceRef
}

// try to resolve through ancestors
for (const base of resourceClass.getAncestors()) {
for (const base of resourceClass.ancestors) {
const ret = tryResolveCfnResource(base);
if (ret) {
return ret;
Expand Down
7 changes: 4 additions & 3 deletions packages/awslint/lib/rules/util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,10 @@ export function getDocTag(documentable: reflect.Documentable, tag: string): stri
const t = documentable.docs.customTag(tag);
if (t) { return t; }

if ((documentable instanceof reflect.Property || documentable instanceof reflect.Method) && documentable.overrides) {
if (documentable.overrides.isClassType() || documentable.overrides.isInterfaceType()) {
const baseMembers = documentable.overrides.allMembers.filter(m => m.name === documentable.name);
if ((documentable instanceof reflect.Property || documentable instanceof reflect.Method)) {
const overrides = documentable.overrides;
if (overrides?.isClassType() || overrides?.isInterfaceType()) {
const baseMembers = overrides.allMembers.filter(m => m.name === documentable.name);
for (const base of baseMembers) {
const baseTag = getDocTag(base, tag);
if (baseTag) {
Expand Down

0 comments on commit 0607b2c

Please sign in to comment.