From 39520cab6c20ce6bc011dfd885bfd232d16ab63b Mon Sep 17 00:00:00 2001 From: Andy Hanson Date: Sat, 31 Dec 2016 10:00:48 -0800 Subject: [PATCH 1/2] Rewrite member-ordering rule --- src/rules/memberOrderingRule.ts | 350 ++++++++---------- .../rules/member-ordering/method/test.ts.lint | 16 + 2 files changed, 166 insertions(+), 200 deletions(-) diff --git a/src/rules/memberOrderingRule.ts b/src/rules/memberOrderingRule.ts index 8d558c9b2d8..7095767dfa2 100644 --- a/src/rules/memberOrderingRule.ts +++ b/src/rules/memberOrderingRule.ts @@ -135,175 +135,120 @@ export class Rule extends Lint.Rules.AbstractRule { } } -/* start code supporting old options (i.e. "public-before-private") */ -interface IModifiers { - isMethod: boolean; - isPrivate: boolean; - isInstance: boolean; -} - -function getModifiers(isMethod: boolean, modifiers?: ts.ModifiersArray): IModifiers { - return { - isInstance: !Lint.hasModifier(modifiers, ts.SyntaxKind.StaticKeyword), - isMethod, - isPrivate: Lint.hasModifier(modifiers, ts.SyntaxKind.PrivateKeyword), - }; -} - -function toString(modifiers: IModifiers): string { - return [ - modifiers.isPrivate ? "private" : "public", - modifiers.isInstance ? "instance" : "static", - "member", - modifiers.isMethod ? "function" : "variable", - ].join(" "); -} -/* end old code */ - -/* start new code */ -enum AccessLevel { - PRIVATE, - PROTECTED, - PUBLIC, -} - -enum Membership { - INSTANCE, - STATIC, -} - -enum Kind { - FIELD, - METHOD, -} - -interface INodeAndModifiers { - accessLevel: AccessLevel; - isConstructor: boolean; - kind: Kind; - membership: Membership; - node: ts.Node; -} - -function getNodeAndModifiers(node: ts.Node, isMethod: boolean, isConstructor = false): INodeAndModifiers { - const { modifiers } = node; - const accessLevel = Lint.hasModifier(modifiers, ts.SyntaxKind.PrivateKeyword) ? AccessLevel.PRIVATE - : Lint.hasModifier(modifiers, ts.SyntaxKind.ProtectedKeyword) ? AccessLevel.PROTECTED - : AccessLevel.PUBLIC; - const kind = isMethod ? Kind.METHOD : Kind.FIELD; - const membership = Lint.hasModifier(modifiers, ts.SyntaxKind.StaticKeyword) ? Membership.STATIC : Membership.INSTANCE; - return { - accessLevel, - isConstructor, - kind, - membership, - node, - }; -} - -function getNodeOption({accessLevel, isConstructor, kind, membership}: INodeAndModifiers) { - if (isConstructor) { - return "constructor"; - } +export class MemberOrderingWalker extends Lint.RuleWalker { + private readonly order: string[] | undefined = (() => { + const allOptions = this.getOptions(); + if (allOptions == null || allOptions.length === 0) { + return undefined; + } - return [ - AccessLevel[accessLevel].toLowerCase(), - Membership[membership].toLowerCase(), - Kind[kind].toLowerCase(), - ].join("-"); -} -/* end new code */ + const firstOption = allOptions[0]; + if (firstOption == null || typeof firstOption !== "object") { + return undefined; + } -export class MemberOrderingWalker extends Lint.RuleWalker { - private previousMember: IModifiers; - private memberStack: INodeAndModifiers[][] = []; - private hasOrderOption = this.getHasOrderOption(); + const orderOption = firstOption[OPTION_ORDER]; + if (Array.isArray(orderOption)) { + return orderOption; + } else if (typeof orderOption === "string") { + return PRESET_ORDERS[orderOption] || PRESET_ORDERS["default"]; + } else { + return undefined; + } + })(); public visitClassDeclaration(node: ts.ClassDeclaration) { - this.resetPreviousModifiers(); - - this.newMemberList(); + this.visitMembers(node.members); super.visitClassDeclaration(node); - this.checkMemberOrder(); } public visitClassExpression(node: ts.ClassExpression) { - this.resetPreviousModifiers(); - - this.newMemberList(); + this.visitMembers(node.members); super.visitClassExpression(node); - this.checkMemberOrder(); } public visitInterfaceDeclaration(node: ts.InterfaceDeclaration) { - this.resetPreviousModifiers(); - - this.newMemberList(); + this.visitMembers(node.members); super.visitInterfaceDeclaration(node); - this.checkMemberOrder(); } - public visitMethodDeclaration(node: ts.MethodDeclaration) { - this.checkModifiersAndSetPrevious(node, getModifiers(true, node.modifiers)); - this.pushMember(getNodeAndModifiers(node, true)); - super.visitMethodDeclaration(node); + public visitTypeLiteral(node: ts.TypeLiteralNode) { + this.visitMembers(node.members); + super.visitTypeLiteral(node); } - public visitMethodSignature(node: ts.SignatureDeclaration) { - this.checkModifiersAndSetPrevious(node, getModifiers(true, node.modifiers)); - this.pushMember(getNodeAndModifiers(node, true)); - super.visitMethodSignature(node); + private visitMembers(members: Member[]) { + if (this.order === undefined) { + this.checkUsingOldOptions(members); + } else { + this.checkUsingNewOptions(members); + } } - public visitConstructorDeclaration(node: ts.ConstructorDeclaration) { - this.checkModifiersAndSetPrevious(node, getModifiers(true, node.modifiers)); - this.pushMember(getNodeAndModifiers(node, true, true)); - super.visitConstructorDeclaration(node); - } + /* start new code */ + private checkUsingNewOptions(members: Member[]) { + let prevRank = -1; + for (const member of members) { + const rank = this.memberRank(member); + if (rank === -1) { + // no explicit ordering for this kind of node specified, so continue + continue; + } - public visitPropertyDeclaration(node: ts.PropertyDeclaration) { - const { initializer } = node; - const isFunction = initializer != null - && (initializer.kind === ts.SyntaxKind.ArrowFunction || initializer.kind === ts.SyntaxKind.FunctionExpression); - this.checkModifiersAndSetPrevious(node, getModifiers(isFunction, node.modifiers)); - this.pushMember(getNodeAndModifiers(node, isFunction)); - super.visitPropertyDeclaration(node); + if (rank < prevRank) { + const nodeType = this.rankName(rank); + const prevNodeType = this.rankName(prevRank); + const lowerRank = this.findLowerRank(members, rank); + const locationHint = lowerRank !== -1 + ? `after ${this.rankName(lowerRank)}s` + : "at the beginning of the class/interface"; + const errorLine1 = `Declaration of ${nodeType} not allowed after declaration of ${prevNodeType}. ` + + `Instead, this should come ${locationHint}.`; + this.addFailureAtNode(member, errorLine1); + } else { + // keep track of last good node + prevRank = rank; + } + } } - public visitPropertySignature(node: ts.PropertyDeclaration) { - this.checkModifiersAndSetPrevious(node, getModifiers(false, node.modifiers)); - this.pushMember(getNodeAndModifiers(node, false)); - super.visitPropertySignature(node); + /** Finds the highest existing rank lower than `targetRank`. */ + private findLowerRank(members: Member[], targetRank: Rank): Rank | -1 { + let max: Rank | -1 = -1; + for (const member of members) { + const rank = this.memberRank(member); + if (rank !== -1 && rank < targetRank) { + max = Math.max(max, rank); + } + } + return max; } - public visitTypeLiteral(_node: ts.TypeLiteralNode) { - // don't call super from here -- we want to skip the property declarations in type literals + private memberRank(member: Member): Rank | -1 { + const optionName = getOptionName(member); + return optionName === undefined ? -1 : this.order!.indexOf(optionName); } - public visitObjectLiteralExpression(_node: ts.ObjectLiteralExpression) { - // again, don't call super here - object literals can have methods, - // and we don't wan't to check these + private rankName(rank: Rank): string { + return this.order![rank].replace(/-/g, " "); } + /* end new code */ /* start old code */ - private resetPreviousModifiers() { - this.previousMember = { - isInstance: false, - isMethod: false, - isPrivate: false, - }; - } - - private checkModifiersAndSetPrevious(node: ts.Node, currentMember: IModifiers) { - if (!this.canAppearAfter(this.previousMember, currentMember)) { - this.addFailureAtNode(node, - `Declaration of ${toString(currentMember)} not allowed to appear after declaration of ${toString(this.previousMember)}`); + private checkUsingOldOptions(members: Member[]) { + let previousModifiers: IModifiers | undefined; + for (const member of members) { + const modifiers = getModifiers(member); + if (previousModifiers !== undefined && !this.canAppearAfter(previousModifiers, modifiers)) { + this.addFailureAtNode(member, + `Declaration of ${toString(modifiers)} not allowed to appear after declaration of ${toString(previousModifiers)}`); + } + previousModifiers = modifiers; } - this.previousMember = currentMember; } - private canAppearAfter(previousMember: IModifiers, currentMember: IModifiers) { - if (previousMember == null || currentMember == null) { + private canAppearAfter(previousMember: IModifiers | undefined, currentMember: IModifiers) { + if (previousMember === undefined) { return true; } @@ -322,75 +267,80 @@ export class MemberOrderingWalker extends Lint.RuleWalker { return true; } /* end old code */ +} - /* start new code */ - private newMemberList() { - if (this.hasOrderOption) { - this.memberStack.push([]); - } - } +/* start code supporting old options (i.e. "public-before-private") */ +interface IModifiers { + isMethod: boolean; + isPrivate: boolean; + isInstance: boolean; +} - private pushMember(node: INodeAndModifiers) { - if (this.hasOrderOption) { - this.memberStack[this.memberStack.length - 1].push(node); - } - } +function getModifiers(member: Member): IModifiers { + return { + isInstance: !Lint.hasModifier(member.modifiers, ts.SyntaxKind.StaticKeyword), + isMethod: isMethodOrConstructor(member), + isPrivate: Lint.hasModifier(member.modifiers, ts.SyntaxKind.PrivateKeyword), + }; +} - private checkMemberOrder() { - if (this.hasOrderOption) { - const memberList = this.memberStack.pop(); - const order = this.getOrder(); - if (memberList !== undefined && order !== null) { - const memberRank = memberList.map((n) => order.indexOf(getNodeOption(n))); - - let prevRank = -1; - memberRank.forEach((rank, i) => { - // no explicit ordering for this kind of node specified, so continue - if (rank === -1) { return; } - - // node should have come before last node, so add a failure - if (rank < prevRank) { - // generate a nice and clear error message - const node = memberList[i].node; - const nodeType = order[rank].split("-").join(" "); - const prevNodeType = order[prevRank].split("-").join(" "); - - const lowerRanks = memberRank.filter((r) => r < rank && r !== -1).sort(); - const locationHint = lowerRanks.length > 0 - ? `after ${order[lowerRanks[lowerRanks.length - 1]].split("-").join(" ")}s` - : "at the beginning of the class/interface"; - - const errorLine1 = `Declaration of ${nodeType} not allowed after declaration of ${prevNodeType}. ` + - `Instead, this should come ${locationHint}.`; - this.addFailureAtNode(node, errorLine1); - } else { - // keep track of last good node - prevRank = rank; - } - }); - } - } +function toString(modifiers: IModifiers): string { + return [ + modifiers.isPrivate ? "private" : "public", + modifiers.isInstance ? "instance" : "static", + "member", + modifiers.isMethod ? "function" : "variable", + ].join(" "); +} +/* end old code */ + +/* start new code */ +const enum MemberKind { Method, Constructor, Field, Ignore } +function getMemberKind(member: Member): MemberKind { + switch (member.kind) { + case ts.SyntaxKind.Constructor: + case ts.SyntaxKind.ConstructSignature: + return MemberKind.Constructor; + case ts.SyntaxKind.MethodDeclaration: + case ts.SyntaxKind.MethodSignature: + return MemberKind.Method; + case ts.SyntaxKind.PropertyDeclaration: + case ts.SyntaxKind.PropertySignature: + const { initializer } = member as ts.PropertyDeclaration; + const isFunction = initializer !== undefined && + (initializer.kind === ts.SyntaxKind.ArrowFunction || initializer.kind === ts.SyntaxKind.FunctionExpression); + return isFunction ? MemberKind.Method : MemberKind.Field; + default: + return MemberKind.Ignore; } +} - private getHasOrderOption() { - const allOptions = this.getOptions(); - if (allOptions == null || allOptions.length === 0) { - return false; - } +function isMethodOrConstructor(member: Member) { + const kind = getMemberKind(member); + return kind === MemberKind.Method || kind === MemberKind.Constructor; +} - const firstOption = allOptions[0]; - return firstOption != null && typeof firstOption === "object" && firstOption[OPTION_ORDER] != null; +/** Returns e.g. "public-static-field". */ +function getOptionName(member: Member): string | undefined { + const memberKind = getMemberKind(member); + switch (memberKind) { + case MemberKind.Constructor: + return "constructor"; + case MemberKind.Ignore: + return undefined; + default: + const accessLevel = hasModifier(ts.SyntaxKind.PrivateKeyword) ? "private" + : hasModifier(ts.SyntaxKind.ProtectedKeyword) ? "protected" + : "public"; + const membership = hasModifier(ts.SyntaxKind.StaticKeyword) ? "static" : "instance"; + const kind = memberKind === MemberKind.Method ? "method" : "field"; + return `${accessLevel}-${membership}-${kind}`; } - - // assumes this.hasOrderOption() === true - private getOrder(): string[] | null { - const orderOption = this.getOptions()[0][OPTION_ORDER]; - if (Array.isArray(orderOption)) { - return orderOption; - } else if (typeof orderOption === "string") { - return PRESET_ORDERS[orderOption] || PRESET_ORDERS["default"]; - } - return null; + function hasModifier(kind: ts.SyntaxKind) { + return Lint.hasModifier(member.modifiers, kind); } - /* end new code */ } + +type Member = ts.TypeElement | ts.ClassElement; +type Rank = number; +/* end new code */ diff --git a/test/rules/member-ordering/method/test.ts.lint b/test/rules/member-ordering/method/test.ts.lint index 1e552a3cbef..044904e4395 100644 --- a/test/rules/member-ordering/method/test.ts.lint +++ b/test/rules/member-ordering/method/test.ts.lint @@ -52,5 +52,21 @@ class Constructor2 { ~~~~~~~~~~~~~~~~~ [0] } +// Works for type literal, just like interface +type T = { + x(): void; + y: number; + ~~~~~~~~~~ [0] +} + +// Works for class inside object literal +const o = { + foo: class C { + x(): void; + y: number; + ~~~~~~~~~~ [0] + } +} + [0]: Declaration of public instance member variable not allowed to appear after declaration of public instance member function From b682a5d8382240c4a30b9c501511b759cb4826b2 Mon Sep 17 00:00:00 2001 From: Andy Hanson Date: Thu, 12 Jan 2017 17:14:39 -0800 Subject: [PATCH 2/2] Move initialization of 'order' to constructor --- src/rules/memberOrderingRule.ts | 45 +++++++++++++++++++-------------- 1 file changed, 26 insertions(+), 19 deletions(-) diff --git a/src/rules/memberOrderingRule.ts b/src/rules/memberOrderingRule.ts index 7095767dfa2..fa699086a88 100644 --- a/src/rules/memberOrderingRule.ts +++ b/src/rules/memberOrderingRule.ts @@ -136,26 +136,12 @@ export class Rule extends Lint.Rules.AbstractRule { } export class MemberOrderingWalker extends Lint.RuleWalker { - private readonly order: string[] | undefined = (() => { - const allOptions = this.getOptions(); - if (allOptions == null || allOptions.length === 0) { - return undefined; - } - - const firstOption = allOptions[0]; - if (firstOption == null || typeof firstOption !== "object") { - return undefined; - } + private readonly order: string[] | undefined; - const orderOption = firstOption[OPTION_ORDER]; - if (Array.isArray(orderOption)) { - return orderOption; - } else if (typeof orderOption === "string") { - return PRESET_ORDERS[orderOption] || PRESET_ORDERS["default"]; - } else { - return undefined; - } - })(); + constructor(sourceFile: ts.SourceFile, options: Lint.IOptions) { + super(sourceFile, options); + this.order = this.getOrder(); + } public visitClassDeclaration(node: ts.ClassDeclaration) { this.visitMembers(node.members); @@ -177,6 +163,27 @@ export class MemberOrderingWalker extends Lint.RuleWalker { super.visitTypeLiteral(node); } + private getOrder(): string[] | undefined { + const allOptions = this.getOptions(); + if (allOptions == null || allOptions.length === 0) { + return undefined; + } + + const firstOption = allOptions[0]; + if (firstOption == null || typeof firstOption !== "object") { + return undefined; + } + + const orderOption = firstOption[OPTION_ORDER]; + if (Array.isArray(orderOption)) { + return orderOption; + } else if (typeof orderOption === "string") { + return PRESET_ORDERS[orderOption] || PRESET_ORDERS["default"]; + } else { + return undefined; + } + } + private visitMembers(members: Member[]) { if (this.order === undefined) { this.checkUsingOldOptions(members);