denoland / deno_lint

Blazing fast linter for JavaScript and TypeScript written in Rust
https://lint.deno.land/
MIT License
1.51k stars 163 forks source link

upgrade no-sync-fn-in-async-fn #1198

Open sigmaSd opened 9 months ago

sigmaSd commented 9 months ago

currently no-sync-fn-in-async-fn can't catch this:

async function a() {
   b() // b is blocking, but its hidden from us, we only warn if we see Deno. Sync syntax
}

function b() {
 Deno.xxxxxSync()
}

I thought maybe its possible to catch even this: for example we can consider each function that have inside it a deno Sync api a blocking function, and save it on a list and now each time we see it we also consider the parent blocking and so on, and now if we see one of these in an async context we emit a warning.

Do you think this is a reasonable idea ?

sigmaSd commented 9 months ago

I wrote the code for this, check it out here https://github.com/denoland/deno_lint/compare/main...sigmaSd:lintup?expand=1

It seems I hit a fatal flow unfortantnly

this gets linted correctly

     function foo() {
        Deno.readTextFileSync("");
      }"
      async function foo2() {
        foo()
      }

but this doesn't

      async function foo2() {
        foo()
      }
     function foo() {
        Deno.readTextFileSync("");
      }"

because the parsing is done from top to bottom, when we reach foo inside foo2 we don't know its a blocking call yet.

Any ideas ? can I somehow parse twice ?

sigmaSd commented 9 months ago

Ofc I can just traverse the ast twice.

bartlomieju commented 1 month ago

@sigmaSd yes, you can traverse the AST twice in this specific rule

sigmaSd commented 1 month ago

Thanks @bartlomieju I already do that in this pr https://github.com/denoland/deno_lint/pull/1199#issuecomment-1742322101

It seems to work but I'm not really confortable with the code base, would be great if you review it