Open rixtox opened 12 months ago
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.
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.
@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 eitherusing
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.
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.
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()
.
@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.
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 🙂
One reason of having the
Disposable
type is to support better static analysis on resource management. However, after we upgraded existing functions to return aDisposable
type, we now have two valid mechanisms to dispose the resource:Symbol.dispose
orSymbol.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: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?