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

Thrift code intelligence #669

Closed
sqs opened this issue Nov 3, 2018 · 7 comments
Closed

Thrift code intelligence #669

sqs opened this issue Nov 3, 2018 · 7 comments
Labels
feature-request team/graph Graph Team (previously Code Intel/Language Tools/Language Platform)
Milestone

Comments

@sqs
Copy link
Member

sqs commented Nov 3, 2018

Proposal

(By @tsenart)

This proposal describes a Sourcegraph extension that provides basic code intelligence for projects using the Thrift Interface Description Language (IDL) that check-in the code-generated servers and clients.

The Thrift project has extensive language bindings with which language specific client and server code is generated using the thrift CLI tool.

Every supported language has its code generated in a deterministic way by the thrift tool which allows us to find the generated code with a high degree of confidence using hierarchical search. Since namespaces and the resulting output package names and directories are configurable by Thrift users, the only heuristic we can rely on to find generated code files is to look for this always-present comment line in those files: Autogenerated by Thrift Compiler.

While this first search term filters results to Thrift generated files, we need another term to look for the specific symbol the user has searched for. Because Thrift's code generation is deterministic, we can use its pre-determined naming rules for each supported language bindings to find the correspondent definitions and their position. As it turns out, this is easy, because Thrift maintains the inflection of the defined names in the generated code.

As an example, this Thrift service has the following code generated in Go and Java:

Thrift

struct SumReply {
	1: i64 value
}

service AddService {
	SumReply Sum(1: i64 a, 2: i64 b)
}

Go

type AddService interface {
  Sum(ctx context.Context, a int64, b int64) (r *SumReply, err error)
}

type AddServiceClient struct {
  c thrift.TClient
}

Java

public class AddService {
  public interface Iface {
    public SumReply Sum(long a, long b) throws org.apache.thrift.TException;
  }

  public interface AsyncIface {
    public void Sum(long a, long b, org.apache.thrift.async.AsyncMethodCallback<SumReply> resultHandler) throws org.apache.thrift.TException;
  }

  public static class Client extends org.apache.thrift.TServiceClient implements Iface {
    // ...
  }

  public static class AsyncClient extends org.apache.thrift.async.TAsyncClient implements AsyncIface {
    // ...
  }

Other languages also yield precise search results with identical name inflection as the source .thrift file names.

Implementation

As a first milestone, it would be sufficiently useful to have a Find generated code button in the hover panel over a symbol in a .thrift file. As an example, if a user would Find generated code for the above AddService, we'd perform the following cross-repo hierarchical search:

("Autogenerated by Thrift Compiler") case:true AddService

In the search results page, matches would be ideally ordered with the following priority:

  1. Exact matches in the same repo
  2. Exact matches in other repos
  3. Other matches

After clicking on a file match of a language that has an enabled extension, users can use that extension's functionality to find cross-repo external references, go to definitions and look-up documentation.

Constraints

In summary, this proposal works under the given constraints:

  • Cross-repo search would only work on enterprise deployments.
  • Hierarchical search must be rolled-out already.
  • Client and server code generated with the thrift CLI tool must be checked-in to the source control management system.
  • Generated source code must not be modified manually.
@sqs sqs added feature-request team/graph Graph Team (previously Code Intel/Language Tools/Language Platform) labels Nov 3, 2018
@sqs sqs added this to the Backlog milestone Nov 3, 2018
@sqs
Copy link
Member Author

sqs commented Nov 27, 2018

Closing in favor of #981

@sqs sqs closed this as completed Nov 27, 2018
@tsenart tsenart changed the title Thrift cross-language definitions/references Thrift code intelligence Dec 3, 2018
@tsenart
Copy link
Contributor

tsenart commented Dec 3, 2018

Re-opening this as a Thrift focused child issue of #981. I'll add the design doc to the issue description and we can then discuss the proposal in the issue comments.

@tsenart
Copy link
Contributor

tsenart commented Dec 3, 2018

I have updated this issue's description with a first proposal for basic Thrift code intelligence. Before discussing implementation, I would like to gather as much feedback from potential users as possible on the proposed UX and user flow.

@tsenart tsenart self-assigned this Dec 4, 2018
@tsenart tsenart modified the milestones: Backlog, 3.0 Dec 4, 2018
@sqs sqs changed the title Thrift code intelligence Thrift "Find generated code" and "Find callers" Dec 4, 2018
@sqs sqs added the roadmap label Dec 4, 2018
@aisbaa
Copy link

aisbaa commented Dec 5, 2018

Will it be possible to jump to definition and jump back to all references as in:

  • jump from generated code to thrift?
  • from thrift back to generated code, either go or java as in example above?

P.S. I'm super excited about this feature!

@tsenart
Copy link
Contributor

tsenart commented Dec 5, 2018

Hi @aisbaa,

Will it be possible to jump to definition and jump back to all references as in:
jump from generated code to thrift?

Since you could have multiple identical .thrift files checked-in in different repos (or even in the same repo), we'd have to keep track of your jump history to do this in a flawless way. Without considering doing that, we have two options:

  1. You CMD+Click on Find generated code, which would open the search results in a new tab, effectively using browser tabs as a stack of jump history :-)
  2. We embrace the essential lack of 1-1 mapping between .thrift definitions and generated code and implement a symmetrical Find Thrift definitions which would also take us to the search results page, showing potential matches, to be filtered out further by a human.

from thrift back to generated code, either go or java as in example above?

That would be just the same user flow as clicking Find generated code the first time around.

P.S. I'm super excited about this feature!

Glad to hear! If you find some time, it'd be great to know in more detail how this feature would help you in your daily job. Detailed user feedback makes for better products!

@tsenart tsenart changed the title Thrift "Find generated code" and "Find callers" Thrift code intelligence Dec 6, 2018
@aisbaa
Copy link

aisbaa commented Dec 6, 2018

In deed 1-1 mapping sounds tricky - Find generated code and Find Thrift definitions seems to address my concerns, thank you.

@nicksnyder nicksnyder modified the milestones: 3.0-preview.2, 3.0, 3.1 Dec 14, 2018
@nicksnyder nicksnyder removed this from the 3.1 milestone Jan 11, 2019
@sqs sqs added this to the Backlog milestone Jan 30, 2019
@tsenart tsenart added team/core-services and removed team/graph Graph Team (previously Code Intel/Language Tools/Language Platform) feature-request roadmap labels Apr 17, 2019
@tsenart tsenart added team/graph Graph Team (previously Code Intel/Language Tools/Language Platform) feature-request roadmap labels Apr 17, 2019
@tsenart tsenart removed their assignment Apr 17, 2019
@aidaeology
Copy link
Contributor

Closing this issue as we are consolidating discussion in #981

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request team/graph Graph Team (previously Code Intel/Language Tools/Language Platform)
Projects
None yet
Development

No branches or pull requests

5 participants