Skip to content
This repository has been archived by the owner on Jan 19, 2019. It is now read-only.

[FEAT] Add 'callable-types' rule #280

Closed
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ You can also enable all the recommended rules at once. Add `plugin:typescript/re
| [`typescript/adjacent-overload-signatures`](./docs/rules/adjacent-overload-signatures.md) | Require that member overloads be consecutive (`adjacent-overload-signatures` from TSLint) | :heavy_check_mark: | |
| [`typescript/array-type`](./docs/rules/array-type.md) | Requires using either `T[]` or `Array<T>` for arrays (`array-type` from TSLint) | :heavy_check_mark: | :wrench: |
| [`typescript/ban-types`](./docs/rules/ban-types.md) | Enforces that types will not to be used (`ban-types` from TSLint) | :heavy_check_mark: | :wrench: |
| [`typescript/callable-types`](./docs/rules/callable-types.md) | Use function types instead of interfaces with call signatures (`callable-types` from TSLint) | | :wrench: |
| [`typescript/camelcase`](./docs/rules/camelcase.md) | Enforce camelCase naming convention | :heavy_check_mark: | |
| [`typescript/class-name-casing`](./docs/rules/class-name-casing.md) | Require PascalCased class and interface names (`class-name` from TSLint) | :heavy_check_mark: | |
| [`typescript/explicit-function-return-type`](./docs/rules/explicit-function-return-type.md) | Require explicit return types on functions and class methods | :heavy_check_mark: | |
Expand Down
4 changes: 2 additions & 2 deletions ROADMAP.md
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
# Roadmap
# Roadmap

## TSLint rules

Expand Down Expand Up @@ -126,7 +126,7 @@
| [`arrow-parens`] | 🌟 | [`arrow-parens`](https://eslint.org/docs/rules/arrow-parens) |
| [`arrow-return-shorthand`] | 🌟 | [`arrow-body-style`](https://eslint.org/docs/rules/arrow-body-style) |
| [`binary-expression-operand-order`] | 🌟 | [`yoda`](https://eslint.org/docs/rules/yoda) |
| [`callable-types`] | 🛑 | N/A |
| [`callable-types`] | | [`typescript/callable-types`] |
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don’t forget to add this reference to the section at the bottom of the page so the link works properly.

| [`class-name`] | ✅ | [`typescript/class-name-casing`] |
| [`comment-format`] | 🌟 | [`capitalized-comments`](https://eslint.org/docs/rules/capitalized-comments) & [`spaced-comment`](https://eslint.org/docs/rules/spaced-comment) |
| [`completed-docs`] | 🔌 | [`eslint-plugin-jsdoc`](https://github.com/gajus/eslint-plugin-jsdoc) |
Expand Down
57 changes: 57 additions & 0 deletions docs/rules/callable-types.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
# Use function types instead of interfaces with call signatures (callable-types)

## Rule Details

This rule suggests using a function type instead of an interface or object type literal with a single call signature.

Examples of **incorrect** code for this rule:

```ts
interface Foo {
(): string;
}
```

```ts
function foo(bar: { (): number }): number {
return bar();
}
```

```ts
interface Foo extends Function {
(): void;
}
```

Examples of **correct** code for this rule:

```ts
interface Foo {
(): void;
bar: number;
}
```

```ts
function foo(bar: { (): string; baz: number }): string {
return bar();
}
```

```ts
interface Foo {
bar: string;
}
interface Bar extends Foo {
(): void;
}
```

## When Not To Use It

If you specifically want to use an interface or type literal with a single call signature for stylistic reasons, you can disable this rule.

## Further Reading

- TSLint: ['callable-types'](https://palantir.github.io/tslint/rules/callable-types/)
uniqueiniquity marked this conversation as resolved.
Show resolved Hide resolved
199 changes: 199 additions & 0 deletions lib/rules/callable-types.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,199 @@
/**
* @fileoverview Use function types instead of interfaces with call signatures
* @author Benjamin Lichtman
*/
"use strict";
const ts = require("typescript");
const tsutils = require("tsutils");
const util = require("../util");

/**
* @typedef {import("eslint").Rule.RuleModule} RuleModule
* @typedef {import("estree").Node} ESTreeNode
*/

//------------------------------------------------------------------------------
// Rule Definition
//------------------------------------------------------------------------------

/**
* @type {RuleModule}
*/
module.exports = {
meta: {
docs: {
description:
"Use function types instead of interfaces with call signatures",
category: "Style",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be TypeScript instead?

recommended: false,
extraDescription: [util.tslintRule("callable-types")],
url: util.metaDocsUrl("callable-types"),
},
fixable: "code", // or "code" or "whitespace"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you remove this comment?

Suggested change
fixable: "code", // or "code" or "whitespace"
fixable: "code",

schema: [],
type: "suggestion",
},

create(context) {
// variables should be defined here

//----------------------------------------------------------------------
// Helpers
//----------------------------------------------------------------------

/**
* @param {string} type The incorrect type of callable construct
* @param {string} sigSuggestion The recommended type of callable construct
* @returns {string} The error message
*/
function failureMessage(type, sigSuggestion) {
return `${type} has only a call signature - use \`${sigSuggestion}\` instead.`;
}

/**
* Checks if there is no supertype or if the supertype is 'Function'
* @param {ESTreeNode} node The node being checked
* @returns {boolean} Returns true iff there is no supertype or if the supertype is 'Function'
*/
function noSupertype(node) {
if (typeof node.heritage === "undefined") {
return true;
}
if (node.heritage.length !== 1) {
return node.heritage.length === 0;
}
const expr = node.heritage[0].id;

return (
util.esTreeNodeHasKind(expr, ts.SyntaxKind.Identifier) &&
expr.name === "Function"
);
}

/**
* @param {ESTreeNode} parent The parent of the call signature causing the diagnostic
* @returns {boolean} true iff the parent node needs to be wrapped for readability
*/
function shouldWrapSuggestion(parent) {
switch (parent.type) {
case util.getESTreeType(ts.SyntaxKind.UnionType):
case util.getESTreeType(ts.SyntaxKind.IntersectionType):
case util.getESTreeType(ts.SyntaxKind.ArrayType):
return true;
default:
return false;
}
}

/**
* @param {ESTreeNode} call The call signature causing the diagnostic
* @param {ESTreeNode} parent The parent of the call
* @returns {string} The suggestion to report
*/
function renderSuggestion(call, parent) {
const sourceCode = context.getSourceCode();
const start = call.range[0];
const colonPos = call.typeAnnotation.range[0] - start;
const text = sourceCode.getText().slice(start, call.range[1]);

let suggestion = `${text.slice(0, colonPos)} =>${text.slice(
colonPos + 1
)}`;

if (shouldWrapSuggestion(parent.parent)) {
suggestion = `(${suggestion})`;
}
if (
util.esTreeNodeHasKind(
parent,
ts.SyntaxKind.InterfaceDeclaration
)
) {
if (typeof parent.typeParameters !== "undefined") {
return `type${sourceCode
.getText()
.slice(
parent.name.pos,
parent.typeParameters.end + 1
)} = ${suggestion}`;
}
return `type ${parent.id.name} = ${suggestion}`;
}
return suggestion.endsWith(";")
? suggestion.slice(0, -1)
: suggestion;
}

/**
* @param {ESTreeNode} member The potential call signature being checked
* @param {ESTreeNode} node The node being checked
* @returns {void}
*/
function checkMember(member, node) {
if (
util.esTreeNodeHasKind(member, ts.SyntaxKind.CallSignature) &&
typeof member.typeAnnotation !== "undefined"
) {
const suggestion = renderSuggestion(member, node);
const fixStart = util.esTreeNodeHasKind(
node,
ts.SyntaxKind.TypeLiteral
)
? node.range[0]
: tsutils
.getChildOfKind(
context.parserServices.esTreeNodeToTSNodeMap.get(
node
),
ts.SyntaxKind.InterfaceKeyword
)
.getStart();

context.report({
node: member,
message: failureMessage(
util.esTreeNodeHasKind(node, ts.SyntaxKind.TypeLiteral)
? "Type literal"
: "Interface",
suggestion
),
fix(fixer) {
return fixer.replaceTextRange(
[fixStart, node.range[1]],
suggestion
);
},
});
}
}

//----------------------------------------------------------------------
// Public
//----------------------------------------------------------------------

return {
/**
* @param {ESTreeNode} node The node being checked
* @returns {void}
*/
TSInterfaceDeclaration(node) {
if (noSupertype(node) && node.body.body.length === 1) {
checkMember(node.body.body[0], node);
}
},
/**
* Won't work until type annotations are visited
* @param {ESTreeNode} node The node being checked
* @returns {void}
*/
TSTypeLiteral(node) {
if (
util.esTreeNodeHasKind(node, ts.SyntaxKind.TypeLiteral) &&
node.members.length === 1
) {
checkMember(node.members[0], node);
}
},
};
},
};
23 changes: 23 additions & 0 deletions lib/util.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
"use strict";

const parser = require("typescript-eslint-parser");
const ts = require("typescript");
const version = require("../package.json").version;

exports.tslintRule = name => `\`${name}\` from TSLint`;
Expand Down Expand Up @@ -126,3 +128,24 @@ exports.getParserServices = context => {
}
return context.parserServices;
};

/**
* @param {ts.SyntaxKind} syntaxKind A TS syntax kind
* @returns {string} The corresponding string representation
*/
function getESTreeType(syntaxKind) {
const tsSyntaxKindName = ts.SyntaxKind[syntaxKind];

return parser.Syntax[tsSyntaxKindName] || `TS${tsSyntaxKindName}`;
Copy link
Contributor

@armano2 armano2 Jan 8, 2019

Choose a reason for hiding this comment

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

this is not always a case, sometimes names do change, i don't think it's good idea..

i think its better to import types from

https://github.com/JamesHenry/typescript-estree/blob/master/src/ast-node-types.ts
"typescript-estree/dist/ast-node-types.js"

with that we don't have to convert names back and forward...

number (SyntaxKind) -> string (SyntaxKind) -> string (not always correct ts node)

Copy link
Author

Choose a reason for hiding this comment

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

That is what I'm doing; parser.Syntax is pulling from there, it just seemed right to do it through typescript-eslint-parser as opposed to directly referencing typescript-estree. The second case is just backup for nodes missing from the file you linked.

Copy link
Contributor

Choose a reason for hiding this comment

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

there are also nodes not present in ts, like TSTypeParameterDeclaration, TSTypeParameterInstantiation

Copy link
Author

Choose a reason for hiding this comment

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

Oh hmm, I didn't see your edit. Let me see if that works better.

Copy link
Contributor

@armano2 armano2 Jan 8, 2019

Choose a reason for hiding this comment

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

also in some rare cases this using conversion ts.SyntaxKind[syntaxKind] will return you invalid name

this enum contains also FirstAssignment, LastAssignment and so on, it will convert it to last key from enum with provided value

for example:
instead of TemplateTail you will get LastTemplateToken

Copy link
Contributor

Choose a reason for hiding this comment

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

TemplateTail is not present in estree its called TemplateElement :)

Copy link
Author

Choose a reason for hiding this comment

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

Yup, I totally see your point. I'll put together a different approach later today.

}

exports.getESTreeType = getESTreeType;

/**
* @param {ESTreeNode} node An ESTree node
* @param {ts.SyntaxKind} kind A TS node kind
* @returns {boolean} Returns true iff the ESTree node has the corresponding TS kind
*/
exports.esTreeNodeHasKind = function(node, kind) {
return node.type === getESTreeType(kind);
};
3 changes: 2 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,11 @@
},
"dependencies": {
"requireindex": "^1.2.0",
"tsutils": "^3.6.0",
"typescript-eslint-parser": "21.0.2"
},
"devDependencies": {
"eslint": "^5.9.0",
"nyc": "^13.1.0",
"eslint-config-eslint": "^5.0.1",
"eslint-config-prettier": "^3.3.0",
"eslint-docs": "^0.2.6",
Expand All @@ -42,6 +42,7 @@
"husky": "^1.2.0",
"lint-staged": "^8.1.0",
"mocha": "^5.2.0",
"nyc": "^13.1.0",
"prettier-eslint-cli": "^4.7.1",
"typescript": "~3.1.1"
},
Expand Down
8 changes: 8 additions & 0 deletions tests/lib/fixtures/tsconfig.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{
"compilerOptions": {
"target": "es5",
"module": "commonjs",
"strict": true,
"esModuleInterop": true
}
}
Loading