sourcegraph / scip

SCIP Code Intelligence Protocol
Apache License 2.0
260 stars 32 forks source link

TS bindings break on .scip files generated by rust-analyzer #274

Open ephraimrothschild opened 2 months ago

ephraimrothschild commented 2 months ago

I don't know if this should be here, or in the rust-analyzer repo, but something appears incompatible between the two of them. After generating an index using rust-analyzer scip . (using any version of rust-analyzer, and any rust repo), the Typescript bindings fail to parse that index. For example, the following tsx script:

#!/usr/bin/env tsx
import {Command} from 'commander';
import {scip} from './scip';
import * as fs from 'fs';

const program = new Command();

program
  .version("1.0.0")
  .description("")
  .parse(process.argv);

const args = program.args;
const file = fs.readFileSync(args[0], null);
const full_index = scip.Index.deserializeBinary(file);

Running ./testscript.ts ./index.scip Will cause the following error:

/scripts/node_modules/google-protobuf/google-protobuf.js:13
function ra(a,b,c){return 2>=arguments.length?Array.prototype.slice.call(a,b):Array.prototype.slice.call(a,b,c)};function sa(a,b,c,d){var f="Assertion failed";if(c){f+=": "+c;var h=d}else a&&(f+=": "+a,h=b);throw Error(f,h||[]);}function n(a,b,c){for(var d=[],f=2;f<arguments.length;++f)d[f-2]=arguments[f];a||sa("",null,b,d);return a}function ta(a,b,c){for(var d=[],f=2;f<arguments.length;++f)d[f-2]=arguments[f];"string"!==typeof a&&sa("Expected string but got %s: %s.",[k(a),a],b,d)}
                                                                                                                                                                                                                     ^

Error: Assertion failed
    at sa (/scripts/node_modules/google-protobuf/google-protobuf.js:13:214)
    at n (/scripts/node_modules/google-protobuf/google-protobuf.js:13:311)
    at L (/scripts/node_modules/google-protobuf/google-protobuf.js:74:362)
    at J.gc (/scripts/node_modules/google-protobuf/google-protobuf.js:74:484)
    at Function.deserialize (/scripts/scip.ts:1734:48)
    at <anonymous> (/scripts/scip.ts:726:133)
    at J.Yb (/scripts/node_modules/google-protobuf/google-protobuf.js:67:146)
    at Function.deserialize (/scripts/scip.ts:726:32)
    at <anonymous> (/scripts/scip.ts:298:129)
    at J.Yb (/scripts/node_modules/google-protobuf/google-protobuf.js:67:146)

Node.js v20.16.0

This does not seem to happen with other indexers (eg running this on an index.scip file generated from scip-python, and scip-java do not result in this error, but this occurs no matter which version of rust-analyzer is used). The scip.ts file used is copied directly from sourcegraph/scip/bindings/scip.ts

varungandhi-src commented 2 months ago

The SCIP CLI written in Go is able to parse the file just fine, and the generated code is not that complex, so I suspect it's likely a bug in one of:

  1. The bundler/configuration.
  2. Protobuf

E.g. I found this issue which has a similar flavor. https://github.com/webpack/webpack/issues/9210

Can you provide a minimal repo with appropriate package.json etc to reproduce? It would also be helpful to provide a stack trace that doesn't involve minified code.

olafurpg commented 2 months ago

Thank you for reporting! What version of google-protobuf are you using? The stack trace does not seem seem related to the scip.ts file itself. I recall hitting on runtime errors in google-protobuf when working on scip-typescript that I resolved by downgrading (can't remember exact version, and didn't find the related commit after a brief search). It might be worth trying a few different versions so see if that works around the issue.

An alternative workaround is to experiment with protoc-gen-es instead of protoc-gen-ts. I pushed a branch that generates the code here https://github.com/sourcegraph/scip/compare/olafurpg/connect-es?expand=1 I think @bufbuild/protobuf is the only runtime dependency. I have not used connect-es myself but it's been on my TODO list to evaluate it as a google-protobuf alternative.

connor4312 commented 2 months ago

I came here to report this as well -- I ran into the same issue, which persisted after updating google-protobuf to its latest version. Here's a minimal repro using an index of this repo: https://github.com/connor4312/scip-typescript-bindings/blob/repro/test.js. I observed the same failure in indicies generated by scip-go as well.

There was also a new version of protoc-gen-ts available, but I didn't try regenerating the bindings with that version because I didn't want to bother instaling protoc on my device.

ephraimrothschild commented 2 months ago

@olafurpg I tried a few different versions of google-protobuf, but I created an example of the error using the latest version (3.21.2) here: https://github.com/ephraimrothschild/example-scip-ts-error

You can reproduce the error by running docker compose run --build example, and then from that command line running either /scripts/example_working.sh or /scripts/example_not_working.sh

example_working creates a scip file with scip-python, and then tries to parse it with the TS bindings, and example_not_working creates a scip file with rust-analyzer and then tries to parse it in exactly the same way and fails.

olafurpg commented 2 months ago

@ephraimrothschild thank you for the minimized reproduction! That is very helpful.

I'm able to reproduce the error with google-protobuf but I can successfully deserialize the index with @bufbuild/protobuf using @connectrpc/protoc-gen-connect-es as the code generator. Minimized reproduction with @bufbuild/protobuf https://github.com/ephraimrothschild/example-scip-ts-error/commit/120e816e5a6667a41f718cbfa35efa1864f6c0e5. This indicates the root bug is in google-protobuf.

I've dealt with enough cryptic errors from google-protobuf that I'm leaning towards using @bufbuild/protobuf instead for new projects going forward. If switching helps resolve your issue then I propose we update the provided SCIP TypeScript bindings to use protoc-gen-connect-es instead of protoc-gen-ts

ephraimrothschild commented 2 months ago

@olafurpg - Can confirm that your proposed changes in 120e816 fixed the error for my use case. Thanks for the help!!