-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
fix(awslint): linting is slow #27860
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -50,7 +50,8 @@ async function main() { | |
|
||
if (args._.length > 1) { | ||
argv.showHelp(); | ||
process.exit(1); | ||
process.exitCode = 1; | ||
return; | ||
Comment on lines
+53
to
+54
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unrelated change to use best practice. |
||
} | ||
|
||
const command = args._[0] || 'lint'; | ||
|
@@ -204,7 +205,7 @@ async function main() { | |
} | ||
|
||
if (errors && !args.save) { | ||
process.exit(1); | ||
process.exitCode = 1; | ||
} | ||
|
||
return; | ||
|
@@ -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(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This enables performance improvements in |
||
|
||
if (ts.roots.length !== 1) { | ||
throw new Error('Expecting only a single root assembly'); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
import * as changeCase from 'change-case'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tested a bunch.
|
||
|
||
function cachedTransform(transform: (x: string) => string): (x: string) => string { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Throwing this in for good measure, so we never convert the same string twice. It saves 3-4s. |
||
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)); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -126,7 +126,7 @@ apiLinter.add({ | |
return; | ||
} | ||
|
||
if (type.type.fqn === e.ctx.core.constructClass.fqn) { | ||
if (type.type.fqn === e.ctx.core.baseConstructClassFqn) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bypasses loading of the type just to compare the fqn. |
||
return; | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are we the only ones using this package? If not, this would be breaking for any users referring it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's public but in "dev preview". Has been for ages. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Never mind, this isn't even a public API. It's not exported at the top-level. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's also not a library. It's a cli tool. 😅 |
||
|
||
public readonly fqn: string; | ||
public readonly interfaceFqn: string; | ||
public readonly propsFqn: string; | ||
|
@@ -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(); | ||
|
@@ -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; | ||
} | ||
Comment on lines
-103
to
-108
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This check was redundant because |
||
expectedParams.push({ | ||
name: 'scope', | ||
type: baseType.fqn, | ||
type: e.ctx.core.baseConstructClassFqn, | ||
}); | ||
|
||
expectedParams.push({ | ||
|
@@ -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); | ||
}, | ||
}); | ||
|
@@ -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}`); | ||
} | ||
} | ||
}, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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() { | ||
|
@@ -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}`); | ||
} | ||
} | ||
Comment on lines
-162
to
-166
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is already checked by jsii |
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Both values are the same. |
||
e.assertSignature(method, { | ||
parameters: [ | ||
{ name: 'scope', type: baseType }, | ||
|
@@ -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; | ||
Comment on lines
-95
to
+94
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Both values are the same. |
||
e.assertSignature(e.ctx.fromAttributesMethod, { | ||
parameters: [ | ||
{ name: 'scope', type: baseType }, | ||
|
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'; | ||
|
@@ -31,10 +31,9 @@ export class ResourceReflection { | |
return []; // not part of the dep stack | ||
} | ||
|
||
return ConstructReflection | ||
.findAllConstructs(assembly) | ||
.filter(c => CoreTypes.isResourceClass(c.classType)) | ||
Comment on lines
-35
to
-36
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This loops twice over all constructs. |
||
.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 | ||
|
@@ -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`; | ||
|
@@ -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. | ||
|
@@ -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'. | ||
|
@@ -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); | ||
}, | ||
}); | ||
|
@@ -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); | ||
}, | ||
|
@@ -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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using the memoized version of |
||
const ret = tryResolveCfnResource(base); | ||
if (ret) { | ||
return ret; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Make sure we only query (the expensive) overrides if we really need to. |
||
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) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was just broken before 🤷🏻