Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(lint): support class methods in noFloatingPromises #5028

Open
wants to merge 7 commits into
base: next
Choose a base branch
from

Conversation

kaykdm
Copy link
Contributor

@kaykdm kaykdm commented Feb 4, 2025

Summary

related: #3187 and #4956
This pull request introduces the support for class methods in noFloatingPromises

Example of invalid code:

calling an async method in the same class

class Test {
  async returnsPromise(): Promise<string> {
    return 'value';
  }
  async someMethod() {
    this.returnsPromise(); // diagnostic here
  }
}

calling an async method from the parent class


class Parent {
  async returnsPromise(): Promise<string> {
    return 'value';
  }
}
class Child extends Parent {
  async someMethod() {
    this.returnsPromise().then(() => { }).finally(() => { }); // diagnostic here
  }
}

property method


class Test {
  returnsPromise: () => Promise<void>
  constructor() {
    this.returnsPromise = () => Promise.resolve();
  }

  async someMethod() {
    this.returnsPromise(); // diagnostic here
  }
}

functions


class Test {
  returnsPromiseFunction = async function (): Promise<string> {
    return 'value';
  }
  returnsPromiseArrowFunction = async (): Promise<string> => {
    return 'value';
  }

  async someMethod() {
    this.returnsPromiseFunction().then(() => { }).finally(() => { }); // diagnostic here
    this.returnsPromiseArrowFunction(); // diagnostic here
  }
}

calling an async method from outside

class Test {
  async returnsPromise(): Promise<string> {
    return 'value';
  }
}
const test = new Test();
test.returnsPromise(); // diagnostic here

Example of valid code:


class Test {
  returnsPromiseProperty: Promise<void>
  constructor() {
    returnsPromiseProperty = new Promise((resolve, reject) => { })
  }
  async returnsPromiseMethod(): Promise<string> {
    return 'value';
  }
  async someMethod() {
    await this.returnsPromiseMethod();
    this.returnsPromiseMethod().catch(() => {})
    await this.returnsPromiseProperty()
  }
}

@github-actions github-actions bot added A-Linter Area: linter L-JavaScript Language: JavaScript and super languages labels Feb 4, 2025
Copy link

codspeed-hq bot commented Feb 4, 2025

CodSpeed Performance Report

Merging #5028 will not alter performance

Comparing kaykdm:support-class-method (590769c) with next (7d5ae45)

Summary

✅ 94 untouched benchmarks

@kaykdm kaykdm force-pushed the support-class-method branch 4 times, most recently from 42789b0 to 62b4b6f Compare February 4, 2025 13:19
@kaykdm kaykdm force-pushed the support-class-method branch from 62b4b6f to 8994cb5 Compare February 5, 2025 11:24
@kaykdm kaykdm marked this pull request as ready for review February 6, 2025 01:08
Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

Thank you @kaykdm, the code looks good, however this checks only class declarations, and doesn't check for class expressions. We should add them

.ancestors()
.skip(1)
.find_map(|ancestor| {
if ancestor.kind() == JsSyntaxKind::JS_CLASS_DECLARATION {
Copy link
Member

Choose a reason for hiding this comment

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

A class can be initialised as an a declaration or an expression. This means we have to cover expressions too. They have a different node. Check the playground:

class A {}

const b = class {}

Copy link
Contributor Author

@kaykdm kaykdm Feb 9, 2025

Choose a reason for hiding this comment

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

I have added support for class initialized as an expression.

Also, I have added support for method calls from outside of the Class

class Test {
  async returnsPromise(): Promise<string> {
    return 'value';
  }
}
const test = new Test();
test.returnsPromise(); // diagnostic here

let js_this_expression = any_js_expression.as_js_this_expression()?;
let js_name = static_member_expr.member().ok()?;
let value_token = js_name.value_token().ok()?;
if !check_this_expression(js_this_expression, &value_token.to_string(), model)? {
Copy link
Member

Choose a reason for hiding this comment

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

Most of the times, you don't want to use to_string. You want to use methods like text_trimmed() or inner_string_text

to_string allocates a string.

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 replaced it with text_trimmed

class_member_list: JsClassMemberList,
target_name: &str,
) -> Option<AnyJsClassMember> {
class_member_list.into_iter().find(|member| match member {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
class_member_list.into_iter().find(|member| match member {
class_member_list.iter().find(|member| match member {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed!

@kaykdm kaykdm force-pushed the support-class-method branch from 1c8d3cf to df9db9f Compare February 8, 2025 11:40
@kaykdm kaykdm force-pushed the support-class-method branch from 5e2016d to a405360 Compare February 9, 2025 11:23
@kaykdm kaykdm force-pushed the support-class-method branch from 1768d6e to 3a7f181 Compare February 9, 2025 12:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Linter Area: linter L-JavaScript Language: JavaScript and super languages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants