Rule: `Promise`-returning methods must be `async` #1779

buu700 · 2016-11-23T17:33:14Z

PR checklist

Addresses an existing issue: Rule proposal: require async for Promise returning methods. #1699
New feature, bugfix, or enhancement
- Includes tests
Documentation update

What changes did you make?

Added the rule proposed in the linked issue.

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

Not sure if my methods of detecting async and Promise are optimal (wasn't sure where to find detailed API documentation on the stuff under ts).

Addresses palantir#1699

palantirtech · 2016-11-23T17:33:16Z

Thanks for your interest in palantir/tslint, @buu700! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request.

nchen63 · 2016-11-24T02:09:02Z

src/rules/promiseAsyncRule.ts

+    }
+
+    private test(node: ts.FunctionExpression|ts.MethodDeclaration): void {
+        const isAsync   = node.getText().split(/[\(=]/)[0].match(/(^|\s)async($|\s)/) !== null;


check for async using

Lint.hasModifier(node.modifiers, ts.SyntaxKind.AsyncKeyword)

nchen63 · 2016-11-24T02:15:15Z

unit tests?

nchen63 · 2016-11-24T02:20:09Z

src/rules/promiseAsyncRule.ts

+
+    private test(node: ts.FunctionExpression|ts.MethodDeclaration): void {
+        const isAsync   = Lint.hasModifier(node.modifiers, ts.SyntaxKind.AsyncKeyword);
+        const isPromise = node.type.getText().indexOf("Promise<") === 0;


searching the body for Promise< can be pretty flaky. Can you use ApplyWithProgram and get the actual return type?

this looks like it's testing just the return type of a function. it would work in some cases, but yeah I generally agree with @nchen63 that this rule should use the type checker (unless maybe if the typedef rule is enabled? in that case you'd be forced to declare : Promise<T> everywhere and a simple syntactic check is sufficient... food for thought)

adidahiya · 2016-11-24T17:54:42Z

src/rules/promiseAsyncRule.ts

+    }
+
+    private test(node: ts.FunctionExpression|ts.MethodDeclaration): void {
+        const isAsync   = Lint.hasModifier(node.modifiers, ts.SyntaxKind.AsyncKeyword);


nit: please avoid aligning = across statements like this; we don't generally do this elsewhere since we find it unmaintainable.

adidahiya · 2016-11-24T17:57:12Z

src/rules/promiseAsyncRule.ts

+
+    private test(node: ts.FunctionExpression|ts.MethodDeclaration): void {
+        const isAsync   = Lint.hasModifier(node.modifiers, ts.SyntaxKind.AsyncKeyword);
+        const isPromise = node.type.getText().indexOf("Promise<") === 0;


this looks like it's testing just the return type of a function. it would work in some cases, but yeah I generally agree with @nchen63 that this rule should use the type checker (unless maybe if the typedef rule is enabled? in that case you'd be forced to declare : Promise<T> everywhere and a simple syntactic check is sufficient... food for thought)

buu700 · 2016-11-24T18:58:47Z

src/rules/promiseAsyncRule.ts

+    private test(node: ts.FunctionExpression|ts.MethodDeclaration): void {
+        const tc = this.getTypeChecker();
+
+        const returnType = tc.typeToString(


Is this + line 64 now the best way to do this, or is something more reliable possible?

i'm fine with this.

buu700 · 2016-11-24T19:27:39Z

Not sure what's going on here, but it looks like my unit tests are failing because the return type of every Promise-returning function/method is being considered any (the number-returning functions are fine). Maybe a missing type definition?

nchen63 · 2016-11-25T04:19:43Z

src/rules/promiseAsyncRule.ts

+export class Rule extends Lint.Rules.TypedRule {
+    /* tslint:disable:object-literal-sort-keys */
+    public static metadata: Lint.IRuleMetadata = {
+        ruleName: "promise-functions-async",


rule name doesn't match file name

Oops, sorry about that. Any preference on whether I change the ruleName or file name, or is there an entirely different name that would better fit your established conventions?

I prefer promise-functions-async. @adidahiya?

promise-function-async sounds good (singular "function") -- although the rule names aren't 100% consistent (#1371), we generally use singular nouns in them.

Cool, I'll go with that then.

nchen63 · 2016-11-25T04:20:32Z

src/rules/promiseAsyncRule.ts

+    public static metadata: Lint.IRuleMetadata = {
+        ruleName: "promise-functions-async",
+        description: "Requires any function or method that returns a promise to be marked async.",
+        rationale: "Ensures that a function can only either return a rejected promise or throw an Error object.",


Can you give the "why" of the rationale? Why is it bad to violate this rule?

How's this? (Is Markdown syntax supported in these descriptions?)

Ensures that each function is only capable of 1) returning a rejected promise, or 2) throwing an Error object. In contrast, non-async Promise-returning functions are technically capable of either. This rule removes a requirement for consuming code to handle both cases.

nchen63 · 2016-11-25T04:54:19Z

test/rules/promise-async/test.ts.lint

@@ -0,0 +1,38 @@
+const nonAsyncPromiseFunction = function(p: Promise<void>) { return p; };


you need declare class Promise<T>{} for this to work. Otherwise, the type checker will not know what a Promise<> is and consider it to be any

that type is provided by lib.es2015.d.ts; I would prefer using that if possible instead of constructing our own type

What would be the best way to get those types included in this test?

good question... I don't think this has come up before. perhaps we can add a "compilerOptions" block to our test tslint.json files?

N.B. as you can see from this thread (#1445), it might be a good idea to put "compilerOptions" under a namespace in the test tslint.json so as to clearly indicate it is a private API for use in this repo only and not a public API for tslint users yet.

Got it, thanks. For this PR, mind if I just put declare class Promise<T>{} at the top of the test file as @nchen63 suggested? That at least seems to resolve the immediate issue.

@buu700 yeah that's fine; @nchen63 can look into the longer term solution after this PR merges

nchen63 · 2016-11-25T04:54:50Z

src/rules/promiseAsyncRule.ts

+        super.visitMethodDeclaration(node);
+    }
+
+    private test(node: ts.FunctionExpression|ts.MethodDeclaration): void {


change the type to simply ts.SignatureDeclaration

nchen63 · 2016-11-25T04:58:36Z

src/rules/promiseAsyncRule.ts

+}
+
+class PromiseAsyncWalker extends Lint.ProgramAwareRuleWalker {
+    public visitFunctionExpression(node: ts.FunctionExpression): void {


also need visitArrowFunction and visitFunctionDeclaration

nit: prefer omitting the : void return typedef on functions/methods

nchen63 · 2016-11-25T05:03:21Z

test/rules/promise-async/test.ts.lint

@@ -0,0 +1,38 @@
+const nonAsyncPromiseFunction = function(p: Promise<void>) { return p; };
+                                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~    [0]
+


also need cases

x.callback(() => return new Promise<void>());

and

function x() { return new Promise<int>(); }

which hit the new visit* cases I suggested in the other file

adidahiya · 2016-11-25T23:53:18Z

src/rules/promiseAsyncRule.ts

+        optionsDescription: "Not configurable.",
+        options: null,
+        optionExamples: ["true"],
+        type: "functionality",


let's classify this as a typescript-only rule. I know the categories aren't perfectly mutually exclusive, but I'd like to highlight the fact that you get this lint rule only via usage of the TypeScript compiler services.

adidahiya · 2016-11-25T23:54:48Z

src/rules/promiseAsyncRule.ts

+        options: null,
+        optionExamples: ["true"],
+        type: "functionality",
+        typescriptOnly: false,


despite my comment on the above line, the rule can be used with either TS or JS code, so this line is correct 👍

Hm, interesting; I'd actually meant to (and forgotten to) change that as part of my last commit. How would a rule that requires type checking work with JS?

The TS compiler provides intellisense for JS too, since the syntax is just a subset of TS. For example, this is enabled by default in VSCode. I haven't tested it with the compiler services, but it should work in theory.

Ah okay, cool.

adidahiya · 2016-11-25T23:55:10Z

src/rules/promiseAsyncRule.ts

+}
+
+class PromiseAsyncWalker extends Lint.ProgramAwareRuleWalker {
+    public visitFunctionExpression(node: ts.FunctionExpression): void {


nit: prefer omitting the : void return typedef on functions/methods

adidahiya · 2016-11-26T00:00:15Z

test/rules/promise-async/test.ts.lint

@@ -0,0 +1,38 @@
+const nonAsyncPromiseFunction = function(p: Promise<void>) { return p; };


good question... I don't think this has come up before. perhaps we can add a "compilerOptions" block to our test tslint.json files?

N.B. as you can see from this thread (#1445), it might be a good idea to put "compilerOptions" under a namespace in the test tslint.json so as to clearly indicate it is a private API for use in this repo only and not a public API for tslint users yet.

nchen63 · 2016-11-26T04:00:17Z

test/rules/promise-function-async/test.ts.lint

+const nonAsyncNonPromiseArrowFunction = (n: number) => n;
+
+class Test {
+    public nonAsyncPromiseMethodA(p: Promise<void>) {


for big functions, seems excessive to highlight the whole thing including the body. How about just highlighting the function signature?

nchen63 · 2016-11-26T04:01:12Z

test/rules/promise-function-async/test.ts.lint

@@ -0,0 +1,65 @@
+declare class Promise<T>{}
+
+const nonAsyncPromiseFunctionExpressionA = function(p: Promise<void>) { return p; };


for every test that causes a lint failure, you should have a matching test that passes

buu700 · 2016-11-26T04:51:20Z

src/rules/promiseFunctionAsyncRule.ts

+        const isAsync = Lint.hasModifier(node.modifiers, ts.SyntaxKind.AsyncKeyword);
+        const isPromise = returnType.indexOf("Promise<") === 0;
+
+        const signatureEnd = node.getText().split(/(\s+)?[\{=]/)[0].length;


Any idea if there's a better way to do this? (I just put this in as a placeholder that works with the current tests, but it'd break in a lot of common cases.)

I didn't see anything under ts.TypeChecker, ts.Signature, or ts.SignatureDeclaration that looked like it would give me a string representation of the signature or the length of just the signature component of the node, so not sure right now how to go about this.

try:

let declarationLength: number; if (node.body == null) { declarationLength = node.getWidth(); } else { declarationLength = node.body.getStart() - node.getStart() - 1; } this.addFailure(this.createFailure(node.getStart(), declarationLength, Rule.FAILURE_STRING));

you'll have to define test's signature as:

private test(node: ts.SignatureDeclaration & { body?: ts.Node}) {

Perfect, thanks!

nchen63 · 2016-11-27T05:20:05Z

@buu700 looks good. Thanks!

Rule: Promise-returning methods must be async

0038b10

Addresses palantir#1699

buu700 added 3 commits November 23, 2016 12:38

minor fix

094d788

linting fixes

68ce14d

async regex fix

7df221f

nchen63 reviewed Nov 24, 2016

View reviewed changes

better method of detecting async

68e1ae5

nchen63 reviewed Nov 24, 2016

View reviewed changes

nchen63 added the PR: waiting for author label Nov 24, 2016

adidahiya suggested changes Nov 24, 2016

View reviewed changes

buu700 added 2 commits November 24, 2016 13:09

promise-async unit tests

72c7498

convert promise-async to typed rule

d326ac9

adidahiya added PR: waiting for reviewer and removed PR: waiting for author labels Nov 24, 2016

buu700 added 4 commits November 24, 2016 13:19

promise-async fix

144f081

promise-async test fix

31f4f95

promise-async fix

d74f033

promise-async linting fix

ec2d941

buu700 commented Nov 24, 2016

View reviewed changes

promise-async test fix

930e902

nchen63 reviewed Nov 25, 2016

View reviewed changes

nchen63 added PR: waiting for author and removed PR: waiting for reviewer labels Nov 25, 2016

adidahiya suggested changes Nov 26, 2016

View reviewed changes

promise-function-async rule working + tests passing

4606e49

buu700 force-pushed the patch-1 branch from ee2f341 to 4606e49 Compare November 26, 2016 03:24

nchen63 reviewed Nov 26, 2016

View reviewed changes

buu700 added 2 commits November 25, 2016 23:32

limit promise-function-async squigglies to signature

23fda9b

add promise-function-async test cases

cb6c569

buu700 commented Nov 26, 2016

View reviewed changes

promise-function-async signature length detection fix

84f4747

nchen63 merged commit 5c7cf4a into palantir:master Nov 27, 2016

buu700 deleted the patch-1 branch November 27, 2016 05:21

andy-hanson mentioned this pull request Apr 2, 2017

Rule proposal: require async for Promise returning methods. #1699

Closed

bradzacher mentioned this pull request May 17, 2020

@typescript-eslint/promise-function-async should not flag Promise subclasses typescript-eslint/typescript-eslint#2034

Closed

		@@ -0,0 +1,38 @@
		const nonAsyncPromiseFunction = function(p: Promise<void>) { return p; };

		@@ -0,0 +1,38 @@
		const nonAsyncPromiseFunction = function(p: Promise<void>) { return p; };
		~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [0]

		@@ -0,0 +1,65 @@
		declare class Promise<T>{}

		const nonAsyncPromiseFunctionExpressionA = function(p: Promise<void>) { return p; };

Rule: Promise-returning methods must be async #1779

Rule: Promise-returning methods must be async #1779

Conversation

buu700 commented Nov 23, 2016 • edited by adidahiya Loading

PR checklist

What changes did you make?

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

palantirtech commented Nov 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nchen63 commented Nov 24, 2016

Choose a reason for hiding this comment

adidahiya Nov 24, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adidahiya Nov 24, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

buu700 commented Nov 24, 2016

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

buu700 Nov 26, 2016 • edited Loading

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

nchen63 Nov 25, 2016 • edited Loading

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

buu700 Nov 26, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nchen63 commented Nov 27, 2016

Rule: `Promise`-returning methods must be `async` #1779

Rule: `Promise`-returning methods must be `async` #1779

buu700 commented Nov 23, 2016 •

edited by adidahiya

Loading

adidahiya Nov 24, 2016 •

edited

Loading

adidahiya Nov 24, 2016 •

edited

Loading

buu700 Nov 26, 2016 •

edited

Loading

nchen63 Nov 25, 2016 •

edited

Loading

buu700 Nov 26, 2016 •

edited

Loading