new-rule: no-unnecessary-class #3119

aervin · 2017-08-11T15:50:14Z

PR checklist

Addresses an existing rule suggestion: #3111
New feature, bugfix, or enhancement
- Includes tests
Documentation update

Overview of change:

Adds the "no-static-only-classes" rule as outlined in the suggestion post.

This rule ignores a class which:

extends an existing class
implements an interface
has anything other than whitespace in its constructor

Is there anything you'd like reviewers to focus on?

Does this rule need to take constructor parameters into account?
Still learning the different APIs--is there a better way to detect empty block scopes (see isEmptyConstructor function)?
Help w/ description/rationale

lambda-fairy · 2017-08-13T05:31:54Z

src/verify/lines.ts

 export class Line { }
+/* tslint:enable:no-static-only-classes */


I would prefer that an empty class is not counted as a static-only class -- I think it would be confusing if it did.

You're right I'll add a check for that.

lambda-fairy · 2017-08-13T05:34:57Z

test/rules/no-static-only-classes/test.ts.lint

+        return true;
+    }
+    constructor() { this.that = what; }
+}


I'd like a test for shorthand properties as well, e.g.:

export class PascalClass { public static helper(): void {} constructor(private that: string) {} }

This should not raise a warning because the private that implicitly declares a non-static property.

ajafff · 2017-08-14T18:53:46Z

src/rules/noStaticOnlyClassesRule.ts

+        for (const statement of sourceFile.statements) {
+            if (isClassDeclaration(statement)
+                && !hasExtendsClause(statement)
+                && !hasImplementsClause(statement)


you can ignore implemented interfaces, because the class will have instance properties or methods if the interface is not empty.

ajafff · 2017-08-14T18:56:15Z

src/rules/noStaticOnlyClassesRule.ts

+
+class NoStaticOnlyClassesWalker extends Lint.AbstractWalker<string[]> {
+    public walk(sourceFile: ts.SourceFile) {
+        for (const statement of sourceFile.statements) {


that will only check classes at the top level of the file. The rule should also check classes nested in functions, namespaces, ...
You can look at class-name for an example on how to iterate the AST correctly.

ajafff · 2017-08-14T18:57:25Z

src/rules/noStaticOnlyClassesRule.ts

+                        return;
+                    }
+                }
+                if (statement.name !== undefined) {


why exclude export default class {static FOO = 1}?

I didn't know this is a possibility. What should the failure message look like in this scenario? I've been underlining the class name to indicate a problem with the class itself.

ajafff · 2017-08-14T18:58:22Z

src/rules/noStaticOnlyClassesRule.ts

+}
+
+function hasExtendsClause(statement: ts.ClassDeclaration): boolean {
+    return (statement.heritageClauses !== undefined) ? statement.heritageClauses[0].token === ts.SyntaxKind.ExtendsKeyword : false;


no need for a ternary expression, simply use &&

ajafff · 2017-08-14T18:59:59Z

src/rules/noStaticOnlyClassesRule.ts

+    return (statement.heritageClauses !== undefined) ? statement.heritageClauses[0].token === ts.SyntaxKind.ImplementsKeyword : false;
+}
+
+function hasStaticModifier(modifiers: ts.NodeArray<ts.Modifier> | undefined): boolean {


use tsutils' hasModifier(node.modifiers, ts.SyntaxKind.StaticKeyword) instead of this function

ajafff · 2017-08-14T19:01:45Z

src/rules/noStaticOnlyClassesRule.ts

+
+function isEmptyConstructor(member: ts.ClassElement): boolean {
+    if (member.kind === ts.SyntaxKind.Constructor
+        && isFunctionWithBody(member)


this line is not needed because you already check if it has a body on the next line

ajafff · 2017-08-14T19:03:00Z

src/rules/noStaticOnlyClassesRule.ts

+function isEmptyConstructor(member: ts.ClassElement): boolean {
+    if (member.kind === ts.SyntaxKind.Constructor
+        && isFunctionWithBody(member)
+        && member.body !== undefined) {


Add && !member.parameters.some(isParameterProperty) to exit if the constructor has parameter properties

ajafff · 2017-08-14T19:03:28Z

src/rules/noStaticOnlyClassesRule.ts

+}
+
+function isEmptyConstructor(member: ts.ClassElement): boolean {
+    if (member.kind === ts.SyntaxKind.Constructor


use isConstructorDeclaration(member) instead

ajafff · 2017-08-14T19:04:19Z

src/rules/noStaticOnlyClassesRule.ts

+    if (member.kind === ts.SyntaxKind.Constructor
+        && isFunctionWithBody(member)
+        && member.body !== undefined) {
+        return member.body.getFullText().trim().replace(/\s+/g, "") === "{}";


return member.body.statements.length === 0

aervin · 2017-08-15T12:06:48Z

Thanks for all the feedback! I'll get started later today.

ajafff · 2017-08-15T19:08:50Z

src/rules/noStaticOnlyClassesRule.ts

+    const classMembers = statement.members;
+    if (classMembers.length === 0) {
+        return true;
+    } else if (classMembers.length === 1 && classMembers[0].kind === ts.SyntaxKind.Constructor) {


why do you need to check if the class has an empty constructor?
you can simplify this whole function body to return statement.members === 0; without changing the semantics of the rule.

ajafff · 2017-08-15T19:09:51Z

src/rules/noStaticOnlyClassesRule.ts

+    }
+}
+
+function hasExtendsClause(statement: ts.ClassDeclaration): boolean {


Can you please rename the parameter statement to something like declaration. Same for the function below.

ajafff · 2017-08-15T19:12:37Z

src/rules/noStaticOnlyClassesRule.ts

+                        return;
+                    }
+                }
+                if (node.name !== undefined) {


If you show the error on the class keyword instead of the class' name, you don't need this condition.

this.addFailureAtNode(getChildOfKind(node, ts.SyntaxKind.ClassKeyword, this.sourceFile)!, Rule.FAILURE_STRING);

aervin · 2017-08-18T17:36:08Z

src/rules/noStaticOnlyClassesRule.ts

+ * An "empty class" for our purposes.
+ */
+function isEmptyClass(declaration: ts.ClassDeclaration): boolean {
+    return declaration.members.length === 0 || declaration.members.every(isConstructorDeclaration);


declaration.members.every(isConstructorDeclaration) because possibility of overloaded constructor? Would like to know if this check is necessary...

That check makes sense to correctly handle constructor overloads.
On a side note, you could remove the length check here because Array.prototype.every returns true for empty arrays.

Your reviews always make me go "oh right... duh!" lol Taking care of this now. However, I only see one suggestion rather than two?

You're, I forgot to save the last comment before submitting 😒

ajafff

Code looks good so far. Added two suggestions

ajafff · 2017-08-18T18:45:18Z

src/rules/noStaticOnlyClassesRule.ts

+ * An "empty class" for our purposes.
+ */
+function isEmptyClass(declaration: ts.ClassDeclaration): boolean {
+    return declaration.members.length === 0 || declaration.members.every(isConstructorDeclaration);


That check makes sense to correctly handle constructor overloads.
On a side note, you could remove the length check here because Array.prototype.every returns true for empty arrays.

ajafff · 2017-08-18T19:41:15Z

src/rules/noStaticOnlyClassesRule.ts

+                for (const member of node.members) {
+                    if (isConstructorWithShorthandProps(member) ||
+                       (!isConstructorDeclaration(member) && !hasModifier(member.modifiers, ts.SyntaxKind.StaticKeyword))) {
+                        return;


This return will cause nested classes to not be checked. Although this might not occur often in real world code, I think it makes sense to also find this error:

class Foo { public prop = 1; constructor() { class Bar { ~~~~~ [lorem ipsum ... static class] static PROP = 2; } } }

Working on a fix but could use your input:

Currently this code is passing in the test file:
class Foo { constructor() { class Bar { private a: SomeType; static PROP = 2; } } }

Personally, I would not want class Foo in my codebase, but should it throw an error?

bad example-- what if private a was public a?

I'd say as soon as there is an instance property or method, the rule should not complain. Therefore class Bar in your example is allowed.
class Foo on the other hand is pretty useless without any property or method. But it's out of scope for this rule (at least with the current name).

But you could just rename the rule to no-unnecessary-class (or similar) and check for

only static members (as it currently does) -> could be converted to a namespace or plain object

constructor only classes -> could be converted to a function

a mix of the above

completely empty classes? - maybe add an option to ignore them

I really like the idea of no-unnecessary-class, but user-config would need some discussion? We could start with a sensible default setting for the rule. I don't see much point in a class with static members and constructors only... Is there a real-life coding scenario where this is useful? I'd be interested to learn.

I don't see much point in a class with static members and constructors only... Is there a real-life coding scenario where this is useful?

It can be useful for writing compilers, where classes simulate discriminated unions (much like "case classes" in Scala). For example:

class While extends Statement { constructor(public condition: Expression, public body: Statement[]) { super(); } } class Break extends Statement {} class Continue extends Statement {}

Then you'd check if a statement is a break using stmt instanceof Break.

Well that makes a lot of sense @lfairy. Have you looked over the test cases for this rule? What do you make of this rule's scope?

…le times

ajafff

Sorry for the delay. I left some comments.

Note to myself: it may make sense to also disallow classes without state, i.e. no instance properties, if it does not implement an interface or extend another class.
That can be implemented afterwards.

ajafff · 2017-09-15T19:38:42Z

src/rules/noUnnecessaryClassRule.ts

+    }
+
+    private hasOption(option: string): boolean {
+        return this.options.indexOf(option) !== -1;


Consider parsing the options upfront and passing an object to the constructor of your Walker.

ajafff · 2017-09-15T19:41:58Z

src/rules/noUnnecessaryClassRule.ts

+                if (node.members.length === 0 && !this.hasOption(OPTION__ALLOW_EMPTY_CLASS)) {
+                    this.addFailureAtNode(getChildOfKind(node, ts.SyntaxKind.ClassKeyword)!, Rule.FAILURE_EMPTY_CLASS);
+                    return;
+                } else if (node.members.length == 0) {


use a nested if statement above instead of duplicating parts of the condition in else-if

ajafff · 2017-09-15T19:44:47Z

src/rules/noUnnecessaryClassRule.ts

+    public walk(sourceFile: ts.SourceFile) {
+        const checkIfStaticOnlyClass = (node: ts.Node): void => {
+            if (isClassDeclaration(node) && !hasExtendsClause(node)) {
+                if (node.members.length === 0 && !this.hasOption(OPTION__ALLOW_EMPTY_CLASS)) {


Consider moving the remainder of this function to a separate method for readability.

ajafff · 2017-09-15T19:48:28Z

src/rules/noUnnecessaryClassRule.ts

+                    return;
+                }
+
+                if (node.members.some(isConstructorWithClassDeclaration)) {


I'd prefer not to have this special case ignored. If there is only a constructor that contains a class declaration, the class is still constructor only.

ajafff · 2017-09-15T19:53:43Z

src/rules/noUnnecessaryClassRule.ts

+
+                const allMembersAreConstructors = node.members.every(isConstructorDeclaration);
+
+                if (allMembersAreConstructors && !this.hasOption(OPTION__ALLOW_CONSTRUCTOR_ONLY)) {


You need to check if the constructor has parameter properties, either by default or add an option for that.

aervin · 2017-09-17T13:48:45Z

Thanks @ajafff! I'll try to get to these changes later today.

…tatic-only-classes

…y classes

aervin · 2017-09-30T00:00:00Z

Hey @ajafff, sorry for the delay.

ajafff · 2017-10-01T13:49:38Z

src/rules/noUnnecessaryClassRule.ts

+    public walk(sourceFile: ts.SourceFile) {
+        const checkIfUnnecessaryClass = (node: ts.Node): void => {
+            if (isClassDeclaration(node) && !hasExtendsClause(node)) {
+                return this.checkMembers(node, checkIfUnnecessaryClass);


don't return here

ajafff · 2017-10-01T13:50:18Z

src/rules/noUnnecessaryClassRule.ts

+        }
+
+        /* Check for classes nested in constructors */
+        for (const member of node.members) {


you can remove this block if you remove the return at the call site (see my other comment above)

ajafff · 2017-10-01T13:53:05Z

src/rules/noUnnecessaryClassRule.ts

+    public static FAILURE_EMPTY_CLASS = "This class has no members.";
+
+    public apply(sourceFile: ts.SourceFile): Lint.RuleFailure[] {
+        return this.applyWithWalker(new NoUnnecessaryClassWalker(sourceFile, this.ruleName, this.ruleArguments));


we typically parse the options before passing them to the Walker.
I mean something like this:

new NoUnnecessaryClassWalker(sourceFile, this.ruleName, { allowConstructorOnly: this.ruleArguments.indexOf(OPTION__ALLOW_CONSTRUCTOR_ONLY) !== -1, allowEmptyClass: this.ruleArguments.indexOf(OPTION__ALLOW_EMPTY_CLASS) !== -1, allowStaticOnly: this.ruleArguments.indexOf(OPTION__ALLOW_STATIC_ONLY) !== -1, })

ajafff · 2017-10-01T13:58:04Z

test/rules/no-unnecessary-class/allow-constructor-only/test.ts.lint

+    constructor() {
+        class Bar {
+            public static helper(): void {}
+            private private Helper(): boolean {


double private

…lker

aervin · 2017-10-03T16:16:07Z

@ajafff I think the description/rationale still needs to be updated. Do you have ideas?

ajafff · 2017-10-03T18:13:59Z

The rationale is still correct.
The description needs to be amended. I'm not the best advisor for such texts. Maybe: "Disallows classes that are not strictly necessary."

adidahiya · 2017-10-20T15:41:04Z

thanks @aervin!

Aaron Ervin added 3 commits August 11, 2017 11:37

ignoring classes w heritage clauses; test for all static/no constructor

d460026

files

5688559

ignoring empty class in lines.ts for now

44f409f

aervin changed the title ~~No static only classes~~ new-rule: no-static-only-classes Aug 11, 2017

unsafe anys removed

a32e011

lambda-fairy reviewed Aug 13, 2017

View reviewed changes

removed disable tag in lines.ts

63e9d6a

ajafff suggested changes Aug 14, 2017

View reviewed changes

addresses all requests except export default class {static FOO = 1}

972715f

ajafff reviewed Aug 15, 2017

View reviewed changes

Aaron Ervin added 3 commits August 17, 2017 08:12

Merge branch 'master' into no-static-only-classes

a2eb7a6

isEmptyClass returns true for constructor-only classes

74e7866

using .every rather than allMembersAreConstructors function

5012ae6

aervin commented Aug 18, 2017

View reviewed changes

ajafff reviewed Aug 18, 2017

View reviewed changes

Aaron Ervin added 5 commits August 18, 2017 16:58

added support for nested class declarations within constructors

bf7af78

test file updates

00174ea

no-unnecessary-class w/ config proposal

5a1c9e5

member functions -> functions...

91bbb09

assigning result of node.members.every(...) instead of calling multip…

5fa280e

…le times

ajafff added the PR: waiting for reviewer label Sep 12, 2017

ajafff self-assigned this Sep 12, 2017

ajafff reviewed Sep 15, 2017

View reviewed changes

ajafff added PR: waiting for author and removed PR: waiting for reviewer labels Sep 29, 2017

Merge branch 'master' of https://github.com/palantir/tslint into no-s…

89ee1dc

…tatic-only-classes

Change requests addressed; additional logic to address contructor-onl…

376af9f

…y classes

aervin changed the title ~~new-rule: no-static-only-classes~~ new-rule: no-unnecessary-class Sep 30, 2017

Aaron Ervin added 6 commits September 29, 2017 20:20

using various Array methods over for of loops for readability

7047d6f

'static int var' is not a thing in typescript...

8553c22

test updates

4fe5677

moved walker logic into separate function

5576fd0

now throws failure in nested classes as well

a191ac8

removed some nested ifs; simplified checkMember logic

176049a

ajafff reviewed Oct 1, 2017

View reviewed changes

removed unnec for of block; parsing options before they make it to wa…

cf33675

…lker

ajafff approved these changes Oct 2, 2017

View reviewed changes

ajafff removed the PR: waiting for author label Oct 2, 2017

Aaron Ervin and others added 3 commits October 3, 2017 17:49

Merge branch 'master' into no-static-only-classes

10c01a8

Description update; pulled in latest master

4de707b

Merge branch 'master' into no-static-only-classes

09e4244

adidahiya approved these changes Oct 20, 2017

View reviewed changes

adidahiya merged commit 10e6c9c into palantir:master Oct 20, 2017

aervin deleted the no-static-only-classes branch October 20, 2017 15:48

ajafff mentioned this pull request Oct 31, 2017

Lint against static-only classes #3111

Closed

HyphnKnight pushed a commit to HyphnKnight/tslint that referenced this pull request Apr 9, 2018

new-rule: no-unnecessary-class (palantir#3119)

5ea7291

		export class Line { }
		/* tslint:enable:no-static-only-classes */


		const allMembersAreConstructors = node.members.every(isConstructorDeclaration);

		if (allMembersAreConstructors && !this.hasOption(OPTION__ALLOW_CONSTRUCTOR_ONLY)) {

new-rule: no-unnecessary-class #3119

new-rule: no-unnecessary-class #3119

Conversation

aervin commented Aug 11, 2017 • edited

PR checklist

Overview of change:

Is there anything you'd like reviewers to focus on?

lambda-fairy Aug 13, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lambda-fairy Aug 13, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aervin commented Aug 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajafff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aervin Aug 18, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aervin Aug 18, 2017 • edited

Choose a reason for hiding this comment

lambda-fairy Aug 19, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajafff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aervin commented Sep 17, 2017

aervin commented Sep 30, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aervin commented Oct 3, 2017

ajafff commented Oct 3, 2017

adidahiya commented Oct 20, 2017

aervin commented Aug 11, 2017 •

edited

lambda-fairy Aug 13, 2017 •

edited

lambda-fairy Aug 13, 2017 •

edited

aervin Aug 18, 2017 •

edited

aervin Aug 18, 2017 •

edited

lambda-fairy Aug 19, 2017 •

edited

aervin commented Sep 30, 2017 •

edited