Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
ephraimrothschild opened this issue Aug 5, 2024 · 6 comments
Open

Comments

@ephraimrothschild
Copy link

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
Copy link
Contributor

varungandhi-src commented Aug 5, 2024

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. webpack/webpack#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
Copy link
Member

olafurpg commented Aug 5, 2024

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
Copy link

connor4312 commented Aug 6, 2024

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
Copy link
Author

@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
Copy link
Member

olafurpg commented Aug 7, 2024

@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 ephraimrothschild/example-scip-ts-error@120e816. 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
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants