Open Bug 1740290 Opened 4 years ago Updated 23 days ago

Consider using SCIP for JS indexing and to benefit from typescript / tsserver inference process

Categories

(Webtools :: Searchfox, enhancement)

Product:
Webtools
Component:
Searchfox
Type:
enhancement

Tracking

(Not tracked)

People

(Reporter: asuth, Unassigned)

References

Details

Converting a Jun 14, 2021 brief investigation into a bug:

I think it likely makes sense to try and move our JS indexing to using the typescript backend used by VS Code, and there's an interesting effort to create a "language server index format" (https://lsif.dev/) which could potentially be used as an easy way to help searchfox ingest languages it doesn't currently do a great job on, with https://github.com/sourcegraph/lsif-node being the JS/TS one.

(Presumably searchfox could do a better job in various places by having a better awareness of path mapping after we do the URL resolution stuff Nika had proposed, etc.)

It sounds like VS Code directly talks to tsserver https://github.com/Microsoft/TypeScript/wiki/Standalone-Server-%28tsserver%29 but there are language server wrappers like https://github.com/theia-ide/typescript-language-server and the lsif-node thing

https://microsoft.github.io/language-server-protocol/specifications/lsif/0.5.0/specification/ has some good examples of what the LSIF data output can look like, which seems like something that could be nicely re-processed as part of a searchfox pipeline.

...

It looks like a transformer (ex: https://github.com/longlho/ts-transform-system-import) (requires custom compiler wrapper per https://github.com/microsoft/TypeScript/issues/14419) or just preprocessing things ourselves could let us rewrite things such that:

  • const { foo, bar } = ChromeUtils.import("resource:///modules/Blah.jsm"); becomes import { foo, bar } from "RESOURCE/modules/Blah.jsm"
  • We establish a tsconfig/jsconfig paths mapping so that those URLs all properly map to the source paths.
  • We re-write const EXPORTED_SYMBOLS = ["foo", "bar"]; to export { foo, bar }.
See Also: → 1761287
See Also: → 1761627
See Also: → 1536835
Depends on: 1775130

We would now use https://github.com/sourcegraph/scip-typescript instead of lsif-node since it provides more information and because of the synergy from :emilio implementing SCIP support in bug 1761287.

As part of this change we would likely:

  • Change from our symbol soup model to something akin to my proposal in bug 1499066.
    • Right now, any definition of a function foo will be mapped to #foo. A class method SomeClass.foo will also be mapped as #foo and #SomeClass.foo. However, thanks to imports/exports and the analysis typescript will already be doing, it's possible for us to do significantly better since we can know the actual imported file.
    • In bug 1775130 I also propose that we can use the work already done with eslint to better support JS "script" files where the work done to understand what globals are available can help us do similar resolution. The one complication is that "script" JS files are more like mix-ins which means that a given token may actually be referring to multiple distinct symbol definitions because of the different contexts in which the script is evaluated. For example, we have IndexedDB tests that are run under both xpcshell and mochitests, and so any given helper invocation will actually be referencing 2 potential different implementations.
  • Be able to support JSX as typescript seems to optionally support JSX.
Depends on: 1761287
See Also: 1761287 → 1499066
Summary: Consider using LSIF (Language Server Index Format) for JS indexing and to benefit from typescript / tsserver inference process → Consider using SCIP for JS indexing and to benefit from typescript / tsserver inference process

I use valid TypeScript JSDoc in my JavaScript code in Gecko, so that I can leverage the in-editor hints. I'm guessing if you are using the typescript language server that these would get picked up by this system. The next step from there would be to type check the results to see if they are accurate.

(In reply to Greg Tatum [:gregtatum] from comment #2)

I use valid TypeScript JSDoc in my JavaScript code in Gecko, so that I can leverage the in-editor hints. I'm guessing if you are using the typescript language server that these would get picked up by this system. The next step from there would be to type check the results to see if they are accurate.

Yeah, these get picked up when they're syntactically correct[1], but at least as exposed in scip-typescript right now, the net result unfortunately isn't particularly useful.

1: One of my experiments was to use the devtools/ subtree, but while there are some places where the syntax is right (ex: @param {Debugger.Object} object), there are also a ton of cases where the syntax is wrong (ex: @param Debugger.Environment aEnvironment).

I came up with a candidate plan about introducing a concept of symbol confidences in my review comment at https://github.com/mozsearch/mozsearch/pull/628#issuecomment-1548575643 that I think could be useful as a mechanism where could incrementally migrate towards using scip-typescript for everything. It would allows us to potentially run both the existing "classic" JS analyzer on everything plus "scip-typescript" on things that it adds something for, and the UI could potentially surface the differences in quality in a prioritized fashion. That said, there would probably also be a lot of upside to moving to finishing up the MVP of the "query" UI and its faceting, as path and/or subsystem faceting would also go a long way to letting people filter out things they're not interested in.

I've looked at this a little recently. I ran https://github.com/sourcegraph/scip-typescript/issues/409 which was limiting the usefulness that I was able to get from running scip-typescript on the components/places. From my current understanding of how scip-typescript works solving this issue may not be easy. That being said, this limitation is caused by our use export function OpenedConnection() {}; OpenedConnection.prototype = { someMethod() {} } classes. If we use JS classes, scip-typescript is able to properly figure out references and definitions.

If we were to convert a bunch of JS to uses classes instead of manual prototype assignment, I think scip-typescript could provide good value out of the box.

That's fantastic news, thanks for looking into the underlying situation more deeply.

Looking at the "mach ts check" command it seems like it is expected for the "tsconfig.json" files to basically be islands. But all of these islands seem to have a chain where they explicitly "extends" tsconfig.json which "extends" tspaths.json which sets a baseUrl which ensures that all of these projects share the same "base directory from which to resolve bare specifier module names".

I think this means we can potentially do something like my initial speculative approach to have us run "scip-typescript --infer-tsconfig" in every dir we found a package.json in but where I think our scip-indexer output would then generate symbols in the same namespace (as long as we do not specify a subtree_name or only specify the same subtree_name; we don't want extra partitioning there).

  • This could potentially allow us to parallelize the runs (assuming a single run doesn't self-parallelize somehow; it seems like things would have to serialize at some point), but more importantly, help reduce/isolate failures. (Although I expect scip-typescript may be less prone to falling into apparent infinite loop traps than scip-python.)
  • We have consensus that "scip_subtrees" was a bad idea (brief discussion with emilio). Since we potentially need to deal with preprocessed JS files, it's possible this just wants to be a per-platform processing pass which means it's going to be largely custom to gecko no matter what. So it might not make sense to try and build tooling to make the automatic mkindex.sh JS indexing work with scip-typescript off the rack, but if we did, I think there's potential to do a bit more in find-repo-files.py. We could treat tsconfig.json files (and possibly package.json files) as claiming indexing ownership of any js/ts files under their subtree, leaving only "loose" JS files for our jsshell-based js-analyzer indexer logic. But a first step could be to have js-analyze skip any JS files that it finds already have analysis files resulting from the gecko per-platform/merge process. That would also provide fallback if scip-typescript broke on a given subtree.
  • searchfox search for tsconfig.json, package.json, and tspaths.json files in m-c.

It'd probably make sense to try with scip-typescript and js-analyze as disconnected islands first and then evaluate how bad the UX breakage is before looking at merging both JS indexers. In the past when we'd have tons of JS in XUL and HTML files, it would be horrible, but if we've gotten rid of most inline JS, things might not be too bad to begin with. The obvious exception would be (plain mochi)tests where in bug 1775130 I was hoping the work we'd already done for eslint could help us understand the global situations better there for free-ish. That might be something where the best answer is to have searchfox's (js-analyze-based) HTML analysis or data scraped from code-coverage runs let us know all of the sets of globals we see and the JS files tat get loaded into those global and use those to transform each HTML page load into a JS-file we can feed to scip-typescript to get precise information, then map/merge that back.

I tried running scip-typescript index against toolkit/components/places and was met with an immediate error:

../../../tools/@types/tsconfig.json(6,15): error TS6046: Argument for '--target' option must be: 'es5', 'es6', 'es2015', 'es2016', 'es2017', 'es2018', 'es2019', 'es2020', 'es2021', 'es2022', 'es2023', 'esnext'.

This is fixed by simply updating scip-typescript's typescript version: https://github.com/sourcegraph/scip-typescript/pull/449.

Here's a list of special handlings done in js-analyze.js, which may need to be ported to the scip-typescript workflow, maybe as a postprocess, or perhaps mergging the results between them:

Thanks for the pointers! I'll check each of these points to see how they are/can be supported by scip-typescript.

I started getting familiar with the way scip-typescript works internally by attempting to fix the issue pointed out by :jrmuizel above about prototype property assignments: https://github.com/sourcegraph/scip-typescript/pull/450. Let's see what upstream thinks about it.

Depends on: 2053875
You need to log in before you can comment on or make changes to this bug.