📎 Implement nursery/noFloatingPromises - typescript-eslint/no-floating-promises

[kaykdm]

Description

The rule will cover no-floating-promises

Some context here

Feature support / tasks checklist

• support regular async functions

  async function returnsPromiseFunction() {
      return 'value';
  }
  returnsPromiseFunction();
  

• assigned functions

  const returnsPromiseAssignedArrowFunction = async (): Promise<string> => {
    return 'value'; 
  }
  returnsPromiseArrowFunction();
  
  const returnsPromiseAssignedFunction = async function (): Promise<string> {
    return 'value'; 
  }
  returnsPromiseAssignedFunction();
  

• Promise

  Promise.all([]);
  Promise.reject('value').catch();
  
  const promise = new Promise((resolve, reject) => resolve('value'));  
  promise;
  

• Class methods

  class Api {
      async returnsPromise(): Promise<string> {
          return 'value';
      }
  
      async someMethod() {
          this.returnsPromise();
      }
  }
  
  const api = new Api();
  api.returnsPromise();
  

• Object method

  const obj = {
      async returnsPromise() {
          return 'value';
      }
  };
  obj.returnsPromise();
  

• Multi-file analysis - after this is implemented
• Promise in function arguments

  // Annotated with Type (Using type Alias)
  type PropsType = {
    returnsPromise: () => Promise<void>;
  };
  function test({ returnsPromise }: PropsType) {
    returnsPromise();
  }
  
  // Annotated with Interface
  interface PropsInterface {
    returnsPromise: () => Promise<void>;
  }
  function test2({ returnsPromise }: PropsInterface) {
    returnsPromise();
  }
  
  // Annotated Directly (Inline Annotation)
  function test3({ returnsPromise }: { returnsPromise: () => Promise<void>; }) {
    returnsPromise();
  }
  

[arendjr]

Other important bullet points to add would be “inferred types” and “derived types”.

The first refers to working with functions that don’t have an explicit return type annotation, and are contextual inference. After you support assigned functions and promises, you’re already partly there, but you also need to consider control flow within a function to determine the possible return values.

Derived types are where the TypeScript type system gets really funky, and you need to support all the generics handling and other way to create types from types. There’s a lot of libraries that do stuff like that, and if you invoke methods on such types we’d also like to detect if they return a promise.

Both can get quite complex, but I expect I may start work on those sooner rather than later. I think we can coordinate by adding subtasks to this issue.

[kaykdm]

@arendjr

Hello,

I’m planning to implement support for multi-file analysis in noFloatingPromises, and I’ve been looking into the DependencyGraph implementation.

As far as I understand, DependencyGraph only exposes the resolved path of an import. However, in the noFloatingPromises rule, I need access to the AST of the imported file to check whether an imported expression is a promise.

My current approach is as follows:

1. Retrieve the resolved path from DependencyGraph.
2. Use the parse() function from biome_js_parser to parse the file.
3. Traverse the AST to find the imported function.

Here’s a rough implementation of this idea:

let import = imports.static_imports.get(specifier)?;
let resolved_path = import.resolved_path.ok()?;
let source_text = read_to_string(resolved_path).ok()?;
let root = parse(
    source_text.as_str(),
    JsFileSource::ts(),
    JsParserOptions::default(),
);
// Traverse `root` to find the imported expression

Does this seem like a valid approach? Or is there a better way to achieve this?

[arendjr]

@kaykdm Awesome! Good to hear progress is going well!

As it happens, I'm just in the process of extending the dependency graph to become a little bit smarter. It's still very much work-in-progress, but for transparency I just pushed my WIP branch here: https://github.com/biomejs/biome/compare/main...arendjr:no-private-imports?expand=1

The idea is that it doesn't merely resolve the paths any more, but it should also be able to resolve to specific symbols within the target files. However, as I was working on this, I also realised that what I'm doing is slightly naive: With a single pass I can resolve to the export statements, but not necessarily to the definition of the symbols themselves. That's sufficient for the noPrivateImports rule I'm currently working on (which will be an evolution of noPackagePrivateImports), but it would be insufficient for many of the cases you'd want to catch with the noFloatingPromises rule. Simply put: You'd be able to resolve export async function foo() {}, but not async function foo() {}; export { foo };.

Just yesterday, I created another issue to tackle that, although @ematipico made some good comments on Discord for why the suggestion I made in that issue might not be exactly what we want. In other words, it's still under consideration how exactly we wish to proceed.

As you can probably guess at this point, reading the files pointed to by the dependency graph and parsing them, is not how we want to proceed. The main reason being that we would have to parse the same files over and over.

And to add to all of that: Once the Biome 2.0 beta is out (which should happen quite soon now), I also want to shift my attention to work on proper type inference for Biome. That's going to be a major undertaking, but it is very much connected to all these pieces we're discussing here, and it will directly influence the future direction of the noFloatingPromises rule as well.

So I think the point you've reached is that you've hit the very forefront of what we can do with our multi-file analysis today :) But I've been following your progress with great interest, and things are moving very much in a direction we'd like to continue. Right now, these are the options I see:

• You could take my WIP branch, add the info you need for finding Promise-returning functions and methods to the OwnExport type I added and work from there. We'll probably get a bit of a merge conflict, but hopefully nothing too major. The real issue is that as long as 📎 Move semantic model to scanner #5312 isn't tackled, you won't be able to resolve export { foo } statements to the definition of foo.
• So alternatively, you can work on 📎 Move semantic model to scanner #5312 instead.
• You can also look ahead and explore other areas of type inference already. I hope to get there soon enough, but more eyes certainly wouldn't hurt. It would be especially valuable to see how we can update noFloatingPromises to reason based on a type model instead of the more ad-hoc AST queries it currently uses.
• Or of course, you could just wait for the dust to settle a bit :)

Feel free to discuss here, or on Discord. Thanks for your efforts so far!

[arendjr]

Another update, I've created a PR based on the WIP branch I linked above: #5329

Once this is merged, it should allow you to continue some of the work on top of that without risking merge conflicts at least :)

[kaykdm]

@arendjr
Thanks for the quick response!

add the info you need for finding Promise-returning functions and methods to the OwnExport type I added and work from there

I'll look into this and see if it's possible to implement a basic export/import check for noFloatingPromises.
As for the other options, I likely won’t have enough time to take them on, so I think it’s best not to commit to those for now.