feat(unstable): fast subset type checking of JSR dependencies #21873

dsherret · 2024-01-09T21:32:23Z

Integrates denoland/deno_graph#346 ("fast check")

This leads to a major speedup of type checking of remote JSR dependencies in Deno that are distributed as TypeScript by only providing TypeScript with the code relevant to the public API of dependencies.

Performance Improvement Example

// main.ts
import $ from "jsr:@dsherret/dax";
console.log($);

Before (no check cache):

> rm C:\Users\david\AppData\Local\deno\check_cache_v1 ; Measure-Command { deno check --all main.ts }
        1266.3429ms

After (no check cache):

> rm C:\Users\david\AppData\Local\deno\check_cache_v1 ; Measure-Command { ../deno/target/release/deno check --all main.ts } 
         590.5861ms

How this works

The public API is determined via a Rust-based TypeScript symbol graph built on top of swc.
The TypeScript source is then transformed to remove parts that are not relevant for the remote module being used as a dependency.
The transformed source is internally stored along with its source map for mapping diagnostic positions.
In the future, we'll cache this information so we don't need to recompute it (this is not implemented atm).

If the public API or a type in the public API cannot be determined, then it falls back to not using this feature and instead uses the full TypeScript sources.

Since this is something hidden away in the runtime, we can drop this feature at any time or provide overrides to disable it.

This feature is not implemented in the LSP atm, but in that case it would expand to the full source once opening a remote dependency (ex. doing "go to definition" on a symbol that ends up in a remote dep), but otherwise it would use the subset.

Example

Given the following source:

// https://.../package/mod.ts
import path from "jsr:@std/path";

export function join(a: string, b: string): string {
  console.log(a, b);
  return path.join(a, b);
}

function notUsed() {
  return Math.random();
}

We can strip out the non-relevant parts and end up with the following for type checking purposes:

export function join(a: string, b: string): string {
  return {} as any;
}

Why not transform to a declaration file?

There's no need and this way allows us to rely on TypeScript's inference in certain cases, especially in cases when TypeScript is going to be fast at inferring.
The semantics of declaration files are slightly different.
This keeps the specifier the same. We don't need to create some fake .d.ts file.

What about globals?

Globals can be defined in any file in TypeScript and if someone defined a global deep in their dependency then this wouldn't work. In this case, this feature only applies to JSR dependencies and we currently do not allow globals being defined in JSR deps. In the future we'll look at relaxing this restriction to likely require defining globals in a certain place. That would allow this to quickly check for globals there and maintain that code.

dsherret · 2024-01-09T23:26:55Z

cli/args/flags.rs

- .default_missing_value(".")
- .value_hint(ValueHint::DirPath)
- .required(true),
- )


Removed for now because it was broken. A lot of our code currently resolves stuff like the config file being resolved based on the cwd.

After some consideration I think we really need to have this soon - maybe as a quick-fix we could spawn subprocess in the correct CWD to it working that way?

It seems easy to just do (cd some_dir && deno publish)? Most people will be publishing from the directory they're in.

I don't think I even used it in a CWD, not a big deal though. We can address it in a follow up

If you remove this, you can’t publish single modules in a workspace anymore. That seems annoying

You can by not publishing from the root (instead you publish in the dir of the package)

I added a test demonstrating this: https://github.com/denoland/deno/pull/21873/files#diff-4d2dc43bd5ca3db03f1a2ab469b13e12e9305f4934b714950429687eb2f09973

@dsherret But this doesn't work if you have two deno.json (workspace level and package level), and you have import maps in both, and your package requires an import map entry from the workspace level one.

@lucacasonato if it doesn't then that's a bug there (not a limitation with not being able to provide a publish directory)

bartlomieju · 2024-01-10T00:30:51Z

cli/args/flags.rs

- .default_missing_value(".")
- .value_hint(ValueHint::DirPath)
- .required(true),
- )


After some consideration I think we really need to have this soon - maybe as a quick-fix we could spawn subprocess in the correct CWD to it working that way?

bartlomieju · 2024-01-10T00:31:59Z

cli/tests/testdata/jsr/registry/@denotest/subset_type_graph/0.1.0/mod.ts

+// this won't be type checked against because the subset
+// type graph will ignore it
+const invalidTypeCheck: number = "";
+console.log(invalidTypeCheck);


Could you describe the reason for different behavior between this...

I updated the comments for these. Let me know if the new comments don't explain it well enough.

That's good, thanks!

bartlomieju · 2024-01-10T00:32:08Z

cli/tests/testdata/jsr/registry/@denotest/subset_type_graph_invalid/0.1.0/mod.ts

+// this will be analyzed because the method above is missing an
+// explicit type which is required for the subset type graph
+const invalidTypeCheck: number = "";
+console.log(invalidTypeCheck);


...and this?

bartlomieju · 2024-01-10T00:38:15Z

cli/tsc/diagnostics.rs

+ // todo(dsherret): use a short lived cache to prevent parsing
+ // source maps so often
+ if let Ok(source_map) =
+ SourceMap::from_slice(&fast_check_module.source_map)
+ {


Could you do it in this PR by adding a cache in apply_fast_check_source_map? Or do you feel it's not gonna have much impact without it for now?

I opened #21877

bartlomieju · 2024-01-10T00:39:02Z

cli/tsc/mod.rs

@@ -586,7 +590,7 @@ fn op_resolve(
 let resolved_dep = graph
 .get(&referrer)
 .and_then(|m| m.esm())
- .and_then(|m| m.dependencies.get(&specifier))


Does this line applies fast check for all type checking in the CLI?

Yeah, only if it appears in the graph.

Wow, positively surprising that no other test failed!

This internally does if let Some(module) = &self.fast_check { &module.dependencies } else { &self.dependencies }.

bartlomieju · 2024-01-10T02:33:08Z

cli/args/flags.rs

- .default_missing_value(".")
- .value_hint(ValueHint::DirPath)
- .required(true),
- )


I don't think I even used it in a CWD, not a big deal though. We can address it in a follow up

bartlomieju · 2024-01-10T02:33:35Z

cli/tsc/mod.rs

@@ -586,7 +590,7 @@ fn op_resolve(
 let resolved_dep = graph
 .get(&referrer)
 .and_then(|m| m.esm())
- .and_then(|m| m.dependencies.get(&specifier))


Wow, positively surprising that no other test failed!

nayeemrmn

LGTM

dsherret and others added 30 commits December 11, 2023 17:20

Low res type checking

ce04c6c

Merge branch 'main' into low_res_type_checking

679aac7

Updates.

faa3429

Updates

0341be9

Update

45b8bbf

Fix

8497474

Make it work.

353e359

Allow checking when there's a single package in the workspace

edc4891

Filter out duplicate diagnostics.

f73a109

Type check low res graph on publish.

321ed86

Fix base.

33c2211

Updates for latest.

080b402

chore: update deno_lint

670dd70

Merge branch 'main' into low_res_type_checking

ea87214

Updates

4e4f780

Merge branch 'main' into low_res_type_checking

70fa791

Rename feature

2a58bc0

Source map the errors.

d0939b0

Merge branch 'main' into low_res_type_checking

db7052f

Latest updates

cd9f83f

Resolve todo about passing in fast_type_check

96f66b8

Remove commented out code

9fe7216

Merge branch 'main' into low_res_type_checking

d5a8bca

Merge branch 'main' into low_res_type_checking

4c06364

Updates.

6032385

Merge branch 'main' into update_deno_lint

14ae9b8

Updates.

9b415bd

Reset CI

3f791d0

Merge branch 'update_deno_lint' into low_res_type_checking

ff00746

Fix compiler errors

5f8e238

dsherret added 5 commits January 9, 2024 15:11

Update

d94ecfb

Add test

5439510

Add test

b4d441e

Fix related information

8143bde

Merge branch 'main' into low_res_type_checking

a2348ca

dsherret commented Jan 9, 2024

View reviewed changes

Add another test

07bd095

dsherret requested review from bartlomieju and nayeemrmn January 9, 2024 23:33

Fix lsp errors

6c20e3d

bartlomieju reviewed Jan 10, 2024

View reviewed changes

dsherret mentioned this pull request Jan 10, 2024

Fast check - Add source map cache #21877

Closed

Fix issues and review feedback

0816b6f

dsherret requested a review from bartlomieju January 10, 2024 01:54

bartlomieju approved these changes Jan 10, 2024

View reviewed changes

nayeemrmn approved these changes Jan 10, 2024

View reviewed changes

dsherret added 2 commits January 10, 2024 16:41

Support JS files

42714f2

Add test for workspace publishing and individual publishing

56b578a

dsherret enabled auto-merge (squash) January 10, 2024 22:15

dsherret merged commit 70ac061 into denoland:main Jan 10, 2024
14 checks passed

dsherret deleted the low_res_type_checking branch January 10, 2024 22:50

bartlomieju pushed a commit that referenced this pull request Jan 12, 2024

feat(unstable): fast subset type checking of JSR dependencies (#21873)

b5e70c5

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(unstable): fast subset type checking of JSR dependencies #21873

feat(unstable): fast subset type checking of JSR dependencies #21873

dsherret commented Jan 9, 2024 •

edited

Loading

dsherret Jan 9, 2024

bartlomieju Jan 10, 2024

dsherret Jan 10, 2024

bartlomieju Jan 10, 2024

lucacasonato Jan 10, 2024

dsherret Jan 10, 2024 •

edited

Loading

dsherret Jan 10, 2024

lucacasonato Jan 11, 2024

dsherret Jan 12, 2024 •

edited

Loading

bartlomieju Jan 10, 2024

bartlomieju Jan 10, 2024 •

edited

Loading

dsherret Jan 10, 2024

bartlomieju Jan 10, 2024

bartlomieju Jan 10, 2024

bartlomieju Jan 10, 2024

dsherret Jan 10, 2024

bartlomieju Jan 10, 2024

dsherret Jan 10, 2024

bartlomieju Jan 10, 2024

dsherret Jan 10, 2024

bartlomieju Jan 10, 2024

bartlomieju Jan 10, 2024

nayeemrmn left a comment

feat(unstable): fast subset type checking of JSR dependencies #21873

feat(unstable): fast subset type checking of JSR dependencies #21873

Conversation

dsherret commented Jan 9, 2024 • edited Loading

Performance Improvement Example

How this works

Example

Why not transform to a declaration file?

What about globals?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsherret Jan 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsherret Jan 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bartlomieju Jan 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nayeemrmn left a comment

Choose a reason for hiding this comment

dsherret commented Jan 9, 2024 •

edited

Loading

dsherret Jan 10, 2024 •

edited

Loading

dsherret Jan 12, 2024 •

edited

Loading

bartlomieju Jan 10, 2024 •

edited

Loading