Skip to content
This repository has been archived by the owner on Mar 25, 2021. It is now read-only.

new-rule: no-unnecessary-class #3119

Merged
merged 26 commits into from
Oct 20, 2017
Merged

Conversation

aervin
Copy link
Contributor

@aervin aervin commented Aug 11, 2017

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

@aervin aervin changed the title No static only classes new-rule: no-static-only-classes Aug 11, 2017
export class Line { }
/* tslint:enable:no-static-only-classes */
Copy link

@lambda-fairy lambda-fairy Aug 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

@lambda-fairy lambda-fairy Aug 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return;
}
}
if (statement.name !== undefined) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need for a ternary expression, simply use &&

return (statement.heritageClauses !== undefined) ? statement.heritageClauses[0].token === ts.SyntaxKind.ImplementsKeyword : false;
}

function hasStaticModifier(modifiers: ts.NodeArray<ts.Modifier> | undefined): boolean {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

function isEmptyConstructor(member: ts.ClassElement): boolean {
if (member.kind === ts.SyntaxKind.Constructor
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use isConstructorDeclaration(member) instead

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@aervin
Copy link
Contributor Author

aervin commented Aug 15, 2017

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}

function hasExtendsClause(statement: ts.ClassDeclaration): boolean {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return;
}
}
if (node.name !== undefined) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@ajafff ajafff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good so far. Added two suggestions

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
        }
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@aervin aervin Aug 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@aervin aervin Aug 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@lambda-fairy lambda-fairy Aug 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@ajafff ajafff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

private hasOption(option: string): boolean {
return this.options.indexOf(option) !== -1;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return;
}

if (node.members.some(isConstructorWithClassDeclaration)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


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

if (allMembersAreConstructors && !this.hasOption(OPTION__ALLOW_CONSTRUCTOR_ONLY)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@aervin
Copy link
Contributor Author

aervin commented Sep 17, 2017

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

@aervin
Copy link
Contributor Author

aervin commented Sep 30, 2017

Hey @ajafff, sorry for the delay.

@aervin aervin changed the title new-rule: no-static-only-classes new-rule: no-unnecessary-class Sep 30, 2017
public walk(sourceFile: ts.SourceFile) {
const checkIfUnnecessaryClass = (node: ts.Node): void => {
if (isClassDeclaration(node) && !hasExtendsClause(node)) {
return this.checkMembers(node, checkIfUnnecessaryClass);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't return here

}

/* Check for classes nested in constructors */
for (const member of node.members) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,
})

constructor() {
class Bar {
public static helper(): void {}
private private Helper(): boolean {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

double private

@aervin
Copy link
Contributor Author

aervin commented Oct 3, 2017

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

@ajafff
Copy link
Contributor

ajafff commented Oct 3, 2017

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
Copy link
Contributor

thanks @aervin!

@adidahiya adidahiya merged commit 10e6c9c into palantir:master Oct 20, 2017
@aervin aervin deleted the no-static-only-classes branch October 20, 2017 15:48
HyphnKnight pushed a commit to HyphnKnight/tslint that referenced this pull request Apr 9, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants