feat(linter): typescript-eslint/no-floating-promises #2912
Conversation
|
OMG this is amazing, fingers crossed for communicating with the ts server, as discussed in #2855 |
CodSpeed Performance ReportMerging #2912 will degrade performances by 3.74%Comparing Summary
Benchmarks breakdown
|
|
@valeneiko Such an amazing effort! Feel free to ping me with preliminary results! |
|
@Boshen Almost there! It now successfully detects a floating Promise in a simple file: cargo run --bin oxlint -- --typecheck-plugin -A all -D no-floating-promises \
--tsconfig ./crates/oxc_linter/fixtures/typecheck/tsconfig.json \
./crates/oxc_linter/fixtures/typecheck/sample.tsasync function funcAsync() {
return Promise.resolve();
}
function func() {
return 7;
}
export async function test() {
func();
funcAsync();
}
|
|
I tested this on cal.com, and it's working on multiple files! It panic with I think it's due to providing utf8 span positions instead if utf16, we don't need to worry about this as there is already a tracking issue. Now onto performance: if performance of what we have right now is less or equal to @typescript/eslint, then we are all good! |
|
@valeneiko Can you time our setup and compare to one or two repositories with no-floating-promises on? https://github.com/search?q=no-floating-promises&type=code https://github.com/mermaid-js/mermaid seems like a good place to start: If the timing is similar for files less than a thousand, then we can start cleaning things up and merge things! |
|
@Boshen I have tried to run lint on the repo you linked, but it fails with the exact same error. And it's cause is not just encoding differences. Take a look at the AST of one of the files from the repo: OXC makes a request for Do you have any suggestions for how we can better map between the two? I am thinking we could try a full path or just child indexes, but now quite sure how to get this from OXC. On a side note, I finished the remaining parts of the lint rule. |
Ahh such a big blocker, where is typescript eslint do the conversion? It's probably in https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/typescript-estree/src/ast-converter.ts but I can't find it from a quick scan. I need to measure the effort of going forward with this experiment with we want to do the conversion. In the meantime, can you help out and just ignore the error / crash and let the whole thing complete so we can compare the total running time between typescript eslint and our approach? It's gonna be a lot of balancing of tradeoffs 🥵 |
Here is the conversion logic in typescript-eslint: https://github.com/typescript-eslint/typescript-eslint/blob/9c94b81b2560558fcc2d85dd644679a5dbf4a277/packages/typescript-estree/src/convert.ts#L757 If I understand it correctly, typescript eslint build their own AST by visiting the AST typescript produced.
Ok. I'll find a way to ignore those errors for now. |
Yeah I saw that too, but I can't find where it's changing the span positions :-(
Thank you! |
They don't need to. Instead of looking up nodes by spans, the just directly map the nodes, because they store which TS node they used to create each one.
I've pushed the commits to ignore the errors. And managed to run the rule against mermaid repo. I've modified their pnpm eslint --ext ".js,.ts,.jsx,.tsx" .OXC with that single rule takes ..\rust\oxc\target\release\oxlint.exe --typecheck-plugin -A all -D no-floating-promises
|
I agree, my guess is that typescript eslint is using some kind of caching technique or some API that we don't know about 😅 |
|
Result of running OXC in debug mode against mermaid with all the measurements: #: average / total
parse: 0.007ms / 0.225s
stringify: 0.019ms / 0.624s
open: 167.9ms / 66.672s
close: 0.320ms / 0.127s
isPromiseArray: 0.173ms / 2.703s
isPromiseLike: 0.014ms / 0.226s
isValidRejectionHandler: 0.039ms / 0.000s
getProgram: 0.015ms / 0.460s
getTypechecker: 0.000ms / 0.008s
getNode: 0.004ms / 0.114s
channelOverhead: 0.070ms / 2.261s
idle: 0.071ms / 2.283sA few observations:
|
I see these are optimizations that can be done later. Performance wise we are on the correct path, which is really good news 👍 . Now the last puzzle piece to solve is the mismatched AST nodes. Let's explore this for a few days before jumping to any conclusions. |
|
After looking at a few different examples, OXC and TS have both different structure and different spans. So mapping code is unavoidable. One idea comes to mind:
|
|
After exploring it a bit more, I don't think my previous idea would be enough. At this point the only solution I see is to fully map the AST. But given the effort required to maintain it I am not sure it would be worth it: we don't gain anything perf wise by having an ability to write these rules in rust. |
😢 It's sad to see that you have spent so much time and energy on this but it's blocked by how typescript works. TypeScript doesn't offer linter plugins, and we are forced to do this whole round trip despite us willing to do all these hard work, but we'll also need to maintain this whole round trip. I think at this point, we should just stop thinking about porting these rules from @typescript/eslint and start innovating our own set of rules that can help people catch bugs. I mean, it was tried before If anyone is going to ask again about typing rules, I'll just point them to this PR :-( So back to the drawing board. Since we have cross-file linting working with What do you think? |
Sound like a logical next step. It does feel like re-implementing TypeScript in Rust, but maybe we only need a very small subset to get most of the way. The complex part would be symbol resolution and I think we should start with that. I am not so sure about adding extra annotation to code, I would prefer enabling this rule instead: explicit-module-boundary-types. Then the cross-file step could easily mark if a function or method returns a promise. And even without the rule, most of those functions would be annotated with |
|
Let me organize this a bit tomorrow.
If the goal is still no-floating-promises, I think we can get it working first by marking exported async functions of a module as oxc/crates/oxc_syntax/src/module_record.rs Lines 67 to 68 in 07076d9
I meant marking things with extra annotations, there is a system in rust where you can mark things with different things in compile time, Now that you have mentioned it, maybe we can leverage isolated-declarations? And just do linting under isolated-declarations 🤔 We prepare for the future 😆 https://devblogs.microsoft.com/typescript/announcing-typescript-5-5-beta/#isolated-declarations |
Yes, this is exactly what I meant. But since that feature is not even out in TS,
That could be a good start. I was mostly concerned about complexity of doing it for cases other than named import of a function: // foo.ts
export function foo() {}
export const obj = { foo() {} };
export class Cls { foo() {} }
// -- default / namespace import
import mod from './foo';
mod.foo(); // <-- How do we resolve foo?
// -- object properties
import {obj} from './foo';
obj.foo(); // <-- How do we resolve foo?
// -- class fields
import {Cls} from './foo';
const x = new Cls();
x.foo(); // <-- How do we resolve foo? |
|
I feel said about closing this PR :-( Thank you so so so much for working on this, I learned a lot. Continue in #3105 (comment) |
#2855