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

Cross-language API/IDL support (GraphQL, Thrift, Protobuf) #981

Open
sqs opened this Issue Nov 13, 2018 · 13 comments

Comments

Projects
None yet
5 participants
@sqs
Copy link
Member

sqs commented Nov 13, 2018

We will be adding support for cross-language API/IDL systems to Sourcegraph, in order of priority:

  • GraphQL (partially done)
  • Thrift
  • Protobuf

You will be able to see hovers with documentation, go to definition, find references, etc., in API/IDL definition and code that is generated from them.

Use cases

  • As a developer who is consuming a service, I want to see usage examples of a service's method so I can quickly see how to use it.
  • As a developer who is consuming a service, I want to see values of a service's type so I can quickly see how to construct such a value.
  • As a developer who is consuming a service, I want to go to the implementation of a service's method to debug an issue I encountered when my client is calling the service.
  • As a developer who maintains a service, I want to see call sites of a service's method to understand how it's being used (for future improvement planning, backcompat/deprecation, capacity planning, and identifying common mistakes).

What makes this hard today:

  • The IDL schema, service, and clients are usually scattered in different locations (or even different repositories), so it's not always clear where to start looking.
  • The IDL schema and service are often maintained by a different team than the clients, so there is no single person who necessarily knows the answers.
  • The IDL schema is usually written in a custom language that has poor or uneven tooling support (.graphql, .thrift, .proto).
  • Service method and type names are usually not globally unique (e.g., Lookup or Delete), so text search yields many false positives.
  • The service and clients are sometimes written in different programming languages, so:
    • Code search wouldn't necessarily find all hits (e.g., if the names are inflected differently in generated code)
    • Code intelligence for a single language wouldn't find all references or definitions
  • Generated code is often used in these scenarios, and it may be:
    • Complex (and its derivation from the original schema may not be obvious)
    • Not checked into source control (so code search would not find it and code intelligence would not work on it)
  • Often it's not possible to know (from the code alone) which service version a client is accessing.

@sqs sqs added this to the 3.0 milestone Nov 13, 2018

@chrismwendt

This comment has been minimized.

Copy link
Contributor

chrismwendt commented Nov 27, 2018

@tsenart I worked on a GraphQL extension a few weeks ago, and the trickiest part was figuring out which GraphQL package to use and how to use it in a browser context. I ended up writing a server component that used the package @playlyfe/gql for analysis (which internally uses the GraphQL language service) and created a tiny HTTP API for it.

The extension code itself might not be useful for cross-language code intelligence because I built the extension only with the aim of supporting single-file code intelligence.

If you end up writing a server-side component that speaks WebSockets, check out how I did this in lang-go. Also make sure you use @felixfbecker's fork of vscode-ws-jsonrpc which fixes a problem with the build that's present in upstream's 0.0.3.

@tsenart

This comment has been minimized.

Copy link
Contributor

tsenart commented Nov 28, 2018

The extension code itself might not be useful for cross-language code intelligence because I built the extension only with the aim of supporting single-file code intelligence.

@chrismwendt: I believe that's the reason why @nicksnyder recommended that we start from scratch and try a search driven heuristics approach inspired by https://github.com/sourcegraph/sourcegraph-basic-code-intel.

Does that make sense to you? I might have missed some context.

@sqs

This comment has been minimized.

Copy link
Member Author

sqs commented Nov 28, 2018

@tsenart Yes, what you said is correct.

@tsenart

This comment has been minimized.

Copy link
Contributor

tsenart commented Nov 29, 2018

Today's search for good heuristics to map GraphQL symbols to their corresponding programming language counterparts has proved unfruitful due to the lack of standards around code generation.

Unlike Thrift or Protobuf, GraphQL doesn't have useful restrictions and conventions that can inform where to look for associated generated code. Without unifying universal conventions, this leaves the solution space for GraphQL code intelligence restricted to search based on configurable heuristics per project.

To require a user to configure these search heuristics makes for poor UX in my opinion. The time and effort the user would have to expend in figuring those out for their own project would perhaps be better spent just using search directly to get to the answers they'd be seeking.

Thrift and Protobuf, however, lend themselves to a more robust solution to a more tractable problem. Essentially, the idea is to leverage the code generation conventions that exist to know where to search for associated generated code first. Highly selective search filters include comment headers in generated code files such as these:

// Autogenerated by Thrift Compiler (1.0.0-dev)
// DO NOT EDIT UNLESS YOU ARE SURE THAT YOU KNOW WHAT YOU ARE DOING

package addsvc

Together with a known mapping of language to code generation rules (name inflections, scoping, namespaces, etc), we can get fairly precise symbol search results across multiple repos, which would be used to then find references and external references with the appropriate language server / extension.

Properly optimized, the user experience of this solution should be good, as far as I can tell. I'd love to get your thoughts on it before proceeding. If you agree, Thrift seems to be higher priority due to the customer need for it, so I'd pursue that first.

Projects that don't check-in the generated code would not be supported. The recommended tool for satisfying the above use cases in those scenarios would be the same as GraphQL: to use search directly.

Thoughts?

@tsenart

This comment has been minimized.

Copy link
Contributor

tsenart commented Nov 29, 2018

Addendum to the previous comment: This approach would require each language extension to register a command that could be invoked by other extensions to retrieve references.

In summary, these would be the steps involved:

  1. For a hovered or clicked symbol, search across repos for associated generated code using the pre-defined highly selective search terms.
  2. For each result, ask the corresponding language extension for references.
  3. Display all aggregated references.
@sqs

This comment has been minimized.

Copy link
Member Author

sqs commented Nov 29, 2018

@tsenart Prioritizing Thrift instead of GraphQL makes sense. As a next step, you should write up a few bullet points that specify “what must be true of the code” for this to work, such as: generated code must be checked in, languages supported are Go/Java/Python/JavaScript (and list the code gen tools that are supported, if there’s more than 1 in wide use for a language), and any other details (eg what version of protobuf if that matters). Then we can share that with customers and confirm it will work for them. (You can just post that as a reply here.)

For each result, ask the language extension for references

This will be possible soon (blocked on #1120 and a little bit more glue code for extensions being able to talk to each other), but it is a significant undertaking, so you should get to an earlier milestone that finds non-precise matches (eg using text search).

So you should write up the above bullet points for both milestone 1 (solving this non-precisely with search) and milestone 2 (solving this precisely with actual language server references).

How does that sound?

@tsenart

This comment has been minimized.

Copy link
Contributor

tsenart commented Nov 29, 2018

Prioritizing Thrift instead of GraphQL makes sense.

👌

As a next step, you should write up a few bullet points that specify “what must be true of the code” for this to work (...)

Sounds good. I'll write down the scope, constraints and pre-requisites that this solution would work with.

Then we can share that with customers and confirm it will work for them. (You can just post that as a reply here.)

👍

This will be possible soon (blocked on #1120 and a little bit more glue code for extensions being able to talk to each other), but it is a significant undertaking, so you should get to an earlier milestone that finds non-precise matches (eg using text search).
So you should write up the above bullet points for both milestone 1 (solving this non-precisely with search) and milestone 2 (solving this precisely with actual language server references).
How does that sound?

As a Sourcegraph user, I expect clicking "Find References" to be an exact operation, not a fuzzy one with potential for false positives. For common names like the ones above (e.g Lookup), finding references with search will not meet those expectations. I would personally find that very surprising.

Do you think we wouldn't have #1120 ready in time for us to implement this properly for the 3.0 release? If not, then my question would be how could we make it explicit in the UI of reference results that the results are fuzzy, as to not break the previously mentioned expectations.

@sqs

This comment has been minimized.

Copy link
Member Author

sqs commented Nov 29, 2018

As a Sourcegraph user, I expect clicking "Find References" to be an exact operation, not a fuzzy one with potential for false positives. For common names like the ones above (e.g Lookup), finding references with search will not meet those expectations. I would personally find that very surprising. ... how could we make it explicit in the UI of reference results that the results are fuzzy, as to not break the previously mentioned expectations.

Agree we must set expectations correctly here, and having a way to do so would be valuable for many things. I just filed #1174 for a UI to mark results as imprecise. That would not be hard to implement, so you can assume such a solution will exist soon. In the meantime, the extension you build would be opt in and we can manually explain the imprecision/limitations to the initial users.

Do you think we wouldn't have #1120 ready in time for us to implement this properly for the 3.0 release?

Yes, it will be ready by 3.0. I just filed #1173 for the additional glue code needed to support this after #1120 is merged.

@tsenart

This comment has been minimized.

Copy link
Contributor

tsenart commented Nov 29, 2018

That would not be hard to implement, so you can assume such a solution will exist soon. In the meantime, the extension you build would be opt in and we can manually explain the imprecision/limitations to the initial users.

Sounds good to me.

Yes, it will be ready by 3.0. I just filed #1173 for the additional glue code needed to support this after #1120 is merged.

Nice, thanks.

@felixfbecker

This comment has been minimized.

Copy link
Member

felixfbecker commented Nov 29, 2018

Today's search for good heuristics to map GraphQL symbols to their corresponding programming language counterparts has proved unfruitful due to the lack of standards around code generation.

Unlike Thrift or Protobuf, GraphQL doesn't have useful restrictions and conventions that can inform where to look for associated generated code. Without unifying universal conventions, this leaves the solution space for GraphQL code intelligence restricted to search based on configurable heuristics per project.

I don't understand why code generation is so important for this. Code generation (as in generation API client functions) in GraphQL is actually not common, best practice is to just define queries in strings in the GraphQL query language, because that is more flexible. The query language is well-known and universal in all languages so plain text search or parsing should be possible. For example, all queries start with query { or mutation { (with optional name and variables). This allows you to detect a query and then parse out the fields that are being queried to search for references or jump-to-definition to a specific field. In JS it's also convention to use the gql template string tag for GraphQL queries.

The only thing that this would not cover is full-blown data loading frameworks like Apollo or Relay. These could potentially get special support.

@nicksnyder

This comment has been minimized.

Copy link
Member

nicksnyder commented Nov 29, 2018

I agree with Felix about the fact that we should be able to have a fuzzy solution for graphql too. A not exact “Find references” via search is better than none at all (just think of it like a convenience function for doing a search).

I don’t object to doing Thrift first, but I do want to stress that we can be incremental here.

@tsenart

This comment has been minimized.

Copy link
Contributor

tsenart commented Nov 29, 2018

I don't understand why code generation is so important for this. Code generation (as in generation API client functions) in GraphQL is actually not common, best practice is to just define queries in strings in the GraphQL query language, because that is more flexible.

That flexibility is indeed what makes it harder to precisely find references (e.g. not a fuzzy search). As elaborated above, I was working under the assumption that imprecise results would break user expectations which led me to develop the code-generation scenario for those IDLs that universally support it first.

With @sqs's input regarding the utility of having reference results marked as imprecise from a UI perspective (#1174), one could think of ways to apply that to the GraphQL problem solved with fuzzy search, but I still can't quite see how. How do we determine if a search result is a false positive or a true positive? 🤔

I agree with Felix about the fact that we should be able to have a fuzzy solution for graphql too. A not exact “Find references” via search is better than none at all (just think of it like a convenience function for doing a search).

With #1174 in place that seems more approachable and when we get to do it sometime after Thrift perhaps @felixfbecker could help out. It seems he's got good ideas on how to make it work reasonably well that my relative lack of experience with GraphQL prevented me from seeing 😸.

I don’t object to doing Thrift first, but I do want to stress that we can be incremental here.

As requested by @sqs, I am writing down a mini design doc today that will elaborate the scope, constraints and pre-requisites that the first Thrift solution would work with. Feedback on that document regarding how incremental the plan of action is, as well as any other input, is much appreciated!

@tsenart

This comment has been minimized.

Copy link
Contributor

tsenart commented Dec 3, 2018

For Thrift code intelligence, let's continue in this focused sub-issue: #669

@nicksnyder nicksnyder modified the milestones: 3.0-preview.2, 3.0, Backlog Dec 14, 2018

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