How to lint legacy usage of new `Disposable` type?

[rixtox]

One reason of having the Disposable type is to support better static analysis on resource management. However, after we upgraded existing functions to return a Disposable type, we now have two valid mechanisms to dispose the resource:

1. Legacy explicit disposal mechanism.
2. Symbol.dispose or Symbol.asyncDispose.

It raises question on how can we apply generic linter rules to effectively detect resource management mistakes when having two mechanisms both being valid.

Take the following examples using the new node:fs Disposable APIs:

import { open } from 'node:fs/promises';

async function legacyUsage() {
    let filehandle;
    try {
        filehandle = await open('thefile.txt', 'r');
    } finally {
        await filehandle?.close();
    } 
}

async function newUsage() {
    await using filehandle = await open('thefile.txt', 'r');
}

async function leak() {
    const filehandle = await open('thefile.txt', 'r');
}

async function excessDispose() {
    await using filehandle = await open('thefile.txt', 'r');
    await filehandle.close();
}

The first two cases are valid, while the others are misuses. How can we design a generic linter to effectively detect the misuses without false positive on the legacy usage?

[ljharb]

You can't design such a thing in JS without type information - but for TS codebases, i assume eslint-plugin-typescript would be a natural place to suggest such a feature.

[bakkot]

The excess dispose is not obviously a problem. Most disposables, including node file handles, can safely be closed multiple times (with later calls being a no-op). using guarantees it will be closed at the end of the block, but it's legitimate to close it earlier.

Also, there's no reason to use an explicit try/finally to clean up a Disposable, so getting a lint warning in that case is fine.

As such, a lint warning for any use of a Disposable which is not either using or being passed into a DisposableStack would be sufficient. There are rare cases when users will want to suppress such a lint rule, but that's fine.

[rixtox]

@bakkot The excess dispose is not obviously a problem. Most disposables, including node file handles, can safely be closed multiple times (with later calls being a no-op). using guarantees it will be closed at the end of the block, but it's legitimate to close it earlier.

Although it's valid in this specific case, it's generally not a good idea to dispose something twice. This is also a challenge for linters to tell if it's safe to disposed a resource more than once. Or should we recommend all Disposable to safely handle multiple dispose?

Also, there's no reason to use an explicit try/finally to clean up a Disposable, so getting a lint warning in that case is fine.

As such, a lint warning for any use of a Disposable which is not either using or being passed into a DisposableStack would be sufficient. There are rare cases when users will want to suppress such a lint rule, but that's fine.

Then all legacy usage would get that warning. A bit annoying but manageable I guess.

[bakkot]

This is also a challenge for linters to tell if it's safe to disposed a resource more than once. Or should we recommend all Disposable to safely handle multiple dispose?

I don't think linters should try to warn you against disposing something multiple times, and I do think Disposables should safely handle multiple disposals (with the second being a no-op).

Yes, sometimes there will be Disposables which are in fact not safe to dispose multiple times, but linters don't need to catch every possible error.

Then all legacy usage would get that warning. A bit annoying but manageable I guess.

Personally, once I start using the new syntax and enabling lint rules and so on for it, I will want get get warnings for the legacy pattern - it's more verbose and error-prone. So I don't really see this as a problem.

[rbuckton]

In lieu of type information, a linter could at the very least have heuristics to recognize well-known host APIs that will produce disposables, such as fs.promises.open().

[rixtox]

@rbuckton In lieu of type information, a linter could at the very least have heuristics to recognize well-known host APIs that will produce disposables, such as fs.promises.open().

I'm extending your idea of type annotating used and unused Disposable types in this comment of yours (under "Linters and type checkers can help as well" section).

What you were describing sounded like a generic type annotation that can help Disposable type authors to provide type hinting for type checkers and linters to understand if a value has been used or not. It appears to me you were suggesting a generic linter for Disposable is possible with the annotated type info.

I'm expanding that thread of discussion to explore more on that idea.

[phryneas]

I've created a very basic lint rule (just warns on use of Disposable/AsyncDisposable without using/await using) as a part of https://github.com/apollographql/apollo-client/pull/11177 - maybe that's of use for someone here 🙂